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

chrome/browser/instant/instant_controller.cc

Index: chrome/browser/instant/instant_controller.cc
diff --git a/chrome/browser/instant/instant_controller.cc b/chrome/browser/instant/instant_controller.cc
index acb9583c85849f88a7e1d93d5ca684c5db1ac6b2..d258176aa48036fd30b31f54531ce89264bcfb8f 100644
--- a/chrome/browser/instant/instant_controller.cc
+++ b/chrome/browser/instant/instant_controller.cc
@@ -165,6 +165,9 @@ bool IsFullHeight(const InstantModel& model) {
 const char* InstantController::kLocalOmniboxPopupURL =
     "chrome://local-omnibox-popup/local-omnibox-popup.html";
 
+// static
+const char* InstantController::kInstantExtendedSearchTermsKey = "search_terms";
+
 InstantController::InstantController(chrome::BrowserInstantController* browser,
                                      bool extended_enabled,
                                      bool use_local_preview_only)
@@ -478,6 +481,10 @@ bool InstantController::IsPreviewingSearchResults() const {
           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;
@@ -521,35 +528,35 @@ bool InstantController::CommitIfPossible(InstantCommitType type) {
   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);

[sreeram, 2013/01/22 04:34:47]

This isn't quite right. See
https://chromiumcodereview.appspot.com/11415276/#msg3 for my recommendation on
how to do it. Specifically, last_suggestion_.text could in fact be a blue
highlight, in which case we shouldn't add it to the query (it's already part of
last_omnibox_text_).

Of course, if there's blue highlight, it means it's a URL, so we won't actually
commit an Instant preview, but we shouldn't depend on that to not run into this
bug.

[Mathieu, 2013/01/22 23:28:15]

Done.

+
+    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)));

[sreeram, 2013/01/22 04:34:47]

Huh. Why are we not using SetExtraData here?

[Mathieu, 2013/01/22 23:28:15]

There is a case where grey text is showing and last_suggestion_.text is empty or
wrong when a suggestion is clicked (a call from the page such as
acceptSuggestion would be good).

As an example, user types [johnny], " depp" is showing as grey text, but upon
clicking on the suggestion "johnny depp movies", CommitIfPossible has no
indication that the latter query is the good one (here, |last_omnibox_text_| =
"johnny", |last_suggestion_.text| = " depp"). I use the virtual URL hack to add
the partial query ("johnny depp"), after which search term extraction will
extract the real query. There is no flash since the user very quickly sees the
query being formed ("johnny depp" and then "johnny depp movies").

There is also the case where grey text is showing (" depp") and
last_suggestion_.text is empty. Another case where the page and chrome are not
in sync. Same principle applies.

I cannot use SetExtraData because this would mean it is final: the omnibox will
favor ExtraData and take it as ground truth. This could result in wrong text in
the omnibox, permanently ("johnny depp"). 

This is making my head spin. I wish we could trust last_suggestion_.text all the
time. WDYT? 

-      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. This is to ensure that the
+      // correct terms are displayed in the omnibox (ToolbarModelImpl::GetText).
+      entry->SetExtraData(std::string(kInstantExtendedSearchTermsKey),
+                          UTF8ToUTF16(query));
     }
+
+    chrome::search::SearchTabHelper::FromWebContents(preview)->
+        NavigationEntryUpdated();

[sreeram, 2013/01/22 04:34:47]

We don't need this anymore, I think. This was originally added to let the
SearchTabHelper() know about the fixed up URL (see http://crrev.com/164271).
Instead, SearchTabHelper should always just look at not just the URL, but also
the active nav entry's search_terms extra data to determine if it should be in
MODE_SEARCH_RESULTS. (I.e., you should add that fix to this CL.)

[Mathieu, 2013/01/22 23:28:15]

I've added a check for the search terms in NavigationEntryUpdated(). Since the
virtual URL hack is still around (for now), I've kept it as well. Let me know if
that sounds reasonable.

   }
 
   // If the preview page has navigated since the last Update(), we need to add