Bug 74583 – [chromium] Move WebMimeRegistry and dependencies to Source/Platform

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Demonstrate one possible way to move chromium WebKit platform API into Source/Platform

[James Robinson]

Created attachment 119368 [details]
Patch

[James Robinson]

Created attachment 119505 [details]
Patch

[James Robinson]

Ignore the patch for now - it's not fully baked.

A key feature of splitting the WebKit API up into client and platform APIs is the ability for WebCore to depend directly on the platform API instead of having to go through an extra layer of indirection. This is a strawman of one way to do this.

Terminology note: I'm calling interfaces like WebWidget that are used by the embedder to talk to WebKit the WebKit client API. I'm calling interfaces like WebMimeRegistry (or someday WebLayer) that WebKit uses to talk to the embedder the WebKit platform API, and types that are used by both APIs, like WebString and WebSize, WebKit common API.

Proposal:

*) leave all APIs - client, platform, and common - in the WebKit namespace
*) leave existing WebKit client API in place, Source/WebKit/chromium/public/
*) move platform API headers to Source/Platform/chromium/public
*) move common API headers to Source/Platform/chromium/common. provide forwarding headers in Source/WebKit/chromium/public
*) move implementations for common API, like WebString.cpp, into Source/Platform/chromium/src

Platform would be a new library. Today, it would probably have to link into libwebkit.so|dll since WebCString.o depends on WebCore's text encoding stuff. In the future, it could be a separate library.

One oddness here is that with the platform and client APIs in the same namespace we'll have some WebCore code depending on things in the WebKit namespace. This typically indicates a problem. I think that we can control this by careful with include paths so that code in WebCore can only include files in WebKit that are part of the WebKit platform or common APIs. This would be restricted to chromium-specific platform code.

Similarly, in chromium we need to ensure that implementations of the WebKit platform API do not depend on anything in the WebKit client API as that would introduce a nasty dependency cycle. I believe we can keep this in check (no pun intended) by careful checkdeps rules. This may get tricky with things like ui/ that suck in the entire world, but this situation is no worse than today.

One idea we've thought about is putting the WebKit platform API into the Platform namespace. This has several problems:
1.) Runs the risk of colliding with cross-platform interfaces that may enter the Platform namespace as code from WebCore/platform migrates over.
2.) There's no good way to deal with the WebKit common API types. If they don't move, then the platform API is a mix of WebKit:: and Platform::. If they do move, then the WebKit client API is a mix of WebKit:: and Platform::. If we put them in a different namespace then both APIs are a mess. There's no good way to alias the types into both namespaces in C++ that I'm aware of.
3.) Platform::WebFoo - really?

[James Robinson]

Created attachment 119527 [details]
Patch

[James Robinson]

This patch sets up a Platform.gyp target and moves 2 common APIs over (WebString and WebCString) and one pure-platform API (WebMimeRegistry).  DumpRenderTree and webkit_unit_tests compile and link for me in release and debug+components. Targets in chrome generally don't, I think mostly due to various targets in chrome depending indirectly on the WebKit API but not declaring that dependency explicitly so they don't pick up the include dirs needed to find the new Platform headers. I believe that these are all bugs in the chromium side's dependency listing and not a flaw of this approach.

WDYT?

[Darin Fisher (:fishd, Google)]

(In reply to comment #3)
> *) leave all APIs - client, platform, and common - in the WebKit namespace
> *) leave existing WebKit client API in place, Source/WebKit/chromium/public/
> *) move platform API headers to Source/Platform/chromium/public
> *) move common API headers to Source/Platform/chromium/common. provide forwarding headers in Source/WebKit/chromium/public
> *) move implementations for common API, like WebString.cpp, into Source/Platform/chromium/src

I feel like this plan strikes a good balance.  I'm happy moving forward with this plan.

I might make a small tweak though:

  How about using Source/Platform/chromium/platform instead of .../public?
  That would help reinforce the meaning of what that directory contains,
  and we would be able to continue including files from there using
  #include <platform/WebFoo.h>.  Also, 'public' bugs me since common is
  just as *public* as the directory named public.

(I'd also be OK merging common into platform since it is fair to say that client is built on top of platform.  I'm not sure how much value common adds.)

You could also introduce a subdirectory of Platform/chromium/ called SPI (although I hate that acronym) that contains the platform, common, and src directories.

[Darin Fisher (:fishd, Google)]

Keep in mind also that over time Source/Platform/chromium/ will contain everything that is presently in Source/WebCore/platform/chromium/.

[Adam Barth]

Comment on attachment 119527 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119527&action=review

> Source/Platform/chromium/Platform.gyp:34
> +            'target_name': 'platform',

This file should be located at Source/Platform/Platform.gyp/Platform.gyp.  The reason is slightly involved, but that's the pattern we use elsewhere.

> Source/Platform/chromium/Platform.gyp:39
> +                '../../WebCore/WebCore.gyp/WebCore.gyp:webcore_platform', # bad bad, but WebCString depends on stuff in WebCore/platform/text/TextEncoding

Hum...  This is going to cause problems when we migrate code from WebCore/platform to Platform, but we can solve that problem when we get there.  I would clean up this comment a bit.  As written, it's unlikely to stand the test of time.

> Source/Platform/chromium/Platform.gyp:50
> +                'common/WebCString.h',
> +                'common/WebCommon.h',
> +                'common/WebString.h',

Is there a large benefit from separating the "common" parts from the rest of the API?  I'm not sure the distinction is super important, and it's nice to have all the public APIs in folders with "public" in their names.

[James Robinson]

The reason I have 'common' isolated is to make it clearer to people editing the headers or implementations in Source/Platform/ what the possibly implications are. For example, any modification to WebString will impact both the client and platform APIs, while changes to WebMimeRegistry only impact the platform API. I'm not sure if that's enough value to keep things distinct.

[James Robinson]

(In reply to comment #8)
> (From update of attachment 119527 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119527&action=review
> 
> > Source/Platform/chromium/Platform.gyp:34
> > +            'target_name': 'platform',
> 
> This file should be located at Source/Platform/Platform.gyp/Platform.gyp.  The reason is slightly involved, but that's the pattern we use elsewhere.
> 

OK

> > Source/Platform/chromium/Platform.gyp:39
> > +                '../../WebCore/WebCore.gyp/WebCore.gyp:webcore_platform', # bad bad, but WebCString depends on stuff in WebCore/platform/text/TextEncoding
> 
> Hum...  This is going to cause problems when we migrate code from WebCore/platform to Platform, but we can solve that problem when we get there.  I would clean up this comment a bit.  As written, it's unlikely to stand the test of time.
> 

I don't intend to land with this comment - it's just a reminder for myself. Somehow we have to be able to handle text encoding without depending on WebCore, I just need to figure out how that happens and do it for WebCString.

[Darin Fisher (:fishd, Google)]

(In reply to comment #8)
> > Source/Platform/chromium/Platform.gyp:39
> > +                '../../WebCore/WebCore.gyp/WebCore.gyp:webcore_platform', # bad bad, but WebCString depends on stuff in WebCore/platform/text/TextEncoding

I think this problem goes away once we move all of Source/WebCore/platform to Source/Platform/, no?

[James Robinson]

(In reply to comment #11)
> (In reply to comment #8)
> > > Source/Platform/chromium/Platform.gyp:39
> > > +                '../../WebCore/WebCore.gyp/WebCore.gyp:webcore_platform', # bad bad, but WebCString depends on stuff in WebCore/platform/text/TextEncoding
> 
> I think this problem goes away once we move all of Source/WebCore/platform to Source/Platform/, no?

Yes. My understanding is that's a long way out, however.

[James Robinson]

(In reply to comment #6)
>   How about using Source/Platform/chromium/platform instead of .../public?
>   That would help reinforce the meaning of what that directory contains,
>   and we would be able to continue including files from there using
>   #include <platform/WebFoo.h>.  Also, 'public' bugs me since common is
>   just as *public* as the directory named public.
> 
> (I'd also be OK merging common into platform since it is fair to say that client is built on top of platform.  I'm not sure how much value common adds.)


One subtle consequence of this is the forwarding headers for WebCommon/WebString/WebCString have to use somewhat-absolute include paths since users of the WebKit API have Source/WebKit/chromium/public/ on the include path already, so we can't just do #include "platform/WebCommon.h" from Source/WebKit/chromium/public/platform/WebCommon.h since it would just be a cycle.  We can however have that file do #include "../../../../Platform/chromium/platform/WebCommon.h" and then from inside Platform/chromium/platform/ have all #includes be without the "platform/" prefix.

[James Robinson]

Created attachment 119676 [details]
Patch

[WebKit Review Bot]

Attachment 119676 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'.gitignore', u'ChangeLog', u'Source/Platfo..." exit_code: 1

Source/Platform/chromium/platform/WebString.h:48:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/Platform/chromium/platform/WebString.h:76:  The parameter name "s" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/Platform/chromium/platform/WebCString.h:51:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/Platform/chromium/platform/WebCommon.h:89:  int16_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/Platform/chromium/platform/WebCommon.h:90:  uint16_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/Platform/chromium/platform/WebCommon.h:91:  int32_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/Platform/chromium/platform/WebCommon.h:92:  uint32_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 7 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[James Robinson]

One question this raises is where to put the forwarding headers for the WebKit client API. Today, WebString.h exists in Source/WebKit/chromium/public/platform/. With this patch I've moved the actual location to Source/Platform/chromium/platform/WebString.h, so includes of the form "platform/WebString.h" still work but absolute includes (like in chromium) need to see a forwarding header.

It's awkward to me that users of the WebKit client API have to be aware of this platform distinction for some headers.  I propose that we leave forwarding headers in Source/WK/chr/public/X.h that forward to platform/X.h so users of the client API don't have to care. Concretely that would mean not having Source/WK/chr/public/platform/WebString.h at all and instead having:

Source/WebKit/chromium/public/WebString.h:
#include "platform/WebString.h"
Source/WebKit/chromium/public/WebClientAPIThatDependsOnWebString.h:
#include "WebString.h"

Source/Platform/chromium/platform/WebString.h:
// real meat of WebString.h


then users the WebKit client API using absolute include paths #include all of their headers from third_party/WebKit/Source/WebKit/chromium/public/, users of the WebKit platform API #include all of their headers from third_party/WebKit/Source/Platform/chromium/platform/, and users of both include what they need from both.

WDYT? This means effectively undoing the platform/ move for the "common" headers.

[James Robinson]

This patch should compile without any chromium-side changes. The style errors are all from the file moves and existed in the original files.

[Darin Fisher (:fishd, Google)]

(In reply to comment #13)
> One subtle consequence of this is the forwarding headers for WebCommon/WebString/WebCString have to use somewhat-absolute include paths since users of the WebKit API have Source/WebKit/chromium/public/ on the include path already, so we can't just do #include "platform/WebCommon.h" from Source/WebKit/chromium/public/platform/WebCommon.h since it would just be a cycle.  We can however have that file do #include "../../../../Platform/chromium/platform/WebCommon.h" and then from inside Platform/chromium/platform/ have all #includes be without the "platform/" prefix.

This is consistent with the way code includes WTF.  Outside of WTF, people typically type #include <wtf/Foo.h>.  Inside WTF, they just type #include "Foo.h".

[WebKit Review Bot]

Comment on attachment 119676 [details]
Patch

Attachment 119676 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10904876

[James Robinson]

https://bugs.webkit.org/show_bug.cgi?id=74761 avoids the need for Platform/ to depend on WebCore. I can also update the platform includes to <> style.

[James Robinson]

Created attachment 119708 [details]
Patch

[WebKit Review Bot]

Attachment 119708 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'.gitignore', u'ChangeLog', u'Source/Platfo..." exit_code: 1

Source/Platform/chromium/platform/WebString.h:48:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/Platform/chromium/platform/WebString.h:76:  The parameter name "s" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/Platform/chromium/platform/WebCString.h:51:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/Platform/chromium/src/WebString.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Source/Platform/chromium/platform/WebCommon.h:89:  int16_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/Platform/chromium/platform/WebCommon.h:90:  uint16_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/Platform/chromium/platform/WebCommon.h:91:  int32_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/Platform/chromium/platform/WebCommon.h:92:  uint32_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 8 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

Ping! Are we ready to go here?

[James Robinson]

Created attachment 121009 [details]
Patch

[James Robinson]

This moves the headers to Source/Platform/chromium/public/

[WebKit Review Bot]

Attachment 121009 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'.gitignore', u'ChangeLog', u'Source/Platfo..." exit_code: 1

Source/Platform/chromium/public/WebCString.h:51:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/Platform/chromium/public/WebString.h:48:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/Platform/chromium/public/WebString.h:76:  The parameter name "s" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/Platform/chromium/public/WebCommon.h:89:  int16_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/Platform/chromium/public/WebCommon.h:90:  uint16_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/Platform/chromium/public/WebCommon.h:91:  int32_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/Platform/chromium/public/WebCommon.h:92:  uint32_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/Platform/chromium/src/WebString.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 8 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

Committed r104048: <http://trac.webkit.org/changeset/104048>