Instant Extended: Add prominent search term support

chrome/browser/ui/toolbar/toolbar_model_impl.cc

Index: chrome/browser/ui/toolbar/toolbar_model_impl.cc
diff --git a/chrome/browser/ui/toolbar/toolbar_model_impl.cc b/chrome/browser/ui/toolbar/toolbar_model_impl.cc
index fb6d815ac947f7fa577751bcd0bb371de4ca8230..b6d9f39cc9d9ac3e686901ce9fe308593ab53625 100644
--- a/chrome/browser/ui/toolbar/toolbar_model_impl.cc
+++ b/chrome/browser/ui/toolbar/toolbar_model_impl.cc
@@ -41,7 +41,8 @@ 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() {
@@ -108,7 +109,7 @@ string16 ToolbarModelImpl::GetText(
 }
 
 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,
@@ -139,8 +140,17 @@ GURL ToolbarModelImpl::GetURL() const {
   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 {
@@ -183,8 +193,13 @@ ToolbarModel::SecurityLevel ToolbarModelImpl::GetSecurityLevel() const {
 }
 
 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,
@@ -193,7 +208,7 @@ int ToolbarModelImpl::GetIcon() const {
     IDR_OMNIBOX_HTTPS_INVALID,
   };
   DCHECK(arraysize(icon_ids) == NUM_SECURITY_LEVELS);
-  return icon_ids[GetSecurityLevel()];
+  return icon_ids[security_level];
 }
 
 string16 ToolbarModelImpl::GetEVCertName() const {
@@ -245,22 +260,31 @@ Profile* ToolbarModelImpl::GetProfile() const {
 }
 
 string16 ToolbarModelImpl::GetSearchTerms() const {
-  const WebContents* contents = delegate_->GetActiveWebContents();
-  string16 search_terms = chrome::GetSearchTerms(contents);
-
-  // 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();
+  string16 search_terms =
+      chrome::GetSearchTerms(delegate_->GetActiveWebContents());
+
+  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;
+}