Remove MostVisitedSites => SupervisedUserService dep.

chrome/browser/android/ntp/most_visited_sites_bridge.cc

Index: chrome/browser/android/ntp/most_visited_sites_bridge.cc
diff --git a/chrome/browser/android/ntp/most_visited_sites_bridge.cc b/chrome/browser/android/ntp/most_visited_sites_bridge.cc
index b606586073708ef5dba7298e42882f8918b9e1ab..425be0233e9ce28efd28d4b075edd4a541c6723c 100644
--- a/chrome/browser/android/ntp/most_visited_sites_bridge.cc
+++ b/chrome/browser/android/ntp/most_visited_sites_bridge.cc
@@ -19,6 +19,9 @@
 #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"
@@ -49,6 +52,56 @@ void CallJavaWithBitmap(
 
 }  // 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:
@@ -108,14 +161,14 @@ void MostVisitedSitesBridge::Observer::OnPopularURLsAvailable(
 }
 
 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));