PlzNavigate: fix DevToolsProtocolTest.CrossSitePauseInBeforeUnload

PlzNavigate: fix DevToolsProtocolTest.CrossSitePauseInBeforeUnload

This CL fixes DevToolsProtocolTest.CrossSitePauseInBeforeUnload when run with
PlzNavigate enabled. Now, devtools protocol messages are deferred only after
the browser has received the BeforeUnload ACK (and not before).

BUG=551000
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/952e7f049fee56594626795eb838b7b0dd68fd9c
Cr-Commit-Position: refs/heads/master@{#378741}

[clamy, 2016-02-24 15:28:48]

Description was changed from

==========
PlzNavigate: fix DevToolsProtocolTest.CrossSitePauseInBeforeUnload

This CL fixes DevToolsProtocolTest.CrossSitePauseInBeforeUnload when run with
PlzNavigate enabled. Now, devtools protocol messages are deferred only after
the browser has received the BeforeUnload ACK (and not before).

BUG=551000
==========

to

==========
PlzNavigate: fix DevToolsProtocolTest.CrossSitePauseInBeforeUnload

This CL fixes DevToolsProtocolTest.CrossSitePauseInBeforeUnload when run with
PlzNavigate enabled. Now, devtools protocol messages are deferred only after
the browser has received the BeforeUnload ACK (and not before).

BUG=551000
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[clamy, 2016-02-24 15:31:44]

Description was changed from

==========
PlzNavigate: fix DevToolsProtocolTest.CrossSitePauseInBeforeUnload

This CL fixes DevToolsProtocolTest.CrossSitePauseInBeforeUnload when run with
PlzNavigate enabled. Now, devtools protocol messages are deferred only after
the browser has received the BeforeUnload ACK (and not before).

BUG=551000
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
PlzNavigate: fix DevToolsProtocolTest.CrossSitePauseInBeforeUnload

This CL fixes DevToolsProtocolTest.CrossSitePauseInBeforeUnload when run with
PlzNavigate enabled. Now, devtools protocol messages are deferred only after
the browser has received the BeforeUnload ACK (and not before).

BUG=551000
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[clamy, 2016-02-24 15:31:44]

clamy@chromium.org changed reviewers:
+ dgozman@chromium.org

[clamy, 2016-02-24 15:33:08]

@dgozman: PTAL. This is an update of carlos's patche for devtools support in
PlzNavigate. The main difference is that the start of the buffering happens
*after* receiving the BeforeUnload ACK, and not when the browser decides to
navigate.

[dgozman, 2016-02-24 19:05:45]

https://codereview.chromium.org/1729373002/diff/1/content/browser/devtools/re...
File content/browser/devtools/render_frame_devtools_agent_host.h (right):

https://codereview.chromium.org/1729373002/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.h:58: static void
OnBeforeNavigation(NavigationHandle* navigation_handle);
I don't really like this. We hoped to move away from devtools-specific static
methods with PlzNavigate. Why do we need this hack again? Should we adjust
navigation API instead?

https://codereview.chromium.org/1729373002/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.h:119: void
WillStartRequest(NavigationHandle* navigation_handle);
This is a strange name. Perhaps, |AboutToNavigate| will be better?

[clamy, 2016-02-25 14:24:16]

Thanks!

https://codereview.chromium.org/1729373002/diff/1/content/browser/devtools/re...
File content/browser/devtools/render_frame_devtools_agent_host.h (right):

https://codereview.chromium.org/1729373002/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.h:58: static void
OnBeforeNavigation(NavigationHandle* navigation_handle);
On 2016/02/24 19:05:45, dgozman wrote:
> I don't really like this. We hoped to move away from devtools-specific static
> methods with PlzNavigate. Why do we need this hack again? Should we adjust
> navigation API instead?

The content policy is not to add methods to the content public API that are not
used outside of content. This would not be the case here. If we were to
introduce such a method in the future, we could move devtools to using it
though.

https://codereview.chromium.org/1729373002/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.h:119: void
WillStartRequest(NavigationHandle* navigation_handle);
On 2016/02/24 19:05:45, dgozman wrote:
> This is a strange name. Perhaps, |AboutToNavigate| will be better?

Done.

[dgozman, 2016-02-25 18:11:25]

lgtm

https://codereview.chromium.org/1729373002/diff/20001/content/browser/devtool...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/1729373002/diff/20001/content/browser/devtool...
content/browser/devtools/render_frame_devtools_agent_host.cc:339:
FindAgentHost(frame_tree_node->render_manager()->current_frame_host());
We matched by |frame_tree_node_| before. Let's add FindAgentHost(FrameTreeNode*)
instead of checking with current_frame_host().

https://codereview.chromium.org/1729373002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1729373002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:209:
RenderFrameDevToolsAgentHost::OnBeforeNavigation(navigation_handle_.get());
So, with PlzNavigate we do not ever send FrameMsg_Navigate to the frame (that's
where we got OnBeforeNavigation before)? Will we end up with two calls?
I'm asking because I don't really know how the new scheme works to verify
whether this is a right place to call OnBeforeNavigation.

[dgozman, 2016-02-25 18:12:17]

Please wait for M50 branch cut before landing this, just to be on the safe side.

[clamy, 2016-03-02 12:33:50]

Thanks! Now that M50 has been cut, I'll send the patch to CQ.

https://codereview.chromium.org/1729373002/diff/20001/content/browser/devtool...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/1729373002/diff/20001/content/browser/devtool...
content/browser/devtools/render_frame_devtools_agent_host.cc:339:
FindAgentHost(frame_tree_node->render_manager()->current_frame_host());
On 2016/02/25 18:11:24, dgozman wrote:
> We matched by |frame_tree_node_| before. Let's add
FindAgentHost(FrameTreeNode*)
> instead of checking with current_frame_host().

Done.

https://codereview.chromium.org/1729373002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1729373002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:209:
RenderFrameDevToolsAgentHost::OnBeforeNavigation(navigation_handle_.get());
On 2016/02/25 18:11:24, dgozman wrote:
> So, with PlzNavigate we do not ever send FrameMsg_Navigate to the frame
(that's
> where we got OnBeforeNavigation before)? Will we end up with two calls?
> I'm asking because I don't really know how the new scheme works to verify
> whether this is a right place to call OnBeforeNavigation.

Yes we never send FrameMsg_Navigate. The place here corresponds to the moment
where we received the BeforeUnload ACK and we are going to send the navigation
to the network stack. So OnBeforeNavigation will always be called just once.

[clamy, 2016-03-02 12:33:56]

The CQ bit was checked by clamy@chromium.org

[clamy, 2016-03-02 12:33:56]

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/1729373002/#ps40001
(title: "Rebase + addressed comments")

[commit-bot: I haz the power, 2016-03-02 12:34:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1729373002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1729373002/40001

[commit-bot: I haz the power, 2016-03-02 14:05:02]

Description was changed from

==========
PlzNavigate: fix DevToolsProtocolTest.CrossSitePauseInBeforeUnload

This CL fixes DevToolsProtocolTest.CrossSitePauseInBeforeUnload when run with
PlzNavigate enabled. Now, devtools protocol messages are deferred only after
the browser has received the BeforeUnload ACK (and not before).

BUG=551000
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
PlzNavigate: fix DevToolsProtocolTest.CrossSitePauseInBeforeUnload

This CL fixes DevToolsProtocolTest.CrossSitePauseInBeforeUnload when run with
PlzNavigate enabled. Now, devtools protocol messages are deferred only after
the browser has received the BeforeUnload ACK (and not before).

BUG=551000
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-03-02 14:05:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-02 14:05:54]

Description was changed from

==========
PlzNavigate: fix DevToolsProtocolTest.CrossSitePauseInBeforeUnload

This CL fixes DevToolsProtocolTest.CrossSitePauseInBeforeUnload when run with
PlzNavigate enabled. Now, devtools protocol messages are deferred only after
the browser has received the BeforeUnload ACK (and not before).

BUG=551000
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
PlzNavigate: fix DevToolsProtocolTest.CrossSitePauseInBeforeUnload

This CL fixes DevToolsProtocolTest.CrossSitePauseInBeforeUnload when run with
PlzNavigate enabled. Now, devtools protocol messages are deferred only after
the browser has received the BeforeUnload ACK (and not before).

BUG=551000
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/952e7f049fee56594626795eb838b7b0dd68fd9c
Cr-Commit-Position: refs/heads/master@{#378741}
==========

[commit-bot: I haz the power, 2016-03-02 14:05:55]

Patchset 3 (id:??) landed as
https://crrev.com/952e7f049fee56594626795eb838b7b0dd68fd9c
Cr-Commit-Position: refs/heads/master@{#378741}