Classify navigations without page id in parallel to the existing classifier.

Classify navigations without page id in parallel to the existing classifier.

For now, this only happens in debug builds.

BUG=369661
TEST=NavigationControllerBrowserTest.NavigationTypeClassification_*
TEST=Every other test on the planet.

Committed: https://crrev.com/d8d93348bbd8c646c337bdaa40fc0c64204fc5ff
Cr-Commit-Position: refs/heads/master@{#327122}

Reverted: https://crrev.com/5348e920f4119aff9a4eb76c0965725dc85a66cc
Cr-Revert-Position: refs/heads/master@{#327152}

Committed: https://crrev.com/5671403d44971669e4d81aecf3f002188ce0e95f
Cr-Commit-Position: refs/heads/master@{#327214}

Reverted: https://crrev.com/49180eb13549e440bbd4f66390e32e84699dcdfd
Cr-Revert-Position: refs/heads/master@{#327269}

Committed: https://crrev.com/a038c6670f450313a8e224ccc5d05dd59e3488c4
Cr-Commit-Position: refs/heads/master@{#328131}

Reverted: https://crrev.com/1f5d6196b8d53e70e7a7a2276cdf3481365e7e04
Cr-Revert-Position: refs/heads/master@{#328282}

Committed: https://crrev.com/61cae85448cfeb793270e804b5ad1023993279c5
Cr-Commit-Position: refs/heads/master@{#328829}

Reverted: https://crrev.com/c0b9c8f3333acddb945165bce44f5605dcf56f0f
Cr-Revert-Position: refs/heads/master@{#328924}

Committed: https://crrev.com/7c6f35e1d8731ff5c51ee3910ba24a14e7a8d87c
Cr-Commit-Position: refs/heads/master@{#328979}

[Avi (use Gerrit), 2015-03-12 16:31:50]

avi@chromium.org changed reviewers:
+ clamy@chromium.org, creis@chromium.org

[Avi (use Gerrit), 2015-03-12 16:31:50]

Charlie: WDYT? Not yet ready for commit, but I want your thoughts
Camille: This collided with your recent change, so I'm curious if this prompts
any comment for you

[Charlie Reis, 2015-03-12 18:28:17]

Nice.  This seems like quite a reasonable start.  The test failures should help
point out the remaining cases to address.

A few minor notes below on first pass.

https://codereview.chromium.org/1002803002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1002803002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1080: // This must be
an inert commit (standard commits are handled above,
Can you explain what inert commit means?  That's a Blink term that I'm not
familiar with.

https://codereview.chromium.org/1002803002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1081: // back/forward
commits are always browser-initiated, and initial child
Minor wording issue, though I'm not sure if it changes your decisions:
Back/forward commits are not always browser-initiated.  They do always go
through the browser process, but they continue to track whether they were
renderer-initiated (e.g., history.back()) or not (e.g., Back button).

https://codereview.chromium.org/1002803002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1118:
RecordAction(base::UserMetricsAction("BadMessageTerminate_NC"));
We probably don't want to kill the renderer in both versions of this method
(i.e., it's not idempotent).  That said, we probably want to keep some version
of the kill when we move to the new approach.  It'd be nice to be able to
distinguish between the new reports and the old reports, since I expect they
will differ (and maybe even go away?).

https://codereview.chromium.org/1002803002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.h (right):

https://codereview.chromium.org/1002803002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.h:237: // deprecated page
id values. http://crbug.com/369661
Mention that we currently only call it in debug builds.

https://codereview.chromium.org/1002803002/diff/40001/content/common/DEPS
File content/common/DEPS (right):

https://codereview.chromium.org/1002803002/diff/40001/content/common/DEPS#new...
content/common/DEPS:50: "+third_party/WebKit/public/web/WebHistoryCommitType.h",
I'm hesitant to add this.

https://codereview.chromium.org/1002803002/diff/40001/content/common/navigati...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/1002803002/diff/40001/content/common/navigati...
content/common/navigation_params.h:157: int32 page_id;
Maybe put a TODO(avi) to remove this in favor of nav_entry_id?

https://codereview.chromium.org/1002803002/diff/40001/content/public/common/f...
File content/public/common/frame_navigate_params.h (right):

https://codereview.chromium.org/1002803002/diff/40001/content/public/common/f...
content/public/common/frame_navigate_params.h:28: int32 page_id;
Same here.

https://codereview.chromium.org/1002803002/diff/40001/content/public/renderer...
File content/public/renderer/navigation_state.h (right):

https://codereview.chromium.org/1002803002/diff/40001/content/public/renderer...
content/public/renderer/navigation_state.h:42: int32 pending_page_id() const {
return pending_page_id_; }
Same here.

https://codereview.chromium.org/1002803002/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1002803002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:2562: successful_history_nav = true;
nit: Put below the following comment.

https://codereview.chromium.org/1002803002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:3724: params.commit_type = commit_type;
I'd like to avoid exposing this directly if possible.  It looks like we only
need a bool for whether it's a standard commit in the long run, since we're only
checking whether it's Inert in DCHECKs.

(I'm fine with it for now as we explore what's needed to get this to work.)

[Avi (use Gerrit), 2015-03-12 19:21:18]

https://codereview.chromium.org/1002803002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1002803002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1080: // This must be
an inert commit (standard commits are handled above,
On 2015/03/12 18:28:17, Charlie Reis wrote:
> Can you explain what inert commit means?  That's a Blink term that I'm not
> familiar with.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

There are four Blink commit types: standard, back/forward, initial in child
frame, inert. That is what I'm referring to here.

https://codereview.chromium.org/1002803002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1081: // back/forward
commits are always browser-initiated, and initial child
On 2015/03/12 18:28:17, Charlie Reis wrote:
> Minor wording issue, though I'm not sure if it changes your decisions:
> Back/forward commits are not always browser-initiated.  They do always go
> through the browser process, but they continue to track whether they were
> renderer-initiated (e.g., history.back()) or not (e.g., Back button).

That doesn't change the code.

Because the request comes via the browser (whether or not it's initiated by it)
it will have a nav entry id.

I will clarify the comment.

https://codereview.chromium.org/1002803002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1118:
RecordAction(base::UserMetricsAction("BadMessageTerminate_NC"));
On 2015/03/12 18:28:17, Charlie Reis wrote:
> We probably don't want to kill the renderer in both versions of this method
> (i.e., it's not idempotent).  That said, we probably want to keep some version
> of the kill when we move to the new approach.  It'd be nice to be able to
> distinguish between the new reports and the old reports, since I expect they
> will differ (and maybe even go away?).

Done.

https://codereview.chromium.org/1002803002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.h (right):

https://codereview.chromium.org/1002803002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.h:237: // deprecated page
id values. http://crbug.com/369661
On 2015/03/12 18:28:17, Charlie Reis wrote:
> Mention that we currently only call it in debug builds.

That depends on how we want to land this.

https://codereview.chromium.org/1002803002/diff/40001/content/common/DEPS
File content/common/DEPS (right):

https://codereview.chromium.org/1002803002/diff/40001/content/common/DEPS#new...
content/common/DEPS:50: "+third_party/WebKit/public/web/WebHistoryCommitType.h",
On 2015/03/12 18:28:17, Charlie Reis wrote:
> I'm hesitant to add this.

Acknowledged.

https://codereview.chromium.org/1002803002/diff/40001/content/common/navigati...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/1002803002/diff/40001/content/common/navigati...
content/common/navigation_params.h:157: int32 page_id;
On 2015/03/12 18:28:17, Charlie Reis wrote:
> Maybe put a TODO(avi) to remove this in favor of nav_entry_id?

It feels weird to tag this one place that uses page id and not tag every other
place.

https://codereview.chromium.org/1002803002/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1002803002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:2562: successful_history_nav = true;
On 2015/03/12 18:28:17, Charlie Reis wrote:
> nit: Put below the following comment.

Done.

https://codereview.chromium.org/1002803002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:3724: params.commit_type = commit_type;
On 2015/03/12 18:28:17, Charlie Reis wrote:
> I'd like to avoid exposing this directly if possible.  It looks like we only
> need a bool for whether it's a standard commit in the long run, since we're
only
> checking whether it's Inert in DCHECKs.
> 
> (I'm fine with it for now as we explore what's needed to get this to work.)

Let me hack on getting the tests to pass, but I'm OK with making it a boolean.

[clamy, 2015-03-16 10:39:55]

Thanks! A few comments on how the patch interacts with PlzNavigate. I'm very
excited about that patch, since we had issues with GetEntryIndexWithPageID which
should be solved by GetEntryIndexWithUniqueID.

I'm also working on a refactoring of the NavigationState which will probably
collide with your patch (but may make things more understandable):
https://codereview.chromium.org/1001043002/.

https://codereview.chromium.org/1002803002/diff/140001/content/browser/frame_...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/1002803002/diff/140001/content/browser/frame_...
content/browser/frame_host/navigator_impl_unittest.cc:70:
static_cast<NavigatorImpl*>(node->navigator())->RequestNavigation(
As explained by mail this is likely to cause a few problems since we bypass the
NavigationController in this helper function, which is used in all the tests
below.

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

https://codereview.chromium.org/1002803002/diff/140001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3960: // TODO(clamy): reset the pending
navigation parameters in RenderView.
As noted in the TODO here we do not reset the NavigationParams properly in
RenderView when committing the navigation in PlzNavigate. This will cause
RenderViewImpl::didCreateDataSource to believe the request was
renderer-initiated and we will not get back the proper navigation entry unique
ID in the DidCommitProvisionalLoad.

[Avi (use Gerrit), 2015-04-09 22:35:09]

Omigod all the bots are green! Review, review, review!

Charlie, Camille, please take a look at the latest patch set.

[clamy, 2015-04-10 12:05:45]

Thanks! A few questions below.

https://codereview.chromium.org/1002803002/diff/360001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1002803002/diff/360001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:815: details->type =
ClassifyNavigation(rfh, params);
With PlzNavigate enabled, ClassifyNavigation fails in the
NavigationControllerTest.CopyRestoredStateAndNavigate test because of a hack
with SiteInstances. Do you think it is possible for us to use
ClassifyNavigationWithoutPageID instead? (I can do it in a follow-up patch to
this).

https://codereview.chromium.org/1002803002/diff/360001/content/browser/frame_...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/1002803002/diff/360001/content/browser/frame_...
content/browser/frame_host/navigator_impl_unittest.cc:56: return
controller().GetPendingEntry()->GetUniqueID();
Should the return come from RequestNavigationWithParameters? It might be clearer
why we can call controller().GetPendingEntry()->GetUniqueID() (since we just
call controller().LoadURLWithParams before).

https://codereview.chromium.org/1002803002/diff/360001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1002803002/diff/360001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1123: DCHECK_NE(0,
request_params.nav_entry_id);
Considering that we are in an IPC handler for a browser-initiated navigation,
shouldn't we always have created an entry for the navigation, and so have
request_params.nav_entry_id not be 0? I guess the only problem would be some
RenderViewTests which call Onnavigate directly, but the RequestNavigationParams
we pass there could be modified to have a non-null nav_entry_id.

[Charlie Reis, 2015-04-10 23:54:21]

I like it.

I glossed over some of the test changes (which seem pretty mechanical) and
focused on the logic changes for now.  A few questions and nits below.

https://codereview.chromium.org/1002803002/diff/360001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1002803002/diff/360001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:1041:
existing_entry->GetURL() == pending_entry_->GetVirtualURL()) {
We probably shouldn't be changing ClassifyNavigation in the same CL that we're
introducing ClassifyNavigationWithoutPageID.  Can we pull this part out into a
preliminary CL?

https://codereview.chromium.org/1002803002/diff/360001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:1077: //
PAGE_TRANSITION_AUTO_SUBFRAME transition. Fix those, and adjust here.
I like this plan.

https://codereview.chromium.org/1002803002/diff/360001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:1108: if
(params.nav_entry_id == 0) {
Can you add a comment saying what nav_entry_id == 0 means?  (I'm guessing from
context below that it equates to renderer-initiated navigations?  Or is it just
a subset of those?)

https://codereview.chromium.org/1002803002/diff/360001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.h (right):

https://codereview.chromium.org/1002803002/diff/360001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.h:238: // deprecated page
id values. http://crbug.com/369661
Also mention that it will replace ClassifyNavigation once we verify that they
behave the same.

https://codereview.chromium.org/1002803002/diff/360001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1002803002/diff/360001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:183: return;
I'm a little surprised this didn't cause issues with the Web Navigation API,
which has pretty specific expectations for hearing about failures.  Maybe it has
already given up on the outstanding request at the time the new pending entry
was created?

(This strikes me as another place that might cause regressions, so it might
benefit from landing in a separate CL.)

https://codereview.chromium.org/1002803002/diff/360001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1002803002/diff/360001/content/common/frame_m...
content/common/frame_messages.h:148: // FrameNavigateParams, instead?
If we don't know it until commit time, then it probably belongs here rather than
FrameNavigateParams.

https://codereview.chromium.org/1002803002/diff/360001/content/public/test/te...
File content/public/test/test_renderer_host.h (right):

https://codereview.chromium.org/1002803002/diff/360001/content/public/test/te...
content/public/test/test_renderer_host.h:78: int nav_entry_id,
Would it help to give some tips for what nav_entry_id should be set to (e.g.,
when it's 0 or not)?

Also, maybe a TODO that page_id is going away?

https://codereview.chromium.org/1002803002/diff/360001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1002803002/diff/360001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2671: // Checking the commit type is
planned to be the replacement for the if()
Would it be better to just have
DCHECK_EQ(successful_history_nav, commit_type == blink::WebBackForwardCommit) ?

The current if (A) DCHECK(B) wouldn't catch cases that were successful history
navs before but weren't WebBackForwardCommits.

[Avi (use Gerrit), 2015-04-13 22:42:49]

https://codereview.chromium.org/1002803002/diff/360001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1002803002/diff/360001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:815: details->type =
ClassifyNavigation(rfh, params);
On 2015/04/10 12:05:44, clamy wrote:
> With PlzNavigate enabled, ClassifyNavigation fails in the
> NavigationControllerTest.CopyRestoredStateAndNavigate test because of a hack
> with SiteInstances. Do you think it is possible for us to use
> ClassifyNavigationWithoutPageID instead? (I can do it in a follow-up patch to
> this).

My current plan is to commit this with the DCHECK (ensuring they behave the same
in debug builds), then switch to CHECK, then switch over.

I want to get some time for it to bake and for us to be confident in it. I
understand that this CL unblocks you, but I want to be sure it doesn't break
things before unleashing it onto the world.

https://codereview.chromium.org/1002803002/diff/360001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:1041:
existing_entry->GetURL() == pending_entry_->GetVirtualURL()) {
On 2015/04/10 23:54:21, Charlie Reis (slow until 4-15) wrote:
> We probably shouldn't be changing ClassifyNavigation in the same CL that we're
> introducing ClassifyNavigationWithoutPageID.  Can we pull this part out into a
> preliminary CL?

https://codereview.chromium.org/1082083002

It should pass all the tests. Fingers crossed.

https://codereview.chromium.org/1002803002/diff/360001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:1077: //
PAGE_TRANSITION_AUTO_SUBFRAME transition. Fix those, and adjust here.
On 2015/04/10 23:54:20, Charlie Reis (slow until 4-15) wrote:
> I like this plan.

Acknowledged.

PageTransitionIsMainFrame is used in a *lot* of places in NavController, not
just in ClassifyNavigation. I would prefer to switch them all over.

https://codereview.chromium.org/1002803002/diff/360001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:1108: if
(params.nav_entry_id == 0) {
On 2015/04/10 23:54:20, Charlie Reis (slow until 4-15) wrote:
> Can you add a comment saying what nav_entry_id == 0 means?  (I'm guessing from
> context below that it equates to renderer-initiated navigations?  Or is it
just
> a subset of those?)

Done.

https://codereview.chromium.org/1002803002/diff/360001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.h (right):

https://codereview.chromium.org/1002803002/diff/360001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.h:238: // deprecated page
id values. http://crbug.com/369661
On 2015/04/10 23:54:21, Charlie Reis (slow until 4-15) wrote:
> Also mention that it will replace ClassifyNavigation once we verify that they
> behave the same.

Done.

https://codereview.chromium.org/1002803002/diff/360001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1002803002/diff/360001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:183: return;
On 2015/04/10 23:54:21, Charlie Reis (slow until 4-15) wrote:
> I'm a little surprised this didn't cause issues with the Web Navigation API,
> which has pretty specific expectations for hearing about failures.  Maybe it
has
> already given up on the outstanding request at the time the new pending entry
> was created?

This was full of surprise to me, and I was kinda shocked we didn't already
handle this case.

> (This strikes me as another place that might cause regressions, so it might
> benefit from landing in a separate CL.)

I was thinking of doing that, but the problem is we can't with the current
system.

- This particular fix relies on nav_entry_id, which of course only exists in
this CL.
- A general fix can't be done with page_id. Because page_id is assigned by the
renderer, the browser has no general cookie value it can use to match up the
failures from the renderer with the pending entry. The fact that we round-trip a
cookie is the only reason we _can_ now catch this.

But yeah, if you can think of a good way to fix this independently, I would
definitely like to land this separately.

https://codereview.chromium.org/1002803002/diff/360001/content/browser/frame_...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/1002803002/diff/360001/content/browser/frame_...
content/browser/frame_host/navigator_impl_unittest.cc:56: return
controller().GetPendingEntry()->GetUniqueID();
On 2015/04/10 12:05:44, clamy wrote:
> Should the return come from RequestNavigationWithParameters? It might be
clearer
> why we can call controller().GetPendingEntry()->GetUniqueID() (since we just
> call controller().LoadURLWithParams before).

Done.

https://codereview.chromium.org/1002803002/diff/360001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1002803002/diff/360001/content/common/frame_m...
content/common/frame_messages.h:148: // FrameNavigateParams, instead?
On 2015/04/10 23:54:21, Charlie Reis (slow until 4-15) wrote:
> If we don't know it until commit time, then it probably belongs here rather
than
> FrameNavigateParams.

Good point.

https://codereview.chromium.org/1002803002/diff/360001/content/public/test/te...
File content/public/test/test_renderer_host.h (right):

https://codereview.chromium.org/1002803002/diff/360001/content/public/test/te...
content/public/test/test_renderer_host.h:78: int nav_entry_id,
On 2015/04/10 23:54:21, Charlie Reis (slow until 4-15) wrote:
> Would it help to give some tips for what nav_entry_id should be set to (e.g.,
> when it's 0 or not)?

I'm not sure. There's no documentation on how to fill out the page_id field in
this and a zillion other calls.

> Also, maybe a TODO that page_id is going away?

If I did that, half this patch would be TODOs :)

https://codereview.chromium.org/1002803002/diff/360001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1002803002/diff/360001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1123: DCHECK_NE(0,
request_params.nav_entry_id);
On 2015/04/10 12:05:44, clamy wrote:
> Considering that we are in an IPC handler for a browser-initiated navigation,
> shouldn't we always have created an entry for the navigation, and so have
> request_params.nav_entry_id not be 0? I guess the only problem would be some
> RenderViewTests which call Onnavigate directly, but the
RequestNavigationParams
> we pass there could be modified to have a non-null nav_entry_id.

I was auditing the code for uses of page id, and adding the equivalent in the
new scheme. Since the old code DCHECKed on page id, I added the equivalent for
nav_entry_id. Would you prefer I drop it?

https://codereview.chromium.org/1002803002/diff/360001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2671: // Checking the commit type is
planned to be the replacement for the if()
On 2015/04/10 23:54:21, Charlie Reis (slow until 4-15) wrote:
> Would it be better to just have
> DCHECK_EQ(successful_history_nav, commit_type == blink::WebBackForwardCommit)
?
> 
> The current if (A) DCHECK(B) wouldn't catch cases that were successful history
> navs before but weren't WebBackForwardCommits.

Good idea.

[Avi (use Gerrit), 2015-04-13 22:46:41]

On 2015/04/10 23:54:21, Charlie Reis (slow until 4-15) wrote:
> I glossed over some of the test changes [...] for now.

Understandable, though when everything else looks good, I would like a
double-checking of those too. I read each test case, and fixed it up as I
understand it for the new world, but I would appreciate a second pair of eyes.

[Charlie Reis, 2015-04-13 23:33:38]

On 2015/04/13 22:46:41, Avi wrote:
> On 2015/04/10 23:54:21, Charlie Reis (slow until 4-15) wrote:
> > I glossed over some of the test changes [...] for now.
> 
> Understandable, though when everything else looks good, I would like a
> double-checking of those too. I read each test case, and fixed it up as I
> understand it for the new world, but I would appreciate a second pair of eyes.

Sure-- will do when we get there.  Also, I still see the previous patch set (19)
as the latest one.  Did you mean to upload a new one?

[Avi (use Gerrit), 2015-04-14 02:01:28]

On 2015/04/13 23:33:38, Charlie Reis (slow until 4-15) wrote:
> On 2015/04/13 22:46:41, Avi wrote:
> > On 2015/04/10 23:54:21, Charlie Reis (slow until 4-15) wrote:
> > > I glossed over some of the test changes [...] for now.
> > 
> > Understandable, though when everything else looks good, I would like a
> > double-checking of those too. I read each test case, and fixed it up as I
> > understand it for the new world, but I would appreciate a second pair of
eyes.
> 
> Sure-- will do when we get there.  Also, I still see the previous patch set
(19)
> as the latest one.  Did you mean to upload a new one?

Um, I thought I did. Oops, will upload tomorrow.

[clamy, 2015-04-14 15:44:47]

Thanks! Lgtm with one comment.

https://codereview.chromium.org/1002803002/diff/360001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1002803002/diff/360001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:815: details->type =
ClassifyNavigation(rfh, params);
On 2015/04/13 22:42:48, Avi wrote:
> On 2015/04/10 12:05:44, clamy wrote:
> > With PlzNavigate enabled, ClassifyNavigation fails in the
> > NavigationControllerTest.CopyRestoredStateAndNavigate test because of a hack
> > with SiteInstances. Do you think it is possible for us to use
> > ClassifyNavigationWithoutPageID instead? (I can do it in a follow-up patch
to
> > this).
> 
> My current plan is to commit this with the DCHECK (ensuring they behave the
same
> in debug builds), then switch to CHECK, then switch over.
> 
> I want to get some time for it to bake and for us to be confident in it. I
> understand that this CL unblocks you, but I want to be sure it doesn't break
> things before unleashing it onto the world.

Acknowledged.

https://codereview.chromium.org/1002803002/diff/360001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1002803002/diff/360001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1123: DCHECK_NE(0,
request_params.nav_entry_id);
On 2015/04/13 22:42:48, Avi wrote:
> On 2015/04/10 12:05:44, clamy wrote:
> > Considering that we are in an IPC handler for a browser-initiated
navigation,
> > shouldn't we always have created an entry for the navigation, and so have
> > request_params.nav_entry_id not be 0? I guess the only problem would be some
> > RenderViewTests which call Onnavigate directly, but the
> RequestNavigationParams
> > we pass there could be modified to have a non-null nav_entry_id.
> 
> I was auditing the code for uses of page id, and adding the equivalent in the
> new scheme. Since the old code DCHECKed on page id, I added the equivalent for
> nav_entry_id. Would you prefer I drop it?

No I was suggesting having the DCHECK for all navigations, not just history
ones.

[Avi (use Gerrit), 2015-04-15 15:10:17]

https://codereview.chromium.org/1002803002/diff/360001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1002803002/diff/360001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1123: DCHECK_NE(0,
request_params.nav_entry_id);
On 2015/04/14 15:44:47, clamy wrote:
> No I was suggesting having the DCHECK for all navigations, not just history
> ones.

There are lots of parameters that are required for proper functioning, though,
and it's weird to check just this one parameter of request_params.

In fact, I'm not understanding what the usefulness is of checking the page id
here and below. I know the historic origins (they were added by Darin in 2009 in
http://codereview.chromium.org/113758 as a sanity check and strengthened by
Charlie in 2011 in https://codereview.chromium.org/8817006) but either we should
do a complete sanity DCHECK of all parameters or not.

I'd actually prefer to remove them.

[Avi (use Gerrit), 2015-04-15 20:28:25]

https://codereview.chromium.org/1002803002/diff/360001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1002803002/diff/360001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2671: // Checking the commit type is
planned to be the replacement for the if()
On 2015/04/13 22:42:49, Avi wrote:
> On 2015/04/10 23:54:21, Charlie Reis (slow until 4-15) wrote:
> > Would it be better to just have
> > DCHECK_EQ(successful_history_nav, commit_type ==
blink::WebBackForwardCommit)
> ?
> > 
> > The current if (A) DCHECK(B) wouldn't catch cases that were successful
history
> > navs before but weren't WebBackForwardCommits.
> 
> Good idea.

And this breaks a lot. I need to dig pretty hard on this now.

[Avi (use Gerrit), 2015-04-21 23:04:06]

OK, please review.

This is completely failing
ClientOnPageFinishedTest#testNotCalledOnDomModificationWithJavascriptUrlAfterNonCommittedLoadFromApi
and I am asking mnaganov for help there.

[Charlie Reis, 2015-04-22 05:27:25]

LGTM with some nits/cleanup below.

https://codereview.chromium.org/1002803002/diff/500001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1002803002/diff/500001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:1122: // This is
history.reload() and client-side redirection.
nit: and client-side redirection -> or a client redirect.

https://codereview.chromium.org/1002803002/diff/500001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:1128:
pending_entry_->GetUniqueID() == params.nav_entry_id) {
Just to make sure I understand... If it looks like the new pending entry
committed but did_create_new_entry is false, then it must be the same page case?

(I notice that we don't have the URL sanity check that you added to the old
ClassifyNavigation.  I suppose it's not necessary now that we have the nav entry
ID check?)

https://codereview.chromium.org/1002803002/diff/500001/content/browser/frame_...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/1002803002/diff/500001/content/browser/frame_...
content/browser/frame_host/navigator_impl_unittest.cc:58: int
RequestNavigationWithParameters(
Let's put comments on these saying what they're returning, since it's not
obvious from the contract.

https://codereview.chromium.org/1002803002/diff/500001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1002803002/diff/500001/content/common/frame_m...
content/common/frame_messages.h:115: // The unique id of the navigation that
failed.
Can this be 0 if a renderer-initiated navigation fails?

https://codereview.chromium.org/1002803002/diff/500001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/1002803002/diff/500001/content/common/navigat...
content/common/navigation_params.h:213: // FrameHostMsg_DidCommitProvisionalLoad
message.
It's 0 for renderer-initiated navigations, right?  We should clarify that here.

https://codereview.chromium.org/1002803002/diff/500001/content/public/common/...
File content/public/common/frame_navigate_params.h (right):

https://codereview.chromium.org/1002803002/diff/500001/content/public/common/...
content/public/common/frame_navigate_params.h:33: // means.
Same about mentioning the 0 case.

https://codereview.chromium.org/1002803002/diff/500001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1002803002/diff/500001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4690: const RequestNavigationParams&
request_params,
Any reason we need to pass in the whole request_params object and not just the
nav_entry_id?  Might be better to limit what's needed to call it.

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

https://codereview.chromium.org/1002803002/diff/500001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:955:
"Number(document.getElementById('pagename').innerHTML == 'Page C')");
This pattern of returning a bool as an int makes me sad.  No need to change it
for this CL, which is doing enough already.  Afterward, we may want to move
TranslateHelper::ExecuteScriptAndGetBoolResult to a more general
ExecuteJavaScriptAndReturnBool utility that others can use.

https://codereview.chromium.org/1002803002/diff/500001/content/test/test_rend...
File content/test/test_render_frame_host.h (right):

https://codereview.chromium.org/1002803002/diff/500001/content/test/test_rend...
content/test/test_render_frame_host.h:55: void SendNavigate(int page_id,
I feel like we should give some guidance on what to pass for the new parameters
(e.g., pending entry's ID for browser-initiated navigations, 0 for
renderer-initiated navigations, etc).

Do you think there's a sensible place to do that here, or are the parameter
header files sufficient?

https://codereview.chromium.org/1002803002/diff/500001/content/test/test_rend...
File content/test/test_render_view_host.h (right):

https://codereview.chromium.org/1002803002/diff/500001/content/test/test_rend...
content/test/test_render_view_host.h:43: // Utility function to initialize
ViewHostMsg_NavigateParams_Params
nit: FrameHostMsg_DidCommitProvisionalLoad_Params

https://codereview.chromium.org/1002803002/diff/500001/content/test/test_web_...
File content/test/test_web_contents.cc (right):

https://codereview.chromium.org/1002803002/diff/500001/content/test/test_web_...
content/test/test_web_contents.cc:194: GetController().GetPendingEntryIndex() ==
-1,
Interesting.  This probably doesn't equal did_create_entry in all cases in
practice (e.g., if the pending entry gets canceled or replaced), but I think it
looks right for unit tests (especially given that we verify there's a pending
entry at the top of this function).  Works for me.

[mnaganov (inactive), 2015-04-22 09:29:01]

mnaganov@chromium.org changed reviewers:
+ mnaganov@chromium.org

[mnaganov (inactive), 2015-04-22 09:29:03]

This is what
testNotCalledOnDomModificationWithJavascriptUrlAfterNonCommittedLoadFromApi test
does:

1. Starts loading a page, the request is artificially delayed by locking the web
server, so the entry stays in pending state.

2. Loads a javascript: url which doesn't produce a result (instead, it modifies
DOM of the page, but this doesn't matter in this case).

3. Stops loading via WebContents::Stop.

4. Waits for WebContentsObserver::DidFailLoad to happen, which is never
happening due to the change in NavigatorImpl::DidFailProvisionalLoadWithError

https://codereview.chromium.org/1002803002/diff/500001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1002803002/diff/500001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:180: if
(controller_->GetPendingEntry() && params.nav_entry_id != 0 &&
This change is causing
ClientOnPageFinishedTest#testNotCalledOnDomModificationWithJavascriptUrlAfterNonCommittedLoadFromApi
to fail.

[Avi (use Gerrit), 2015-04-22 18:31:51]

Charlie: this patch set addresses all your issues.
Mikhail: I am looking into your breakage, but I wanted to get all the easy nits
out of the way first.

https://codereview.chromium.org/1002803002/diff/500001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1002803002/diff/500001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:1122: // This is
history.reload() and client-side redirection.
On 2015/04/22 05:27:25, Charlie Reis wrote:
> nit: and client-side redirection -> or a client redirect.

Done.

https://codereview.chromium.org/1002803002/diff/500001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:1128:
pending_entry_->GetUniqueID() == params.nav_entry_id) {
On 2015/04/22 05:27:24, Charlie Reis wrote:
> Just to make sure I understand... If it looks like the new pending entry
> committed but did_create_new_entry is false, then it must be the same page
case?

Exactly.

> (I notice that we don't have the URL sanity check that you added to the old
> ClassifyNavigation.  I suppose it's not necessary now that we have the nav
entry
> ID check?)

Correct. The key here is that with the old classifier, when there was a pending
entry we were assuming that the load was of that. Here, we pass the nav entry id
around, so we know where we were going, and that the pending entry isn't
related.

https://codereview.chromium.org/1002803002/diff/500001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1002803002/diff/500001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:180: if
(controller_->GetPendingEntry() && params.nav_entry_id != 0 &&
On 2015/04/22 09:29:03, mnaganov wrote:
> This change is causing
>
ClientOnPageFinishedTest#testNotCalledOnDomModificationWithJavascriptUrlAfterNonCommittedLoadFromApi
> to fail.

Ah. I will look into this, and will call out when I fix it.

https://codereview.chromium.org/1002803002/diff/500001/content/browser/frame_...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/1002803002/diff/500001/content/browser/frame_...
content/browser/frame_host/navigator_impl_unittest.cc:58: int
RequestNavigationWithParameters(
On 2015/04/22 05:27:25, Charlie Reis wrote:
> Let's put comments on these saying what they're returning, since it's not
> obvious from the contract.

Done.

https://codereview.chromium.org/1002803002/diff/500001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1002803002/diff/500001/content/common/frame_m...
content/common/frame_messages.h:115: // The unique id of the navigation that
failed.
On 2015/04/22 05:27:25, Charlie Reis wrote:
> Can this be 0 if a renderer-initiated navigation fails?

Yes.

https://codereview.chromium.org/1002803002/diff/500001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/1002803002/diff/500001/content/common/navigat...
content/common/navigation_params.h:213: // FrameHostMsg_DidCommitProvisionalLoad
message.
On 2015/04/22 05:27:25, Charlie Reis wrote:
> It's 0 for renderer-initiated navigations, right?  We should clarify that
here.

Done.

https://codereview.chromium.org/1002803002/diff/500001/content/public/common/...
File content/public/common/frame_navigate_params.h (right):

https://codereview.chromium.org/1002803002/diff/500001/content/public/common/...
content/public/common/frame_navigate_params.h:33: // means.
On 2015/04/22 05:27:25, Charlie Reis wrote:
> Same about mentioning the 0 case.

Done.

https://codereview.chromium.org/1002803002/diff/500001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1002803002/diff/500001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4690: const RequestNavigationParams&
request_params,
On 2015/04/22 05:27:25, Charlie Reis wrote:
> Any reason we need to pass in the whole request_params object and not just the
> nav_entry_id?  Might be better to limit what's needed to call it.

Done.

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

https://codereview.chromium.org/1002803002/diff/500001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:955:
"Number(document.getElementById('pagename').innerHTML == 'Page C')");
On 2015/04/22 05:27:25, Charlie Reis wrote:
> This pattern of returning a bool as an int makes me sad.  No need to change it
> for this CL, which is doing enough already.  Afterward, we may want to move
> TranslateHelper::ExecuteScriptAndGetBoolResult to a more general
> ExecuteJavaScriptAndReturnBool utility that others can use.

Acknowledged.

https://codereview.chromium.org/1002803002/diff/500001/content/test/test_rend...
File content/test/test_render_frame_host.h (right):

https://codereview.chromium.org/1002803002/diff/500001/content/test/test_rend...
content/test/test_render_frame_host.h:55: void SendNavigate(int page_id,
On 2015/04/22 05:27:25, Charlie Reis wrote:
> I feel like we should give some guidance on what to pass for the new
parameters
> (e.g., pending entry's ID for browser-initiated navigations, 0 for
> renderer-initiated navigations, etc).
> 
> Do you think there's a sensible place to do that here, or are the parameter
> header files sufficient?

My opinion, based on reading tests that use these functions, is that people only
have a vague idea of how this is supposed to work, and pretty much make up page
ids, etc. I'll write up header documentation, but I have zero faith that people
won't just end up copy/pasting from other tests and fiddling with it until it
works.

https://codereview.chromium.org/1002803002/diff/500001/content/test/test_rend...
File content/test/test_render_view_host.h (right):

https://codereview.chromium.org/1002803002/diff/500001/content/test/test_rend...
content/test/test_render_view_host.h:43: // Utility function to initialize
ViewHostMsg_NavigateParams_Params
On 2015/04/22 05:27:25, Charlie Reis wrote:
> nit: FrameHostMsg_DidCommitProvisionalLoad_Params

Done.

https://codereview.chromium.org/1002803002/diff/500001/content/test/test_web_...
File content/test/test_web_contents.cc (right):

https://codereview.chromium.org/1002803002/diff/500001/content/test/test_web_...
content/test/test_web_contents.cc:194: GetController().GetPendingEntryIndex() ==
-1,
On 2015/04/22 05:27:25, Charlie Reis wrote:
> Interesting.  This probably doesn't equal did_create_entry in all cases in
> practice (e.g., if the pending entry gets canceled or replaced), but I think
it
> looks right for unit tests (especially given that we verify there's a pending
> entry at the top of this function).  Works for me.

This whole thing is fakery for unit tests, and while this is definitely a "good
enough" deal, I'm OK with it too.

[Charlie Reis, 2015-04-22 21:02:52]

Thanks.  The updates look good (apart from the compile error), so it's just the
DidFailProvisionalLoadWithError issue.  I admit to not fully understanding why
that particular change was needed.

[Avi (use Gerrit), 2015-04-22 21:12:24]

On 2015/04/22 21:02:52, Charlie Reis wrote:
> Thanks.  The updates look good (apart from the compile error), so it's just
the
> DidFailProvisionalLoadWithError issue.  I admit to not fully understanding why
> that particular change was needed.

The new classifier needs the pending entry to classify. If you remove that block
of code, there's at least one test case where the pending entry gets prematurely
cleared and we incorrectly classify with the new classifier.

The old and new classifiers have different edge cases. The old classifier
happened to not need the pending entry in that case, but the new one does.

[Avi (use Gerrit), 2015-04-23 21:04:39]

I'm a bit stuck here.

On 2015/04/22 09:29:03, mnaganov wrote:
> 4. Waits for WebContentsObserver::DidFailLoad to happen, which is never
> happening due to the change in NavigatorImpl::DidFailProvisionalLoadWithError

DidFailLoad and DidFailProvisionalLoadWithError are two different code paths, so
it's a surprise to me that a change in one could affect the other.

My questions to you:

1. Are you certain it is DidFailLoad, and not DidFailProvisionalLoadWithError?
2. I'm trying to trace back what your test is doing. It uses an
OnPageFinishedHelper but I'm a bit stumped as to where in C++ land that connects
up, and what callbacks on the WebContentsObserver qualify. I've written the
equivalent test in C++, and DidFailProvisionalLoad gets called consistently no
matter what I do.

[Avi (use Gerrit), 2015-04-23 21:12:52]

To clarify, on WebContentsObserver, DidFailLoad and DidFailProvisionalLoad are
different calls. If the server never replies (as in your step 1) then the
request hangs out as a provisional load, and its failure is classified as a
provisional load failure.

[Avi (use Gerrit), 2015-04-23 21:25:03]

OK, I just figured it out.

1. Yes, you meant DidFailProvisionalLoad.
2. You're executing the javascript with "load url" which creates its own pending
entry.... ugh.

Working on it.

[mnaganov (inactive), 2015-04-24 08:48:51]

On 2015/04/23 21:25:03, Avi wrote:
> OK, I just figured it out.
> 
> 1. Yes, you meant DidFailProvisionalLoad.

Yes, apologies. I missed the fact that WebContentsObserverProxy merges
didFailLoad and didFailProvisionalLoad, and just puts a boolean flag to
distinguish between them.

For your convenience, this is what is happening on the browser side (without
your patch) when we call WebView.stopLoading:

org.chromium.android_webview.AwContentsClient.onPageFinished(AwContentsClient.java:269)
org.chromium.android_webview.AwWebContentsObserver.didFailLoad(AwWebContentsObserver.java:48)
org.chromium.content.browser.webcontents.WebContentsObserverProxy.didFailLoad(WebContentsObserverProxy.java:99)

---^ to Java

content::WebContentsObserverProxy::DidFailLoadInternal(bool, bool, int,
std::basic_string<unsigned short, base::string16_char_traits,
std::allocator<unsigned short> > const&, GURL const&)
    content/browser/android/web_contents_observer_proxy.cc:259
content::WebContentsObserverProxy::DidFailProvisionalLoad(content::RenderFrameHost*,
GURL const&, int, std::basic_string<unsigned short, base::string16_char_traits,
std::allocator<unsigned short> > const&)
    content/browser/android/web_contents_observer_proxy.cc:101
content::WebContentsImpl::DidFailProvisionalLoadWithError(content::RenderFrameHostImpl*,
FrameHostMsg_DidFailProvisionalLoadWithError_Params const&)
    content/browser/web_contents/web_contents_impl.cc:2595 (discriminator 5)
content::NavigatorImpl::DidFailProvisionalLoadWithError(content::RenderFrameHostImpl*,
FrameHostMsg_DidFailProvisionalLoadWithError_Params const&)
    content/browser/frame_host/navigator_impl.cc:241

---^ from IPC

> 2. You're executing the javascript with "load url" which creates its own
pending
> entry.... ugh.
> 

Yes, this is what apps that use WebView can do.

There is another thing that I don't like -- as the javascript: load replaces the
original pending entry, when Navigator receives a message from the renderer
about the original load, it "recreates" the pending entry in
NavigatorImpl::DidStartProvisionalLoad, and as a result, it loses the orignal
TransitionType, effectively erasing PAGE_TRANSITION_FROM_API flag that was on it
originally. Not sure, if anything can be done about that.

> Working on it.

Thanks! Sorry for the trouble.

[Avi (use Gerrit), 2015-04-24 15:07:13]

On 2015/04/24 08:48:51, mnaganov wrote:
> There is another thing that I don't like -- as the javascript: load replaces
the
> original pending entry, when Navigator receives a message from the renderer
> about the original load, it "recreates" the pending entry in
> NavigatorImpl::DidStartProvisionalLoad, and as a result, it loses the orignal
> TransitionType, effectively erasing PAGE_TRANSITION_FROM_API flag that was on
it
> originally.

Yes, we're currently extremely cavalier about killing the pending entry and
recreating it. If this were cleaner we wouldn't be in this situation, and it
would be a good idea to fix it up, but I'm not clear if the right approach is to
land this first with a bit of a hack to get around that and come back to that
later or not. That's all part of the discussion.

> Thanks! Sorry for the trouble.,

This isn't your fault; no apologies. Your test exposed a real weakness in my
code, so now I get to fix it :)

[mnaganov (inactive), 2015-04-24 15:11:45]

On 2015/04/24 15:07:13, Avi wrote:
> On 2015/04/24 08:48:51, mnaganov wrote:
> > There is another thing that I don't like -- as the javascript: load replaces
> the
> > original pending entry, when Navigator receives a message from the renderer
> > about the original load, it "recreates" the pending entry in
> > NavigatorImpl::DidStartProvisionalLoad, and as a result, it loses the
orignal
> > TransitionType, effectively erasing PAGE_TRANSITION_FROM_API flag that was
on
> it
> > originally.
> 
> Yes, we're currently extremely cavalier about killing the pending entry and
> recreating it. If this were cleaner we wouldn't be in this situation, and it
> would be a good idea to fix it up, but I'm not clear if the right approach is
to
> land this first with a bit of a hack to get around that and come back to that
> later or not. That's all part of the discussion.
>

This doesn't affect the test, so it isn't needed to be fixed in this patch. I'm
just making a note.
 
> > Thanks! Sorry for the trouble.,
> 
> This isn't your fault; no apologies. Your test exposed a real weakness in my
> code, so now I get to fix it :)

[Avi (use Gerrit), 2015-04-24 21:29:01]

avi@chromium.org changed reviewers:
+ hajimehoshi@chromium.org, isherman@chromium.org, jeremyim@chromium.org,
mattm@chromium.org, mvanouwerkerk@chromium.org, nasko@chromium.org,
phajdan.jr@chromium.org, stevenjb@chromium.org

[Avi (use Gerrit), 2015-04-24 21:29:03]

OK! Let's get this reviewed.

Charlie: You LGed it earlier, but this makes everything green via the boolean we
discussed. You're OK with this?
Camille: Take a look; it should still be satisfactory.
Nasko: frame_messages.h IPC
Jeremy: components/data_reduction_proxy
Paweł: chrome/test
Steven: chrome/browser/chromeos
Michael: chrome/browser/geolocation
Matt: chrome/browser/safe_browsing
Hajime: chrome/browser/translate
Ilya: chrome/browser/ui/autofill

Thank you!

[Ilya Sherman, 2015-04-24 21:34:08]

autofill lgtm % nit

https://codereview.chromium.org/1002803002/diff/560001/chrome/browser/ui/auto...
File
chrome/browser/ui/autofill/generated_credit_card_bubble_controller_unittest.cc
(right):

https://codereview.chromium.org/1002803002/diff/560001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/generated_credit_card_bubble_controller_unittest.cc:85:
// The transition is in two places; quite a bummer.
nit: I feel your pain, but I'm not sure this comment will be helpful for future
readers of the code.

[Avi (use Gerrit), 2015-04-24 21:37:16]

https://codereview.chromium.org/1002803002/diff/560001/chrome/browser/ui/auto...
File
chrome/browser/ui/autofill/generated_credit_card_bubble_controller_unittest.cc
(right):

https://codereview.chromium.org/1002803002/diff/560001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/generated_credit_card_bubble_controller_unittest.cc:85:
// The transition is in two places; quite a bummer.
On 2015/04/24 21:34:08, Ilya Sherman wrote:
> nit: I feel your pain, but I'm not sure this comment will be helpful for
future
> readers of the code.

Done.

[Charlie Reis, 2015-04-24 22:02:02]

Diff from 28 to 29 LGTM with nits.

https://codereview.chromium.org/1002803002/diff/560001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1002803002/diff/560001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:1141: // got cleared in
the meanwhile. Classify as EXISTING_PAGE.
Let's clarify that this is essentially a SAME_PAGE navigation, but we treat it
as EXISTING_PAGE because we don't want to clear the pending entry.  (Maybe same
for the case below as well.)

https://codereview.chromium.org/1002803002/diff/560001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1002803002/diff/560001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1093:
LOG(ERROR) << "FailureWatcher::DidFailLoad";
nit: Let's remove the LOG statements.

https://codereview.chromium.org/1002803002/diff/560001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1157:
shell()->web_contents()->Stop();
Maybe we can add an EXPECT that the pending entry is gone before the Stop call?

(It's fine to change that if we later add support for multiple pending entries,
but it's useful documentation for our current behavior in the meantime.  For
example, it makes it clear why this case is tricky.)

https://codereview.chromium.org/1002803002/diff/560001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1002803002/diff/560001/content/common/frame_m...
content/common/frame_messages.h:152: // This is the value from the browser
indicated whether it intended to make a
nit: "Whether the browser intends for this navigation to make a new entry."

This might belong above did_create_new_entry.

https://codereview.chromium.org/1002803002/diff/560001/content/common/frame_m...
content/common/frame_messages.h:155: // nav_entry_id, above.
nit: Drop "above."

https://codereview.chromium.org/1002803002/diff/560001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1002803002/diff/560001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3894: params.intended_as_new_entry =
nit: Move above did_create_new_entry if you do that in the params object as
well.

[Avi (use Gerrit), 2015-04-24 22:17:15]

https://codereview.chromium.org/1002803002/diff/560001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1002803002/diff/560001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:1141: // got cleared in
the meanwhile. Classify as EXISTING_PAGE.
On 2015/04/24 22:02:02, Charlie Reis wrote:
> Let's clarify that this is essentially a SAME_PAGE navigation, but we treat it
> as EXISTING_PAGE because we don't want to clear the pending entry.  (Maybe
same
> for the case below as well.)

Done.

https://codereview.chromium.org/1002803002/diff/560001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1002803002/diff/560001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1093:
LOG(ERROR) << "FailureWatcher::DidFailLoad";
On 2015/04/24 22:02:02, Charlie Reis wrote:
> nit: Let's remove the LOG statements.

Gah, part of my debugging via lots of log statement strategy. Done.

https://codereview.chromium.org/1002803002/diff/560001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1157:
shell()->web_contents()->Stop();
On 2015/04/24 22:02:02, Charlie Reis wrote:
> Maybe we can add an EXPECT that the pending entry is gone before the Stop
call?
> 
> (It's fine to change that if we later add support for multiple pending
entries,
> but it's useful documentation for our current behavior in the meantime.  For
> example, it makes it clear why this case is tricky.)

Done.

https://codereview.chromium.org/1002803002/diff/560001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1002803002/diff/560001/content/common/frame_m...
content/common/frame_messages.h:152: // This is the value from the browser
indicated whether it intended to make a
On 2015/04/24 22:02:02, Charlie Reis wrote:
> nit: "Whether the browser intends for this navigation to make a new entry."
> 
> This might belong above did_create_new_entry.

Done.

https://codereview.chromium.org/1002803002/diff/560001/content/common/frame_m...
content/common/frame_messages.h:155: // nav_entry_id, above.
On 2015/04/24 22:02:02, Charlie Reis wrote:
> nit: Drop "above."

Done.

https://codereview.chromium.org/1002803002/diff/560001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1002803002/diff/560001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3894: params.intended_as_new_entry =
On 2015/04/24 22:02:02, Charlie Reis wrote:
> nit: Move above did_create_new_entry if you do that in the params object as
> well.

Done.

[mattm, 2015-04-24 23:42:37]

https://codereview.chromium.org/1002803002/diff/560001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc
(right):

https://codereview.chromium.org/1002803002/diff/560001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc:315:
Navigate(kGoodURL, 2, 0, true);
is nav_entry_id=0 some magic value? Why is it only non-zero sometimes? Would be
nice if TestDidNavigate was documented.

https://codereview.chromium.org/1002803002/diff/560001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc:564:
NavigateCrossSite(kBadURL, 2, pending_id, false, true);
Since NavigateCrossSite is only directly called with is_cross_site=true, maybe
it should just mean that, and there can be some internal function that both
Navigate and NavigateCrossSite call. Also it looks like did_create_new_entry is
always true when calling Navigate and always false when calling
NavigateCrossSite, is that always the case? Can it also only be exposed on the
internal function to simplify things?

[Avi (use Gerrit), 2015-04-25 19:24:51]

mattm: please review again.

https://codereview.chromium.org/1002803002/diff/560001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc
(right):

https://codereview.chromium.org/1002803002/diff/560001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc:315:
Navigate(kGoodURL, 2, 0, true);
On 2015/04/24 23:42:37, mattm wrote:
> is nav_entry_id=0 some magic value? Why is it only non-zero sometimes? Would
be
> nice if TestDidNavigate was documented.

I documented those values on RenderFrameHostTester but failed to document them
on WebContentsTester. Done.

https://codereview.chromium.org/1002803002/diff/560001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc:564:
NavigateCrossSite(kBadURL, 2, pending_id, false, true);
On 2015/04/24 23:42:37, mattm wrote:
> Since NavigateCrossSite is only directly called with is_cross_site=true, maybe
> it should just mean that, and there can be some internal function that both
> Navigate and NavigateCrossSite call. Also it looks like did_create_new_entry
is
> always true when calling Navigate and always false when calling
> NavigateCrossSite, is that always the case? Can it also only be exposed on the
> internal function to simplify things?

did_create_new_entry is false because it is a history navigation. See the new
documentation that I added on WebContentsTester. I can't hide it because only
the caller of the Navigate functions would know the correct value.

Re the code cleanup, done. (I stole this bit of the code from the
DataReductionProxyDebugBlockingPageTest, so I made these changes there too.)

[hajimehoshi, 2015-04-27 04:45:56]

translate lgtm

[Paweł Hajdan Jr., 2015-04-27 08:53:55]

chrome/test, content/test: LGTM

[Michael van Ouwerkerk, 2015-04-27 11:20:25]

geolocation lgtm with nit

https://codereview.chromium.org/1002803002/diff/640001/chrome/browser/geoloca...
File chrome/browser/geolocation/geolocation_permission_context_unittest.cc
(right):

https://codereview.chromium.org/1002803002/diff/640001/chrome/browser/geoloca...
chrome/browser/geolocation/geolocation_permission_context_unittest.cc:230:
->SendNavigate(extra_tabs_.size() + 1, entry->GetUniqueID(), true, url);
What does |true| stand for? Maybe add a comment like |true /* shouldDoFoo */|

[clamy, 2015-04-27 11:50:05]

Thanks! One comment below.

https://codereview.chromium.org/1002803002/diff/640001/content/common/navigat...
File content/common/navigation_params.cc (right):

https://codereview.chromium.org/1002803002/diff/640001/content/common/navigat...
content/common/navigation_params.cc:87: nav_entry_id(0),
Shouldn't we initialize intended_as_new_entry as well?

[nasko, 2015-04-27 14:03:56]

https://codereview.chromium.org/1002803002/diff/640001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1002803002/diff/640001/content/common/frame_m...
content/common/frame_messages.h:117: IPC_STRUCT_MEMBER(int, nav_entry_id)
Tracing through the path of this IPC message, I can't see where this value is
used.

[Avi (use Gerrit), 2015-04-27 14:56:33]

Camille, Nasko: please take another look.

https://codereview.chromium.org/1002803002/diff/640001/chrome/browser/geoloca...
File chrome/browser/geolocation/geolocation_permission_context_unittest.cc
(right):

https://codereview.chromium.org/1002803002/diff/640001/chrome/browser/geoloca...
chrome/browser/geolocation/geolocation_permission_context_unittest.cc:230:
->SendNavigate(extra_tabs_.size() + 1, entry->GetUniqueID(), true, url);
On 2015/04/27 11:20:25, Michael van Ouwerkerk wrote:
> What does |true| stand for? Maybe add a comment like |true /* shouldDoFoo */|

true is for did_create_new_entry.

I can't comment that. This call is made hundreds of times (literally) in the
unit tests.

https://codereview.chromium.org/1002803002/diff/640001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1002803002/diff/640001/content/common/frame_m...
content/common/frame_messages.h:117: IPC_STRUCT_MEMBER(int, nav_entry_id)
On 2015/04/27 14:03:56, nasko (very slow until 04-27) wrote:
> Tracing through the path of this IPC message, I can't see where this value is
> used. 

I removed the part of the code that used this, but forgot to remove the IPC.
Done.

https://codereview.chromium.org/1002803002/diff/640001/content/common/navigat...
File content/common/navigation_params.cc (right):

https://codereview.chromium.org/1002803002/diff/640001/content/common/navigat...
content/common/navigation_params.cc:87: nav_entry_id(0),
On 2015/04/27 11:50:04, clamy wrote:
> Shouldn't we initialize intended_as_new_entry as well?

Gah. Done.

[clamy, 2015-04-27 15:02:55]

Thanks! Lgtm.

[nasko, 2015-04-27 18:42:11]

LGTM

[stevenjb, 2015-04-27 18:55:20]

c/b/chromeos lgtm

[mattm, 2015-04-27 19:12:57]

safe_browsing lgtm

[bengr, 2015-04-27 20:00:26]

bengr@chromium.org changed reviewers:
+ bengr@chromium.org

[bengr, 2015-04-27 20:00:27]

data_reduction_proxy lgtm

[Avi (use Gerrit), 2015-04-27 20:44:15]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2015-04-27 20:44:16]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
isherman@chromium.org, hajimehoshi@chromium.org, phajdan.jr@chromium.org,
mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/1002803002/#ps660001
(title: "clamy & nasko")

[commit-bot: I haz the power, 2015-04-27 20:44:49]

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

[commit-bot: I haz the power, 2015-04-27 21:17:09]

Committed patchset #34 (id:660001)

[commit-bot: I haz the power, 2015-04-27 21:17:49]

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

[mohsen, 2015-04-27 22:04:43]

A revert of this CL (patchset #34 id:660001) has been created in
https://codereview.chromium.org/1104403002/ by mohsen@chromium.org.

The reason for reverting is: Caused compile error in "Google Chrome Mac" bot:

https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Mac/b....

[cpu_(ooo_6.6-7.5), 2015-04-28 00:44:23]

cpu@chromium.org changed reviewers:
+ cpu@chromium.org

[cpu_(ooo_6.6-7.5), 2015-04-28 00:44:25]

lgtm for rlz

[Avi (use Gerrit), 2015-04-28 00:45:39]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2015-04-28 00:45:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
hajimehoshi@chromium.org, stevenjb@chromium.org, phajdan.jr@chromium.org,
bengr@chromium.org, nasko@chromium.org, mattm@chromium.org, clamy@chromium.org,
isherman@chromium.org, creis@chromium.org, mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/1002803002/#ps680001
(title: "rlz")

[commit-bot: I haz the power, 2015-04-28 00:47:20]

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

[commit-bot: I haz the power, 2015-04-28 01:30:10]

Committed patchset #35 (id:680001)

[commit-bot: I haz the power, 2015-04-28 01:31:06]

Patchset 35 (id:??) landed as
https://crrev.com/5671403d44971669e4d81aecf3f002188ce0e95f
Cr-Commit-Position: refs/heads/master@{#327214}

[vkuzkokov, 2015-04-28 12:06:05]

A revert of this CL (patchset #35 id:680001) has been created in
https://codereview.chromium.org/1110943003/ by vkuzkokov@chromium.org.

The reason for reverting is: Very flaky on ChromeOS debug bots. See
crbug.com/481910.

[Nikita (slow), 2015-04-28 13:11:21]

nkostylev@chromium.org changed reviewers:
+ nkostylev@chromium.org

[Nikita (slow), 2015-04-28 13:11:23]

Another reason for revert, this change breaks Instrumentation test
AndroidWebViewTest

http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%...

[Avi (use Gerrit), 2015-05-04 14:58:45]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2015-05-04 14:58:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
hajimehoshi@chromium.org, stevenjb@chromium.org, cpu@chromium.org,
phajdan.jr@chromium.org, bengr@chromium.org, nasko@chromium.org,
mattm@chromium.org, clamy@chromium.org, isherman@chromium.org,
creis@chromium.org, mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/1002803002/#ps720001
(title: "fix comments")

[commit-bot: I haz the power, 2015-05-04 14:59:16]

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

[commit-bot: I haz the power, 2015-05-04 15:55:40]

Committed patchset #37 (id:720001)

[commit-bot: I haz the power, 2015-05-04 15:56:39]

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

[pdr., 2015-05-05 05:17:29]

A revert of this CL (patchset #37 id:720001) has been created in
https://codereview.chromium.org/1121083004/ by pdr@chromium.org.

The reason for reverting is: This patch broke
missing-repaint-after-slow-style-sheet.pl, see crbug.com/484531 for more
information..

[Avi (use Gerrit), 2015-05-07 14:43:15]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2015-05-07 14:43:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
hajimehoshi@chromium.org, stevenjb@chromium.org, cpu@chromium.org,
phajdan.jr@chromium.org, bengr@chromium.org, nasko@chromium.org,
mattm@chromium.org, clamy@chromium.org, isherman@chromium.org,
creis@chromium.org, mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/1002803002/#ps740001
(title: "rebase atop https://codereview.chromium.org/1128893003")

[commit-bot: I haz the power, 2015-05-07 14:43:31]

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

[Avi (use Gerrit), 2015-05-07 15:35:51]

The CQ bit was unchecked by avi@chromium.org

[Avi (use Gerrit), 2015-05-07 20:54:11]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2015-05-07 20:54:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
hajimehoshi@chromium.org, stevenjb@chromium.org, cpu@chromium.org,
phajdan.jr@chromium.org, bengr@chromium.org, nasko@chromium.org,
mattm@chromium.org, clamy@chromium.org, isherman@chromium.org,
creis@chromium.org, mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/1002803002/#ps780001
(title: "with comment")

[commit-bot: I haz the power, 2015-05-07 20:58:13]

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

[commit-bot: I haz the power, 2015-05-07 21:12:57]

Committed patchset #40 (id:780001)

[commit-bot: I haz the power, 2015-05-07 21:14:09]

Patchset 40 (id:??) landed as
https://crrev.com/61cae85448cfeb793270e804b5ad1023993279c5
Cr-Commit-Position: refs/heads/master@{#328829}

[Kunihiko Sakamoto, 2015-05-08 04:14:15]

A revert of this CL (patchset #40 id:780001) has been created in
https://codereview.chromium.org/1136553005/ by ksakamoto@chromium.org.

The reason for reverting is: This still breaks Instrumentation test
AndroidWebViewTest.
failures:
org.chromium.android_webview.test.LoadDataWithBaseUrlTest#testOnPageFinishedUrlIsBaseUrl

http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%...
.

[Avi (use Gerrit), 2015-05-08 15:20:02]

On 2015/05/08 04:14:15, Kunihiko Sakamoto wrote:
> A revert of this CL (patchset #40 id:780001) has been created in
> https://codereview.chromium.org/1136553005/ by mailto:ksakamoto@chromium.org.
> 
> The reason for reverting is: This still breaks Instrumentation test
> AndroidWebViewTest.
> failures:
>
org.chromium.android_webview.test.LoadDataWithBaseUrlTest#testOnPageFinishedUrlIsBaseUrl
> 
>
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%...
> .

In the logs is
f9f23:  05-07 22:43:38.589 16355 16355 F chromium:
[FATAL:navigation_controller_impl.cc(829)] Check failed: details->type ==
ClassifyNavigationWithoutPageID(rfh, params) (2 vs. 3)
f9f23:  05-07 22:44:38.789 16464 16464 F chromium:
[FATAL:navigation_controller_impl.cc(829)] Check failed: details->type ==
ClassifyNavigationWithoutPageID(rfh, params) (2 vs. 3)
f9f23:  05-07 22:44:55.779 16610 16610 F chromium:
[FATAL:navigation_controller_impl.cc(829)] Check failed: details->type ==
ClassifyNavigationWithoutPageID(rfh, params) (2 vs. 3)
719c0:  05-07 22:41:12.449  7399  7399 F chromium:
[FATAL:navigation_controller_impl.cc(829)] Check failed: details->type ==
ClassifyNavigationWithoutPageID(rfh, params) (2 vs. 3)
719c0:  05-07 22:43:37.789 15156 15156 F chromium:
[FATAL:navigation_controller_impl.cc(829)] Check failed: details->type ==
ClassifyNavigationWithoutPageID(rfh, params) (3 vs. 2)
0210f:  05-07 22:40:15.449  4805  4805 F chromium:
[FATAL:navigation_controller_impl.cc(829)] Check failed: details->type ==
ClassifyNavigationWithoutPageID(rfh, params) (3 vs. 2)

Constant disagreement with SAME_PAGE. I'm going to neuter the comparison there
in those cases.

[Avi (use Gerrit), 2015-05-08 15:54:15]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2015-05-08 15:54:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
hajimehoshi@chromium.org, stevenjb@chromium.org, cpu@chromium.org,
phajdan.jr@chromium.org, bengr@chromium.org, nasko@chromium.org,
mattm@chromium.org, clamy@chromium.org, isherman@chromium.org,
creis@chromium.org, mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/1002803002/#ps800001
(title: "relax the dcheck")

[commit-bot: I haz the power, 2015-05-08 15:55:33]

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

[commit-bot: I haz the power, 2015-05-08 17:52:55]

Committed patchset #41 (id:800001)

[commit-bot: I haz the power, 2015-05-08 17:53:46]

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