Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2132673002:
Adding Navigation Throttles to DevTools (Closed)
|
Created:
4 years, 5 months ago by alex clarke (OOO till 29th) Modified:
4 years, 5 months ago CC:
chromium-reviews, caseq+blink_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, darin-cc_chromium.org, devtools-reviews_chromium.org, blink-reviews, apavlov+blink_chromium.org, sergeyv+blink_chromium.org, pfeldman, kozyatinskiy+blink_chromium.org, clamy Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAadding Navigation Throttles to devtools
Extends the devtools Page domain with commands to generate and
manage Navigation Throttles. This functionality will be useful
for WebDriver style test frameworks and is needed for Headless
Chrome.
If enabled by Page.setControlNavigations a Page.navigationRequested
event will be fired if the main frame or a child frame attempts to
navigate, or it receives a server side redirect. The navigation will
be deferred until a corresponding Page.processNavigation command is sent
in response.
BUG=546953
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/f33b4f2e7962249e7b9f31b5e8a74e66059ddd1f
Cr-Commit-Position: refs/heads/master@{#405866}
Patch Set 1 #Patch Set 2 : Add missing files #
Total comments: 22
Patch Set 3 : Rename thing plus make Page.disable deal with any in flight throttles #
Total comments: 4
Patch Set 4 : Removed frameID #
Total comments: 12
Patch Set 5 : Changes for dgozman #
Total comments: 4
Patch Set 6 : Walk up frame tree nodes #
Total comments: 6
Patch Set 7 : Moved RegisterNavigationThrottles #
Total comments: 12
Patch Set 8 : changes for Nasko #
Total comments: 5
Patch Set 9 : Renamed Page.setNavigationThrottleEnabled to Page.setControlNavigations #
Total comments: 24
Patch Set 10 : Changes for Nasko #Patch Set 11 : Reduce number of delta lines in DevToolsProtocolTest #
Total comments: 14
Patch Set 12 : Remove rendererInitiated #Patch Set 13 : Sent that last patchset out too soon. #Patch Set 14 : Got browser test to work with IsolateAllSitesForTesting #
Total comments: 2
Patch Set 15 : Remove print #Messages
Total messages: 127 (74 generated)
Description was changed from ========== Prototype adding Navigation Throttles to devtools Extends the devtools Page domain with commands to generate and manage Navigation Throttles. This functionality will be useful for WebDriver style test frameworks and is needed for Headless Chrome. BUG=546953 ========== to ========== Prototype adding Navigation Throttles to devtools Extends the devtools Page domain with commands to generate and manage Navigation Throttles. This functionality will be useful for WebDriver style test frameworks and is needed for Headless Chrome. BUG=546953 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Description was changed from ========== Prototype adding Navigation Throttles to devtools Extends the devtools Page domain with commands to generate and manage Navigation Throttles. This functionality will be useful for WebDriver style test frameworks and is needed for Headless Chrome. BUG=546953 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Prototype adding Navigation Throttles to devtools Extends the devtools Page domain with commands to generate and manage Navigation Throttles. This functionality will be useful for WebDriver style test frameworks and is needed for Headless Chrome. BUG=546953 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
pfeldman@chromium.org changed reviewers: + dgozman@chromium.org, pfeldman@chromium.org
overall, this looks good. we need to figure out something with the ids. https://codereview.chromium.org/2132673002/diff/20001/content/browser/devtool... File content/browser/devtools/page_navigation_throttle.cc (right): https://codereview.chromium.org/2132673002/diff/20001/content/browser/devtool... content/browser/devtools/page_navigation_throttle.cc:56: navigation_handle()->GetFrameTreeNodeId()); The rest of the code uses IdentifiersFactory::frameId (which is not as good as yours), but unfortunately we don't have cross-cutting id system for frames in chrome yet. https://codereview.chromium.org/2132673002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2132673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:182: "enum": ["Proceed", "Cancel", "CancelAndIgnore"], We don't have guidelines for the enum literal naming, we should probably get some... https://codereview.chromium.org/2132673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:502: "name": "navigationThrottleResponse", processNavigation https://codereview.chromium.org/2132673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:643: "name": "shouldAllowNavigation", navigationRequested https://codereview.chromium.org/2132673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:654: "name": "shouldAllowRedirect", redirectRequested
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2132673002/diff/20001/content/browser/devtool... File content/browser/devtools/protocol/page_handler.cc (right): https://codereview.chromium.org/2132673002/diff/20001/content/browser/devtool... content/browser/devtools/protocol/page_handler.cc:109: navigation_throttle_enabled_(false), We should resume and remove any throttles we currently have in Disable(). And clear this flag. https://codereview.chromium.org/2132673002/diff/20001/content/browser/devtool... content/browser/devtools/protocol/page_handler.cc:130: host_ = host; I was about to say that we should clean up everything for old host here. But navigations are per frame tree node. This makes this class split between per-host and per-frametreenode parts. If you have any ideas on how to make it better, that would be nice! https://codereview.chromium.org/2132673002/diff/20001/content/browser/devtool... content/browser/devtools/protocol/page_handler.cc:380: navigation_throttle_enabled_ = enabled; Even better, clean up existing throttles here and call this with |false| from Disable(). https://codereview.chromium.org/2132673002/diff/20001/content/browser/devtool... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2132673002/diff/20001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:355: RenderFrameDevToolsAgentHost* agent_host = FindAgentHost(frame_tree_node); Do you want to throttle navigations in subframes as well?
https://codereview.chromium.org/2132673002/diff/20001/content/browser/devtool... File content/browser/devtools/page_navigation_throttle.cc (right): https://codereview.chromium.org/2132673002/diff/20001/content/browser/devtool... content/browser/devtools/page_navigation_throttle.cc:56: navigation_handle()->GetFrameTreeNodeId()); On 2016/07/07 17:26:18, pfeldman wrote: > The rest of the code uses IdentifiersFactory::frameId (which is not as good as > yours) FWIW I'm under the impression this gives the same result as IdentifiersFactory::frameId. >, but unfortunately we don't have cross-cutting id system for frames in > chrome yet. What should we do here? I had hoped for interoperability of frame identifiers :) https://codereview.chromium.org/2132673002/diff/20001/content/browser/devtool... File content/browser/devtools/protocol/page_handler.cc (right): https://codereview.chromium.org/2132673002/diff/20001/content/browser/devtool... content/browser/devtools/protocol/page_handler.cc:109: navigation_throttle_enabled_(false), On 2016/07/07 19:25:58, dgozman wrote: > We should resume and remove any throttles we currently have in Disable(). > And clear this flag. Good point! Done. https://codereview.chromium.org/2132673002/diff/20001/content/browser/devtool... content/browser/devtools/protocol/page_handler.cc:130: host_ = host; On 2016/07/07 19:25:58, dgozman wrote: > I was about to say that we should clean up everything for old host here. > But navigations are per frame tree node. This makes this class split between > per-host and per-frametreenode parts. If you have any ideas on how to make it > better, that would be nice! Acknowledged. I'll have to think about that. https://codereview.chromium.org/2132673002/diff/20001/content/browser/devtool... content/browser/devtools/protocol/page_handler.cc:380: navigation_throttle_enabled_ = enabled; On 2016/07/07 19:25:58, dgozman wrote: > Even better, clean up existing throttles here and call this with |false| from > Disable(). Done. https://codereview.chromium.org/2132673002/diff/20001/content/browser/devtool... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2132673002/diff/20001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:355: RenderFrameDevToolsAgentHost* agent_host = FindAgentHost(frame_tree_node); On 2016/07/07 19:25:58, dgozman wrote: > Do you want to throttle navigations in subframes as well? Yes we do :) Is it better to go via the WebContents? https://codereview.chromium.org/2132673002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2132673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:182: "enum": ["Proceed", "Cancel", "CancelAndIgnore"], On 2016/07/07 17:26:18, pfeldman wrote: > We don't have guidelines for the enum literal naming, we should probably get > some... Acknowledged. Happy to change these if you can think of clearer names. https://codereview.chromium.org/2132673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:502: "name": "navigationThrottleResponse", On 2016/07/07 17:26:18, pfeldman wrote: > processNavigation Done. https://codereview.chromium.org/2132673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:643: "name": "shouldAllowNavigation", On 2016/07/07 17:26:18, pfeldman wrote: > navigationRequested Done. https://codereview.chromium.org/2132673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:654: "name": "shouldAllowRedirect", On 2016/07/07 17:26:18, pfeldman wrote: > redirectRequested Done.
The CQ bit was checked by alexclarke@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2132673002/diff/20001/content/browser/devtool... File content/browser/devtools/page_navigation_throttle.cc (right): https://codereview.chromium.org/2132673002/diff/20001/content/browser/devtool... content/browser/devtools/page_navigation_throttle.cc:56: navigation_handle()->GetFrameTreeNodeId()); Are you still under that impression, even after my comment? :-) Fixing this is hard and Sami has the background for this. Do you absolutely need to pass frame ids at this point?
clamy@chromium.org changed reviewers: + clamy@chromium.org
A few comments on the NavigationHandle and the throttle code. https://codereview.chromium.org/2132673002/diff/40001/content/browser/devtool... File content/browser/devtools/page_navigation_throttle.cc (right): https://codereview.chromium.org/2132673002/diff/40001/content/browser/devtool... content/browser/devtools/page_navigation_throttle.cc:58: base::GetProcId(web_contents->GetRenderProcessHost()->GetHandle()), Does it work for cross-process navigations? https://codereview.chromium.org/2132673002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2132673002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:331: ScopedVector<NavigationThrottle> throttles_to_register = Considering that we're now adding more than one throttle, I'd prefer if this were put in a specific method.
https://codereview.chromium.org/2132673002/diff/20001/content/browser/devtool... File content/browser/devtools/page_navigation_throttle.cc (right): https://codereview.chromium.org/2132673002/diff/20001/content/browser/devtool... content/browser/devtools/page_navigation_throttle.cc:56: navigation_handle()->GetFrameTreeNodeId()); On 2016/07/08 15:33:14, pfeldman wrote: > Are you still under that impression, even after my comment? :-) > > Fixing this is hard and Sami has the background for this. Do you absolutely need > to pass frame ids at this point? Looking at some of the use cases we had in mind we can get away with knowing if the main frame was navigating or if it was a child frame. So I've gone ahead and removed this and added a couple of booleans to the events. https://codereview.chromium.org/2132673002/diff/40001/content/browser/devtool... File content/browser/devtools/page_navigation_throttle.cc (right): https://codereview.chromium.org/2132673002/diff/40001/content/browser/devtool... content/browser/devtools/page_navigation_throttle.cc:58: base::GetProcId(web_contents->GetRenderProcessHost()->GetHandle()), On 2016/07/08 15:47:33, clamy wrote: > Does it work for cross-process navigations? It appeared to, although I didn't test it thoroughly. I've removed it anyway. https://codereview.chromium.org/2132673002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2132673002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:331: ScopedVector<NavigationThrottle> throttles_to_register = On 2016/07/08 15:47:33, clamy wrote: > Considering that we're now adding more than one throttle, I'd prefer if this > were put in a specific method. I'm not 100% sure if I've done what you had in mind. I've pulled this whole section out into it's own function.
The CQ bit was checked by alexclarke@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2132673002/diff/20001/content/browser/devtool... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2132673002/diff/20001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:355: RenderFrameDevToolsAgentHost* agent_host = FindAgentHost(frame_tree_node); On 2016/07/08 10:37:58, alex clarke wrote: > On 2016/07/07 19:25:58, dgozman wrote: > > Do you want to throttle navigations in subframes as well? > > Yes we do :) Is it better to go via the WebContents? OOPIFs step in here: we should go up the tree to the local root. See how it's done here (but you'll have to use frame tree nodes I think): DevToolsAgentHost::GetOrCreateFor(RenderFrameHost* frame_host) https://codereview.chromium.org/2132673002/diff/60001/content/browser/devtool... File content/browser/devtools/protocol/page_handler.cc (right): https://codereview.chromium.org/2132673002/diff/60001/content/browser/devtool... content/browser/devtools/protocol/page_handler.cc:384: for (auto& pair : navigation_throttles_) { Clear navigation_throttles_ after this. https://codereview.chromium.org/2132673002/diff/60001/content/browser/devtool... content/browser/devtools/protocol/page_handler.cc:394: return Response::InternalError("Unknown navigation"); Response::InvalidParams("Unknown navigation id") https://codereview.chromium.org/2132673002/diff/60001/content/browser/devtool... content/browser/devtools/protocol/page_handler.cc:408: return Response::InternalError("Unrecognized response"); Response::InvalidParams("Unrecognized response") https://codereview.chromium.org/2132673002/diff/60001/content/browser/devtool... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2132673002/diff/60001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:356: return agent_host->page_handler_->GetThrottleForNavigation( Not that page_handler_ may be null for subframes DTAH. https://codereview.chromium.org/2132673002/diff/60001/content/child/blink_pla... File content/child/blink_platform_impl.cc (right): https://codereview.chromium.org/2132673002/diff/60001/content/child/blink_pla... content/child/blink_platform_impl.cc:983: // ready. Keep PageNavigationThrottle::GetFrameId in sync with this. Not relevant anymore. https://codereview.chromium.org/2132673002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2132673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:657: "parameters": [ These two events have exactly the same parameters. Should we merge them adding a boolean flag "isRedirect"? Or even not add a flag if we don't need it.
PTAL https://codereview.chromium.org/2132673002/diff/20001/content/browser/devtool... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2132673002/diff/20001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:355: RenderFrameDevToolsAgentHost* agent_host = FindAgentHost(frame_tree_node); On 2016/07/08 19:19:59, dgozman wrote: > On 2016/07/08 10:37:58, alex clarke wrote: > > On 2016/07/07 19:25:58, dgozman wrote: > > > Do you want to throttle navigations in subframes as well? > > > > Yes we do :) Is it better to go via the WebContents? > > OOPIFs step in here: we should go up the tree to the local root. See how it's > done here (but you'll have to use frame tree nodes I think): > DevToolsAgentHost::GetOrCreateFor(RenderFrameHost* frame_host) I'm not up to speed with these trees but it looks like it's possible to get the frame host and use the same logic from DevToolsAgentHost::GetOrCreateFor(RenderFrameHost* frame_host). https://codereview.chromium.org/2132673002/diff/60001/content/browser/devtool... File content/browser/devtools/protocol/page_handler.cc (right): https://codereview.chromium.org/2132673002/diff/60001/content/browser/devtool... content/browser/devtools/protocol/page_handler.cc:384: for (auto& pair : navigation_throttles_) { On 2016/07/08 19:19:59, dgozman wrote: > Clear navigation_throttles_ after this. Done. https://codereview.chromium.org/2132673002/diff/60001/content/browser/devtool... content/browser/devtools/protocol/page_handler.cc:394: return Response::InternalError("Unknown navigation"); On 2016/07/08 19:20:00, dgozman wrote: > Response::InvalidParams("Unknown navigation id") Done. https://codereview.chromium.org/2132673002/diff/60001/content/browser/devtool... content/browser/devtools/protocol/page_handler.cc:408: return Response::InternalError("Unrecognized response"); On 2016/07/08 19:19:59, dgozman wrote: > Response::InvalidParams("Unrecognized response") Done. https://codereview.chromium.org/2132673002/diff/60001/content/browser/devtool... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2132673002/diff/60001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:356: return agent_host->page_handler_->GetThrottleForNavigation( On 2016/07/08 19:20:00, dgozman wrote: > Not that page_handler_ may be null for subframes DTAH. Done. https://codereview.chromium.org/2132673002/diff/60001/content/child/blink_pla... File content/child/blink_platform_impl.cc (right): https://codereview.chromium.org/2132673002/diff/60001/content/child/blink_pla... content/child/blink_platform_impl.cc:983: // ready. Keep PageNavigationThrottle::GetFrameId in sync with this. On 2016/07/08 19:20:00, dgozman wrote: > Not relevant anymore. Done. https://codereview.chromium.org/2132673002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2132673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:657: "parameters": [ On 2016/07/08 19:20:00, dgozman wrote: > These two events have exactly the same parameters. Should we merge them adding a > boolean flag "isRedirect"? Or even not add a flag if we don't need it. Lets merge them and add a flag. Knowing it's a redirect is actually pretty useful.
The CQ bit was checked by alexclarke@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: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2132673002/diff/80001/content/browser/devtool... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2132673002/diff/80001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:355: RenderFrameHost* frame_host = frame_tree_node->current_frame_host(); Could there be no current frame host? Let's instead walk up the frame tree nodes? I'm not an expert in RFHs and FTNs though. https://codereview.chromium.org/2132673002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2132673002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:656: "name": "redirectRequested", Remove this one now as it's unused.
Description was changed from ========== Prototype adding Navigation Throttles to devtools Extends the devtools Page domain with commands to generate and manage Navigation Throttles. This functionality will be useful for WebDriver style test frameworks and is needed for Headless Chrome. BUG=546953 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Aadding Navigation Throttles to devtools Extends the devtools Page domain with commands to generate and manage Navigation Throttles. This functionality will be useful for WebDriver style test frameworks and is needed for Headless Chrome. BUG=546953 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
https://codereview.chromium.org/2132673002/diff/80001/content/browser/devtool... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2132673002/diff/80001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:355: RenderFrameHost* frame_host = frame_tree_node->current_frame_host(); On 2016/07/09 01:57:00, dgozman wrote: > Could there be no current frame host? Let's instead walk up the frame tree > nodes? I'm not an expert in RFHs and FTNs though. Done. https://codereview.chromium.org/2132673002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2132673002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:656: "name": "redirectRequested", On 2016/07/09 01:57:00, dgozman wrote: > Remove this one now as it's unused. Done.
The CQ bit was checked by alexclarke@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Thanks! https://codereview.chromium.org/2132673002/diff/100001/content/browser/devtoo... File content/browser/devtools/page_navigation_throttle.h (right): https://codereview.chromium.org/2132673002/diff/100001/content/browser/devtoo... content/browser/devtools/page_navigation_throttle.h:13: namespace devtools { We do not use use nested namespaces inside content/. https://codereview.chromium.org/2132673002/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2132673002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:337: void NavigationHandleImpl::RegisterNavigationThrottles() { Please ensure that the order of the functions in the .cc file matches the one in the .h file. https://codereview.chromium.org/2132673002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:349: std::unique_ptr<NavigationThrottle> devtools_throttle = Sanity check: this will mean that the devtools throttle will execute last. In particular, if any other throttle cancel the navigation it won't know about it. Is this WAI?
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
https://codereview.chromium.org/2132673002/diff/100001/content/browser/devtoo... File content/browser/devtools/page_navigation_throttle.h (right): https://codereview.chromium.org/2132673002/diff/100001/content/browser/devtoo... content/browser/devtools/page_navigation_throttle.h:13: namespace devtools { On 2016/07/11 14:20:36, clamy wrote: > We do not use use nested namespaces inside content/. It seems devtools code may be an exception? :) E.g. https://cs.chromium.org/chromium/src/content/browser/devtools/protocol/page_h... https://codereview.chromium.org/2132673002/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2132673002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:337: void NavigationHandleImpl::RegisterNavigationThrottles() { On 2016/07/11 14:20:36, clamy wrote: > Please ensure that the order of the functions in the .cc file matches the one in > the .h file. Done. https://codereview.chromium.org/2132673002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:349: std::unique_ptr<NavigationThrottle> devtools_throttle = On 2016/07/11 14:20:36, clamy wrote: > Sanity check: this will mean that the devtools throttle will execute last. In > particular, if any other throttle cancel the navigation it won't know about it. > Is this WAI? That is what I intended. My thoughts where we'd want to know about navigations that are /actually/ going to occur rather than ones that get cancelled by some other filter.
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_...)
The CQ bit was checked by alexclarke@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_...)
The CQ bit was checked by alexclarke@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...
Lgtm.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2132673002/#ps120001 (title: "Moved RegisterNavigationThrottles")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
nasko@chromium.org changed reviewers: + nasko@chromium.org
https://codereview.chromium.org/2132673002/diff/120001/content/browser/devtoo... File content/browser/devtools/page_navigation_throttle.cc (right): https://codereview.chromium.org/2132673002/diff/120001/content/browser/devtoo... content/browser/devtools/page_navigation_throttle.cc:50: return ThrottleCheckResult::PROCEED; Why do you need to override this one? It already has this as the default implementation. https://codereview.chromium.org/2132673002/diff/120001/content/browser/devtoo... File content/browser/devtools/page_navigation_throttle.h (right): https://codereview.chromium.org/2132673002/diff/120001/content/browser/devtoo... content/browser/devtools/page_navigation_throttle.h:18: class PageNavigationThrottle : public content::NavigationThrottle { Can you put a class level comment? What is this class being used for? https://codereview.chromium.org/2132673002/diff/120001/content/browser/devtoo... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2132673002/diff/120001/content/browser/devtoo... content/browser/devtools/render_frame_devtools_agent_host.cc:356: frame_tree_node = frame_tree_node->parent(); Is this code compatible with out-of-process iframes? Why is it always looking at the root node?
The CQ bit was unchecked by nasko@chromium.org
Description was changed from ========== Aadding Navigation Throttles to devtools Extends the devtools Page domain with commands to generate and manage Navigation Throttles. This functionality will be useful for WebDriver style test frameworks and is needed for Headless Chrome. BUG=546953 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Aadding Navigation Throttles to devtools Extends the devtools Page domain with commands to generate and manage Navigation Throttles. This functionality will be useful for WebDriver style test frameworks and is needed for Headless Chrome. If enabled by Page.setNavigationThrottleEnabled a Page.navigationRequested event will be fired if the main frame or a child frame attempts to navigate, or it receives a server side redirect. The navigation will be deferred until a corresponding Page.processNavigation command is sent in response. BUG=546953 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
PTAL https://codereview.chromium.org/2132673002/diff/120001/content/browser/devtoo... File content/browser/devtools/page_navigation_throttle.cc (right): https://codereview.chromium.org/2132673002/diff/120001/content/browser/devtoo... content/browser/devtools/page_navigation_throttle.cc:50: return ThrottleCheckResult::PROCEED; On 2016/07/12 18:03:11, nasko wrote: > Why do you need to override this one? It already has this as the default > implementation. Done. https://codereview.chromium.org/2132673002/diff/120001/content/browser/devtoo... File content/browser/devtools/page_navigation_throttle.h (right): https://codereview.chromium.org/2132673002/diff/120001/content/browser/devtoo... content/browser/devtools/page_navigation_throttle.h:18: class PageNavigationThrottle : public content::NavigationThrottle { On 2016/07/12 18:03:11, nasko wrote: > Can you put a class level comment? What is this class being used for? Done. https://codereview.chromium.org/2132673002/diff/120001/content/browser/devtoo... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2132673002/diff/120001/content/browser/devtoo... content/browser/devtools/render_frame_devtools_agent_host.cc:356: frame_tree_node = frame_tree_node->parent(); On 2016/07/12 18:03:11, nasko wrote: > Is this code compatible with out-of-process iframes? I'm not sure. I'm not up to speed with OOPIF. > Why is it always looking at > the root node? I'm assuming that's where the relevant RenderFrameDevToolsAgentHost is. The intended usage is request a throttle at the Page level which will send Page.navigationRequested events if the main frame or a child frame attempts to navigate.
The CQ bit was checked by alexclarke@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 to run a CQ dry run
Dry run: Try jobs failed on following builders: linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexclarke@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...
https://codereview.chromium.org/2132673002/diff/120001/content/browser/devtoo... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2132673002/diff/120001/content/browser/devtoo... content/browser/devtools/render_frame_devtools_agent_host.cc:356: frame_tree_node = frame_tree_node->parent(); On 2016/07/12 20:36:14, alex clarke wrote: > On 2016/07/12 18:03:11, nasko wrote: > > Is this code compatible with out-of-process iframes? > I'm not sure. I'm not up to speed with OOPIF. > > > Why is it always looking at > > the root node? > I'm assuming that's where the relevant RenderFrameDevToolsAgentHost is. The > intended usage is request a throttle at the Page level which will send > Page.navigationRequested events if the main frame or a child frame attempts to > navigate. > I'd suggest discussing more with dgozman@, as I'm not very fluent in DevTools, but my understanding is that RenderFrameDevToolsAgentHost can be attached to any frame in the tree. Therefore it seems more correct to me that the FindAgentHost(frame_tree_node) call is made within the while loop and exited once a DevTools agent is found. https://codereview.chromium.org/2132673002/diff/140001/content/browser/devtoo... File content/browser/devtools/page_navigation_throttle.h (right): https://codereview.chromium.org/2132673002/diff/140001/content/browser/devtoo... content/browser/devtools/page_navigation_throttle.h:19: // Page.setNavigationThrottleEnabled and Page.processNavigation commands. Is Page.setNavigationThrottleEnabled going to be a temporary API? It seems strange to expose implementation details at the DevTools protocol level.
https://codereview.chromium.org/2132673002/diff/120001/content/browser/devtoo... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2132673002/diff/120001/content/browser/devtoo... content/browser/devtools/render_frame_devtools_agent_host.cc:356: frame_tree_node = frame_tree_node->parent(); On 2016/07/12 23:08:21, nasko wrote: > On 2016/07/12 20:36:14, alex clarke wrote: > > On 2016/07/12 18:03:11, nasko wrote: > > > Is this code compatible with out-of-process iframes? > > I'm not sure. I'm not up to speed with OOPIF. > > > > > Why is it always looking at > > > the root node? > > I'm assuming that's where the relevant RenderFrameDevToolsAgentHost is. The > > intended usage is request a throttle at the Page level which will send > > Page.navigationRequested events if the main frame or a child frame attempts to > > navigate. > > > > I'd suggest discussing more with dgozman@, as I'm not very fluent in DevTools, > but my understanding is that RenderFrameDevToolsAgentHost can be attached to any > frame in the tree. Therefore it seems more correct to me that the > FindAgentHost(frame_tree_node) call is made within the while loop and exited > once a DevTools agent is found. I think this API only makes sense for the page as a whole, so going up to the root is good. https://codereview.chromium.org/2132673002/diff/140001/content/browser/devtoo... File content/browser/devtools/page_navigation_throttle.h (right): https://codereview.chromium.org/2132673002/diff/140001/content/browser/devtoo... content/browser/devtools/page_navigation_throttle.h:19: // Page.setNavigationThrottleEnabled and Page.processNavigation commands. On 2016/07/12 23:08:21, nasko wrote: > Is Page.setNavigationThrottleEnabled going to be a temporary API? It seems > strange to expose implementation details at the DevTools protocol level. I don't think this will be temporary. We can rename it to something like "Page.setControlNavigations", but the meaning would be the same.
The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2132673002/diff/120001/content/browser/devtoo... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2132673002/diff/120001/content/browser/devtoo... content/browser/devtools/render_frame_devtools_agent_host.cc:356: frame_tree_node = frame_tree_node->parent(); On 2016/07/12 23:56:15, dgozman wrote: > On 2016/07/12 23:08:21, nasko wrote: > > On 2016/07/12 20:36:14, alex clarke wrote: > > > On 2016/07/12 18:03:11, nasko wrote: > > > > Is this code compatible with out-of-process iframes? > > > I'm not sure. I'm not up to speed with OOPIF. > > > > > > > Why is it always looking at > > > > the root node? > > > I'm assuming that's where the relevant RenderFrameDevToolsAgentHost is. The > > > intended usage is request a throttle at the Page level which will send > > > Page.navigationRequested events if the main frame or a child frame attempts > to > > > navigate. > > > > > > > I'd suggest discussing more with dgozman@, as I'm not very fluent in DevTools, > > but my understanding is that RenderFrameDevToolsAgentHost can be attached to > any > > frame in the tree. Therefore it seems more correct to me that the > > FindAgentHost(frame_tree_node) call is made within the while loop and exited > > once a DevTools agent is found. > > I think this API only makes sense for the page as a whole, so going up to the > root is good. > I agree. https://codereview.chromium.org/2132673002/diff/140001/content/browser/devtoo... File content/browser/devtools/page_navigation_throttle.h (right): https://codereview.chromium.org/2132673002/diff/140001/content/browser/devtoo... content/browser/devtools/page_navigation_throttle.h:19: // Page.setNavigationThrottleEnabled and Page.processNavigation commands. On 2016/07/12 23:56:16, dgozman wrote: > On 2016/07/12 23:08:21, nasko wrote: > > Is Page.setNavigationThrottleEnabled going to be a temporary API? It seems > > strange to expose implementation details at the DevTools protocol level. > > I don't think this will be temporary. We can rename it to something like > "Page.setControlNavigations", but the meaning would be the same. Right it's not meant to be temporary, the ability to programmatically cancel navigations is on of the features we want to provide with headless chrome. I understand the underlying content APIs are not intended to be stable and if there's any changes to this patch that could be made to make things more future proof please let me know.
https://codereview.chromium.org/2132673002/diff/120001/content/browser/devtoo... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2132673002/diff/120001/content/browser/devtoo... content/browser/devtools/render_frame_devtools_agent_host.cc:356: frame_tree_node = frame_tree_node->parent(); On 2016/07/13 09:18:44, alex clarke wrote: > On 2016/07/12 23:56:15, dgozman wrote: > > On 2016/07/12 23:08:21, nasko wrote: > > > On 2016/07/12 20:36:14, alex clarke wrote: > > > > On 2016/07/12 18:03:11, nasko wrote: > > > > > Is this code compatible with out-of-process iframes? > > > > I'm not sure. I'm not up to speed with OOPIF. > > > > > > > > > Why is it always looking at > > > > > the root node? > > > > I'm assuming that's where the relevant RenderFrameDevToolsAgentHost is. > The > > > > intended usage is request a throttle at the Page level which will send > > > > Page.navigationRequested events if the main frame or a child frame > attempts > > to > > > > navigate. > > > > > > > > > > I'd suggest discussing more with dgozman@, as I'm not very fluent in > DevTools, > > > but my understanding is that RenderFrameDevToolsAgentHost can be attached to > > any > > > frame in the tree. Therefore it seems more correct to me that the > > > FindAgentHost(frame_tree_node) call is made within the while loop and exited > > > once a DevTools agent is found. > > > > I think this API only makes sense for the page as a whole, so going up to the > > root is good. > > > > I agree. Is this feature supposed to allow you to control navigations in subframes or only the main frame? If only the main frame is of interest, then this code should work fine. However, if you want to control subframes too, in out-of-process iframes mode I'm not convinced this code will work correctly. https://codereview.chromium.org/2132673002/diff/140001/content/browser/devtoo... File content/browser/devtools/page_navigation_throttle.h (right): https://codereview.chromium.org/2132673002/diff/140001/content/browser/devtoo... content/browser/devtools/page_navigation_throttle.h:19: // Page.setNavigationThrottleEnabled and Page.processNavigation commands. On 2016/07/13 09:18:44, alex clarke wrote: > On 2016/07/12 23:56:16, dgozman wrote: > > On 2016/07/12 23:08:21, nasko wrote: > > > Is Page.setNavigationThrottleEnabled going to be a temporary API? It seems > > > strange to expose implementation details at the DevTools protocol level. > > > > I don't think this will be temporary. We can rename it to something like > > "Page.setControlNavigations", but the meaning would be the same. > > Right it's not meant to be temporary, the ability to programmatically cancel > navigations is on of the features we want to provide with headless chrome. I > understand the underlying content APIs are not intended to be stable and if > there's any changes to this patch that could be made to make things more future > proof please let me know. Let's call the method something more generic. I think the suggestion of Page.setControlNavigations is much better than the current name, which includes internal implementation names.
PTAL https://codereview.chromium.org/2132673002/diff/120001/content/browser/devtoo... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2132673002/diff/120001/content/browser/devtoo... content/browser/devtools/render_frame_devtools_agent_host.cc:356: frame_tree_node = frame_tree_node->parent(); On 2016/07/13 14:55:55, nasko wrote: > On 2016/07/13 09:18:44, alex clarke wrote: > > On 2016/07/12 23:56:15, dgozman wrote: > > > On 2016/07/12 23:08:21, nasko wrote: > > > > On 2016/07/12 20:36:14, alex clarke wrote: > > > > > On 2016/07/12 18:03:11, nasko wrote: > > > > > > Is this code compatible with out-of-process iframes? > > > > > I'm not sure. I'm not up to speed with OOPIF. > > > > > > > > > > > Why is it always looking at > > > > > > the root node? > > > > > I'm assuming that's where the relevant RenderFrameDevToolsAgentHost is. > > The > > > > > intended usage is request a throttle at the Page level which will send > > > > > Page.navigationRequested events if the main frame or a child frame > > attempts > > > to > > > > > navigate. > > > > > > > > > > > > > I'd suggest discussing more with dgozman@, as I'm not very fluent in > > DevTools, > > > > but my understanding is that RenderFrameDevToolsAgentHost can be attached > to > > > any > > > > frame in the tree. Therefore it seems more correct to me that the > > > > FindAgentHost(frame_tree_node) call is made within the while loop and > exited > > > > once a DevTools agent is found. > > > > > > I think this API only makes sense for the page as a whole, so going up to > the > > > root is good. > > > > > > > I agree. > > Is this feature supposed to allow you to control navigations in subframes or > only the main frame? In both. > If only the main frame is of interest, then this code > should work fine. However, if you want to control subframes too, in > out-of-process iframes mode I'm not convinced this code will work correctly. What would be needed to fix it for OOPIF? https://codereview.chromium.org/2132673002/diff/140001/content/browser/devtoo... File content/browser/devtools/page_navigation_throttle.h (right): https://codereview.chromium.org/2132673002/diff/140001/content/browser/devtoo... content/browser/devtools/page_navigation_throttle.h:19: // Page.setNavigationThrottleEnabled and Page.processNavigation commands. On 2016/07/13 14:55:55, nasko wrote: > On 2016/07/13 09:18:44, alex clarke wrote: > > On 2016/07/12 23:56:16, dgozman wrote: > > > On 2016/07/12 23:08:21, nasko wrote: > > > > Is Page.setNavigationThrottleEnabled going to be a temporary API? It seems > > > > strange to expose implementation details at the DevTools protocol level. > > > > > > I don't think this will be temporary. We can rename it to something like > > > "Page.setControlNavigations", but the meaning would be the same. > > > > Right it's not meant to be temporary, the ability to programmatically cancel > > navigations is on of the features we want to provide with headless chrome. I > > understand the underlying content APIs are not intended to be stable and if > > there's any changes to this patch that could be made to make things more > future > > proof please let me know. > > Let's call the method something more generic. I think the suggestion of > Page.setControlNavigations is much better than the current name, which includes > internal implementation names. Done.
The CQ bit was checked by alexclarke@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...
https://codereview.chromium.org/2132673002/diff/120001/content/browser/devtoo... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2132673002/diff/120001/content/browser/devtoo... content/browser/devtools/render_frame_devtools_agent_host.cc:356: frame_tree_node = frame_tree_node->parent(); On 2016/07/13 16:14:23, alex clarke wrote: > On 2016/07/13 14:55:55, nasko wrote: > > On 2016/07/13 09:18:44, alex clarke wrote: > > > On 2016/07/12 23:56:15, dgozman wrote: > > > > On 2016/07/12 23:08:21, nasko wrote: > > > > > On 2016/07/12 20:36:14, alex clarke wrote: > > > > > > On 2016/07/12 18:03:11, nasko wrote: > > > > > > > Is this code compatible with out-of-process iframes? > > > > > > I'm not sure. I'm not up to speed with OOPIF. > > > > > > > > > > > > > Why is it always looking at > > > > > > > the root node? > > > > > > I'm assuming that's where the relevant RenderFrameDevToolsAgentHost > is. > > > The > > > > > > intended usage is request a throttle at the Page level which will send > > > > > > Page.navigationRequested events if the main frame or a child frame > > > attempts > > > > to > > > > > > navigate. > > > > > > > > > > > > > > > > I'd suggest discussing more with dgozman@, as I'm not very fluent in > > > DevTools, > > > > > but my understanding is that RenderFrameDevToolsAgentHost can be > attached > > to > > > > any > > > > > frame in the tree. Therefore it seems more correct to me that the > > > > > FindAgentHost(frame_tree_node) call is made within the while loop and > > exited > > > > > once a DevTools agent is found. > > > > > > > > I think this API only makes sense for the page as a whole, so going up to > > the > > > > root is good. > > > > > > > > > > I agree. > > > > Is this feature supposed to allow you to control navigations in subframes or > > only the main frame? > In both. > > > > If only the main frame is of interest, then this code > > should work fine. However, if you want to control subframes too, in > > out-of-process iframes mode I'm not convinced this code will work correctly. > > What would be needed to fix it for OOPIF? I'm not sure what the design goal of this CL is, so not easy to give direct advice. Is there a design doc of what this API is supposed to do and how it is expected to behave? Alternatively, we do a quick chat offline?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
I've done a bunch more digging trying to understand the full picture and with everyone's help I get it much better. Thanks! I did another pass through the CL and have a few more comments. It doesn't alter the main idea of the CL, mostly cleanups and suggestion for test coverage. https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... File content/browser/devtools/page_navigation_throttle.cc (right): https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... content/browser/devtools/page_navigation_throttle.cc:34: response_pending_ = true; nit: This boolean seems to indicate that the navigation is deferred. Can we just name it to be closer to its meaning? https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... File content/browser/devtools/page_navigation_throttle.h (right): https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... content/browser/devtools/page_navigation_throttle.h:47: bool response_pending_; nit: Some basic comments on these will be useful. https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right): https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:675: } Let's add a test that ensures navigating iframes are also being properly handled by the new API. You can use content/test/data/cross_site_iframe_factory.html to create arbitrary tree of iframes (plenty of examples in codesearch) and IsolateAllSitesForTesting to turn on OOPIF for all cross-site iframes (for bonus points :) ). https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... File content/browser/devtools/protocol/page_handler.cc (right): https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... content/browser/devtools/protocol/page_handler.cc:412: std::unique_ptr<PageNavigationThrottle> PageHandler::GetThrottleForNavigation( nit: This should be named Create, as it doesn't Get an existing one, it always creates one. https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... content/browser/devtools/protocol/page_handler.cc:434: ->set_is_parent_main_frame(navigation_handle->IsParentMainFrame()) Is this really needed? I'd rather not expose it if possible, as ideally I'd remove this method from NavigationHandle. If we expose it, then it becomes much harder to remove in the future. https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... content/browser/devtools/render_frame_devtools_agent_host.cc:353: // NOTE a single throttle registered by Page.setControlNavigations is intended This comment is not entirely correct. Each navigation is 1:1 with NavigationHandle and in turn NavigationHandle is associated with a NavigationThrottle. One throttle cannot handle multiple navigations. https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... content/browser/devtools/render_frame_devtools_agent_host.cc:361: if (agent_host && agent_host->page_handler_) { After offline chat with dgozman@, it became more clear to me that page_handler_ only exists for the main frame. This is definitely something that wasn't obvious to me, so let's make sure we put a comment on this method. Also, it makes the above comment obsolete and what is one thing that is common across all navigations is the PageHadnler object. https://codereview.chromium.org/2132673002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2132673002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:179: "id": "NavigationThrottleResponse", Let's avoid using Throttle here as well. Throttles are just internal implementation. https://codereview.chromium.org/2132673002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:647: { "name": "isParentMainFrame", "type": "boolean", "description": "Whether the navigation is taking place in a frame that is a direct child of the main frame." }, In general, I'd suggest exposing the minimum amount of data from the API, as that locks us into supporting it long term. Unless we really really need this specific boolean, let's remove it. I'll be curious to know if we do indeed need it, what the use case is.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... File content/browser/devtools/page_navigation_throttle.cc (right): https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... content/browser/devtools/page_navigation_throttle.cc:34: response_pending_ = true; On 2016/07/13 22:37:34, nasko wrote: > nit: This boolean seems to indicate that the navigation is deferred. Can we just > name it to be closer to its meaning? Done. https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... File content/browser/devtools/page_navigation_throttle.h (right): https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... content/browser/devtools/page_navigation_throttle.h:47: bool response_pending_; On 2016/07/13 22:37:34, nasko wrote: > nit: Some basic comments on these will be useful. Done. https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right): https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:675: } On 2016/07/13 22:37:34, nasko wrote: > Let's add a test that ensures navigating iframes are also being properly handled > by the new API. You can use content/test/data/cross_site_iframe_factory.html to > create arbitrary tree of iframes (plenty of examples in codesearch) and > IsolateAllSitesForTesting to turn on OOPIF for all cross-site iframes (for bonus > points :) ). It turns out this test is actually very awkward to write with IsolateAllSitesForTesting enabled. The Page domain behaves differently because the commands handled on the renderer only work for the main frame. Presumably there's now new Page domains for each of the child processes. Putting some prints in PageNavigationThrottle shows it does appear to be working however with IsolateAllSitesForTesting. If there's some way of detecting when everything has finished loading and interrogating the iframe URLs without the page domain then I could make this test work with IsolateAllSitesForTesting. https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... File content/browser/devtools/protocol/page_handler.cc (right): https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... content/browser/devtools/protocol/page_handler.cc:412: std::unique_ptr<PageNavigationThrottle> PageHandler::GetThrottleForNavigation( On 2016/07/13 22:37:34, nasko wrote: > nit: This should be named Create, as it doesn't Get an existing one, it always > creates one. Done. https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... content/browser/devtools/protocol/page_handler.cc:434: ->set_is_parent_main_frame(navigation_handle->IsParentMainFrame()) On 2016/07/13 22:37:34, nasko wrote: > Is this really needed? I'd rather not expose it if possible, as ideally I'd > remove this method from NavigationHandle. If we expose it, then it becomes much > harder to remove in the future. Done. https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... content/browser/devtools/render_frame_devtools_agent_host.cc:353: // NOTE a single throttle registered by Page.setControlNavigations is intended On 2016/07/13 22:37:34, nasko wrote: > This comment is not entirely correct. Each navigation is 1:1 with > NavigationHandle and in turn NavigationHandle is associated with a > NavigationThrottle. One throttle cannot handle multiple navigations. Acknowledged. https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... content/browser/devtools/render_frame_devtools_agent_host.cc:361: if (agent_host && agent_host->page_handler_) { On 2016/07/13 22:37:34, nasko wrote: > After offline chat with dgozman@, it became more clear to me that page_handler_ > only exists for the main frame. This is definitely something that wasn't obvious > to me, so let's make sure we put a comment on this method. > Also, it makes the above comment obsolete and what is one thing that is common > across all navigations is the PageHadnler object. Done. https://codereview.chromium.org/2132673002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2132673002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:179: "id": "NavigationThrottleResponse", On 2016/07/13 22:37:34, nasko wrote: > Let's avoid using Throttle here as well. Throttles are just internal > implementation. Done. https://codereview.chromium.org/2132673002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:647: { "name": "isParentMainFrame", "type": "boolean", "description": "Whether the navigation is taking place in a frame that is a direct child of the main frame." }, On 2016/07/13 22:37:34, nasko wrote: > In general, I'd suggest exposing the minimum amount of data from the API, as > that locks us into supporting it long term. Unless we really really need this > specific boolean, let's remove it. I'll be curious to know if we do indeed need > it, what the use case is. isParentMainFrame was speculative. We can probably live without it.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
The CQ bit was checked by alexclarke@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right): https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:675: } On 2016/07/14 16:06:04, alex clarke wrote: > On 2016/07/13 22:37:34, nasko wrote: > > Let's add a test that ensures navigating iframes are also being properly > handled > > by the new API. You can use content/test/data/cross_site_iframe_factory.html > to > > create arbitrary tree of iframes (plenty of examples in codesearch) and > > IsolateAllSitesForTesting to turn on OOPIF for all cross-site iframes (for > bonus > > points :) ). > > It turns out this test is actually very awkward to write with > IsolateAllSitesForTesting enabled. The Page domain behaves differently because > the commands handled on the renderer only work for the main frame. Presumably > there's now new Page domains for each of the child processes. > > Putting some prints in PageNavigationThrottle shows it does appear to be working > however with IsolateAllSitesForTesting. > > If there's some way of detecting when everything has finished loading and > interrogating the iframe URLs without the page domain then I could make this > test work with IsolateAllSitesForTesting. It's possible to attach to all oopifs separately, as we do in site_per_process_devtools_browsertest.cc. However, issuing and listening for protocol events there would be a pain, so I won't insist on doing this. We lack test infrastructure in this area.
Mostly there. Thanks for adding the extra test! https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right): https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:675: } On 2016/07/14 16:06:04, alex clarke wrote: > On 2016/07/13 22:37:34, nasko wrote: > > Let's add a test that ensures navigating iframes are also being properly > handled > > by the new API. You can use content/test/data/cross_site_iframe_factory.html > to > > create arbitrary tree of iframes (plenty of examples in codesearch) and > > IsolateAllSitesForTesting to turn on OOPIF for all cross-site iframes (for > bonus > > points :) ). > > It turns out this test is actually very awkward to write with > IsolateAllSitesForTesting enabled. The Page domain behaves differently because > the commands handled on the renderer only work for the main frame. Does your CL care about the renderer process at all? If I got it correctly from dgozman@, the remote debugging protocol shouldn't expose details whether we have IsolateAllSitesForTesting enabled or not. > Presumably > there's now new Page domains for each of the child processes. There should be at least a new RenderFrameDevToolsAgentHost. The PageHandler should be the same, but I'm not sure what "Page domain" is. > Putting some prints in PageNavigationThrottle shows it does appear to be working > however with IsolateAllSitesForTesting. > > If there's some way of detecting when everything has finished loading and > interrogating the iframe URLs without the page domain then I could make this > test work with IsolateAllSitesForTesting. You can use TestNavigationObserver from content/public/test/test_navigation_observer.h and the single param constructor taking a WebContents. It will wait for the load stopped event, which is fired at the end when all frames have completed loading. https://codereview.chromium.org/2132673002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2132673002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:647: { "name": "isParentMainFrame", "type": "boolean", "description": "Whether the navigation is taking place in a frame that is a direct child of the main frame." }, On 2016/07/14 16:06:05, alex clarke wrote: > On 2016/07/13 22:37:34, nasko wrote: > > In general, I'd suggest exposing the minimum amount of data from the API, as > > that locks us into supporting it long term. Unless we really really need this > > specific boolean, let's remove it. I'll be curious to know if we do indeed > need > > it, what the use case is. > > isParentMainFrame was speculative. We can probably live without it. The rule of thumb is that we only expose parts that are absolutely needed and used. We can always expose additional bits once we need we know them and what for. https://codereview.chromium.org/2132673002/diff/200001/content/browser/devtoo... File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right): https://codereview.chromium.org/2132673002/diff/200001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:681: IN_PROC_BROWSER_TEST_F(DevToolsProtocolTest, ControlNavigations_MainFrame) { No underscores, just CamelCase. https://codereview.chromium.org/2132673002/diff/200001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:723: "http://127.0.0.1:\\d+/devtools/control_navigations/meta_tag.html")); very minor nit: You could just strip the port number from the frame_url and compare it directly, since you aren't matching the port number exactly anyway. Might make the code a bit more readable. https://codereview.chromium.org/2132673002/diff/200001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:727: IN_PROC_BROWSER_TEST_F(DevToolsProtocolTest, ControlNavigations_ChildFrames) { Thanks for adding this test! https://codereview.chromium.org/2132673002/diff/200001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:732: NavigateToURLBlockUntilNavigationsComplete(shell(), GURL("about:blank"), 1); Why do you need to navigate to about:blank first? https://codereview.chromium.org/2132673002/diff/200001/content/browser/devtoo... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2132673002/diff/200001/content/browser/devtoo... content/browser/devtools/render_frame_devtools_agent_host.cc:359: // NOTE Page.setControlNavigations is intended to control navigations in the minor nit: "Note: ..." https://codereview.chromium.org/2132673002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2132673002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:650: { "name": "rendererInitiated", "type": "boolean", "description": "Whether the navigation was initated by the renderer process." } Let's remove this bit too. It shouldn't matter whether the navigation was initiated by renderer process or browser process.
Description was changed from ========== Aadding Navigation Throttles to devtools Extends the devtools Page domain with commands to generate and manage Navigation Throttles. This functionality will be useful for WebDriver style test frameworks and is needed for Headless Chrome. If enabled by Page.setNavigationThrottleEnabled a Page.navigationRequested event will be fired if the main frame or a child frame attempts to navigate, or it receives a server side redirect. The navigation will be deferred until a corresponding Page.processNavigation command is sent in response. BUG=546953 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Aadding Navigation Throttles to devtools Extends the devtools Page domain with commands to generate and manage Navigation Throttles. This functionality will be useful for WebDriver style test frameworks and is needed for Headless Chrome. If enabled by Page.setControlNavigations a Page.navigationRequested event will be fired if the main frame or a child frame attempts to navigate, or it receives a server side redirect. The navigation will be deferred until a corresponding Page.processNavigation command is sent in response. BUG=546953 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Aadding Navigation Throttles to devtools Extends the devtools Page domain with commands to generate and manage Navigation Throttles. This functionality will be useful for WebDriver style test frameworks and is needed for Headless Chrome. If enabled by Page.setControlNavigations a Page.navigationRequested event will be fired if the main frame or a child frame attempts to navigate, or it receives a server side redirect. The navigation will be deferred until a corresponding Page.processNavigation command is sent in response. BUG=546953 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Aadding Navigation Throttles to devtools Extends the devtools Page domain with commands to generate and manage Navigation Throttles. This functionality will be useful for WebDriver style test frameworks and is needed for Headless Chrome. If enabled by Page.setControlNavigations a Page.navigationRequested event will be fired if the main frame or a child frame attempts to navigate, or it receives a server side redirect. The navigation will be deferred until a corresponding Page.processNavigation command is sent in response. BUG=546953 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Aadding Navigation Throttles to devtools Extends the devtools Page domain with commands to generate and manage Navigation Throttles. This functionality will be useful for WebDriver style test frameworks and is needed for Headless Chrome. If enabled by Page.setControlNavigations a Page.navigationRequested event will be fired if the main frame or a child frame attempts to navigate, or it receives a server side redirect. The navigation will be deferred until a corresponding Page.processNavigation command is sent in response. BUG=546953 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Aadding Navigation Throttles to devtools Extends the devtools Page domain with commands to generate and manage Navigation Throttles. This functionality will be useful for WebDriver style test frameworks and is needed for Headless Chrome. If enabled by Page.setControlNavigations a Page.navigationRequested event will be fired if the main frame or a child frame attempts to navigate, or it receives a server side redirect. The navigation will be deferred until a corresponding Page.processNavigation command is sent in response. BUG=546953 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
PTAL https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right): https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:675: } On 2016/07/14 16:47:19, nasko wrote: > On 2016/07/14 16:06:04, alex clarke wrote: > > On 2016/07/13 22:37:34, nasko wrote: > > > Let's add a test that ensures navigating iframes are also being properly > > handled > > > by the new API. You can use content/test/data/cross_site_iframe_factory.html > > to > > > create arbitrary tree of iframes (plenty of examples in codesearch) and > > > IsolateAllSitesForTesting to turn on OOPIF for all cross-site iframes (for > > bonus > > > points :) ). > > > > It turns out this test is actually very awkward to write with > > IsolateAllSitesForTesting enabled. The Page domain behaves differently > because > > the commands handled on the renderer only work for the main frame. > > Does your CL care about the renderer process at all? If I got it correctly from > dgozman@, the remote debugging protocol shouldn't expose details whether we have > IsolateAllSitesForTesting enabled or not. Yes it does :( If IsolateAllSitesForTesting is enabled you only get events for the main frame sent. > > > Presumably > > there's now new Page domains for each of the child processes. > > There should be at least a new RenderFrameDevToolsAgentHost. The PageHandler > should be the same, but I'm not sure what "Page domain" is. > > > Putting some prints in PageNavigationThrottle shows it does appear to be > working > > however with IsolateAllSitesForTesting. > > > > If there's some way of detecting when everything has finished loading and > > interrogating the iframe URLs without the page domain then I could make this > > test work with IsolateAllSitesForTesting. > > You can use TestNavigationObserver from > content/public/test/test_navigation_observer.h and the single param constructor > taking a WebContents. It will wait for the load stopped event, which is fired at > the end when all frames have completed loading. I tried that. It was only counting 1 or 2 loads. I'm speculating that's because the iframes are navigating concurrently. https://codereview.chromium.org/2132673002/diff/200001/content/browser/devtoo... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2132673002/diff/200001/content/browser/devtoo... content/browser/devtools/render_frame_devtools_agent_host.cc:359: // NOTE Page.setControlNavigations is intended to control navigations in the On 2016/07/14 16:47:20, nasko wrote: > minor nit: "Note: ..." Done. https://codereview.chromium.org/2132673002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2132673002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:650: { "name": "rendererInitiated", "type": "boolean", "description": "Whether the navigation was initated by the renderer process." } On 2016/07/14 16:47:20, nasko wrote: > Let's remove this bit too. It shouldn't matter whether the navigation was > initiated by renderer process or browser process. Done.
The CQ bit was checked by alexclarke@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
https://codereview.chromium.org/2132673002/diff/200001/content/browser/devtoo... File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right): https://codereview.chromium.org/2132673002/diff/200001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:681: IN_PROC_BROWSER_TEST_F(DevToolsProtocolTest, ControlNavigations_MainFrame) { On 2016/07/14 16:47:19, nasko wrote: > No underscores, just CamelCase. Done. https://codereview.chromium.org/2132673002/diff/200001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:723: "http://127.0.0.1:\\d+/devtools/control_navigations/meta_tag.html")); On 2016/07/14 16:47:19, nasko wrote: > very minor nit: You could just strip the port number from the frame_url and > compare it directly, since you aren't matching the port number exactly anyway. > Might make the code a bit more readable. Seems like a good idea, are there any utilities for doing that before I invent something? https://codereview.chromium.org/2132673002/diff/200001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:732: NavigateToURLBlockUntilNavigationsComplete(shell(), GURL("about:blank"), 1); On 2016/07/14 16:47:19, nasko wrote: > Why do you need to navigate to about:blank first? For two reasons: 1. To make sure there's a page we can send DevTools commands to 2. To make sure there's no race between Page.setControlNavigations and the initial navigation.
The CQ bit was checked by alexclarke@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
Thanks for working through all the nits I had. LGTM! https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right): https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:675: } On 2016/07/14 17:26:17, alex clarke wrote: > On 2016/07/14 16:47:19, nasko wrote: > > On 2016/07/14 16:06:04, alex clarke wrote: > > > On 2016/07/13 22:37:34, nasko wrote: > > > > Let's add a test that ensures navigating iframes are also being properly > > > handled > > > > by the new API. You can use > content/test/data/cross_site_iframe_factory.html > > > to > > > > create arbitrary tree of iframes (plenty of examples in codesearch) and > > > > IsolateAllSitesForTesting to turn on OOPIF for all cross-site iframes (for > > > bonus > > > > points :) ). > > > > > > It turns out this test is actually very awkward to write with > > > IsolateAllSitesForTesting enabled. The Page domain behaves differently > > because > > > the commands handled on the renderer only work for the main frame. > > > > Does your CL care about the renderer process at all? If I got it correctly > from > > dgozman@, the remote debugging protocol shouldn't expose details whether we > have > > IsolateAllSitesForTesting enabled or not. > > Yes it does :( If IsolateAllSitesForTesting is enabled you only get events for > the main frame sent. Ok, let's put a TODO to enable OOPIFs later on, once test infrastructure is caught up. > > > > > Presumably > > > there's now new Page domains for each of the child processes. > > > > There should be at least a new RenderFrameDevToolsAgentHost. The PageHandler > > should be the same, but I'm not sure what "Page domain" is. > > > > > Putting some prints in PageNavigationThrottle shows it does appear to be > > working > > > however with IsolateAllSitesForTesting. > > > > > > If there's some way of detecting when everything has finished loading and > > > interrogating the iframe URLs without the page domain then I could make this > > > test work with IsolateAllSitesForTesting. > > > > You can use TestNavigationObserver from > > content/public/test/test_navigation_observer.h and the single param > constructor > > taking a WebContents. It will wait for the load stopped event, which is fired > at > > the end when all frames have completed loading. > > I tried that. It was only counting 1 or 2 loads. I'm speculating that's > because the iframes are navigating concurrently. You should only care about 1 load, as the load of the main frame encloses the loads of all subframes. You will *not* see 5 loads. Think of the TestNavigationObserver being the test equivalent of the spinner in the UI - whenever something is spinning it is waiting. The moment it stops spinning, it returns from the wait. https://codereview.chromium.org/2132673002/diff/200001/content/browser/devtoo... File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right): https://codereview.chromium.org/2132673002/diff/200001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:723: "http://127.0.0.1:\\d+/devtools/control_navigations/meta_tag.html")); On 2016/07/14 17:38:34, alex clarke wrote: > On 2016/07/14 16:47:19, nasko wrote: > > very minor nit: You could just strip the port number from the frame_url and > > compare it directly, since you aren't matching the port number exactly anyway. > > Might make the code a bit more readable. > > Seems like a good idea, are there any utilities for doing that before I invent > something? You can use a GURL::Replacements. There are some examples of usage in site_per_process_devtools_browsertest.cc. https://codereview.chromium.org/2132673002/diff/200001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:732: NavigateToURLBlockUntilNavigationsComplete(shell(), GURL("about:blank"), 1); On 2016/07/14 17:38:34, alex clarke wrote: > On 2016/07/14 16:47:19, nasko wrote: > > Why do you need to navigate to about:blank first? > > For two reasons: > 1. To make sure there's a page we can send DevTools commands to > 2. To make sure there's no race between Page.setControlNavigations and the > initial navigation. In general, each frame starts with an initial blank document, so it shouldn't be needed, but if it doesn't work otherwise, I don't have the cycles right now to help debug why.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right): https://codereview.chromium.org/2132673002/diff/160001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:675: } On 2016/07/14 17:55:51, nasko wrote: > On 2016/07/14 17:26:17, alex clarke wrote: > > On 2016/07/14 16:47:19, nasko wrote: > > > On 2016/07/14 16:06:04, alex clarke wrote: > > > > On 2016/07/13 22:37:34, nasko wrote: > > > > > Let's add a test that ensures navigating iframes are also being properly > > > > handled > > > > > by the new API. You can use > > content/test/data/cross_site_iframe_factory.html > > > > to > > > > > create arbitrary tree of iframes (plenty of examples in codesearch) and > > > > > IsolateAllSitesForTesting to turn on OOPIF for all cross-site iframes > (for > > > > bonus > > > > > points :) ). > > > > > > > > It turns out this test is actually very awkward to write with > > > > IsolateAllSitesForTesting enabled. The Page domain behaves differently > > > because > > > > the commands handled on the renderer only work for the main frame. > > > > > > Does your CL care about the renderer process at all? If I got it correctly > > from > > > dgozman@, the remote debugging protocol shouldn't expose details whether we > > have > > > IsolateAllSitesForTesting enabled or not. > > > > Yes it does :( If IsolateAllSitesForTesting is enabled you only get events for > > the main frame sent. > > Ok, let's put a TODO to enable OOPIFs later on, once test infrastructure is > caught up. > > > > > > > > Presumably > > > > there's now new Page domains for each of the child processes. > > > > > > There should be at least a new RenderFrameDevToolsAgentHost. The PageHandler > > > should be the same, but I'm not sure what "Page domain" is. > > > > > > > Putting some prints in PageNavigationThrottle shows it does appear to be > > > working > > > > however with IsolateAllSitesForTesting. > > > > > > > > If there's some way of detecting when everything has finished loading and > > > > interrogating the iframe URLs without the page domain then I could make > this > > > > test work with IsolateAllSitesForTesting. > > > > > > You can use TestNavigationObserver from > > > content/public/test/test_navigation_observer.h and the single param > > constructor > > > taking a WebContents. It will wait for the load stopped event, which is > fired > > at > > > the end when all frames have completed loading. > > > > I tried that. It was only counting 1 or 2 loads. I'm speculating that's > > because the iframes are navigating concurrently. > > You should only care about 1 load, as the load of the main frame encloses the > loads of all subframes. You will *not* see 5 loads. Think of the > TestNavigationObserver being the test equivalent of the spinner in the UI - > whenever something is spinning it is waiting. The moment it stops spinning, it > returns from the wait. I figured out how to get this to work with IsolateAllSitesForTesting. https://codereview.chromium.org/2132673002/diff/200001/content/browser/devtoo... File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right): https://codereview.chromium.org/2132673002/diff/200001/content/browser/devtoo... content/browser/devtools/protocol/devtools_protocol_browsertest.cc:723: "http://127.0.0.1:\\d+/devtools/control_navigations/meta_tag.html")); On 2016/07/14 17:55:51, nasko wrote: > On 2016/07/14 17:38:34, alex clarke wrote: > > On 2016/07/14 16:47:19, nasko wrote: > > > very minor nit: You could just strip the port number from the frame_url and > > > compare it directly, since you aren't matching the port number exactly > anyway. > > > Might make the code a bit more readable. > > > > Seems like a good idea, are there any utilities for doing that before I invent > > something? > > You can use a GURL::Replacements. There are some examples of usage in > site_per_process_devtools_browsertest.cc. Done.
Thanks all
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org, dgozman@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2132673002/#ps260001 (title: "Got browser test to work with IsolateAllSitesForTesting")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2132673002/diff/260001/content/public/test/te... File content/public/test/test_navigation_observer.cc (right): https://codereview.chromium.org/2132673002/diff/260001/content/public/test/te... content/public/test/test_navigation_observer.cc:55: fprintf(stderr, "YARR! %s\n", validated_url.spec().c_str()); You plan to remove this before commit, right?
The CQ bit was unchecked by alexclarke@chromium.org
https://codereview.chromium.org/2132673002/diff/260001/content/public/test/te... File content/public/test/test_navigation_observer.cc (right): https://codereview.chromium.org/2132673002/diff/260001/content/public/test/te... content/public/test/test_navigation_observer.cc:55: fprintf(stderr, "YARR! %s\n", validated_url.spec().c_str()); On 2016/07/15 14:42:31, nasko wrote: > You plan to remove this before commit, right? Done.
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org, dgozman@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2132673002/#ps280001 (title: "Remove print")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by alexclarke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
Message was sent while issue was closed.
Description was changed from ========== Aadding Navigation Throttles to devtools Extends the devtools Page domain with commands to generate and manage Navigation Throttles. This functionality will be useful for WebDriver style test frameworks and is needed for Headless Chrome. If enabled by Page.setControlNavigations a Page.navigationRequested event will be fired if the main frame or a child frame attempts to navigate, or it receives a server side redirect. The navigation will be deferred until a corresponding Page.processNavigation command is sent in response. BUG=546953 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Aadding Navigation Throttles to devtools Extends the devtools Page domain with commands to generate and manage Navigation Throttles. This functionality will be useful for WebDriver style test frameworks and is needed for Headless Chrome. If enabled by Page.setControlNavigations a Page.navigationRequested event will be fired if the main frame or a child frame attempts to navigate, or it receives a server side redirect. The navigation will be deferred until a corresponding Page.processNavigation command is sent in response. BUG=546953 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Aadding Navigation Throttles to devtools Extends the devtools Page domain with commands to generate and manage Navigation Throttles. This functionality will be useful for WebDriver style test frameworks and is needed for Headless Chrome. If enabled by Page.setControlNavigations a Page.navigationRequested event will be fired if the main frame or a child frame attempts to navigate, or it receives a server side redirect. The navigation will be deferred until a corresponding Page.processNavigation command is sent in response. BUG=546953 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Aadding Navigation Throttles to devtools Extends the devtools Page domain with commands to generate and manage Navigation Throttles. This functionality will be useful for WebDriver style test frameworks and is needed for Headless Chrome. If enabled by Page.setControlNavigations a Page.navigationRequested event will be fired if the main frame or a child frame attempts to navigate, or it receives a server side redirect. The navigation will be deferred until a corresponding Page.processNavigation command is sent in response. BUG=546953 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/f33b4f2e7962249e7b9f31b5e8a74e66059ddd1f Cr-Commit-Position: refs/heads/master@{#405866} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/f33b4f2e7962249e7b9f31b5e8a74e66059ddd1f Cr-Commit-Position: refs/heads/master@{#405866} |
