Bug 82587 - [Chromium] Move createURLLoader() into Platform
Summary: [Chromium] Move createURLLoader() into Platform
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: 2012-03-29 02:31 PDT by Adam Barth
Modified: 2012-03-29 12:48 PDT (History)
5 users (show)

See Also:


Attachments
Patch (11.89 KB, patch)
2012-03-29 02:35 PDT, Adam Barth
jamesr: 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 2012-03-29 02:31:37 PDT
Move createURLLoader() into Platform
Comment 1 Adam Barth 2012-03-29 02:35:09 PDT
Created attachment 134532 [details]
Patch
Comment 2 WebKit Review Bot 2012-03-29 02:38:37 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 James Robinson 2012-03-29 10:51:25 PDT
Comment on attachment 134532 [details]
Patch

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

R=me but one question about headers in the comments.

> Source/WebKit/chromium/public/platform/Platform.h:26
> +#include "../../../../Platform/chromium/public/Platform.h"

do we need this forwarding header? I've been assuming that we only need a forwarding header in here if we expect users of the client API to need to see the header and don't want them to have to put both APIs on their include path. I don't think that's true here, since we don't expect users of the API to be using Platform.h directly, right?

If we do need this to be exposed for users of the client API then I think it should live in WebKit/chromium/public/

> Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:34
> +#include "Platform.h"

could this be #include <public/Platform.h> or "../../../Platform/chromium/public/Platform.h"  to avoid the forwarding header?
Comment 4 Adam Barth 2012-03-29 10:59:48 PDT
(In reply to comment #3)
> (From update of attachment 134532 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134532&action=review
> 
> R=me but one question about headers in the comments.
> 
> > Source/WebKit/chromium/public/platform/Platform.h:26
> > +#include "../../../../Platform/chromium/public/Platform.h"
> 
> do we need this forwarding header? I've been assuming that we only need a forwarding header in here if we expect users of the client API to need to see the header and don't want them to have to put both APIs on their include path. I don't think that's true here, since we don't expect users of the API to be using Platform.h directly, right?

The issue is that Platform is a base class for WebKitPlatformSupport, so embedders need to see the header so that they can see all of WebKitPlatformSupport.

> If we do need this to be exposed for users of the client API then I think it should live in WebKit/chromium/public/
> 
> > Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:34
> > +#include "Platform.h"
> 
> could this be #include <public/Platform.h> or "../../../Platform/chromium/public/Platform.h"  to avoid the forwarding header?

#include <public/Platform.h> doesn't work because clients who include WebKitPlatformSupport.h wouldn't be able to use this include path.  The second thing works, but I thought it was cleaner to use a forwarding header.

I'm not 100% sure what we want to do with these forwarding headers in the long term.  My thinking was that I'd just make this one fit the pattern used by the others and we can worry about what to do with the forwarding headers once we're further along.
Comment 5 James Robinson 2012-03-29 11:05:53 PDT
The other headers have that pattern because they are part of the client API - i.e. someone who wants to talk to WebKit needs to be able to see the WebString header.  Right now Platform.h is just an implementation detail for people using the client API, so I think it makes more sense for now to "hide" it inside WebKitPlatformSupport.h instead of having a dedicated forwarding header.
Comment 6 Adam Barth 2012-03-29 11:41:45 PDT
Ok.  Will do.
Comment 7 Adam Barth 2012-03-29 12:48:31 PDT
Committed r112563: <http://trac.webkit.org/changeset/112563>