SuggestionsService blacklist handling.

chrome/browser/android/most_visited_sites.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/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"
 #include "base/android/scoped_java_ref.h"
 #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"
 #include "chrome/browser/search/suggestions/proto/suggestions.pb.h"
 #include "chrome/browser/search/suggestions/suggestions_service.h"
 #include "chrome/browser/search/suggestions/suggestions_service_factory.h"
 #include "chrome/browser/search/suggestions/suggestions_source.h"
 #include "chrome/browser/thumbnails/thumbnail_list_source.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_source.h"
@@ skipping 28 matching lines @@
     const history::MostVisitedURL& visited = visited_list[i];
 
     if (visited.url.is_empty())
       break;  // This is the signal that there are no more real visited sites.
 
     titles->push_back(visited.title);
     urls->push_back(visited.url.spec());
   }
 }
 
-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(),
       image_data.size()));
   return image.get() ? *image : SkBitmap();
 }
 
 void OnObtainedThumbnail(
     ScopedJavaGlobalRef<jobject>* bitmap,
     ScopedJavaGlobalRef<jobject>* j_callback) {
@@ skipping 67 matching lines @@
 
 void MostVisitedSites::SetMostVisitedURLsObserver(JNIEnv* env,
                                                   jobject obj,
                                                   jobject j_observer,
                                                   jint num_sites) {
   observer_.Reset(env, j_observer);
   num_sites_ = num_sites;
 
   QueryMostVisitedURLs();
 
   history::TopSites* top_sites = profile_->GetTopSites();

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

should we not be doing this block if we are using the suggestions service?

Since we check the source in the observer, it seems like we shouldn't be
registering this block at all.

Also, as these start diverging more based on source, I think we should consider
splitting these bits out of this class and have the source be something this
delegates to.

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

The TopSites notification actually does get used in the fallback scenario: if
the suggestion service can't get recommendations, it calls
OnSuggestionsProfileAvailable with an empty profile, which triggers a
InitiateTopSitesQuery which gets TopSites urls to show and changes the source to
TOP_SITES. At this point, the user might blacklist something and we rely on the
notification. Though we could register / unregister when we change the source.
Does that seem like a good idea?
I agree that what we have now is messy. Would you be ok leaving that for a
subsequent CL? I don't think the current cl makes things worse, and separating
the blacklist change from the refactoring may be a good idea.

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

Ah, I missed the InitiateTopSitesQuery call in the OnSuggestionsProfileAvailable
function.

I do think unregistering is slightly cleaner, but since you are guarding the
call in the observer I don't think it really matters too much.
Follow up CL is definitely fine.

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

Acknowledged.

   if (top_sites) {
     // TopSites updates itself after a delay. To ensure up-to-date results,
     // force an update now.
     top_sites->SyncWithHistory();
 
     // Register for notification when TopSites changes so that we can update
     // ourself.
     registrar_.Add(this, chrome::NOTIFICATION_TOP_SITES_CHANGED,
                    content::Source<history::TopSites>(top_sites));
   }
@@ skipping 13 matching lines @@
   BrowserThread::PostTask(
       BrowserThread::DB, FROM_HERE, base::Bind(
           &GetUrlThumbnailTask,
           url_string,
           top_sites, base::Owned(j_callback)));
 }
 
 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);
 
-  std::string url_string = ConvertJavaStringToUTF8(env, j_url);
-  top_sites->AddBlacklistedURL(GURL(url_string));
+  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;
+    }
+
+    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,
                                const content::NotificationSource& source,
                                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
 bool MostVisitedSites::Register(JNIEnv* env) {
   return RegisterNativesImpl(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(
         base::Bind(
           &MostVisitedSites::OnSuggestionsProfileAvailable,
           weak_ptr_factory_.GetWeakPtr(),
           base::Owned(new ScopedJavaGlobalRef<jobject>(observer_))));
   } else {
     InitiateTopSitesQuery();
   }
 }
 
 void MostVisitedSites::InitiateTopSitesQuery() {
   TopSites* top_sites = profile_->GetTopSites();
   if (!top_sites)
     return;
 
   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) {
   size_t size = suggestions_profile.suggestions_size();
   if (size == 0) {
     // No suggestions data available, initiate Top Sites query.
     InitiateTopSitesQuery();
     return;
   }
 
   std::vector<base::string16> titles;
   std::vector<std::string> urls;
   for (size_t i = 0; i < size; ++i) {
     const ChromeSuggestion& suggestion = suggestions_profile.suggestions(i);
     titles.push_back(base::UTF8ToUTF16(suggestion.title()));
     urls.push_back(suggestion.url());
   }
 
+  mv_source_ = SUGGESTIONS_SERVICE;
+
   JNIEnv* env = AttachCurrentThread();
   Java_MostVisitedURLsObserver_onMostVisitedURLsAvailable(
       env,
       j_observer->obj(),
       ToJavaArrayOfStrings(env, titles).obj(),
       ToJavaArrayOfStrings(env, urls).obj());
 }
 
 static jlong Init(JNIEnv* env, jobject obj, jobject jprofile) {
   MostVisitedSites* most_visited_sites =
       new MostVisitedSites(ProfileAndroid::FromProfileAndroid(jprofile));
   return reinterpret_cast<intptr_t>(most_visited_sites);
 }