Bug 73066 – [Chromium] Move WebKitPlatformSupport.h and dependencies to new public/platform directory

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

[Adam Barth]

[Chromium] Move WebKitPlatformSupport.h and dependencies to new public/platform directory

[Adam Barth]

Created attachment 116474 [details]
Patch

[WebKit Review Bot]

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

[Darin Fisher (:fishd, Google)]

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?

[Darin Fisher (:fishd, Google)]

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.

[Adam Barth]

> good question.

My initial read is "no", but I need to investigate more why it's used by WebKitPlatformSupport.

[Adam Barth]

Thanks for the review.

[Adam Barth]

Committed r101122: <http://trac.webkit.org/changeset/101122>

[Adam Barth]

> 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.

[Darin Fisher (:fishd, Google)]

(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.

[Adam Barth]

> 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).

[James Robinson]

(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).

[James Robinson]

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.

[Adam Barth]

> 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.

[Darin Fisher (:fishd, Google)]

(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.

[Darin Fisher (:fishd, Google)]

(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.