Fixes and simplification to ShortcutsBackend including addition of "DeleteAll"

TEST=unit-tests
BUG=none
Review URL: http://codereview.chromium.org/7821011

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@101611 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 102 insertions, 103 deletions

diff --git a/chrome/browser/autocomplete/shortcuts_provider.cc b/chrome/browser/autocomplete/shortcuts_provider.cc
index 1b29dac..4ef6ae9 100644
--- a/chrome/browser/autocomplete/shortcuts_provider.cc
+++ b/chrome/browser/autocomplete/shortcuts_provider.cc
@@ -61,11 +61,12 @@ ShortcutsProvider::ShortcutsProvider(ACProviderListener* listener,
                                      Profile* profile)
     : AutocompleteProvider(listener, profile, "ShortcutsProvider"),
       languages_(profile_->GetPrefs()->GetString(prefs::kAcceptLanguages)),
+      initialized_(false),
       shortcuts_backend_(profile->GetShortcutsBackend()) {
   if (shortcuts_backend_.get()) {
     shortcuts_backend_->AddObserver(this);
     if (shortcuts_backend_->initialized())
-      LoadShortcuts();
+      initialized_ = true;
   }
 }
 
@@ -81,6 +82,9 @@ void ShortcutsProvider::Start(const AutocompleteInput& input,
   if (input.type() == AutocompleteInput::INVALID)
     return;
 
+  if (!initialized_)
+    return;
+
   base::TimeTicks start_time = base::TimeTicks::Now();
   GetMatches(input);
   if (input.text().length() < 6) {
@@ -114,29 +118,7 @@ void ShortcutsProvider::DeleteMatch(const AutocompleteMatch& match) {
 }
 
 void ShortcutsProvider::OnShortcutsLoaded() {
-  LoadShortcuts();
-}
-
-void ShortcutsProvider::OnShortcutAddedOrUpdated(const Shortcut& shortcut) {
-  shortcuts_provider::GuidToShortcutsIteratorMap::iterator it =
-      guid_map_.find(shortcut.id);
-  if (it != guid_map_.end()) {
-    shortcuts_map_.erase(it->second);
-  }
-  guid_map_[shortcut.id] = shortcuts_map_.insert(
-      std::make_pair(base::i18n::ToLower(shortcut.text), shortcut));
-}
-
-void ShortcutsProvider::OnShortcutsRemoved(
-    const std::vector<std::string>& shortcuts_ids) {
-  for (size_t i = 0; i < shortcuts_ids.size(); ++i) {
-    shortcuts_provider::GuidToShortcutsIteratorMap::iterator it =
-        guid_map_.find(shortcuts_ids[i]);
-    if (it != guid_map_.end()) {
-      shortcuts_map_.erase(it->second);
-      guid_map_.erase(it);
-    }
-  }
+  initialized_ = true;
 }
 
 void ShortcutsProvider::DeleteMatchesWithURLs(const std::set<GURL>& urls) {
@@ -145,13 +127,6 @@ void ShortcutsProvider::DeleteMatchesWithURLs(const std::set<GURL>& urls) {
 }
 
 void ShortcutsProvider::DeleteShortcutsWithURLs(const std::set<GURL>& urls) {
-  for (ShortcutMap::iterator it = shortcuts_map_.begin();
-       it != shortcuts_map_.end();) {
-    if (urls.find(it->second.url) != urls.end())
-      shortcuts_map_.erase(it++);
-    else
-      ++it;
-  }
   if (!shortcuts_backend_.get())
     return;  // We are off the record.
   for (std::set<GURL>::const_iterator url = urls.begin(); url != urls.end();
@@ -165,8 +140,8 @@ void ShortcutsProvider::GetMatches(const AutocompleteInput& input) {
   string16 term_string(base::i18n::ToLower(input.text()));
   DCHECK(!term_string.empty());
 
-  for (ShortcutMap::iterator it = FindFirstMatch(term_string);
-       it != shortcuts_map_.end() &&
+  for (ShortcutMap::const_iterator it = FindFirstMatch(term_string);
+       it != shortcuts_backend_->shortcuts_map().end() &&
             StartsWith(it->first, term_string, true); ++it)
     matches_.push_back(ShortcutToACMatch(input, term_string, it));
   std::partial_sort(matches_.begin(),
@@ -182,7 +157,7 @@ void ShortcutsProvider::GetMatches(const AutocompleteInput& input) {
 AutocompleteMatch ShortcutsProvider::ShortcutToACMatch(
     const AutocompleteInput& input,
     const string16& term_string,
-    ShortcutMap::iterator it) {
+    ShortcutMap::const_iterator it) {
   AutocompleteMatch match(this, CalculateScore(term_string, it->second),
                           true, AutocompleteMatch::HISTORY_TITLE);
   match.destination_url = it->second.url;
@@ -280,13 +255,15 @@ ACMatchClassifications ShortcutsProvider::ClassifyAllMatchesInString(
   return output;
 }
 
-ShortcutMap::iterator ShortcutsProvider::FindFirstMatch(
+ShortcutMap::const_iterator ShortcutsProvider::FindFirstMatch(
     const string16& keyword) {
-  ShortcutMap::iterator it = shortcuts_map_.lower_bound(keyword);
+  ShortcutMap::const_iterator it =
+      shortcuts_backend_->shortcuts_map().lower_bound(keyword);
   // Lower bound not necessarily matches the keyword, check for item pointed by
   // the lower bound iterator to at least start with keyword.
-  return ((it == shortcuts_map_.end()) ||
-    StartsWith(it->first, keyword, true)) ? it : shortcuts_map_.end();
+  return ((it == shortcuts_backend_->shortcuts_map().end()) ||
+    StartsWith(it->first, keyword, true)) ? it :
+    shortcuts_backend_->shortcuts_map().end();
 }
 
 // static
@@ -320,11 +297,11 @@ int ShortcutsProvider::CalculateScore(const string16& terms,
       0.5);
 }
 
-void ShortcutsProvider::LoadShortcuts() {
-  DCHECK(shortcuts_backend_.get());
-  guid_map_.clear();
-  CHECK(shortcuts_backend_->GetShortcuts(&shortcuts_map_));
-  for (shortcuts_provider::ShortcutMap::iterator it = shortcuts_map_.begin();
-       it != shortcuts_map_.end(); ++it)
-    guid_map_[it->second.id] = it;
+void ShortcutsProvider::set_shortcuts_backend(
+    history::ShortcutsBackend* shortcuts_backend) {
+  DCHECK(shortcuts_backend);
+  shortcuts_backend_ = shortcuts_backend;
+  shortcuts_backend_->AddObserver(this);
+  if (shortcuts_backend_->initialized())
+    initialized_ = true;
 }
diff --git a/chrome/browser/autocomplete/shortcuts_provider.h b/chrome/browser/autocomplete/shortcuts_provider.h
index 38161d4..d41a70b 100644
--- a/chrome/browser/autocomplete/shortcuts_provider.h
+++ b/chrome/browser/autocomplete/shortcuts_provider.h
@@ -51,10 +51,6 @@ class ShortcutsProvider
 
   // ShortcutsBackendObserver:
   virtual void OnShortcutsLoaded() OVERRIDE;
-  virtual void OnShortcutAddedOrUpdated(
-      const shortcuts_provider::Shortcut& shortcut) OVERRIDE;
-  virtual void OnShortcutsRemoved(
-      const std::vector<std::string>& shortcut_ids) OVERRIDE;
 
   void DeleteMatchesWithURLs(const std::set<GURL>& urls);
   void DeleteShortcutsWithURLs(const std::set<GURL>& urls);
@@ -65,7 +61,7 @@ class ShortcutsProvider
   AutocompleteMatch ShortcutToACMatch(
       const AutocompleteInput& input,
       const string16& terms,
-      shortcuts_provider::ShortcutMap::iterator it);
+      shortcuts_provider::ShortcutMap::const_iterator it);
 
   // Given |text| and a corresponding base set of classifications
   // |original_class|, adds ACMatchClassification::MATCH markers for all
@@ -85,25 +81,16 @@ class ShortcutsProvider
 
   // Returns iterator to first item in |shortcuts_map_| matching |keyword|.
   // Returns shortcuts_map_.end() if there are no matches.
-  shortcuts_provider::ShortcutMap::iterator FindFirstMatch(
+  shortcuts_provider::ShortcutMap::const_iterator FindFirstMatch(
       const string16& keyword);
 
   static int CalculateScore(const string16& terms,
                             const shortcuts_provider::Shortcut& shortcut);
-
-  // Loads shortcuts from the backend.
-  void LoadShortcuts();
+  // For unit-test only.
+  void set_shortcuts_backend(history::ShortcutsBackend* shortcuts_backend);
 
   std::string languages_;
-
-  // The following two maps are duplicated from the ShortcutsBackend. If any
-  // of the copies of ShortcutProvider makes a change and calls ShortcutBackend
-  // the change will be committed to storage and to the back-end's copy on the
-  // DB thread and propagated back to all copies of provider, so eventually they
-  // all are going to be in sync.
-  shortcuts_provider::ShortcutMap shortcuts_map_;
-  // This is a helper map for quick access to a shortcut by guid.
-  shortcuts_provider::GuidToShortcutsIteratorMap guid_map_;
+  bool initialized_;
 
   scoped_refptr<history::ShortcutsBackend> shortcuts_backend_;
 };
diff --git a/chrome/browser/autocomplete/shortcuts_provider_unittest.cc b/chrome/browser/autocomplete/shortcuts_provider_unittest.cc
index 534c53d..7b98206 100644
--- a/chrome/browser/autocomplete/shortcuts_provider_unittest.cc
+++ b/chrome/browser/autocomplete/shortcuts_provider_unittest.cc
@@ -18,6 +18,7 @@
 #include "chrome/browser/autocomplete/autocomplete_match.h"
 #include "chrome/browser/history/history.h"
 #include "chrome/browser/history/in_memory_url_index.h"
+#include "chrome/browser/history/shortcuts_backend.h"
 #include "chrome/browser/history/url_database.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/common/pref_names.h"
@@ -31,6 +32,7 @@ using base::TimeDelta;
 namespace {
 
 struct TestShortcutInfo {
+  std::string guid;
   std::string url;
   std::string title;  // The text that orginally was searched for.
   std::string contents;
@@ -40,62 +42,79 @@ struct TestShortcutInfo {
   int typed_count;
   int days_from_now;
 } shortcut_test_db[] = {
-  { "http://www.google.com/", "goog",
+  { "BD85DBA2-8C29-49F9-84AE-48E1E90880E0",
+    "http://www.google.com/", "goog",
     "Google", "0,1,4,0", "Google", "0,3,4,1", 100, 1 },
-  { "http://slashdot.org/", "slash",
+  { "BD85DBA2-8C29-49F9-84AE-48E1E90880E1",
+    "http://slashdot.org/", "slash",
     "slashdot.org", "0,3,5,1",
     "Slashdot - News for nerds, stuff that matters", "0,2,5,0", 100, 0},
-  { "http://slashdot.org/", "news",
+  { "BD85DBA2-8C29-49F9-84AE-48E1E90880E2",
+    "http://slashdot.org/", "news",
     "slashdot.org", "0,1",
     "Slashdot - News for nerds, stuff that matters", "0,0,11,2,15,0", 5, 0},
-  { "http://sports.yahoo.com/", "news",
+  { "BD85DBA2-8C29-49F9-84AE-48E1E90880E3",
+    "http://sports.yahoo.com/", "news",
     "sports.yahoo.com", "0,1",
     "Yahoo! Sports - Sports News, Scores, Rumors, Fantasy Games, and more",
     "0,0,23,2,27,0", 5, 2},
-  { "http://www.cnn.com/index.html", "news weather",
+  { "BD85DBA2-8C29-49F9-84AE-48E1E90880E4",
+    "http://www.cnn.com/index.html", "news weather",
     "www.cnn.com/index.html", "0,1",
     "CNN.com - Breaking News, U.S., World, Weather, Entertainment & Video",
     "0,0,19,2,23,0,38,2,45,0", 10, 1},
-  { "http://sports.yahoo.com/", "nhl scores",
+  { "BD85DBA2-8C29-49F9-84AE-48E1E90880E5",
+    "http://sports.yahoo.com/", "nhl scores",
     "sports.yahoo.com", "0,1",
     "Yahoo! Sports - Sports News, Scores, Rumors, Fantasy Games, and more",
     "0,0,29,2,35,0", 10, 1},
-  { "http://www.nhl.com/scores/index.html", "nhl scores",
+  { "BD85DBA2-8C29-49F9-84AE-48E1E90880E6",
+    "http://www.nhl.com/scores/index.html", "nhl scores",
     "www.nhl.com/scores/index.html", "0,1,4,3,7,1",
     "January 13, 2010 - NHL.com - Scores", "0,0,19,2,22,0,29,2,35,0", 1, 5},
-  { "http://www.testsite.com/a.html", "just",
+  { "BD85DBA2-8C29-49F9-84AE-48E1E90880E7",
+    "http://www.testsite.com/a.html", "just",
     "www.testsite.com/a.html", "0,1",
     "Test - site - just a test", "0,0,14,2,18,0", 1, 5},
-  { "http://www.testsite.com/b.html", "just",
+  { "BD85DBA2-8C29-49F9-84AE-48E1E90880E8",
+    "http://www.testsite.com/b.html", "just",
     "www.testsite.com/b.html", "0,1",
     "Test - site - just a test", "0,0,14,2,18,0", 2, 5},
-  { "http://www.testsite.com/c.html", "just",
+  { "BD85DBA2-8C29-49F9-84AE-48E1E90880E9",
+    "http://www.testsite.com/c.html", "just",
     "www.testsite.com/c.html", "0,1",
     "Test - site - just a test", "0,0,14,2,18,0", 1, 8},
-  { "http://www.testsite.com/d.html", "just a",
+  { "BD85DBA2-8C29-49F9-84AE-48E1E90880EA",
+    "http://www.testsite.com/d.html", "just a",
     "www.testsite.com/d.html", "0,1",
     "Test - site - just a test", "0,0,14,2,18,0", 1, 12},
-  { "http://www.testsite.com/e.html", "just a t",
+  { "BD85DBA2-8C29-49F9-84AE-48E1E90880EB",
+    "http://www.testsite.com/e.html", "just a t",
     "www.testsite.com/e.html", "0,1",
     "Test - site - just a test", "0,0,14,2,18,0", 1, 12},
-  { "http://www.testsite.com/f.html", "just a te",
+  { "BD85DBA2-8C29-49F9-84AE-48E1E90880EC",
+    "http://www.testsite.com/f.html", "just a te",
     "www.testsite.com/f.html", "0,1",
     "Test - site - just a test", "0,0,14,2,18,0", 1, 12},
-  { "http://www.daysagotest.com/a.html", "ago",
+  { "BD85DBA2-8C29-49F9-84AE-48E1E90880ED",
+    "http://www.daysagotest.com/a.html", "ago",
     "www.daysagotest.com/a.html", "0,1,8,3,11,1",
     "Test - site", "0,0", 1, 1},
-  { "http://www.daysagotest.com/b.html", "ago",
+  { "BD85DBA2-8C29-49F9-84AE-48E1E90880EE",
+    "http://www.daysagotest.com/b.html", "ago",
     "www.daysagotest.com/b.html", "0,1,8,3,11,1",
     "Test - site", "0,0", 1, 2},
-  { "http://www.daysagotest.com/c.html", "ago",
+  { "BD85DBA2-8C29-49F9-84AE-48E1E90880EF",
+    "http://www.daysagotest.com/c.html", "ago",
     "www.daysagotest.com/c.html", "0,1,8,3,11,1",
     "Test - site", "0,0", 1, 3},
-  { "http://www.daysagotest.com/d.html", "ago",
+  { "BD85DBA2-8C29-49F9-84AE-48E1E90880F0",
+    "http://www.daysagotest.com/d.html", "ago",
     "www.daysagotest.com/d.html", "0,1,8,3,11,1",
     "Test - site", "0,0", 1, 4},
 };
 
-}  // end namespace
+}  // namespace
 
 class ShortcutsProviderTest : public testing::Test,
                               public ACProviderListener {
@@ -139,6 +158,7 @@ class ShortcutsProviderTest : public testing::Test,
 
   ACMatches ac_matches_;  // The resulting matches after running RunTest.
 
+  scoped_refptr<history::ShortcutsBackend> mock_backend_;
   scoped_refptr<ShortcutsProvider> provider_;
 };
 
@@ -154,6 +174,9 @@ void ShortcutsProviderTest::SetUp() {
   profile_.reset(new TestingProfile());
   profile_->CreateHistoryService(true, false);
   provider_ = new ShortcutsProvider(this, profile_.get());
+  mock_backend_ = new history::ShortcutsBackend(FilePath(), profile_.get());
+  mock_backend_->Init();
+  provider_->set_shortcuts_backend(mock_backend_.get());
   FillData();
 }
 
@@ -163,7 +186,7 @@ void ShortcutsProviderTest::TearDown() {
 
 void ShortcutsProviderTest::FillData() {
   DCHECK(provider_.get());
-  provider_->shortcuts_map_.clear();
+  mock_backend_->DeleteAllShortcuts();
   for (size_t i = 0; i < arraysize(shortcut_test_db); ++i) {
     const TestShortcutInfo& cur = shortcut_test_db[i];
     const GURL current_url(cur.url);
@@ -177,8 +200,8 @@ void ShortcutsProviderTest::FillData() {
         shortcuts_provider::SpansFromString(
             ASCIIToUTF16(cur.description_class)));
     shortcut.last_access_time = visit_time;
-    provider_->shortcuts_map_.insert(std::make_pair(ASCIIToUTF16(cur.title),
-                                                    shortcut));
+    shortcut.id = cur.guid;
+    mock_backend_->AddShortcut(shortcut);
   }
 }
 
@@ -536,18 +559,22 @@ TEST_F(ShortcutsProviderTest, CalculateScore) {
 
 TEST_F(ShortcutsProviderTest, DeleteMatch) {
   TestShortcutInfo shortcuts_to_test_delete[3] = {
-    { "http://www.deletetest.com/1.html", "delete",
+    { "BD85DBA2-8C29-49F9-84AE-48E1E90880F1",
+      "http://www.deletetest.com/1.html", "delete",
       "http://www.deletetest.com/1.html", "0,2",
       "Erase this shortcut!", "0,0", 1, 1},
-    { "http://www.deletetest.com/1.html", "erase",
+    { "BD85DBA2-8C29-49F9-84AE-48E1E90880F2",
+      "http://www.deletetest.com/1.html", "erase",
       "http://www.deletetest.com/1.html", "0,2",
       "Erase this shortcut!", "0,0", 1, 1},
-    { "http://www.deletetest.com/2.html", "delete",
+    { "BD85DBA2-8C29-49F9-84AE-48E1E90880F3",
+      "http://www.deletetest.com/2.html", "delete",
       "http://www.deletetest.com/2.html", "0,2",
       "Erase this shortcut!", "0,0", 1, 1},
   };
 
-  size_t original_shortcuts_count = provider_->shortcuts_map_.size();
+  size_t original_shortcuts_count =
+      provider_->shortcuts_backend_->shortcuts_map().size();
 
   for (size_t i = 0; i < arraysize(shortcuts_to_test_delete); ++i) {
     const TestShortcutInfo& cur = shortcuts_to_test_delete[i];
@@ -562,15 +589,18 @@ TEST_F(ShortcutsProviderTest, DeleteMatch) {
         shortcuts_provider::SpansFromString(
             ASCIIToUTF16(cur.description_class)));
     shortcut.last_access_time = visit_time;
-    provider_->shortcuts_map_.insert(std::make_pair(ASCIIToUTF16(cur.title),
-                                                    shortcut));
+    shortcut.id = cur.guid;
+    mock_backend_->AddShortcut(shortcut);
   }
 
-  EXPECT_EQ(original_shortcuts_count + 3, provider_->shortcuts_map_.size());
-  EXPECT_FALSE(provider_->shortcuts_map_.end() ==
-               provider_->shortcuts_map_.find(ASCIIToUTF16("delete")));
-  EXPECT_FALSE(provider_->shortcuts_map_.end() ==
-               provider_->shortcuts_map_.find(ASCIIToUTF16("erase")));
+  EXPECT_EQ(original_shortcuts_count + 3,
+            provider_->shortcuts_backend_->shortcuts_map().size());
+  EXPECT_FALSE(provider_->shortcuts_backend_->shortcuts_map().end() ==
+               provider_->shortcuts_backend_->shortcuts_map().find(
+                   ASCIIToUTF16("delete")));
+  EXPECT_FALSE(provider_->shortcuts_backend_->shortcuts_map().end() ==
+               provider_->shortcuts_backend_->shortcuts_map().find(
+                   ASCIIToUTF16("erase")));
 
   AutocompleteMatch match(provider_, 1200, true,
                           AutocompleteMatch::HISTORY_TITLE);
@@ -583,18 +613,23 @@ TEST_F(ShortcutsProviderTest, DeleteMatch) {
 
   // |shortcuts_to_test_delete[0]| and |shortcuts_to_test_delete[1]| should be
   // deleted, but not |shortcuts_to_test_delete[2]| as it has different url.
-  EXPECT_EQ(original_shortcuts_count + 1, provider_->shortcuts_map_.size());
-  EXPECT_FALSE(provider_->shortcuts_map_.end() ==
-               provider_->shortcuts_map_.find(ASCIIToUTF16("delete")));
-  EXPECT_TRUE(provider_->shortcuts_map_.end() ==
-              provider_->shortcuts_map_.find(ASCIIToUTF16("erase")));
+  EXPECT_EQ(original_shortcuts_count + 1,
+            provider_->shortcuts_backend_->shortcuts_map().size());
+  EXPECT_FALSE(provider_->shortcuts_backend_->shortcuts_map().end() ==
+               provider_->shortcuts_backend_->shortcuts_map().find(
+                   ASCIIToUTF16("delete")));
+  EXPECT_TRUE(provider_->shortcuts_backend_->shortcuts_map().end() ==
+              provider_->shortcuts_backend_->shortcuts_map().find(
+                  ASCIIToUTF16("erase")));
 
   match.destination_url = GURL(shortcuts_to_test_delete[2].url);
   match.contents = ASCIIToUTF16(shortcuts_to_test_delete[2].contents);
   match.description = ASCIIToUTF16(shortcuts_to_test_delete[2].description);
 
   provider_->DeleteMatch(match);
-  EXPECT_EQ(original_shortcuts_count, provider_->shortcuts_map_.size());
-  EXPECT_TRUE(provider_->shortcuts_map_.end() ==
-              provider_->shortcuts_map_.find(ASCIIToUTF16("delete")));
+  EXPECT_EQ(original_shortcuts_count,
+            provider_->shortcuts_backend_->shortcuts_map().size());
+  EXPECT_TRUE(provider_->shortcuts_backend_->shortcuts_map().end() ==
+              provider_->shortcuts_backend_->shortcuts_map().find(
+                  ASCIIToUTF16("delete")));
 }