PlzNavigate: Use WebNavigationPolicyHandledByClient

PlzNavigate: Use WebNavigationPolicyHandledByClient

When PlzNavigate is enabled, this CL has
RenderFrameImpl::decidePolicyForNavigation return
WebNavigationPolicyHandledByClient when the navigation is sent ot the browser.
Before it would return WebNavigationPolicyIgnore, which would result in an
incorrect tracking of load state in the renderer.

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

Committed: https://crrev.com/2567242af04760d30f47a96e08bb0314a4c0b6b7
Cr-Commit-Position: refs/heads/master@{#376761}

[clamy, 2016-02-02 17:01:47]

Description was changed from

==========
PlzNavigate: Use WebNavigationPolicyHandledByClient

When PlzNavigate is enabled, this CL has
RenderFrameImpl::decidePolicyForNavigation return
WebNavigationPolicyHandledByClient when the navigation is sent ot the browser.
Before it would return WebNavigationPolicyIgnore, which would result in an
incorrect tracking of load state in the renderer.

BUG=576265
==========

to

==========
PlzNavigate: Use WebNavigationPolicyHandledByClient

When PlzNavigate is enabled, this CL has
RenderFrameImpl::decidePolicyForNavigation return
WebNavigationPolicyHandledByClient when the navigation is sent ot the browser.
Before it would return WebNavigationPolicyIgnore, which would result in an
incorrect tracking of load state in the renderer.

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

[clamy, 2016-02-02 17:02:39]

Description was changed from

==========
PlzNavigate: Use WebNavigationPolicyHandledByClient

When PlzNavigate is enabled, this CL has
RenderFrameImpl::decidePolicyForNavigation return
WebNavigationPolicyHandledByClient when the navigation is sent ot the browser.
Before it would return WebNavigationPolicyIgnore, which would result in an
incorrect tracking of load state in the renderer.

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

to

==========
PlzNavigate: Use WebNavigationPolicyHandledByClient

When PlzNavigate is enabled, this CL has
RenderFrameImpl::decidePolicyForNavigation return
WebNavigationPolicyHandledByClient when the navigation is sent ot the browser.
Before it would return WebNavigationPolicyIgnore, which would result in an
incorrect tracking of load state in the renderer.

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

[clamy, 2016-02-02 17:02:39]

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

[clamy, 2016-02-02 17:05:04]

PTAL: this is the followup on the is_loading patch to enable the right
navigation policy in PlzNavigate.

https://codereview.chromium.org/1608283002/diff/20001/testing/buildbot/filter...
File
testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter
(right):

https://codereview.chromium.org/1608283002/diff/20001/testing/buildbot/filter...
testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter:36:
-WebContentsImplBrowserTest.LoadProgressAfterInterruptedNav
This test is written in a way that is incompatible with PlzNavigate, it will
need to be rewritten.

[carlosk, 2016-02-03 12:41:56]

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

https://codereview.chromium.org/1608283002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1707: return;
Instead of ignoring shouldn't we stop it from being sent from the renderer in
case the navigation is to a different document? Otherwise maybe add a DCHECK
here that is_loading is set to true.

[nasko, 2016-02-03 22:28:14]

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

https://codereview.chromium.org/1608283002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1048: if (!is_loading()) {
Why not put this in the same section above, where we make a decision that this
is a synchronous navigation?

[clamy, 2016-02-10 16:39:35]

Thanks!

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

https://codereview.chromium.org/1608283002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1048: if (!is_loading()) {
On 2016/02/03 22:28:14, nasko wrote:
> Why not put this in the same section above, where we make a decision that this
> is a synchronous navigation?

Done.

https://codereview.chromium.org/1608283002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1707: return;
On 2016/02/03 12:41:56, carlosk wrote:
> Instead of ignoring shouldn't we stop it from being sent from the renderer in
> case the navigation is to a different document? Otherwise maybe add a DCHECK
> here that is_loading is set to true.

Done.

[nasko, 2016-02-10 23:02:19]

LGTM with one comment.

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

https://codereview.chromium.org/1608283002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1713:
DCHECK(!IsBrowserSideNavigationEnabled() || !to_different_document);
If the renderer is no longer supposed to be sending this IPC for synchronous
navigations in PlzNavigate mode, why is this not
bad_message::ReceivedBadMessage?

[carlosk, 2016-02-11 09:48:46]

LGTM.

[clamy, 2016-02-11 15:36:15]

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

[clamy, 2016-02-11 15:36:40]

Thanks!
@isherman: PTAL at the changes in histograms

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

https://codereview.chromium.org/1608283002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1713:
DCHECK(!IsBrowserSideNavigationEnabled() || !to_different_document);
On 2016/02/10 23:02:19, nasko wrote:
> If the renderer is no longer supposed to be sending this IPC for synchronous
> navigations in PlzNavigate mode, why is this not
> bad_message::ReceivedBadMessage?

Done.

[nasko, 2016-02-11 15:47:30]

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

https://codereview.chromium.org/1608283002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1715:
bad_message::RFH_UNEXPECTED_LOAD_START);
Do we want to proceed further in this method if this is a bad message? Most
places we just return.

[Ilya Sherman, 2016-02-11 23:52:12]

histograms.xml lgtm

[clamy, 2016-02-12 12:33:21]

Thanks!

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

https://codereview.chromium.org/1608283002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1715:
bad_message::RFH_UNEXPECTED_LOAD_START);
On 2016/02/11 15:47:30, nasko wrote:
> Do we want to proceed further in this method if this is a bad message? Most
> places we just return.

Done.

[clamy, 2016-02-12 18:56:31]

The CQ bit was checked by clamy@chromium.org

[clamy, 2016-02-12 18:56:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
carlosk@chromium.org, nasko@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1608283002/#ps100001
(title: "Return after bad message")

[commit-bot: I haz the power, 2016-02-12 18:58:45]

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

[commit-bot: I haz the power, 2016-02-12 20:28:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-12 20:28:03]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[clamy, 2016-02-22 16:55:53]

The CQ bit was checked by clamy@chromium.org

[clamy, 2016-02-22 16:55:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, carlosk@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1608283002/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2016-02-22 16:56:24]

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

[commit-bot: I haz the power, 2016-02-22 18:27:50]

Description was changed from

==========
PlzNavigate: Use WebNavigationPolicyHandledByClient

When PlzNavigate is enabled, this CL has
RenderFrameImpl::decidePolicyForNavigation return
WebNavigationPolicyHandledByClient when the navigation is sent ot the browser.
Before it would return WebNavigationPolicyIgnore, which would result in an
incorrect tracking of load state in the renderer.

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

to

==========
PlzNavigate: Use WebNavigationPolicyHandledByClient

When PlzNavigate is enabled, this CL has
RenderFrameImpl::decidePolicyForNavigation return
WebNavigationPolicyHandledByClient when the navigation is sent ot the browser.
Before it would return WebNavigationPolicyIgnore, which would result in an
incorrect tracking of load state in the renderer.

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

[commit-bot: I haz the power, 2016-02-22 18:27:51]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-02-22 18:29:13]

Description was changed from

==========
PlzNavigate: Use WebNavigationPolicyHandledByClient

When PlzNavigate is enabled, this CL has
RenderFrameImpl::decidePolicyForNavigation return
WebNavigationPolicyHandledByClient when the navigation is sent ot the browser.
Before it would return WebNavigationPolicyIgnore, which would result in an
incorrect tracking of load state in the renderer.

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

to

==========
PlzNavigate: Use WebNavigationPolicyHandledByClient

When PlzNavigate is enabled, this CL has
RenderFrameImpl::decidePolicyForNavigation return
WebNavigationPolicyHandledByClient when the navigation is sent ot the browser.
Before it would return WebNavigationPolicyIgnore, which would result in an
incorrect tracking of load state in the renderer.

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

Committed: https://crrev.com/2567242af04760d30f47a96e08bb0314a4c0b6b7
Cr-Commit-Position: refs/heads/master@{#376761}
==========

[commit-bot: I haz the power, 2016-02-22 18:29:14]

Patchset 7 (id:??) landed as
https://crrev.com/2567242af04760d30f47a96e08bb0314a4c0b6b7
Cr-Commit-Position: refs/heads/master@{#376761}