WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
68803
Make NavigationAction wrap a ResourceRequest instead of a KURL.
https://bugs.webkit.org/show_bug.cgi?id=68803
Summary
Make NavigationAction wrap a ResourceRequest instead of a KURL.
jochen
Reported
2011-09-26 08:03:30 PDT
When asking the FrameLoaderClient to create a new page, pass along the ResourceRequest we intend to use to load that frame
Attachments
Patch
(25.22 KB, patch)
2011-09-26 08:04 PDT
,
jochen
no flags
Details
Formatted Diff
Diff
Patch
(1.62 KB, patch)
2011-09-27 06:18 PDT
,
jochen
no flags
Details
Formatted Diff
Diff
Patch
(15.39 KB, patch)
2011-10-13 12:22 PDT
,
jochen
no flags
Details
Formatted Diff
Diff
Patch
(23.87 KB, patch)
2011-10-14 11:34 PDT
,
jochen
no flags
Details
Formatted Diff
Diff
Patch
(23.88 KB, patch)
2011-10-14 16:55 PDT
,
jochen
no flags
Details
Formatted Diff
Diff
Patch
(25.19 KB, patch)
2011-10-17 13:21 PDT
,
jochen
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
jochen
Comment 1
2011-09-26 08:04:42 PDT
Created
attachment 108668
[details]
Patch
Darin Fisher (:fishd, Google)
Comment 2
2011-09-26 11:01:28 PDT
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?
jochen
Comment 3
2011-09-26 11:08:06 PDT
(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
Comment 4
2011-09-26 20:36:41 PDT
Could you please provide a rationale for this change? It seems confusing to pass arguments with overlapping data.
jochen
Comment 5
2011-09-27 06:03:02 PDT
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
jochen
Comment 6
2011-09-27 06:18:39 PDT
Created
attachment 108835
[details]
Patch
Darin Fisher (:fishd, Google)
Comment 7
2011-10-13 10:18:32 PDT
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?
jochen
Comment 8
2011-10-13 12:22:51 PDT
Created
attachment 110887
[details]
Patch
Darin Fisher (:fishd, Google)
Comment 9
2011-10-13 13:03:48 PDT
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)
Comment 10
2011-10-13 13:04:30 PDT
Nate and Adam may also have opinions about this approach. I'm supportive as I think this leads to a cleaner solution.
Adam Barth
Comment 11
2011-10-13 13:08:03 PDT
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 12
2011-10-13 13:10:51 PDT
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 13
2011-10-13 13:38:28 PDT
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 14
2011-10-13 13:39:04 PDT
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
jochen
Comment 15
2011-10-14 11:34:00 PDT
Created
attachment 111034
[details]
Patch
jochen
Comment 16
2011-10-14 11:35:39 PDT
(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 17
2011-10-14 12:38:14 PDT
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
jochen
Comment 18
2011-10-14 16:55:04 PDT
Created
attachment 111109
[details]
Patch
jochen
Comment 19
2011-10-14 16:56:02 PDT
Now without layout tests failures (I hope)
Daniel Bates
Comment 20
2011-10-16 22:05:36 PDT
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 21
2011-10-17 10:34:10 PDT
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 :(
jochen
Comment 22
2011-10-17 10:38:16 PDT
(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 23
2011-10-17 10:45:26 PDT
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.
jochen
Comment 24
2011-10-17 13:21:47 PDT
Created
attachment 111309
[details]
Patch
WebKit Review Bot
Comment 25
2011-10-17 20:21:00 PDT
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
jochen
Comment 26
2011-10-17 20:44:17 PDT
Comment on
attachment 111309
[details]
Patch doesn't fail locally, try again
WebKit Review Bot
Comment 27
2011-10-17 21:46:15 PDT
Comment on
attachment 111309
[details]
Patch Clearing flags on attachment: 111309 Committed
r97716
: <
http://trac.webkit.org/changeset/97716
>
WebKit Review Bot
Comment 28
2011-10-17 21:46:22 PDT
All reviewed patches have been landed. Closing bug.
Kent Tamura
Comment 29
2011-10-17 23:23:26 PDT
Follow-up change:
http://trac.webkit.org/changeset/97722
jochen
Comment 30
2011-10-18 06:42:45 PDT
(In reply to
comment #29
)
> Follow-up change:
http://trac.webkit.org/changeset/97722
doh thx for fixing
Benjamin Poulain
Comment 31
2013-04-02 14:00:05 PDT
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.
jochen
Comment 32
2013-04-02 14:28:48 PDT
(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
Comment 33
2013-04-02 14:58:24 PDT
(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?
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug