Bug 56885 – WebFrame::createAssociatedURLLoader can't accept WebURLLoaderOptions

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

[Bill Budge]

WebFrame's createAssociatedURLLoader method takes no parameters, so the returned WebURLLoader can't be configured for CORS support. We should add an overload, or a parameter with a default value to the existing method declaration.

[Bill Budge]

Created attachment 86633 [details]
Proposed Patch

I had to add a createAssociatedURLLoader overload to WebFrame. Once this is landed, I can modify client code to use this new overload and remove the old one.

[Bill Budge]

Created attachment 86647 [details]
Proposed Patch

[Darin Adler]

Comment on attachment 86647 [details]
Proposed Patch

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

> Source/WebKit/chromium/public/WebFrame.h:349
> +    // TODO(bbudge) Remove this overload when clients have been changed to pass options.

We don’t put our names in the code. And we use FIXME, not TODO. Please follow our usual style.

[Darin Adler]

Comment on attachment 86647 [details]
Proposed Patch

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

>> Source/WebKit/chromium/public/WebFrame.h:349
>> +    // TODO(bbudge) Remove this overload when clients have been changed to pass options.
> 
> We don’t put our names in the code. And we use FIXME, not TODO. Please follow our usual style.

Except maybe the rules are different in the Chromium-specific code.

[Bill Budge]

Created attachment 86648 [details]
Proposed Patch

Ack. Still learning your ways.

[David Levin]

(In reply to comment #5)
> Created an attachment (id=86648) [details]
> Proposed Patch
> 
> Ack. Still learning your ways.

May I humbly suggest an appropriate penance would be to fix our style checking tool to catch this automatically? :)

It shouldn't be too hard even if you don't know python. I'd be happy to give you pointers/help with doing this. (The code is in Tools/Scripts/webkitpy/style/checkers/cpp.py and I suspect we'd need code similar to where we check for 2 spaces after a period in comments -- as a note to myself that occurs where there is a comment "    # Next, we check for proper spacing with respect to comments.").

[Dmitry Titov]

It looks ok, but could you please add info or reference to other bugs here so it'd be clear in what context this change is made? 

Usually, if there is a big work item that is split into several small patches (like this one appears to be), there is an uber-bug that shortly describes the bigger change, and small bugs (one per patch) that link back to the uber-bug.

This structure is useful when some time later another engineer tries to figure out what changes were made for what and how they are related.

[Dmitry Titov]

Comment on attachment 86648 [details]
Proposed Patch

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

> Source/WebKit/chromium/public/WebFrame.h:355
> +    virtual WebURLLoader* createAssociatedURLLoader(const WebURLLoaderOptions&) = 0;

Nit: I understand you just adding a parameter to existing method, but it would be nice to understand why this "createFoo()" method returns a naked pointer instead PassXXXPtr?

[Bill Budge]

This work is needed to get the benefits of recent changes to AssociatedURLLoader which add support for CORS. https://bugs.webkit.org/show_bug.cgi?id=53925

This change allows clients to specify CORS options.

[Dmitry Titov]

Comment on attachment 86648 [details]
Proposed Patch

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

Almost there. just a few tiny things.

> Source/WebKit/chromium/ChangeLog:6
> +        https://bugs.webkit.org/show_bug.cgi?id=56885

This ChangeLog is not informative. It is obvious that the patch adds a parameter. Good ChangeLog entry tells why (as you did in the bug itself).

> Source/WebKit/chromium/public/WebFrame.h:350
> +    virtual WebURLLoader* createAssociatedURLLoader() = 0;

/FIXME/FIXME:/

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1055
> +// FIXME Remove this overload when clients have been changed to pass options.

/FIXME/FIXME:/

> Source/WebKit/chromium/src/WebFrameImpl.h:136
> +    // FIXME Remove this overload when clients have been changed to pass options.

/FIXME/FIXME:/

[David Levin]

(In reply to comment #8)
> (From update of attachment 86648 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86648&action=review
> 
> > Source/WebKit/chromium/public/WebFrame.h:355
> > +    virtual WebURLLoader* createAssociatedURLLoader(const WebURLLoaderOptions&) = 0;
> 
> Nit: I understand you just adding a parameter to existing method, but it would be nice to understand why this "createFoo()" method returns a naked pointer instead PassXXXPtr?

Plus it would be nice to understand this.

[Bill Budge]

Created attachment 86802 [details]
Proposed Patch

To answer Dmitri's question, this method returns a raw pointer because it is used in Chromium "glue" code that uses Chromium smart pointers rather than the WebKit ones. None of these files reference the wtf namespace.

[Bill Budge]

Thanks for the reviews and suggestions, Dmitry.

[WebKit Commit Bot]

Comment on attachment 86802 [details]
Proposed Patch

Clearing flags on attachment: 86802

Committed r81913: <http://trac.webkit.org/changeset/81913>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/81913 might have broken Chromium Win Release

[Bill Budge]

I made another WebKit patch which fixes it. https://bugs.webkit.org/show_bug.cgi?id=57064

Otherwise, revert and I will redo it.