Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2549373004:
PlzNavigate: Call NavigationHandle::WillProcessResponse for 204/205s (Closed)
|
Created:
4 years ago by clamy Modified:
3 years, 12 months ago CC:
chromium-reviews, creis+watch_chromium.org, nasko+codewatch_chromium.org, Randy Smith (Not in Mondays), darin-cc_chromium.org, loading-reviews_chromium.org, mmenke Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: Call NavigationHandle::WillProcessResponse for 204/205s
This CL ensures that NavigationHandle::WillProcessResponse wil be called
for 204/205s and downloads. It also ensures that no renderer process
will be created to commit the navigation (since there is nothing to
commit), and that NavigationHandle::ReadyToCommit will not be called in
that case.
BUG=652767
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/2fc22f544753c756cd50786c8a03c2b73f5b1817
Cr-Commit-Position: refs/heads/master@{#440646}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Rebase #Patch Set 3 : Addressed comments #Patch Set 4 : Fixed issue with wrong function modified #Patch Set 5 : Rebase #Patch Set 6 : Rebase #Patch Set 7 : Fixed test issue #
Total comments: 4
Patch Set 8 : Rebase + addressed comments #
Messages
Total messages: 40 (28 generated)
Description was changed from ========== PlzNavigate: Call NavigationHandle::WillProcessResponse for 204/205s This CL ensures that NavigationHandle::WillProcessResponse wil be called for 204/205s and downloads. It also ensures that no renderer process will be created to commit the navigation (since there is nothing to commit), and that NavigationHandle::ReadyToCommit will not be called in that case. BUG=652767 ========== to ========== PlzNavigate: Call NavigationHandle::WillProcessResponse for 204/205s This CL ensures that NavigationHandle::WillProcessResponse wil be called for 204/205s and downloads. It also ensures that no renderer process will be created to commit the navigation (since there is nothing to commit), and that NavigationHandle::ReadyToCommit will not be called in that case. BUG=652767 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by clamy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== PlzNavigate: Call NavigationHandle::WillProcessResponse for 204/205s This CL ensures that NavigationHandle::WillProcessResponse wil be called for 204/205s and downloads. It also ensures that no renderer process will be created to commit the navigation (since there is nothing to commit), and that NavigationHandle::ReadyToCommit will not be called in that case. BUG=652767 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: Call NavigationHandle::WillProcessResponse for 204/205s This CL ensures that NavigationHandle::WillProcessResponse wil be called for 204/205s and downloads. It also ensures that no renderer process will be created to commit the navigation (since there is nothing to commit), and that NavigationHandle::ReadyToCommit will not be called in that case. BUG=652767 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
clamy@chromium.org changed reviewers: + nasko@chromium.org, yzshen@chromium.org
@nasko, yzshen: PTAL Following yesterday's discussion, I tried to look at ways to make NavigationHandle go through the WillProcessResponse calls for 204/205 & downloads without creating a process we won't need. (Note: this is based on yzhen's patch at https://codereview.chromium.org/2510083003/). This is an alternative to merging the NavigationRequest and NavigationHandle which might be a bit hard before PlzNavigate launches. https://codereview.chromium.org/2549373004/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2549373004/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:416: RenderFrameHostImpl* render_frame_host = nullptr; Note that this will mean having a null RFH if NavigationHandle::GetRenderFrameHost is called during WillProcessResponse. I considered the alternative of passing the current RFH, but I think this is less error-prone for users of the API.
https://codereview.chromium.org/2549373004/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2549373004/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:580: // TODO(clamy): distinguish between CANCEL and CANCEL_AND_IGNORE. Do we need to call frame_tree_node_->navigator()->DiscardPendingEntryIfNeeded() for 204/etc.? If I recall correctly, if we don't do that the url of 204/etc. will remain in the address bar. https://codereview.chromium.org/2549373004/diff/1/content/browser/loader/navi... File content/browser/loader/navigation_resource_handler.cc (right): https://codereview.chromium.org/2549373004/diff/1/content/browser/loader/navi... content/browser/loader/navigation_resource_handler.cc:144: return true; This if-block doesn't seem necessary, we return true at line 146 anyway.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2549373004/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2549373004/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:416: RenderFrameHostImpl* render_frame_host = nullptr; On 2016/12/06 17:01:58, clamy wrote: > Note that this will mean having a null RFH if > NavigationHandle::GetRenderFrameHost is called during WillProcessResponse. I > considered the alternative of passing the current RFH, but I think this is less > error-prone for users of the API. Will that be the case only during WillProcessResponse? During WCO::DidFinishNavigation the correct (also current) RFH will be returned, right? https://codereview.chromium.org/2549373004/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:575: // Abort the request if needed. This include requests that were blocked by nit: includes https://codereview.chromium.org/2549373004/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:576: // NavigationThrottles and requests that should not commit (eg downloads, nit.: e.g.
The CQ bit was checked by clamy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.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 to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by clamy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
@nasko, yzshen: PTAL (note: there's still an issue with one fo the content_unittests, I'll have a look at it in the next patchset). https://codereview.chromium.org/2549373004/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2549373004/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:416: RenderFrameHostImpl* render_frame_host = nullptr; On 2016/12/08 18:25:24, nasko (very slow til Dec 13th) wrote: > On 2016/12/06 17:01:58, clamy wrote: > > Note that this will mean having a null RFH if > > NavigationHandle::GetRenderFrameHost is called during WillProcessResponse. I > > considered the alternative of passing the current RFH, but I think this is > less > > error-prone for users of the API. > > Will that be the case only during WillProcessResponse? During > WCO::DidFinishNavigation the correct (also current) RFH will be returned, right? No, we won't set the RFH to current since there will be no commit. https://codereview.chromium.org/2549373004/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:575: // Abort the request if needed. This include requests that were blocked by On 2016/12/08 18:25:24, nasko (very slow til Dec 13th) wrote: > nit: includes Done. https://codereview.chromium.org/2549373004/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:576: // NavigationThrottles and requests that should not commit (eg downloads, On 2016/12/08 18:25:24, nasko (very slow til Dec 13th) wrote: > nit.: e.g. Done. https://codereview.chromium.org/2549373004/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:580: // TODO(clamy): distinguish between CANCEL and CANCEL_AND_IGNORE. On 2016/12/06 17:57:37, yzshen1 wrote: > Do we need to call frame_tree_node_->navigator()->DiscardPendingEntryIfNeeded() > for 204/etc.? > > If I recall correctly, if we don't do that the url of 204/etc. will remain in > the address bar. > Done. https://codereview.chromium.org/2549373004/diff/1/content/browser/loader/navi... File content/browser/loader/navigation_resource_handler.cc (right): https://codereview.chromium.org/2549373004/diff/1/content/browser/loader/navi... content/browser/loader/navigation_resource_handler.cc:144: return true; On 2016/12/06 17:57:37, yzshen1 wrote: > This if-block doesn't seem necessary, we return true at line 146 anyway. Done. I've also removed the comment & TODO since I don't think they make sense anymore.
LGTM
The CQ bit was checked by clamy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
@nasko: PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
FWIW this lgtm
I have one question about downloads, otherwise LGTM. https://codereview.chromium.org/2549373004/diff/120001/content/browser/frame_... File content/browser/frame_host/navigation_request.h (right): https://codereview.chromium.org/2549373004/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_request.h:229: // Whether the navigation should be sent to a renderer. This is true, except nit: s/renderer/renderer process/ https://codereview.chromium.org/2549373004/diff/120001/content/browser/loader... File content/browser/loader/navigation_resource_handler.cc (right): https://codereview.chromium.org/2549373004/diff/120001/content/browser/loader... content/browser/loader/navigation_resource_handler.cc:132: // NavigationThrottle checks be performed. Why wouldn't we want downloads to go through the throttle checks? Explicit downloads won't hit them, but a navigation that turns into a download probably should. Does anything break if we defer downloads here?
Thanks! Note that I have removed the testing filter updates from the latest patchset so that this can go through the SiteIsolation bot where one test is failing (independently of this patch). The test filter can be updated in a later patchset. https://codereview.chromium.org/2549373004/diff/120001/content/browser/frame_... File content/browser/frame_host/navigation_request.h (right): https://codereview.chromium.org/2549373004/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_request.h:229: // Whether the navigation should be sent to a renderer. This is true, except On 2016/12/22 18:16:09, nasko wrote: > nit: s/renderer/renderer process/ Done. https://codereview.chromium.org/2549373004/diff/120001/content/browser/loader... File content/browser/loader/navigation_resource_handler.cc (right): https://codereview.chromium.org/2549373004/diff/120001/content/browser/loader... content/browser/loader/navigation_resource_handler.cc:132: // NavigationThrottle checks be performed. On 2016/12/22 18:16:09, nasko wrote: > Why wouldn't we want downloads to go through the throttle checks? Explicit > downloads won't hit them, but a navigation that turns into a download probably > should. Does anything break if we defer downloads here? That doesn't work because of the InterceptingResourceHandler as mentionned above. This will be fixed when I split this in two, with the part doing the checks moved before the InterceptingResourceHandler. Clarified the TODO.
The CQ bit was checked by clamy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org, jam@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2549373004/#ps140001 (title: "Rebase + addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1482512085079120,
"parent_rev": "b0eac54712ca8fff9764923fd2b604f1034905ff", "commit_rev":
"0cc017c25a1b15a60cd89ecf6b684409b3e7d5b0"}
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: Call NavigationHandle::WillProcessResponse for 204/205s This CL ensures that NavigationHandle::WillProcessResponse wil be called for 204/205s and downloads. It also ensures that no renderer process will be created to commit the navigation (since there is nothing to commit), and that NavigationHandle::ReadyToCommit will not be called in that case. BUG=652767 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: Call NavigationHandle::WillProcessResponse for 204/205s This CL ensures that NavigationHandle::WillProcessResponse wil be called for 204/205s and downloads. It also ensures that no renderer process will be created to commit the navigation (since there is nothing to commit), and that NavigationHandle::ReadyToCommit will not be called in that case. BUG=652767 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2549373004 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: Call NavigationHandle::WillProcessResponse for 204/205s This CL ensures that NavigationHandle::WillProcessResponse wil be called for 204/205s and downloads. It also ensures that no renderer process will be created to commit the navigation (since there is nothing to commit), and that NavigationHandle::ReadyToCommit will not be called in that case. BUG=652767 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2549373004 ========== to ========== PlzNavigate: Call NavigationHandle::WillProcessResponse for 204/205s This CL ensures that NavigationHandle::WillProcessResponse wil be called for 204/205s and downloads. It also ensures that no renderer process will be created to commit the navigation (since there is nothing to commit), and that NavigationHandle::ReadyToCommit will not be called in that case. BUG=652767 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/2fc22f544753c756cd50786c8a03c2b73f5b1817 Cr-Commit-Position: refs/heads/master@{#440646} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/2fc22f544753c756cd50786c8a03c2b73f5b1817 Cr-Commit-Position: refs/heads/master@{#440646} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
