Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10829436:
Recreate the loader as soon as it is deleted and ensure that it does not become stale. (Closed)
|
Created:
8 years, 4 months ago by Shishir Modified:
8 years, 3 months ago CC:
chromium-reviews, sreeram, gideonwald, dominich, David Black, Jered, Shishir Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionRecreate the InstantLoader as soon as it is deleted and ensure that it does not become stale.
BUG=143745
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=153574
Patch Set 1 #Patch Set 2 : Adding tests. #
Total comments: 30
Patch Set 3 : Addressing Sreeram's comments. #
Total comments: 12
Patch Set 4 : Addressing Sreerams comments. #
Total comments: 2
Patch Set 5 : Spelling fix. #
Messages
Total messages: 13 (0 generated)
Could you please take a look? I am working on the test.
Added a test. PTAL.
I've suggested a few name changes, which is a bit nitpicky, I admit. Hope it's not too inconvenient. Also, I thought about our offline discussion more, and it seems to me that it's more correct to (re)start the timer when we actually load the page, and not whenever we use it (as the latter can cause us to keep reusing a stale page forever). So, here's the algorithm I propose: 1. Whenever we do a loader_->Init() is when we (re)start the timer. 2. When the timer fires, we check is_showing_. If it's false, refresh the loader. If it's true, do nothing. 3. In Hide(), after we call delegate_->HideInstant(), check the timer. If it's not running, it means it must have expired. Refresh the loader. Step #3 makes sure that if the timer expires when we are showing the page, we refresh the loader at the first possible opportunity, instead of waiting another 3 hours. http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... File chrome/browser/instant/instant_browsertest.cc (right): http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_browsertest.cc:359: // Tests that the instant loader is refreshed when it gets stale but is not Also, why put this test here, in the middle of the file? Seems logical to add new tests (that are not obviously part of an existing group) to the bottom of the file. http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_browsertest.cc:359: // Tests that the instant loader is refreshed when it gets stale but is not Tests -> Test instant -> Instant I'd reword this comment slightly, since you are not actually testing the "when it gets stale" part: // Test that the Instant loader can be refreshed, but not if it's showing. http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_browsertest.cc:366: // Instant is not showing, so a refresh should create a new the preview a new the preview -> a new preview http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_browsertest.cc:369: WaitFor(chrome::NOTIFICATION_INSTANT_SUPPORT_DETERMINED); Put in a comment about why we are not checking EXPECT_NE(before, after). I.e., because after deleting and recreating, we could end up with the same address. http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_browsertest.cc:371: // Show instant. instant -> Instant http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... File chrome/browser/instant/instant_controller.cc (right): http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_controller.cc:56: // that the page does not become stale. // If an Instant page has not been used in these many milliseconds, ... http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_controller.cc:57: const int kInstantRefreshIntervalMS = 3 * 3600 * 1000; Perhaps call this kStaleLoaderTimeoutMS? http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_controller.cc:512: } We don't need this method (CreateLoader) at all. We could just have these: void ResetLoader(...) { if (GetPreviewContents() && ...) DeleteLoader(); if (!GetPreviewContents()) { loader_.reset(...); loader_->Init(); ResetStaleLoaderTimer(); AddPreviewUsageForHistogram(...); } } void CreateDefaultLoader(...) { const TabContents* active_tab = ...; ... const TemplateURL* template_url = ...; ... ResetLoader(instant_url, active_tab); } void OnStaleLoader() { if (!is_showing_) { DeleteLoader(); CreateDefaultLoader(); } } void ResetStaleLoaderTimer() { stale_loader_timer_.Stop(); stale_loader_timer_.Start(...); } http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... File chrome/browser/instant/instant_controller.h (right): http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_controller.h:162: // Ensures that the |loader_| uses the default instant url, recreating it if instant url -> Instant URL http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_controller.h:163: // necessary. Uses the current tab if the |loader_| needs to be recreated. You can lose this part ("Uses the current tab ... recreated"). It doesn't tell much (why or how it is being used, or even why "current" tab, when in the next line you call it "active" tab). http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_controller.h:164: // Will not do anything if the instant url could not be determined or the instant url -> Instant URL http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_controller.h:166: void ResetLoaderWithDefaultUrlAndCurrentTab(); Rename this to CreateDefaultLoader(). http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_controller.h:169: void ScheduleLoaderRefresh(); Rename to ResetStaleLoaderTimer? http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_controller.h:173: void RefreshLoader(); Call this OnStaleLoader? http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_controller.h:244: base::OneShotTimer<InstantController> loader_refresh_timer_; Call this stale_loader_timer_?
Addressed all the comments. The refresh now happens on OnAutocompleteLostFocus after the timer expires. PTAL. http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... File chrome/browser/instant/instant_browsertest.cc (right): http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_browsertest.cc:359: // Tests that the instant loader is refreshed when it gets stale but is not On 2012/08/22 17:39:35, sreeram wrote: > Also, why put this test here, in the middle of the file? Seems logical to add > new tests (that are not obviously part of an existing group) to the bottom of > the file. Done. http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_browsertest.cc:359: // Tests that the instant loader is refreshed when it gets stale but is not On 2012/08/22 17:39:35, sreeram wrote: > Tests -> Test > instant -> Instant > > I'd reword this comment slightly, since you are not actually testing the "when > it gets stale" part: > // Test that the Instant loader can be refreshed, but not if it's showing. Done. http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_browsertest.cc:366: // Instant is not showing, so a refresh should create a new the preview On 2012/08/22 17:39:35, sreeram wrote: > a new the preview -> a new preview Done. http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_browsertest.cc:369: WaitFor(chrome::NOTIFICATION_INSTANT_SUPPORT_DETERMINED); On 2012/08/22 17:39:35, sreeram wrote: > Put in a comment about why we are not checking EXPECT_NE(before, after). I.e., > because after deleting and recreating, we could end up with the same address. Done. http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_browsertest.cc:371: // Show instant. On 2012/08/22 17:39:35, sreeram wrote: > instant -> Instant Done. http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... File chrome/browser/instant/instant_controller.cc (right): http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_controller.cc:56: // that the page does not become stale. On 2012/08/22 17:39:35, sreeram wrote: > // If an Instant page has not been used in these many milliseconds, ... Done. http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_controller.cc:57: const int kInstantRefreshIntervalMS = 3 * 3600 * 1000; On 2012/08/22 17:39:35, sreeram wrote: > Perhaps call this kStaleLoaderTimeoutMS? Done. http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_controller.cc:512: } On 2012/08/22 17:39:35, sreeram wrote: > We don't need this method (CreateLoader) at all. We could just have these: > > void ResetLoader(...) { > if (GetPreviewContents() && ...) > DeleteLoader(); > if (!GetPreviewContents()) { > loader_.reset(...); > loader_->Init(); > ResetStaleLoaderTimer(); > AddPreviewUsageForHistogram(...); > } > } > > void CreateDefaultLoader(...) { > const TabContents* active_tab = ...; > ... > const TemplateURL* template_url = ...; > ... > ResetLoader(instant_url, active_tab); > } > > void OnStaleLoader() { > if (!is_showing_) { > DeleteLoader(); > CreateDefaultLoader(); > } > } > > void ResetStaleLoaderTimer() { > stale_loader_timer_.Stop(); > stale_loader_timer_.Start(...); > } Done. http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... File chrome/browser/instant/instant_controller.h (right): http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_controller.h:162: // Ensures that the |loader_| uses the default instant url, recreating it if On 2012/08/22 17:39:35, sreeram wrote: > instant url -> Instant URL Done. http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_controller.h:163: // necessary. Uses the current tab if the |loader_| needs to be recreated. On 2012/08/22 17:39:35, sreeram wrote: > You can lose this part ("Uses the current tab ... recreated"). It doesn't tell > much (why or how it is being used, or even why "current" tab, when in the next > line you call it "active" tab). Done. http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_controller.h:164: // Will not do anything if the instant url could not be determined or the On 2012/08/22 17:39:35, sreeram wrote: > instant url -> Instant URL Done. http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_controller.h:166: void ResetLoaderWithDefaultUrlAndCurrentTab(); On 2012/08/22 17:39:35, sreeram wrote: > Rename this to CreateDefaultLoader(). Done, but note that this need not create the loader if it already exists and has the right url. http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_controller.h:169: void ScheduleLoaderRefresh(); On 2012/08/22 17:39:35, sreeram wrote: > Rename to ResetStaleLoaderTimer? Done. http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_controller.h:173: void RefreshLoader(); On 2012/08/22 17:39:35, sreeram wrote: > Call this OnStaleLoader? Done. http://codereview.chromium.org/10829436/diff/1003/chrome/browser/instant/inst... chrome/browser/instant/instant_controller.h:244: base::OneShotTimer<InstantController> loader_refresh_timer_; On 2012/08/22 17:39:35, sreeram wrote: > Call this stale_loader_timer_? Done.
A couple of minor comments. Once they are addressed, I think this is good to go. http://codereview.chromium.org/10829436/diff/23001/chrome/browser/instant/ins... File chrome/browser/instant/instant_browsertest.cc (right): http://codereview.chromium.org/10829436/diff/23001/chrome/browser/instant/ins... chrome/browser/instant/instant_browsertest.cc:847: // may be resued when the new loader is created. I just realized that we _can_ test something here (and so you can remove this comment about not being able to compare stuff): // Instant is not showing, so a refresh should create a new preview contents. EXPECT_TRUE(instant()->loader()->supports_instant()); instant()->OnStaleLoader(); EXPECT_FALSE(instant()->loader()->supports_instant()); WaitFor(chrome::NOTIFICATION_INSTANT_SUPPORT_DETERMINED); ... http://codereview.chromium.org/10829436/diff/23001/chrome/browser/instant/ins... chrome/browser/instant/instant_browsertest.cc:856: EXPECT_EQ(preview_tab, instant()->GetPreviewContents()); Since we know the pointers can be reused, this isn't a very strong check. Instead, do this (because, if the loader had been deleted and recreated, it wouldn't yet support instant, let alone be showing). // Refresh the loader, the preview contents should remain the same. instant()->OnStaleLoader(); EXPECT_TRUE(instant()->is_showing()); Also, add these lines after that (and document it at the top of the test as the third bullet point): // The refresh should happen once the omnibox loses focus. EXPECT_TRUE(instant()->loader()->supports_instant()); instant()->OnAutocompleteLostFocus(); EXPECT_FALSE(instant()->loader()->supports_instant()); http://codereview.chromium.org/10829436/diff/23001/chrome/browser/instant/ins... File chrome/browser/instant/instant_controller.cc (right): http://codereview.chromium.org/10829436/diff/23001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.cc:55: // If an Instant page has not been used in this many milliseconds, it is this -> these http://codereview.chromium.org/10829436/diff/23001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.cc:551: // time the autocomplete looses focus. looses -> loses http://codereview.chromium.org/10829436/diff/23001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.cc:575: stale_loader_timer_.Stop(); Don't do this. Consider this sequence of events: 1. User focuses the omnibox. 2. We create the loader, and start the timer. 3. The loader comes back and says "instant not supported". 4. We call DeleteLoader(). --> This now stops the timer. Without this line, we'll at least try again after 3 hours to see if the page now supports Instant. If the user starts typing before 3 hours, of course, we'll try to load the page also. But it seems that it's not a bad idea to try again after 3 hours, if the user hasn't caused an explicit load by typing. http://codereview.chromium.org/10829436/diff/23001/chrome/browser/instant/ins... File chrome/browser/instant/instant_controller.h (right): http://codereview.chromium.org/10829436/diff/23001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.h:157: // necessary. Will not do anything if the Instant Url could not be determined Url -> URL in both lines above.
http://codereview.chromium.org/10829436/diff/23001/chrome/browser/instant/ins... File chrome/browser/instant/instant_browsertest.cc (right): http://codereview.chromium.org/10829436/diff/23001/chrome/browser/instant/ins... chrome/browser/instant/instant_browsertest.cc:847: // may be resued when the new loader is created. On 2012/08/24 21:06:25, sreeram wrote: > I just realized that we _can_ test something here (and so you can remove this > comment about not being able to compare stuff): > > // Instant is not showing, so a refresh should create a new preview contents. > EXPECT_TRUE(instant()->loader()->supports_instant()); > instant()->OnStaleLoader(); > EXPECT_FALSE(instant()->loader()->supports_instant()); > > WaitFor(chrome::NOTIFICATION_INSTANT_SUPPORT_DETERMINED); > ... Done. Although this works, it feels like we are using a hack to check that the loader has changed. Maybe we should add a creation time to the loader or number of loaders created to instant controller to better check this. http://codereview.chromium.org/10829436/diff/23001/chrome/browser/instant/ins... chrome/browser/instant/instant_browsertest.cc:856: EXPECT_EQ(preview_tab, instant()->GetPreviewContents()); On 2012/08/24 21:06:25, sreeram wrote: > Since we know the pointers can be reused, this isn't a very strong check. > Instead, do this (because, if the loader had been deleted and recreated, it > wouldn't yet support instant, let alone be showing). > > // Refresh the loader, the preview contents should remain the same. > instant()->OnStaleLoader(); > EXPECT_TRUE(instant()->is_showing()); > > Also, add these lines after that (and document it at the top of the test as the > third bullet point): > > // The refresh should happen once the omnibox loses focus. > EXPECT_TRUE(instant()->loader()->supports_instant()); > instant()->OnAutocompleteLostFocus(); > EXPECT_FALSE(instant()->loader()->supports_instant()); Done. Will have to stop timer here though. http://codereview.chromium.org/10829436/diff/23001/chrome/browser/instant/ins... File chrome/browser/instant/instant_controller.cc (right): http://codereview.chromium.org/10829436/diff/23001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.cc:55: // If an Instant page has not been used in this many milliseconds, it is On 2012/08/24 21:06:25, sreeram wrote: > this -> these Done. http://codereview.chromium.org/10829436/diff/23001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.cc:551: // time the autocomplete looses focus. On 2012/08/24 21:06:25, sreeram wrote: > looses -> loses Done. http://codereview.chromium.org/10829436/diff/23001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.cc:575: stale_loader_timer_.Stop(); On 2012/08/24 21:06:25, sreeram wrote: > Don't do this. Consider this sequence of events: > 1. User focuses the omnibox. > 2. We create the loader, and start the timer. > 3. The loader comes back and says "instant not supported". > 4. We call DeleteLoader(). > --> This now stops the timer. > > Without this line, we'll at least try again after 3 hours to see if the page now > supports Instant. > > If the user starts typing before 3 hours, of course, we'll try to load the page > also. But it seems that it's not a bad idea to try again after 3 hours, if the > user hasn't caused an explicit load by typing. Removed. I think we need to handle the doenst support case better - depending on why the doesn't support instant fails - like network error. http://codereview.chromium.org/10829436/diff/23001/chrome/browser/instant/ins... File chrome/browser/instant/instant_controller.h (right): http://codereview.chromium.org/10829436/diff/23001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.h:157: // necessary. Will not do anything if the Instant Url could not be determined On 2012/08/24 21:06:25, sreeram wrote: > Url -> URL in both lines above. Done.
lgtm http://codereview.chromium.org/10829436/diff/24002/chrome/browser/instant/ins... File chrome/browser/instant/instant_browsertest.cc (right): http://codereview.chromium.org/10829436/diff/24002/chrome/browser/instant/ins... chrome/browser/instant/instant_browsertest.cc:838: // - Instant loader will be recreated when omnibox looses focus after the timer looses -> loses :)
http://codereview.chromium.org/10829436/diff/24002/chrome/browser/instant/ins... File chrome/browser/instant/instant_browsertest.cc (right): http://codereview.chromium.org/10829436/diff/24002/chrome/browser/instant/ins... chrome/browser/instant/instant_browsertest.cc:838: // - Instant loader will be recreated when omnibox looses focus after the timer On 2012/08/24 22:30:01, sreeram wrote: > looses -> loses :) Done.
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Adding sky for review. PTAL.
Rubber stamp LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shishir@chromium.org/10829436/17009
Change committed as 153574 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
