Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1153193011:
PlzNavigate: send Javascript urls synchronously to the renderer (Closed)
|
Created:
5 years, 6 months ago by clamy Modified:
5 years, 6 months ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, creis+watch_chromium.org, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: send Javascript urls synchronously to the renderer
This CL makes it so that Javascripts url are properly sent synchornously to the
renderer with browser-side navigation enabled. This fixes the Bookmarklet
browser tests when run with browser-side navigation enabled.
BUG=475027
Committed: https://crrev.com/656cc77073b71ae85bda0dd8a014d3fc7f6a85ec
Cr-Commit-Position: refs/heads/master@{#332393}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Addressed comments #Patch Set 3 : #Patch Set 4 : Updated unit test now that data urls are sent committed synchronously #
Messages
Total messages: 18 (6 generated)
clamy@chromium.org changed reviewers: + carlosk@chromium.org, fdegans@chromium.org, nasko@chromium.org
@nasko: PTAL @carlosk, fdegans: FYI
Thanks! https://codereview.chromium.org/1153193011/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1153193011/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:823: nit: empty line here https://codereview.chromium.org/1153193011/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:832: frame_tree_node->current_frame_host()->DispatchBeforeUnload(true); nit: indent is wrong.
https://codereview.chromium.org/1153193011/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1153193011/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree_node.cc:191: url::kJavaScriptScheme)) Why not keep it closer to existing functionality? The loading state is explicitly set in RFH::Navigate - https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr.... https://codereview.chromium.org/1153193011/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1153193011/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:830: if (NavigationRequest::ShouldMakeNetworkRequest( In this particular codepath, it isn't really about whether a network request must be made or not. It is more about whether the navigation can go cross-process. If it can, then we need to run the beforeunload handler. It kind of fits nicely with the network request bit, but maybe a comment might be worthwhile. https://codereview.chromium.org/1153193011/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:834: BeginNavigation(frame_tree_node); Why did data URLs work without this?
Thanks! https://codereview.chromium.org/1153193011/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1153193011/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree_node.cc:191: url::kJavaScriptScheme)) On 2015/05/29 14:17:46, nasko wrote: > Why not keep it closer to existing functionality? The loading state is > explicitly set in RFH::Navigate - > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr.... > We cannot use a RFH to set the loading state as we don't have one until the navigation is ready to commit, and we need to be aware we are loading before that. So we're using the NavigationRequest until the navigation has committed. I have added a comment as to why this specific case about JavaScript urls is needed. https://codereview.chromium.org/1153193011/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1153193011/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:823: On 2015/05/29 12:37:08, Fabrice wrote: > nit: empty line here Done. https://codereview.chromium.org/1153193011/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:830: if (NavigationRequest::ShouldMakeNetworkRequest( On 2015/05/29 14:17:46, nasko wrote: > In this particular codepath, it isn't really about whether a network request > must be made or not. It is more about whether the navigation can go > cross-process. If it can, then we need to run the beforeunload handler. It kind > of fits nicely with the network request bit, but maybe a comment might be > worthwhile. Updated the comment. https://codereview.chromium.org/1153193011/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:832: frame_tree_node->current_frame_host()->DispatchBeforeUnload(true); On 2015/05/29 12:37:08, Fabrice wrote: > nit: indent is wrong. Done. https://codereview.chromium.org/1153193011/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:834: BeginNavigation(frame_tree_node); On 2015/05/29 14:17:46, nasko wrote: > Why did data URLs work without this? Apparently no test tries to do a browser-intiated navigation to a data url. In fact, I think content_shell won't even navigate to a data url that is typed in its address bar.
https://codereview.chromium.org/1153193011/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1153193011/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree_node.cc:191: url::kJavaScriptScheme)) On 2015/05/29 16:02:06, clamy wrote: > On 2015/05/29 14:17:46, nasko wrote: > > Why not keep it closer to existing functionality? The loading state is > > explicitly set in RFH::Navigate - > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr.... > > > > We cannot use a RFH to set the loading state as we don't have one until the > navigation is ready to commit, and we need to be aware we are loading before > that. So we're using the NavigationRequest until the navigation has committed. I > have added a comment as to why this specific case about JavaScript urls is > needed. Do we even create NavigationRequest objects for JS URLs? Purely on code flow, if there is navigation_request_ for JS URL, we will fall back to current_frame_host->is_loading(). What happens if we don't have a current host - new tab that is going to load a JS URL? https://codereview.chromium.org/1153193011/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1153193011/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:834: BeginNavigation(frame_tree_node); On 2015/05/29 16:02:06, clamy wrote: > On 2015/05/29 14:17:46, nasko wrote: > > Why did data URLs work without this? > > Apparently no test tries to do a browser-intiated navigation to a data url. In > fact, I think content_shell won't even navigate to a data url that is typed in > its address bar. Maybe we should add one : ). I don't see a reason why we can't have data URLs in the omnibox, especially as it definitely works in regular Chrome. Try it : ).
Patchset #3 (id:40001) has been deleted
Thanks! PTAL at the lastest patchset. https://codereview.chromium.org/1153193011/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1153193011/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree_node.cc:191: url::kJavaScriptScheme)) On 2015/06/01 20:38:22, nasko wrote: > On 2015/05/29 16:02:06, clamy wrote: > > On 2015/05/29 14:17:46, nasko wrote: > > > Why not keep it closer to existing functionality? The loading state is > > > explicitly set in RFH::Navigate - > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr.... > > > > > > > We cannot use a RFH to set the loading state as we don't have one until the > > navigation is ready to commit, and we need to be aware we are loading before > > that. So we're using the NavigationRequest until the navigation has committed. > I > > have added a comment as to why this specific case about JavaScript urls is > > needed. > > Do we even create NavigationRequest objects for JS URLs? Purely on code flow, if > there is navigation_request_ for JS URL, we will fall back to > current_frame_host->is_loading(). What happens if we don't have a current host - > new tab that is going to load a JS URL? After further thinking, I removed this part, and now reset the NavigationRequest in NavigatorImpl::CommitNavigation, since we are not expecting a commit from a Javascript URL. This seems better and solve the loading state issue if the Javascript URL triggers a new navigation. https://codereview.chromium.org/1153193011/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1153193011/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:834: BeginNavigation(frame_tree_node); On 2015/06/01 20:38:22, nasko wrote: > On 2015/05/29 16:02:06, clamy wrote: > > On 2015/05/29 14:17:46, nasko wrote: > > > Why did data URLs work without this? > > > > Apparently no test tries to do a browser-intiated navigation to a data url. In > > fact, I think content_shell won't even navigate to a data url that is typed in > > its address bar. > > Maybe we should add one : ). I don't see a reason why we can't have data URLs in > the omnibox, especially as it definitely works in regular Chrome. Try it : ). So after further investigation, it appears that we do currently support navigation to data urls (see BookmarkletTest::NavigateToStartPage for test support), however they are not synchronous in the current implementation, nor are they even with this patch in PlzNavigate. I suspect this is due to something in Blink's implementation of LoadDataURL.
Thanks! This looks much nicer! LGTM
The CQ bit was checked by clamy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1153193011/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by clamy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org Link to the patchset: https://codereview.chromium.org/1153193011/#ps80001 (title: "Updated unit test now that data urls are sent committed synchronously")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1153193011/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/656cc77073b71ae85bda0dd8a014d3fc7f6a85ec Cr-Commit-Position: refs/heads/master@{#332393} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
