New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 768298 link

Starred by 3 users

Issue metadata

Status: Started
Owner:
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 2
Type: Bug



Sign in to add a comment

Remove mojo dependency cycle from Blink platform

Project Member Reported by dcheng@chromium.org, Sep 25 2017

Issue description

Today, we only partially express the dependencies for Blink variants of things like KURL. If we try to add it in, then GN complains:

  //third_party/WebKit/Source/platform:platform ->
  //storage/public/interfaces:interfaces_blink ->
  //url/mojo:url_mojom_gurl_blink ->
  //third_party/WebKit/Source/platform:platform

The reason we don't need it today is because all the code for KURL's struct traits live in the header. However, best practice is usually to out-of-line larger methods (such as Read), and we can't do that without expressing the dependency explicitly.

We should split the things that use Mojo off into separate components, so the layering looks like:

  //third_party/WebKit/Source/platform/blob:blob ->
  //storage/public/interfaces:interfaces_blink ->
  //url/mojo:url_mojom_gurl_blink ->
  //third_party/WebKit/Source/platform:platform

And isn't a cycle anymore.
 

Comment 1 by dcheng@chromium.org, Sep 25 2017

However, there are a lot of interdependencies in code in platform... so I'm not sure how this is going to be split out cleanly. =/

Another option might be to try to split out the core types (wtf + KURL + WebString.cpp + runtime enabled features) and just give up on splitting up the platform target...

Comment 2 by jam@chromium.org, Sep 25 2017

Cc: roc...@chromium.org yzshen@chromium.org

Comment 3 by yzshen@chromium.org, Sep 25 2017

One possible alternative to solve the problem described in comment#0 is to make //url/mojo/url_mojom_gurl_blink part of the platform component:

Specify the following for //url/mojo/url_mojom_gurl:
  export_class_attribute_blink = "BLINK_PLATFORM_EXPORT"
  export_define_blink = "BLINK_PLATFORM_IMPLEMENTATION=1"
  export_header_blink = "third_party/WebKit/public/platform/WebCommon.h"

And make sure no one depends on //url/mojo/url_mojom_gurl_blink directly except platform itself.
For mojom targets, you will need to use something like the following:
  overridden_deps_blink = [ "//url/mojo/url_mojom_gurl_blink" ]
  component_deps_blink = [ "//third_party/WebKit/Source/platform" ]

And with these you should be able put traits code into .cc files.

(Not saying that this is a better approach, and I haven't tried it out myself.)


Labels: -Pri-3 OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows Pri-2
I did find an alternate solution: rather than placing Blink's struct trait .cc files in the typemap's sources list, I just placed them directly in the sources list for the platform target.

The current issues with dependencies and components builds are probably still worth looking into generically: the complexities here are difficult to understand, especially when Blink depends on Blink variants of non-Blink mojo interfaces (https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/nDyAeWQFdP0 is another recent example). But I need to do a bit more investigation on my side first.
Thanks Daniel for looking into this!

(Just a general comment: In my mental model, platform/ is a library specific to core/ and modules/. Any chromium-side code should not depend on platform/. From that perspective, I'd personally prefer somehow preventing Mojo from depending on platform/.)

Unfortunately, it's not easy to remove that dependency edge: it's due to the fact that we generate Blink variants of non-Blink interfaces. The non-Blink interfaces, by convention, live outside Blink. However, the Blink variant also requires that url.mojom.Url, string, and url.mojom.Origin be backed by KURL, WTF::String, and SecurityOrigin, respectively. This means that any Mojo interface that uses strings, origins, or urls will end up implicitly depending on platform.

While this dependency isn't as problematic conceptually (it makes sense that the Blink variant of Mojo interfaces, used only within Blink, depend on Blink platform types), it creates some issues with component builds and figuring out the correct dependencies.
What's making it hard to put the Blink variant in the platform's link unit? The platform's link unit should be able to depend on non-Blink interfaces, right? (I'm just trying to understand the problem.)

Suppose Blink platform uses //storage/public/interfaces/blobs.mojom.

The Blink variant is something like //storage/public/interfaces/interfaces_blink. This target is defined outside Blink but needs to depend on Blink platform (due to the usage of KURL, et cetera). This is true even when we link the Blink variant as part of platform, since the deps problem is at the GN layer where we define targets.

There are a couple of other issues as well, but I need to do a bit more investigation to make sure I capture the details correctly.

Comment 9 by mek@chromium.org, Oct 2 2017

Btw, to make things more complicated I have some WIP CLs where I'm introducing typemaps that depend on core/ as well (typemapping something to SerializedScriptValue), which makes things even more complicated I imagine (since then we'll have some mojom's where the blink variant will also depend on core, not just platform. At least I don't (currently) expect and mojom files outside of blink to use these types, so maybe it isn't quite as bad of a situation.
Cc: -roc...@chromium.org rockot@google.com

Sign in to add a comment