|
|
Created:
7 years, 7 months ago by Charlie Reis Modified:
7 years, 6 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, cbentzel+watch_chromium.org, melevin+watch_chromium.org, tburkard+watch_chromium.org, dhollowa+watch_chromium.org, dougw+watch_chromium.org, jam, gideonwald, dominich, gavinp+prer_chromium.org, dominich+watch_chromium.org, David Black, Jered, samarth+watch_chromium.org, darin-cc_chromium.org, kmadhusu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionReplace 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
Patch Set 1 : Initial patch #Patch Set 2 : Update tests, mark TODOs #
Total comments: 7
Patch Set 3 : Remove old invariant and add test for it. #
Total comments: 2
Patch Set 4 : Update comment #Patch Set 5 : Add test for 234809 crash. #Patch Set 6 : Rebase #
Total comments: 9
Patch Set 7 : CHECK rather than return false. #Patch Set 8 : Rebase #Patch Set 9 : Fix Instant local NTP case #Patch Set 10 : Draft for prerender test. #
Total comments: 1
Patch Set 11 : Apply Chris's prerender fix. #
Total comments: 4
Patch Set 12 : Move protocol handler to IO thread. #Patch Set 13 : Fix whitespace typo. #
Total comments: 6
Patch Set 14 : Fix style nits. #
Total comments: 2
Patch Set 15 : Change to CHECK. #Messages
Total messages: 52 (0 generated)
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
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.
mmenke may have the best idea for the test.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
Thanks, comment is nice. lgtm
On 2013/05/23 19:39:03, brettw wrote: > Thanks, comment is nice. lgtm Sorry, was away on vacation. Will look now.
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?
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.
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.
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.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/creis@chromium.org/15041004/44001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/creis@chromium.org/15041004/44001
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.
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.
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.
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.
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.
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).
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.
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.
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!
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/
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.
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.
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.
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.
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/
instant pieces LGTM
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?
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).
prerender LGTM - as do the core NavigationController changes. Thanks!
lgtm for content_view_core Will investigate if we need to handle the failure case.
Scott, can you look at chrome/browser/ui/browser_commands.cc for owners approval?
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/creis@chromium.org/15041004/117001
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?
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.
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.
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.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/creis@chromium.org/15041004/127001
Message was sent while issue was closed.
Change committed as 203492 |