InstantExtended: don't preload the local NTP.

chrome/browser/ui/search/instant_controller.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/search/instant_controller.h"
 
 #include "base/metrics/histogram.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "base/utf_string_conversions.h"
@@ skipping 202 matching lines @@
   return true;
 }
 
 }  // namespace
 
 InstantController::InstantController(BrowserInstantController* browser,
                                      bool extended_enabled)
     : browser_(browser),
       extended_enabled_(extended_enabled),
       instant_enabled_(false),
-      use_local_overlay_only_(true),
+      use_local_page_only_(true),
       model_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
       last_omnibox_text_has_inline_autocompletion_(false),
       last_verbatim_(false),
       last_transition_type_(content::PAGE_TRANSITION_LINK),
       last_match_was_search_(false),
       omnibox_focus_state_(OMNIBOX_FOCUS_NONE),
       omnibox_bounds_(-1, -1, 0, 0),
       allow_overlay_to_show_search_suggestions_(false),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
 
@@ skipping 263 matching lines @@
     return scoped_ptr<content::WebContents>(NULL);
 
   LOG_INSTANT_DEBUG_EVENT(this, "ReleaseNTPContents");
 
   // Switch to the local NTP unless we're already using one.
   if (!ntp_ || (ShouldSwitchToLocalNTP() && !ntp_->IsLocalNTP()))
     ResetNTP(false, true);
 
   scoped_ptr<content::WebContents> ntp_contents = ntp_->ReleaseContents();
 
-  // Override the blacklist on an explicit user action.
-  ResetNTP(true, false);
+  if (!use_local_page_only_) {
+    // Preload a new Instant NTP, unless using the local NTP which is not
+    // preloaded to conserve memory.
+    //
+    // Since this corresponds to a user action, give the remote page another
+    // try, even if it's blacklisted. We don't want to blacklist pages for the
+    // duration of a Browser object (for some people, that's effectively
+    // "forever").
+    ResetNTP(true, false);
+  }
   return ntp_contents.Pass();
 }
 
 // TODO(tonyg): This method only fires when the omnibox bounds change. It also
 // needs to fire when the overlay bounds change (e.g.: open/close info bar).
 void InstantController::SetPopupBounds(const gfx::Rect& bounds) {
   if (!extended_enabled_ && !instant_enabled_)
     return;
 
   if (popup_bounds_ == bounds)
@@ skipping 361 matching lines @@
 void InstantController::TabDeactivated(content::WebContents* contents) {
   LOG_INSTANT_DEBUG_EVENT(this, "TabDeactivated");
   if (extended_enabled_ && !contents->IsBeingDestroyed())
     CommitIfPossible(INSTANT_COMMIT_FOCUS_LOST);
 
   if (GetOverlayContents())
     HideOverlay();
 }
 
 void InstantController::SetInstantEnabled(bool instant_enabled,
-                                          bool use_local_overlay_only) {
+                                          bool use_local_page_only) {
   LOG_INSTANT_DEBUG_EVENT(this, base::StringPrintf(
-      "SetInstantEnabled: instant_enabled=%d, use_local_overlay_only=%d",
-      instant_enabled, use_local_overlay_only));
+      "SetInstantEnabled: instant_enabled=%d, use_local_page_only=%d",
+      instant_enabled, use_local_page_only));
 
-  // Non extended mode does not care about |use_local_overlay_only|.
+  // Non extended mode does not care about |use_local_page_only|.
   if (instant_enabled == instant_enabled_ &&
       (!extended_enabled_ ||
-       use_local_overlay_only == use_local_overlay_only_)) {
+       use_local_page_only == use_local_page_only_)) {
     return;
   }
 
   instant_enabled_ = instant_enabled;
-  use_local_overlay_only_ = use_local_overlay_only;
+  use_local_page_only_ = use_local_page_only;
   HideInternal();
   overlay_.reset();
   if (extended_enabled_ || instant_enabled_)
     EnsureOverlayIsCurrent(false);
-  if (extended_enabled_)
-    ResetNTP(false, false);
+  if (extended_enabled_) {
+    // Preload the Instant NTP. But if we are using the local NTP, delete any
+    // existing preloaded NTP (which we should no longer use) and don't preload
+    // the local one to conserve memory.
+    if (use_local_page_only_)
+      ntp_.reset();
+    else
+      ResetNTP(false, false);
+  }
   if (instant_tab_)
     instant_tab_->SetDisplayInstantResults(instant_enabled_);
 }
 
 void InstantController::ThemeChanged(const ThemeBackgroundInfo& theme_info) {
   if (!extended_enabled_)
     return;
 
   if (overlay_)
     overlay_->SendThemeBackgroundInfo(theme_info);
@@ skipping 522 matching lines @@
   DCHECK_LE(0, intersection.x());
   DCHECK_LE(0, intersection.width());
   DCHECK_LE(0, intersection.height());
 
   overlay_->SetPopupBounds(intersection);
 }
 
 bool InstantController::GetInstantURL(Profile* profile,
                                       bool ignore_blacklist,
                                       std::string* instant_url) const {
-  if (extended_enabled_ && use_local_overlay_only_)
+  if (extended_enabled_ && use_local_page_only_)
     return false;
 
   const GURL instant_url_obj = chrome::GetInstantURL(profile,
                                                      omnibox_bounds_.x());
   if (!instant_url_obj.is_valid())
     return false;
 
   *instant_url = instant_url_obj.spec();
 
   if (!ignore_blacklist) {
@@ skipping 10 matching lines @@
   }
 
   return true;
 }
 
 void InstantController::BlacklistAndResetNTP() {
   ++blacklisted_urls_[ntp_->instant_url()];
   RecordEventHistogram(INSTANT_CONTROLLER_EVENT_URL_ADDED_TO_BLACKLIST);
   delete ntp_->ReleaseContents().release();
   MessageLoop::current()->DeleteSoon(FROM_HERE, ntp_.release());
   ResetNTP(false, false);

[sreeram, 2013/04/18 00:00:34]

You should add a similar sort of "if (!use_local_page_only_)" check here, I
think. I suspect that at shutdown we might get a RenderViewGone(), which might
lead to this place trying to reload the page.

[samarth, 2013/04/18 00:31:19]

The only situation I can think of where we get here with use_local_page_only_ is
if we load the local NTP in ReleaseNTPContents and it crashes before we release
it (is that even possible?).  And in that case, reloading sounds right to me.

During regular shutdown, ntp_ would be NULL since we never preload ntp_ if
|use_local_page_only_|

 }
 
 void InstantController::BlacklistAndResetOverlay() {
   ++blacklisted_urls_[overlay_->instant_url()];
   RecordEventHistogram(INSTANT_CONTROLLER_EVENT_URL_ADDED_TO_BLACKLIST);
   HideInternal();
   delete overlay_->ReleaseContents().release();
   MessageLoop::current()->DeleteSoon(FROM_HERE, overlay_.release());
   EnsureOverlayIsCurrent(false);
 }
@@ skipping 146 matching lines @@
     return false;
 
   // If this is not window startup, switch.
   // TODO(shishir): This is not completely reliable. Find a better way to detect
   // startup time.
   if (browser_->GetActiveWebContents())
     return true;
 
   return chrome::IsAggressiveLocalNTPFallbackEnabled();
 }