Bug 68803 – Make NavigationAction wrap a ResourceRequest instead of a KURL.

• Component: New Bugs
• Status: RESOLVED FIXED

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

[Darin Fisher (:fishd, Google)]

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

[Alexey Proskuryakov]

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

[Darin Fisher (:fishd, Google)]

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

[Darin Fisher (:fishd, Google)]

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?

[Darin Fisher (:fishd, Google)]

Nate and Adam may also have opinions about this approach.  I'm supportive as I think this leads to a cleaner solution.

[Adam Barth]

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.

[Darin Fisher (:fishd, Google)]

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.

[Nate Chapin]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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)

[Daniel Bates]

Comment on attachment 111109 [details]
Patch

Attachment 111109 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/10077816

[Nate Chapin]

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

[Nate Chapin]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

Comment on attachment 111309 [details]
Patch

Clearing flags on attachment: 111309

Committed r97716: <http://trac.webkit.org/changeset/97716>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

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

[Benjamin Poulain]

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.

[Benjamin Poulain]

(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?