|
|
Created:
6 years, 11 months ago by davidben Modified:
6 years, 11 months ago Reviewers:
mmenke CC:
chromium-reviews, tburkard+watch_chromium.org, gavinp+prer_chromium.org, cbentzel+watch_chromium.org, dominich+watch_chromium.org, davidben+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionprerender: Add NavigationOrSwapObserver in browsertest.
Prerender browser tests are currently sensitive to prerenders occurring
synchronously. Already we require an ad-hoc content::RunMessageLoop() in
PrerenderPageNewTab because that one is asynchronous. Instead, introduce
a NavigationOrSwapObserver so NavigateToURL always waits for either the
swap or a normal navigation. This allows removing the
content::RunMessageLoop() call and is necessary for fixing beforeunload
handling.
Also split off DidDisplayPass and DidPrerenderPass into helper functions.
BUG=307592, 304932
TEST=PrerenderBrowserTest.*
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243616
Patch Set 1 #
Total comments: 15
Patch Set 2 : Address comments #Messages
Total messages: 12 (0 generated)
Splitting this part out of the resource throttle prerender CL. Less to review and this'll be worthwhile however we resolve beforeunload so our tests are less sensitive to prerenders being synchronous. (Still not completely because of the OpenURL return value check.) Also it removes one content::RunMessageLoop call.
Just nits, LGTM! And nice and small enough to be an easy review - much appreciated. Could you try and make the description clearer? "Also, however we fix onbeforeunload, prerenders will need to become asynchronous. This makes the prerender tests less sensitive to this." isn't terribly clear. https://codereview.chromium.org/102433010/diff/1/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/102433010/diff/1/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:245: if (old_contents != web_contents()) Does this ever happen? If not, could use an EXPECT_EQ https://codereview.chromium.org/102433010/diff/1/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:651: class NeverStartURLRequestJob : public net::URLRequestJob { While you're here, could you rename this? Maybe NeverCompleteURLRequestJob or just HangingURLRequestJob? "NeverStart()" sounds to me like Start() should never be called. https://codereview.chromium.org/102433010/diff/1/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:676: if (first_run_) { While this isn't the most complicated class, this first run behavior isn't clear from the class name. Suggest either a short comment, or making a name that encompasses this behavior (NeverStartFirstRequestProtocolHandler?) https://codereview.chromium.org/102433010/diff/1/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:682: private: nit: Blank line before private. https://codereview.chromium.org/102433010/diff/1/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:1222: &display_test_result)); Do we need these CHECKs? Seems like we can just return false. https://codereview.chromium.org/102433010/diff/1/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:1373: // prerendered page. This comment is no longer correct.
Revised description. Also fixed build on some of the bots. https://codereview.chromium.org/102433010/diff/1/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/102433010/diff/1/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:245: if (old_contents != web_contents()) On 2014/01/07 15:09:17, mmenke wrote: > Does this ever happen? If not, could use an EXPECT_EQ Hrm. Doesn't currently happen, but I was thinking this would just apply to a particular WebContents. Perhaps we want to test two simultaneous prerenders or something. I've tweaked it to pass both TabStripModel and WebContents as parameter and added a comment to make it clear it only works for CURRENT_TAB. I'm not sure why we even have NavigateToURLWithDisposition; nothing ever passes a different disposition and we don't even prerender in that case. Anyway, I've added a TODO for now. We can make it use OpenURL's return value when all swaps become async. Though, even that's not enough for NEW_WINDOW. We'd either need to static_cast the delegate to a Browser* or call chrome::Navigate instead with a NavigateParams and look at params.browser. I think chrome::Navigate will fill that in. https://codereview.chromium.org/102433010/diff/1/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:651: class NeverStartURLRequestJob : public net::URLRequestJob { On 2014/01/07 15:09:17, mmenke wrote: > While you're here, could you rename this? Maybe NeverCompleteURLRequestJob or > just HangingURLRequestJob? "NeverStart()" sounds to me like Start() should > never be called. Done. https://codereview.chromium.org/102433010/diff/1/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:676: if (first_run_) { On 2014/01/07 15:09:17, mmenke wrote: > While this isn't the most complicated class, this first run behavior isn't clear > from the class name. Suggest either a short comment, or making a name that > encompasses this behavior (NeverStartFirstRequestProtocolHandler?) Done. https://codereview.chromium.org/102433010/diff/1/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:682: private: On 2014/01/07 15:09:17, mmenke wrote: > nit: Blank line before private. Done. https://codereview.chromium.org/102433010/diff/1/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:1222: &display_test_result)); On 2014/01/07 15:09:17, mmenke wrote: > Do we need these CHECKs? Seems like we can just return false. Done. I think it's worthwhile to be able to distinguish "DidDisplayPass() returned false" from "something went horribly wrong and we couldn't run JS at all", but there are DLOG(ERROR)s, so we're probably good? https://codereview.chromium.org/102433010/diff/1/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:1373: // prerendered page. On 2014/01/07 15:09:17, mmenke wrote: > This comment is no longer correct. Done.
Still LGTM. https://codereview.chromium.org/102433010/diff/1/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/102433010/diff/1/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:245: if (old_contents != web_contents()) On 2014/01/07 17:31:34, David Benjamin wrote: > On 2014/01/07 15:09:17, mmenke wrote: > > Does this ever happen? If not, could use an EXPECT_EQ > > Hrm. Doesn't currently happen, but I was thinking this would just apply to a > particular WebContents. Perhaps we want to test two simultaneous prerenders or > something. I've tweaked it to pass both TabStripModel and WebContents as > parameter and added a comment to make it clear it only works for CURRENT_TAB. > > I'm not sure why we even have NavigateToURLWithDisposition; nothing ever passes > a different disposition and we don't even prerender in that case. Anyway, I've > added a TODO for now. We can make it use OpenURL's return value when all swaps > become async. Though, even that's not enough for NEW_WINDOW. We'd either need to > static_cast the delegate to a Browser* or call chrome::Navigate instead with a > NavigateParams and look at params.browser. I think chrome::Navigate will fill > that in. NavigateToDestURLInNewTab()? There used to be a test that did a background tab that way, too. https://codereview.chromium.org/102433010/diff/1/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:1222: &display_test_result)); On 2014/01/07 17:31:34, David Benjamin wrote: > On 2014/01/07 15:09:17, mmenke wrote: > > Do we need these CHECKs? Seems like we can just return false. > > Done. I think it's worthwhile to be able to distinguish "DidDisplayPass() > returned false" from "something went horribly wrong and we couldn't run JS at > all", but there are DLOG(ERROR)s, so we're probably good? You could return a testing::AssertionResult, with different text in the two cases, if you really wanted. Or could just stick with the CHECK, if you're that concerned about it.
https://codereview.chromium.org/102433010/diff/1/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/102433010/diff/1/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:245: if (old_contents != web_contents()) On 2014/01/07 17:44:50, mmenke wrote: > On 2014/01/07 17:31:34, David Benjamin wrote: > > On 2014/01/07 15:09:17, mmenke wrote: > > > Does this ever happen? If not, could use an EXPECT_EQ > > > > Hrm. Doesn't currently happen, but I was thinking this would just apply to a > > particular WebContents. Perhaps we want to test two simultaneous prerenders or > > something. I've tweaked it to pass both TabStripModel and WebContents as > > parameter and added a comment to make it clear it only works for CURRENT_TAB. > > > > I'm not sure why we even have NavigateToURLWithDisposition; nothing ever > passes > > a different disposition and we don't even prerender in that case. Anyway, I've > > added a TODO for now. We can make it use OpenURL's return value when all swaps > > become async. Though, even that's not enough for NEW_WINDOW. We'd either need > to > > static_cast the delegate to a Browser* or call chrome::Navigate instead with a > > NavigateParams and look at params.browser. I think chrome::Navigate will fill > > that in. > > NavigateToDestURLInNewTab()? There used to be a test that did a background tab > that way, too. Well, NavigateToDestURLInNewTab creates a new tab and then uses CURRENT_TAB. The background tab one is gone now I guess. git log -S comes up with http://codereview.chromium.org/8392041. Right now MaybeUsePrerenderedPage doesn't run on non-CURRENT_TAB navigations at all.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidben@chromium.org/102433010/100001
Retried try job too often on win_rel for step(s) nacl_integration http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
On 2014/01/07 21:00:53, I haz the power (commit-bot) wrote: > Retried try job too often on win_rel for step(s) nacl_integration > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... Trying this again. I don't see failures in prerender_browsertest.cc and I didn't touch any other files.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidben@chromium.org/102433010/100001
Retried try job too often on win_rel for step(s) nacl_integration http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidben@chromium.org/102433010/100001
Message was sent while issue was closed.
Change committed as 243616 |