Supervised users: When an extension requires new permissions, send request to custodian

chrome/browser/supervised_user/supervised_user_service.cc

Index: chrome/browser/supervised_user/supervised_user_service.cc
diff --git a/chrome/browser/supervised_user/supervised_user_service.cc b/chrome/browser/supervised_user/supervised_user_service.cc
index e464400c0cc5618681451f5eba6191d746681cc7..37e06acfc606533976f1ae0401d94a4586abd50d 100644
--- a/chrome/browser/supervised_user/supervised_user_service.cc
+++ b/chrome/browser/supervised_user/supervised_user_service.cc
@@ -225,6 +225,42 @@ void SupervisedUserService::URLFilterContext::InitAsyncURLChecker(
                  io_url_filter_, context, cx));
 }
 
+class SupervisedUserService::SendPermissionRequestHelper {
+ public:
+  virtual ~SendPermissionRequestHelper() {}
+
+  virtual void SendPermissionRequest(PermissionRequestCreator* creator,

[Bernhard Bauer, 2015/03/02 23:09:59]

Nice! Now that I see this, I'm wondering though whether this would be even
cleaner with callbacks? That would remove a lot of the boilerplate here.

[Marc Treib, 2015/03/03 10:04:50]

You're right, it's much less boilerplate with callbacks. Done, thanks! (How
didn't I think of that?!)

+                                     const SuccessCallback& callback) = 0;
+};
+
+class SupervisedUserService::SendURLAccessRequestHelper
+    : public SupervisedUserService::SendPermissionRequestHelper {
+ public:
+  explicit SendURLAccessRequestHelper(const GURL& url) : url_(url) {}
+
+  void SendPermissionRequest(PermissionRequestCreator* creator,
+                             const SuccessCallback& callback) override {
+    creator->CreateURLAccessRequest(url_, callback);
+  }
+
+ private:
+  const GURL url_;
+};
+
+class SupervisedUserService::SendExtensionUpdateRequestHelper
+    : public SupervisedUserService::SendPermissionRequestHelper {
+ public:
+  explicit SendExtensionUpdateRequestHelper(const std::string& id) : id_(id) {}
+
+  void SendPermissionRequest(PermissionRequestCreator* creator,
+                             const SuccessCallback& callback) override {
+    creator->CreateExtensionUpdateRequest(id_, callback);
+  }
+
+ private:
+  const std::string id_;
+};
+
 SupervisedUserService::SupervisedUserService(Profile* profile)
     : includes_sync_sessions_type_(true),
       profile_(profile),
@@ -531,8 +567,8 @@ size_t SupervisedUserService::FindEnabledPermissionRequestCreator(
   return permissions_creators_.size();
 }
 
-void SupervisedUserService::AddAccessRequestInternal(
-    const GURL& url,
+void SupervisedUserService::AddPermissionRequestInternal(
+    scoped_ptr<SendPermissionRequestHelper> helper,
     const SuccessCallback& callback,
     size_t index) {
   // Find a permission request creator that is enabled.
@@ -542,14 +578,15 @@ void SupervisedUserService::AddAccessRequestInternal(
     return;
   }
 
-  permissions_creators_[next_index]->CreatePermissionRequest(
-      url,
+  helper->SendPermissionRequest(
+      permissions_creators_[next_index],
       base::Bind(&SupervisedUserService::OnPermissionRequestIssued,
-                 weak_ptr_factory_.GetWeakPtr(), url, callback, next_index));
+                 weak_ptr_factory_.GetWeakPtr(), base::Passed(&helper),

[Marc Treib, 2015/03/02 18:26:00]

This is ugly: The base::Passed nulls the helper, but apparently the
helper->SendPermissionRequest is resolved first. Don't know if that's
well-defined though - do you know? I guess the alternative would be to grab a
raw pointer to the helper before the Bind...

[Bernhard Bauer, 2015/03/02 23:09:59]

Wait, how would we call the method before its arguments are fully resolved? Or
do we dereference the scoped_ptr, *then* resolve the arguments, then call it?
That seems indeed kind of scary to me, and yes, I would in that case get a raw
pointer (which really just makes the order explicit in which things happen).

[Marc Treib, 2015/03/03 10:04:50]

Thinking about this again, it probably depends on the order of evaluation of
method arguments: "helper->" is basically the first argument, and apparently
that's evaluated before the "base::Passed(&helper)". That's not well-defined
though.
Luckily, the version with callbacks makes this unnecessary :)

+                 callback, next_index));
 }
 
 void SupervisedUserService::OnPermissionRequestIssued(
-    const GURL& url,
+    scoped_ptr<SendPermissionRequestHelper> helper,
     const SuccessCallback& callback,
     size_t index,
     bool success) {
@@ -558,7 +595,7 @@ void SupervisedUserService::OnPermissionRequestIssued(
     return;
   }
 
-  AddAccessRequestInternal(url, callback, index + 1);
+  AddPermissionRequestInternal(helper.Pass(), callback, index + 1);
 }
 
 void SupervisedUserService::OnSupervisedUserIdChanged() {
@@ -631,10 +668,21 @@ bool SupervisedUserService::AccessRequestsEnabled() {
   return FindEnabledPermissionRequestCreator(0) < permissions_creators_.size();
 }
 
-void SupervisedUserService::AddAccessRequest(const GURL& url,
-                                             const SuccessCallback& callback) {
-  AddAccessRequestInternal(SupervisedUserURLFilter::Normalize(url), callback,
-                           0);
+void SupervisedUserService::AddURLAccessRequest(
+    const GURL& url,
+    const SuccessCallback& callback) {
+  AddPermissionRequestInternal(
+      make_scoped_ptr(new SendURLAccessRequestHelper(
+          SupervisedUserURLFilter::Normalize(url))),
+      callback, 0);
+}
+
+void SupervisedUserService::AddExtensionUpdateRequest(
+    const std::string& extension_id,
+    const SuccessCallback& callback) {
+  AddPermissionRequestInternal(
+      make_scoped_ptr(new SendExtensionUpdateRequestHelper(extension_id)),
+      callback, 0);
 }
 
 void SupervisedUserService::InitSync(const std::string& refresh_token) {