[Search] No longer reverting omnibox text on instant search

chrome/browser/ui/toolbar/toolbar_model_impl.cc

-// Copyright 2012 The Chromium Authors. All rights reserved.

[sky, 2012/12/07 16:29:25]

Don't remove this.

[Mathieu, 2012/12/11 00:30:17]

Done. Not sure what happened.

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/toolbar/toolbar_model_impl.h"
 
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/autocomplete/autocomplete_input.h"
 #include "chrome/browser/google/google_util.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
@@ skipping 30 matching lines @@
     : delegate_(delegate),
       input_in_progress_(false) {
 }
 
 ToolbarModelImpl::~ToolbarModelImpl() {
 }
 
 // ToolbarModelImpl Implementation.
 string16 ToolbarModelImpl::GetText(
     bool display_search_urls_as_search_terms) const {
+
+  NavigationEntry* entry;

[akalin, 2012/12/06 23:14:46]

this still isn't fixed

[Mathieu, 2012/12/11 00:30:17]

Done. Thanks.

   if (display_search_urls_as_search_terms) {
+    const NavigationController* navigation_controller =
+        GetNavigationController();
+    if (navigation_controller) {
+      entry = navigation_controller->GetVisibleEntry();
+      if (entry && !entry->GetSearchTerms().empty()) {
+        // Entry has existing search terms.
+        return entry->GetSearchTerms();
+      }
+    }
     string16 search_terms = TryToExtractSearchTermsFromURL();

[sky, 2012/12/07 16:29:25]

If we can build this on the fly do we need to persist it to disk by session/tab
restore?

[Mathieu, 2012/12/11 00:30:17]

We would like to restore the correct search terms regardless of virtual URLs,
and maintain any "fixed" NavigationEntry in the navigation stack so that the
back/forward experience maintains the correct terms. For example, we use
SetSearchTerms in InstantController to fix a special case of the virtual URL not
being correct for the moment of a navigation. We want that persisted.

[sky, 2012/12/11 22:59:16]

Ok, but that doesn't seem to indicate we need to persist them. Couldn't we reset
when restoring?

[Mathieu, 2012/12/12 19:37:53]

Well, as I mentioned there are special cases where we fix NavigationEntries as
they come through and we use the search terms field to show omnibox text. When
restoring with the method contained in this CL, we know for sure that the whole
stack of navigation entries is correctly fixed (if the user presses the back
button, there is no-reparsing and the text is as we want it to be). Without this
field, reparsing in the correct way (i.e. handling special cases) upon restore
would require the proper context in instant_controller, which is gone at this
point since this is a restore.

[sky, 2012/12/12 21:18:05]

Forgive my pushing back, but I want to make sure we really need to persist
things to disk. What context do you need that isn't available upon restore?

I would rather see the places that need the search terms extract them as
necessary. Extracting is fairly cheap, results in far less API and doesn't
change the on disk session state.

[Mathieu, 2012/12/12 21:32:02]

No problem, glad to discuss. I'll refer now to the proposed changes in
instant_controller.cc. In this case, the user has typed out a query e.g. [q] and
the preview makes a search for [quotes]. Upon committing a certain way, the
omnibox will go through a state where the virtual URL contains "&q=q&". This
needs to be fixed with the context that the actual query being done is [quotes]
(stored in |last_suggestion.text|). The virtual URL will stay q=q which would be
wrongly extracted.

As far as API calls are concerned, this should be no more frequent than the
changes to the virtual URL, title of the page and so forth. The sync team raised
no concerns to that effect, although obviously we shouldn't be wasteful on
purpose. We think it's essential here. Let me know.

-    if (!search_terms.empty())
+    if (!search_terms.empty()) {
+      if (entry)
+        entry->SetSearchTerms(search_terms);

[sky, 2012/12/07 16:29:25]

This seems like the wrong place to do this. Shouldn't we do this on navigation,
perhaps in a tabhelper?

[Mathieu, 2012/12/11 00:30:17]

I agree that it's not necessarily elegant to put side-effects here. I've thought
about having an Observer to NAV_ENTRY_COMMITTED, but I was wondering where I'd
be putting it. Here, or OmniboxEditModel perhaps? What do you think?

[sky, 2012/12/11 22:59:16]

How about SearchTabHelper.

[Mathieu, 2012/12/12 19:37:53]

Done. Refactored TryToExtractSearchTermsFromURL to TemplateURLService, which it
was relying on anyway. Now SearchTabHelper will update the search terms on
NAV_ENTRY_COMMITTED.

       return search_terms;
+    }
   }
   std::string languages;  // Empty if we don't have a |navigation_controller|.
   Profile* profile = GetProfile();
   if (profile)
     languages = profile->GetPrefs()->GetString(prefs::kAcceptLanguages);
 
   GURL url(GetURL());
   if (url.spec().length() > content::kMaxURLDisplayChars)
     url = url.IsStandard() ? url.GetOrigin() : GURL(url.scheme() + ":");
   // Note that we can't unescape spaces here, because if the user copies this
@@ skipping 166 matching lines @@
   template_url->ExtractSearchTermsFromURL(url, &result);
   return result;
 }
 
 Profile* ToolbarModelImpl::GetProfile() const {
   NavigationController* navigation_controller = GetNavigationController();
   return navigation_controller ?
       Profile::FromBrowserContext(navigation_controller->GetBrowserContext()) :
       NULL;
 }