Fixes bug in instant where we would end up incorrectly using the

chrome/browser/instant/instant_controller.cc

 // Copyright (c) 2011 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/message_loop.h"
 #include "base/metrics/histogram.h"
 #include "base/rand_util.h"
@@ skipping 21 matching lines @@
 #if defined(TOOLKIT_VIEWS)
 #include "views/focus/focus_manager.h"
 #include "views/view.h"
 #include "views/widget/widget.h"
 #endif
 
 InstantController::InstantController(Profile* profile,
                                      InstantDelegate* delegate)
     : delegate_(delegate),
       tab_contents_(NULL),
-      is_active_(false),
       is_displayable_(false),
+      is_out_of_date_(false),

[sreeram, 2011/10/14 20:07:32]

Should we start with is_out_of_date_ being true? Consider that the
OnAutocompleteGotFocus() method creates a loader, but doesn't set this to false.

       commit_on_mouse_up_(false),
       last_transition_type_(content::PAGE_TRANSITION_LINK),
       ALLOW_THIS_IN_INITIALIZER_LIST(destroy_factory_(this)) {
   PrefService* service = profile->GetPrefs();
   if (service && !InstantFieldTrial::IsExperimentGroup(profile)) {
     // kInstantEnabledOnce was added after instant, set it now to make sure it
     // is correctly set.
     service->SetBoolean(prefs::kInstantEnabledOnce, true);
   }
 }
@@ skipping 101 matching lines @@
                                string16* suggested_text) {
   suggested_text->clear();
 
   tab_contents_ = tab_contents;
   commit_on_mouse_up_ = false;
   last_transition_type_ = match.transition;
   last_url_ = match.destination_url;
   last_user_text_ = user_text;
 
   if (!ShouldUseInstant(match)) {
-    DestroyPreviewContentsAndLeaveActive();
+    Hide();
     return false;
   }
 
   const TemplateURL* template_url = match.template_url;
   DCHECK(template_url);  // ShouldUseInstant returns false if no turl.
   if (!loader_.get())
     loader_.reset(new InstantLoader(this, template_url->id()));
-  is_active_ = true;
 
   // In some rare cases (involving group policy), Instant can go from the field
   // trial to normal mode, with no intervening call to DestroyPreviewContents()
   // This would leave the loader in a weird state, which would manifest if the
   // user pressed <Enter> without calling Update(). TODO(sreeram): Handle it.
   if (InstantFieldTrial::IsHiddenExperiment(tab_contents->profile())) {
     loader_->MaybeLoadInstantURL(tab_contents, template_url);
     return true;
   }
 
@@ skipping 17 matching lines @@
   if (loader_.get())
     loader_->SetOmniboxBounds(bounds);
 }
 
 void InstantController::DestroyPreviewContents() {
   if (!loader_.get()) {
     // We're not showing anything, nothing to do.
     return;
   }
 
-  // ReleasePreviewContents sets is_active_ to false, but we need to set it
-  // before notifying the delegate, otherwise if the delegate asks for the state
-  // we'll still be active.
-  is_active_ = false;
   delegate_->HideInstant();
   delete ReleasePreviewContents(INSTANT_COMMIT_DESTROY);
 }
 
-void InstantController::DestroyPreviewContentsAndLeaveActive() {
+void InstantController::Hide() {
+  is_out_of_date_ = true;
   commit_on_mouse_up_ = false;
   if (is_displayable_) {
     is_displayable_ = false;
     delegate_->HideInstant();
   }
   last_user_text_.clear();
 }
 
 bool InstantController::IsCurrent() {
   // TODO(mmenke):  See if we can do something more intelligent in the
   //                navigation pending case.
   return is_displayable_ && !loader_->IsNavigationPending() &&
       !loader_->needs_reload();
 }
 
 bool InstantController::PrepareForCommit() {
   // If we are not in the HIDDEN field trial, return the status of the preview.
   if (!InstantFieldTrial::IsHiddenExperiment(tab_contents_->profile()))
     return IsCurrent();
 
   TemplateURLService* model = TemplateURLServiceFactory::GetForProfile(
       tab_contents_->profile());
   if (!model)
     return false;
 
   const TemplateURL* template_url = model->GetDefaultSearchProvider();
+  // For the HIDDEN field trial we ignore |is_out_of_date_| as we're going to
+  // update the loader below with the correct value.

[sreeram, 2011/10/14 20:07:32]

Actually, we do care about is_out_of_date_ for the HIDDEN trial as well. At the
time of commit, we want to make sure that whatever we have stored in
last_user_text_ is still the current autocomplete match. Which is why if there
had been an intervening call to Update() with something that we shouldn't be
using Instant for (such as a URL), we would've cleared last_user_text_ in
Hide().

In short, "if (last_user_text_.empty())" serves the same purpose as "if
(is_out_of_date_)". Since you've now added is_out_of_date_, we might as well
stop mucking with last_user_text_ and just check is_out_of_date_ directly. But
of course, you'll need to set it to true in the if() block on lines 178-181
above (in addition to doing it in UpdateLoader()).

   if (last_user_text_.empty() ||
       !IsValidInstantTemplateURL(template_url) ||
       !loader_.get() ||
       loader_->template_url_id() != template_url->id() ||
       loader_->IsNavigationPending() ||
       loader_->is_determining_if_page_supports_instant()) {
     return false;
   }
 
   // Ignore the suggested text, as we are about to commit the verbatim query.
@@ skipping 130 matching lines @@
   loader_->MaybeLoadInstantURL(tab_contents, template_url);
 }
 
 TabContentsWrapper* InstantController::ReleasePreviewContents(
     InstantCommitType type) {
   if (!loader_.get())
     return NULL;
 
   TabContentsWrapper* tab = loader_->ReleasePreviewContents(type);
   ClearBlacklist();
-  is_active_ = false;
   is_displayable_ = false;
   commit_on_mouse_up_ = false;
   omnibox_bounds_ = gfx::Rect();
   loader_.reset();
   return tab;
 }
 
 void InstantController::CompleteRelease(TabContentsWrapper* tab) {
   tab->blocked_content_tab_helper()->SetAllContentsBlocked(false);
 }
@@ skipping 50 matching lines @@
   UpdateIsDisplayable();
 }
 
 void InstantController::SwappedTabContents(InstantLoader* loader) {
   if (is_displayable_)
     delegate_->ShowInstant(loader->preview_contents());
 }
 
 void InstantController::UpdateIsDisplayable() {
   bool displayable =
-      (loader_.get() && loader_->ready() && loader_->http_status_ok());
+      (!is_out_of_date_ && loader_.get() && loader_->ready() &&
+       loader_->http_status_ok());
   if (displayable == is_displayable_)
     return;
 
   is_displayable_ = displayable;
   if (!is_displayable_) {
     delegate_->HideInstant();
   } else {
     delegate_->ShowInstant(loader_->preview_contents());
     NotificationService::current()->Notify(
         chrome::NOTIFICATION_INSTANT_CONTROLLER_SHOWN,
         Source<InstantController>(this),
         NotificationService::NoDetails());
   }
 }
 
 void InstantController::UpdateLoader(const TemplateURL* template_url,
                                      const GURL& url,
                                      content::PageTransition transition_type,
                                      const string16& user_text,
                                      bool verbatim,
                                      string16* suggested_text) {
+  is_out_of_date_ = false;
   loader_->SetOmniboxBounds(omnibox_bounds_);
   loader_->Update(tab_contents_, template_url, url, transition_type, user_text,
                   verbatim, suggested_text);
   UpdateIsDisplayable();
 }
 
 bool InstantController::ShouldUseInstant(const AutocompleteMatch& match) {
   TemplateURLService* model = TemplateURLServiceFactory::GetForProfile(
       tab_contents_->profile());
   if (!model)
@@ skipping 39 matching lines @@
   if (destroy_factory_.empty()) {
     MessageLoop::current()->PostTask(
         FROM_HERE, destroy_factory_.NewRunnableMethod(
             &InstantController::DestroyLoaders));
   }
 }
 
 void InstantController::DestroyLoaders() {
   loaders_to_destroy_.reset();
 }