Make the OmniboxNavigationObserver update the ShortcutsProvider.

chrome/browser/history/shortcuts_backend.cc

 // Copyright (c) 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/history/shortcuts_backend.h"
 
 #include <map>
 #include <string>
 #include <vector>
 
@@ skipping 142 matching lines @@
   if (!suppress_db) {
     db_ = new ShortcutsDatabase(
         profile->GetPath().Append(chrome::kShortcutsDatabaseName));
   }
   // |profile| can be NULL in tests.
   if (profile) {
     notification_registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_UNLOADED,
                                 content::Source<Profile>(profile));
     notification_registrar_.Add(this, chrome::NOTIFICATION_HISTORY_URLS_DELETED,
                                 content::Source<Profile>(profile));
-    notification_registrar_.Add(this, chrome::NOTIFICATION_OMNIBOX_OPENED_URL,
-                                content::Source<Profile>(profile));
   }
 }
 
 bool ShortcutsBackend::Init() {
   if (current_state_ != NOT_INITIALIZED)
     return false;
 
   if (no_db_access_) {
     current_state_ = INITIALIZED;
     return true;
   }
 
   current_state_ = INITIALIZING;
   return BrowserThread::PostTask(BrowserThread::DB, FROM_HERE,
       base::Bind(&ShortcutsBackend::InitInternal, this));
 }
 
 bool ShortcutsBackend::DeleteShortcutsWithUrl(const GURL& shortcut_url) {
   return initialized() && DeleteShortcutsWithUrl(shortcut_url, true);
 }
 
 void ShortcutsBackend::AddObserver(ShortcutsBackendObserver* obs) {
   observer_list_.AddObserver(obs);
 }
 
 void ShortcutsBackend::RemoveObserver(ShortcutsBackendObserver* obs) {
   observer_list_.RemoveObserver(obs);
 }
 
+void ShortcutsBackend::OnOmniboxNavigation(const string16& text,
+                                           const AutocompleteMatch& match) {
+  string16 text_lowercase(base::i18n::ToLower(text));

[Bart N., 2013/10/14 21:31:54]

const

[Peter Kasting, 2013/10/16 01:28:55]

I agree, and I made the change here, but note that most Chrome code does not use
(and many reviewers frown on) const on locals.

+
+  for (ShortcutMap::const_iterator it(

[Bart N., 2013/10/14 21:31:54]

Hmmm... why don't you use GUID -> shortcut iterator map here? It seems that you
need to look up match.destination_url and check for the prefix.

[Peter Kasting, 2013/10/16 01:28:55]

I'm confused.  How is using the GUID map useful?  We don't have a GUID sitting
around, so we'd have to iterate through the whole map; at least with the
ShortcutMap we can reduce to only iterating through shortcuts whose text begins
with the user text, which should be a much smaller set.

Plus, with the GUID map, we'd still want to check the text to ensure it begins
with the user's input... so it just seems worse to use this?

[Bart N., 2013/10/16 17:42:47]

Sorry, my mistake. I thought that this was a URL hash as opposite to a global
ID. If so, my proposal was to reduce time to O(1) from O(logN).

+       shortcuts_map_.lower_bound(text_lowercase));
+       it != shortcuts_map_.end() &&
+           StartsWith(it->first, text_lowercase, true); ++it) {
+    if (match.destination_url == it->second.match_core.destination_url) {
+      UpdateShortcut(Shortcut(it->second.id, text, Shortcut::MatchCore(match),
+                              base::Time::Now(),

[Bart N., 2013/10/14 21:31:54]

For consistency and performance, you should take this outside the loop, i.e.

const base::Time now = base::Time::Now();

[Peter Kasting, 2013/10/16 01:28:55]

Done.

+                              it->second.number_of_hits + 1));
+      return;
+    }
+  }
+  AddShortcut(Shortcut(base::GenerateGUID(), text, Shortcut::MatchCore(match),
+                       base::Time::Now(), 1));

[Bart N., 2013/10/14 21:31:54]

Replace it with "now" introduced in previous step.

[Peter Kasting, 2013/10/16 01:28:55]

Done.

+}
+
 ShortcutsBackend::~ShortcutsBackend() {
 }
 
 void ShortcutsBackend::ShutdownOnUIThread() {
   DCHECK(!BrowserThread::IsThreadInitialized(BrowserThread::UI) ||
          BrowserThread::CurrentlyOn(BrowserThread::UI));
   notification_registrar_.RemoveAll();
 }
 
 void ShortcutsBackend::Observe(int type,
                                const content::NotificationSource& source,
                                const content::NotificationDetails& details) {
   if (!initialized())
     return;
 
   if (type == chrome::NOTIFICATION_EXTENSION_UNLOADED) {
     // When an extension is unloaded, we want to remove any Shortcuts associated
     // with it.
     DeleteShortcutsWithUrl(content::Details<extensions::UnloadedExtensionInfo>(
         details)->extension->url(), false);
     return;
   }
 
-  if (type == chrome::NOTIFICATION_HISTORY_URLS_DELETED) {
-    if (content::Details<const history::URLsDeletedDetails>(details)->
-            all_history) {
-      DeleteAllShortcuts();
-    }
-    const URLRows& rows(
-        content::Details<const history::URLsDeletedDetails>(details)->rows);
-    std::vector<std::string> shortcut_ids;
+  DCHECK_EQ(chrome::NOTIFICATION_HISTORY_URLS_DELETED, type);
+  if (content::Details<const history::URLsDeletedDetails>(details)->

[Bart N., 2013/10/14 21:31:54]

Style: -> should be on the second line.

[Peter Kasting, 2013/10/16 01:28:55]

Operators go on ends of lines, not starts.  But I solved this a different way by
factoring out a temp.

[Bart N., 2013/10/16 17:42:47]

Interesting, unlike the internal Google style.

+          all_history) {
+    DeleteAllShortcuts();
+  }
+  const URLRows& rows(
+      content::Details<const history::URLsDeletedDetails>(details)->rows);
+  std::vector<std::string> shortcut_ids;
 
-    for (GuidMap::const_iterator it(guid_map_.begin()); it != guid_map_.end();
-         ++it) {
-      if (std::find_if(
-          rows.begin(), rows.end(), URLRow::URLRowHasURL(
-              it->second->second.match_core.destination_url)) != rows.end())
-        shortcut_ids.push_back(it->first);
-    }
-    DeleteShortcutsWithIds(shortcut_ids);
-    return;
+  for (GuidMap::const_iterator it(guid_map_.begin()); it != guid_map_.end();
+        ++it) {
+    if (std::find_if(
+        rows.begin(), rows.end(), URLRow::URLRowHasURL(
+            it->second->second.match_core.destination_url)) != rows.end())
+      shortcut_ids.push_back(it->first);
   }
-
-  DCHECK(type == chrome::NOTIFICATION_OMNIBOX_OPENED_URL);
-
-  OmniboxLog* log = content::Details<OmniboxLog>(details).ptr();
-  string16 text_lowercase(base::i18n::ToLower(log->text));
-
-  const AutocompleteMatch& match(log->result.match_at(log->selected_index));
-  for (ShortcutMap::const_iterator it(
-       shortcuts_map_.lower_bound(text_lowercase));
-       it != shortcuts_map_.end() &&
-           StartsWith(it->first, text_lowercase, true); ++it) {
-    if (match.destination_url == it->second.match_core.destination_url) {
-      UpdateShortcut(Shortcut(it->second.id, log->text,
-                              Shortcut::MatchCore(match), base::Time::Now(),
-                              it->second.number_of_hits + 1));
-      return;
-    }
-  }
-  AddShortcut(Shortcut(base::GenerateGUID(), log->text,
-                       Shortcut::MatchCore(match), base::Time::Now(), 1));
+  DeleteShortcutsWithIds(shortcut_ids);
 }
 
 void ShortcutsBackend::InitInternal() {
   DCHECK(current_state_ == INITIALIZING);
   db_->Init();
   ShortcutsDatabase::GuidToShortcutMap shortcuts;
   db_->LoadShortcuts(&shortcuts);
   temp_shortcuts_map_.reset(new ShortcutMap);
   temp_guid_map_.reset(new GuidMap);
   for (ShortcutsDatabase::GuidToShortcutMap::const_iterator it(
@@ skipping 93 matching lines @@
   shortcuts_map_.clear();
   guid_map_.clear();
   FOR_EACH_OBSERVER(ShortcutsBackendObserver, observer_list_,
                     OnShortcutsChanged());
   return no_db_access_ || BrowserThread::PostTask(BrowserThread::DB, FROM_HERE,
       base::Bind(base::IgnoreResult(&ShortcutsDatabase::DeleteAllShortcuts),
                  db_.get()));
 }
 
 }  // namespace history