SuggestionsService blacklist handling.

chrome/browser/android/most_visited_sites.cc

Index: chrome/browser/android/most_visited_sites.cc
diff --git a/chrome/browser/android/most_visited_sites.cc b/chrome/browser/android/most_visited_sites.cc
index 6fc326f7d9426cbad6c43d979ed6810d674a4b71..3a7dc5b96598de7d24ecd48bdab951a71e14f83f 100644
--- a/chrome/browser/android/most_visited_sites.cc
+++ b/chrome/browser/android/most_visited_sites.cc
@@ -4,6 +4,9 @@
 
 #include "chrome/browser/android/most_visited_sites.h"
 
+#include <string>
+#include <vector>
+
 #include "base/android/jni_android.h"
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
@@ -11,7 +14,6 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "chrome/browser/chrome_notification_types.h"
-#include "chrome/browser/history/history_types.h"
 #include "chrome/browser/history/top_sites.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_android.h"
@@ -60,22 +62,6 @@ void ExtractMostVisitedTitlesAndURLs(
   }
 }
 
-void OnMostVisitedURLsAvailable(
-    ScopedJavaGlobalRef<jobject>* j_observer,
-    int num_sites,
-    const history::MostVisitedURLList& visited_list) {
-  std::vector<base::string16> titles;
-  std::vector<std::string> urls;
-  ExtractMostVisitedTitlesAndURLs(visited_list, &titles, &urls, num_sites);
-
-  JNIEnv* env = AttachCurrentThread();
-  Java_MostVisitedURLsObserver_onMostVisitedURLsAvailable(
-      env,
-      j_observer->obj(),
-      ToJavaArrayOfStrings(env, titles).obj(),
-      ToJavaArrayOfStrings(env, urls).obj());
-}
-
 SkBitmap ExtractThumbnail(const base::RefCountedMemory& image_data) {
   scoped_ptr<SkBitmap> image(gfx::JPEGCodec::Decode(
       image_data.front(),
@@ -197,12 +183,31 @@ void MostVisitedSites::GetURLThumbnail(JNIEnv* env,
 void MostVisitedSites::BlacklistUrl(JNIEnv* env,
                                     jobject obj,
                                     jstring j_url) {
-  TopSites* top_sites = profile_->GetTopSites();
-  if (!top_sites)
-    return;
+  std::string url = ConvertJavaStringToUTF8(env, j_url);
+
+  switch (mv_source_) {
+    case TOP_SITES: {
+      TopSites* top_sites = profile_->GetTopSites();
+      if (top_sites) {

[Ted C, 2014/05/27 23:59:09]

braces aren't needed...and should we be dchecking the existence of top_sites and
suggestion_service below as you shouldn't have been able to fetch any data
without these being valid.

[manzagop (departed), 2014/05/29 21:40:31]

Removed braces. (Btw is it frowned upon in chromium? I know google c++ is fine
either way.)

Wrt DCHECKing instead of ifs: could top_sites / suggestion_service become null
after being non-null (something shuts the service down, the browser shuts down)?

[Ted C, 2014/05/30 16:37:13]

It's more about consistency.  I think most places don't use braces so I
generally avoid them for one liners as well, but again it's better to follow the
code around it.
My hope is that this object would be destroyed before these objects could become
null.  Otherwise people will think they are blacklisting a URL successfully only
to find out that it didn't work because something is terribly wrong in the
underbelly of chrome. 

[manzagop (departed), 2014/06/02 13:18:56]

Got it.
Sgtm. Switched to DCHECK.

+        top_sites->AddBlacklistedURL(GURL(url));
+      }
+      break;
+    }
 
-  std::string url_string = ConvertJavaStringToUTF8(env, j_url);
-  top_sites->AddBlacklistedURL(GURL(url_string));
+    case SUGGESTIONS_SERVICE: {
+      SuggestionsService* suggestions_service =
+          SuggestionsServiceFactory::GetForProfile(profile_);
+      if (suggestions_service) {
+        suggestions_service->AddBlacklistedURL(
+            GURL(url),
+            base::Bind(
+              &MostVisitedSites::OnSuggestionsProfileAvailable,

[Ted C, 2014/05/27 23:59:09]

+2 indent on these lines

[manzagop (departed), 2014/05/29 21:40:31]

Done.

+              weak_ptr_factory_.GetWeakPtr(),
+              base::Owned(new ScopedJavaGlobalRef<jobject>(observer_))));
+      }
+      break;
+    }
+  }
 }
 
 void MostVisitedSites::Observe(int type,
@@ -210,8 +215,10 @@ void MostVisitedSites::Observe(int type,
                                const content::NotificationDetails& details) {
   DCHECK_EQ(type, chrome::NOTIFICATION_TOP_SITES_CHANGED);
 
-  // Most visited urls changed, query again.
-  QueryMostVisitedURLs();
+  if (mv_source_ == TOP_SITES) {
+    // The displayed suggestions are invalidated.
+    QueryMostVisitedURLs();
+  }
 }
 
 // static
@@ -220,10 +227,8 @@ bool MostVisitedSites::Register(JNIEnv* env) {
 }
 
 void MostVisitedSites::QueryMostVisitedURLs() {
-  SuggestionsServiceFactory* suggestions_service_factory =
-      SuggestionsServiceFactory::GetInstance();
   SuggestionsService* suggestions_service =
-      suggestions_service_factory->GetForProfile(profile_);
+      SuggestionsServiceFactory::GetForProfile(profile_);

[Ted C, 2014/05/27 23:59:09]

The data source seems to be entirely determined off the existence of this
suggestions_service.  Will this service exist during construction of the most
visited sites object?  Just wondering if we should just set the source in the
constructor instead of in the callbacks.

[manzagop (departed), 2014/05/29 21:40:31]

The source may change in case of fallback (eg becoming offline offline).

   if (suggestions_service) {
     // Suggestions service is enabled, initiate a query.
     suggestions_service->FetchSuggestionsData(
@@ -243,12 +248,31 @@ void MostVisitedSites::InitiateTopSitesQuery() {
 
   top_sites->GetMostVisitedURLs(
       base::Bind(
-          &OnMostVisitedURLsAvailable,
+          &MostVisitedSites::OnMostVisitedURLsAvailable,
+          weak_ptr_factory_.GetWeakPtr(),
           base::Owned(new ScopedJavaGlobalRef<jobject>(observer_)),
           num_sites_),
       false);
 }
 
+void MostVisitedSites::OnMostVisitedURLsAvailable(
+    ScopedJavaGlobalRef<jobject>* j_observer,
+    int num_sites,
+    const history::MostVisitedURLList& visited_list) {
+  std::vector<base::string16> titles;
+  std::vector<std::string> urls;
+  ExtractMostVisitedTitlesAndURLs(visited_list, &titles, &urls, num_sites);
+
+  mv_source_ = TOP_SITES;

[Ted C, 2014/05/27 23:59:09]

if you set the source in the constructor, then this method can move back to the
empty namespace

[manzagop (departed), 2014/05/29 21:40:31]

As mentioned above, this is because of the fallback case which can switch the
source back to topsites.

+
+  JNIEnv* env = AttachCurrentThread();
+  Java_MostVisitedURLsObserver_onMostVisitedURLsAvailable(
+      env,
+      j_observer->obj(),
+      ToJavaArrayOfStrings(env, titles).obj(),
+      ToJavaArrayOfStrings(env, urls).obj());
+}
+
 void MostVisitedSites::OnSuggestionsProfileAvailable(
     ScopedJavaGlobalRef<jobject>* j_observer,
     const SuggestionsProfile& suggestions_profile) {
@@ -267,6 +291,8 @@ void MostVisitedSites::OnSuggestionsProfileAvailable(
     urls.push_back(suggestion.url());
   }
 
+  mv_source_ = SUGGESTIONS_SERVICE;
+
   JNIEnv* env = AttachCurrentThread();
   Java_MostVisitedURLsObserver_onMostVisitedURLsAvailable(
       env,