Send navigation_start to the browser in DidStartProvisionalLoad IPC

Send navigation_start to the browser in DidStartProvisionalLoad IPC

The timestamp gets attached to NavigationHandle, which exposes it to all consumers.

This is 3/3 of the effort to add a navigation_start timestamp to DidStartProvisional load. The full CL we're breaking up is here:
https://codereview.chromium.org/1425823002/

BUG=548335
Committed: https://crrev.com/e77e5ce0db3c24b3a5e9e5b8aac2afe410a37ceb
Cr-Commit-Position: refs/heads/master@{#359745}

[Charlie Harrison, 2015-11-03 19:02:18]

Description was changed from

==========
Send navigation_start to the browser in DidStartProvisionalLoad IPC

BUG=548335
==========

to

==========
Send navigation_start to the browser in DidStartProvisionalLoad IPC

The timestamp gets attached to NavigationHandle, which exposes it to all
consumers. 

This is 3/3 of the effort to add a navigation_start timestamp to
DidStartProvisional load. The full CL we're breaking up is here:
https://codereview.chromium.org/1425823002/

BUG=548335
==========

[Charlie Harrison, 2015-11-03 19:17:31]

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

[Charlie Harrison, 2015-11-03 19:17:31]

PTAL. Note this still leaves some functionality absent, notably the logic in
when the browser gets a BeforeUnloadACK. It should set the pending
NavigationHandle's nav_start to the time directly after the unload is dispatched
to the document.

We can do this in a later CL, or now.

[clamy, 2015-11-04 13:53:40]

Thanks! A few comments. BeforeUnload is only an issue with PlzNavigate (where we
create the NavigationHandle before issuing the BeforeUnload event). In the
current architecture, DidStartProvisionalLoad will always be received after the
BeforeUnload event has executed.

https://codereview.chromium.org/1427633004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1427633004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.h:168: // PlzNavigate:
Renderer-initated navigations send this with their
In PlzNavigate, it should always come from the CommonNavigationParams for
greater simplicity (and in the code it does !). Rephrase comment so that it is
simpler.

https://codereview.chromium.org/1427633004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.h:209: base::TimeTicks
navigation_start_;
This should be const.

https://codereview.chromium.org/1427633004/diff/1/content/renderer/render_vie...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/1427633004/diff/1/content/renderer/render_vie...
content/renderer/render_view_browsertest.cc:2313: TEST_F(RenderViewImplTest,
BrowserNavigationStartSuccessfullyTransmitted) {
Should you also test for a renderer initiated navigation? Same for a reload and
a history navigation?

https://codereview.chromium.org/1427633004/diff/1/content/renderer/render_vie...
content/renderer/render_view_browsertest.cc:2316: RequestNavigationParams
request_params;
No need to declare this, just use the default constructor in the function. Same
with the StartNavigationParams, since the navigation does not need to be a POST.

[Charlie Harrison, 2015-11-04 14:24:52]

Will work on the tests later today. Thanks for the prompt reviews!

https://codereview.chromium.org/1427633004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1427633004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.h:168: // PlzNavigate:
Renderer-initated navigations send this with their
On 2015/11/04 13:53:39, clamy wrote:
> In PlzNavigate, it should always come from the CommonNavigationParams for
> greater simplicity (and in the code it does !). Rephrase comment so that it is
> simpler.

Ahh you're right. Oops! Done.

https://codereview.chromium.org/1427633004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.h:209: base::TimeTicks
navigation_start_;
On 2015/11/04 13:53:39, clamy wrote:
> This should be const.

Done.

https://codereview.chromium.org/1427633004/diff/1/content/renderer/render_vie...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/1427633004/diff/1/content/renderer/render_vie...
content/renderer/render_view_browsertest.cc:2313: TEST_F(RenderViewImplTest,
BrowserNavigationStartSuccessfullyTransmitted) {
On 2015/11/04 13:53:40, clamy wrote:
> Should you also test for a renderer initiated navigation? Same for a reload
and
> a history navigation?

Yeah probably. Would you like me to write a bunch of test cases or one big one?

https://codereview.chromium.org/1427633004/diff/1/content/renderer/render_vie...
content/renderer/render_view_browsertest.cc:2316: RequestNavigationParams
request_params;
On 2015/11/04 13:53:39, clamy wrote:
> No need to declare this, just use the default constructor in the function.
Same
> with the StartNavigationParams, since the navigation does not need to be a
POST.

Acknowledged.

[clamy, 2015-11-04 15:59:09]

https://codereview.chromium.org/1427633004/diff/1/content/renderer/render_vie...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/1427633004/diff/1/content/renderer/render_vie...
content/renderer/render_view_browsertest.cc:2313: TEST_F(RenderViewImplTest,
BrowserNavigationStartSuccessfullyTransmitted) {
On 2015/11/04 14:24:52, csharrison wrote:
> On 2015/11/04 13:53:40, clamy wrote:
> > Should you also test for a renderer initiated navigation? Same for a reload
> and
> > a history navigation?
> 
> Yeah probably. Would you like me to write a bunch of test cases or one big
one?

I think smaller independant test cases are easier to read.

[Charlie Harrison, 2015-11-05 04:34:36]

Updated the tests. They needed a little more infra for back/forward navs.

[clamy, 2015-11-05 17:35:06]

Thanks! My main issue is with the update to the test infra, due to the
visibility of RenderViewTest. Consider adding what you need to
RenderViewImplTest instead, which is strictly private to content.

https://codereview.chromium.org/1427633004/diff/60001/content/public/test/ren...
File content/public/test/render_view_test.h (right):

https://codereview.chromium.org/1427633004/diff/60001/content/public/test/ren...
content/public/test/render_view_test.h:39: struct CommonNavigationParams;
I don't think these content internal classes should appear in the public API.

https://codereview.chromium.org/1427633004/diff/60001/content/public/test/ren...
content/public/test/render_view_test.h:205: void GoToOffsetWithParams(int
offset,
These content internal classes should not appear on the public API, especially
in a protected method (that could be used by a subclass outside of content).

[Charlie Harrison, 2015-11-05 17:40:12]

On 2015/11/05 17:35:06, clamy wrote:
> Thanks! My main issue is with the update to the test infra, due to the
> visibility of RenderViewTest. Consider adding what you need to
> RenderViewImplTest instead, which is strictly private to content.
> 
>
https://codereview.chromium.org/1427633004/diff/60001/content/public/test/ren...
> File content/public/test/render_view_test.h (right):
> 
>
https://codereview.chromium.org/1427633004/diff/60001/content/public/test/ren...
> content/public/test/render_view_test.h:39: struct CommonNavigationParams;
> I don't think these content internal classes should appear in the public API.
> 
>
https://codereview.chromium.org/1427633004/diff/60001/content/public/test/ren...
> content/public/test/render_view_test.h:205: void GoToOffsetWithParams(int
> offset,
> These content internal classes should not appear on the public API, especially
> in a protected method (that could be used by a subclass outside of content).

Hm good point. I'll just revert to something like what I did in patch set 3. I
only updated RenderViewTest to reduce code duplication.

[Charlie Harrison, 2015-11-05 18:18:13]

On 2015/11/05 17:40:12, csharrison wrote:
> On 2015/11/05 17:35:06, clamy wrote:
> > Thanks! My main issue is with the update to the test infra, due to the
> > visibility of RenderViewTest. Consider adding what you need to
> > RenderViewImplTest instead, which is strictly private to content.
> > 
> >
>
https://codereview.chromium.org/1427633004/diff/60001/content/public/test/ren...
> > File content/public/test/render_view_test.h (right):
> > 
> >
>
https://codereview.chromium.org/1427633004/diff/60001/content/public/test/ren...
> > content/public/test/render_view_test.h:39: struct CommonNavigationParams;
> > I don't think these content internal classes should appear in the public
API.
> > 
> >
>
https://codereview.chromium.org/1427633004/diff/60001/content/public/test/ren...
> > content/public/test/render_view_test.h:205: void GoToOffsetWithParams(int
> > offset,
> > These content internal classes should not appear on the public API,
especially
> > in a protected method (that could be used by a subclass outside of content).
> 
> Hm good point. I'll just revert to something like what I did in patch set 3. I
> only updated RenderViewTest to reduce code duplication.

Updated! Just moved GoToOffsetWithParams to RenderViewImplTest.

[clamy, 2015-11-06 14:28:40]

Thanks! The code inside content/ is fine, but there is an issue with the public
API. Once this gets resolved, it will be good for my part.

https://codereview.chromium.org/1427633004/diff/80001/content/public/browser/...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/1427633004/diff/80001/content/public/browser/...
content/public/browser/navigation_handle.h:45: virtual const base::TimeTicks&
GetNavigationStart() = 0;
The policy of the content public API forbids adding a method that is not used
outside of content/. Consider:
1) Adding code outside of content that use this.
or
2) Landing the method in NavigationHandleImpl and only making it public in the
CL that uses it outside of content/.

https://codereview.chromium.org/1427633004/diff/80001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/1427633004/diff/80001/content/renderer/render...
content/renderer/render_view_browsertest.cc:2351: TEST_F(RenderViewImplTest,
BrowserNavigationStartNotUsedForReload) {
Following the answer from igrigorik@ it seems we should use the browser provided
value for reloads and history navigations as well. It's up to you to see if this
should go in this patch or a next one.

[Charlie Harrison, 2015-11-06 15:09:37]

On 2015/11/06 14:28:40, clamy wrote:
> Thanks! The code inside content/ is fine, but there is an issue with the
public
> API. Once this gets resolved, it will be good for my part.
> 
>
https://codereview.chromium.org/1427633004/diff/80001/content/public/browser/...
> File content/public/browser/navigation_handle.h (right):
> 
>
https://codereview.chromium.org/1427633004/diff/80001/content/public/browser/...
> content/public/browser/navigation_handle.h:45: virtual const base::TimeTicks&
> GetNavigationStart() = 0;
> The policy of the content public API forbids adding a method that is not used
> outside of content/. Consider:
> 1) Adding code outside of content that use this.
> or
> 2) Landing the method in NavigationHandleImpl and only making it public in the
> CL that uses it outside of content/.
> 
>
https://codereview.chromium.org/1427633004/diff/80001/content/renderer/render...
> File content/renderer/render_view_browsertest.cc (right):
> 
>
https://codereview.chromium.org/1427633004/diff/80001/content/renderer/render...
> content/renderer/render_view_browsertest.cc:2351: TEST_F(RenderViewImplTest,
> BrowserNavigationStartNotUsedForReload) {
> Following the answer from igrigorik@ it seems we should use the browser
provided
> value for reloads and history navigations as well. It's up to you to see if
this
> should go in this patch or a next one.

Oh okay, didn't know about that rule. I removed it from NavigationHandle for
now. I have a CL in the works that will re-add it. I'll create an issue tracking
the browser/history stuff, as I'm still not confident that they should get the
browser-initiated timestamp.

[clamy, 2015-11-06 16:28:20]

Thanks! Lgtm with 2 nits.

https://codereview.chromium.org/1427633004/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1427633004/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:94: // TODO(csharrison) add
this to the public class and mark this override.
nit: please put the comment that was with the public method here (it can go back
to the public .h file when the method is made public).

https://codereview.chromium.org/1427633004/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:168: // |navigation_start|
comes from the DidStartProvisionalLoad IPC, which tracks
nit: I would move this comment to the factory method in the public part.

[Charlie Harrison, 2015-11-11 23:54:51]

nasko@ friendly ping

[nasko, 2015-11-13 21:52:52]

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

[nasko, 2015-11-13 21:52:54]

Adding avi@ for review of history navigation in the unit test.

https://codereview.chromium.org/1427633004/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1427633004/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:102: const base::TimeTicks&
NavigationHandleImpl::GetNavigationStart() {
This is just an accessor, should be hacker_cased and inlined in the header file.

https://codereview.chromium.org/1427633004/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1427633004/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:99: // process. Corresponds
to Navigation Timing API Standard.
nit: Drop "Standard".

https://codereview.chromium.org/1427633004/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:100: // TODO(csharrison) add
this to the public class and mark this override.
nit: Missing ":" after ). Also, start the comment with capital letter, as it is
a sentence.

https://codereview.chromium.org/1427633004/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/1427633004/diff/120001/content/browser/frame_...
content/browser/frame_host/navigator.h:59: const base::TimeTicks&
navigation_start){};
nit: There should be space between ) and {}. Is this clang-format clean?

https://codereview.chromium.org/1427633004/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1427633004/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:951: validated_params.url,
frame_tree_node_, base::TimeTicks::Now());
Why not the actual start time from the renderer process?

https://codereview.chromium.org/1427633004/diff/120001/content/renderer/rende...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/1427633004/diff/120001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:179: int pending_offset = offset +
view()->history_list_offset_;
Hmmm, not sure if asking the view() is the right thing long term, as we are
moving history navigation around. Will add avi@ to take a poke at this one.

https://codereview.chromium.org/1427633004/diff/120001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:2357: common_params.transition =
ui::PAGE_TRANSITION_LINK;
Why not PAGE_TRANSITION_RELOAD?

https://codereview.chromium.org/1427633004/diff/120001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:2370:
EXPECT_GT(base::get<1>(host_nav_params), common_params.navigation_start);
Do we have enough granularity to ensure we are not going to collide if the test
runs too fast? It can lead to flakiness if not. Maybe check if the ticks object
is high resolution or not?
https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&l...

https://codereview.chromium.org/1427633004/diff/120001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:2395: &host_nav_params);
This code for reading the IPC is done enough times that it might be good to
extract it into a helper method called by all the tests using it.

[Charlie Harrison, 2015-11-14 00:25:00]

Thanks for the review!

https://codereview.chromium.org/1427633004/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1427633004/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:102: const base::TimeTicks&
NavigationHandleImpl::GetNavigationStart() {
On 2015/11/13 21:52:54, nasko (slow to review) wrote:
> This is just an accessor, should be hacker_cased and inlined in the header
file.

Ah okay. Wasn't aware of that rule for accessors. Done.

https://codereview.chromium.org/1427633004/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1427633004/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:99: // process. Corresponds
to Navigation Timing API Standard.
On 2015/11/13 21:52:54, nasko (slow to review) wrote:
> nit: Drop "Standard".

Done.

https://codereview.chromium.org/1427633004/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:100: // TODO(csharrison) add
this to the public class and mark this override.
On 2015/11/13 21:52:54, nasko (slow to review) wrote:
> nit: Missing ":" after ). Also, start the comment with capital letter, as it
is
> a sentence.

Done.

https://codereview.chromium.org/1427633004/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/1427633004/diff/120001/content/browser/frame_...
content/browser/frame_host/navigator.h:59: const base::TimeTicks&
navigation_start){};
On 2015/11/13 21:52:54, nasko (slow to review) wrote:
> nit: There should be space between ) and {}. Is this clang-format clean?

That's odd. clang-format actually prefers no space for me. Added the space.

https://codereview.chromium.org/1427633004/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1427633004/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:951: validated_params.url,
frame_tree_node_, base::TimeTicks::Now());
On 2015/11/13 21:52:54, nasko (slow to review) wrote:
> Why not the actual start time from the renderer process?

We discussed this briefly over email, but I think to track same-page navigations
like this from the renderer, we would need to send the timestamp on commit IPCs
as well as provisional. Since these navigations are less important, I figured
that adding this additional timestamp and complexity would not be worth it until
a consumer wanted it.

https://codereview.chromium.org/1427633004/diff/120001/content/renderer/rende...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/1427633004/diff/120001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:179: int pending_offset = offset +
view()->history_list_offset_;
On 2015/11/13 21:52:54, nasko (slow to review) wrote:
> Hmmm, not sure if asking the view() is the right thing long term, as we are
> moving history navigation around. Will add avi@ to take a poke at this one.

For context, most of this logic is copied from render_view_test's GoToOffset. I
just needed to finely tune the parameters.

https://codereview.chromium.org/1427633004/diff/120001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:2357: common_params.transition =
ui::PAGE_TRANSITION_LINK;
On 2015/11/13 21:52:54, nasko (slow to review) wrote:
> Why not PAGE_TRANSITION_RELOAD?

No good reason. Fixed.

https://codereview.chromium.org/1427633004/diff/120001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:2370:
EXPECT_GT(base::get<1>(host_nav_params), common_params.navigation_start);
On 2015/11/13 21:52:54, nasko (slow to review) wrote:
> Do we have enough granularity to ensure we are not going to collide if the
test
> runs too fast? It can lead to flakiness if not. Maybe check if the ticks
object
> is high resolution or not?
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&l...

Good idea. It should be monotonic either way, so I'll make the check GTE if
we're using the low resolution timer, because we could possibly mark the
navigationStart in the renderer within 15ms (assuming comment in time.h is
correct).

https://codereview.chromium.org/1427633004/diff/120001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:2395: &host_nav_params);
On 2015/11/13 21:52:54, nasko (slow to review) wrote:
> This code for reading the IPC is done enough times that it might be good to
> extract it into a helper method called by all the tests using it.

Sounds good to me. This might not end up being a win for readability unless we
can templatize it. Uploading a draft now to apply to just my tests. I can
probably make a separate CL later cleaning up other tests.

[nasko, 2015-11-14 00:33:24]

LGTM with a few nits. Please wait for avi@'s review on the bit in the tests that
does session history navigation.

https://codereview.chromium.org/1427633004/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1427633004/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:102: const base::TimeTicks&
NavigationHandleImpl::GetNavigationStart() {
On 2015/11/14 00:24:59, csharrison wrote:
> On 2015/11/13 21:52:54, nasko (slow to review) wrote:
> > This is just an accessor, should be hacker_cased and inlined in the header
> file.
> 
> Ah okay. Wasn't aware of that rule for accessors. Done.

Maybe freshen up on the style guide ;).

https://codereview.chromium.org/1427633004/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/1427633004/diff/120001/content/browser/frame_...
content/browser/frame_host/navigator.h:59: const base::TimeTicks&
navigation_start){};
On 2015/11/14 00:25:00, csharrison wrote:
> On 2015/11/13 21:52:54, nasko (slow to review) wrote:
> > nit: There should be space between ) and {}. Is this clang-format clean?
> 
> That's odd. clang-format actually prefers no space for me. Added the space.

This might be some bug in clang-format. If it repros for you, file a bug. It
seems inconsistent with the rest of the file, so let's leave it with the space
in and investigate the possible bug async.

https://codereview.chromium.org/1427633004/diff/120001/content/renderer/rende...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/1427633004/diff/120001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:2395: &host_nav_params);
On 2015/11/14 00:25:00, csharrison wrote:
> On 2015/11/13 21:52:54, nasko (slow to review) wrote:
> > This code for reading the IPC is done enough times that it might be good to
> > extract it into a helper method called by all the tests using it.
> 
> Sounds good to me. This might not end up being a win for readability unless we
> can templatize it. Uploading a draft now to apply to just my tests. I can
> probably make a separate CL later cleaning up other tests.

This looks good.

https://codereview.chromium.org/1427633004/diff/140001/content/renderer/rende...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/1427633004/diff/140001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:140: // Timestamps logged within
~15ms of each other under low resolution timers
nit: No need to quote exact time difference, as this isn't guaranteed by your
code here at all.

https://codereview.chromium.org/1427633004/diff/140001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:2362: ASSERT_PRED2(TimeTicksGT,
base::get<1>(host_nav_params),
EXPECT

[Charlie Harrison, 2015-11-14 00:53:13]

Done. I'll look into the clang-format bug. Thanks again.

[Avi (use Gerrit), 2015-11-14 01:08:34]

This is essentially a unit test where everything is faked, and this isn't an
unreasonable way of doing it.

LGTM with nits.

https://codereview.chromium.org/1427633004/diff/160001/content/renderer/rende...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/1427633004/diff/160001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:142: // constraints o systems with
these timers.
s/o/on/

https://codereview.chromium.org/1427633004/diff/160001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:185:
view()->historyForwardListCount() + 1;
If you're going into the RenderViewImpl and pulling out the history_list_offset_
on the next line, why not just go in and use the history_list_length_ here?

[Charlie Harrison, 2015-11-14 01:30:45]

Done. Thanks Avi!

[Charlie Harrison, 2015-11-14 01:31:10]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2015-11-14 01:31:10]

The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org,
nasko@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1427633004/#ps180001
(title: "Avi review")

[commit-bot: I haz the power, 2015-11-14 01:33:16]

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

[commit-bot: I haz the power, 2015-11-14 02:17:47]

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

[commit-bot: I haz the power, 2015-11-14 02:17:49]

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

[Charlie Harrison, 2015-11-15 00:30:07]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2015-11-15 00:30:07]

The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org,
avi@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1427633004/#ps200001
(title: "Conflict Resolution: FrameHostMsg_DidStartProvisionalLoad")

[commit-bot: I haz the power, 2015-11-15 00:30:43]

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

[commit-bot: I haz the power, 2015-11-15 02:31:26]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-11-15 02:32:23]

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