Proposal for the future dependency model in Blink (WAS: [blink-dev] Removing the WTF namespace)

150 views
Skip to first unread message

Kentaro Hara

unread,
May 27, 2015, 8:56:23 PM5/27/15
to blink-dev
Hi

Thanks all for a lot of valuable inputs in the original thread. Based on the inputs, I re-revised the proposal --- I think this is the least controversial proposal that solves the problems I want to solve by doing the componentization :)

The figure attached to this email illustrates the proposal by drawing what files are going to be moved to what directories. In short, I propose to change the dependency as follows:

(before) web/ => modules/ => core/ => platform/ => wtf/
(after) modules/ => core/ => platform/ => wtf/ => base/

- After merging the repository, wtf/ will depend on base/. It will enable us to remove duplicated code from wtf/ and port some of wtf/ things to base/. (In this proposal, I'm not intending to propose what needs to be moved from wtf/ to base/. Let's discuss this in another thread.)

- We will keep the WTF namespace as is.

- We will keep platform/. Currently platform/ is a mixed bag of various things, so we will move the following things from platform/ to other places, but we will keep other things (e.g., Timer, LifecycleObserver, networking, webaudio etc) in platform/ as is.

--- All WebXXX things in platform/exported/ will be moved to core/web/ or modules/web/ (that's the key to fix the broken dependency).
--- Oilpan and a couple of other fundamental classes will be moved to wtf/.

- All public APIs will be put in public/. Their implementations will be in either of core/web/, modules/web/ and content/.

- This componentization work will be mostly done in the Blink side. No substantial changes will be needed in the content/ side (except for rewriting #include paths of public APIs etc).


Feedback is welcome!


--
Kentaro Hara, Tokyo, Japan
before_after_with_files.png

Jochen Eisinger

unread,
May 28, 2015, 5:42:30 AM5/28/15
to Kentaro Hara, blink-dev
On Thu, May 28, 2015 at 2:56 AM Kentaro Hara <har...@chromium.org> wrote:
Hi

Thanks all for a lot of valuable inputs in the original thread. Based on the inputs, I re-revised the proposal --- I think this is the least controversial proposal that solves the problems I want to solve by doing the componentization :)

The figure attached to this email illustrates the proposal by drawing what files are going to be moved to what directories. In short, I propose to change the dependency as follows:

(before) web/ => modules/ => core/ => platform/ => wtf/
(after) modules/ => core/ => platform/ => wtf/ => base/


why does wtf have to depend on wtf? To remove duplication, it's enough if platform (continues to) depend on base (and we'll actually allow using it), no?
 

- After merging the repository, wtf/ will depend on base/. It will enable us to remove duplicated code from wtf/ and port some of wtf/ things to base/. (In this proposal, I'm not intending to propose what needs to be moved from wtf/ to base/. Let's discuss this in another thread.)

- We will keep the WTF namespace as is.

- We will keep platform/. Currently platform/ is a mixed bag of various things, so we will move the following things from platform/ to other places, but we will keep other things (e.g., Timer, LifecycleObserver, networking, webaudio etc) in platform/ as is.

--- All WebXXX things in platform/exported/ will be moved to core/web/ or modules/web/ (that's the key to fix the broken dependency).

that includes all the network things, no? Weren't they supposed to stay in platform?

If I understand this correctly, this is the main motivation for this change? Can you explain a bit more how the current model is broken?
 
--- Oilpan and a couple of other fundamental classes will be moved to wtf/.

Doesn't oilpan know about core types and v8? Will wtf depend on v8 then? 

Kentaro Hara

unread,
May 28, 2015, 8:02:18 AM5/28/15
to Jochen Eisinger, blink-dev
On Thu, May 28, 2015 at 6:42 PM, Jochen Eisinger <joc...@chromium.org> wrote:


On Thu, May 28, 2015 at 2:56 AM Kentaro Hara <har...@chromium.org> wrote:
Hi

Thanks all for a lot of valuable inputs in the original thread. Based on the inputs, I re-revised the proposal --- I think this is the least controversial proposal that solves the problems I want to solve by doing the componentization :)

The figure attached to this email illustrates the proposal by drawing what files are going to be moved to what directories. In short, I propose to change the dependency as follows:

(before) web/ => modules/ => core/ => platform/ => wtf/
(after) modules/ => core/ => platform/ => wtf/ => base/


why does wtf have to depend on wtf? To remove duplication, it's enough if platform (continues to) depend on base (and we'll actually allow using it), no?

We can disallow the wtf/ => base/ dependency, but I don't see much benefit in disallowing the dependency. So I think it's better to just make wtf/ depend on base/. Then we will have a world where:

  base/: A standard library of Chromium (including Blink).
  wtf/: A standard library specific to Blink. wtf/ can use things in base/.

 
 
- After merging the repository, wtf/ will depend on base/. It will enable us to remove duplicated code from wtf/ and port some of wtf/ things to base/. (In this proposal, I'm not intending to propose what needs to be moved from wtf/ to base/. Let's discuss this in another thread.)

- We will keep the WTF namespace as is.

- We will keep platform/. Currently platform/ is a mixed bag of various things, so we will move the following things from platform/ to other places, but we will keep other things (e.g., Timer, LifecycleObserver, networking, webaudio etc) in platform/ as is.

--- All WebXXX things in platform/exported/ will be moved to core/web/ or modules/web/ (that's the key to fix the broken dependency).

that includes all the network things, no? Weren't they supposed to stay in platform?

network is not WebXXX things. (All WebXXX things in platform/ are in platform/exported/.) So platform/network/ will stay in platform/.

 
If I understand this correctly, this is the main motivation for this change? Can you explain a bit more how the current model is broken? 

Hard to explain in a couple of words :) Please take a look at the "current architecture" section of the design doc (https://docs.google.com/document/d/1vtpoNdywErT2wtbE6a64scePBnZycuRhkbuuaIpiu2I/edit).

 
--- Oilpan and a couple of other fundamental classes will be moved to wtf/.

Doesn't oilpan know about core types and v8? Will wtf depend on v8 then? 


Conceptually Oilpan shouldn't know core types (although it might be broken in some places). Oilpan now knows v8, but I think the dependency should be factored out. I agree with you that it's not a good idea to make wtf/ depend on v8.


 
- All public APIs will be put in public/. Their implementations will be in either of core/web/, modules/web/ and content/.

- This componentization work will be mostly done in the Blink side. No substantial changes will be needed in the content/ side (except for rewriting #include paths of public APIs etc).


Feedback is welcome!


--
Kentaro Hara, Tokyo, Japan

Jochen Eisinger

unread,
May 28, 2015, 8:15:28 AM5/28/15
to Kentaro Hara, blink-dev
On Thu, May 28, 2015 at 2:02 PM Kentaro Hara <har...@chromium.org> wrote:
On Thu, May 28, 2015 at 6:42 PM, Jochen Eisinger <joc...@chromium.org> wrote:


On Thu, May 28, 2015 at 2:56 AM Kentaro Hara <har...@chromium.org> wrote:
Hi

Thanks all for a lot of valuable inputs in the original thread. Based on the inputs, I re-revised the proposal --- I think this is the least controversial proposal that solves the problems I want to solve by doing the componentization :)

The figure attached to this email illustrates the proposal by drawing what files are going to be moved to what directories. In short, I propose to change the dependency as follows:

(before) web/ => modules/ => core/ => platform/ => wtf/
(after) modules/ => core/ => platform/ => wtf/ => base/


why does wtf have to depend on wtf? To remove duplication, it's enough if platform (continues to) depend on base (and we'll actually allow using it), no?

We can disallow the wtf/ => base/ dependency, but I don't see much benefit in disallowing the dependency. So I think it's better to just make wtf/ depend on base/. Then we will have a world where:

  base/: A standard library of Chromium (including Blink).
  wtf/: A standard library specific to Blink. wtf/ can use things in base/.

 
 
- After merging the repository, wtf/ will depend on base/. It will enable us to remove duplicated code from wtf/ and port some of wtf/ things to base/. (In this proposal, I'm not intending to propose what needs to be moved from wtf/ to base/. Let's discuss this in another thread.)

- We will keep the WTF namespace as is.

- We will keep platform/. Currently platform/ is a mixed bag of various things, so we will move the following things from platform/ to other places, but we will keep other things (e.g., Timer, LifecycleObserver, networking, webaudio etc) in platform/ as is.

--- All WebXXX things in platform/exported/ will be moved to core/web/ or modules/web/ (that's the key to fix the broken dependency).

that includes all the network things, no? Weren't they supposed to stay in platform?

network is not WebXXX things. (All WebXXX things in platform/ are in platform/exported/.) So platform/network/ will stay in platform/.

There's platform/exported/WebURL{Request,Response}.cpp for example 

Kentaro Hara

unread,
May 28, 2015, 8:51:48 AM5/28/15
to Jochen Eisinger, blink-dev
- We will keep platform/. Currently platform/ is a mixed bag of various things, so we will move the following things from platform/ to other places, but we will keep other things (e.g., Timer, LifecycleObserver, networking, webaudio etc) in platform/ as is.

--- All WebXXX things in platform/exported/ will be moved to core/web/ or modules/web/ (that's the key to fix the broken dependency).

that includes all the network things, no? Weren't they supposed to stay in platform?

network is not WebXXX things. (All WebXXX things in platform/ are in platform/exported/.) So platform/network/ will stay in platform/.

> There's platform/exported/WebURL{Request,Response}.cpp for example 

Thanks, I understand your point.

platform/network/Resource{Request,Response} depends on platform/exported/WebURL{Request,Response}.cpp. So if we move WebURL{Request,Response} to core/web/, we need to move Resource{Request,Response} to core/, which may bring more things in platform/network/ to core/.

(In the original proposal, I was planning to remove platform/ completely. In that proposal, platform/network/ was going to be moved to core/, of course.)

I think the same discussion will hold for platform/audio/ etc. If we move platform/exported/WebAudio* to modules/web/, then we need to move platform/audio/ to modules/webaudio/ accordingly.

So, in the proposal, platform/ cannot contain classes that depend on WebXXX.

Kentaro Hara

unread,
May 28, 2015, 10:53:49 AM5/28/15
to Jochen Eisinger, blink-dev
Maybe I can rephase the problem as follows:

The more layered link units we have, the more abstraction classes we need to introduce. We can remove the abstraction classes and simplify the code base by reducing the layered link units, but it will break the layering. It is important to find a good balancing point.

The current architecture is:

  web/ => modules/ => core/ => platform/ => wtf/

The simplest architecture would be:

  wtf/ (i.e., Put everything in wtf/, then we can remove all abstraction classes!)

The balancing point should be somewhere between the current one and the simplest one. As far as I understand the current architecture, the largest pain point that results in a lot of abstraction classes is the fact that WebXXX is located in web/ and platform/. It often happens that core/ wants to use web/WebXXX, but it can't. It often happens that platform/WebXXX wants to use core/, but it can't. Then you need to introduce abstraction classes to bypass the disallowed dependency (or even uglier hacks in some cases).

By putting WebXXX into core/web/ and modules/web/ solves the problem. As a result, we will have:

  modules/ (including modules/web/) => core/ (including core/web/) => platform/ => wtf/

As Jochen pointed out, moving platform/WebXXX to core/web/ will bring a bunch of platform/ things to core/ as well. This will make the layering somewhat worse, but that is a trade-off with the advantages we can get by removing the abstraction classes & hacks. I think that this is a good balancing point.

(One thing I'm not yet sure is whether we should keep platform/ or not. Given that platform/ things that depend on WebXXX are moved to core/web/ or modules/web/, platform/ will become thin. Maybe it makes sense to remove platform/ by moving the contents to core/ or wtf/. I think we can revisit this issue after finishing the key parts of the componentization.)








Jochen Eisinger

unread,
May 28, 2015, 11:03:40 AM5/28/15
to Kentaro Hara, blink-dev
After reading the document again, I wonder whether we just need some form of blink::Platform() for web/, maybe blink::ChromePlatform() to provide the clipboard (for your first example)?

I'm not convinced that the refactoring you propose will improve the situation. We'll still keep dependencies from core to modules for example (for mediastream, yes, yes, that's the only example I know of, but I like it).

moving network into core is IMO a considerable step backwards.

The refactoring you propose looks like a large amount of work - are there other benefits than a layering that has a better set of trade-offs?

Kinuko Yasuda

unread,
May 28, 2015, 11:07:19 AM5/28/15
to Kentaro Hara, Jochen Eisinger, blink-dev
Thanks for summarizing the proposal.  I was writing a response to previous email, but let me reply to this one.

On Thu, May 28, 2015 at 11:53 PM, Kentaro Hara <har...@chromium.org> wrote:
Maybe I can rephase the problem as follows:

The more layered link units we have, the more abstraction classes we need to introduce. We can remove the abstraction classes and simplify the code base by reducing the layered link units, but it will break the layering. It is important to find a good balancing point.

The current architecture is:

  web/ => modules/ => core/ => platform/ => wtf/

The simplest architecture would be:

  wtf/ (i.e., Put everything in wtf/, then we can remove all abstraction classes!)

The balancing point should be somewhere between the current one and the simplest one. As far as I understand the current architecture, the largest pain point that results in a lot of abstraction classes is the fact that WebXXX is located in web/ and platform/. It often happens that core/ wants to use web/WebXXX, but it can't. It often happens that platform/WebXXX wants to use core/, but it can't. Then you need to introduce abstraction classes to bypass the disallowed dependency (or even uglier hacks in some cases).

By putting WebXXX into core/web/ and modules/web/ solves the problem. As a result, we will have:

  modules/ (including modules/web/) => core/ (including core/web/) => platform/ => wtf/

As Jochen pointed out, moving platform/WebXXX to core/web/ will bring a bunch of platform/ things to core/ as well. This will make the layering somewhat worse, but that is a trade-off with the advantages we can get by removing the abstraction classes & hacks. I think that this is a good balancing point.

(One thing I'm not yet sure is whether we should keep platform/ or not. Given that platform/ things that depend on WebXXX are moved to core/web/ or modules/web/, platform/ will become thin. Maybe it makes sense to remove platform/ by moving the contents to core/ or wtf/. I think we can revisit this issue after finishing the key parts of the componentization.)

I'm feeling the same concern.  Conceptually I agree that platform/ and core/ are different layer, and that's why we used to have the layering as is today, but it seems to conflict with what the original proposal tried to address. It feels introduction of platform/ is making the proposal a bit unclear.

 
On Thu, May 28, 2015 at 9:51 PM, Kentaro Hara <har...@chromium.org> wrote:
- We will keep platform/. Currently platform/ is a mixed bag of various things, so we will move the following things from platform/ to other places, but we will keep other things (e.g., Timer, LifecycleObserver, networking, webaudio etc) in platform/ as is.

--- All WebXXX things in platform/exported/ will be moved to core/web/ or modules/web/ (that's the key to fix the broken dependency).

that includes all the network things, no? Weren't they supposed to stay in platform?

network is not WebXXX things. (All WebXXX things in platform/ are in platform/exported/.) So platform/network/ will stay in platform/.

> There's platform/exported/WebURL{Request,Response}.cpp for example 

Thanks, I understand your point.

platform/network/Resource{Request,Response} depends on platform/exported/WebURL{Request,Response}.cpp. So if we move WebURL{Request,Response} to core/web/, we need to move Resource{Request,Response} to core/, which may bring more things in platform/network/ to core/.

I just did quick grep, looks like FormData, FormDataBuilder and NetworkHints will need to be moved to core/ too.  I assume WebSocketHandshake{Request,Response} are also moved to module/websocket.  It's not in the network/ directory but MIMETypeRegistry and PlatformResourceLoader also need to be moved to core/.

For the same reason many of platform/graphics, platform/scroll, platform/image-decoders will need to be moved to core/.

And thread is also not platform but core, because it's implemented by content/.

So... at this point I feel it's getting blur what 'platform' means here?

Kentaro Hara

unread,
May 28, 2015, 11:22:27 AM5/28/15
to Jochen Eisinger, blink-dev
After reading the document again, I wonder whether we just need some form of blink::Platform() for web/, maybe blink::ChromePlatform() to provide the clipboard (for your first example)?

So your suggestion is:

- When platform/WebXXX wants to use core/, use it via blink::ChromePlatform().
- When core/ wants to use web/WebXXX, use it via blink::Platform(). (I think ChromeClient is something like that.)

I cannot immediately imagine how it will solve the complexity of WebIDB, ServiceWorker etc. Let me grep the code base tomorrow.


The refactoring you propose looks like a large amount of work - are there other benefits than a layering that has a better set of trade-offs?

The benefits are 1) removing abstraction classes & hacks and 2) making Chromium-Blink interactions faster (e.g., We don't need to deep-copy XXX to WebXXX. See the WebDragData & DataObject example in the document. WebIDB circumvents the copying overhead by using inlining hacks).

Maybe I can try to describe how WebIDB or ServiceWorker works and how the architectural problems are going to be solved.


I'm not convinced that the refactoring you propose will improve the situation. We'll still keep dependencies from core to modules for example (for mediastream, yes, yes, that's the only example I know of, but I like it).

I'd argue that the dependency issues between core/ and modules/ are relatively limited, compared to the issues around web/ and platform/. Most of the back dependency issues from core/ to modules/ can be solved in the binding layer and we actually did it.

Kentaro Hara

unread,
May 28, 2015, 11:24:42 AM5/28/15
to Kinuko Yasuda, Jochen Eisinger, blink-dev
> (One thing I'm not yet sure is whether we should keep platform/ or not. Given that platform/ things that depend on WebXXX are moved to core/web/ or modules/web/, platform/ will become thin. Maybe it makes sense to remove platform/ by moving the contents to core/ or wtf/. I think we can revisit this issue after finishing the key parts of the componentization.)

I'm feeling the same concern.  Conceptually I agree that platform/ and core/ are different layer, and that's why we used to have the layering as is today, but it seems to conflict with what the original proposal tried to address. It feels introduction of platform/ is making the proposal a bit unclear.

Yeah, I begin to feel the same thing. Introduction of platform/ makes the proposal unclear. So getting back to the original proposal:

  modules/ (including modules/web/) => core/ (including core/web/) => wtf/

platform/ things will go to core/ or modules/ or wtf/.

Kentaro Hara

unread,
May 28, 2015, 12:00:04 PM5/28/15
to Kinuko Yasuda, Jochen Eisinger, blink-dev
> After reading the document again, I wonder whether we just need some form of blink::Platform() for web/, maybe blink::ChromePlatform() to provide the clipboard (for your first example)?

So your suggestion is:
- When platform/WebXXX wants to use core/, use it via blink::ChromePlatform().
- When core/ wants to use web/WebXXX, use it via blink::Platform(). (I think ChromeClient is something like that.)

That will solve the dependency problem, but it seems that we're just adding another way to bypass disallowed dependencies.

Overall, I believe there is no doubt about the simplicity of the proposed dependency model (i.e., modules/ => core/ => wtf/). The key would be trade-offs between the advantages and the disadvantages. What are disadvantages of losing platform/ and web/?

Kenichi Ishibashi

unread,
May 29, 2015, 12:02:48 AM5/29/15
to Kentaro Hara, Kinuko Yasuda, Jochen Eisinger, blink-dev
Moving network into core is unfortunate, but I generally support haraken's proposal given that:
- Disallowing web/ => core/ is the cause of complexity (e.g. DragClient and WebAutofillClient). We want to fix this.
- Moving web/ and platform/WebXXX => core/web sounds a reasonable solution to me.
- Partially moving some files from web/ (and platform/) into core/ would make the situation even worse.

Just my two cents.

Yury Semikhatsky

unread,
May 29, 2015, 5:59:11 AM5/29/15
to blink-dev, Kentaro Hara
On Thu, May 28, 2015 at 3:56 PM, Kentaro Hara <har...@chromium.org> wrote:
Is it possible to factor out Oilpan support into a separate component? wtf/ doesn't have to depend on managed heap. Classes that depend on Oilpan could explicitly import corresponding library.

In the good old days, we were doing that (i.e., Source/heap/). However, it turned out that it's hard to keep it in a separate component, so we moved it to platform/heap/ (Oilpan uses several platform/ things such as TraceEvent, WebThread, MemoryDumper etc). Now it is getting hard to keep it in platform/ because a bunch of wtf/ collections are implemented by Oilpan. In total, tkent and I are thinking that it would be better to move Oilpan to wtf/.

You can find a lot of WTF things in Oilpan. We need to ask compilers to inline those WTF things into wtf.dll to overcome the disallowed dependency.


What is the disallowed dependency here, can you elaborate? I don't see a problem in having the following dependency core => oilpan => wtf. The advantage of such layering is that non-oilpan code can import only wtf without oilpan machinery. Moreover, oilpan library can depend on both wtf and platform if needed.
 

Do you have any concern about putting Oilpan in wtf/?

WTF like STL is a generic library that can be used by various components, not all of which are necessarily use Oillpan for memory management. If we were using STL instead of WTF in Blink there would be specializations of STL classes that would make them suitable for Oilpan code. Oilpan implementation would stay separate from STL, it would depend on STL but not vise versa. I feel like it should be the same for WTF.

 

On Thu, May 28, 2015 at 9:45 PM, Yury Semikhatsky <yu...@chromium.org> wrote:


On Thu, May 28, 2015 at 3:01 PM, Kentaro Hara <har...@chromium.org> wrote:


On Thu, May 28, 2015 at 6:42 PM, Jochen Eisinger <joc...@chromium.org> wrote:


On Thu, May 28, 2015 at 2:56 AM Kentaro Hara <har...@chromium.org> wrote:
Hi

Thanks all for a lot of valuable inputs in the original thread. Based on the inputs, I re-revised the proposal --- I think this is the least controversial proposal that solves the problems I want to solve by doing the componentization :)

The figure attached to this email illustrates the proposal by drawing what files are going to be moved to what directories. In short, I propose to change the dependency as follows:

(before) web/ => modules/ => core/ => platform/ => wtf/
(after) modules/ => core/ => platform/ => wtf/ => base/


why does wtf have to depend on wtf? To remove duplication, it's enough if platform (continues to) depend on base (and we'll actually allow using it), no?

We can disallow the wtf/ => base/ dependency, but I don't see much benefit in disallowing the dependency. So I think it's better to just make wtf/ depend on base/. Then we will have a world where:

  base/: A standard library of Chromium (including Blink).
  wtf/: A standard library specific to Blink. wtf/ can use things in base/.

 
 
- After merging the repository, wtf/ will depend on base/. It will enable us to remove duplicated code from wtf/ and port some of wtf/ things to base/. (In this proposal, I'm not intending to propose what needs to be moved from wtf/ to base/. Let's discuss this in another thread.)

- We will keep the WTF namespace as is.

- We will keep platform/. Currently platform/ is a mixed bag of various things, so we will move the following things from platform/ to other places, but we will keep other things (e.g., Timer, LifecycleObserver, networking, webaudio etc) in platform/ as is.

--- All WebXXX things in platform/exported/ will be moved to core/web/ or modules/web/ (that's the key to fix the broken dependency).

that includes all the network things, no? Weren't they supposed to stay in platform?

network is not WebXXX things. (All WebXXX things in platform/ are in platform/exported/.) So platform/network/ will stay in platform/.

 
If I understand this correctly, this is the main motivation for this change? Can you explain a bit more how the current model is broken? 

Hard to explain in a couple of words :) Please take a look at the "current architecture" section of the design doc (https://docs.google.com/document/d/1vtpoNdywErT2wtbE6a64scePBnZycuRhkbuuaIpiu2I/edit).

 
--- Oilpan and a couple of other fundamental classes will be moved to wtf/.

Doesn't oilpan know about core types and v8? Will wtf depend on v8 then? 


Conceptually Oilpan shouldn't know core types (although it might be broken in some places). Oilpan now knows v8, but I think the dependency should be factored out. I agree with you that it's not a good idea to make wtf/ depend on v8.


Is it possible to factor out Oilpan support into a separate component? wtf/ doesn't have to depend on managed heap. Classes that depend on Oilpan could explicitly import corresponding library.

 

 
- All public APIs will be put in public/. Their implementations will be in either of core/web/, modules/web/ and content/.

- This componentization work will be mostly done in the Blink side. No substantial changes will be needed in the content/ side (except for rewriting #include paths of public APIs etc).


Feedback is welcome!


--
Kentaro Hara, Tokyo, Japan



--
Kentaro Hara, Tokyo, Japan

Kentaro Hara

unread,
May 29, 2015, 6:26:58 AM5/29/15
to Yury Semikhatsky, blink-dev
On Fri, May 29, 2015 at 6:59 PM, Yury Semikhatsky <yu...@chromium.org> wrote:
On Thu, May 28, 2015 at 3:56 PM, Kentaro Hara <har...@chromium.org> wrote:
Is it possible to factor out Oilpan support into a separate component? wtf/ doesn't have to depend on managed heap. Classes that depend on Oilpan could explicitly import corresponding library.

In the good old days, we were doing that (i.e., Source/heap/). However, it turned out that it's hard to keep it in a separate component, so we moved it to platform/heap/ (Oilpan uses several platform/ things such as TraceEvent, WebThread, MemoryDumper etc). Now it is getting hard to keep it in platform/ because a bunch of wtf/ collections are implemented by Oilpan. In total, tkent and I are thinking that it would be better to move Oilpan to wtf/.

You can find a lot of WTF things in Oilpan. We need to ask compilers to inline those WTF things into wtf.dll to overcome the disallowed dependency.


What is the disallowed dependency here, can you elaborate? I don't see a problem in having the following dependency core => oilpan => wtf. The advantage of such layering is that non-oilpan code can import only wtf without oilpan machinery. Moreover, oilpan library can depend on both wtf and platform if needed.

wtf/ => platform/ is disallowed. This causes a problem because wtf/Vector, wtf/HashTable etc use Oilpan. We currently bypass the disallowed dependency with inlining hacks.

Another example would be this one:

 

Do you have any concern about putting Oilpan in wtf/?

WTF like STL is a generic library that can be used by various components, not all of which are necessarily use Oillpan for memory management. If we were using STL instead of WTF in Blink there would be specializations of STL classes that would make them suitable for Oilpan code. Oilpan implementation would stay separate from STL, it would depend on STL but not vise versa. I feel like it should be the same for WTF.

We can do that, but I'd argue that Oilpan is now a part of wtf/. (c.f., PartitionAlloc is in wtf/.)

As you point out, there is a way to implement Oilpan outside wtf/ (in theory, there is a way to implement anything in any dependency). However, Oilpan and wtf/ are tightly coupled. So if we can put Oilpan in wtf/, it will make a lot of things easier. It would mean that it's better to put Oilpan in wtf/.

Philip Jägenstedt

unread,
May 29, 2015, 1:18:12 PM5/29/15
to Kentaro Hara, Jochen Eisinger, blink-dev
On Thu, May 28, 2015 at 5:21 PM, Kentaro Hara <har...@chromium.org> wrote:
>> I'm not convinced that the refactoring you propose will improve the
>> situation. We'll still keep dependencies from core to modules for example
>> (for mediastream, yes, yes, that's the only example I know of, but I like
>> it).
>
>
> I'd argue that the dependency issues between core/ and modules/ are
> relatively limited, compared to the issues around web/ and platform/. Most
> of the back dependency issues from core/ to modules/ can be solved in the
> binding layer and we actually did it.

The HTMLMediaElement.srcObject dependency on MediaStream and
MediaSource remains unresolved, however. Is this a particular case
that you think can be solved in bindings? The dependency from core/ to
modules/ would be very weak here, essentially core/ just needs to be
allowed to use a forward-declared MediaSource* pointer and use that to
create a special kind of WebMediaPlayer.

Philip

Kentaro Hara

unread,
May 29, 2015, 8:43:40 PM5/29/15
to Philip Jägenstedt, Jochen Eisinger, blink-dev
I hope we can solve it like this:

- The hardest part is that the IDL compiler needs to generate code for "MediaStream? HTMLMediaElement.srcObject" so that the generated code (which is located at core/) doesn't depend on modules/ (where MediaStream is located). This is solvable in the binding layer.

- Regarding the creation of WebMediaPlayer, we can use a proxy class. We're already using this technique to create AXObjects (which are located at modules/) from Document (which is located at core/). https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/dom/AXObjectCache.h&q=axobjectcache&sq=package:chromium&type=cs&l=134

(I apologize that I objected to using a proxy class when we were discussing a way to implement HTMLMediaElement.srcObject a while back. After understanding the dependency problems, I realized that a proxy class would be the most straightforward way to bypass disallowed dependencies.)

Philip Jägenstedt

unread,
Jun 1, 2015, 10:20:24 AM6/1/15
to Kentaro Hara, Jochen Eisinger, blink-dev
Is there an existing bit of code following this model? If it's
something that can be done today, I'm sure the WebRTC team would be
interested. Also note that srcObject is a `typedef (MediaStream or
MediaSource or Blob) MediaProvider`, but I hope that doesn't
complicate anything.

Kentaro Hara

unread,
Jun 1, 2015, 10:28:14 AM6/1/15
to Philip Jägenstedt, Kenichi Ishibashi (via Google Docs), Takashi Sakamoto, Jochen Eisinger, blink-dev
On Mon, Jun 1, 2015 at 11:20 PM, Philip Jägenstedt <phi...@opera.com> wrote:
Is there an existing bit of code following this model? If it's
something that can be done today, I'm sure the WebRTC team would be
interested. Also note that srcObject is a `typedef (MediaStream or
MediaSource or Blob) MediaProvider`, but I hope that doesn't
complicate anything.

+tasak +bashi

MediaStream and MediaSource are in modules/. Blob is in core/. Does the IDL compiler support a union type that mixes core/ types and modules/ types?

Kenichi Ishibashi

unread,
Jun 1, 2015, 9:52:46 PM6/1/15
to Kentaro Hara, Philip Jägenstedt, Takashi Sakamoto, Jochen Eisinger, blink-dev
On Mon, Jun 1, 2015 at 11:27 PM, Kentaro Hara <har...@chromium.org> wrote:
On Mon, Jun 1, 2015 at 11:20 PM, Philip Jägenstedt <phi...@opera.com> wrote:
Is there an existing bit of code following this model? If it's
something that can be done today, I'm sure the WebRTC team would be
interested. Also note that srcObject is a `typedef (MediaStream or
MediaSource or Blob) MediaProvider`, but I hope that doesn't
complicate anything.

+tasak +bashi

MediaStream and MediaSource are in modules/. Blob is in core/. Does the IDL compiler support a union type that mixes core/ types and modules/ types?
In this particular case, no. We can have a union type which has types from core/ and modules/ but we can only use the union from modules/. It seems that we need to use MediaProvider from core/. This problem might be solvable in the binding layer, but I don't think it's a straightforward.
Reply all
Reply to author
Forward
0 new messages