Most visited thumbnails and favicons need id-based urls

chrome/browser/instant/instant_controller.cc

Index: chrome/browser/instant/instant_controller.cc
diff --git a/chrome/browser/instant/instant_controller.cc b/chrome/browser/instant/instant_controller.cc
index ff58a6cadc23b1a2ee84a390a024f97265cacedc..120cc3719fee9d0d2c819fe7947a46bdd22ba1f8 100644
--- a/chrome/browser/instant/instant_controller.cc
+++ b/chrome/browser/instant/instant_controller.cc
@@ -15,6 +15,8 @@
 #include "chrome/browser/history/top_sites.h"
 #include "chrome/browser/instant/instant_ntp.h"
 #include "chrome/browser/instant/instant_overlay.h"
+#include "chrome/browser/instant/instant_service.h"
+#include "chrome/browser/instant/instant_service_factory.h"
 #include "chrome/browser/instant/instant_tab.h"
 #include "chrome/browser/instant/search.h"
 #include "chrome/browser/platform_util.h"
@@ -195,6 +197,23 @@ void EnsureSearchTermsAreSet(content::WebContents* contents,
       NavigationEntryUpdated();
 }
 
+bool GetURLForRestrictedId(Profile* profile, uint64 restricted_id, GURL* url) {

[palmer, 2013/03/11 20:42:31]

|profile| should be declared const, or const & — it's not an out parameter.

[dhollowa, 2013/03/11 23:27:59]

This is endemic in Chrome code.  Pretty much nowhere supports const Profile.  So
doing anywhere becomes a viral nightmare.

+  InstantService* instant_service =
+      InstantServiceFactory::GetForProfile(profile);
+  if (!instant_service)
+    return false;
+  return instant_service->GetURLForRestrictedId(restricted_id, url);
+}
+
+// Creates a new restriced id if one is not found.
+size_t GetRestrictedIDForURL(Profile* profile, const GURL& url) {

[palmer, 2013/03/11 20:42:31]

const |profile|.

[dhollowa, 2013/03/11 23:27:59]

Ditto.

+  InstantService* instant_service =

[palmer, 2013/03/11 20:42:31]

Perhaps the caller of these two functions should pass a const InstantService*
(or &), instead of Profile.

[dhollowa, 2013/03/11 23:27:59]

Pushing that up to the caller becomes quite cluttered.  I could keep a cached
member variable for a new |instant_service_| but that feels like overkill for
this modest replication.  If this pattern continues to grow I will do so
however.

+      InstantServiceFactory::GetForProfile(profile);
+  if (!instant_service)
+    return 0;
+  return instant_service->AddURL(url);
+}
+
 }  // namespace
 
 InstantController::InstantController(chrome::BrowserInstantController* browser,
@@ -860,20 +879,24 @@ void InstantController::ClearDebugEvents() {
   debug_events_.clear();
 }
 
-void InstantController::DeleteMostVisitedItem(const GURL& url) {
+void InstantController::DeleteMostVisitedItem(uint64 restricted_id) {
   history::TopSites* top_sites = browser_->profile()->GetTopSites();
   if (!top_sites)
     return;
 
-  top_sites->AddBlacklistedURL(url);
+  GURL url;
+  if (GetURLForRestrictedId(browser_->profile(), restricted_id, &url))
+    top_sites->AddBlacklistedURL(url);
 }
 
-void InstantController::UndoMostVisitedDeletion(const GURL& url) {
+void InstantController::UndoMostVisitedDeletion(uint64 restricted_id) {
   history::TopSites* top_sites = browser_->profile()->GetTopSites();
   if (!top_sites)
     return;
 
-  top_sites->RemoveBlacklistedURL(url);
+  GURL url;
+  if (GetURLForRestrictedId(browser_->profile(), restricted_id, &url))
+    top_sites->RemoveBlacklistedURL(url);
 }
 
 void InstantController::UndoAllMostVisitedDeletions() {
@@ -1460,11 +1483,12 @@ void InstantController::RequestMostVisitedItems() {
 
 void InstantController::OnMostVisitedItemsReceived(
     const history::MostVisitedURLList& data) {
-  std::vector<MostVisitedItem> most_visited_items;
+  std::vector<InstantMostVisitedItem> most_visited_items;
   for (size_t i = 0; i < data.size(); i++) {
     const history::MostVisitedURL& url = data[i];
 
-    MostVisitedItem item;
+    InstantMostVisitedItem item;
+    item.restricted_id = GetRestrictedIDForURL(browser_->profile(), url.url);
     item.url = url.url;
     item.title = url.title;
 
@@ -1474,7 +1498,7 @@ void InstantController::OnMostVisitedItemsReceived(
 }
 
 void InstantController::SendMostVisitedItems(
-    const std::vector<MostVisitedItem>& items) {
+    const std::vector<InstantMostVisitedItem>& items) {
   if (overlay_)
     overlay_->SendMostVisitedItems(items);
   if (ntp_)