Replace PruneAllButActive with PruneAllButVisible.

Replace PruneAllButActive with PruneAllButVisible.

Before we would forget the entry for the visible page, so we now require there
to be a last committed entry, no transient entry, and optionally a new pending
entry (not an existing pending entry).

BUG=234809
TEST=Prerendering should not swap in without a last committed entry.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203492

[Charlie Reis, 2013-05-15 02:52:30]

Sreeram and Chris--
  This is still failing some tests, and I'm wondering if I can get some
suggestions from you.

Sreeram: Instant appears to be trying to swap in the instant page during OpenURL
before it has committed, which we can't allow.  (If the navigation failed, we
would show a blank page under a possibly existing NavigationEntry.)  I'm seeing
failures in RedirectToLocalOnLoadFailure, PreloadedNTPDoesntSupportInstant, and
PreloadedNTPRenderViewGone as a result.  Any ideas how this could be fixed?  Can
we call SwapInInstantNTP as part of commit instead?

Chris: I need to find a way to get the prerender_view.js test to wait for the
page to commit before proceeding.  Any tips?  I'm also looking for a way to
write a unit test for the original bug, as described in my TODO in
prerender_browsertest.cc.  I'm not sure how to set that up.

Thanks!
Charlie

[sreeram, 2013-05-15 03:02:26]

On 2013/05/15 02:52:30, creis wrote:
> Sreeram: Instant appears to be trying to swap in the instant page during
OpenURL
> before it has committed, which we can't allow.  (If the navigation failed, we
> would show a blank page under a possibly existing NavigationEntry.)

Okay, so this is due to the "local" page. If the WebContents we are swapping in
has a fully loaded and ready "remote" page (i.e., google.com), then we'll use it
as is. In which case, you shouldn't have a problem, because there would be a
committed entry in it. However, if the google.com page hasn't fully loaded yet,
we immediately LoadURL("chrome-search://local-ntp/blah/blah") into the
WebContents and then try to commit it. This is a local, baked-into-Chrome page
that does not refer to any external resources. You're right that at this point,
there may be no committed entry (if the google.com load didn't commit). I am not
sure what to do here. I can't imagine why this load would fail, so perhaps just
"promote" it to a committed entry? Alternatives?

> I'm seeing
> failures in RedirectToLocalOnLoadFailure, PreloadedNTPDoesntSupportInstant,
and
> PreloadedNTPRenderViewGone as a result.  Any ideas how this could be fixed? 
Can
> we call SwapInInstantNTP as part of commit instead?

Let's figure out the test issues after figuring out how to handle the local page
issue mentioned above.

[cbentzel, 2013-05-15 18:26:21]

mmenke may have the best idea for the test.

[mmenke, 2013-05-15 20:44:52]

On 2013/05/15 18:26:21, cbentzel wrote:
> mmenke may have the best idea for the test.

I'd suggest creatomg a net::ProtocolHandler (Which creates URLRequestJobs) that
serves up one URLRequestJobs that hangs indefinitely, and then serves up mock
URLRequestJobs that complete successfully.  To avoid having it used for favicon
requests, only use it on exact matches.

For how to inject a ProtocolHandler (Which creates URLRequestJobs) to create
URLRequestJobs, see net::URLRequestFilter.  content/test/net/ has some mock
URLRequestJobs.  I suspect you can just make one dummy job that never returns,
and create one of those mock jobs directly for the successful page load.


So what you do is this:  Navigate to a page that prerenders some other page, and
wait until the ProtocolHandler creates a URLRequestJob (job1) that hangs
indefinitely for that page.  Once that happens, trigger a navigation to the url
of the prerendered page.

My understanding is that the navigation should trigger a new page load will
request the same URL as the first request.  Have the second request the
ProtocolHandler gets return a URLRequestJob that succeeds.  That way, you can
just wait for the second navigation to complete.

At the end, you can optionally wait for job1 to be cancelled, if the prerender
should be cancelled in that case (If the prerender merely isn't used, this step
should not be done, of course).

This is complicated, since URLRequestJobs run on another thread, but I'm not
sure there's a better solution.  If we didn't have to make sure job1 was
created, we could hook into the prerendering infrastructure, and wait for the
prerender to be successfully constructed, but in that case, there wouldn't be a
guarantee whether or not job1 would have been created, which would cause flake.

If we had the first job redirect to a second URL which always hung, we could
have a WebContentsObserver on the PrerenderedContents watch for the redirect,
but I think at that point things are more complicated than waiting for job1 to
be created.

[cbentzel, 2013-05-17 00:32:14]

Thanks for doing this, Charlie.

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

https://codereview.chromium.org/15041004/diff/11001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:435: // entry, we might be able to
transfer the network request instead.
I'd like to add UMA to see how frequently this happens - possibly even by
canceling the prerender contents. I'm happy to do it in a followup CL.

https://codereview.chromium.org/15041004/diff/11001/content/browser/web_conte...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://codereview.chromium.org/15041004/diff/11001/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:1231:
DCHECK_EQ(GetEntryCount() - 1, last_committed_entry_index_);
I understand this was in here before. I think the DCHECK should either be
removed, or this logic needs to go into CanPruneAllButVisible.

In the prerender case, we could be in this state if the initial prerendered page
commits, does a navigation which also commits, and then goes back via
history.back()

I think it's OK to show the currently committed page in that case (which would
argue in favor of removing the DCHECK). Can you think of reasons that that
shouldn't happen?

https://codereview.chromium.org/15041004/diff/11001/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:1316: if
(!CanPruneAllButVisible()) {
Should this just be a DCHECK? The only callers are class methods which should
make sure that the invariant holds.

[Charlie Reis, 2013-05-17 23:50:21]

Thanks for the comments, and sorry for the delay!  Crazy week.

Responses inline.

On 2013/05/15 03:02:26, sreeram wrote:
> On 2013/05/15 02:52:30, creis wrote:
> > Sreeram: Instant appears to be trying to swap in the instant page during
> OpenURL
> > before it has committed, which we can't allow.  (If the navigation failed,
we
> > would show a blank page under a possibly existing NavigationEntry.)
> 
> Okay, so this is due to the "local" page. If the WebContents we are swapping
in
> has a fully loaded and ready "remote" page (i.e., http://google.com), then
we'll use it
> as is. In which case, you shouldn't have a problem, because there would be a
> committed entry in it. However, if the http://google.com page hasn't fully
loaded yet,
> we immediately LoadURL("chrome-search://local-ntp/blah/blah") into the
> WebContents and then try to commit it. This is a local, baked-into-Chrome page
> that does not refer to any external resources. You're right that at this
point,
> there may be no committed entry (if the http://google.com load didn't commit).
I am not
> sure what to do here. I can't imagine why this load would fail, so perhaps
just
> "promote" it to a committed entry? Alternatives?

Unfortunately, just being a "local" WebUI page doesn't guarantee it will commit.
 The NTP process could hang or crash, for example.  In that case, we would have
a possibly hung/slow WebUI process visible underneath a NavigationEntry for a
normal web page, and that sounds like a possible recipe for bindings escalation.

Is there any way to swap in the local NTP case when it commits instead?  I don't
think the current approach is safe.


On 2013/05/15 20:44:52, mmenke wrote:
> On 2013/05/15 18:26:21, cbentzel wrote:
> > mmenke may have the best idea for the test.
> 
> I'd suggest creatomg a net::ProtocolHandler (Which creates URLRequestJobs)
that
> serves up one URLRequestJobs that hangs indefinitely, and then serves up mock
> URLRequestJobs that complete successfully.  To avoid having it used for
favicon
> requests, only use it on exact matches.
> 
> For how to inject a ProtocolHandler (Which creates URLRequestJobs) to create
> URLRequestJobs, see net::URLRequestFilter.  content/test/net/ has some mock
> URLRequestJobs.  I suspect you can just make one dummy job that never returns,
> and create one of those mock jobs directly for the successful page load.
> 
> 
> So what you do is this:  Navigate to a page that prerenders some other page,
and
> wait until the ProtocolHandler creates a URLRequestJob (job1) that hangs
> indefinitely for that page.  Once that happens, trigger a navigation to the
url
> of the prerendered page.
> 
> My understanding is that the navigation should trigger a new page load will
> request the same URL as the first request.  Have the second request the
> ProtocolHandler gets return a URLRequestJob that succeeds.  That way, you can
> just wait for the second navigation to complete.
> 
> At the end, you can optionally wait for job1 to be cancelled, if the prerender
> should be cancelled in that case (If the prerender merely isn't used, this
step
> should not be done, of course).
> 
> This is complicated, since URLRequestJobs run on another thread, but I'm not
> sure there's a better solution.  If we didn't have to make sure job1 was
> created, we could hook into the prerendering infrastructure, and wait for the
> prerender to be successfully constructed, but in that case, there wouldn't be
a
> guarantee whether or not job1 would have been created, which would cause
flake.
> 
> If we had the first job redirect to a second URL which always hung, we could
> have a WebContentsObserver on the PrerenderedContents watch for the redirect,
> but I think at that point things are more complicated than waiting for job1 to
> be created.

Wow, this is much more complicated than I'm looking for.  :)  I was thinking
something more like a unit test, where I can simulate the navigate and commit
events separately (like we do in WebContents and NavigationController unit
tests).  Then we don't need to worry about timing, threads, etc.

My main question was just about prerender_unittest.cc, since I'm not sure I
understand what the calls there are doing.  PendingPrerenderTest might be
something to model it after, if I can just tell the prerender
NavigationController to load URLs and later commit them.

I'm also not sure how to fix the test in prerender_view.js to wait for commit. 
mmenke or cbentzel, any ideas there?

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

https://codereview.chromium.org/15041004/diff/11001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:435: // entry, we might be able to
transfer the network request instead.
On 2013/05/17 00:32:15, cbentzel wrote:
> I'd like to add UMA to see how frequently this happens - possibly even by
> canceling the prerender contents. I'm happy to do it in a followup CL. 

Yes, that's a good idea.  Just increment a stat here if there's no last
committed entry but there is a pending entry.  (I can do it in this CL if you
let me know what name you want.  There's a bunch for prerendering and I'm not
sure where this would fit in.)

https://codereview.chromium.org/15041004/diff/11001/content/browser/web_conte...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://codereview.chromium.org/15041004/diff/11001/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:1231:
DCHECK_EQ(GetEntryCount() - 1, last_committed_entry_index_);
On 2013/05/17 00:32:15, cbentzel wrote:
> I understand this was in here before. I think the DCHECK should either be
> removed, or this logic needs to go into CanPruneAllButVisible.

We can't put it in CanPruneAllButVisible, because it's valid to call
PruneAllButVisible when we're not at the last entry.  ViewSource does that, for
example.

> In the prerender case, we could be in this state if the initial prerendered
page
> commits, does a navigation which also commits, and then goes back via
> history.back()
> 
> I think it's OK to show the currently committed page in that case (which would
> argue in favor of removing the DCHECK). Can you think of reasons that that
> shouldn't happen?

Hmm, I can't think of any reason why that wouldn't work.  I've removed the
condition and added a test for that case.

https://codereview.chromium.org/15041004/diff/11001/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:1316: if
(!CanPruneAllButVisible()) {
On 2013/05/17 00:32:15, cbentzel wrote:
> Should this just be a DCHECK? The only callers are class methods which should
> make sure that the invariant holds.

The public PruneAllButVisible doesn't call CanPruneAllButVisible.  We're relying
on external callers there, so I'd like to keep this.

[sreeram, 2013-05-18 01:18:58]

On Fri, May 17, 2013 at 4:50 PM,  <creis@chromium.org> wrote:
> Unfortunately, just being a "local" WebUI page doesn't guarantee it will
> commit.
>  The NTP process could hang or crash, for example.  In that case, we would
> have
> a possibly hung/slow WebUI process visible underneath a NavigationEntry for
> a
> normal web page, and that sounds like a possible recipe for bindings
> escalation.
>
> Is there any way to swap in the local NTP case when it commits instead?  I
> don't
> think the current approach is safe.

Okay. This will require some work from the Instant side. I am
currently OOO and will only be back next week to work on this. Here's
what I propose doing:
1. Instant will preload google.com into a WebContents (A).
2. Instant will also preload the local page into another WebContents (B).
3. When a request comes in for the NTP, Instant will check if A is
fully loaded and ready (which requires that A have committed). If yes,
it swaps that in.
4. If A is not ready, it will check if B has committed the pending
load. If yes, it will swap B in (even if it hasn't fully loaded).
5. If B hasn't committed, we'll simply LoadURL("local page") into the
supplied WebContents. So, no swap happens; no CopyStateFromAndPrune.
(I.e., we've just changed the URL to be loaded from "chrome://newtab"
to something else; we're not changing WebContents.)

A simplification to the above scheme would be to get rid of steps 2
and 4 (so we never preload the local page, and always just LoadURL it
on demand). That would be no slower than the current way of loading
chrome://newtab on demand, but I would like to retain the flexibility
to make the local page also appear fast.

[Charlie Reis, 2013-05-18 01:27:07]

On 2013/05/18 01:18:58, sreeram wrote:
> On Fri, May 17, 2013 at 4:50 PM,  <mailto:creis@chromium.org> wrote:
> > Unfortunately, just being a "local" WebUI page doesn't guarantee it will
> > commit.
> >  The NTP process could hang or crash, for example.  In that case, we would
> > have
> > a possibly hung/slow WebUI process visible underneath a NavigationEntry for
> > a
> > normal web page, and that sounds like a possible recipe for bindings
> > escalation.
> >
> > Is there any way to swap in the local NTP case when it commits instead?  I
> > don't
> > think the current approach is safe.
> 
> Okay. This will require some work from the Instant side. I am
> currently OOO and will only be back next week to work on this. Here's
> what I propose doing:
> 1. Instant will preload http://google.com into a WebContents (A).
> 2. Instant will also preload the local page into another WebContents (B).
> 3. When a request comes in for the NTP, Instant will check if A is
> fully loaded and ready (which requires that A have committed). If yes,
> it swaps that in.
> 4. If A is not ready, it will check if B has committed the pending
> load. If yes, it will swap B in (even if it hasn't fully loaded).
> 5. If B hasn't committed, we'll simply LoadURL("local page") into the
> supplied WebContents. So, no swap happens; no CopyStateFromAndPrune.
> (I.e., we've just changed the URL to be loaded from "chrome://newtab"
> to something else; we're not changing WebContents.)
> 
> A simplification to the above scheme would be to get rid of steps 2
> and 4 (so we never preload the local page, and always just LoadURL it
> on demand). That would be no slower than the current way of loading
> chrome://newtab on demand, but I would like to retain the flexibility
> to make the local page also appear fast.

Are you already doing 2?  I don't see why we would add it unless that's already
happening.  (Seems like a huge overhead to always create two prerendered
WebContents in separate processes for the NTP.)

If I understand correctly, you could leave things as they are and just do 5 if
the local page fails to swap in (because it hasn't committed yet).  That seems
pretty straightforward.

[sreeram, 2013-05-18 02:00:34]

On Fri, May 17, 2013 at 6:27 PM,  <creis@chromium.org> wrote:
> Are you already doing 2?  I don't see why we would add it unless that's
> already
> happening.  (Seems like a huge overhead to always create two prerendered
> WebContents in separate processes for the NTP.)

Yeah, we are already preloading the local page (when we know that the
google page can't be loaded - e.g.: Google is not the search provider,
or you are known to be offline, etc).

Fair enough, I'll dial back on the two prerendered contents thing.

> If I understand correctly, you could leave things as they are and just do 5
> if
> the local page fails to swap in (because it hasn't committed yet).  That
> seems
> pretty straightforward.

Right, that's basically what I have to do.

[cbentzel, 2013-05-18 21:11:08]

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

https://codereview.chromium.org/15041004/diff/11001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:435: // entry, we might be able to
transfer the network request instead.
On 2013/05/17 23:50:21, creis wrote:
> On 2013/05/17 00:32:15, cbentzel wrote:
> > I'd like to add UMA to see how frequently this happens - possibly even by
> > canceling the prerender contents. I'm happy to do it in a followup CL. 
> 
> Yes, that's a good idea.  Just increment a stat here if there's no last
> committed entry but there is a pending entry.  (I can do it in this CL if you
> let me know what name you want.  There's a bunch for prerendering and I'm not
> sure where this would fit in.)

For now, I would actually destroy the prerender and add a new final status - see
many examples below like FINAL_STATUS_WINDOW_OPENER.

But I think it's easier to do that in a followup CL to keep this small - that
will require final_status.[h|cc] changes, plus histograms.xml change, etc.

[Charlie Reis, 2013-05-20 17:02:13]

Brett, while I work through the Instant and Prerendering questions and tests
with Sreeram and Chris, can I ask you to take a look at the NavigationController
and WebContents changes?

I'm making the invariants for calling PruneAllButActive stricter to avoid
mismatches between the page in the WebContents and what the NavigationController
thinks is committed.  You now have to pass CanPruneAllButVisible() before
calling it.

[brettw, 2013-05-22 18:22:00]

https://codereview.chromium.org/15041004/diff/23001/content/public/browser/na...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/15041004/diff/23001/content/public/browser/na...
content/public/browser/navigation_controller.h:399: // Returns whether it is
safe to call PruneAllButVisible or
It would be very helpful if this comment said why these restrictions exist and
why it isn't safe to prune if any of those conditions are met.

[Charlie Reis, 2013-05-22 20:14:44]

https://codereview.chromium.org/15041004/diff/23001/content/public/browser/na...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/15041004/diff/23001/content/public/browser/na...
content/public/browser/navigation_controller.h:399: // Returns whether it is
safe to call PruneAllButVisible or
On 2013/05/22 18:22:00, brettw wrote:
> It would be very helpful if this comment said why these restrictions exist and
> why it isn't safe to prune if any of those conditions are met.

Good call.  I summarized the comments from navigation_controller_impl.cc here.

[Charlie Reis, 2013-05-23 19:12:11]

Chris: I've added a NavigationController unit test to cover the original cause
of the crash in patch 4 (rather than a Prerendering unit test), so that's
covered.  Can I get your advice on the prerender_view.js test?  I think I need
it to wait until the prerendered page commits before swapping it in.

[brettw, 2013-05-23 19:39:03]

Thanks, comment is nice. lgtm

[cbentzel, 2013-05-28 15:43:53]

On 2013/05/23 19:39:03, brettw wrote:
> Thanks, comment is nice. lgtm

Sorry, was away on vacation. Will look now.

[cbentzel, 2013-05-28 18:39:16]

https://codereview.chromium.org/15041004/diff/44001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/15041004/diff/44001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_browsertest.cc:1374:
MessageLoop::current()->PostTask(
I'm confused. I thought what this would test the case of starting a prerender,
never committing the first navigation, and trying to swap it in (or at least
that is my interpretation of the comments). Do you have any tests for this case?

https://codereview.chromium.org/15041004/diff/44001/content/public/browser/na...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/15041004/diff/44001/content/public/browser/na...
content/public/browser/navigation_controller.h:397: virtual bool
CopyStateFromAndPrune(NavigationController* source) = 0;
The bool return argument for this (as well as PruneAllButVisible) are only based
on CanPruneAllButVisible in the implementation. However - calling that is
specified as a prerequisite to calling these functions.

I think it would make sense to change these to return void (and CHECK for
CanPruneAllButVisible internally).

What do you think?

[cbentzel, 2013-05-28 18:49:10]

https://codereview.chromium.org/15041004/diff/44001/chrome/test/data/webui/ne...
File chrome/test/data/webui/net_internals/prerender_view.js (right):

https://codereview.chromium.org/15041004/diff/44001/chrome/test/data/webui/ne...
chrome/test/data/webui/net_internals/prerender_view.js:101:
this.navigate_(prerenderInfo);
This is probably the location where we need to hold off navigating until the
prerendered page commits - rather than the checkDone_ as your TODO mentions
below. Otherwise, for "should be successful" prerenders we run the risk of
navigating too early and not swapping in the prerender.

Matt Menke is probably the best person to ask for how to pipe that information
in - he wrote all of this test infrastructure for net-internals.

[mmenke, 2013-05-28 19:06:35]

https://codereview.chromium.org/15041004/diff/44001/chrome/test/data/webui/ne...
File chrome/test/data/webui/net_internals/prerender_view.js (right):

https://codereview.chromium.org/15041004/diff/44001/chrome/test/data/webui/ne...
chrome/test/data/webui/net_internals/prerender_view.js:101:
this.navigate_(prerenderInfo);
On 2013/05/28 18:49:11, cbentzel wrote:
> This is probably the location where we need to hold off navigating until the
> prerendered page commits - rather than the checkDone_ as your TODO mentions
> below. Otherwise, for "should be successful" prerenders we run the risk of
> navigating too early and not swapping in the prerender.
> 
> Matt Menke is probably the best person to ask for how to pipe that information
> in - he wrote all of this test infrastructure for net-internals.  

It seems like the simplest thing to do is to wait until we've seen the
URLRequest for the prerender complete.  Since we're conveniently running in
net-internals, we can just watch for the corresponding NetLog events.  To do
this, you'd need to add |this| as an observer of the event stream, and hold off
on navigating until we see the start and end REQUEST_ALIVE events for the URL
request for this.url_.

Since the REQUEST_ALIVE event doesn't contain the URL, you'll have to check the
URL_REQUEST_START_JOB event to get the URL.

[mmenke, 2013-05-28 19:20:04]

https://codereview.chromium.org/15041004/diff/44001/chrome/test/data/webui/ne...
File chrome/test/data/webui/net_internals/prerender_view.js (right):

https://codereview.chromium.org/15041004/diff/44001/chrome/test/data/webui/ne...
chrome/test/data/webui/net_internals/prerender_view.js:101:
this.navigate_(prerenderInfo);
On 2013/05/28 19:06:36, mmenke wrote:
> On 2013/05/28 18:49:11, cbentzel wrote:
> > This is probably the location where we need to hold off navigating until the
> > prerendered page commits - rather than the checkDone_ as your TODO mentions
> > below. Otherwise, for "should be successful" prerenders we run the risk of
> > navigating too early and not swapping in the prerender.
> > 
> > Matt Menke is probably the best person to ask for how to pipe that
information
> > in - he wrote all of this test infrastructure for net-internals.  
> 
> It seems like the simplest thing to do is to wait until we've seen the
> URLRequest for the prerender complete.  Since we're conveniently running in
> net-internals, we can just watch for the corresponding NetLog events.  To do
> this, you'd need to add |this| as an observer of the event stream, and hold
off
> on navigating until we see the start and end REQUEST_ALIVE events for the URL
> request for this.url_.
> 
> Since the REQUEST_ALIVE event doesn't contain the URL, you'll have to check
the
> URL_REQUEST_START_JOB event to get the URL.

Just realized this may not be waiting long enough, and even it if is, there's no
guarantee that won't regress.

One simple option is to include whether or not a page has finished loading in
the prerenderInfo data structure, and wait for that to switch to true.  We can
also add it to the table, just for kicks.

May be simplest to do this in another CL, and land it first.

[commit-bot: I haz the power, 2013-05-28 20:08:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/15041004/44001

[commit-bot: I haz the power, 2013-05-28 20:19:27]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-05-28 21:04:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/15041004/44001

[Charlie Reis, 2013-05-28 21:05:24]

On 2013/05/28 20:19:27, I haz the power (commit-bot) wrote:
> Retried try job too often on chromium_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

Hmm, who clicked the commit box?  It's not ready for commit.

[aarya, 2013-05-28 21:07:28]

On 2013/05/28 21:05:24, creis wrote:
> On 2013/05/28 20:19:27, I haz the power (commit-bot) wrote:
> > Retried try job too often on chromium_presubmit for step(s) presubmit
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
> 
> Hmm, who clicked the commit box?  It's not ready for commit.

Sorry it is me, i thought the previous commit-bot failed, so checked for retry.
I won't do it now.

[Charlie Reis, 2013-05-28 23:21:59]

Thanks for your attention on this, Chris and Matt!  A few follow-up questions
below, and I've updated the patch to remove the bool return value.

https://chromiumcodereview.appspot.com/15041004/diff/44001/chrome/browser/pre...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://chromiumcodereview.appspot.com/15041004/diff/44001/chrome/browser/pre...
chrome/browser/prerender/prerender_browsertest.cc:1374:
MessageLoop::current()->PostTask(
On 2013/05/28 18:39:17, cbentzel wrote:
> I'm confused. I thought what this would test the case of starting a prerender,
> never committing the first navigation, and trying to swap it in (or at least
> that is my interpretation of the comments). Do you have any tests for this
case?

I thought that's what the FINAL_STATUS_CANCELLED line was doing, but I don't
really understand how all these helper functions in the prerender tests work.  I
was modeling this off the other tests that cancel a prerender, so I'm glad you
caught it.

What's the right way to validate that the prerender is not used when we try to
swap it in before the navigation commits?

https://chromiumcodereview.appspot.com/15041004/diff/44001/chrome/test/data/w...
File chrome/test/data/webui/net_internals/prerender_view.js (right):

https://chromiumcodereview.appspot.com/15041004/diff/44001/chrome/test/data/w...
chrome/test/data/webui/net_internals/prerender_view.js:101:
this.navigate_(prerenderInfo);
On 2013/05/28 19:20:04, mmenke wrote:
> On 2013/05/28 19:06:36, mmenke wrote:
> > On 2013/05/28 18:49:11, cbentzel wrote:
> > > This is probably the location where we need to hold off navigating until
the
> > > prerendered page commits - rather than the checkDone_ as your TODO
mentions
> > > below. Otherwise, for "should be successful" prerenders we run the risk of
> > > navigating too early and not swapping in the prerender.
> > > 
> > > Matt Menke is probably the best person to ask for how to pipe that
> information
> > > in - he wrote all of this test infrastructure for net-internals.  
> > 
> > It seems like the simplest thing to do is to wait until we've seen the
> > URLRequest for the prerender complete.  Since we're conveniently running in
> > net-internals, we can just watch for the corresponding NetLog events.  To do
> > this, you'd need to add |this| as an observer of the event stream, and hold
> off
> > on navigating until we see the start and end REQUEST_ALIVE events for the
URL
> > request for this.url_.
> > 
> > Since the REQUEST_ALIVE event doesn't contain the URL, you'll have to check
> the
> > URL_REQUEST_START_JOB event to get the URL.
> 
> Just realized this may not be waiting long enough, and even it if is, there's
no
> guarantee that won't regress.
> 
> One simple option is to include whether or not a page has finished loading in
> the prerenderInfo data structure, and wait for that to switch to true.  We can
> also add it to the table, just for kicks.
> 
> May be simplest to do this in another CL, and land it first. 

I follow you conceptually, but I'm not familiar with any of this code.  Where
does the prerenderInfo object come from, who would set the commit value on it,
and how would we wait for that to be true?

Doing this in a separate CL makes sense to me.  Matt, I'm happy to send one your
way if you can explain what needs to be done, unless you think it'll be quicker
to draft it yourself.  :)

https://chromiumcodereview.appspot.com/15041004/diff/44001/content/public/bro...
File content/public/browser/navigation_controller.h (right):

https://chromiumcodereview.appspot.com/15041004/diff/44001/content/public/bro...
content/public/browser/navigation_controller.h:397: virtual bool
CopyStateFromAndPrune(NavigationController* source) = 0;
On 2013/05/28 18:39:17, cbentzel wrote:
> The bool return argument for this (as well as PruneAllButVisible) are only
based
> on CanPruneAllButVisible in the implementation. However - calling that is
> specified as a prerequisite to calling these functions.
> 
> I think it would make sense to change these to return void (and CHECK for
> CanPruneAllButVisible internally).
> 
> What do you think?

The current approach was partly driven by my level of confidence around the
ViewSource case.  I don't think there's a way for it to fail there, but it would
avoid crashing the browser process if I was wrong.  That said, your suggestion
feels cleaner, and it's probably better to hear about and fix any issues with
ViewSource than silently dealing with them.  I've changed it to CHECK instead.

[mmenke, 2013-05-28 23:38:19]

https://chromiumcodereview.appspot.com/15041004/diff/44001/chrome/test/data/w...
File chrome/test/data/webui/net_internals/prerender_view.js (right):

https://chromiumcodereview.appspot.com/15041004/diff/44001/chrome/test/data/w...
chrome/test/data/webui/net_internals/prerender_view.js:101:
this.navigate_(prerenderInfo);
On 2013/05/28 23:22:00, creis wrote:
> On 2013/05/28 19:20:04, mmenke wrote:
> > On 2013/05/28 19:06:36, mmenke wrote:
> > > On 2013/05/28 18:49:11, cbentzel wrote:
> > > > This is probably the location where we need to hold off navigating until
> the
> > > > prerendered page commits - rather than the checkDone_ as your TODO
> mentions
> > > > below. Otherwise, for "should be successful" prerenders we run the risk
of
> > > > navigating too early and not swapping in the prerender.
> > > > 
> > > > Matt Menke is probably the best person to ask for how to pipe that
> > information
> > > > in - he wrote all of this test infrastructure for net-internals.  
> > > 
> > > It seems like the simplest thing to do is to wait until we've seen the
> > > URLRequest for the prerender complete.  Since we're conveniently running
in
> > > net-internals, we can just watch for the corresponding NetLog events.  To
do
> > > this, you'd need to add |this| as an observer of the event stream, and
hold
> > off
> > > on navigating until we see the start and end REQUEST_ALIVE events for the
> URL
> > > request for this.url_.
> > > 
> > > Since the REQUEST_ALIVE event doesn't contain the URL, you'll have to
check
> > the
> > > URL_REQUEST_START_JOB event to get the URL.
> > 
> > Just realized this may not be waiting long enough, and even it if is,
there's
> no
> > guarantee that won't regress.
> > 
> > One simple option is to include whether or not a page has finished loading
in
> > the prerenderInfo data structure, and wait for that to switch to true.  We
can
> > also add it to the table, just for kicks.
> > 
> > May be simplest to do this in another CL, and land it first. 
> 
> I follow you conceptually, but I'm not familiar with any of this code.  Where
> does the prerenderInfo object come from, who would set the commit value on it,
> and how would we wait for that to be true?
> 
> Doing this in a separate CL makes sense to me.  Matt, I'm happy to send one
your
> way if you can explain what needs to be done, unless you think it'll be
quicker
> to draft it yourself.  :)

Yea, I think it's best I just do it myself.  Think it'll be a pretty quick CL,
and you don't really gain anything by learning the code well enough to do it.

[cbentzel, 2013-05-29 19:02:01]

For the Prerender test - what we want to do is

a) Navigate to a page that triggers a prerender for URL "www.example.com".
b) Add hooks so www.example.com load never commits. 
c) Navigate to "www.example.com" 
d) Confirm that the prerendered version of www.example.com is not swapped in,
since it never committed.
e) Cancel the prerender for "www.example.com" so expectation works without
waiting for 5 minutes. 

Eventually if we add a FINAL_STATUS for the non-committed case, we can skip step
e).

This would ultimately look like

IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderNoCommitNoSwap) {
  PrerenderTestURL("files/prerender/prerender_no_commit.html",
                   FINAL_STATUS_CANCELLED,
                   0);  // This line takes care of a) and b), and sets up
expectations.
  NavigateToDestURL();  // This line takes care of c)
 
EXPECT_TRUE(UrlIsInPrerenderManager("files/prerender/prerender_no_commit.html");
// Takes care of d)

  // Post a task to cancel all the prerenders. Takes care of e)
  base::MessageLoop::current()->PostTask(
      FROM_HERE, base::Bind(&CancelAllPrerenders, GetPrerenderManager()));
  content::RunMessageLoop();
}

The hard part that this doesn't address is how to order b) and c) in a non-flaky
manner. I think you could either create a new URLRequestJob subclass which
stalls prerender_no_commit.html forever, or possibly hack testserver to never
respond for the no_commit.html URL. I think the URLREquestJob is less hacky
though.

[mmenke, 2013-05-29 19:05:34]

On 2013/05/29 19:02:01, cbentzel wrote:
> For the Prerender test - what we want to do is
> 
> a) Navigate to a page that triggers a prerender for URL "www.example.com".
> b) Add hooks so http://www.example.com load never commits. 
> c) Navigate to "www.example.com" 
> d) Confirm that the prerendered version of http://www.example.com is not
swapped in,
> since it never committed.
> e) Cancel the prerender for "www.example.com" so expectation works without
> waiting for 5 minutes. 
> 
> Eventually if we add a FINAL_STATUS for the non-committed case, we can skip
step
> e).
> 
> This would ultimately look like
> 
> IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderNoCommitNoSwap) {
>   PrerenderTestURL("files/prerender/prerender_no_commit.html",
>                    FINAL_STATUS_CANCELLED,
>                    0);  // This line takes care of a) and b), and sets up
> expectations.
>   NavigateToDestURL();  // This line takes care of c)
>  
>
EXPECT_TRUE(UrlIsInPrerenderManager("files/prerender/prerender_no_commit.html");
> // Takes care of d)
> 
>   // Post a task to cancel all the prerenders. Takes care of e)
>   base::MessageLoop::current()->PostTask(
>       FROM_HERE, base::Bind(&CancelAllPrerenders, GetPrerenderManager()));
>   content::RunMessageLoop();
> }
> 
> The hard part that this doesn't address is how to order b) and c) in a
non-flaky
> manner. I think you could either create a new URLRequestJob subclass which
> stalls prerender_no_commit.html forever, or possibly hack testserver to never
> respond for the no_commit.html URL. I think the URLREquestJob is less hacky
> though.

Yea, the URLRequestJob is the way to go.  There are a couple mocks in
content/test/net/url_request_mock_http_job.h, but none of them quite does what
you want.  The also use a deprecated method to hook things up (Other than
URLRequestPrepackagedInterceptor, which uses the new, shiny, preferred way).

[Charlie Reis, 2013-05-29 22:30:51]

I've updated the Instant changes with Sreeram's suggestion, so we pass the
Instant tests now.

mmenke also has a CL in progress for prerender_view.js:
https://codereview.chromium.org/16194008/

The new PrerenderBrowserTest should be the last thing after that.

On 2013/05/29 19:05:34, mmenke wrote:
> On 2013/05/29 19:02:01, cbentzel wrote:
> > For the Prerender test - what we want to do is
> > 
> > a) Navigate to a page that triggers a prerender for URL "www.example.com".
> > b) Add hooks so http://www.example.com load never commits. 
> > c) Navigate to "www.example.com" 
> > d) Confirm that the prerendered version of http://www.example.com is not
> swapped in,
> > since it never committed.
> > e) Cancel the prerender for "www.example.com" so expectation works without
> > waiting for 5 minutes. 
> > 
> > Eventually if we add a FINAL_STATUS for the non-committed case, we can skip
> step
> > e).
> > 
> > This would ultimately look like
> > 
> > IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderNoCommitNoSwap) {
> >   PrerenderTestURL("files/prerender/prerender_no_commit.html",
> >                    FINAL_STATUS_CANCELLED,
> >                    0);  // This line takes care of a) and b), and sets up
> > expectations.
> >   NavigateToDestURL();  // This line takes care of c)

This part fails because NavigateToDestURL assumes that the PrerenderContents
will be gone.  I've added an argument to it so that we can handle this case.

> >  
> >
>
EXPECT_TRUE(UrlIsInPrerenderManager("files/prerender/prerender_no_commit.html");
> > // Takes care of d)
> > 
> >   // Post a task to cancel all the prerenders. Takes care of e)
> >   base::MessageLoop::current()->PostTask(
> >       FROM_HERE, base::Bind(&CancelAllPrerenders, GetPrerenderManager()));
> >   content::RunMessageLoop();
> > }
> > 
> > The hard part that this doesn't address is how to order b) and c) in a
> non-flaky
> > manner. I think you could either create a new URLRequestJob subclass which
> > stalls prerender_no_commit.html forever, or possibly hack testserver to
never
> > respond for the no_commit.html URL. I think the URLREquestJob is less hacky
> > though.
> 
> Yea, the URLRequestJob is the way to go.  There are a couple mocks in
> content/test/net/url_request_mock_http_job.h, but none of them quite does what
> you want.  The also use a deprecated method to hook things up (Other than
> URLRequestPrepackagedInterceptor, which uses the new, shiny, preferred way).

My current draft uses testserver.py's slow?5, but that's obviously flaky.  It
sounds like we could add something similar to URLRequestPrepackedInterceptor
that doesn't commit a URL.  How would that work?  I'm not familiar with the
URLRequest classes.

Thanks for all the help.

https://codereview.chromium.org/15041004/diff/29002/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/15041004/diff/29002/chrome/browser/prerender/...
chrome/browser/prerender/prerender_browsertest.cc:1376: const GURL no_commit_url
= test_server()->GetURL("/slow?5");
This needs to be replaced with a request that doesn't commit.

[mmenke, 2013-05-30 16:13:52]

On 2013/05/29 22:30:51, creis wrote:
> I've updated the Instant changes with Sreeram's suggestion, so we pass the
> Instant tests now.
> 
> mmenke also has a CL in progress for prerender_view.js:
> https://codereview.chromium.org/16194008/
> 
> The new PrerenderBrowserTest should be the last thing after that.
> 
> On 2013/05/29 19:05:34, mmenke wrote:
> > On 2013/05/29 19:02:01, cbentzel wrote:
> > > For the Prerender test - what we want to do is
> > > 
> > > a) Navigate to a page that triggers a prerender for URL "www.example.com".
> > > b) Add hooks so http://www.example.com load never commits. 
> > > c) Navigate to "www.example.com" 
> > > d) Confirm that the prerendered version of http://www.example.com is not
> > swapped in,
> > > since it never committed.
> > > e) Cancel the prerender for "www.example.com" so expectation works without
> > > waiting for 5 minutes. 
> > > 
> > > Eventually if we add a FINAL_STATUS for the non-committed case, we can
skip
> > step
> > > e).
> > > 
> > > This would ultimately look like
> > > 
> > > IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderNoCommitNoSwap) {
> > >   PrerenderTestURL("files/prerender/prerender_no_commit.html",
> > >                    FINAL_STATUS_CANCELLED,
> > >                    0);  // This line takes care of a) and b), and sets up
> > > expectations.
> > >   NavigateToDestURL();  // This line takes care of c)
> 
> This part fails because NavigateToDestURL assumes that the PrerenderContents
> will be gone.  I've added an argument to it so that we can handle this case.
> 
> > >  
> > >
> >
>
EXPECT_TRUE(UrlIsInPrerenderManager("files/prerender/prerender_no_commit.html");
> > > // Takes care of d)
> > > 
> > >   // Post a task to cancel all the prerenders. Takes care of e)
> > >   base::MessageLoop::current()->PostTask(
> > >       FROM_HERE, base::Bind(&CancelAllPrerenders, GetPrerenderManager()));
> > >   content::RunMessageLoop();
> > > }
> > > 
> > > The hard part that this doesn't address is how to order b) and c) in a
> > non-flaky
> > > manner. I think you could either create a new URLRequestJob subclass which
> > > stalls prerender_no_commit.html forever, or possibly hack testserver to
> never
> > > respond for the no_commit.html URL. I think the URLREquestJob is less
hacky
> > > though.
> > 
> > Yea, the URLRequestJob is the way to go.  There are a couple mocks in
> > content/test/net/url_request_mock_http_job.h, but none of them quite does
what
> > you want.  The also use a deprecated method to hook things up (Other than
> > URLRequestPrepackagedInterceptor, which uses the new, shiny, preferred way).
> 
> My current draft uses testserver.py's slow?5, but that's obviously flaky.  It
> sounds like we could add something similar to URLRequestPrepackedInterceptor
> that doesn't commit a URL.  How would that work?  I'm not familiar with the
> URLRequest classes.
> 
> Thanks for all the help.

I'll get back to you later this afternoon.  Have a lot on my plate today.

[Charlie Reis, 2013-05-30 16:30:25]

On 2013/05/30 16:13:52, mmenke wrote:
> I'll get back to you later this afternoon.  Have a lot on my plate today.

No worries-- Chris said he would help with this particular test.  Thanks!

[cbentzel, 2013-05-30 19:54:19]

Here's the diff you should apply:

diff --git a/chrome/browser/prerender/prerender_browsertest.cc
b/chrome/browser/prerender/prerender_browsertest.cc
index 97702af..0e57bc2 100644
--- a/chrome/browser/prerender/prerender_browsertest.cc
+++ b/chrome/browser/prerender/prerender_browsertest.cc
@@ -59,6 +59,8 @@
 #include "net/dns/mock_host_resolver.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_getter.h"
+#include "net/url_request/url_request_filter.h"
+#include "net/url_request/url_request_job.h"
 #include "ui/base/l10n/l10n_util.h"

 using content::BrowserThread;
@@ -579,6 +581,33 @@ class RestorePrerenderMode {
   PrerenderManager::PrerenderManagerMode prev_mode_;
 };

+// URLRequestJob (and associated handler) which never starts.
+class NeverStartURLRequestJob : public net::URLRequestJob {
+ public:
+  NeverStartURLRequestJob(net::URLRequest* request,
+                            net::NetworkDelegate* network_delegate) :
+      net::URLRequestJob(request, network_delegate) {
+  }
+
+  virtual void Start() OVERRIDE {}
+
+ private:
+  virtual ~NeverStartURLRequestJob() {}
+};
+
+class NeverStartProtocolHandler :
+      public net::URLRequestJobFactory::ProtocolHandler {
+ public:
+  NeverStartProtocolHandler() {}
+  virtual ~NeverStartProtocolHandler() {}
+
+  virtual net::URLRequestJob* MaybeCreateJob(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate) const OVERRIDE {
+    return new NeverStartURLRequestJob(request, network_delegate);
+  }
+};
+
 }  // namespace

 class PrerenderBrowserTest : virtual public InProcessBrowserTest {
@@ -1373,7 +1402,11 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest,
PrerenderVisibility) {
 IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderNoCommitNoSwap) {
   // Navigate to a page that triggers a prerender for a URL that never
commits.
   // TODO(creis): Use a mock HTTP job for a URL that never commits.
-  const GURL no_commit_url = test_server()->GetURL("/slow?5");
+  const GURL no_commit_url("http://never-respond.example.com");
+  scoped_ptr<net::URLRequestJobFactory::ProtocolHandler>
never_respond_handler(
+      new NeverStartProtocolHandler());
+  net::URLRequestFilter::GetInstance()->AddUrlProtocolHandler(
+      no_commit_url, never_respond_handler.Pass());
   PrerenderTestURL(no_commit_url,
                    FINAL_STATUS_CANCELLED,
                    0);

On Thu, May 30, 2013 at 12:30 PM,  <creis@chromium.org> wrote:
> On 2013/05/30 16:13:52, mmenke wrote:
>>
>> I'll get back to you later this afternoon.  Have a lot on my plate today.
>
>
> No worries-- Chris said he would help with this particular test.  Thanks!
>
> https://codereview.chromium.org/15041004/

[Charlie Reis, 2013-05-30 20:20:58]

Thanks guys!  This should be ready for a final review from Sreeram and Chris
now.  I think we're only waiting for Matt's
https://codereview.chromium.org/16194008/ CL to land.

[mmenke, 2013-05-30 20:24:07]

https://codereview.chromium.org/15041004/diff/106001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/15041004/diff/106001/chrome/browser/prerender...
chrome/browser/prerender/prerender_browsertest.cc:1408: no_commit_url,
never_respond_handler.Pass());
This should be done on the IO thread.

[cbentzel, 2013-05-30 20:30:29]

https://codereview.chromium.org/15041004/diff/106001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/15041004/diff/106001/chrome/browser/prerender...
chrome/browser/prerender/prerender_browsertest.cc:1408: no_commit_url,
never_respond_handler.Pass());
On 2013/05/30 20:24:08, mmenke wrote:
> This should be done on the IO thread.

Yeah, you are right. URLRequestFilter should have DCHECK's for this. I suppose
with MessageLoop ordering we could just do a PostTask and proceed without having
to wait for an IO->UI thread ACK here that the registration completed.

[mmenke, 2013-05-30 20:33:15]

https://codereview.chromium.org/15041004/diff/106001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/15041004/diff/106001/chrome/browser/prerender...
chrome/browser/prerender/prerender_browsertest.cc:1408: no_commit_url,
never_respond_handler.Pass());
On 2013/05/30 20:30:30, cbentzel wrote:
> On 2013/05/30 20:24:08, mmenke wrote:
> > This should be done on the IO thread.
> 
> Yeah, you are right. URLRequestFilter should have DCHECK's for this. I suppose
> with MessageLoop ordering we could just do a PostTask and proceed without
having
> to wait for an IO->UI thread ACK here that the registration completed.

Yea, no need to wait for anything.

Definitely agree on the DCHECK.

[cbentzel, 2013-05-30 20:35:37]

crbug.com/245413

On Thu, May 30, 2013 at 4:33 PM,  <mmenke@chromium.org> wrote:
>
>
https://codereview.chromium.org/15041004/diff/106001/chrome/browser/prerender...
> File chrome/browser/prerender/prerender_browsertest.cc (right):
>
>
https://codereview.chromium.org/15041004/diff/106001/chrome/browser/prerender...
> chrome/browser/prerender/prerender_browsertest.cc:1408: no_commit_url,
> never_respond_handler.Pass());
> On 2013/05/30 20:30:30, cbentzel wrote:
>>
>> On 2013/05/30 20:24:08, mmenke wrote:
>> > This should be done on the IO thread.
>
>
>> Yeah, you are right. URLRequestFilter should have DCHECK's for this. I
>
> suppose
>>
>> with MessageLoop ordering we could just do a PostTask and proceed
>
> without having
>>
>> to wait for an IO->UI thread ACK here that the registration completed.
>
>
> Yea, no need to wait for anything.
>
> Definitely agree on the DCHECK.
>
> https://codereview.chromium.org/15041004/

[sreeram, 2013-05-30 22:31:45]

instant pieces LGTM

[Charlie Reis, 2013-05-30 23:09:58]

https://codereview.chromium.org/15041004/diff/106001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/15041004/diff/106001/chrome/browser/prerender...
chrome/browser/prerender/prerender_browsertest.cc:1408: no_commit_url,
never_respond_handler.Pass());
On 2013/05/30 20:33:16, mmenke wrote:
> On 2013/05/30 20:30:30, cbentzel wrote:
> > On 2013/05/30 20:24:08, mmenke wrote:
> > > This should be done on the IO thread.
> > 
> > Yeah, you are right. URLRequestFilter should have DCHECK's for this. I
suppose
> > with MessageLoop ordering we could just do a PostTask and proceed without
> having
> > to wait for an IO->UI thread ACK here that the registration completed.
> 
> Yea, no need to wait for anything.
> 
> Definitely agree on the DCHECK.

Cool.  I see Chris filed http://crbug.com/245413 for the DCHECK.  I've moved the
protocol handler creation to the IO thread here.  Can you make sure I've done it
properly?

[Charlie Reis, 2013-05-30 23:13:16]

tedchoc: Can you take a look at content_view_core_impl.cc?  It's possible the
PruneAllButVisible call will fail in ClearHistory (see
NavigationController::CanPruneAllButVisible for reasons why).

[cbentzel, 2013-05-31 01:54:52]

prerender LGTM - as do the core NavigationController changes.

Thanks!

[Ted C, 2013-05-31 05:32:10]

lgtm for content_view_core

Will investigate if we need to handle the failure case.

[Charlie Reis, 2013-05-31 15:01:33]

Scott, can you look at chrome/browser/ui/browser_commands.cc for owners
approval?

[sky, 2013-05-31 17:02:44]

LGTM

[commit-bot: I haz the power, 2013-05-31 18:19:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/15041004/117001

[mmenke, 2013-05-31 18:20:18]

A couple last-minute nits.

https://codereview.chromium.org/15041004/diff/117001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/15041004/diff/117001/chrome/browser/prerender...
chrome/browser/prerender/prerender_browsertest.cc:588: net::NetworkDelegate*
network_delegate) :
nit:  Colon should go on next line.

https://codereview.chromium.org/15041004/diff/117001/chrome/browser/prerender...
chrome/browser/prerender/prerender_browsertest.cc:598: class
NeverStartProtocolHandler :
Per the style guide, colon should go on next line, which should only be indented
4 spaces.

https://codereview.chromium.org/15041004/diff/117001/chrome/browser/prerender...
chrome/browser/prerender/prerender_browsertest.cc:1412: const GURL
no_commit_url("http://never-respond.example.com");
nit:  Should this be kNoCommitUrl?

[Charlie Reis, 2013-05-31 18:27:54]

mmenke: Good catch, updated.  PTAL.

https://codereview.chromium.org/15041004/diff/117001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/15041004/diff/117001/chrome/browser/prerender...
chrome/browser/prerender/prerender_browsertest.cc:588: net::NetworkDelegate*
network_delegate) :
On 2013/05/31 18:20:19, mmenke wrote:
> nit:  Colon should go on next line.

Done.

https://codereview.chromium.org/15041004/diff/117001/chrome/browser/prerender...
chrome/browser/prerender/prerender_browsertest.cc:598: class
NeverStartProtocolHandler :
On 2013/05/31 18:20:19, mmenke wrote:
> Per the style guide, colon should go on next line, which should only be
indented
> 4 spaces.

Done.

https://codereview.chromium.org/15041004/diff/117001/chrome/browser/prerender...
chrome/browser/prerender/prerender_browsertest.cc:1412: const GURL
no_commit_url("http://never-respond.example.com");
On 2013/05/31 18:20:19, mmenke wrote:
> nit:  Should this be kNoCommitUrl?

Done.

[mmenke, 2013-05-31 18:31:40]

LGTM, thanks!

https://codereview.chromium.org/15041004/diff/125001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/15041004/diff/125001/chrome/browser/prerender...
chrome/browser/prerender/prerender_browsertest.cc:612:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
Since this is in a test, should probably be a CHECK.  Or an ASSERT_TRUE,
depending on what you think of the whole CHECK vs ASSERT+timeout in tests issue.

[Charlie Reis, 2013-05-31 18:34:28]

https://codereview.chromium.org/15041004/diff/125001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/15041004/diff/125001/chrome/browser/prerender...
chrome/browser/prerender/prerender_browsertest.cc:612:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2013/05/31 18:31:41, mmenke wrote:
> Since this is in a test, should probably be a CHECK.  Or an ASSERT_TRUE,
> depending on what you think of the whole CHECK vs ASSERT+timeout in tests
issue.

Done.

[commit-bot: I haz the power, 2013-05-31 18:34:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/15041004/127001

[commit-bot: I haz the power, 2013-05-31 22:31:18]

Change committed as 203492