Bug 56885 - WebFrame::createAssociatedURLLoader can't accept WebURLLoaderOptions
: WebFrame::createAssociatedURLLoader can't accept WebURLLoaderOptions
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Platform
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-22 17:23 PDT by Bill Budge
Modified: 2011-03-24 16:56 PDT (History)
7 users (show)

See Also:


Attachments
Proposed Patch (3.30 KB, patch)
2011-03-23 09:53 PDT, Bill Budge
no flags Details | Formatted Diff | Diff
Proposed Patch (3.66 KB, patch)
2011-03-23 11:04 PDT, Bill Budge
no flags Details | Formatted Diff | Diff
Proposed Patch (3.64 KB, patch)
2011-03-23 11:17 PDT, Bill Budge
dimich: review-
Details | Formatted Diff | Diff
Proposed Patch (3.73 KB, patch)
2011-03-24 11:24 PDT, Bill Budge
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bill Budge 2011-03-22 17:23:05 PDT
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.
Comment 1 Bill Budge 2011-03-23 09:53:01 PDT
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.
Comment 2 Bill Budge 2011-03-23 11:04:30 PDT
Created attachment 86647 [details]
Proposed Patch
Comment 3 Darin Adler 2011-03-23 11:06:52 PDT
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.
Comment 4 Darin Adler 2011-03-23 11:07:28 PDT
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.
Comment 5 Bill Budge 2011-03-23 11:17:02 PDT
Created attachment 86648 [details]
Proposed Patch

Ack. Still learning your ways.
Comment 6 David Levin 2011-03-23 13:35:50 PDT
(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.").
Comment 7 Dmitry Titov 2011-03-23 13:48:00 PDT
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.
Comment 8 Dmitry Titov 2011-03-23 14:03:24 PDT
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?
Comment 9 Bill Budge 2011-03-23 18:00:12 PDT
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.
Comment 10 Dmitry Titov 2011-03-23 18:24:27 PDT
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:/
Comment 11 David Levin 2011-03-23 18:28:42 PDT
(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.
Comment 12 Bill Budge 2011-03-24 11:24:02 PDT
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.
Comment 13 Bill Budge 2011-03-24 13:48:41 PDT
Thanks for the reviews and suggestions, Dmitry.
Comment 14 WebKit Commit Bot 2011-03-24 15:54:21 PDT
Comment on attachment 86802 [details]
Proposed Patch

Clearing flags on attachment: 86802

Committed r81913: <http://trac.webkit.org/changeset/81913>
Comment 15 WebKit Commit Bot 2011-03-24 15:54:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 WebKit Review Bot 2011-03-24 16:08:26 PDT
http://trac.webkit.org/changeset/81913 might have broken Chromium Win Release
Comment 17 Bill Budge 2011-03-24 16:56:08 PDT
I made another WebKit patch which fixes it. https://bugs.webkit.org/show_bug.cgi?id=57064

Otherwise, revert and I will redo it.