Instant Extended: Add prominent search term support

chrome/browser/ui/toolbar/toolbar_model_impl.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/ui/toolbar/toolbar_model_impl.h"
 
 #include "base/command_line.h"
 #include "base/prefs/pref_service.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/autocomplete/autocomplete_classifier.h"
@@ skipping 23 matching lines @@
 #include "net/cert/x509_certificate.h"
 #include "ui/base/l10n/l10n_util.h"
 
 using content::NavigationController;
 using content::NavigationEntry;
 using content::SSLStatus;
 using content::WebContents;
 
 ToolbarModelImpl::ToolbarModelImpl(ToolbarModelDelegate* delegate)
     : delegate_(delegate),
-      input_in_progress_(false) {
+      input_in_progress_(false),
+      is_prominent_search_term_ui_supported_(false) {
 }
 
 ToolbarModelImpl::~ToolbarModelImpl() {
 }
 
 ToolbarModel::SecurityLevel ToolbarModelImpl::GetSecurityLevelForWebContents(
       content::WebContents* web_contents) {
   if (!web_contents)
     return NONE;
 
@@ skipping 46 matching lines @@
     url = url.IsStandard() ? url.GetOrigin() : GURL(url.scheme() + ":");
   // Note that we can't unescape spaces here, because if the user copies this
   // and pastes it into another program, that program may think the URL ends at
   // the space.
   return AutocompleteInput::FormattedStringWithEquivalentMeaning(
       url, net::FormatUrl(url, languages, net::kFormatUrlOmitAll,
                           net::UnescapeRule::NORMAL, NULL, NULL, NULL));
 }
 
 string16 ToolbarModelImpl::GetCorpusNameForMobile() const {
-  if (!WouldReplaceSearchURLWithSearchTerms())
+  if (GetSearchTermType() == SEARCH_TERM_NONE)
     return string16();
   GURL url(GetURL());
   // If there is a query in the url fragment look for the corpus name there,
   // otherwise look for the corpus name in the query parameters.
   const std::string& query_str(google_util::HasGoogleSearchQueryParam(
       url.ref()) ? url.ref() : url.query());
   url_parse::Component query(0, query_str.length()), key, value;
   const char kChipKey[] = "sboxchip";
   while (url_parse::ExtractQueryKeyValue(query_str.c_str(), &query, &key,
                                          &value)) {
@@ skipping 10 matching lines @@
   const NavigationController* navigation_controller = GetNavigationController();
   if (navigation_controller) {
     const NavigationEntry* entry = navigation_controller->GetVisibleEntry();
     if (entry)
       return ShouldDisplayURL() ? entry->GetVirtualURL() : GURL();
   }
 
   return GURL(chrome::kAboutBlankURL);
 }
 
-bool ToolbarModelImpl::WouldReplaceSearchURLWithSearchTerms() const {
-  return !GetSearchTerms().empty();
+ToolbarModel::SearchTermType ToolbarModelImpl::GetSearchTermType() const {
+  string16 search_terms = GetSearchTerms();
+  if (search_terms.empty())
+    return SEARCH_TERM_NONE;
+  if (AreProminentSearchTerms(search_terms))

[sreeram, 2013/04/23 22:56:45]

This now runs the classifier for the second time (GetSearchTerms() already ran
it once). Can we avoid this somehow? Perhaps by changing GetSearchTerms to this:
    ToolbarModel::SearchTermType GetSearchTerms(string16* search_terms);

[sail, 2013/04/24 00:22:23]

Changing the API won't reduce the number of times the classifier is run.
Normally callers that care about search terms don't care about search type and
vice versa.

It seems like being able to cache search terms in general would be useful. On my
machine it takes about 100 microseconds and is called hundreds of times when the
window is resized.

[sail, 2013/04/25 02:08:16]

Done.
Gah, I obviously didn't understand this at all. Fixed, exactly as you
recommended.

+    return SEARCH_TERM_PROMINENT;
+  return SEARCH_TERM_NORMAL;
+}
+
+void ToolbarModelImpl::SetIsProminentSearchTermUISupported(bool value) {
+  is_prominent_search_term_ui_supported_ = value;
 }
 
 bool ToolbarModelImpl::ShouldDisplayURL() const {
   // Note: The order here is important.
   // - The WebUI test must come before the extension scheme test because there
   //   can be WebUIs that have extension schemes (e.g. the bookmark manager). In
   //   that case, we should prefer what the WebUI instance says.
   // - The view-source test must come before the NTP test because of the case
   //   of view-source:chrome://newtab, which should display its URL despite what
   //   chrome://newtab says.
@@ skipping 22 matching lines @@
 }
 
 ToolbarModel::SecurityLevel ToolbarModelImpl::GetSecurityLevel() const {
   if (input_in_progress_)  // When editing, assume no security style.
     return NONE;
 
   return GetSecurityLevelForWebContents(delegate_->GetActiveWebContents());
 }
 
 int ToolbarModelImpl::GetIcon() const {
-  if (WouldReplaceSearchURLWithSearchTerms())
+  SecurityLevel security_level = GetSecurityLevel();
+  SearchTermType search_term_type = GetSearchTermType();
+  if (search_term_type == SEARCH_TERM_NORMAL ||
+      (search_term_type == SEARCH_TERM_PROMINENT &&
+       security_level != SECURITY_WARNING)) {

[sreeram, 2013/04/23 22:56:45]

Do we need the security_level check? AreProminentSearchTerms() already does the
check, so what does it buy us to do it here again?

[sail, 2013/04/24 00:22:23]

This check is needed when the search term is URL shaped.

     return IDR_OMNIBOX_SEARCH;
+  }
   static int icon_ids[NUM_SECURITY_LEVELS] = {
     IDR_LOCATION_BAR_HTTP,
     IDR_OMNIBOX_HTTPS_VALID,
     IDR_OMNIBOX_HTTPS_VALID,
     IDR_OMNIBOX_HTTPS_WARNING,
     IDR_OMNIBOX_HTTPS_INVALID,
   };
   DCHECK(arraysize(icon_ids) == NUM_SECURITY_LEVELS);
-  return icon_ids[GetSecurityLevel()];
+  return icon_ids[security_level];
 }
 
 string16 ToolbarModelImpl::GetEVCertName() const {
   DCHECK_EQ(GetSecurityLevel(), EV_SECURE);
   scoped_refptr<net::X509Certificate> cert;
   // Note: Navigation controller and active entry are guaranteed non-NULL or
   // the security level would be NONE.
   content::CertStore::GetInstance()->RetrieveCert(
       GetNavigationController()->GetVisibleEntry()->GetSSL().cert_id, &cert);
   return GetEVCertName(*cert);
@@ skipping 31 matching lines @@
 }
 
 Profile* ToolbarModelImpl::GetProfile() const {
   NavigationController* navigation_controller = GetNavigationController();
   return navigation_controller ?
       Profile::FromBrowserContext(navigation_controller->GetBrowserContext()) :
       NULL;
 }
 
 string16 ToolbarModelImpl::GetSearchTerms() const {
-  const WebContents* contents = delegate_->GetActiveWebContents();
-  string16 search_terms = chrome::GetSearchTerms(contents);
+  string16 search_terms =
+      chrome::GetSearchTerms(delegate_->GetActiveWebContents());
 
-  // Don't extract search terms that the omnibox would treat as a navigation.
-  // This might confuse users into believing that the search terms were the
-  // URL of the current page, and could cause problems if users hit enter in
-  // the omnibox expecting to reload the page.
-  if (!search_terms.empty()) {
-    AutocompleteMatch match;
-    Profile* profile =
-        Profile::FromBrowserContext(contents->GetBrowserContext());
-    AutocompleteClassifierFactory::GetForProfile(profile)->Classify(
-        search_terms, false, false, &match, NULL);
-    if (!AutocompleteMatch::IsSearchType(match.type))
-      search_terms.clear();
+  if (!is_prominent_search_term_ui_supported_ &&
+      AreProminentSearchTerms(search_terms)) {
+    return string16();
   }
 
   return search_terms;
 }
+
+bool ToolbarModelImpl::AreProminentSearchTerms(
+    const string16& search_terms) const {
+  if (search_terms.empty())
+    return false;
+  if (GetSecurityLevel() == SECURITY_WARNING)
+    return true;

[sreeram, 2013/04/23 22:56:45]

Shouldn't this be "security_level != EV_SECURE && ... != SECURE"? I'm wondering
whether security_level can be NONE or SECURITY_ERROR and you'd still return
false.

[sail, 2013/04/24 00:22:23]

Done.

+
+  AutocompleteMatch match;
+  Profile* profile = Profile::FromBrowserContext(
+      delegate_->GetActiveWebContents()->GetBrowserContext());
+  AutocompleteClassifierFactory::GetForProfile(profile)->Classify(
+      search_terms, false, false, &match, NULL);
+  if (!AutocompleteMatch::IsSearchType(match.type))
+    return true;
+
+  return false;
+}