bookmarks: Get rid of the dependency on history_notifications.h

In order to do this we moved the FaviconChangeDetails struct into c/b/favicon,
which bookmarks is already allowed to depend on, as well is history.

BUG=144783
R=sky@chromium.org

Review URL: https://chromiumcodereview.appspot.com/15275004

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

chrome/browser/bookmarks/DEPS

@@ -10,7 +10,6 @@ include_rules = [
   # TODO(kaiwang): Bring this list to zero.
   # Do not add to the list of temporarily-allowed dependencies below,
   # and please do not introduce more #includes of these files.
-  "!chrome/browser/history/history_notifications.h",
   "!chrome/browser/history/history_service.h",
   "!chrome/browser/history/history_service_factory.h",
   "!chrome/browser/history/query_parser.h",

chrome/browser/bookmarks/bookmark_model.cc

@@ -21,10 +21,10 @@
 #include "chrome/browser/bookmarks/bookmark_model_observer.h"
 #include "chrome/browser/bookmarks/bookmark_storage.h"
 #include "chrome/browser/bookmarks/bookmark_utils.h"
+#include "chrome/browser/favicon/favicon_changed_details.h"
 #include "chrome/browser/favicon/favicon_service.h"
 #include "chrome/browser/favicon/favicon_service_factory.h"
 #include "chrome/browser/favicon/favicon_types.h"
-#include "chrome/browser/history/history_notifications.h"
 #include "chrome/browser/history/history_service.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
@@ -954,7 +954,7 @@ void BookmarkModel::Observe(int type,
   switch (type) {
     case chrome::NOTIFICATION_FAVICON_CHANGED: {
       // Prevent the observers from getting confused for multiple favicon loads.
-      content::Details<history::FaviconChangeDetails> favicon_details(details);
+      content::Details<FaviconChangedDetails> favicon_details(details);
       for (std::set<GURL>::const_iterator i = favicon_details->urls.begin();
            i != favicon_details->urls.end(); ++i) {
         std::vector<const BookmarkNode*> nodes;

chrome/browser/bookmarks/bookmark_model_unittest.cc

@@ -22,9 +22,6 @@
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/bookmarks/bookmark_model_observer.h"
 #include "chrome/browser/bookmarks/bookmark_utils.h"
-#include "chrome/browser/history/history_notifications.h"
-#include "chrome/browser/history/history_service.h"
-#include "chrome/browser/history/history_service_factory.h"
 #include "chrome/test/base/model_test_utils.h"
 #include "chrome/test/base/testing_profile.h"
 #include "chrome/test/base/ui_test_utils.h"
@@ -950,30 +947,6 @@ TEST_F(BookmarkModelTestWithProfile, CreateAndRestore) {
   }
 }
 
-// Simple test that removes a bookmark. This test exercises the code paths in
-// History that block till bookmark bar model is loaded.
-TEST_F(BookmarkModelTestWithProfile, RemoveNotification) {
-  profile_.reset(new TestingProfile());
-
-  profile_->CreateHistoryService(false, false);
-  profile_->CreateBookmarkModel(true);
-  BlockTillBookmarkModelLoaded();
-
-  // Add a URL.
-  GURL url("http://www.google.com");
-  bookmark_utils::AddIfNotBookmarked(bb_model_, url, string16());
-
-  HistoryServiceFactory::GetForProfile(
-      profile_.get(), Profile::EXPLICIT_ACCESS)->AddPage(
-          url, base::Time::Now(), NULL, 1, GURL(), history::RedirectList(),
-          content::PAGE_TRANSITION_TYPED, history::SOURCE_BROWSED, false);
-
-  // This won't actually delete the URL, rather it'll empty out the visits.
-  // This triggers blocking on the BookmarkModel.
-  HistoryServiceFactory::GetForProfile(
-      profile_.get(), Profile::EXPLICIT_ACCESS)->DeleteURL(url);
-}
-
 TEST_F(BookmarkModelTest, Sort) {
   // Populate the bookmark bar node with nodes for 'B', 'a', 'd' and 'C'.
   // 'C' and 'a' are folders.

chrome/browser/favicon/DEPS

@@ -18,6 +18,7 @@
   # and please do not introduce more #includes of these files.
   "!chrome/browser/bookmarks/bookmark_service.h",
   "!chrome/browser/history/history_backend.h",
+  "!chrome/browser/history/history_details.h",
   "!chrome/browser/history/history_service.h",
   "!chrome/browser/history/history_service_factory.h",
   "!chrome/browser/history/select_favicon_frames.h",

chrome/browser/favicon/favicon_changed_details.cc

+// Copyright 2013 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/favicon/favicon_changed_details.h"
+
+FaviconChangedDetails::FaviconChangedDetails() {}
+
+FaviconChangedDetails::~FaviconChangedDetails() {}

chrome/browser/favicon/favicon_changed_details.h

+// Copyright 2013 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.
+
+#ifndef CHROME_BROWSER_FAVICON_FAVICON_CHANGED_DETAILS_H_
+#define CHROME_BROWSER_FAVICON_FAVICON_CHANGED_DETAILS_H_
+
+#include <set>
+
+#include "chrome/browser/history/history_details.h"
+#include "googleurl/src/gurl.h"
+
+// Details for chrome::NOTIFICATION_FAVICON_CHANGED.
+struct FaviconChangedDetails : public history::HistoryDetails {
+  FaviconChangedDetails();
+  virtual ~FaviconChangedDetails();
+
+  std::set<GURL> urls;
+};
+
+#endif  // CHROME_BROWSER_FAVICON_FAVICON_CHANGED_DETAILS_H_

chrome/browser/history/android/android_provider_backend.cc

@@ -6,6 +6,7 @@
 
 #include "base/i18n/case_conversion.h"
 #include "chrome/browser/bookmarks/bookmark_service.h"
+#include "chrome/browser/favicon/favicon_changed_details.h"
 #include "chrome/browser/history/android/android_time.h"
 #include "chrome/browser/history/android/android_urls_sql_handler.h"
 #include "chrome/browser/history/android/bookmark_model_sql_handler.h"
@@ -334,7 +335,7 @@ bool AndroidProviderBackend::UpdateHistoryAndBookmarks(
   *updated_count = ids_set.size();
 
   scoped_ptr<URLsModifiedDetails> modified(new URLsModifiedDetails);
-  scoped_ptr<FaviconChangeDetails> favicon(new FaviconChangeDetails);
+  scoped_ptr<FaviconChangedDetails> favicon(new FaviconChangedDetails);
 
   for (TableIDRows::const_iterator i = ids_set.begin(); i != ids_set.end();
        ++i) {
@@ -390,11 +391,11 @@ AndroidURLID AndroidProviderBackend::InsertHistoryAndBookmark(
     return false;
   modified->changed_urls.push_back(url_row);
 
-  scoped_ptr<FaviconChangeDetails> favicon;
+  scoped_ptr<FaviconChangedDetails> favicon;
   // No favicon should be changed if the thumbnail_db_ is not available.
   if (row.is_value_set_explicitly(HistoryAndBookmarkRow::FAVICON) &&
       row.favicon_valid() && thumbnail_db_) {
-    favicon.reset(new FaviconChangeDetails);
+    favicon.reset(new FaviconChangedDetails);
     if (!favicon.get())
       return false;
     favicon->urls.insert(url_row.url());
@@ -975,7 +976,7 @@ bool AndroidProviderBackend::SimulateUpdateURL(
   new_row.set_title(statement->statement()->ColumnString16(3));
 
   scoped_ptr<URLsDeletedDetails> deleted_details(new URLsDeletedDetails);
-  scoped_ptr<FaviconChangeDetails> favicon_details(new FaviconChangeDetails);
+  scoped_ptr<FaviconChangedDetails> favicon_details(new FaviconChangedDetails);
   scoped_ptr<URLsModifiedDetails> modified(new URLsModifiedDetails);
   URLRow old_url_row;
   if (!history_db_->GetURLRow(ids[0].url_id, &old_url_row))
@@ -1104,7 +1105,7 @@ bool AndroidProviderBackend::DeleteHistoryInternal(
     bool delete_bookmarks,
     HistoryNotifications* notifications) {
   scoped_ptr<URLsDeletedDetails> deleted_details(new URLsDeletedDetails);
-  scoped_ptr<FaviconChangeDetails> favicon(new FaviconChangeDetails);
+  scoped_ptr<FaviconChangedDetails> favicon(new FaviconChangedDetails);
   for (TableIDRows::const_iterator i = urls.begin(); i != urls.end(); ++i) {
     URLRow url_row;
     if (!history_db_->GetURLRow(i->url_id, &url_row))

chrome/browser/history/android/android_provider_backend_unittest.cc

@@ -15,6 +15,7 @@
 #include "chrome/browser/bookmarks/bookmark_model.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/bookmarks/bookmark_service.h"
+#include "chrome/browser/favicon/favicon_changed_details.h"
 #include "chrome/browser/history/android/android_time.h"
 #include "chrome/browser/history/history_backend.h"
 #include "chrome/browser/profiles/profile_manager.h"
@@ -67,7 +68,7 @@ class AndroidProviderBackendDelegate : public HistoryBackend::Delegate {
         deleted_details_.reset(static_cast<URLsDeletedDetails*>(details));
         break;
       case chrome::NOTIFICATION_FAVICON_CHANGED:
-        favicon_details_.reset(static_cast<FaviconChangeDetails*>(details));
+        favicon_details_.reset(static_cast<FaviconChangedDetails*>(details));
         break;
       case chrome::NOTIFICATION_HISTORY_URLS_MODIFIED:
         modified_details_.reset(static_cast<URLsModifiedDetails*>(details));
@@ -87,7 +88,7 @@ class AndroidProviderBackendDelegate : public HistoryBackend::Delegate {
     return modified_details_.get();
   }
 
-  FaviconChangeDetails* favicon_details() const {
+  FaviconChangedDetails* favicon_details() const {
     return favicon_details_.get();
   }
 
@@ -100,7 +101,7 @@ class AndroidProviderBackendDelegate : public HistoryBackend::Delegate {
  private:
   scoped_ptr<URLsDeletedDetails> deleted_details_;
   scoped_ptr<URLsModifiedDetails> modified_details_;
-  scoped_ptr<FaviconChangeDetails> favicon_details_;
+  scoped_ptr<FaviconChangedDetails> favicon_details_;
 
   DISALLOW_COPY_AND_ASSIGN(AndroidProviderBackendDelegate);
 };

chrome/browser/history/history_backend.cc

@@ -25,6 +25,7 @@
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/autocomplete/history_url_provider.h"
 #include "chrome/browser/bookmarks/bookmark_service.h"
+#include "chrome/browser/favicon/favicon_changed_details.h"
 #include "chrome/browser/favicon/imported_favicon_usage.h"
 #include "chrome/browser/history/download_row.h"
 #include "chrome/browser/history/history_db_task.h"
@@ -2237,7 +2238,7 @@ void HistoryBackend::SetImportedFavicons(
 
   if (!favicons_changed.empty()) {
     // Send the notification about the changed favicon URLs.
-    FaviconChangeDetails* changed_details = new FaviconChangeDetails;
+    FaviconChangedDetails* changed_details = new FaviconChangedDetails;
     changed_details->urls.swap(favicons_changed);
     BroadcastNotifications(chrome::NOTIFICATION_FAVICON_CHANGED,
                            changed_details);
@@ -2604,7 +2605,7 @@ void HistoryBackend::SendFaviconChangedNotificationForPageAndRedirects(
   history::RedirectList redirect_list;
   GetCachedRecentRedirects(page_url, &redirect_list);
 
-  FaviconChangeDetails* changed_details = new FaviconChangeDetails;
+  FaviconChangedDetails* changed_details = new FaviconChangedDetails;
   for (size_t i = 0; i < redirect_list.size(); ++i)
     changed_details->urls.insert(redirect_list[i]);
 

chrome/browser/history/history_backend_unittest.cc

@@ -19,10 +19,13 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
+#include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/bookmarks/bookmark_utils.h"
 #include "chrome/browser/favicon/imported_favicon_usage.h"
 #include "chrome/browser/history/history_backend.h"
 #include "chrome/browser/history/history_notifications.h"
+#include "chrome/browser/history/history_service.h"
+#include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/history/in_memory_database.h"
 #include "chrome/browser/history/in_memory_history_backend.h"
 #include "chrome/browser/history/visit_filter.h"
@@ -30,9 +33,12 @@
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/thumbnail_score.h"
+#include "chrome/test/base/testing_profile.h"
+#include "chrome/test/base/ui_test_utils.h"
 #include "chrome/tools/profiles/thumbnail-inl.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_source.h"
+#include "content/public/test/test_browser_thread.h"
 #include "googleurl/src/gurl.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
@@ -118,7 +124,8 @@ class HistoryBackendTest : public testing::Test {
   HistoryBackendTest()
       : bookmark_model_(NULL),
         loaded_(false),
-        num_broadcasted_notifications_(0) {
+        num_broadcasted_notifications_(0),
+        ui_thread_(content::BrowserThread::UI, &message_loop_) {
   }
 
   virtual ~HistoryBackendTest() {
@@ -410,6 +417,7 @@ class HistoryBackendTest : public testing::Test {
   base::FilePath test_dir_;
   history::MostVisitedURLList most_visited_list_;
   history::FilteredURLList filtered_list_;
+  content::TestBrowserThread ui_thread_;
 };
 
 void HistoryBackendTestDelegate::SetInMemoryBackend(int backend_id,
@@ -2781,4 +2789,30 @@ TEST_F(HistoryBackendSegmentDurationTest, SegmentDuration) {
   EXPECT_EQ(segment1_time_delta.InHours(), data[1]->duration().InHours());
 }
 
+// Simple test that removes a bookmark. This test exercises the code paths in
+// History that block till bookmark bar model is loaded.
+TEST_F(HistoryBackendTest, RemoveNotification) {
+  scoped_ptr<TestingProfile> profile(new TestingProfile());
+
+  profile->CreateHistoryService(false, false);
+  profile->CreateBookmarkModel(true);
+  BookmarkModel* model = BookmarkModelFactory::GetForProfile(profile.get());
+  ui_test_utils::WaitForBookmarkModelToLoad(model);
+
+  // Add a URL.
+  GURL url("http://www.google.com");
+  bookmark_utils::AddIfNotBookmarked(model, url, base::string16());
+
+  HistoryService* service = HistoryServiceFactory::GetForProfile(
+      profile.get(), Profile::EXPLICIT_ACCESS);
+
+  service->AddPage(
+      url, base::Time::Now(), NULL, 1, GURL(), RedirectList(),
+      content::PAGE_TRANSITION_TYPED, SOURCE_BROWSED, false);
+
+  // This won't actually delete the URL, rather it'll empty out the visits.
+  // This triggers blocking on the BookmarkModel.
+  service->DeleteURL(url);
+}
+
 }  // namespace history

chrome/browser/history/history_details.h

+// Copyright 2013 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.
+
+#ifndef CHROME_BROWSER_HISTORY_HISTORY_DETAILS_H_
+#define CHROME_BROWSER_HISTORY_HISTORY_DETAILS_H_
+
+namespace history {
+
+// Base class for history notifications. This needs only a virtual destructor
+// so that the history service's broadcaster can delete it when the request
+// is complete.
+struct HistoryDetails {
+ public:
+  virtual ~HistoryDetails() {}
+};
+
+}  // namespace history
+
+#endif  // CHROME_BROWSER_HISTORY_HISTORY_DETAILS_H_

chrome/browser/history/history_notifications.cc

@@ -22,10 +22,6 @@ URLsDeletedDetails::URLsDeletedDetails()
 
 URLsDeletedDetails::~URLsDeletedDetails() {}
 
-FaviconChangeDetails::FaviconChangeDetails() {}
-
-FaviconChangeDetails::~FaviconChangeDetails() {}
-
 KeywordSearchTermDetails::KeywordSearchTermDetails() : keyword_id(0) {}
 
 KeywordSearchTermDetails::~KeywordSearchTermDetails() {}

chrome/browser/history/history_notifications.h

@@ -8,22 +8,14 @@
 #define CHROME_BROWSER_HISTORY_HISTORY_NOTIFICATIONS_H__
 
 #include <set>
-#include <vector>
 
-#include "googleurl/src/gurl.h"
+#include "chrome/browser/history/history_details.h"
 #include "chrome/browser/history/history_types.h"
 #include "chrome/browser/search_engines/template_url_id.h"
+#include "googleurl/src/gurl.h"
 
 namespace history {
 
-// Base class for history notifications. This needs only a virtual destructor
-// so that the history service's broadcaster can delete it when the request
-// is complete.
-struct HistoryDetails {
- public:
-  virtual ~HistoryDetails() {}
-};
-
 // Details for NOTIFICATION_HISTORY_URL_VISITED.
 struct URLVisitedDetails : public HistoryDetails {
   URLVisitedDetails();
@@ -69,14 +61,6 @@ struct URLsDeletedDetails : public HistoryDetails {
   std::set<GURL> favicon_urls;
 };
 
-// Details for NOTIFY_FAVICON_CHANGED.
-struct FaviconChangeDetails : public HistoryDetails {
-  FaviconChangeDetails();
-  virtual ~FaviconChangeDetails();
-
-  std::set<GURL> urls;
-};
-
 // Details for HISTORY_KEYWORD_SEARCH_TERM_UPDATED.
 struct KeywordSearchTermDetails : public HistoryDetails {
   KeywordSearchTermDetails();

chrome/browser/sync/glue/session_change_processor.cc

@@ -9,7 +9,7 @@
 
 #include "base/logging.h"
 #include "chrome/browser/extensions/tab_helper.h"
-#include "chrome/browser/history/history_notifications.h"
+#include "chrome/browser/favicon/favicon_changed_details.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/glue/session_model_associator.h"
 #include "chrome/browser/sync/profile_sync_service.h"
@@ -90,7 +90,7 @@ void SessionChangeProcessor::Observe(
   std::vector<SyncedTabDelegate*> modified_tabs;
   switch (type) {
     case chrome::NOTIFICATION_FAVICON_CHANGED: {
-      content::Details<history::FaviconChangeDetails> favicon_details(details);
+      content::Details<FaviconChangedDetails> favicon_details(details);
       session_model_associator_->FaviconsUpdated(favicon_details->urls);
       // Note: we favicon notifications don't affect tab contents, so we return
       // here instead of continuing on to reassociate tabs/windows.

chrome/chrome_browser.gypi

@@ -526,6 +526,8 @@
         'browser/external_protocol/external_protocol_observer.cc',
         'browser/external_protocol/external_protocol_observer.h',
         'browser/external_tab/external_tab_container.h',
+        'browser/favicon/favicon_changed_details.cc',
+        'browser/favicon/favicon_changed_details.h',
         'browser/favicon/favicon_handler.cc',
         'browser/favicon/favicon_handler.h',
         'browser/favicon/favicon_handler_delegate.h',
@@ -716,6 +718,7 @@
         'browser/history/history_database.cc',
         'browser/history/history_database.h',
         'browser/history/history_db_task.h',
+        'browser/history/history_details.h',
         'browser/history/history_marshaling.h',
         'browser/history/history_marshaling_android.h',
         'browser/history/history_notifications.cc',

chrome/common/chrome_notification_types.h

@@ -309,8 +309,8 @@ enum NotificationType {
   NOTIFICATION_HISTORY_KEYWORD_SEARCH_TERM_UPDATED,
 
   // Sent by history when the favicon of a URL changes.  The source is the
-  // profile, and the details is history::FaviconChangeDetails (see
-  // history_notifications.h).
+  // profile, and the details is FaviconChangedDetails (see
+  // chrome/browser/favicon/favicon_changed_details.h).
   NOTIFICATION_FAVICON_CHANGED,
 
   // Sent by FaviconTabHelper when a tab's favicon has been successfully