PermissionWizard: Add delegate and permission-checking logic
This adds a permission-checking Delegate interface, which would
implement the permission-checks, and provide the results via
asynchronous callback to the PermissionWizard. This also adds the
logic to poll the permission-checker and show the appropriate UI.
This will skip showing any pages whose permissions are already granted.
A dialog (page of the wizard) is only shown for each denied permission.
If all permissions are granted, nothing at all will be shown to the
user. If at least one permission is denied, the wizard will be shown,
and the user will see the "All set!" page at the end.
Still TODO: add a mechanism for the caller of PermissionWizard to know
when the user cancels or completes the wizard. This would, for example,
allow a command-line tool to wait until the wizard is finished before
quitting with an exit-code.
Bug: 1015201
Change-Id: Icf977d122c548b31c91c4ae0a71200c43ed241cb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1902116
Reviewed-by: Yuwei Huang <yuweih@chromium.org>
Commit-Queue: Lambros Lambrou <lambroslambrou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#714069}
diff --git a/remoting/host/mac/permission_wizard.h b/remoting/host/mac/permission_wizard.h
index 2c68249b..e53e4bc 100644
--- a/remoting/host/mac/permission_wizard.h
+++ b/remoting/host/mac/permission_wizard.h
@@ -6,8 +6,9 @@
#define REMOTING_HOST_MAC_PERMISSION_WIZARD_H_
#include <memory>
+#include <string>
-#include "base/macros.h"
+#include "base/callback.h"
#include "base/memory/scoped_refptr.h"
namespace base {
@@ -21,21 +22,46 @@
// needed MacOS permissions for the host process.
class PermissionWizard final {
public:
- PermissionWizard();
+ class Impl;
+
+ // Callback for the Delegate to inform this class whether a permission was
+ // granted.
+ using ResultCallback = base::OnceCallback<void(bool granted)>;
+
+ // Interface to delegate the permission-checks. This will be invoked to test
+ // each permission, and will return (via callback) whether the permission was
+ // granted.
+ class Delegate {
+ public:
+ virtual ~Delegate() = default;
+
+ // Returns the name of the bundle that needs to be granted permission, as
+ // it would appear in the System Preferences applet. This will be included
+ // in the dialog's instructional text.
+ virtual std::string GetBundleName() = 0;
+
+ // These checks will be invoked on the UI thread, and the result should be
+ // returned to the callback on the same thread.
+ // They should cause the bundle-name to be added to System Preferences
+ // applet. As far as possible, the check should not trigger a system prompt,
+ // but this may be unavoidable.
+ virtual void CheckAccessibilityPermission(ResultCallback onResult) = 0;
+ virtual void CheckScreenRecordingPermission(ResultCallback onResult) = 0;
+ };
+
+ explicit PermissionWizard(std::unique_ptr<Delegate> checker);
+ PermissionWizard(const PermissionWizard&) = delete;
+ PermissionWizard& operator=(const PermissionWizard&) = delete;
~PermissionWizard();
void Start(scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner);
private:
- class Impl;
-
// Private implementation, to hide the Objective-C and Cocoa objects from C++
// callers.
std::unique_ptr<Impl> impl_;
scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner_;
-
- DISALLOW_COPY_AND_ASSIGN(PermissionWizard);
};
} // namespace mac
diff --git a/remoting/host/mac/permission_wizard.mm b/remoting/host/mac/permission_wizard.mm
index 29791029..289361f8 100644
--- a/remoting/host/mac/permission_wizard.mm
+++ b/remoting/host/mac/permission_wizard.mm
@@ -7,15 +7,28 @@
#import <Cocoa/Cocoa.h>
#include "base/bind.h"
+#include "base/memory/weak_ptr.h"
#include "base/single_thread_task_runner.h"
#include "base/strings/sys_string_conversions.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/task/post_task.h"
+#include "base/time/time.h"
+#include "base/timer/timer.h"
#include "remoting/base/string_resources.h"
#include "ui/base/cocoa/window_size_constants.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/base/l10n/l10n_util_mac.h"
+using remoting::mac::PermissionWizard;
+using Delegate = PermissionWizard::Delegate;
+using ResultCallback = PermissionWizard::ResultCallback;
+
namespace {
+// Interval between permission checks, used to update the UI when the user
+// grants permission.
+constexpr base::TimeDelta kPollingInterval = base::TimeDelta::FromSeconds(1);
+
// The steps of the wizard.
enum class WizardPage {
ACCESSIBILITY,
@@ -27,12 +40,99 @@
@interface PermissionWizardController : NSWindowController
-- (instancetype)initWithWindow:(NSWindow*)window;
+- (instancetype)initWithWindow:(NSWindow*)window
+ impl:(PermissionWizard::Impl*)impl;
- (void)hide;
-- (void)initializeWindow;
+- (void)start;
+
+// Used by C++ PermissionWizardImpl to provide the result of a permission check
+// to the WindowController.
+- (void)onPermissionCheckResult:(bool)result;
@end
+namespace remoting {
+namespace mac {
+
+// C++ implementation of the PermissionWizard.
+class PermissionWizard::Impl {
+ public:
+ explicit Impl(std::unique_ptr<PermissionWizard::Delegate> checker);
+ ~Impl();
+
+ void Start();
+
+ std::string GetBundleName();
+
+ // Called by PermissionWizardController to initiate permission checks. The
+ // result will be passed back via onPermissionCheckResult().
+ void CheckAccessibilityPermission(base::TimeDelta delay);
+ void CheckScreenRecordingPermission(base::TimeDelta delay);
+
+ private:
+ void CheckAccessibilityPermissionNow();
+ void CheckScreenRecordingPermissionNow();
+
+ void OnPermissionCheckResult(bool result);
+
+ PermissionWizardController* window_controller_ = nil;
+ std::unique_ptr<Delegate> checker_;
+ base::OneShotTimer timer_;
+ base::WeakPtrFactory<Impl> weak_factory_{this};
+};
+
+PermissionWizard::Impl::Impl(
+ std::unique_ptr<PermissionWizard::Delegate> checker)
+ : checker_(std::move(checker)) {}
+
+PermissionWizard::Impl::~Impl() {
+ [window_controller_ hide];
+ [window_controller_ release];
+}
+
+void PermissionWizard::Impl::Start() {
+ NSWindow* window =
+ [[[NSWindow alloc] initWithContentRect:ui::kWindowSizeDeterminedLater
+ styleMask:NSWindowStyleMaskTitled
+ backing:NSBackingStoreBuffered
+ defer:NO] autorelease];
+ window_controller_ = [[PermissionWizardController alloc] initWithWindow:window
+ impl:this];
+ [window_controller_ start];
+}
+
+std::string PermissionWizard::Impl::GetBundleName() {
+ return checker_->GetBundleName();
+}
+
+void PermissionWizard::Impl::CheckAccessibilityPermission(
+ base::TimeDelta delay) {
+ timer_.Start(FROM_HERE, delay, this, &Impl::CheckAccessibilityPermissionNow);
+}
+
+void PermissionWizard::Impl::CheckScreenRecordingPermission(
+ base::TimeDelta delay) {
+ timer_.Start(FROM_HERE, delay, this,
+ &Impl::CheckScreenRecordingPermissionNow);
+}
+
+void PermissionWizard::Impl::CheckAccessibilityPermissionNow() {
+ checker_->CheckAccessibilityPermission(base::BindOnce(
+ &Impl::OnPermissionCheckResult, weak_factory_.GetWeakPtr()));
+}
+
+void PermissionWizard::Impl::CheckScreenRecordingPermissionNow() {
+ checker_->CheckScreenRecordingPermission(base::BindOnce(
+ &Impl::OnPermissionCheckResult, weak_factory_.GetWeakPtr()));
+}
+
+void PermissionWizard::Impl::OnPermissionCheckResult(bool result) {
+ [window_controller_ onPermissionCheckResult:result];
+}
+
+} // namespace mac
+} // namespace remoting
+
@implementation PermissionWizardController {
NSTextField* _instructionText;
NSButton* _cancelButton;
@@ -44,12 +144,31 @@
// Whether the relevant permission has been granted for the current page. If
// YES, the user will be able to advance to the next page of the wizard.
BOOL _hasPermission;
+
+ // Set to YES when the user cancels the wizard. This allows code to
+ // distinguish between "window hidden because it hasn't been presented yet"
+ // and "window hidden because user closed it".
+ BOOL _cancelled;
+
+ // If YES, the wizard will automatically move onto the next page instead of
+ // showing the "Next" button when permission is granted. This allows the
+ // wizard to skip past any pages whose permission is already granted.
+ BOOL _autoAdvance;
+
+ // Reference used for permission-checking. Its lifetime should outlast this
+ // Controller.
+ PermissionWizard::Impl* _impl;
}
-- (instancetype)initWithWindow:(NSWindow*)window {
- self = [super initWithWindow:(NSWindow*)window];
+- (instancetype)initWithWindow:(NSWindow*)window
+ impl:(PermissionWizard::Impl*)impl {
+ DCHECK(window);
+ DCHECK(impl);
+ self = [super initWithWindow:window];
if (self) {
+ _impl = impl;
_page = WizardPage::ACCESSIBILITY;
+ _autoAdvance = YES;
}
return self;
}
@@ -58,6 +177,13 @@
[self close];
}
+- (void)start {
+ [self initializeWindow];
+
+ // Start polling for permission status.
+ [self requestPermissionCheck:base::TimeDelta()];
+}
+
- (void)initializeWindow {
self.window.title =
l10n_util::GetNSStringF(IDS_MAC_PERMISSION_WIZARD_TITLE,
@@ -146,24 +272,29 @@
options:0
metrics:nil
views:views]];
-
- [self updateUI];
-
- [self.window makeKeyAndOrderFront:NSApp];
- [self.window center];
}
- (void)onCancel:(id)sender {
+ _cancelled = YES;
[self hide];
}
- (void)onNext:(id)sender {
- NOTIMPLEMENTED();
+ if (_page == WizardPage::ALL_SET) {
+ // OK button closes the window.
+ [self hide];
+ return;
+ }
+ if (_hasPermission) {
+ [self advanceToNextPage];
+ } else {
+ [self launchSystemPreferences];
+ }
}
// Updates the dialog controls according to the object's state.
- (void)updateUI {
- // TODO(lambroslambrou): Parameterize the name of the app needing permission.
+ base::string16 bundleName = base::UTF8ToUTF16(_impl->GetBundleName());
switch (_page) {
case WizardPage::ACCESSIBILITY:
_instructionText.stringValue = l10n_util::GetNSStringF(
@@ -171,7 +302,7 @@
l10n_util::GetStringUTF16(IDS_PRODUCT_NAME),
l10n_util::GetStringUTF16(
IDS_ACCESSIBILITY_PERMISSION_DIALOG_OPEN_BUTTON),
- base::SysNSStringToUTF16(@"ChromeRemoteDesktopHost"));
+ bundleName);
break;
case WizardPage::SCREEN_RECORDING:
_instructionText.stringValue = l10n_util::GetNSStringF(
@@ -179,7 +310,7 @@
l10n_util::GetStringUTF16(IDS_PRODUCT_NAME),
l10n_util::GetStringUTF16(
IDS_SCREEN_RECORDING_PERMISSION_DIALOG_OPEN_BUTTON),
- base::SysNSStringToUTF16(@"ChromeRemoteDesktopHost"));
+ bundleName);
break;
case WizardPage::ALL_SET:
_instructionText.stringValue =
@@ -221,42 +352,110 @@
}
}
+- (void)advanceToNextPage {
+ DCHECK(_hasPermission);
+ switch (_page) {
+ case WizardPage::ACCESSIBILITY:
+ _page = WizardPage::SCREEN_RECORDING;
+ break;
+ case WizardPage::SCREEN_RECORDING:
+ _page = WizardPage::ALL_SET;
+ [self updateUI];
+ return;
+ default:
+ NOTREACHED();
+ }
+
+ // Kick off a permission check for the new page. The UI will be updated only
+ // after the result comes back.
+ _hasPermission = NO;
+ _autoAdvance = YES;
+ [self requestPermissionCheck:base::TimeDelta()];
+}
+
+- (void)requestPermissionCheck:(base::TimeDelta)delay {
+ DCHECK(!_hasPermission);
+ switch (_page) {
+ case WizardPage::ACCESSIBILITY:
+ _impl->CheckAccessibilityPermission(delay);
+ break;
+ case WizardPage::SCREEN_RECORDING:
+ _impl->CheckScreenRecordingPermission(delay);
+ return;
+ default:
+ NOTREACHED();
+ }
+}
+
+- (void)launchSystemPreferences {
+ switch (_page) {
+ case WizardPage::ACCESSIBILITY:
+ // Launch the Security and Preferences pane with Accessibility selected.
+ [[NSWorkspace sharedWorkspace]
+ openURL:[NSURL URLWithString:
+ @"x-apple.systempreferences:com.apple."
+ @"preference.security?Privacy_Accessibility"]];
+ break;
+ case WizardPage::SCREEN_RECORDING:
+ // Launch the Security and Preferences pane with Screen Recording
+ // selected.
+ [[NSWorkspace sharedWorkspace]
+ openURL:[NSURL URLWithString:
+ @"x-apple.systempreferences:com.apple."
+ @"preference.security?Privacy_ScreenCapture"]];
+ return;
+ default:
+ NOTREACHED();
+ }
+}
+
+- (void)onPermissionCheckResult:(bool)result {
+ if (_cancelled)
+ return;
+
+ _hasPermission = result;
+
+ if (_hasPermission && _autoAdvance) {
+ // Skip showing the "Next" button, and immediately kick off a permission
+ // check for the next page, if any.
+ [self advanceToNextPage];
+ return;
+ }
+
+ // Update the whole UI, not just the "Next" button, in case a different page
+ // was previously shown.
+ [self updateUI];
+
+ if (!_hasPermission) {
+ // Permission denied, so turn off auto-advance for this page, and present
+ // the dialog to the user if needed. After the user grants this permission,
+ // they should be able to click "Next" to acknowledge and advance the
+ // wizard. Note that, if all permissions are granted, the user will not
+ // see the wizard at all (not even the ALL_SET page). A dialog is only
+ // shown when a permission-check fails.
+ _autoAdvance = NO;
+ if (![self window].visible) {
+ [self presentWindow];
+ }
+
+ // Keep polling until permission is granted.
+ [self requestPermissionCheck:kPollingInterval];
+ }
+}
+
+- (void)presentWindow {
+ [self.window makeKeyAndOrderFront:NSApp];
+ [self.window center];
+ [self showWindow:nil];
+}
+
@end
namespace remoting {
namespace mac {
-class PermissionWizard::Impl {
- public:
- Impl() = default;
- ~Impl();
-
- void Start();
-
- private:
- PermissionWizardController* window_controller_ = nil;
-};
-
-PermissionWizard::Impl::~Impl() {
- // PermissionWizardController is responsible for releasing itself in its
- // windowWillClose: method.
- [window_controller_ hide];
- window_controller_ = nil;
-}
-
-void PermissionWizard::Impl::Start() {
- NSWindow* window =
- [[[NSWindow alloc] initWithContentRect:ui::kWindowSizeDeterminedLater
- styleMask:NSWindowStyleMaskTitled
- backing:NSBackingStoreBuffered
- defer:NO] autorelease];
- window_controller_ =
- [[PermissionWizardController alloc] initWithWindow:window];
- [window_controller_ initializeWindow];
- [window_controller_ showWindow:nil];
-}
-
-PermissionWizard::PermissionWizard() = default;
+PermissionWizard::PermissionWizard(std::unique_ptr<Delegate> checker)
+ : impl_(std::make_unique<PermissionWizard::Impl>(std::move(checker))) {}
PermissionWizard::~PermissionWizard() {
ui_task_runner_->DeleteSoon(FROM_HERE, impl_.release());
@@ -265,10 +464,8 @@
void PermissionWizard::Start(
scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner) {
ui_task_runner_ = ui_task_runner;
- impl_ = std::make_unique<PermissionWizard::Impl>();
- ui_task_runner->PostTask(FROM_HERE,
- base::BindOnce(&PermissionWizard::Impl::Start,
- base::Unretained(impl_.get())));
+ ui_task_runner->PostTask(
+ FROM_HERE, base::BindOnce(&Impl::Start, base::Unretained(impl_.get())));
}
} // namespace mac
