Summary: | [Chromium] Move createURLLoader() into Platform | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||
Component: | New Bugs | Assignee: | Adam Barth <abarth> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | dglazkov, fishd, jamesr, tkent, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Adam Barth
2012-03-29 02:31:37 PDT
Created attachment 134532 [details]
Patch
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 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? (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. 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. Ok. Will do. Committed r112563: <http://trac.webkit.org/changeset/112563> |