Make the OmniboxNavigationObserver update the ShortcutsProvider.

chrome/browser/history/shortcuts_backend.cc

Index: chrome/browser/history/shortcuts_backend.cc
===================================================================
--- chrome/browser/history/shortcuts_backend.cc	(revision 228521)
+++ chrome/browser/history/shortcuts_backend.cc	(working copy)
@@ -160,8 +160,6 @@
                                 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));
   }
 }
 
@@ -191,6 +189,25 @@
   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() {
 }
 
@@ -214,45 +231,23 @@
     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;
-
-    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;
+  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;
 
-  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;
-    }
+  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);
   }
-  AddShortcut(Shortcut(base::GenerateGUID(), log->text,
-                       Shortcut::MatchCore(match), base::Time::Now(), 1));
+  DeleteShortcutsWithIds(shortcut_ids);
 }
 
 void ShortcutsBackend::InitInternal() {