Move most visited item state info from InstantController to InstantService.

chrome/browser/search/instant_service.cc

Index: chrome/browser/search/instant_service.cc
diff --git a/chrome/browser/search/instant_service.cc b/chrome/browser/search/instant_service.cc
index e6db319cb57adf2df49f10adc6d2c80b44557534..b4e98d73a8f8eb68e6ddbf5bdf166386775b4a5f 100644
--- a/chrome/browser/search/instant_service.cc
+++ b/chrome/browser/search/instant_service.cc
@@ -4,14 +4,24 @@
 
 #include "chrome/browser/search/instant_service.h"
 
+#include <vector>
+
+#include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
+#include "build/build_config.h"
 #include "chrome/browser/history/history_notifications.h"
+#include "chrome/browser/history/top_sites.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search/instant_io_context.h"
 #include "chrome/browser/search/instant_service_factory.h"
 #include "chrome/browser/search/local_ntp_source.h"
 #include "chrome/browser/search/most_visited_iframe_source.h"
 #include "chrome/browser/search/suggestion_iframe_source.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_instant_controller.h"
+#include "chrome/browser/ui/browser_list.h"
+#include "chrome/browser/ui/host_desktop.h"
+#include "chrome/browser/ui/search/instant_controller.h"
 #include "chrome/browser/ui/webui/favicon_source.h"
 #include "chrome/browser/ui/webui/ntp/thumbnail_source.h"
 #include "chrome/browser/ui/webui/theme_source.h"
@@ -28,7 +38,8 @@ using content::BrowserThread;
 
 InstantService::InstantService(Profile* profile)
     : profile_(profile),
-      most_visited_item_cache_(kMaxInstantMostVisitedItemCacheSize) {
+      most_visited_item_cache_(kMaxInstantMostVisitedItemCacheSize),
+      weak_ptr_factory_(this) {
   // Stub for unit tests.
   if (!BrowserThread::CurrentlyOn(BrowserThread::UI))
     return;
@@ -37,6 +48,12 @@ InstantService::InstantService(Profile* profile)
                  content::NOTIFICATION_RENDERER_PROCESS_TERMINATED,
                  content::NotificationService::AllSources());
 
+  history::TopSites* top_sites = profile_->GetTopSites();
+  if (top_sites) {

[sreeram, 2013/06/03 20:40:42]

Nit: This works for now, because the InstantService isn't created until first
use, but is brittle. I.e., in reality this check succeeds and we get Most
Visited info because nobody currently calls
InstantServiceFactory::GetForProfile() early enough. It would be better to do
the following:
1. Declare the dependencies for InstantServiceFactory (for example, see
https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...).
2. Instantiate the factory in ChromeBrowserMainExtraPartsProfiles::
EnsureBrowserContextKeyedServiceFactoriesBuilt().

Feel free to do the above in a followup CL (add a TODO or bug).

Note: I'm not talking about the "if" check. That's certainly a good idea and you
should keep it. In fact, I'm not recommending any change to the code here. I
just mean that, in production, we want the check to succeed more reliably, than
just by accident.

[kmadhusu, 2013/06/03 21:40:49]

Filed crbug.com/246351.

+    registrar_.Add(this,
+                   chrome::NOTIFICATION_TOP_SITES_CHANGED,
+                   content::Source<history::TopSites>(top_sites));
+  }
   instant_io_context_ = new InstantIOContext();
 
   if (profile_ && profile_->GetResourceContext()) {
@@ -138,18 +155,35 @@ void InstantService::AddMostVisitedItems(
   }
 }
 
+void InstantService::DeleteMostVisitedItem(const GURL& url) {
+  history::TopSites* top_sites = profile_->GetTopSites();
+  if (!top_sites)
+    return;
+
+  top_sites->AddBlacklistedURL(url);
+}
+
+void InstantService::UndoMostVisitedDeletion(const GURL& url) {
+  history::TopSites* top_sites = profile_->GetTopSites();
+  if (!top_sites)
+    return;
+
+  top_sites->RemoveBlacklistedURL(url);
+}
+
+void InstantService::UndoAllMostVisitedDeletions() {
+  history::TopSites* top_sites = profile_->GetTopSites();
+  if (!top_sites)
+    return;
+
+  top_sites->ClearBlacklistedURLs();
+}
+
 void InstantService::GetCurrentMostVisitedItems(
     std::vector<InstantMostVisitedItemIDPair>* items) const {
   most_visited_item_cache_.GetCurrentItems(items);
 }
 
-bool InstantService::GetMostVisitedItemForID(
-    InstantRestrictedID most_visited_item_id,
-    InstantMostVisitedItem* item) const {
-  return most_visited_item_cache_.GetItemWithRestrictedID(
-      most_visited_item_id, item);
-}
-
 void InstantService::Shutdown() {
   process_ids_.clear();
 
@@ -179,7 +213,55 @@ void InstantService::Observe(int type,
       }
       break;
     }
+    case chrome::NOTIFICATION_TOP_SITES_CHANGED: {
+      history::TopSites* top_sites = profile_->GetTopSites();
+      if (top_sites) {
+        top_sites->GetMostVisitedURLs(
+            base::Bind(&InstantService::OnMostVisitedItemsReceived,
+                       weak_ptr_factory_.GetWeakPtr()));
+      }
+      break;
+    }
     default:
       NOTREACHED() << "Unexpected notification type in InstantService.";
   }
 }
+
+bool InstantService::GetMostVisitedItemForID(
+    InstantRestrictedID most_visited_item_id,
+    InstantMostVisitedItem* item) const {
+  return most_visited_item_cache_.GetItemWithRestrictedID(
+      most_visited_item_id, item);
+}
+
+void InstantService::OnMostVisitedItemsReceived(
+    const history::MostVisitedURLList& data) {
+  // Android doesn't use Browser/BrowserList. Do nothing for Android platform.
+#if !defined(OS_ANDROID)
+  history::MostVisitedURLList reordered_data(data);
+  history::TopSites::MaybeShuffle(&reordered_data);
+
+  std::vector<InstantMostVisitedItem> most_visited_items;
+  for (size_t i = 0; i < reordered_data.size(); i++) {
+    const history::MostVisitedURL& url = reordered_data[i];
+    InstantMostVisitedItem item;
+    item.url = url.url;
+    item.title = url.title;
+    most_visited_items.push_back(item);
+  }
+  AddMostVisitedItems(most_visited_items);
+
+  const BrowserList* browser_list =
+      BrowserList::GetInstance(chrome::GetActiveDesktop());
+  for (BrowserList::const_iterator it = browser_list->begin();
+       it != browser_list->end(); ++it) {
+    if ((*it)->profile() != profile_ || !((*it)->instant_controller()))
+      continue;
+
+    InstantController* controller = (*it)->instant_controller()->instant();
+    if (!controller)
+      continue;
+    controller->UpdateMostVisitedItems();
+  }

[sreeram, 2013/06/03 20:40:42]

This is okay for now. But I think it would be cleaner to have each
InstantController register itself as an InstantServiceObserver and push out
updates that way. See
https://codereview.chromium.org/12386019/diff/16001/chrome/browser/instant/in...

Again, fine to do as a cleanup.

[kmadhusu, 2013/06/03 21:40:49]

Filed crbug.com/246355.

+#endif
+}