Bug 58287

Summary: [chromium] WebCrossOriginRequestPolicy enum should use 'Web' prefix for values
Product: WebKit Reporter: Bill Budge <bbudge>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fishd
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed Patch
none
Proposed Patch
none
Proposed Patch none

Description Bill Budge 2011-04-11 16:51:34 PDT
WebKit enums values should be prefixed with 'Web'.
Comment 1 Bill Budge 2011-04-11 17:02:25 PDT
Created attachment 89125 [details]
Proposed Patch
Comment 2 David Levin 2011-04-11 17:04:31 PDT
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.
Comment 3 David Levin 2011-04-11 17:05:25 PDT
Note this patch doesn't have an r?. Also, since Darin is back and this is a chromium api patch, he should review it.
Comment 4 Bill Budge 2011-04-11 17:19:24 PDT
Created attachment 89128 [details]
Proposed Patch
Comment 5 Darin Fisher (:fishd, Google) 2011-04-11 20:30:53 PDT
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.
Comment 6 Bill Budge 2011-04-12 07:38:02 PDT
Created attachment 89201 [details]
Proposed Patch

Moved WebURLLoaderOptions into its own file. Renamed CrossOriginRequestPolicy enum values to be consistent with WebKit style.
Comment 7 Darin Fisher (:fishd, Google) 2011-04-12 09:18:55 PDT
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.
Comment 8 Bill Budge 2011-04-12 09:55:04 PDT
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.
Comment 9 Darin Fisher (:fishd, Google) 2011-04-12 10:16:05 PDT
(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.
Comment 10 Bill Budge 2011-04-12 10:26:38 PDT
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 11 Darin Fisher (:fishd, Google) 2011-04-12 10:46:51 PDT
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 12 WebKit Commit Bot 2011-04-12 11:19:17 PDT
Comment on attachment 89201 [details]
Proposed Patch

Clearing flags on attachment: 89201

Committed r83613: <http://trac.webkit.org/changeset/83613>
Comment 13 WebKit Commit Bot 2011-04-12 11:19:21 PDT
All reviewed patches have been landed.  Closing bug.