Bug 74583

Summary: [chromium] Move WebMimeRegistry and dependencies to Source/Platform
Product: WebKit Reporter: James Robinson <jamesr>
Component: New BugsAssignee: James Robinson <jamesr>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, dpranke, fishd, levin, ojan, piman, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 74761    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch fishd: review+

Description James Robinson 2011-12-14 20:51:10 PST
[chromium] Demonstrate one possible way to move chromium WebKit platform API into Source/Platform
Comment 1 James Robinson 2011-12-14 20:52:11 PST
Created attachment 119368 [details]
Patch
Comment 2 James Robinson 2011-12-15 15:08:41 PST
Created attachment 119505 [details]
Patch
Comment 3 James Robinson 2011-12-15 17:14:39 PST
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?
Comment 4 James Robinson 2011-12-15 17:19:58 PST
Created attachment 119527 [details]
Patch
Comment 5 James Robinson 2011-12-15 17:21:49 PST
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?
Comment 6 Darin Fisher (:fishd, Google) 2011-12-15 17:25:48 PST
(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.
Comment 7 Darin Fisher (:fishd, Google) 2011-12-15 17:26:28 PST
Keep in mind also that over time Source/Platform/chromium/ will contain everything that is presently in Source/WebCore/platform/chromium/.
Comment 8 Adam Barth 2011-12-15 17:33:54 PST
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.
Comment 9 James Robinson 2011-12-15 17:43:50 PST
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.
Comment 10 James Robinson 2011-12-15 17:45:10 PST
(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.
Comment 11 Darin Fisher (:fishd, Google) 2011-12-16 09:41:15 PST
(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?
Comment 12 James Robinson 2011-12-16 11:13:55 PST
(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.
Comment 13 James Robinson 2011-12-16 13:04:44 PST
(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.
Comment 14 James Robinson 2011-12-16 14:50:50 PST
Created attachment 119676 [details]
Patch
Comment 15 WebKit Review Bot 2011-12-16 14:54:30 PST
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.
Comment 16 WebKit Review Bot 2011-12-16 14:54:51 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 17 James Robinson 2011-12-16 14:55:04 PST
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.
Comment 18 James Robinson 2011-12-16 14:56:45 PST
This patch should compile without any chromium-side changes. The style errors are all from the file moves and existed in the original files.
Comment 19 Darin Fisher (:fishd, Google) 2011-12-16 15:04:16 PST
(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".
Comment 20 WebKit Review Bot 2011-12-16 15:20:49 PST
Comment on attachment 119676 [details]
Patch

Attachment 119676 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10904876
Comment 21 James Robinson 2011-12-16 15:26:21 PST
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.
Comment 22 James Robinson 2011-12-16 17:52:10 PST
Created attachment 119708 [details]
Patch
Comment 23 WebKit Review Bot 2011-12-16 17:54:36 PST
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.
Comment 24 James Robinson 2012-01-03 12:55:27 PST
Ping! Are we ready to go here?
Comment 25 James Robinson 2012-01-03 15:51:50 PST
Created attachment 121009 [details]
Patch
Comment 26 James Robinson 2012-01-03 15:53:09 PST
This moves the headers to Source/Platform/chromium/public/
Comment 27 WebKit Review Bot 2012-01-03 15:56:07 PST
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.
Comment 28 James Robinson 2012-01-04 11:08:14 PST
Committed r104048: <http://trac.webkit.org/changeset/104048>