Bug 56885 - WebFrame::createAssociatedURLLoader can't accept WebURLLoaderOptions
: WebFrame::createAssociatedURLLoader can't accept WebURLLoaderOptions
Status: RESOLVED FIXED
: WebKit
Platform
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-03-22 17:23 PST by
Modified: 2011-03-24 16:56 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-03-22 17:23:05 PST
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 From 2011-03-23 09:53:01 PST -------
Created an attachment (id=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 From 2011-03-23 11:04:30 PST -------
Created an attachment (id=86647) [details]
Proposed Patch
------- Comment #3 From 2011-03-23 11:06:52 PST -------
(From update of attachment 86647 [details])
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 From 2011-03-23 11:07:28 PST -------
(From update of attachment 86647 [details])
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 From 2011-03-23 11:17:02 PST -------
Created an attachment (id=86648) [details]
Proposed Patch

Ack. Still learning your ways.
------- Comment #6 From 2011-03-23 13:35:50 PST -------
(In reply to comment #5)
> Created an attachment (id=86648) [details] [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 From 2011-03-23 13:48:00 PST -------
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 From 2011-03-23 14:03:24 PST -------
(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?
------- Comment #9 From 2011-03-23 18:00:12 PST -------
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 From 2011-03-23 18:24:27 PST -------
(From update of attachment 86648 [details])
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 From 2011-03-23 18:28:42 PST -------
(In reply to comment #8)
> (From update of attachment 86648 [details] [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 From 2011-03-24 11:24:02 PST -------
Created an attachment (id=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 From 2011-03-24 13:48:41 PST -------
Thanks for the reviews and suggestions, Dmitry.
------- Comment #14 From 2011-03-24 15:54:21 PST -------
(From update of attachment 86802 [details])
Clearing flags on attachment: 86802

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

Otherwise, revert and I will redo it.