Summary: | [chromium] WebCrossOriginRequestPolicy enum should use 'Web' prefix for values | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Bill Budge <bbudge> | ||||||||
Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, fishd | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Bill Budge
2011-04-11 16:51:34 PDT
Created attachment 89125 [details]
Proposed Patch
Comment on attachment 89125 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89125&action=review > Source/WebKit/chromium/src/AssociatedURLLoader.cpp:171 > + m_options.crossOriginRequestPolicy = WebAllowCrossOriginRequests; // TODO(bbudge) Default should be WebDenyCrossOriginRequests, but this would break some tests. Ideally FIXME (with no name) instead of TODO. Note this patch doesn't have an r?. Also, since Darin is back and this is a chromium api patch, he should review it. Created attachment 89128 [details]
Proposed Patch
Hi Bill, I didn't fully explain myself in the Chromium review. There's actually a few conventions worth noting: 1- Top-level struct, class and enums each get their own header file in the WebKit API. 2- Sometimes a nested enum or struct is better if the enum/struct is scoped to being used by only a single interface. Nested enums/structs are not prefixed with Web. 3- Enums are named like so: enum Foo { FooBar, FooBaz }; <-- values prefixed by enum name These are in addition to the convention about globals starting with the Web prefix. I wish I had explained all of this so that it would save you time. I only bother to point all of this out as it is nice to keep the WebKit API consistent style-wise. Created attachment 89201 [details]
Proposed Patch
Moved WebURLLoaderOptions into its own file. Renamed CrossOriginRequestPolicy enum values to be consistent with WebKit style.
Comment on attachment 89201 [details]
Proposed Patch
R=me
Since this is likely to break Chromium, what is the plan for landing this change?
A couple ideas:
1- You could add a #define in WebURLLoader.h, and then in Chromium code you can land #ifdefs that switch on that #define to determine which definition of WebURLLoaderOptions and CrossOriginRequestPolicy to use. Then, land that Chromium patch before landing this WebKit patch.
2- You could change this WebKit patch to make WebURLLoader.h #include WebURLLoaderOptions.h, and you could also keep the old enum around, defined in terms of the new one.
I think this can land safely. The only change I landed in Chromiumfor this was in WebKit/mocks/mock_weframe.h / .cpp, and those files never need the full definition of WebURLLoaderOptions. This is assuming no one has since made changes depending on it. I can't find any. (In reply to comment #8) > I think this can land safely. The only change I landed in Chromiumfor this was in WebKit/mocks/mock_weframe.h / .cpp, and those files never need the full definition of WebURLLoaderOptions. This is assuming no one has since made changes depending on it. I can't find any. I see... but surely you will at least need a #include "WebURLLoaderOptions.h" in WebURLLoader.h. I don't think it's necessary to include WebURLLoaderOptions.h. WebURLLoaderOptions really has nothing to do with the WebURLLoader abstraction, just with instantiating AssociatedURLLoaders (for now). In fact, at first I called these AssociatedURLLoaderOptions. Later I thought it was bad to expose a class with that name, which is really an implementation detail, and WebURLLoaderOptions seemed like a natural name. So it seems incorrect (though perhaps convenient) to include the header in WebURLLoader. Anyone who wants to set up options has to include an extra file. Of course, I have to change my 2 patches to PPAPI to adapt to these changes. Comment on attachment 89201 [details]
Proposed Patch
OK, I see... I was concerned that the build would be temporarily broken by this change, but I see that the Chromium code is not broken because it sees forward declares of WebURLLoaderOptions where needed. Sorry for the noise. CQ+
Comment on attachment 89201 [details] Proposed Patch Clearing flags on attachment: 89201 Committed r83613: <http://trac.webkit.org/changeset/83613> All reviewed patches have been landed. Closing bug. |