Public Sessions - prompt the user for pageCapture requests

chrome/browser/extensions/api/page_capture/page_capture_permission_helper.cc

+// Copyright 2016 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/extensions/api/page_capture/page_capture_permission_helper.h"
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/memory/ptr_util.h"
+#include "chrome/common/pref_names.h"
+#include "chromeos/login/login_state.h"
+#include "components/prefs/pref_change_registrar.h"
+#include "components/prefs/scoped_user_pref_update.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/permissions/api_permission.h"
+#include "extensions/common/permissions/api_permission_set.h"
+#include "extensions/common/permissions/manifest_permission_set.h"
+#include "extensions/common/permissions/permission_set.h"
+#include "extensions/common/url_pattern_set.h"
+
+using extensions::PageCapturePermissionHelper;

[Devlin, 2017/01/06 20:45:49]

instead, just put this file in the extensions:: namespace

[Ivan Šandrk, 2017/01/09 13:14:55]

Done.

+
+namespace {
+
+const char kNoAvailableBrowser[] =
+    "No available browser window to show the prompt.";
+const char kUserDenied[] = "User denied request.";
+
+}  // namespace
+
+PageCapturePermissionHelper::PageCapturePermissionHelper(
+    const extensions::Extension* extension,
+    PrefService* prefs,
+    base::Callback<void()> success_callback,
+    base::Callback<void(const std::string&)> failure_callback)
+  : extension_(extension), prefs_(prefs), success_callback_(success_callback),

[Devlin, 2017/01/06 20:45:49]

nit: indentation off

[Ivan Šandrk, 2017/01/09 13:14:56]

Done.

+    failure_callback_(failure_callback) {}
+
+PageCapturePermissionHelper::~PageCapturePermissionHelper() {}
+
+void PageCapturePermissionHelper::HandlePermissionRequest(
+    content::WebContents* web_contents) {
+  auto user_choice = UserChoiceGet();
+  if (user_choice == PermissionState::ALLOWED ||

[Devlin, 2017/01/06 20:45:49]

I'd prefer we use a switch here

[Ivan Šandrk, 2017/01/09 13:14:56]

Done.

+      user_choice == PermissionState::DENIED) {
+    ResolvePermissionRequest();
+    return;
+  }
+  if (user_choice == PermissionState::NOT_PROMPTED) {
+    ShowPermissionPrompt(web_contents);
+  }
+  // It's possible that several PageCapture requests come before user resolves
+  // the permission dialog, so setup a callback that gets called when pref
+  // changes (dialog is resolved).
+  pref_change_registrar_.reset(new PrefChangeRegistrar);

[Devlin, 2017/01/06 20:45:49]

prefer base::MakeUnique

[Ivan Šandrk, 2017/01/09 13:14:56]

Done.

+  pref_change_registrar_->Init(prefs_);
+  // base::Unretained is safe here because PrefChangeRegistrar will unregister
+  // callback on destruction of this object.
+  pref_change_registrar_->Add(
+      prefs::kPublicSessionPermissionsUserChoiceCache,
+      base::Bind(&PageCapturePermissionHelper::ResolvePermissionRequest,
+                 base::Unretained(this)));
+}
+
+void PageCapturePermissionHelper::ShowPermissionPrompt(
+    content::WebContents* web_contents) {
+  extensions::APIPermissionSet new_apis;
+  new_apis.insert(extensions::APIPermission::kPageCapture);
+  auto permission_set = base::MakeUnique<extensions::PermissionSet>(
+      new_apis, extensions::ManifestPermissionSet(),
+      extensions::URLPatternSet(), extensions::URLPatternSet());
+
+  if (!web_contents) {

[Devlin, 2017/01/06 20:45:49]

Can we put this first (before creating the permission set?)

[Ivan Šandrk, 2017/01/09 13:14:56]

Done.

+    failure_callback_.Run(kNoAvailableBrowser);
+    return;
+  }
+  prompt_ = base::MakeUnique<ExtensionInstallPrompt>(web_contents);
+  prompt_->ShowDialog(
+      base::Bind(&PageCapturePermissionHelper::ResolvePermissionPrompt,
+                 base::Unretained(this)),
+      extension_,
+      nullptr,  // Uses the extension icon.
+      base::MakeUnique<ExtensionInstallPrompt::Prompt>(
+          ExtensionInstallPrompt::PERMISSIONS_PROMPT),
+      std::move(permission_set),
+      ExtensionInstallPrompt::GetDefaultShowDialogCallback());
+
+  UserChoiceSet(PermissionState::SHOWN_PROMPT);
+}
+
+void PageCapturePermissionHelper::ResolvePermissionPrompt(
+    ExtensionInstallPrompt::Result prompt_result) {
+  // Dispose of the prompt as it's not needed anymore.
+  prompt_.reset();
+
+  bool allowed = prompt_result == ExtensionInstallPrompt::Result::ACCEPTED;
+  UserChoiceSet(allowed ? PermissionState::ALLOWED : PermissionState::DENIED);
+}
+
+void PageCapturePermissionHelper::ResolvePermissionRequest() {
+  auto user_choice = UserChoiceGet();
+  // Permission was allowed/denied for some other extension, continue sleeping.

[Devlin, 2017/01/06 20:45:49]

nit: s/sleeping/waiting.  sleep often has a specific meaning.

[Ivan Šandrk, 2017/01/09 13:14:55]

Done.

+  if (user_choice == PermissionState::SHOWN_PROMPT) {

[Devlin, 2017/01/06 20:45:49]

preferably a switch here too

[Ivan Šandrk, 2017/01/09 13:14:56]

Done.

+    return;
+  }
+
+  if (user_choice == PermissionState::DENIED) {
+    failure_callback_.Run(kUserDenied);
+  } else {
+    DCHECK(user_choice == PermissionState::ALLOWED);
+    success_callback_.Run();
+  }
+}
+
+void PageCapturePermissionHelper::UserChoiceSet(PermissionState value) {
+  DictionaryPrefUpdate update(prefs_,
+                              prefs::kPublicSessionPermissionsUserChoiceCache);
+  update->SetInteger(extension_->id(), value);
+}
+
+PageCapturePermissionHelper::PermissionState
+PageCapturePermissionHelper::UserChoiceGet() {
+  auto dictionary = prefs_->GetDictionary(

[Devlin, 2017/01/06 20:45:49]

here, let's not use auto.  It's unclear what the return result is (a
DictionaryValue*?  std::unique_ptr<DictionaryValue>?  DictionaryPrefUpdate? 
Some other dictionary?)

[Ivan Šandrk, 2017/01/09 13:14:55]

Done.

Is this because of the compiler or because of the reader? Or is this for
future-proofing the code? Because there's only one PrefService::GetDictionary.

[Devlin, 2017/01/09 18:05:07]

TL;DR: This is just for the reader's benefit. auto should pretty much only be
used when it's clear from this file/class what the result is.

We should only use auto when it's abundantly clear what the return type is
and/or when auto itself improves readability.  So, good examples for auto:
auto foo = base::MakeUnique<Foo>();  // MakeUnique<> takes the class name as the
template; impossible for this to return anything but a std::unique_ptr<Foo>.
std::vector<Foo> foos;
for (const auto& foo : foos) {
  // Since we know |foos| is a vector of Foo, we know that auto deduces to Foo -
and this is arguably much more readable than
  // for (std::vector<Foo>::const_iterator iter = foos.begin(); iter !=
foos.end(); ++iter)
}

In this particular case, while we might know that this returns a "dictionary" of
some kind, we don't know exactly what it is from looking at this code (or
knowing this class) alone.  We can probably assume that it's a
base::DictionaryValue from the chromium codebase and the methods called, but
even then, we'd actually have no idea if this was a std::unique_ptr<> or a
base::DictionaryValue* without looking up the method signature (which creates
more work for the reader/reviewer).  Similarly, we don't know anything about the
const-ness of the result.  On the other hand, just using const
base::DictionaryValue* is short, sweet, and clear. :)

+      prefs::kPublicSessionPermissionsUserChoiceCache);
+  int value = PermissionState::NOT_PROMPTED;
+  // In case no value was set previously, we return the default value
+  // (PermissionState::NOT_PROMPTED).
+  dictionary->GetInteger(extension_->id(), &value);
+  return static_cast<PermissionState>(value);
+}