[Search] Store and recall search terms using NavigationEntry to improve search term extraction

chrome/browser/instant/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/instant/instant_controller.h"
 
 #include "base/command_line.h"
 #include "base/metrics/histogram.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
@@ skipping 147 matching lines @@
 bool IsFullHeight(const InstantModel& model) {
   return model.height() == 100 && model.height_units() == INSTANT_SIZE_PERCENT;
 }
 
 }  // namespace
 
 // static
 const char* InstantController::kLocalOmniboxPopupURL =
     "chrome://local-omnibox-popup/local-omnibox-popup.html";
 
+// static
+const std::string InstantController::kSearchTermsKey = "search_terms";
+
 InstantController::InstantController(chrome::BrowserInstantController* browser,
                                      bool extended_enabled,
                                      bool use_local_preview_only)
     : browser_(browser),
       extended_enabled_(extended_enabled),
       instant_enabled_(false),
       use_local_preview_only_(use_local_preview_only),
       model_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
       last_omnibox_text_has_inline_autocompletion_(false),
       last_verbatim_(false),
@@ skipping 293 matching lines @@
 content::WebContents* InstantController::GetPreviewContents() const {
   return loader_ ? loader_->contents() : NULL;
 }
 
 bool InstantController::IsPreviewingSearchResults() const {
   return model_.mode().is_search_suggestions() && IsFullHeight(model_) &&
          (last_match_was_search_ ||
           last_suggestion_.behavior == INSTANT_COMPLETE_NEVER);
 }
 
+bool InstantController::IsInstantExtendedSearch() const {
+  return extended_enabled_  && instant_enabled_ && last_match_was_search_;
+}
+
 bool InstantController::CommitIfPossible(InstantCommitType type) {
   if (!extended_enabled_ && !instant_enabled_)
     return false;
 
   DVLOG(1) << "CommitIfPossible: type=" << type << " last_omnibox_text_='"
            << last_omnibox_text_ << "' last_match_was_search_="
            << last_match_was_search_ << " instant_tab_=" << instant_tab_;
 
   // If we are on an already committed search results page, send a submit event
   // to the page, but otherwise, nothing else to do.
@@ skipping 23 matching lines @@
 
   if (type == INSTANT_COMMIT_FOCUS_LOST)
     loader_->Cancel(last_omnibox_text_);
   else if (type != INSTANT_COMMIT_NAVIGATED &&
            type != INSTANT_COMMIT_CLICKED_QUERY_SUGGESTION)
     loader_->Submit(last_omnibox_text_);
 
   content::WebContents* preview = loader_->ReleaseContents();
 
   if (extended_enabled_) {
-    // Consider what's happening:
-    //   1. The user has typed a query in the omnibox and committed it (either
-    //      by pressing Enter or clicking on the preview).
-    //   2. We commit the preview to the tab strip, and tell the page.
-    //   3. The page will update the URL hash fragment with the query terms.
-    // After steps 1 and 3, the omnibox will show the query terms. However, if
-    // the URL we are committing at step 2 doesn't already have query terms, it
-    // will flash for a brief moment as a plain URL. So, avoid that flicker by
-    // pretending that the plain URL is actually the typed query terms.
-    // TODO(samarth,beaudoin): Instead of this hack, we should add a new field
-    // to NavigationEntry to keep track of what the correct query, if any, is.
     content::NavigationEntry* entry =
         preview->GetController().GetVisibleEntry();
-    std::string url = entry->GetVirtualURL().spec();
-    if (!google_util::IsInstantExtendedAPIGoogleSearchUrl(url) &&
-        google_util::IsGoogleDomainUrl(url, google_util::ALLOW_SUBDOMAIN,
-                                       google_util::ALLOW_NON_STANDARD_PORTS)) {
-      // Hitting ENTER searches for what the user typed, so use
-      // last_omnibox_text_. Clicking on the overlay commits what is currently
-      // showing, so add in the gray text in that case.
-      std::string query(UTF16ToUTF8(last_omnibox_text_));
-      if (type != INSTANT_COMMIT_PRESSED_ENTER)
-        query += UTF16ToUTF8(last_suggestion_.text);
+    // Adjust the search terms shown in the omnibox for this query. Hitting
+    // ENTER searches for what the user typed, so use last_omnibox_text_.
+    // Clicking on the overlay commits what is currently showing, so add in the
+    // gray text in that case.
+    std::string query(UTF16ToUTF8(last_omnibox_text_));
+    if (type != INSTANT_COMMIT_PRESSED_ENTER)
+      query += UTF16ToUTF8(last_suggestion_.text);
+
+    if (type == INSTANT_COMMIT_CLICKED_QUERY_SUGGESTION ||
+        type == INSTANT_COMMIT_FOCUS_LOST) {
+      // We update the virtual URL's hash fragment to the current omnibox text
+      // to avoid the flash of a URL that may not contain any search terms. The
+      // virtual URL will get reparsed once it is updated by the page to contain
+      // the valid search terms.
       entry->SetVirtualURL(GURL(
-          url + "#q=" +
+          entry->GetVirtualURL().spec() + "#q=" +
           net::EscapeQueryParamValue(query, true)));
-      chrome::search::SearchTabHelper::FromWebContents(preview)->
-          NavigationEntryUpdated();
+    } else {
+      // The search terms contained in |query| are valid and should be
+      // persisted, with no need to be reparsed.
+      entry->SetExtraData(kSearchTermsKey, UTF8ToUTF16(query));

[sky, 2013/01/16 22:32:37]

Is there a reason we do this here rather than in a WebContentsHelper?

[Mathieu, 2013/01/17 15:59:29]

We set the displayed search terms here because InstantController contains the
whole context needed to display the correct search terms. In the lines before
this IF statement, we use |last_omnibox_text_| and |last_suggestion_.text| as
well as the commit |type| to determine the correct terms to display. This is not
context that we would have in a helper, say SearchTabHelper. 

[sky, 2013/01/17 17:23:22]

Make sense. We also persist search terms to the database. See
TemplateURLService::UpdateKeywordSearchTermsForURL. Should that set the search
term too? If not, then kSearchTermsKey should be named more narrowly.

     }
+
+    chrome::search::SearchTabHelper::FromWebContents(preview)->
+        NavigationEntryUpdated();
   }
 
   // If the preview page has navigated since the last Update(), we need to add
   // the navigation to history ourselves. Else, the page will navigate after
   // commit, and it will be added to history in the usual manner.
   const history::HistoryAddPageArgs& last_navigation =
       loader_->last_navigation();
   if (!last_navigation.url.is_empty()) {
     content::NavigationEntry* entry = preview->GetController().GetActiveEntry();
 
@@ skipping 662 matching lines @@
   std::map<std::string, int>::const_iterator iter =
       blacklisted_urls_.find(*instant_url);
   if (iter != blacklisted_urls_.end() &&
       iter->second > kMaxInstantSupportFailures) {
     RecordEventHistogram(INSTANT_CONTROLLER_EVENT_URL_BLOCKED_BY_BLACKLIST);
     return false;
   }
 
   return true;
 }