Fix spurious visibility events when committing Instant.

chrome/browser/instant/instant_controller.cc

 // Copyright (c) 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/instant/instant_controller.h"
 
 #include "base/command_line.h"
 #include "base/i18n/case_conversion.h"
 #include "base/metrics/histogram.h"
 #include "base/string_util.h"
@@ skipping 299 matching lines @@
   loader_->OnUpOrDownKeyPressed(count);
   return true;
 }
 
 TabContents* InstantController::GetPreviewContents() const {
   return loader_.get() ? loader_->preview_contents() : NULL;
 }
 
 void InstantController::Hide() {
   last_active_tab_ = NULL;
-  model_.SetDisplayState(InstantModel::NOT_READY, 0, INSTANT_SIZE_PERCENT);
+
+  // The only time when the model is not already in the desired NOT_READY state
+  // and GetPreviewContents() returns NULL is when we are in the commit path.
+  // In that case, don't change the state just yet; otherwise we may cause the
+  // preview to hide unnecessarily. Instead, the state will be set correctly
+  // after the commit is done.
+  if (GetPreviewContents())
+    model_.SetDisplayState(InstantModel::NOT_READY, 0, INSTANT_SIZE_PERCENT);
 
   if (GetPreviewContents() && !last_full_text_.empty()) {
     // Send a blank query to ask the preview to clear out old results.
     last_full_text_.clear();
     last_user_text_.clear();
     loader_->Update(last_full_text_, true);
   }
 }
 
 bool InstantController::IsCurrent() const {
@@ skipping 52 matching lines @@
   // Add a fake history entry with a non-Instant search URL, so that search
   // terms extraction (for autocomplete history matches) works.
   if (HistoryService* history = HistoryServiceFactory::GetForProfile(
           preview->profile(), Profile::EXPLICIT_ACCESS)) {
     history->AddPage(url_for_history_, base::Time::Now(), NULL, 0, GURL(),
                      history::RedirectList(), last_transition_type_,
                      history::SOURCE_BROWSED, false);
   }
 
   AddPreviewUsageForHistogram(mode_, PREVIEW_COMMITTED);
-
-  // We may have gotten here from CommitInstant(), which means the loader may
-  // still be on the stack. So, schedule a destruction for later.
-  MessageLoop::current()->DeleteSoon(FROM_HERE, loader_.release());
-
-  // This call is here to reset view state. It won't actually delete |loader_|
-  // because it was just released to DeleteSoon().
   DeleteLoader();
 
   preview->web_contents()->GetController().PruneAllButActive();
 
   if (type != INSTANT_COMMIT_PRESSED_ALT_ENTER) {
     const TabContents* active_tab = browser_->GetActiveTabContents();
     AddSessionStorageHistogram(mode_, active_tab, preview);
     preview->web_contents()->GetController().CopyStateFromAndPrune(
         &active_tab->web_contents()->GetController());
   }
 
   // Browser takes ownership of the preview.
   browser_->CommitInstant(preview, type == INSTANT_COMMIT_PRESSED_ALT_ENTER);
 
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_INSTANT_COMMITTED,
       content::Source<content::WebContents>(preview->web_contents()),
       content::NotificationService::NoDetails());
 
+  model_.SetDisplayState(InstantModel::NOT_READY, 0, INSTANT_SIZE_PERCENT);
+
   // Try to create another loader immediately so that it is ready for the next
   // user interaction.
   CreateDefaultLoader();
 }
 
 void InstantController::OnAutocompleteLostFocus(
     gfx::NativeView view_gaining_focus) {
   is_omnibox_focused_ = false;
 
   // If there is no preview, nothing to do.
@@ skipping 172 matching lines @@
 void InstantController::InstantLoaderPreviewLoaded(InstantLoader* loader) {
   AddPreviewUsageForHistogram(mode_, PREVIEW_LOADED);
 }
 
 void InstantController::InstantSupportDetermined(InstantLoader* loader,
                                                  bool supports_instant) {
   if (supports_instant) {
     blacklisted_urls_.erase(loader->instant_url());
   } else {
     ++blacklisted_urls_[loader->instant_url()];
-    if (loader_ == loader) {
-      if (GetPreviewContents())
-        AddPreviewUsageForHistogram(mode_, PREVIEW_DELETED);
-
-      // Because of the state of the stack, we can't destroy the loader now.
-      MessageLoop::current()->DeleteSoon(FROM_HERE, loader_.release());
+    if (loader_ == loader)
       DeleteLoader();
-    }
   }
 
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_INSTANT_SUPPORT_DETERMINED,
       content::Source<InstantController>(this),
       content::NotificationService::NoDetails());
 }
 
 void InstantController::SwappedTabContents(InstantLoader* loader) {
   if (loader_ == loader)
@@ skipping 24 matching lines @@
 }
 
 void InstantController::ResetLoader(const std::string& instant_url,
                                     const TabContents* active_tab) {
   if (GetPreviewContents() && loader_->instant_url() != instant_url)
     DeleteLoader();
 
   if (!GetPreviewContents()) {
     loader_.reset(new InstantLoader(this, instant_url, active_tab));
     loader_->Init();
+
     // Ensure the searchbox API has the correct focus state and context.
     if (is_omnibox_focused_)
       loader_->OnAutocompleteGotFocus();
     else
       loader_->OnAutocompleteLostFocus();
     loader_->OnActiveTabModeChanged(active_tab_is_ntp_);
+
     AddPreviewUsageForHistogram(mode_, PREVIEW_CREATED);
 
     // Reset the loader timer.
     stale_loader_timer_.Stop();
     stale_loader_timer_.Start(
         FROM_HERE,
         base::TimeDelta::FromMilliseconds(kStaleLoaderTimeoutMS), this,
         &InstantController::OnStaleLoader);
   }
 }
@@ skipping 32 matching lines @@
     OnStaleLoader();
 }
 
 void InstantController::DeleteLoader() {
   last_active_tab_ = NULL;
   last_full_text_.clear();
   last_user_text_.clear();
   last_verbatim_ = false;
   last_suggestion_ = InstantSuggestion();
   last_match_was_search_ = false;
-  model_.SetDisplayState(InstantModel::NOT_READY, 0, INSTANT_SIZE_PERCENT);
   loader_processed_last_update_ = false;
   last_omnibox_bounds_ = gfx::Rect();
   url_for_history_ = GURL();
-  if (GetPreviewContents())
+  if (GetPreviewContents()) {
     AddPreviewUsageForHistogram(mode_, PREVIEW_DELETED);
-  loader_.reset();
+    model_.SetDisplayState(InstantModel::NOT_READY, 0, INSTANT_SIZE_PERCENT);
+  }
+  // Schedule the deletion for later, since we may have gotten here from a call
+  // within a |loader_| method (i.e., it's still on the stack). If we deleted
+  // the loader immediately, things would still be fine so long as the caller
+  // doesn't access any instance members after we return, but why rely on that?
+  MessageLoop::current()->DeleteSoon(FROM_HERE, loader_.release());
 }
 
 void InstantController::Show(int height, InstantSizeUnits units) {
   // Call even if showing in case height changed.
   if (!model_.is_ready())
     AddPreviewUsageForHistogram(mode_, PREVIEW_SHOWED);
   model_.SetDisplayState(InstantModel::QUERY_RESULTS, height, units);
 }
 
 void InstantController::SendBoundsToPage() {
@@ skipping 74 matching lines @@
       iter->second > kMaxInstantSupportFailures)
     return false;
 
   return true;
 }
 
 bool InstantController::IsOutOfDate() const {
   return !last_active_tab_ ||
          last_active_tab_ != browser_->GetActiveTabContents();
 }