PlzNavigate: support NavigationThrottle::WillProcessResponse

PlzNavigate: support NavigationThrottle::WillProcessResponse

This CL introduces support in PlzNavigate for canceling navigations or pausing
navigations in NavigationThrottle::WillProcessResponse.

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

Committed: https://crrev.com/5a3c36494131da824529e5abca31ffad62270f00
Cr-Commit-Position: refs/heads/master@{#383092}

[clamy, 2016-03-17 13:08:08]

Description was changed from

==========
PlzNavigate: support NavigationThrottle::WillProcessResponse

This CL introduces support in PlzNavigate for canceling navigations or pausing
navigations in NavigationThrottle::WillProcessResponse.

BUG=555418
==========

to

==========
PlzNavigate: support NavigationThrottle::WillProcessResponse

This CL introduces support in PlzNavigate for canceling navigations or pausing
navigations in NavigationThrottle::WillProcessResponse.

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

[clamy, 2016-03-17 16:44:51]

Description was changed from

==========
PlzNavigate: support NavigationThrottle::WillProcessResponse

This CL introduces support in PlzNavigate for canceling navigations or pausing
navigations in NavigationThrottle::WillProcessResponse.

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

to

==========
PlzNavigate: support NavigationThrottle::WillProcessResponse

This CL introduces support in PlzNavigate for canceling navigations or pausing
navigations in NavigationThrottle::WillProcessResponse.

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

[clamy, 2016-03-17 16:44:51]

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

[clamy, 2016-03-17 16:47:16]

@nasko: PTAL. This is the support for NavigationThrottle::WillProcessResponse in
PlzNavigate. This is needed for the XFO throttle, and I also need to be able to
cancel the navigation at the same stage for one of the CrossSiteTransferTests,
so I thought I would do this one first.

[nasko, 2016-03-18 21:51:43]

Nice! Few comments, but exciting functionality!

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

https://codereview.chromium.org/1811913003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:237:
install_loop_runner_->Quit();
Why do we need to run the message loop here?

https://codereview.chromium.org/1811913003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:245:
navigation_throttle_ = nullptr;
nit: .reset()?

https://codereview.chromium.org/1811913003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:437: // Ensure
that NavigationThrottles can cancel the navigation at navigation
Thanks for adding those browser tests!

nit: NavigationThrottle.

https://codereview.chromium.org/1811913003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:454: // The
navigation should have been canceled before being redirected.
nit: Empty line above comment for readability.

https://codereview.chromium.org/1811913003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:540:
installer.WaitForThrottleInstallation();
Why do we need to wait for this? Isn't the throttle just added to the list,
which lives on the UI thread? Or is it just verifying that the installation has
succeeded?

https://codereview.chromium.org/1811913003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:570:
EXPECT_TRUE(observer.was_redirected());
Isn't this the same check as the one two lines above?

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

https://codereview.chromium.org/1811913003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.h:170: // Has a RenderFrameHost
commit the navigation. The NavigationRequest will be
nit: s/Has/Have/

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

https://codereview.chromium.org/1811913003/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:61: #include
"content/common/view_messages.h"
This makes me sad. RenderFrame(Host) objects shouldn't deal with ViewMsg types.

https://codereview.chromium.org/1811913003/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2157: new
ViewMsg_EnableViewSourceMode(render_view_host()->GetRoutingID()));
View source can never have subframes. Why not move this over to be a FrameMsg
and send it to the RenderFrame itself?

[clamy, 2016-03-23 14:36:44]

Thanks!

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

https://codereview.chromium.org/1811913003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:237:
install_loop_runner_->Quit();
On 2016/03/18 21:51:43, nasko (slow) wrote:
> Why do we need to run the message loop here?

Removed the functionality to wait for the installation of the Throttle, since
it's not very useful.

https://codereview.chromium.org/1811913003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:245:
navigation_throttle_ = nullptr;
On 2016/03/18 21:51:43, nasko (slow) wrote:
> nit: .reset()?

navigation_throttle_ is a raw pointer (NavigationHandle owns the scoped_ptr).

https://codereview.chromium.org/1811913003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:437: // Ensure
that NavigationThrottles can cancel the navigation at navigation
On 2016/03/18 21:51:43, nasko (slow) wrote:
> Thanks for adding those browser tests!
> 
> nit: NavigationThrottle.

Done.

https://codereview.chromium.org/1811913003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:454: // The
navigation should have been canceled before being redirected.
On 2016/03/18 21:51:43, nasko (slow) wrote:
> nit: Empty line above comment for readability.

Done.

https://codereview.chromium.org/1811913003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:540:
installer.WaitForThrottleInstallation();
On 2016/03/18 21:51:43, nasko (slow) wrote:
> Why do we need to wait for this? Isn't the throttle just added to the list,
> which lives on the UI thread? Or is it just verifying that the installation
has
> succeeded?

Removed it since it's not very useful.

https://codereview.chromium.org/1811913003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_browsertest.cc:570:
EXPECT_TRUE(observer.was_redirected());
On 2016/03/18 21:51:43, nasko (slow) wrote:
> Isn't this the same check as the one two lines above?

Indeed. Removed it.

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

https://codereview.chromium.org/1811913003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.h:170: // Has a RenderFrameHost
commit the navigation. The NavigationRequest will be
On 2016/03/18 21:51:43, nasko (slow) wrote:
> nit: s/Has/Have/

Done.

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

https://codereview.chromium.org/1811913003/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:61: #include
"content/common/view_messages.h"
On 2016/03/18 21:51:43, nasko (slow) wrote:
> This makes me sad. RenderFrame(Host) objects shouldn't deal with ViewMsg
types.

Done.

https://codereview.chromium.org/1811913003/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2157: new
ViewMsg_EnableViewSourceMode(render_view_host()->GetRoutingID()));
On 2016/03/18 21:51:43, nasko (slow) wrote:
> View source can never have subframes. Why not move this over to be a FrameMsg
> and send it to the RenderFrame itself?

Done. This adds a bit more changes though.

[nasko, 2016-03-23 22:16:47]

LGTM, though I think the move of the IPC belongs in a separate CL.

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

https://codereview.chromium.org/1811913003/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2157: new
ViewMsg_EnableViewSourceMode(render_view_host()->GetRoutingID()));
On 2016/03/23 14:36:44, clamy wrote:
> On 2016/03/18 21:51:43, nasko (slow) wrote:
> > View source can never have subframes. Why not move this over to be a
FrameMsg
> > and send it to the RenderFrame itself?
> 
> Done. This adds a bit more changes though.

This could always be landed as a small separate CL. I think it will be cleaner,
then rebase this one on it and land it. Additionally, it will be easier to
bisect potential regressions if some crop up.

https://codereview.chromium.org/1811913003/diff/60001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1811913003/diff/60001/content/common/frame_me...
content/common/frame_messages.h:882: // Used to instruct the RenderFrame to go
into "view source" mode.
nit: Add a bit of comment that it is valid to send only for the main frame.

[clamy, 2016-03-24 13:30:04]

I split the IPC move in https://codereview.chromium.org/1832803002/. Will rebase
this CL when the other lands.

[clamy, 2016-03-24 16:24:09]

Now that the other CL landed, I'm landing this.

[clamy, 2016-03-24 16:24:14]

The CQ bit was checked by clamy@chromium.org

[clamy, 2016-03-24 16:24:15]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1811913003/#ps80001
(title: "Rebase on top of 1832803002")

[commit-bot: I haz the power, 2016-03-24 16:24:32]

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

[commit-bot: I haz the power, 2016-03-24 17:47:08]

Description was changed from

==========
PlzNavigate: support NavigationThrottle::WillProcessResponse

This CL introduces support in PlzNavigate for canceling navigations or pausing
navigations in NavigationThrottle::WillProcessResponse.

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

to

==========
PlzNavigate: support NavigationThrottle::WillProcessResponse

This CL introduces support in PlzNavigate for canceling navigations or pausing
navigations in NavigationThrottle::WillProcessResponse.

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

[commit-bot: I haz the power, 2016-03-24 17:47:09]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-24 17:48:05]

Description was changed from

==========
PlzNavigate: support NavigationThrottle::WillProcessResponse

This CL introduces support in PlzNavigate for canceling navigations or pausing
navigations in NavigationThrottle::WillProcessResponse.

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

to

==========
PlzNavigate: support NavigationThrottle::WillProcessResponse

This CL introduces support in PlzNavigate for canceling navigations or pausing
navigations in NavigationThrottle::WillProcessResponse.

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

Committed: https://crrev.com/5a3c36494131da824529e5abca31ffad62270f00
Cr-Commit-Position: refs/heads/master@{#383092}
==========

[commit-bot: I haz the power, 2016-03-24 17:48:06]

Patchset 5 (id:??) landed as
https://crrev.com/5a3c36494131da824529e5abca31ffad62270f00
Cr-Commit-Position: refs/heads/master@{#383092}