Remove MostVisitedSites => SupervisedUserService dep.

chrome/browser/android/ntp/most_visited_sites_bridge.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/ntp/most_visited_sites_bridge.h"
 
 #include <utility>
 
 #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/bind.h"
 #include "base/callback.h"
 #include "chrome/browser/android/ntp/popular_sites.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/history/top_sites_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_android.h"
 #include "chrome/browser/search/suggestions/suggestions_service_factory.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
+#include "chrome/browser/supervised_user/supervised_user_service.h"
+#include "chrome/browser/supervised_user/supervised_user_service_factory.h"
+#include "chrome/browser/supervised_user/supervised_user_url_filter.h"
 #include "chrome/browser/thumbnails/thumbnail_list_source.h"
 #include "components/history/core/browser/top_sites.h"
 #include "content/public/browser/url_data_source.h"
 #include "jni/MostVisitedSites_jni.h"
 #include "ui/gfx/android/java_bitmap.h"
 #include "url/gurl.h"
 
 using base::android::AttachCurrentThread;
 using base::android::ConvertJavaStringToUTF8;
 using base::android::ScopedJavaGlobalRef;
@@ skipping 10 matching lines @@
   JNIEnv* env = AttachCurrentThread();
   ScopedJavaLocalRef<jobject> j_bitmap;
   if (bitmap)
     j_bitmap = gfx::ConvertToJavaBitmap(bitmap);
   Java_ThumbnailCallback_onMostVisitedURLsThumbnailAvailable(
       env, j_callback->obj(), j_bitmap.obj(), is_local_thumbnail);
 }
 
 }  // namespace
 
+MostVisitedSitesBridge::SupervisorBridge::SupervisorBridge(Profile* profile)
+    : profile_(profile),
+      register_observer_(this) {
+  register_observer_.Add(
+      SupervisedUserServiceFactory::GetForProfile(profile_));
+}
+
+void
+MostVisitedSitesBridge::SupervisorBridge::SetObserver(Observer* new_observer) {

[Marc Treib, 2016/05/09 08:22:48]

Can this method move into MostVisitedSitesSupervisor? IMO it fits better there
(even if it won't be a pure interface anymore). WDYT?

[sfiera, 2016/05/09 09:33:35]

A rough check of Chromium code suggests that of AddObserver() methods, there
are:

101 pure virtual
https://cs.corp.google.com/search/?q=package:clankium+void%5C+AddObserver.*%3...;
193 override
https://cs.corp.google.com/search/?q=package:clankium+void%5C+AddObserver.*ov...;
302 neither
https://cs.corp.google.com/search/?q=package:clankium+void%5C+AddObserver.*%5C);

Spot-checking a couple of samples from category 3 suggests that most are
concrete classes, no pure virtuals, so I think that what you suggest is
generally not The Chromium Way.

[Marc Treib, 2016/05/09 09:55:12]

Okay, fair enough.
My rationale was that the method has quite specific semantics, so every
implementation would basically need to duplicate this exact implementation. But
since there'll probably only ever be this one implementation, I guess that
doesn't matter.

[sfiera, 2016/05/09 10:26:17]

If I were to go further, I think that really The Chromium Way would be to have
{Add,Remove}Observer() and delegate the implementation to an ObserverList,
right?

We can head that way if the interface needs to be used anywhere else.

+  if (new_observer) {
+    DCHECK(!observer_);
+  } else {
+    DCHECK(observer_);
+  }
+  observer_ = new_observer;
+}
+
+bool MostVisitedSitesBridge::SupervisorBridge::IsBlocked(const GURL& url) {
+  SupervisedUserService* supervised_user_service =
+    SupervisedUserServiceFactory::GetForProfile(profile_);

[Marc Treib, 2016/05/09 08:22:48]

wrong indent

[sfiera, 2016/05/09 09:33:35]

Done.

+  auto url_filter = supervised_user_service->GetURLFilterForUIThread();
+  return url_filter->GetFilteringBehaviorForURL(url) ==
+    SupervisedUserURLFilter::FilteringBehavior::BLOCK;

[Marc Treib, 2016/05/09 08:22:48]

here too

[sfiera, 2016/05/09 09:33:35]

Done.

+}
+
+std::vector<MostVisitedSitesSupervisor::Whitelist>
+MostVisitedSitesBridge::SupervisorBridge::whitelists() {
+  std::vector<MostVisitedSitesSupervisor::Whitelist> results;
+  SupervisedUserService* supervised_user_service =
+    SupervisedUserServiceFactory::GetForProfile(profile_);

[Marc Treib, 2016/05/09 08:22:48]

and here

[sfiera, 2016/05/09 09:33:35]

Done.

+  for (const auto& whitelist : supervised_user_service->whitelists()) {
+    results.emplace_back(Whitelist{

[Marc Treib, 2016/05/09 08:22:48]

I think when called like this, emplace_back will do the exact same thing as
push_back. If you actually want in-place construction, you need to leave out the
"Whitelist", just pass the args directly to emplace_back. (You'll probably have
to define an explicit ctor for that to work.)

[sfiera, 2016/05/09 09:33:35]

I tried without the explicit ctor, and you're right, it won't work without one.
I do think that the compiler is allowed to elide the copies given the way I've
written it, and probably does.

Do you want me to change this, or do more investigation? My inclination was to
write something clear that the compiler can probably optimize, and not worry
about it too much :)

[Marc Treib, 2016/05/09 09:55:12]

Eh, it's fine. I guess I'd slightly prefer push_back since AFAIK it'll do the
exact same thing, and this is sort of an abuse of emplace_back IMO. But it
doesn't really matter.

+        whitelist->title(),
+        whitelist->entry_point(),
+        whitelist->large_icon_path(),
+    });
+  }
+  return results;
+}
+
+bool MostVisitedSitesBridge::SupervisorBridge::IsChildProfile() {
+  return profile_->IsChild();
+}
+
+void MostVisitedSitesBridge::SupervisorBridge::OnURLFilterChanged() {
+  if (observer_) {

[Marc Treib, 2016/05/09 08:22:48]

nit: no braces

[sfiera, 2016/05/09 09:33:35]

(OK, these were mine)

+    observer_->OnBlockedSitesChanged();
+  }
+}
+
 class MostVisitedSitesBridge::Observer
     : public MostVisitedSites::Observer {
  public:
   Observer(JNIEnv* env, const JavaParamRef<jobject>& obj);
 
   void OnMostVisitedURLsAvailable(
       const MostVisitedSites::SuggestionsVector& suggestions) override;
 
   void OnPopularURLsAvailable(
       const MostVisitedSites::PopularSitesVector& sites) override;
@@ skipping 39 matching lines @@
     favicon_urls.emplace_back(site.favicon_url.spec());
     large_icon_urls.emplace_back(site.large_icon_url.spec());
   }
   Java_MostVisitedURLsObserver_onPopularURLsAvailable(
       env, observer_.obj(), ToJavaArrayOfStrings(env, urls).obj(),
       ToJavaArrayOfStrings(env, favicon_urls).obj(),
       ToJavaArrayOfStrings(env, large_icon_urls).obj());
 }
 
 MostVisitedSitesBridge::MostVisitedSitesBridge(Profile* profile)
-    : most_visited_(profile->GetPrefs(),
+    : supervisor_(profile),
+      most_visited_(profile->GetPrefs(),
                     TemplateURLServiceFactory::GetForProfile(profile),
                     g_browser_process->variations_service(),
                     profile->GetRequestContext(),
                     TopSitesFactory::GetForProfile(profile),
                     SuggestionsServiceFactory::GetForProfile(profile),
-                    profile->IsChild(),
-                    profile) {
+                    &supervisor_) {
   // Register the thumbnails debugging page.
   // TODO(sfiera): find thumbnails a home. They don't belong here.
   content::URLDataSource::Add(profile, new ThumbnailListSource(profile));
 }
 
 MostVisitedSitesBridge::~MostVisitedSitesBridge() {}
 
 void MostVisitedSitesBridge::Destroy(
     JNIEnv* env, const JavaParamRef<jobject>& obj) {
   delete this;
@@ skipping 52 matching lines @@
 }
 
 static jlong Init(JNIEnv* env,
                   const JavaParamRef<jobject>& obj,
                   const JavaParamRef<jobject>& jprofile) {
   MostVisitedSitesBridge* most_visited_sites =
       new MostVisitedSitesBridge(
           ProfileAndroid::FromProfileAndroid(jprofile));
   return reinterpret_cast<intptr_t>(most_visited_sites);
 }