Prompt for granting permission to access USB devices.

Prompt for granting permission to access USB devices.

extensions::DevicePermissionsPrompt may be implemented by an embedder
of the extensions system to provide a way to prompt the user for
permission to access a set of USB devices. This is expandable to other
device types. Implementations for Chrome using the Views framework
(for Chrome OS, Linux and Windows) and Cocoa (for OS X) are provided
in this change.

Screenshots are attached to bug 420317.

BUG=352720, 420317
Committed: https://crrev.com/43a9462cd259b1a3c6cc961b28518ba6aac261e9
Cr-Commit-Position: refs/heads/master@{#299983}

[Reilly Grant (use Gerrit), 2014-10-14 23:13:17]

reillyg@chromium.org changed reviewers:
+ avi@chromium.org

[Reilly Grant (use Gerrit), 2014-10-15 00:15:29]

Finnur, please look at the DevicePermissionsPrompt infrastructure and Views
implementation. Avi, please look at the Cocoa side of things. I'll need someone
to resave this XIB with 10.8.x before I submit.

[Avi (use Gerrit), 2014-10-15 01:57:47]

Cocoa side LGTM with nits. Ping me tomorrow while I'm in the office and I'll
help with the xib.

https://codereview.chromium.org/633793002/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/device_permissions_dialog_controller.h
(right):

https://codereview.chromium.org/633793002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/device_permissions_dialog_controller.h:1: //
Copyright (c) 2014 The Chromium Authors. All rights reserved.
No (c) in new files.

https://codereview.chromium.org/633793002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/device_permissions_dialog_controller.h:12:
#import "chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h"
alphabetical order

https://codereview.chromium.org/633793002/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/device_permissions_dialog_controller.mm
(right):

https://codereview.chromium.org/633793002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/device_permissions_dialog_controller.mm:1: //
Copyright (c) 2014 The Chromium Authors. All rights reserved.
No (c)

https://codereview.chromium.org/633793002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/device_permissions_dialog_controller.mm:58:
delegate_ = NULL;
nullptr

https://codereview.chromium.org/633793002/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/device_permissions_view_controller.h
(right):

https://codereview.chromium.org/633793002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/device_permissions_view_controller.h:1: //
Copyright (c) 2014 The Chromium Authors. All rights reserved.
yadda yadda (c)

https://codereview.chromium.org/633793002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/device_permissions_view_controller.h:33:
prompt;
hahaha... I'd rather you not align on the colons and get that all in on one
line, if you can.

https://codereview.chromium.org/633793002/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/device_permissions_view_controller.mm
(right):

https://codereview.chromium.org/633793002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/device_permissions_view_controller.mm:16:
@end
You don't need a class extension if you have nothing to add to the class.

https://codereview.chromium.org/633793002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/device_permissions_view_controller.mm:38:
enumerateIndexesUsingBlock:^void(NSUInteger index, BOOL* stop) {
Drop the "void"; you shouldn't need to explicitly specify the return type.

[Finnur, 2014-10-15 10:01:26]

https://codereview.chromium.org/633793002/diff/40001/extensions/browser/api/DEPS
File extensions/browser/api/DEPS (right):

https://codereview.chromium.org/633793002/diff/40001/extensions/browser/api/D...
extensions/browser/api/DEPS:3: ]
I'm not sure what all these DEPS changes are for. Need I worry?

https://codereview.chromium.org/633793002/diff/40001/extensions/browser/api/d...
File extensions/browser/api/device_permissions_prompt.cc (right):

https://codereview.chromium.org/633793002/diff/40001/extensions/browser/api/d...
extensions/browser/api/device_permissions_prompt.cc:41: }
None of the class members get initialized?

https://codereview.chromium.org/633793002/diff/40001/extensions/browser/api/d...
extensions/browser/api/device_permissions_prompt.cc:93: }
nit: Single-line if, no braces. Here and everywhere below.

https://codereview.chromium.org/633793002/diff/40001/extensions/browser/api/d...
extensions/browser/api/device_permissions_prompt.cc:95:
std::vector<scoped_refptr<UsbDevice>> devices;
style: s/>>/> >/

https://codereview.chromium.org/633793002/diff/40001/extensions/browser/api/d...
extensions/browser/api/device_permissions_prompt.cc:172: :
web_contents_(web_contents) {
delegate_(NULL) ?
Well, I guess we're using nullptr now.

https://codereview.chromium.org/633793002/diff/40001/extensions/browser/api/d...
File extensions/browser/api/device_permissions_prompt.h (right):

https://codereview.chromium.org/633793002/diff/40001/extensions/browser/api/d...
extensions/browser/api/device_permissions_prompt.h:36: base::string16 name,
nit: const base::string16& ?

https://codereview.chromium.org/633793002/diff/40001/extensions/browser/api/d...
extensions/browser/api/device_permissions_prompt.h:97: void DoDeviceQuery();
You are not a big fan of comments, I've noticed... :)

https://codereview.chromium.org/633793002/diff/40001/extensions/browser/api/d...
extensions/browser/api/device_permissions_prompt.h:132:
std::vector<device::UsbDeviceFilter> filters);
Can this be const std::vector<device::UsbDeviceFilter>& ?

https://codereview.chromium.org/633793002/diff/40001/extensions/extensions_st...
File extensions/extensions_strings.grd (right):

https://codereview.chromium.org/633793002/diff/40001/extensions/extensions_st...
extensions/extensions_strings.grd:28: </if>
Looks like this should be reverted.

[Reilly Grant (use Gerrit), 2014-10-15 19:15:34]

https://codereview.chromium.org/633793002/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/device_permissions_dialog_controller.h
(right):

https://codereview.chromium.org/633793002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/device_permissions_dialog_controller.h:1: //
Copyright (c) 2014 The Chromium Authors. All rights reserved.
On 2014/10/15 01:57:46, Avi wrote:
> No (c) in new files.

Done.

https://codereview.chromium.org/633793002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/device_permissions_dialog_controller.h:12:
#import "chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h"
On 2014/10/15 01:57:46, Avi wrote:
> alphabetical order

It's amazing how easily I can forget how the alphabet works.

https://codereview.chromium.org/633793002/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/device_permissions_dialog_controller.mm
(right):

https://codereview.chromium.org/633793002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/device_permissions_dialog_controller.mm:1: //
Copyright (c) 2014 The Chromium Authors. All rights reserved.
On 2014/10/15 01:57:46, Avi wrote:
> No (c)

Done.

https://codereview.chromium.org/633793002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/device_permissions_dialog_controller.mm:58:
delegate_ = NULL;
On 2014/10/15 01:57:46, Avi wrote:
> nullptr

Done.

https://codereview.chromium.org/633793002/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/device_permissions_view_controller.h
(right):

https://codereview.chromium.org/633793002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/device_permissions_view_controller.h:1: //
Copyright (c) 2014 The Chromium Authors. All rights reserved.
On 2014/10/15 01:57:46, Avi wrote:
> yadda yadda (c)

Done.

https://codereview.chromium.org/633793002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/device_permissions_view_controller.h:33:
prompt;
On 2014/10/15 01:57:46, Avi wrote:
> hahaha... I'd rather you not align on the colons and get that all in on one
> line, if you can.

Ick, I think clang-format did this when I wasn't looking. :(

https://codereview.chromium.org/633793002/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/device_permissions_view_controller.mm
(right):

https://codereview.chromium.org/633793002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/device_permissions_view_controller.mm:16:
@end
On 2014/10/15 01:57:47, Avi wrote:
> You don't need a class extension if you have nothing to add to the class.

Oops. I put that there in case I needed it.

https://codereview.chromium.org/633793002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/device_permissions_view_controller.mm:38:
enumerateIndexesUsingBlock:^void(NSUInteger index, BOOL* stop) {
On 2014/10/15 01:57:47, Avi wrote:
> Drop the "void"; you shouldn't need to explicitly specify the return type.

Done.

https://codereview.chromium.org/633793002/diff/40001/extensions/browser/api/DEPS
File extensions/browser/api/DEPS (right):

https://codereview.chromium.org/633793002/diff/40001/extensions/browser/api/D...
extensions/browser/api/DEPS:3: ]
On 2014/10/15 10:01:25, Finnur wrote:
> I'm not sure what all these DEPS changes are for. Need I worry?

Nothing sneaky. Adding a dependency on device/usb at the extensions/browser/api
level just negated the need for it further down. I can keep the duplicated
dependencies if people wish.

https://codereview.chromium.org/633793002/diff/40001/extensions/browser/api/d...
File extensions/browser/api/device_permissions_prompt.cc (right):

https://codereview.chromium.org/633793002/diff/40001/extensions/browser/api/d...
extensions/browser/api/device_permissions_prompt.cc:41: }
On 2014/10/15 10:01:25, Finnur wrote:
> None of the class members get initialized?

Done.

https://codereview.chromium.org/633793002/diff/40001/extensions/browser/api/d...
extensions/browser/api/device_permissions_prompt.cc:93: }
On 2014/10/15 10:01:26, Finnur wrote:
> nit: Single-line if, no braces. Here and everywhere below.

"In general, curly braces are not required for single-line statements, but they
are allowed if you like them," quoth the coding standard. I like them.

https://codereview.chromium.org/633793002/diff/40001/extensions/browser/api/d...
extensions/browser/api/device_permissions_prompt.cc:95:
std::vector<scoped_refptr<UsbDevice>> devices;
On 2014/10/15 10:01:25, Finnur wrote:
> style: s/>>/> >/

clang-format has been updated to prefer the no-space style.

https://codereview.chromium.org/633793002/diff/40001/extensions/browser/api/d...
extensions/browser/api/device_permissions_prompt.cc:172: :
web_contents_(web_contents) {
On 2014/10/15 10:01:26, Finnur wrote:
> delegate_(NULL) ?
> Well, I guess we're using nullptr now.

Done.

https://codereview.chromium.org/633793002/diff/40001/extensions/browser/api/d...
File extensions/browser/api/device_permissions_prompt.h (right):

https://codereview.chromium.org/633793002/diff/40001/extensions/browser/api/d...
extensions/browser/api/device_permissions_prompt.h:36: base::string16 name,
On 2014/10/15 10:01:26, Finnur wrote:
> nit: const base::string16& ?

Done.

https://codereview.chromium.org/633793002/diff/40001/extensions/browser/api/d...
extensions/browser/api/device_permissions_prompt.h:97: void DoDeviceQuery();
On 2014/10/15 10:01:26, Finnur wrote:
> You are not a big fan of comments, I've noticed... :)

My previous employer's coding standard went a bit too far in the other direction
with regards to comments. I'm suffering a bit of whiplash and still trying to
figure out what's expected in Chrome.

https://codereview.chromium.org/633793002/diff/40001/extensions/browser/api/d...
extensions/browser/api/device_permissions_prompt.h:132:
std::vector<device::UsbDeviceFilter> filters);
On 2014/10/15 10:01:26, Finnur wrote:
> Can this be const std::vector<device::UsbDeviceFilter>& ?

Done.

https://codereview.chromium.org/633793002/diff/40001/extensions/extensions_st...
File extensions/extensions_strings.grd (right):

https://codereview.chromium.org/633793002/diff/40001/extensions/extensions_st...
extensions/extensions_strings.grd:28: </if>
On 2014/10/15 10:01:26, Finnur wrote:
> Looks like this should be reverted.

Done.

[Finnur, 2014-10-16 09:20:21]

LGTM, one nit.

https://codereview.chromium.org/633793002/diff/40001/extensions/browser/api/DEPS
File extensions/browser/api/DEPS (right):

https://codereview.chromium.org/633793002/diff/40001/extensions/browser/api/D...
extensions/browser/api/DEPS:3: ]
Nope.

https://codereview.chromium.org/633793002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/device_permissions_dialog_view.h
(right):

https://codereview.chromium.org/633793002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/device_permissions_dialog_view.h:17: //
Displays an device permissions selector prompt as a modal dialog constrained
nit: s/an/a/

[Reilly Grant (use Gerrit), 2014-10-16 17:30:30]

reillyg@chromium.org changed reviewers:
+ thestig@chromium.org

[Reilly Grant (use Gerrit), 2014-10-16 17:31:02]

Lei, please take a look at chrome/chrome_nibs.gyp.

[Lei Zhang, 2014-10-16 21:08:57]

lgtm stamp

[Reilly Grant (use Gerrit), 2014-10-16 21:11:54]

The CQ bit was checked by reillyg@chromium.org

[commit-bot: I haz the power, 2014-10-16 21:13:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/633793002/100001

[commit-bot: I haz the power, 2014-10-16 21:59:42]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-10-16 22:00:27]

Patchset 6 (id:??) landed as
https://crrev.com/43a9462cd259b1a3c6cc961b28518ba6aac261e9
Cr-Commit-Position: refs/heads/master@{#299983}