When asking the FrameLoaderClient to create a new page, pass along the ResourceRequest we intend to use to load that frame
Created attachment 108668 [details] Patch
Comment on attachment 108668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108668&action=review > Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:860 > +Frame* FrameLoaderClientImpl::dispatchCreatePage(const NavigationAction& action, const ResourceRequest& request) NavigationAction has an URL. perhaps the NavigationAction should hold a ResourceRequest instead? maybe it should hold a FrameLoadRequest?
(In reply to comment #2) > (From update of attachment 108668 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=108668&action=review > > > Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:860 > > +Frame* FrameLoaderClientImpl::dispatchCreatePage(const NavigationAction& action, const ResourceRequest& request) > > NavigationAction has an URL. perhaps the NavigationAction should hold a ResourceRequest instead? maybe it should hold a FrameLoadRequest? in chromium's FrameLoaderClientImpl, we create a FrameLoadRequest, so FrameLoadRequest sounds like a viable alternative
Could you please provide a rationale for this change? It seems confusing to pass arguments with overlapping data.
The issue I'm trying to fix is that of you click on a link with target=_blank, ChromeClient::createWindow with a FrameLoadRequest holding a ResourceRequest is eventually called (in the chromium port at least). When you middle click on the same link, ChromeClient::createWindow is called with a FrameLoadRequest with an empty ResourceRequest, so some of the checks we've put in createWindow don't work. I wasn't aware that NavigationAction already holds the URL, so I guess the easiest way to fix this is to create a resource request from the navigation action's url inside the chromium port before calling createWindow
Created attachment 108835 [details] Patch
Comment on attachment 108835 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108835&action=review > Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:864 > + m_webFrame->frame(), FrameLoadRequest(m_webFrame->frame()->document()->securityOrigin(), ResourceRequest(action.url())), what if we are opening a new page as a result of a form POST submission? the ResourceRequest generated here will say GET even though it is actually a POST. maybe that won't cause you any problems for your use case, but it might lead to bugs in the future. it seems like it would be ideal if the NavigationAction held the actual ResourceRequest. also, reading FrameLoader.cpp, i see that the createWindow function defined in there calls Chrome::createWindow passing an empty NavigationAction. the url property was not set. is that going to cause you a problem?
Created attachment 110887 [details] Patch
Comment on attachment 110887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110887&action=review > Source/WebCore/loader/FrameLoader.cpp:3095 > + ResourceRequest requestForOriginalURL(request); I guess this one could be slightly lossy, right?
Nate and Adam may also have opinions about this approach. I'm supportive as I think this leads to a cleaner solution.
I looked at the patch briefly. The general approach of using ResourceRequest in more places instead of KURL seems like a good idea. Nate made a similar change to CachedResourceLoader, which has had a bunch of good effects. I'm secretly hoping Nate would be willing to review this patch. If not, I can take a closer look.
Comment on attachment 110887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110887&action=review >> Source/WebCore/loader/FrameLoader.cpp:3095 >> + ResourceRequest requestForOriginalURL(request); > > I guess this one could be slightly lossy, right? Nevermind. I overlooked the fact that you are initializing requestForOriginalURL with the copy constructor.
Comment on attachment 110887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110887&action=review This seems good in general, I just have a couple questions.... Is it possible to test this via layout test? > Source/WebKit/chromium/src/ChromeClientImpl.cpp:260 > Page* ChromeClientImpl::createWindow( > - Frame* frame, const FrameLoadRequest& r, const WindowFeatures& features, const NavigationAction&) > + Frame* frame, const FrameLoadRequest& r, const WindowFeatures& features, const NavigationAction& action) > { Is there any chance we could just remove the FrameLoadRequest parameter altogether? It looks like chromium and webkit2 are the only ChromeClient implementations that use it, and chromium is the only one that uses it for anything other than the ResourceRequest.
Comment on attachment 110887 [details] Patch Attachment 110887 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10068090 New failing tests: fast/loader/scroll-position-restored-on-back-non-cached.html http/tests/navigation/postredirect-goback1.html fast/loader/scroll-position-restored-on-back.html http/tests/navigation/postredirect-frames-goback1.html fast/loader/stateobjects/document-destroyed-navigate-back-with-fragment-scroll.html fast/loader/stateobjects/document-destroyed-navigate-back.html fast/history/history-replace-updates-current-item.html fast/history/location-replace-hash.html
Created attachment 111034 [details] Patch
(In reply to comment #13) > (From update of attachment 110887 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=110887&action=review > > This seems good in general, I just have a couple questions.... > > Is it possible to test this via layout test? done > > > Source/WebKit/chromium/src/ChromeClientImpl.cpp:260 > > Page* ChromeClientImpl::createWindow( > > - Frame* frame, const FrameLoadRequest& r, const WindowFeatures& features, const NavigationAction&) > > + Frame* frame, const FrameLoadRequest& r, const WindowFeatures& features, const NavigationAction& action) > > { > > Is there any chance we could just remove the FrameLoadRequest parameter altogether? It looks like chromium and webkit2 are the only ChromeClient implementations that use it, and chromium is the only one that uses it for anything other than the ResourceRequest. we'd need a way to get the frame name to createWindow (it's sometimes set to e.g. _blank), so we'd need an additional parameter if we drop the frame load request. Not sure whether that's worthwhile.
Comment on attachment 111034 [details] Patch Attachment 111034 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10062593 New failing tests: fast/loader/scroll-position-restored-on-back-non-cached.html http/tests/navigation/postredirect-goback1.html fast/loader/scroll-position-restored-on-back.html http/tests/navigation/postredirect-frames-goback1.html fast/loader/stateobjects/document-destroyed-navigate-back-with-fragment-scroll.html fast/loader/stateobjects/document-destroyed-navigate-back.html fast/history/history-replace-updates-current-item.html fast/history/location-replace-hash.html
Created attachment 111109 [details] Patch
Now without layout tests failures (I hope)
Comment on attachment 111109 [details] Patch Attachment 111109 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/10077816
Comment on attachment 111109 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111109&action=review > Tools/DumpRenderTree/chromium/WebViewHost.cpp:246 > -WebView* WebViewHost::createView(WebFrame*, const WebURLRequest&, const WebWindowFeatures&, const WebString&) > +WebView* WebViewHost::createView(WebFrame*, const WebURLRequest& request, const WebWindowFeatures&, const WebString&) > { > if (!layoutTestController()->canOpenWindows()) > return 0; > + if (layoutTestController()->shouldDumpCreateView()) > + fprintf(stdout, "createView(%s)\n", URLDescription(request.url()).c_str()); > return m_shell->createNewWindow(WebURL())->webView(); Is this test something that should be run on all platforms? As written this is going to fail on everything but chromium, so we should either make it cross-platform or make it clear thaqt it is expected to fail for everyone else (i.e., move it to LayoutTests/platform/chromium/fast/loader/). Sorry for not asking about this sooner :(
(In reply to comment #21) > (From update of attachment 111109 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=111109&action=review > > > Tools/DumpRenderTree/chromium/WebViewHost.cpp:246 > > -WebView* WebViewHost::createView(WebFrame*, const WebURLRequest&, const WebWindowFeatures&, const WebString&) > > +WebView* WebViewHost::createView(WebFrame*, const WebURLRequest& request, const WebWindowFeatures&, const WebString&) > > { > > if (!layoutTestController()->canOpenWindows()) > > return 0; > > + if (layoutTestController()->shouldDumpCreateView()) > > + fprintf(stdout, "createView(%s)\n", URLDescription(request.url()).c_str()); > > return m_shell->createNewWindow(WebURL())->webView(); > > Is this test something that should be run on all platforms? As written this is going to fail on everything but chromium, so we should either make it cross-platform or make it clear thaqt it is expected to fail for everyone else (i.e., move it to LayoutTests/platform/chromium/fast/loader/). > > Sorry for not asking about this sooner :( I don't think that test makes sense for other platforms, as the other ports don't necessarily implement dispatchCreatePage the way chromium does. The test is already in the platform/chromium/fast/loader directory
Comment on attachment 111109 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111109&action=review r+, please just verify that this isn't going to compile break mac before landing. >>> Tools/DumpRenderTree/chromium/WebViewHost.cpp:246 >> >> Is this test something that should be run on all platforms? As written this is going to fail on everything but chromium, so we should either make it cross-platform or make it clear thaqt it is expected to fail for everyone else (i.e., move it to LayoutTests/platform/chromium/fast/loader/). >> >> Sorry for not asking about this sooner :( > > I don't think that test makes sense for other platforms, as the other ports don't necessarily implement dispatchCreatePage the way chromium does. > > The test is already in the platform/chromium/fast/loader directory So it is. Sorry for my apparent blindness.
Created attachment 111309 [details] Patch
Comment on attachment 111309 [details] Patch Rejecting attachment 111309 [details] from commit-queue. New failing tests: plugins/access-after-page-destroyed.html Full output: http://queues.webkit.org/results/10116121
Comment on attachment 111309 [details] Patch doesn't fail locally, try again
Comment on attachment 111309 [details] Patch Clearing flags on attachment: 111309 Committed r97716: <http://trac.webkit.org/changeset/97716>
All reviewed patches have been landed. Closing bug.
Follow-up change: http://trac.webkit.org/changeset/97722
(In reply to comment #29) > Follow-up change: http://trac.webkit.org/changeset/97722 doh thx for fixing
Comment on attachment 111309 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111309&action=review > LayoutTests/ChangeLog:10 > + * platform/chromium/fast/loader/create-view-target-blank-expected.txt: Added. > + * platform/chromium/fast/loader/create-view-target-blank.html: Added. > + * platform/chromium/fast/loader/resources/done.html: Added. Why is this a Chromium test? This looks like a completely cross-port thing to test.
(In reply to comment #31) > (From update of attachment 111309 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=111309&action=review > > > LayoutTests/ChangeLog:10 > > + * platform/chromium/fast/loader/create-view-target-blank-expected.txt: Added. > > + * platform/chromium/fast/loader/create-view-target-blank.html: Added. > > + * platform/chromium/fast/loader/resources/done.html: Added. > > Why is this a Chromium test? This looks like a completely cross-port thing to test. it requires changing the plumbing of all other ports to pass through the navigation action's resource request. e.g. wk2 creates a new frame load request (with an empty resource request) to open the page.
(In reply to comment #32) > (In reply to comment #31) > > (From update of attachment 111309 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=111309&action=review > > > > > LayoutTests/ChangeLog:10 > > > + * platform/chromium/fast/loader/create-view-target-blank-expected.txt: Added. > > > + * platform/chromium/fast/loader/create-view-target-blank.html: Added. > > > + * platform/chromium/fast/loader/resources/done.html: Added. > > > > Why is this a Chromium test? This looks like a completely cross-port thing to test. > > it requires changing the plumbing of all other ports to pass through the navigation action's resource request. e.g. wk2 creates a new frame load request (with an empty resource request) to open the page. I understand the patch does that. But why is the test Chromium specific?