Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2628593002:
PlzNavigate: check report_raw_headers based on the main frame (Closed)
DescriptionPlzNavigate: check report_raw_headers based on the main frame
Currently we attempt to query
RenderFrameDevToolsAgentHost::IsNetworkHandlerEnabled based on the frame
that is navigating. However, there may not be a
RenderFrameDevToolsAgentHost for a subframe that is navigating even
though there is one for the top-level frame, where the NetworkHandler is
active. This prevents from properly reporting headers in subframe
navigations.
BUG=551000
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2628593002
Cr-Commit-Position: refs/heads/master@{#442961}
Committed: https://chromium.googlesource.com/chromium/src/+/b907305177b8605893e4c6c0f46226ecaacd93be
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebase + addressed comments #
Total comments: 2
Messages
Total messages: 21 (14 generated)
Description was changed from ========== PlzNavigate: check report_raw_headers based on the main frame Currently we attempt to query RenderFrameDevToolsAgentHost::IsNetworkHandlerEnabled based on the frame that is navigating. However, there may not be a RenderFrameDevToolsAgentHost for a subframe that is navigating even though there is one for the top-level frame, where the NetworkHandler is active. This prevents from properly reporting headers in subframe navigations. BUG=551000 ========== to ========== PlzNavigate: check report_raw_headers based on the main frame Currently we attempt to query RenderFrameDevToolsAgentHost::IsNetworkHandlerEnabled based on the frame that is navigating. However, there may not be a RenderFrameDevToolsAgentHost for a subframe that is navigating even though there is one for the top-level frame, where the NetworkHandler is active. This prevents from properly reporting headers in subframe navigations. BUG=551000 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: check report_raw_headers based on the main frame Currently we attempt to query RenderFrameDevToolsAgentHost::IsNetworkHandlerEnabled based on the frame that is navigating. However, there may not be a RenderFrameDevToolsAgentHost for a subframe that is navigating even though there is one for the top-level frame, where the NetworkHandler is active. This prevents from properly reporting headers in subframe navigations. BUG=551000 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: check report_raw_headers based on the main frame Currently we attempt to query RenderFrameDevToolsAgentHost::IsNetworkHandlerEnabled based on the frame that is navigating. However, there may not be a RenderFrameDevToolsAgentHost for a subframe that is navigating even though there is one for the top-level frame, where the NetworkHandler is active. This prevents from properly reporting headers in subframe navigations. BUG=551000 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
clamy@chromium.org changed reviewers: + dgozman@chromium.org
@dgozman: PTAL
https://codereview.chromium.org/2628593002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2628593002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:638: frame_tree_node_->frame_tree()->root()); In OOPIF world, this is not necessarily the main frame. We should instead alter RFDTAH::IsNetworkHandlerEnabled to climb up the hierarchy. Something like this: https://cs.chromium.org/chromium/src/content/browser/devtools/render_frame_de...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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...
Thanks! https://codereview.chromium.org/2628593002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2628593002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:638: frame_tree_node_->frame_tree()->root()); On 2017/01/10 17:36:31, dgozman wrote: > In OOPIF world, this is not necessarily the main frame. We should instead alter > RFDTAH::IsNetworkHandlerEnabled to climb up the hierarchy. Something like this: > https://cs.chromium.org/chromium/src/content/browser/devtools/render_frame_de... Ok I wasn't sure if I needed to take the top level frame or do a walk of the frame tree. I've opted to add this to RenderFrameDevToolsAgentHost instead, since this seems to be a detail of the DevTools implementation. https://codereview.chromium.org/2628593002/diff/20001/content/browser/devtool... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2628593002/diff/20001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:96: return ShouldCreateDevToolsFor(ftn->current_frame_host()); I'm not sure if we should check for the Speculative/Pending RFH here as well.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm. Thanks! https://codereview.chromium.org/2628593002/diff/20001/content/browser/devtool... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2628593002/diff/20001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:96: return ShouldCreateDevToolsFor(ftn->current_frame_host()); On 2017/01/11 13:47:29, clamy wrote: > I'm not sure if we should check for the Speculative/Pending RFH here as well. Good question. Let's not for now - I hope PlzNavigate will make this a bit easier in the future.
Thanks!
The CQ bit was checked by clamy@chromium.org
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": 20001, "attempt_start_ts": 1484159233070530,
"parent_rev": "ff8ffc7fe93a0348148eea2221b093feb711da9a", "commit_rev":
"b907305177b8605893e4c6c0f46226ecaacd93be"}
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: check report_raw_headers based on the main frame Currently we attempt to query RenderFrameDevToolsAgentHost::IsNetworkHandlerEnabled based on the frame that is navigating. However, there may not be a RenderFrameDevToolsAgentHost for a subframe that is navigating even though there is one for the top-level frame, where the NetworkHandler is active. This prevents from properly reporting headers in subframe navigations. BUG=551000 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: check report_raw_headers based on the main frame Currently we attempt to query RenderFrameDevToolsAgentHost::IsNetworkHandlerEnabled based on the frame that is navigating. However, there may not be a RenderFrameDevToolsAgentHost for a subframe that is navigating even though there is one for the top-level frame, where the NetworkHandler is active. This prevents from properly reporting headers in subframe navigations. BUG=551000 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2628593002 Cr-Commit-Position: refs/heads/master@{#442961} Committed: https://chromium.googlesource.com/chromium/src/+/b907305177b8605893e4c6c0f462... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/b907305177b8605893e4c6c0f462... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
