Public Sessions - prompt the user for pageCapture requests

chrome/browser/chromeos/extensions/public_session_permission_helper_impl.h

+// Copyright 2017 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.
+
+#ifndef CHROME_BROWSER_CHROMEOS_EXTENSIONS_PUBLIC_SESSION_PERMISSION_HELPER_IMPL_H_
+#define CHROME_BROWSER_CHROMEOS_EXTENSIONS_PUBLIC_SESSION_PERMISSION_HELPER_IMPL_H_
+
+#include <map>
+#include <memory>
+#include <vector>
+
+#include "base/callback_forward.h"
+#include "base/macros.h"
+#include "chrome/browser/chromeos/extensions/public_session_permission_helper.h"
+#include "chrome/browser/extensions/extension_install_prompt.h"
+#include "extensions/common/permissions/api_permission.h"
+#include "extensions/common/permissions/api_permission_set.h"
+
+namespace extensions {
+namespace permission_helper {
+
+struct RequestCallback {

[Devlin, 2017/01/23 22:59:48]

Can this be internal to the class (i.e., a private struct)?

[Ivan Šandrk, 2017/01/24 19:57:22]

Done.

+  RequestCallback(const base::Closure& success_callback,
+                  const base::Closure& failure_callback,
+                  const PermissionHelperSet& permission_list);
+  RequestCallback(const RequestCallback& other);
+  ~RequestCallback();
+  base::Closure success_callback;
+  base::Closure failure_callback;
+  PermissionHelperSet permission_list;
+};
+using RequestCallbackList = std::vector<RequestCallback>;
+
+class PublicSessionPermissionHelperImpl {

[Devlin, 2017/01/23 22:59:48]

It looks to me like this class can probably be combined with the other.  Since
this predominantly just contains a bit of data, we could probably just replace
the entry of PublicSessionPermissionHelperImpl in the map with a struct
containing that data.  That would cut down on the duplication of code between
the two files.

[Ivan Šandrk, 2017/01/24 19:57:22]

In one of the earlier iterations I had it all in one class but it seemed too
messy to me. Every time I had to access one of the member variables, I had to
also put in the extension_id. This way it seems more elegant to me, logical
separation at the extension level. I can tho merge the files together, and have
the impl be private to public_session_permission_helper.cc. And maybe pick a
better name than just _impl (sometimes I'm not creative with names).

[Devlin, 2017/01/25 16:00:31]

To elaborate a little here.

Currently, PublicSessionPermissionHelper just contains a map<ExtensionId,
HelperImpl>.  What if we instead do something like:

// .h

class Helper {
 public:
  static HandlePermissionRequest();
  static GetUserChoice();

 private:
  HandlePermissionRequest();
  GetUserChoice();

  prompt_map_;
  prompted_set_;
  ....
};

// .cc

namespace {
LazyInstance<std::map<ExtensionId, Helper>> helpers;
}

// static
Helper::HandlePermissionRequest() {
  (*helpers.Get())[extension->id()]->HandlePermissionRequest();
}

// static
Helper::GetUserChoice() {
  (*helpers.Get())[extension->id()]->GetUserChoice();
}

Helper::HandlePermissionChoice() {
  // impl
}

Helper::GetUserChoice() {
  // impl
}

That way, the separation and class logic remains the same, but we coalesce the
two files into one.  WDYT?

[Ivan Šandrk, 2017/01/26 18:53:22]

Ah I see, excellent idea! Done!

+ public:
+  PublicSessionPermissionHelperImpl();
+  PublicSessionPermissionHelperImpl(PublicSessionPermissionHelperImpl&& other);
+  ~PublicSessionPermissionHelperImpl();
+
+  // Sets up the prompt asking the user for additional permission(s), handles
+  // the result, caches it, and then runs either success_callback or
+  // failure_callback depending on all permissions being allowed.
+  // Supports handling multiple requests for the same permission(s). Only the
+  // first request causes the prompt to be shown, subsequent ones are just
+  // enqueued to be called when the permission(s) is resolved.
+  // Caller must ensure that web_contents is valid. Must be called on UI thread.
+  // If finer resolving is needed, pass the same function in both callbacks and
+  // check the individual permissions by calling GetUserChoice inside your
+  // function.
+  void HandlePermissionRequest(const Extension* extension,
+                               PermissionHelperSet requested_permissions,
+                               content::WebContents* web_contents,
+                               const base::Closure& success_callback,
+                               const base::Closure& failure_callback);
+
+
+  PermissionState GetUserChoice(APIPermission::ID permission_id);
+
+ private:
+  void SetUserChoice(APIPermission::ID permission_id, PermissionState value);

[Devlin, 2017/01/23 22:59:48]

Never defined

[Ivan Šandrk, 2017/01/24 19:57:22]

Right, had a cleanup and missed this. Thanks!

+
+  void ResolvePermissionPrompt(const Extension* extension,
+                               PermissionIDSet unprompted_permissions,
+                               ExtensionInstallPrompt::Result prompt_result);
+
+  std::map<APIPermission::ID, std::unique_ptr<ExtensionInstallPrompt>>
+      prompt_map_;
+  PermissionIDSet prompted_permission_set_;
+  PermissionIDSet allowed_permission_set_;
+  PermissionIDSet denied_permission_set_;
+  RequestCallbackList callbacks_;
+
+  DISALLOW_COPY_AND_ASSIGN(PublicSessionPermissionHelperImpl);
+};
+
+}  // namespace permission_helper
+}  // namespace extensions
+
+#endif  // CHROME_BROWSER_CHROMEOS_EXTENSIONS_PUBLIC_SESSION_PERMISSION_HELPER_IMPL_H_