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.
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.
Created attachment 86647 [details] Proposed Patch
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 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.
Created attachment 86648 [details] Proposed Patch Ack. Still learning your ways.
(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.").
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 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?
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 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:/
(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.
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.
Thanks for the reviews and suggestions, Dmitry.
Comment on attachment 86802 [details] Proposed Patch Clearing flags on attachment: 86802 Committed r81913: <http://trac.webkit.org/changeset/81913>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/81913 might have broken Chromium Win Release
I made another WebKit patch which fixes it. https://bugs.webkit.org/show_bug.cgi?id=57064 Otherwise, revert and I will redo it.