Bug 73066 - [Chromium] Move WebKitPlatformSupport.h and dependencies to new public/platform directory
Summary: [Chromium] Move WebKitPlatformSupport.h and dependencies to new public/platfo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-23 20:08 PST by Adam Barth
Modified: 2011-12-01 09:25 PST (History)
3 users (show)

See Also:


Attachments
Patch (155.75 KB, patch)
2011-11-23 20:12 PST, Adam Barth
fishd: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2011-11-23 20:08:19 PST
[Chromium] Move WebKitPlatformSupport.h and dependencies to new public/platform directory
Comment 1 Adam Barth 2011-11-23 20:12:42 PST
Created attachment 116474 [details]
Patch
Comment 2 WebKit Review Bot 2011-11-23 21:35:29 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Darin Fisher (:fishd, Google) 2011-11-23 22:52:43 PST
This smells like all kinds of awesome.  What about the idea of moving these headers into Source/Platform/chromium/public/?  Is WebKit/chromium/public/platform/ staging ground for that eventual move?
Comment 4 Darin Fisher (:fishd, Google) 2011-11-23 22:57:06 PST
Comment on attachment 116474 [details]
Patch

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

> Source/WebKit/chromium/public/platform/WebSerializedScriptValue.h:49
> +// FIXME: Should this class be in platform?

good question.
Comment 5 Adam Barth 2011-11-23 23:10:27 PST
> good question.

My initial read is "no", but I need to investigate more why it's used by WebKitPlatformSupport.
Comment 6 Adam Barth 2011-11-23 23:10:35 PST
Thanks for the review.
Comment 7 Adam Barth 2011-11-23 23:13:37 PST
Committed r101122: <http://trac.webkit.org/changeset/101122>
Comment 8 Adam Barth 2011-11-24 00:06:13 PST
> This smells like all kinds of awesome.  What about the idea of moving these headers into Source/Platform/chromium/public/?  Is WebKit/chromium/public/platform/ staging ground for that eventual move?

Yes, that's the idea.  I'm going to try to sort out which headers belong on which side of the split.  Using forwarding headers lets us feel this out without impacting the rest of Chromium too much.

I'd also like to split up the CPP files in src, which will require use to be a bit more honest about our dependencies.
Comment 9 Darin Fisher (:fishd, Google) 2011-11-24 00:13:17 PST
(In reply to comment #8)
> > This smells like all kinds of awesome.  What about the idea of moving these headers into Source/Platform/chromium/public/?  Is WebKit/chromium/public/platform/ staging ground for that eventual move?
> 
> Yes, that's the idea.  I'm going to try to sort out which headers belong on which side of the split.  Using forwarding headers lets us feel this out without impacting the rest of Chromium too much.
> 
> I'd also like to split up the CPP files in src, which will require use to be a bit more honest about our dependencies.

Sounds great.  Hopefully, there is an end-state that permits WebCore (from Chromium-specific .cpp files) to talk directly to these platform/ headers.
Comment 10 Adam Barth 2011-11-24 00:23:14 PST
> Hopefully, there is an end-state that permits WebCore (from Chromium-specific .cpp files) to talk directly to these platform/ headers.

My thought was that Chromium-specific code in Source/Platform could talk to them.  WebCore-proper code would still need to go through some sort of port-agnostic Platform abstraction.

This should also let us solve the problem of having WebCore-namespaced code in WebKit/chromium/src (although I need to study all uses of this to make sure we can handle them all this way).
Comment 11 James Robinson 2011-11-29 23:21:44 PST
(In reply to comment #10)
> > Hopefully, there is an end-state that permits WebCore (from Chromium-specific .cpp files) to talk directly to these platform/ headers.
> 
> My thought was that Chromium-specific code in Source/Platform could talk to them.  WebCore-proper code would still need to go through some sort of port-agnostic Platform abstraction.

Is this presupposing that code that today exists in Source/WebCore/platform moves to Source/Platform?  Concretely I'd like to implement WebCore::GraphicsLayer in terms of interfaces in this platform/ section.  What needs to happen in order to do that? Can I have an implementation in platform/graphics/chromium/GraphicsLayerChromium.cpp talk directly to types in this new platform directory or not?

> 
> This should also let us solve the problem of having WebCore-namespaced code in WebKit/chromium/src (although I need to study all uses of this to make sure we can handle them all this way).
Comment 12 James Robinson 2011-11-29 23:23:10 PST
Also, do we plan to keep using the Web* prefix for platform APIs? I don't think it makes as much sense for platform stuff as it does for client interfaces, personally.
Comment 13 Adam Barth 2011-11-30 00:01:19 PST
> Is this presupposing that code that today exists in Source/WebCore/platform moves to Source/Platform?  

Yes, but we don't need to wait.  I was just trying to think ahead.

> Concretely I'd like to implement WebCore::GraphicsLayer in terms of interfaces in this platform/ section.  What needs to happen in order to do that?

Mostly adding the include path to the WebCore.gyp file.  Past that, it's mostly aesthetics.

> Can I have an implementation in platform/graphics/chromium/GraphicsLayerChromium.cpp talk directly to types in this new platform directory or not?

Currently that's not allowed, but we could change our minds about what's allowed.  IMHO, that's the direction we should move in.

> Also, do we plan to keep using the Web* prefix for platform APIs? I don't think it makes as much sense for platform stuff as it does for client interfaces, personally.

That sounds like a question for fishd.
Comment 14 Darin Fisher (:fishd, Google) 2011-12-01 09:24:28 PST
(In reply to comment #13)
> > Can I have an implementation in platform/graphics/chromium/GraphicsLayerChromium.cpp talk directly to types in this new platform directory or not?
> 
> Currently that's not allowed, but we could change our minds about what's allowed.  IMHO, that's the direction we should move in.

+1


> > Also, do we plan to keep using the Web* prefix for platform APIs? I don't think it makes as much sense for platform stuff as it does for client interfaces, personally.
> 
> That sounds like a question for fishd.

I had wrestled with this a bit, but I came back to sticking with the Web* prefix.  Reason:  WebString and friends will need to live in the platform/ directory.  If we are going to have some Web* in the platform directory, then we might as well be consistent about using it for all interfaces.

I think the rule for platform/ APIs should be that they are _only_ APIs that are needed to support WebCore.  There should be no static functions at this level in other words.
Comment 15 Darin Fisher (:fishd, Google) 2011-12-01 09:25:28 PST
(In reply to comment #14)
> There should be no static functions at this level in other words.

^^^ Whoops, scratch that.  WebString::fromUTF8 is an obvious counter example.