Bluetooth API Permission Dialogs.

Bluetooth API Permission Dialogs.

This CL introduces two new concepts:
- API permissions that cannot be found in the manifest (and therefore
  need not be in the optional permissions section to be optionally
  requested)
- optional API permission requests that have associated data

BUG=119473
TBR=sky@chromium.org


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170514

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170809

[Matt Perry, 2012-11-27 21:35:32]

nice change overall

https://codereview.chromium.org/11413099/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/permissions/permissions_api_helpers.cc
(right):

https://codereview.chromium.org/11413099/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/permissions/permissions_api_helpers.cc:64: //
processing here.
how easy would it be to do the following processing in the renderer instead:
  permissions: ["tabs", {"bluetooth": address}]
  -->
  permissions: [{"tabs": true}, {"bluetooth": address}]
?

That way you always have an array of objects.

https://codereview.chromium.org/11413099/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/permissions/permissions_api_helpers.cc:81:
BluetoothDevicePermission *permission =
nit: * before space.

https://codereview.chromium.org/11413099/diff/1/chrome/common/extensions/perm...
File chrome/common/extensions/permissions/bluetooth_device_permission.cc
(right):

https://codereview.chromium.org/11413099/diff/1/chrome/common/extensions/perm...
chrome/common/extensions/permissions/bluetooth_device_permission.cc:41: const
std::string &devices_string) {
& before space

https://codereview.chromium.org/11413099/diff/1/chrome/common/extensions/perm...
chrome/common/extensions/permissions/bluetooth_device_permission.cc:89: if
(device_identifier.length() == 0) {
nit: device_identifier.empty()

https://codereview.chromium.org/11413099/diff/1/chrome/common/extensions/perm...
File chrome/common/extensions/permissions/bluetooth_device_permission.h (right):

https://codereview.chromium.org/11413099/diff/1/chrome/common/extensions/perm...
chrome/common/extensions/permissions/bluetooth_device_permission.h:26: struct
CheckParam : APIPermission::CheckParam {
does this need to use public inheritance?

https://codereview.chromium.org/11413099/diff/1/chrome/common/extensions/perm...
chrome/common/extensions/permissions/bluetooth_device_permission.h:38: void
AddDevicesFromString(const std::string &devices_string);
nit: & before space

https://codereview.chromium.org/11413099/diff/1/chrome/common/extensions/perm...
chrome/common/extensions/permissions/bluetooth_device_permission.h:61:
std::set<std::string> allowed_devices_;
shouldn't this be requested_devices instead?

[bryeung, 2012-11-28 21:55:04]

https://codereview.chromium.org/11413099/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/permissions/permissions_api_helpers.cc
(right):

https://codereview.chromium.org/11413099/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/permissions/permissions_api_helpers.cc:64: //
processing here.
On 2012/11/27 21:35:32, Matt Perry wrote:
> how easy would it be to do the following processing in the renderer instead:
>   permissions: ["tabs", {"bluetooth": address}]
>   -->
>   permissions: [{"tabs": true}, {"bluetooth": address}]
> ?
> 
> That way you always have an array of objects.

Discussed offline: this can't be easily done because it would make the
Permissions objects in getAll and the events return objects...which would break
existing code.

https://codereview.chromium.org/11413099/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/permissions/permissions_api_helpers.cc:81:
BluetoothDevicePermission *permission =
On 2012/11/27 21:35:32, Matt Perry wrote:
> nit: * before space.

Gah!  I thought I had finally broken that old habit of mine.  I guess not.

https://codereview.chromium.org/11413099/diff/1/chrome/common/extensions/perm...
File chrome/common/extensions/permissions/bluetooth_device_permission.cc
(right):

https://codereview.chromium.org/11413099/diff/1/chrome/common/extensions/perm...
chrome/common/extensions/permissions/bluetooth_device_permission.cc:41: const
std::string &devices_string) {
On 2012/11/27 21:35:32, Matt Perry wrote:
> & before space

Done.

https://codereview.chromium.org/11413099/diff/1/chrome/common/extensions/perm...
chrome/common/extensions/permissions/bluetooth_device_permission.cc:89: if
(device_identifier.length() == 0) {
On 2012/11/27 21:35:32, Matt Perry wrote:
> nit: device_identifier.empty()

done.  (Also fixed braces.)

https://codereview.chromium.org/11413099/diff/1/chrome/common/extensions/perm...
File chrome/common/extensions/permissions/bluetooth_device_permission.h (right):

https://codereview.chromium.org/11413099/diff/1/chrome/common/extensions/perm...
chrome/common/extensions/permissions/bluetooth_device_permission.h:26: struct
CheckParam : APIPermission::CheckParam {
On 2012/11/27 21:35:32, Matt Perry wrote:
> does this need to use public inheritance?

Yes: we do casts from APIPermission::CheckParam to
BluetoothDevicePermission::CheckParam.

https://codereview.chromium.org/11413099/diff/1/chrome/common/extensions/perm...
chrome/common/extensions/permissions/bluetooth_device_permission.h:38: void
AddDevicesFromString(const std::string &devices_string);
On 2012/11/27 21:35:32, Matt Perry wrote:
> nit: & before space

Done.

https://codereview.chromium.org/11413099/diff/1/chrome/common/extensions/perm...
chrome/common/extensions/permissions/bluetooth_device_permission.h:61:
std::set<std::string> allowed_devices_;
On 2012/11/27 21:35:32, Matt Perry wrote:
> shouldn't this be requested_devices instead?

Depends.  If it's a request, then yes.  But if this is representing a stored
permission, then they're allowed.

I've just renamed to devices_, okay?

[Matt Perry, 2012-11-28 21:59:00]

lgtm

https://codereview.chromium.org/11413099/diff/1/chrome/common/extensions/perm...
File chrome/common/extensions/permissions/bluetooth_device_permission.h (right):

https://codereview.chromium.org/11413099/diff/1/chrome/common/extensions/perm...
chrome/common/extensions/permissions/bluetooth_device_permission.h:61:
std::set<std::string> allowed_devices_;
On 2012/11/28 21:55:05, bryeung wrote:
> On 2012/11/27 21:35:32, Matt Perry wrote:
> > shouldn't this be requested_devices instead?
> 
> Depends.  If it's a request, then yes.  But if this is representing a stored
> permission, then they're allowed.
> 
> I've just renamed to devices_, okay?

Sounds good.

(though it seems the latest upload doesn't have that change)

[bryeung, 2012-11-29 15:38:00]

On 2012/11/28 21:59:00, Matt Perry wrote:
> Sounds good.
> 
> (though it seems the latest upload doesn't have that change)

Oops!  Good catch.  Not sure how I lost those changes :-/

[commit-bot: I haz the power, 2012-11-29 15:38:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryeung@chromium.org/11413099/7005

[commit-bot: I haz the power, 2012-11-29 15:38:47]

Failed to apply patch for
chrome/common/extensions/permissions/permission_message.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/common/extensions/permissions/permission_message.h
  Hunk #1 FAILED at 53.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/common/extensions/permissions/permission_message.h.rej

Patch:       chrome/common/extensions/permissions/permission_message.h
Index: chrome/common/extensions/permissions/permission_message.h
diff --git a/chrome/common/extensions/permissions/permission_message.h
b/chrome/common/extensions/permissions/permission_message.h
index
743efa65dd8ec4f801250cbd7ce736cf436bd8bb..3a4a3bd269358f363932911937bb7735cae2fbbf
100644
--- a/chrome/common/extensions/permissions/permission_message.h
+++ b/chrome/common/extensions/permissions/permission_message.h
@@ -53,6 +53,7 @@ class PermissionMessage {
     kSocketSpecificHosts,
     kBluetooth,
     kUsb,
+    kBluetoothDevice,
     kEnumBoundary
   };

[commit-bot: I haz the power, 2012-11-29 15:45:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryeung@chromium.org/11413099/3007

[commit-bot: I haz the power, 2012-11-29 16:31:20]

Retried try job too often on linux_chromeos for step(s) unit_tests

[bryeung, 2012-11-29 19:06:48]

Matt: I had to update a test that I missed.  I doubt you'll have a problem with
the change, but let me know if you'd like me to do something different.  In the
meantime, I'll send this back to the CQ.

[commit-bot: I haz the power, 2012-11-29 19:07:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryeung@chromium.org/11413099/9028

[Matt Perry, 2012-11-29 19:18:48]

lgtm

[commit-bot: I haz the power, 2012-11-29 20:51:08]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2012-11-30 15:39:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryeung@chromium.org/11413099/8011

[commit-bot: I haz the power, 2012-11-30 19:55:07]

Change committed as 170514

[commit-bot: I haz the power, 2012-12-03 16:12:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryeung@chromium.org/11413099/3051

[commit-bot: I haz the power, 2012-12-03 18:14:03]

Retried try job too often on win_rel for step(s) ash_unittests

[commit-bot: I haz the power, 2012-12-03 18:50:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryeung@chromium.org/11413099/3051

[commit-bot: I haz the power, 2012-12-03 21:05:26]

Change committed as 170809