Remove the use of ProvisionalChangeToMainFrameUrl from prerender code.

Remove the use of ProvisionalChangeToMainFrameUrl from prerender code.

BUG=78512
TEST=no change

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

[Avi (use Gerrit), 2014-09-14 20:17:54]

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

[Avi (use Gerrit), 2014-09-14 20:17:55]

I fixed the test by altering the expectations. That sounds horrible so let me
explain.

ProvisionalChangeToMainFrameUrl gets called in two places:
WebContentsImpl::DidStartProvisionalLoad and
WebContentsImpl::DidRedirectProvisionalLoad. In the affected test,
PrerenderBrowserTest.MatchCompleteDummyCancelNavigation, it never flows through
DidRedirectProvisionalLoad but is only called by DidStartProvisionalLoad.

I had two choices.

First would be to copy lots of the histogram code from
ProvisionalChangeToMainFrameUrl to DidStartProvisionalLoadForFrame. But the
problem is that you already have a DidStartProvisionalLoadForFrame on
PrerenderTabHelper, so I imagine that it already does everything that it needs
to.

So the second choice was to guess that PrerenderTabHelper only really needs that
function for _real_ redirections. But that means that the metrics will fire
differently, so it would be appropriate to change the expectations.

Thus this particular CL. Let me know what you think.

[Avi (use Gerrit), 2014-09-15 15:09:29]

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

[Avi (use Gerrit), 2014-09-15 15:09:30]

David, thoughts?

[cbentzel, 2014-09-16 00:33:40]

cbentzel@chromium.org changed reviewers:
+ tburkard@chromium.org

[cbentzel, 2014-09-16 00:33:41]

+tburkard

I need to think this through some more. Or probably the other reviewers are
better suited for it than me at this point.

https://codereview.chromium.org/565263005/diff/40001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/565263005/diff/40001/chrome/browser/prerender...
chrome/browser/prerender/prerender_manager.cc:1100: if (details.resource_type !=
content::RESOURCE_TYPE_MAIN_FRAME)
This is coming at a pretty different time for ProvisionalChangeToMainFrameUrl
did, still trying to work through implications.

[tburkard, 2014-09-16 12:58:01]

I don't think this will work, because ProvisionalChangeToMainFrameUrl does not
just happen on a redirect, but also on an initial navigation.

I think I added ProvisionalChangeToMainFrameUrl years ago, and it covered the
two cases of navigating to a page without it having committed yet, and a change
in the main frame url as a result of a redirect.

If you wanted to remove that again, you'd have to cover the exact same cases,
not just the redirect case.

Thanks.

[Avi (use Gerrit), 2014-09-16 16:43:35]

On 2014/09/16 12:58:01, tburkard wrote:
> I don't think this will work, because ProvisionalChangeToMainFrameUrl does not
> just happen on a redirect, but also on an initial navigation.
> 
> I think I added ProvisionalChangeToMainFrameUrl years ago, and it covered the
> two cases of navigating to a page without it having committed yet, and a
change
> in the main frame url as a result of a redirect.
> 
> If you wanted to remove that again, you'd have to cover the exact same cases,
> not just the redirect case.
> 
> Thanks.

Are you talking about the use in PrerenderManager, PrerenderTabHelper, or both?

[tburkard, 2014-09-16 18:16:10]

both

On Tue, Sep 16, 2014 at 6:43 PM, <avi@chromium.org> wrote:

> On 2014/09/16 12:58:01, tburkard wrote:
>
>> I don't think this will work, because ProvisionalChangeToMainFrameUrl
>> does not
>> just happen on a redirect, but also on an initial navigation.
>>
>
>  I think I added ProvisionalChangeToMainFrameUrl years ago, and it
>> covered the
>> two cases of navigating to a page without it having committed yet, and a
>>
> change
>
>> in the main frame url as a result of a redirect.
>>
>
>  If you wanted to remove that again, you'd have to cover the exact same
>> cases,
>> not just the redirect case.
>>
>
>  Thanks.
>>
>
> Are you talking about the use in PrerenderManager, PrerenderTabHelper, or
> both?
>
> https://codereview.chromium.org/565263005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[davidben, 2014-09-16 18:51:45]

I still need to take the time to review the other use (sorry, been swamped with
perf), but I believe the PendingSwap use had always been somewhat redundant
anyway and can possibly just be removed; the navigation should have been
cancelled first with DidFailProvisionalLoad anyway, and the other check was more
defensive.

(Hooking into navigation right now is pretty screwy and somewhat racy. One of a
long list of things that'll get simpler post-PlzNavigate.)

[Avi (use Gerrit), 2014-09-16 19:52:44]

On 2014/09/16 18:51:45, David Benjamin wrote:
> I still need to take the time to review the other use (sorry, been swamped
with
> perf), but I believe the PendingSwap use had always been somewhat redundant
> anyway and can possibly just be removed; the navigation should have been
> cancelled first with DidFailProvisionalLoad anyway, and the other check was
more
> defensive.
> 
> (Hooking into navigation right now is pretty screwy and somewhat racy. One of
a
> long list of things that'll get simpler post-PlzNavigate.)

I can see what you mean; it should definitely be covered by
DidCommitProvisionalLoadForFrame and DidFailProvisionalLoad.

Let me experiment with the tab helper. When you have a chance to look at the tab
helper I'd like to have your thoughts there too.

Thanks!

[Avi (use Gerrit), 2014-09-16 21:03:08]

tburkard: Duplicated the code into
PrerenderTabHelper::DidStartProvisionalLoadForFrame.
davidben: Removed the code from PrerenderManager, as mentioned.
cbentzel: Let me know the results of the implication pondering.

https://codereview.chromium.org/565263005/diff/40001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/565263005/diff/40001/chrome/browser/prerender...
chrome/browser/prerender/prerender_manager.cc:1100: if (details.resource_type !=
content::RESOURCE_TYPE_MAIN_FRAME)
On 2014/09/16 00:33:41, cbentzel wrote:
> This is coming at a pretty different time for ProvisionalChangeToMainFrameUrl
> did, still trying to work through implications.

Acknowledged.

[tburkard, 2014-09-17 14:19:43]

https://codereview.chromium.org/565263005/diff/60001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_manager.cc (left):

https://codereview.chromium.org/565263005/diff/60001/chrome/browser/prerender...
chrome/browser/prerender/prerender_manager.cc:1103: if (url != url_)
How about this logic here? Why was this removed completely?

[tburkard, 2014-09-17 14:21:02]

https://codereview.chromium.org/565263005/diff/60001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_manager.cc (left):

https://codereview.chromium.org/565263005/diff/60001/chrome/browser/prerender...
chrome/browser/prerender/prerender_manager.cc:1103: if (url != url_)
On 2014/09/17 14:19:42, tburkard wrote:
> How about this logic here? Why was this removed completely?

Ah I see that davidben@ mentioned that this may be removed. I'd feel more
comfortable leaving it in there in this change, and consider removing it in a
separate change, and verifying that this is indeed appropriate.

[davidben, 2014-09-17 15:08:31]

This lgtm, but you should probably also wait for Timo too, particularly since I
was less familiar with the histograms code when I worked on prerender.

Aside: I'm curious why removing this hook is needed for your navigation work.
(The page id / history item fixup, right?) The reason I ask is that, at least
post-PlzNavigate, I actually think exposing a navigation redirect hook would
make more sense layering-wise than letting WebContents consumers listen to all
redirects of all requests at the loader level. (And, indeed, it looks like all
DidGetRedirectForResourceRequest implementations are just trying to hook into
navigation.) Doing it at the loader level means the observer processes the
redirect in parallel with the actual consumer rather after/as-a-part-of the
actual consumer's processing, which seems odd to me.

(But PlzNavigate is a long way off, so maybe this version is better right now.
I've just been trying to look at things from the perspective of my mental model
of the post-PlzNavigate-and-other-projects-to-Solve-All-Our-Problems future and
maintain a delta between us and that mental layering.)

https://codereview.chromium.org/565263005/diff/60001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_manager.cc (left):

https://codereview.chromium.org/565263005/diff/60001/chrome/browser/prerender...
chrome/browser/prerender/prerender_manager.cc:1103: if (url != url_)
On 2014/09/17 14:21:02, tburkard wrote:
> On 2014/09/17 14:19:42, tburkard wrote:
> > How about this logic here? Why was this removed completely?
> 
> Ah I see that davidben@ mentioned that this may be removed. I'd feel more
> comfortable leaving it in there in this change, and consider removing it in a
> separate change, and verifying that this is indeed appropriate.

That logic is actually a pretty good indication that this check was either
broken or redundant anyway. :-) If we were actually relying on it, we'd fail to
abort the async swap if you GET a URL and, before it commits, cancel the
navigation and decide to POST to that same URL instead.

https://codereview.chromium.org/565263005/diff/60001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_tab_helper.cc (right):

https://codereview.chromium.org/565263005/diff/60001/chrome/browser/prerender...
chrome/browser/prerender/prerender_tab_helper.cc:85: void
PrerenderTabHelper::DidGetRedirectForResourceRequest(
For redirects, the difference between this hook and the previous is that on
occurs when the browser receives the redirect and the other is after the
renderer processes it. (It may reject the redirect for some reason. But there
aren't many places where this happens for a top-level navigation. The only ones
I can think of are some CSP rules.)

I do not believe that will have a meaningful difference on this code which is
just metrics anyway, but I'm less familiar with it. (As I recall, these metrics
don't quite measure things right anyway.)

https://codereview.chromium.org/565263005/diff/60001/chrome/browser/prerender...
chrome/browser/prerender/prerender_tab_helper.cc:162: void
PrerenderTabHelper::DidStartProvisionalLoadForFrame(
[Verified that, for the initial ProvisionalChangeToMainFrameUrl, duplicating the
block here is equivalent.]

https://codereview.chromium.org/565263005/diff/60001/chrome/browser/prerender...
chrome/browser/prerender/prerender_tab_helper.cc:191: web_contents());
Maybe split this and the other block into their own function.

[tburkard, 2014-09-18 09:36:04]

https://codereview.chromium.org/565263005/diff/60001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_manager.cc (left):

https://codereview.chromium.org/565263005/diff/60001/chrome/browser/prerender...
chrome/browser/prerender/prerender_manager.cc:1103: if (url != url_)
On 2014/09/17 15:08:30, David Benjamin wrote:
> On 2014/09/17 14:21:02, tburkard wrote:
> > On 2014/09/17 14:19:42, tburkard wrote:
> > > How about this logic here? Why was this removed completely?
> > 
> > Ah I see that davidben@ mentioned that this may be removed. I'd feel more
> > comfortable leaving it in there in this change, and consider removing it in
a
> > separate change, and verifying that this is indeed appropriate.
> 
> That logic is actually a pretty good indication that this check was either
> broken or redundant anyway. :-) If we were actually relying on it, we'd fail
to
> abort the async swap if you GET a URL and, before it commits, cancel the
> navigation and decide to POST to that same URL instead.

Saying a certain code path is not needed because it would result in a bug in the
event of a very unlikely corner case does not seem like valid reasoning.

Yes, there may be a bug related to this very uncommon corner case. This does not
mean that because of this, this check is redundant. Removing it could create a
whole new class of more common bugs.

Applying your logic, you could probably prune a ton of stuff in Chrome (esp.
related to navigations) and completely break Chrome in the process.

I don't see what the big issue is of duplicating this code, especially for the
purpose of landing this change.

We can file a bug and later carefully assess whether this check is indeed
redundant an can be removed, but unless we have a better answer than your logic
of "if we rely on this there is a bug for a corner case, so it must be
redundant", I am not okay with just removing it.

https://codereview.chromium.org/565263005/diff/60001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_tab_helper.cc (right):

https://codereview.chromium.org/565263005/diff/60001/chrome/browser/prerender...
chrome/browser/prerender/prerender_tab_helper.cc:85: void
PrerenderTabHelper::DidGetRedirectForResourceRequest(
On 2014/09/17 15:08:30, David Benjamin wrote:
> For redirects, the difference between this hook and the previous is that on
> occurs when the browser receives the redirect and the other is after the
> renderer processes it. (It may reject the redirect for some reason. But there
> aren't many places where this happens for a top-level navigation. The only
ones
> I can think of are some CSP rules.)
> 
> I do not believe that will have a meaningful difference on this code which is
> just metrics anyway, but I'm less familiar with it. (As I recall, these
metrics
> don't quite measure things right anyway.)

What do you mean by "previous" here?

[Avi (use Gerrit), 2014-09-18 21:00:21]

> I'm curious why removing this hook is needed for your navigation work.

ProvisionalChangeToMainFrameUrl is implemented on the back of
RenderFrameHostImpl::OnDidRedirectProvisionalLoad whose page id values are wrong
in certain corner cases. When investigating this particular corner case, Charlie
and I ran into http://crbug.com/78512 . So, rather than disentangle page id from
the guts of the implementation of this callback, it seemed easier and more
productive to just kill the callback.

David, as someone working on PlzNavigate, you have the best vision here of what
the future direction should look like. If it seems to you that it would be best
to go a different way to mesh better with your plans, please let me know and I'd
be happy to coordinate with you.

https://codereview.chromium.org/565263005/diff/60001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_manager.cc (left):

https://codereview.chromium.org/565263005/diff/60001/chrome/browser/prerender...
chrome/browser/prerender/prerender_manager.cc:1103: if (url != url_)
OK, I'm putting it back.

https://codereview.chromium.org/565263005/diff/60001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_tab_helper.cc (right):

https://codereview.chromium.org/565263005/diff/60001/chrome/browser/prerender...
chrome/browser/prerender/prerender_tab_helper.cc:85: void
PrerenderTabHelper::DidGetRedirectForResourceRequest(
On 2014/09/18 09:36:04, tburkard wrote:
> On 2014/09/17 15:08:30, David Benjamin wrote:
> > For redirects, the difference between this hook and the previous is that on
> > occurs when the browser receives the redirect and the other is after the
> > renderer processes it. (It may reject the redirect for some reason. But
there
> > aren't many places where this happens for a top-level navigation. The only
> ones
> > I can think of are some CSP rules.)
> > 
> > I do not believe that will have a meaningful difference on this code which
is
> > just metrics anyway, but I'm less familiar with it. (As I recall, these
> metrics
> > don't quite measure things right anyway.)
> 
> What do you mean by "previous" here?

The difference between ProvisionalChangeToMainFrameUrl and
DidGetRedirectForResourceRequest. Those apparently get called at different
times.

https://codereview.chromium.org/565263005/diff/60001/chrome/browser/prerender...
chrome/browser/prerender/prerender_tab_helper.cc:191: web_contents());
On 2014/09/17 15:08:30, David Benjamin wrote:
> Maybe split this and the other block into their own function.

Done.

[davidben, 2014-09-18 22:14:21]

https://codereview.chromium.org/565263005/diff/60001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_manager.cc (left):

https://codereview.chromium.org/565263005/diff/60001/chrome/browser/prerender...
chrome/browser/prerender/prerender_manager.cc:1103: if (url != url_)
On 2014/09/18 09:36:04, tburkard wrote:
> On 2014/09/17 15:08:30, David Benjamin wrote:
> > On 2014/09/17 14:21:02, tburkard wrote:
> > > On 2014/09/17 14:19:42, tburkard wrote:
> > > > How about this logic here? Why was this removed completely?
> > > 
> > > Ah I see that davidben@ mentioned that this may be removed. I'd feel more
> > > comfortable leaving it in there in this change, and consider removing it
in
> a
> > > separate change, and verifying that this is indeed appropriate.
> > 
> > That logic is actually a pretty good indication that this check was either
> > broken or redundant anyway. :-) If we were actually relying on it, we'd fail
> to
> > abort the async swap if you GET a URL and, before it commits, cancel the
> > navigation and decide to POST to that same URL instead.
> 
> Saying a certain code path is not needed because it would result in a bug in
the
> event of a very unlikely corner case does not seem like valid reasoning.
> 
> Yes, there may be a bug related to this very uncommon corner case. This does
not
> mean that because of this, this check is redundant. Removing it could create a
> whole new class of more common bugs.
> 
> Applying your logic, you could probably prune a ton of stuff in Chrome (esp.
> related to navigations) and completely break Chrome in the process.
> 
> I don't see what the big issue is of duplicating this code, especially for the
> purpose of landing this change.
> 
> We can file a bug and later carefully assess whether this check is indeed
> redundant an can be removed, but unless we have a better answer than your
logic
> of "if we rely on this there is a bug for a corner case, so it must be
> redundant", I am not okay with just removing it.

Heh. Well, the better answer is that DidFailProvisionalLoad will always fire. I
thought I'd mentioned this in a previous review, actually, though I can't seem
to find it. (I'd certainly been operating under the assumption it was defensive
when I tried to move the swap-in later. Though that work had since been set
aside for PlzNavigate which should let us do this properly.)

There's three cases where this'll trigger:

a) We get a redirect on the navigation request we are intercepting.

b) We start a new navigation.

c) The previous navigation raced with the one we're intercepting.

(a) cannot happen if things are working; the navigation request will get
throttled. If some other issue causes us to fail catch and throttle the request,
we're already racing the swap with the actual navigation and they're the same
anyway, so canceling the swap or not doesn't matter too much.

For (b), we will always get a DidFailProvisionalLoad before this signal which
will abort the pending swap.

For (c), this code actually makes us incorrectly clear the pending swap. But it
doesn't matter because we'll incorrectly clear it later anyway when the
DidFailProvisonalLoad or DidCommitProvisionalLoadForFrame of the previous
navigation failing to starting goes through.


Moreover, the failure mode of us failing to clear a pending swap is actually not
that bad. (If it were, some of these races would be a bigger problem than they
are.) We're swapping out entire WebContentses, so the worst that happens is that
we swap to the prerender when the navigation actually aborted and/or was
replaced by another. But everything is still internally consistent. (URLs match
what is shown, etc.)

Investigating the removal separately sounds reasonable though.

https://codereview.chromium.org/565263005/diff/60001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_tab_helper.cc (right):

https://codereview.chromium.org/565263005/diff/60001/chrome/browser/prerender...
chrome/browser/prerender/prerender_tab_helper.cc:85: void
PrerenderTabHelper::DidGetRedirectForResourceRequest(
On 2014/09/18 21:00:21, Avi wrote:
> On 2014/09/18 09:36:04, tburkard wrote:
> > On 2014/09/17 15:08:30, David Benjamin wrote:
> > > For redirects, the difference between this hook and the previous is that
on
> > > occurs when the browser receives the redirect and the other is after the
> > > renderer processes it. (It may reject the redirect for some reason. But
> there
> > > aren't many places where this happens for a top-level navigation. The only
> > ones
> > > I can think of are some CSP rules.)
> > > 
> > > I do not believe that will have a meaningful difference on this code which
> is
> > > just metrics anyway, but I'm less familiar with it. (As I recall, these
> > metrics
> > > don't quite measure things right anyway.)
> > 
> > What do you mean by "previous" here?
> 
> The difference between ProvisionalChangeToMainFrameUrl and
> DidGetRedirectForResourceRequest. Those apparently get called at different
> times.

Sorry, that was poorly phrased. Yeah, that was what I meant by "previous".

[davidben, 2014-09-18 22:47:34]

On 2014/09/18 21:00:21, Avi wrote:
> > I'm curious why removing this hook is needed for your navigation work.
> 
> ProvisionalChangeToMainFrameUrl is implemented on the back of
> RenderFrameHostImpl::OnDidRedirectProvisionalLoad whose page id values are
wrong
> in certain corner cases. When investigating this particular corner case,
Charlie
> and I ran into http://crbug.com/78512 . So, rather than disentangle page id
from
> the guts of the implementation of this callback, it seemed easier and more
> productive to just kill the callback.
> 
> David, as someone working on PlzNavigate, you have the best vision here of
what
> the future direction should look like. If it seems to you that it would be
best
> to go a different way to mesh better with your plans, please let me know and
I'd
> be happy to coordinate with you.

Ah. Looking at
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

It seems the page_id business and URL checks is mostly to correlate the renderer
signals with the navigation attempt? Which would be just the thing that's
awkward about intercepting navigations today and that I hope PlzNavigate will
resolve. :-) Actually, I'm sort of puzzled about whether that code even works to
begin with... it checks entry->GetURL() against source_url, right? What if you
redirect twice? Unless we're updating the entry's URL somewhere else, it doesn't
look like this hook even works...

Anyway, removing it sounds reasonable, I guess? It doesn't look like the current
one actually works, and the correlating is pretty awkward today. Intercepting
navigations at the resource layer is pretty screwy layering-wise, but it is
consistent with how we intercept navigations elsewhere today.

I vaguely suspect we'll want to restore something similar later though:
<rambling>
I'm thinking, in the future, we'll want some kind of WebContents observer or
delegate API to allow the embedder to hook into the provisional load for a
navigation at the UI thread, and at a layer where the parameters of the pending
navigation are available. The embedder should be able to pause or cancel the
intercepted navigation

- Before the request starts, but after things like beforeunload and the
same-document check have resolved.
 
https://codereview.chromium.org/519533002/diff/200001/content/browser/frame_h...
<- around line 29

- At every redirect, before the next leg is followed.
 
https://codereview.chromium.org/519533002/diff/200001/content/browser/frame_h...
<- around line 41

If they decided to pause the current navigation to do some async check, they
also need to know when the paused navigation request was canceled. I don't have
particular opinions on whether those two should be separate hooks or a single
one. If a single one, ProvisionalChangeToMainFrameUrl would be basically the
correct semantics, but without the RenderFrameHost parameter. (Maybe with a
handle to the NavigationRequest, I dunno. Or perhaps we don't expose explicit
handles and it's all implicit from events which bracket the lifetime and knowing
only one is ever in flight browser-side. I don't have particular opinions there
either.)

This would largely be to support features that needs to intercept navigations to
some URL, like prerender or Android's intent checks. In the case of Android's
intent checks, I think the check is synchronous? So it just needs to be able to
cancel it. In the case of prerender, the check is sometimes async (namely the
code we're modifying here). So for the async one, we'd pause the navigation and
do this sessionStorage merge operation, resuming or aborting the paused
navigation as appropriate.

But the navigation attempt and the URL request will be tied to each other, so we
won't need this page_id, URL, etc., check to deal with correlating the requests.
</rambling>

[Avi (use Gerrit), 2014-09-22 18:52:53]

I returned the callback to the PrerenderManager, and unified the code in the
PrerenderTabHelper.

Timo: we can have the discussion about whether all the callbacks are strictly
necessary later; WDYT?
David: that's a huge question about how things should work with Plz; what do you
think about this as it stands now?

[davidben, 2014-09-22 19:09:14]

On 2014/09/22 18:52:53, Avi wrote:
> I returned the callback to the PrerenderManager, and unified the code in the
> PrerenderTabHelper.
> 
> Timo: we can have the discussion about whether all the callbacks are strictly
> necessary later; WDYT?
> David: that's a huge question about how things should work with Plz; what do
you
> think about this as it stands now?

Oh, sorry, I seem to have buried my answer to that question in long rambling.
:-) This as it stands now lgtm.

In fact, it looks like ProvisionalChangeToMainFrameUrl for redirects basically
doesn't work at all right now. I'm sticking in LOG statements and pointing it at
a long (same-site) redirect chain. I can get at most one
ProvisionalChangeToMainFrameUrl out of the redirect codepath and sometimes zero.
(Zero if not switching renderers, one if we are.)

One of the bugs seems to be the URL problem I noted in the previous comment.
("Actually, I'm sort of puzzled about whether that code even works to
begin with... it checks entry->GetURL() against source_url, right? What if you
redirect twice? Unless we're updating the entry's URL somewhere else, it doesn't
look like this hook even works...") I'm guessing the other is your page_id
thing.

[davidben, 2014-09-22 19:13:16]

Oof. Sorry, spoke too soon.

https://codereview.chromium.org/565263005/diff/80001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/565263005/diff/80001/chrome/browser/prerender...
chrome/browser/prerender/prerender_manager.cc:1099: const
content::ResourceRedirectDetails& details) {
If we're keeping the check for now, this should listen both here and
DidStartProvisionalLoadForFrame. And actually the url check really only needs to
be in the DidStartProvisionalLoadForFrame one.

(The URL check is because this code actually sees
DidStartProvisionalLoadForFrame for the load it's intercepting.)

[Avi (use Gerrit), 2014-09-22 19:54:24]

https://codereview.chromium.org/565263005/diff/80001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/565263005/diff/80001/chrome/browser/prerender...
chrome/browser/prerender/prerender_manager.cc:1099: const
content::ResourceRedirectDetails& details) {
On 2014/09/22 19:13:16, David Benjamin wrote:
> If we're keeping the check for now, this should listen both here and
> DidStartProvisionalLoadForFrame. And actually the url check really only needs
to
> be in the DidStartProvisionalLoadForFrame one.
> 
> (The URL check is because this code actually sees
> DidStartProvisionalLoadForFrame for the load it's intercepting.)

Done.

[tburkard, 2014-09-23 08:55:48]

lgtm

[Avi (use Gerrit), 2014-09-23 14:05:32]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2014-09-23 14:27:45]

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

[commit-bot: I haz the power, 2014-09-23 14:27:53]

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

[commit-bot: I haz the power, 2014-09-23 14:41:06]

Committed patchset #6 (id:100001) as cff578772dc2728b2d2f429a629f0c6c122fa4e4

[commit-bot: I haz the power, 2014-09-23 14:41:46]

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