Bug 58287 – [chromium] WebCrossOriginRequestPolicy enum should use 'Web' prefix for values

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Bill Budge]

WebKit enums values should be prefixed with 'Web'.

[Bill Budge]

Created attachment 89125 [details]
Proposed Patch

[David Levin]

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.

[David Levin]

Note this patch doesn't have an r?. Also, since Darin is back and this is a chromium api patch, he should review it.

[Bill Budge]

Created attachment 89128 [details]
Proposed Patch

[Darin Fisher (:fishd, Google)]

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.

[Bill Budge]

Created attachment 89201 [details]
Proposed Patch

Moved WebURLLoaderOptions into its own file. Renamed CrossOriginRequestPolicy enum values to be consistent with WebKit style.

[Darin Fisher (:fishd, Google)]

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.

[Bill Budge]

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.

[Darin Fisher (:fishd, Google)]

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

[Bill Budge]

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.

[Darin Fisher (:fishd, Google)]

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+

[WebKit Commit Bot]

Comment on attachment 89201 [details]
Proposed Patch

Clearing flags on attachment: 89201

Committed r83613: <http://trac.webkit.org/changeset/83613>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.