Remove mojo dependency cycle from Blink platform |
||||
Issue descriptionToday, 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.
,
Sep 25 2017
,
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.)
,
Oct 1 2017
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.
,
Oct 2 2017
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/.)
,
Oct 2 2017
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.
,
Oct 2 2017
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.)
,
Oct 2 2017
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.
,
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.
,
Oct 17
|
||||
►
Sign in to add a comment |
||||
Comment 1 by dcheng@chromium.org
, Sep 25 2017