PlzNavigate: Remove the RequestNavigation IPC

PlzNavigate: Remove the RequestNavigation IPC

This CL removes the RequestNavigation IPC. Instead, browser-side navigation
will now send a BeforeUnload IPC to the renderer if needed and wait for a
BeforeUnloadACK to proceed with navigations.

BUG=376006
Committed: https://crrev.com/0ab288e2bdc393fb6b037f725ddd95fe4b39e64c
Cr-Commit-Position: refs/heads/master@{#314830}

[clamy, 2015-01-26 18:01:00]

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

[clamy, 2015-01-26 18:03:06]

@carlosk, davidben, nasko: PTAL
@fdegans: FYI

As we've been discussing for a while, here is a "small" refactoring to get rid
of RequestNavigation and replace it with using BeforeUnload instead. I do think
it improves the quality of the code, and makes it clear which navigation
requests are browser-initiated or not.

[nasko, 2015-02-02 21:17:44]

This is exciting! I like the simplifications in lots of areas in the code. Just
a few comments.

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl.cc:722: // This is a renderer
initiated navigation, so generate a new
nit: renderer-initiated

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl.cc:853: // Have the current renderer
execute its BeforeUnload event if needed. If it
nit: beforeUnload

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl.cc:859:
frame_tree_node->current_frame_host()->DispatchBeforeUnload(true);
Do we know this is a cross-site transition? It could be just regular
browser-initiated navigation for the same site.

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
File content/browser/frame_host/navigator_impl.h (right):

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl.h:115: // to execute the BeforeUnload
event. Otherwise, the navigation request will
nit: beforeUnload

https://chromiumcodereview.appspot.com/872473003/diff/1/content/common/naviga...
File content/common/navigation_params.h (right):

https://chromiumcodereview.appspot.com/872473003/diff/1/content/common/naviga...
content/common/navigation_params.h:72: struct CONTENT_EXPORT
RequestNavigationParams {
Do we still need this struct if we have removed the IPC?

https://chromiumcodereview.appspot.com/872473003/diff/1/content/common/naviga...
content/common/navigation_params.h:94: BeginNavigationParams();
Should we allow a parameterless constructor? The current version of this
constructor doesn't initialize defaults for method or headers.

[davidben, 2015-02-03 02:23:21]

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
File content/browser/frame_host/navigation_request.cc (right):

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigation_request.cc:66:
headers.SetHeaderIfMissing("Accept", "*/*");
This is the in the old code too, but Blink's default Accept line is somewhat
messier. I think partly because it's used to detect WebP (possible to fetch WebP
on a top-level request if you link directly to the image). The other stuff is I
think because crazy HTTP content-negotiation people successfully pushed for it.
(Why yes, I definitely want the same URL to respond with image, HTML, or RDF
based on Accept headers!)

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
File content/browser/frame_host/navigation_request.h (right):

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigation_request.h:74: // thread. After calling
this function, |body_| can no longer be manipulated
body_ doesn't seem to exist. I think this sentence is probably redundant with
the one in CreateRendererInitiated() and we can say that, as soon as you call
that function, you can't touch it from your thread anymore.

(Maybe add a TODO to see if we can un-refcount ResourceRequestBody? I haven't
looked into how painful that would be, but this threading subtly does suggest
that unique ownership would be better.)

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigation_request.h:78: CommonNavigationParams&
common_params() { return common_params_; }
I think this is no longer needed now that NavigatorImpl doesn't fill in the
referrer for this object.

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
File content/browser/frame_host/navigation_request_info.h (right):

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigation_request_info.h:31: const
BeginNavigationParams begin_params;
Perhaps CommonNavigationParams should also be placed here. It's a little odd
that NavigationURLLoader gets some parameters out of NavigationRequestInfo and
some out of an explicitly-passed CommonNavigationParams.

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
File content/browser/frame_host/navigator.h (right):

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigator.h:136: scoped_refptr<ResourceRequestBody>
body) {}
MSVC is being grumpy about the compile. I think the problem is this function
can't be inline; the empty body has an implicit call to
~scoped_refptr<ResourceRequestBody> which needs to have the definition of
ResourceRequestBody available. (Same reason CommitNavigation below isn't
inline.)

Not sure why it's only MSVC.

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl.cc:704:
NavigationRequest::WAITING_FOR_RENDERER_RESPONSE);
Nit: DCHECK_EQ

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl.cc:859:
frame_tree_node->current_frame_host()->DispatchBeforeUnload(true);
On 2015/02/02 21:17:43, nasko wrote:
> Do we know this is a cross-site transition? It could be just regular
> browser-initiated navigation for the same site.

Even if the navigation is same-site, I think we'll want beforeunload to be
driven by the browser.

I think the reason it's special in today's flow is that today we ask a renderer
to navigate on the browser's behalf. And if it's the same renderer, it'll run
beforeunload for us because that's what Blink does. But, in PlzNavigate, by the
time the renderer finds out about the navigation, it'll be too late for it to
beforeunload anyway.

[carlosk, 2015-02-03 16:06:02]

Whew! Well... After the first few files the rest was not that demanding... :)

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigation_controller_impl_unittest.cc:322:
main_test_rfh()->PrepareForCommit(urls[0]);
As PrepareForCommit has ASSERTs in its implementation and I noticed this pattern
of gtest code being added to subroutines I need to mention an important FYI:
those don't behave as you think they do and they might hide failures!

See:
https://code.google.com/p/googletest/wiki/AdvancedGuide#Using_Assertions_in_S...

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
File content/browser/frame_host/navigation_request.h (right):

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigation_request.h:78: CommonNavigationParams&
common_params() { return common_params_; }
On 2015/02/03 02:23:21, David Benjamin wrote:
> I think this is no longer needed now that NavigatorImpl doesn't fill in the
> referrer for this object.

It's still needed because it's accessed in some places to get information like
the navigation URL. But it most probably can become a const/const call.

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigation_request.h:135: CommonNavigationParams
common_params_;
I think this one could become const as well.

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl.cc:704:
NavigationRequest::WAITING_FOR_RENDERER_RESPONSE);
On 2015/02/03 02:23:21, David Benjamin wrote:
> Nit: DCHECK_EQ

Isn't it possible that the original request was cancelled AND another request
was created in the meantime?

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl.cc:729: navigation_request_map_.set(
Don't we need to verify first if another request is already ongoing?

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl.cc:860: return true;
This return doesn't seem to be meaningful anymore.

[clamy, 2015-02-03 16:17:10]

Thanks!

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigation_controller_impl_unittest.cc:322:
main_test_rfh()->PrepareForCommit(urls[0]);
On 2015/02/03 16:06:02, carlosk wrote:
> As PrepareForCommit has ASSERTs in its implementation and I noticed this
pattern
> of gtest code being added to subroutines I need to mention an important FYI:
> those don't behave as you think they do and they might hide failures!
> 
> See:
>
https://code.google.com/p/googletest/wiki/AdvancedGuide#Using_Assertions_in_S...
> 

Interesting. I'm not planning to change that in this patch though (already big
enough).

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
File content/browser/frame_host/navigation_request.cc (right):

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigation_request.cc:66:
headers.SetHeaderIfMissing("Accept", "*/*");
On 2015/02/03 02:23:20, David Benjamin wrote:
> This is the in the old code too, but Blink's default Accept line is somewhat
> messier. I think partly because it's used to detect WebP (possible to fetch
WebP
> on a top-level request if you link directly to the image). The other stuff is
I
> think because crazy HTTP content-negotiation people successfully pushed for
it.
> (Why yes, I definitely want the same URL to respond with image, HTML, or RDF
> based on Accept headers!)
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

Yeah we were missing a TODO to match what blink is doing with accept headers.
Added it.

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
File content/browser/frame_host/navigation_request.h (right):

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigation_request.h:74: // thread. After calling
this function, |body_| can no longer be manipulated
On 2015/02/03 02:23:20, David Benjamin wrote:
> body_ doesn't seem to exist. I think this sentence is probably redundant with
> the one in CreateRendererInitiated() and we can say that, as soon as you call
> that function, you can't touch it from your thread anymore.
> 
> (Maybe add a TODO to see if we can un-refcount ResourceRequestBody? I haven't
> looked into how painful that would be, but this threading subtly does suggest
> that unique ownership would be better.)

Done.

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigation_request.h:78: CommonNavigationParams&
common_params() { return common_params_; }
On 2015/02/03 16:06:02, carlosk wrote:
> On 2015/02/03 02:23:21, David Benjamin wrote:
> > I think this is no longer needed now that NavigatorImpl doesn't fill in the
> > referrer for this object.
> 
> It's still needed because it's accessed in some places to get information like
> the navigation URL. But it most probably can become a const/const call.

We already have a const accessor, so I removed the non-const one.

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigation_request.h:135: CommonNavigationParams
common_params_;
On 2015/02/03 16:06:02, carlosk wrote:
> I think this one could become const as well.

No, because redirects will modify the url there.

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
File content/browser/frame_host/navigation_request_info.h (right):

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigation_request_info.h:31: const
BeginNavigationParams begin_params;
On 2015/02/03 02:23:21, David Benjamin wrote:
> Perhaps CommonNavigationParams should also be placed here. It's a little odd
> that NavigationURLLoader gets some parameters out of NavigationRequestInfo and
> some out of an explicitly-passed CommonNavigationParams.

Done. Note that this is a copy of CommonNavigationParams since NavigationRequest
releases the ownership of the NavigationRequestInfo in BeginNavigation.

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
File content/browser/frame_host/navigator.h (right):

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigator.h:136: scoped_refptr<ResourceRequestBody>
body) {}
On 2015/02/03 02:23:21, David Benjamin wrote:
> MSVC is being grumpy about the compile. I think the problem is this function
> can't be inline; the empty body has an implicit call to
> ~scoped_refptr<ResourceRequestBody> which needs to have the definition of
> ResourceRequestBody available. (Same reason CommitNavigation below isn't
> inline.)
> 
> Not sure why it's only MSVC.

Done.

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl.cc:704:
NavigationRequest::WAITING_FOR_RENDERER_RESPONSE);
On 2015/02/03 02:23:21, David Benjamin wrote:
> Nit: DCHECK_EQ

Done.

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl.cc:704:
NavigationRequest::WAITING_FOR_RENDERER_RESPONSE);
On 2015/02/03 16:06:02, carlosk wrote:
> On 2015/02/03 02:23:21, David Benjamin wrote:
> > Nit: DCHECK_EQ
> 
> Isn't it possible that the original request was cancelled AND another request
> was created in the meantime? 

Yes but in that case we do not send another BeforeUnload IPC. So we should not
be receiving an ACK when we are in another state.

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl.cc:722: // This is a renderer
initiated navigation, so generate a new
On 2015/02/02 21:17:43, nasko wrote:
> nit: renderer-initiated

Done.

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl.cc:729: navigation_request_map_.set(
On 2015/02/03 16:06:02, carlosk wrote:
> Don't we need to verify first if another request is already ongoing?

That's the TODO above this block of code. The patch is large enough that I did
not want to go in fully implementing the navigation cancelation and associated
tests.

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl.cc:853: // Have the current renderer
execute its BeforeUnload event if needed. If it
On 2015/02/02 21:17:43, nasko wrote:
> nit: beforeUnload

Done.

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl.cc:859:
frame_tree_node->current_frame_host()->DispatchBeforeUnload(true);
On 2015/02/03 02:23:21, David Benjamin wrote:
> On 2015/02/02 21:17:43, nasko wrote:
> > Do we know this is a cross-site transition? It could be just regular
> > browser-initiated navigation for the same site.
> 
> Even if the navigation is same-site, I think we'll want beforeunload to be
> driven by the browser.
> 
> I think the reason it's special in today's flow is that today we ask a
renderer
> to navigate on the browser's behalf. And if it's the same renderer, it'll run
> beforeunload for us because that's what Blink does. But, in PlzNavigate, by
the
> time the renderer finds out about the navigation, it'll be too late for it to
> beforeunload anyway.

My understanding is that this flag is used by RenderFrameHostImpl to distinguish
whether we are waiting for the BeforeUnloadACK because we are navigating
(cross-site only in the current implementation) or because we are closing the
page. Given that we are navigating here, having for for_cross_site_transition
true made more sense. Maybe it could be renamed to for_navigation, which would
make it clearer.

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl.cc:860: return true;
On 2015/02/03 16:06:02, carlosk wrote:
> This return doesn't seem to be meaningful anymore.

We either always return true here, or in NavigateToEntry (since NavigateToEntry
needs a return value). Not sure which one is better.

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
File content/browser/frame_host/navigator_impl.h (right):

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl.h:115: // to execute the BeforeUnload
event. Otherwise, the navigation request will
On 2015/02/02 21:17:43, nasko wrote:
> nit: beforeUnload

Done.

https://chromiumcodereview.appspot.com/872473003/diff/1/content/common/naviga...
File content/common/navigation_params.h (right):

https://chromiumcodereview.appspot.com/872473003/diff/1/content/common/naviga...
content/common/navigation_params.h:72: struct CONTENT_EXPORT
RequestNavigationParams {
On 2015/02/02 21:17:43, nasko wrote:
> Do we still need this struct if we have removed the IPC?

Done.

https://chromiumcodereview.appspot.com/872473003/diff/1/content/common/naviga...
content/common/navigation_params.h:94: BeginNavigationParams();
On 2015/02/02 21:17:43, nasko wrote:
> Should we allow a parameterless constructor? The current version of this
> constructor doesn't initialize defaults for method or headers.

Actually it seems we need one to pass this struct as an IPC parameter.

[nasko, 2015-02-04 00:14:19]

LGTM

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl.cc:859:
frame_tree_node->current_frame_host()->DispatchBeforeUnload(true);
On 2015/02/03 16:17:09, clamy wrote:
> On 2015/02/03 02:23:21, David Benjamin wrote:
> > On 2015/02/02 21:17:43, nasko wrote:
> > > Do we know this is a cross-site transition? It could be just regular
> > > browser-initiated navigation for the same site.
> > 
> > Even if the navigation is same-site, I think we'll want beforeunload to be
> > driven by the browser.
> > 
> > I think the reason it's special in today's flow is that today we ask a
> renderer
> > to navigate on the browser's behalf. And if it's the same renderer, it'll
run
> > beforeunload for us because that's what Blink does. But, in PlzNavigate, by
> the
> > time the renderer finds out about the navigation, it'll be too late for it
to
> > beforeunload anyway.
> 
> My understanding is that this flag is used by RenderFrameHostImpl to
distinguish
> whether we are waiting for the BeforeUnloadACK because we are navigating
> (cross-site only in the current implementation) or because we are closing the
> page. Given that we are navigating here, having for for_cross_site_transition
> true made more sense. Maybe it could be renamed to for_navigation, which would
> make it clearer.

Indeed renaming it makes sense.

https://chromiumcodereview.appspot.com/872473003/diff/1/content/browser/frame...
content/browser/frame_host/navigator_impl.cc:860: return true;
On 2015/02/03 16:17:09, clamy wrote:
> On 2015/02/03 16:06:02, carlosk wrote:
> > This return doesn't seem to be meaningful anymore.
> 
> We either always return true here, or in NavigateToEntry (since
NavigateToEntry
> needs a return value). Not sure which one is better.

If this method cannot fail, it shouldn't be returning a boolean. I'm fine with
doing it in a separate CL to keep this one from exploding.

[clamy, 2015-02-05 12:42:31]

New patchsets have been uploaded after l-g-t-m from nasko@chromium.org

[clamy, 2015-02-05 15:33:13]

Thanks!

@davidben: friendly ping :)

https://codereview.chromium.org/872473003/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/872473003/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:859:
frame_tree_node->current_frame_host()->DispatchBeforeUnload(true);
On 2015/02/04 00:14:19, nasko wrote:
> On 2015/02/03 16:17:09, clamy wrote:
> > On 2015/02/03 02:23:21, David Benjamin wrote:
> > > On 2015/02/02 21:17:43, nasko wrote:
> > > > Do we know this is a cross-site transition? It could be just regular
> > > > browser-initiated navigation for the same site.
> > > 
> > > Even if the navigation is same-site, I think we'll want beforeunload to be
> > > driven by the browser.
> > > 
> > > I think the reason it's special in today's flow is that today we ask a
> > renderer
> > > to navigate on the browser's behalf. And if it's the same renderer, it'll
> run
> > > beforeunload for us because that's what Blink does. But, in PlzNavigate,
by
> > the
> > > time the renderer finds out about the navigation, it'll be too late for it
> to
> > > beforeunload anyway.
> > 
> > My understanding is that this flag is used by RenderFrameHostImpl to
> distinguish
> > whether we are waiting for the BeforeUnloadACK because we are navigating
> > (cross-site only in the current implementation) or because we are closing
the
> > page. Given that we are navigating here, having for
for_cross_site_transition
> > true made more sense. Maybe it could be renamed to for_navigation, which
would
> > make it clearer.
> 
> Indeed renaming it makes sense.

Included the renaming in this CL.

https://codereview.chromium.org/872473003/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:860: return true;
On 2015/02/04 00:14:19, nasko wrote:
> On 2015/02/03 16:17:09, clamy wrote:
> > On 2015/02/03 16:06:02, carlosk wrote:
> > > This return doesn't seem to be meaningful anymore.
> > 
> > We either always return true here, or in NavigateToEntry (since
> NavigateToEntry
> > needs a return value). Not sure which one is better.
> 
> If this method cannot fail, it shouldn't be returning a boolean. I'm fine with
> doing it in a separate CL to keep this one from exploding.

It's a simple enough change, so including it in this CL.

[davidben, 2015-02-05 15:59:49]

lgtm

https://codereview.chromium.org/872473003/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/872473003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:333: // PlzNavigate: this
calls happens on all navigations.
Nit: this calls -> this call.

(It's just the browser-initiated ones, right? Ditto with comment on
unload_ack_is_for_navigation_.)

[clamy, 2015-02-05 16:31:35]

New patchsets have been uploaded after l-g-t-m from davidben@chromium.org

[clamy, 2015-02-05 16:32:04]

Thanks!

https://codereview.chromium.org/872473003/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/872473003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:333: // PlzNavigate: this
calls happens on all navigations.
On 2015/02/05 15:59:49, David Benjamin wrote:
> Nit: this calls -> this call.
> 
> (It's just the browser-initiated ones, right? Ditto with comment on
> unload_ack_is_for_navigation_.)

Done.

[clamy, 2015-02-05 16:32:12]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2015-02-05 16:32:51]

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

[commit-bot: I haz the power, 2015-02-05 17:39:44]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-02-05 17:41:53]

Patchset 6 (id:??) landed as
https://crrev.com/0ab288e2bdc393fb6b037f725ddd95fe4b39e64c
Cr-Commit-Position: refs/heads/master@{#314830}