Store Bluetooth permissions in website settings

Store Bluetooth permissions in website settings

This patch adds a new class BluetoothPermissionContext
that stores a list of chosen objects for chooser-based
permissions in website settings. It is similar to
UsbChooserContext.

BUG=577953

[juncai, 2016-01-05 22:41:25]

Description was changed from

==========
Store Bluetooth permissions in website settings

BUG=535633, 535635
==========

to

==========
Store Bluetooth permissions in website settings

This patch adds a new class BluetoothChooserContext that stores a list of chosen
objects for chooser-based permissions in website settings. It is similar to
UsbChooserContext.

BUG=535633, 535635
==========

[juncai, 2016-01-05 22:41:45]

Description was changed from

==========
Store Bluetooth permissions in website settings

This patch adds a new class BluetoothChooserContext that stores a list of chosen
objects for chooser-based permissions in website settings. It is similar to
UsbChooserContext.

BUG=535633, 535635
==========

to

==========
Store Bluetooth permissions in website settings

This patch adds a new class BluetoothChooserContext
that stores a list of chosen objects for chooser-based
permissions in website settings. It is similar to
UsbChooserContext.

BUG=535633, 535635
==========

[juncai, 2016-01-05 22:44:49]

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

[juncai, 2016-01-05 22:44:50]

Please review this patch.

[Reilly Grant (use Gerrit), 2016-01-05 23:08:51]

https://codereview.chromium.org/1560263002/diff/1/chrome/browser/profiles/chr...
File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
(right):

https://codereview.chromium.org/1560263002/diff/1/chrome/browser/profiles/chr...
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:342:
BluetoothChooserContextFactory::GetInstance();
Bluetooth is implemented on Android so it should be outside the #ifdef.

https://codereview.chromium.org/1560263002/diff/1/chrome/browser/ui/bluetooth...
File chrome/browser/ui/bluetooth/bluetooth_chooser_context.h (right):

https://codereview.chromium.org/1560263002/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_context.h:22: const std::string&
device_id);
Like UsbChooserContext this should also store the human-readable device name so
that it can be displayed in the UI that lets users audit and revoke permissions.

Also, are you sure that device_id is persistent across restarts?

[juncai, 2016-01-05 23:56:15]

https://codereview.chromium.org/1560263002/diff/1/chrome/browser/profiles/chr...
File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
(right):

https://codereview.chromium.org/1560263002/diff/1/chrome/browser/profiles/chr...
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:342:
BluetoothChooserContextFactory::GetInstance();
On 2016/01/05 23:08:51, Reilly Grant wrote:
> Bluetooth is implemented on Android so it should be outside the #ifdef.

Done.

https://codereview.chromium.org/1560263002/diff/1/chrome/browser/ui/bluetooth...
File chrome/browser/ui/bluetooth/bluetooth_chooser_context.h (right):

https://codereview.chromium.org/1560263002/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_context.h:22: const std::string&
device_id);
On 2016/01/05 23:08:51, Reilly Grant wrote:
> Like UsbChooserContext this should also store the human-readable device name
so
> that it can be displayed in the UI that lets users audit and revoke
permissions.

Done

> 
> Also, are you sure that device_id is persistent across restarts?

I did some testing to power on/off the bluetooth devices and the device_id
doesn't change.

[Reilly Grant (use Gerrit), 2016-01-06 00:16:52]

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

[Reilly Grant (use Gerrit), 2016-01-06 00:16:52]

Aside from my comments this looks good. Jeffrey should review to make sure this
policy makes sense for Bluetooth. Based on our previous conversation I believe
it does.

https://codereview.chromium.org/1560263002/diff/40001/chrome/browser/ui/bluet...
File chrome/browser/ui/bluetooth/bluetooth_chooser_context.h (right):

https://codereview.chromium.org/1560263002/diff/40001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_context.h:30: const
base::string16& device_name,
If |device_id| identifies the device then |device_name| is unnecessary. This is
how the USB version works.

https://codereview.chromium.org/1560263002/diff/40001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_context.h:37: const
base::string16& device_name,
Same here.

[juncai, 2016-01-06 17:14:01]

https://codereview.chromium.org/1560263002/diff/40001/chrome/browser/ui/bluet...
File chrome/browser/ui/bluetooth/bluetooth_chooser_context.h (right):

https://codereview.chromium.org/1560263002/diff/40001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_context.h:30: const
base::string16& device_name,
On 2016/01/06 00:16:52, Reilly Grant wrote:
> If |device_id| identifies the device then |device_name| is unnecessary. This
is
> how the USB version works.

Done.

https://codereview.chromium.org/1560263002/diff/40001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_context.h:37: const
base::string16& device_name,
On 2016/01/06 00:16:52, Reilly Grant wrote:
> Same here.

Done.

[Reilly Grant (use Gerrit), 2016-01-06 18:44:13]

https://codereview.chromium.org/1560263002/diff/40001/chrome/browser/ui/bluet...
File chrome/browser/ui/bluetooth/bluetooth_chooser_context.cc (right):

https://codereview.chromium.org/1560263002/diff/40001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_context.cc:48:
device_dict->SetString(kDeviceNameKey, device_name);
No, don't remove this one! We want to store the device name. We just don't need
to check it in the methods below.

[Jeffrey Yasskin, 2016-01-06 19:10:55]

https://codereview.chromium.org/1560263002/diff/60001/chrome/browser/ui/bluet...
File chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.h (right):

https://codereview.chromium.org/1560263002/diff/60001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.h:13: #include
"url/gurl.h"
I think you can forward-declare GURL too.

https://codereview.chromium.org/1560263002/diff/60001/chrome/browser/ui/bluet...
File chrome/browser/ui/bluetooth/bluetooth_chooser_context.cc (right):

https://codereview.chromium.org/1560263002/diff/60001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_context.cc:10: #include
"chrome/browser/profiles/profile.h"
I don't think you're using this #include.

https://codereview.chromium.org/1560263002/diff/60001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_context.cc:69: return
object.size() == 1 && object.HasKey(kDeviceIdKey);
Also check that the device ID is a string?

https://codereview.chromium.org/1560263002/diff/60001/chrome/browser/ui/bluet...
File chrome/browser/ui/bluetooth/bluetooth_chooser_context.h (right):

https://codereview.chromium.org/1560263002/diff/60001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_context.h:13: class
BluetoothChooserContext : public ChooserContextBase {
Is "ChooserContext" the right class name here? IIUC, this is the
storage/persistence system for bluetooth permissions, while I'd expect a
"chooser context" to be the context in which a chooser appears: maybe the origin
or tab.

https://codereview.chromium.org/1560263002/diff/60001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_context.h:19: // |device_id|.
"known as |device_id|" doesn't really tell us which ID you're using. Specify
that it's either BluetoothDevice::GetIdentifier() or
BluetoothDevice::GetAddress(). (It's probably the address, since that's what the
rest of the system uses for now.)

https://codereview.chromium.org/1560263002/diff/60001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_context.h:19: // |device_id|.
We also need to store the set of allowed service UUIDs for each origin and
device.
https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto...
starts recording this set, but you can record an empty set for now, just so
there's space for it.

https://codereview.chromium.org/1560263002/diff/60001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_context.h:32: bool
HasDevicePermission(const GURL& requesting_origin,
This'll probably need to return the set of services.

[Jeffrey Yasskin, 2016-01-06 19:23:45]

https://codereview.chromium.org/1560263002/diff/60001/chrome/browser/ui/bluet...
File chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.cc (right):

https://codereview.chromium.org/1560263002/diff/60001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.cc:54:
chooser_context->GrantDevicePermission(origin_, embedding_origin,
After looking at your Android patch, I think this belongs in
BluetoothDispatcherHost::FinishClosingChooser() rather than in each chooser's UI
implementation. Be compatible with Giovanni's patch that adds the origin to the
RequestDeviceSession:
https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto...

[ortuno, 2016-01-11 20:27:58]

ortuno@chromium.org changed reviewers:
+ ortuno@chromium.org

[ortuno, 2016-01-11 20:27:58]

The issues this CL points to are not related to the purpose of this change. Can
you open another issue or find one that describes why you are implementing this?

[juncai, 2016-01-11 21:02:49]

Description was changed from

==========
Store Bluetooth permissions in website settings

This patch adds a new class BluetoothChooserContext
that stores a list of chosen objects for chooser-based
permissions in website settings. It is similar to
UsbChooserContext.

BUG=535633, 535635
==========

to

==========
Store Bluetooth permissions in website settings

This patch adds a new class BluetoothPermissionContext
that stores a list of chosen objects for chooser-based
permissions in website settings. It is similar to
UsbChooserContext.

BUG=535633, 535635
==========

[juncai, 2016-01-11 21:55:16]

https://codereview.chromium.org/1560263002/diff/40001/chrome/browser/ui/bluet...
File chrome/browser/ui/bluetooth/bluetooth_chooser_context.cc (right):

https://codereview.chromium.org/1560263002/diff/40001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_context.cc:48:
device_dict->SetString(kDeviceNameKey, device_name);
On 2016/01/06 18:44:13, Reilly Grant wrote:
> No, don't remove this one! We want to store the device name. We just don't
need
> to check it in the methods below.

Done.

https://codereview.chromium.org/1560263002/diff/60001/chrome/browser/ui/bluet...
File chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.cc (right):

https://codereview.chromium.org/1560263002/diff/60001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.cc:54:
chooser_context->GrantDevicePermission(origin_, embedding_origin,
On 2016/01/06 19:23:45, Jeffrey Yasskin wrote:
> After looking at your Android patch, I think this belongs in
> BluetoothDispatcherHost::FinishClosingChooser() rather than in each chooser's
UI
> implementation. Be compatible with Giovanni's patch that adds the origin to
the
> RequestDeviceSession:
>
https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto...

Done.

https://codereview.chromium.org/1560263002/diff/60001/chrome/browser/ui/bluet...
File chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.h (right):

https://codereview.chromium.org/1560263002/diff/60001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.h:13: #include
"url/gurl.h"
On 2016/01/06 19:10:55, Jeffrey Yasskin wrote:
> I think you can forward-declare GURL too.

This include is removed since the related code has been moved to
bluetooth_dispatcher_host.cc

https://codereview.chromium.org/1560263002/diff/60001/chrome/browser/ui/bluet...
File chrome/browser/ui/bluetooth/bluetooth_chooser_context.cc (right):

https://codereview.chromium.org/1560263002/diff/60001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_context.cc:10: #include
"chrome/browser/profiles/profile.h"
On 2016/01/06 19:10:55, Jeffrey Yasskin wrote:
> I don't think you're using this #include.

Done.

https://codereview.chromium.org/1560263002/diff/60001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_context.cc:69: return
object.size() == 1 && object.HasKey(kDeviceIdKey);
On 2016/01/06 19:10:55, Jeffrey Yasskin wrote:
> Also check that the device ID is a string?

Done.

https://codereview.chromium.org/1560263002/diff/60001/chrome/browser/ui/bluet...
File chrome/browser/ui/bluetooth/bluetooth_chooser_context.h (right):

https://codereview.chromium.org/1560263002/diff/60001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_context.h:13: class
BluetoothChooserContext : public ChooserContextBase {
On 2016/01/06 19:10:55, Jeffrey Yasskin wrote:
> Is "ChooserContext" the right class name here? IIUC, this is the
> storage/persistence system for bluetooth permissions, while I'd expect a
> "chooser context" to be the context in which a chooser appears: maybe the
origin
> or tab.

Renamed it to class BluetoothPermissionContext.

Done.

https://codereview.chromium.org/1560263002/diff/60001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_context.h:19: // |device_id|.
On 2016/01/06 19:10:55, Jeffrey Yasskin wrote:
> "known as |device_id|" doesn't really tell us which ID you're using. Specify
> that it's either BluetoothDevice::GetIdentifier() or
> BluetoothDevice::GetAddress(). (It's probably the address, since that's what
the
> rest of the system uses for now.)

Done.

https://codereview.chromium.org/1560263002/diff/60001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_context.h:19: // |device_id|.
On 2016/01/06 19:10:55, Jeffrey Yasskin wrote:
> We also need to store the set of allowed service UUIDs for each origin and
> device.
>
https://codereview.chromium.org/1502663003/diff/100001/content/browser/blueto...
> starts recording this set, but you can record an empty set for now, just so
> there's space for it.

Done.

https://codereview.chromium.org/1560263002/diff/60001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_context.h:32: bool
HasDevicePermission(const GURL& requesting_origin,
On 2016/01/06 19:10:55, Jeffrey Yasskin wrote:
> This'll probably need to return the set of services.

Done.

[Jeffrey Yasskin, 2016-01-15 01:27:57]

Description was changed from

==========
Store Bluetooth permissions in website settings

This patch adds a new class BluetoothPermissionContext
that stores a list of chosen objects for chooser-based
permissions in website settings. It is similar to
UsbChooserContext.

BUG=535633, 535635
==========

to

==========
Store Bluetooth permissions in website settings

This patch adds a new class BluetoothPermissionContext
that stores a list of chosen objects for chooser-based
permissions in website settings. It is similar to
UsbChooserContext.

BUG=577953
==========

[Jeffrey Yasskin, 2016-01-15 01:28:16]

On 2016/01/11 20:27:58, ortuno wrote:
> The issues this CL points to are not related to the purpose of this change.
Can
> you open another issue or find one that describes why you are implementing
this?

I've updated the bug to https://crbug.com/577953

[juncai, 2016-01-19 21:03:05]

ping jyasskin@, :).

[ortuno, 2016-01-19 21:09:31]

https://codereview.chromium.org/1560263002/diff/160001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1560263002/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1203: GURL
embedding_origin =
Can you add a TODO that points to an issue to change the
ChooserPermissionManager to take an Origin instead? Something like

TODO(...): Change to GetLastCommitedOrigin when ChooserPermissionManager takes
an Origin. https://crbug.com/...

https://codereview.chromium.org/1560263002/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1204:
session->web_contents->GetMainFrame()->GetLastCommittedURL().GetOrigin();
Could the WebContents object be deleted before FinishClosingChooser is called?

[juncai, 2016-01-19 22:21:37]

https://codereview.chromium.org/1560263002/diff/160001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1560263002/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1203: GURL
embedding_origin =
Modified ChooserPermissionManager to take url::Origin instead of URL as
parameter.

Done.

https://codereview.chromium.org/1560263002/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1204:
session->web_contents->GetMainFrame()->GetLastCommittedURL().GetOrigin();
store chooser_permission_manager pointer instead of web_contents pointer into
the session object.

Done.

[Jeffrey Yasskin, 2016-01-21 00:32:30]

Sorry it's taking me so long to get through this. I'm getting stuck on how the
overall design should work, and figuring out how other bits of the system work
and whether we should try to fix the tangential bits that don't work the way I
think they should. Here's a round of comments, but there will probably be more
on the files I haven't gotten to yet.

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/permiss...
File chrome/browser/permissions/chooser_permission_manager.cc (right):

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/permiss...
chrome/browser/permissions/chooser_permission_manager.cc:20:
BluetoothPermissionContext* permission_context =
Something called "ChooserPermissionManager" can't be this hard-wired to
Bluetooth. Assuming this will eventually support USB too, and that it can't fold
into the existing PermissionManager, can we do something similar to
https://code.google.com/p/chromium/codesearch/#chromium/src/content/public/br...
with a ChooserPermissionType enum (or even re-use PermissionType)?

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/permiss...
File chrome/browser/permissions/chooser_permission_manager.h (right):

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/permiss...
chrome/browser/permissions/chooser_permission_manager.h:9: #include
"base/memory/weak_ptr.h"
You're not using this #include.

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/permiss...
chrome/browser/permissions/chooser_permission_manager.h:15: class
ChooserPermissionManager : public KeyedService,
I believe we generally call this class "ChromeChooserPermissionManager", to
distinguish it from the content:: one. Ah, but you're matching
PermissionManager, which also doesn't prefix its Chrome version with "Chrome".

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/permiss...
File chrome/browser/permissions/chooser_permission_manager_factory.cc (right):

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/permiss...
chrome/browser/permissions/chooser_permission_manager_factory.cc:31:
DependsOn(HostContentSettingsMapFactory::GetInstance());
The ChooserPermissionManager depends on the BluetoothPermissionContextFactory,
so you need to reflect that here.

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/permiss...
chrome/browser/permissions/chooser_permission_manager_factory.cc:32: for
(KeyedServiceBaseFactory* factory : PermissionContext::GetFactories())
Does the ChooserPermissionManager really depend on all the non-Chooser
PermissionContexts?

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/profile...
chrome/browser/profiles/profile_impl.cc:50: #include
"chrome/browser/permissions/chooser_permission_manager.h"
I think you don't need this #include.

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/ui/blue...
File chrome/browser/ui/bluetooth/bluetooth_permission_context.cc (right):

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_permission_context.cc:58:
scoped_ptr<base::ListValue> device_uuids_list(new base::ListValue());
Please break up functions like this a little more with blank lines and comments
saying what each section is doing.

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_permission_context.cc:59:
device::BluetoothDevice::UUIDList uuid_list = device->GetUUIDs();
The domain doesn't automatically get permission to access all the UUIDs the
device includes. It only gets the ones that were listed in the requestDevice()
call, available as the union of a
BluetoothDispatcherHost::RequestDeviceSession's filters and optional_services
fields.

I think this function needs to accept that union as an argument, but maybe it
should accept the filters and optional_services and compute the union itself.
This is specified at
https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetooth-requestdevice step
9.

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_permission_context.cc:117: return
object.size() == 3 && object.HasKey(kDeviceAddressKey) &&
Could you comment that this check needs to be backward-compatible with
preferences written by previous versions of Chrome? There are no such
preferences right now, but I want to warn future maintainers.

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_permission_context.cc:118:
object.GetString(kDeviceAddressKey, &device_address) &&
Pass nullptr as the second argument to GetString or GetList to check the type of
the value without copying it:
https://code.google.com/p/chromium/codesearch/#chromium/src/base/values.h&l=284

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/ui/blue...
File chrome/browser/ui/bluetooth/bluetooth_permission_context.h (right):

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_permission_context.h:5: #ifndef
CHROME_BROWSER_UI_BLUETOOTH_BLUETOOTH_PERMISSION_CONTEXT_H_
This couple files don't seem like UI code, so they should go in
chrome/browser/bluetooth instead of chrome/browser/ui/bluetooth.

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_permission_context.h:46:
std::vector<std::string> HasDevicePermission(
A non-boolean function shouldn't be named Has*. This could be
GetDevicePermission() or GetAllowedServices(), depending on whether we need to
expose it to BluetoothDispatcherHost in content/. We probably will need to
expose it, and in that case it needs a signature compatible with other choosers'
data so the result can go through the content::ChooserPermissionManager
interface. So, probably GetDevicePermission(), returning a
base::DictionaryValue.

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_permission_context.h:51: void
OnGetAdapter(scoped_refptr<device::BluetoothAdapter> adapter);
I believe this can be private.

[juncai, 2016-01-25 18:53:32]

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/permiss...
File chrome/browser/permissions/chooser_permission_manager.cc (right):

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/permiss...
chrome/browser/permissions/chooser_permission_manager.cc:20:
BluetoothPermissionContext* permission_context =
On 2016/01/21 00:32:30, Jeffrey Yasskin wrote:
> Something called "ChooserPermissionManager" can't be this hard-wired to
> Bluetooth. Assuming this will eventually support USB too, and that it can't
fold
> into the existing PermissionManager, can we do something similar to
>
https://code.google.com/p/chromium/codesearch/#chromium/src/content/public/br...
> with a ChooserPermissionType enum (or even re-use PermissionType)? 

Done.

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/permiss...
File chrome/browser/permissions/chooser_permission_manager.h (right):

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/permiss...
chrome/browser/permissions/chooser_permission_manager.h:9: #include
"base/memory/weak_ptr.h"
On 2016/01/21 00:32:30, Jeffrey Yasskin wrote:
> You're not using this #include.

Done.

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/permiss...
chrome/browser/permissions/chooser_permission_manager.h:15: class
ChooserPermissionManager : public KeyedService,
On 2016/01/21 00:32:30, Jeffrey Yasskin wrote:
> I believe we generally call this class "ChromeChooserPermissionManager", to
> distinguish it from the content:: one. Ah, but you're matching
> PermissionManager, which also doesn't prefix its Chrome version with "Chrome".

Yes, :).

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/permiss...
File chrome/browser/permissions/chooser_permission_manager_factory.cc (right):

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/permiss...
chrome/browser/permissions/chooser_permission_manager_factory.cc:31:
DependsOn(HostContentSettingsMapFactory::GetInstance());
On 2016/01/21 00:32:30, Jeffrey Yasskin wrote:
> The ChooserPermissionManager depends on the BluetoothPermissionContextFactory,
> so you need to reflect that here.

Done.

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/permiss...
chrome/browser/permissions/chooser_permission_manager_factory.cc:32: for
(KeyedServiceBaseFactory* factory : PermissionContext::GetFactories())
On 2016/01/21 00:32:30, Jeffrey Yasskin wrote:
> Does the ChooserPermissionManager really depend on all the non-Chooser
> PermissionContexts?

Done.

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/profile...
chrome/browser/profiles/profile_impl.cc:50: #include
"chrome/browser/permissions/chooser_permission_manager.h"
On 2016/01/21 00:32:30, Jeffrey Yasskin wrote:
> I think you don't need this #include.

Without this include, line:
return ChooserPermissionManagerFactory::GetForProfile(this);
will get compile error:
error: cannot initialize return object of type
'content::ChooserPermissionManager *' with an rvalue of type
'ChooserPermissionManager *'

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/ui/blue...
File chrome/browser/ui/bluetooth/bluetooth_permission_context.cc (right):

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_permission_context.cc:58:
scoped_ptr<base::ListValue> device_uuids_list(new base::ListValue());
On 2016/01/21 00:32:30, Jeffrey Yasskin wrote:
> Please break up functions like this a little more with blank lines and
comments
> saying what each section is doing.

Done.

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_permission_context.cc:59:
device::BluetoothDevice::UUIDList uuid_list = device->GetUUIDs();
On 2016/01/21 00:32:30, Jeffrey Yasskin wrote:
> The domain doesn't automatically get permission to access all the UUIDs the
> device includes. It only gets the ones that were listed in the requestDevice()
> call, available as the union of a
> BluetoothDispatcherHost::RequestDeviceSession's filters and optional_services
> fields.
> 
> I think this function needs to accept that union as an argument, but maybe it
> should accept the filters and optional_services and compute the union itself.
> This is specified at
> https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetooth-requestdevice
step
> 9.

Done.

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_permission_context.cc:117: return
object.size() == 3 && object.HasKey(kDeviceAddressKey) &&
On 2016/01/21 00:32:30, Jeffrey Yasskin wrote:
> Could you comment that this check needs to be backward-compatible with
> preferences written by previous versions of Chrome? There are no such
> preferences right now, but I want to warn future maintainers.

Done.

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_permission_context.cc:118:
object.GetString(kDeviceAddressKey, &device_address) &&
On 2016/01/21 00:32:30, Jeffrey Yasskin wrote:
> Pass nullptr as the second argument to GetString or GetList to check the type
of
> the value without copying it:
>
https://code.google.com/p/chromium/codesearch/#chromium/src/base/values.h&l=284

Done.

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/ui/blue...
File chrome/browser/ui/bluetooth/bluetooth_permission_context.h (right):

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_permission_context.h:5: #ifndef
CHROME_BROWSER_UI_BLUETOOTH_BLUETOOTH_PERMISSION_CONTEXT_H_
On 2016/01/21 00:32:30, Jeffrey Yasskin wrote:
> This couple files don't seem like UI code, so they should go in
> chrome/browser/bluetooth instead of chrome/browser/ui/bluetooth.

Done.

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_permission_context.h:46:
std::vector<std::string> HasDevicePermission(
On 2016/01/21 00:32:30, Jeffrey Yasskin wrote:
> A non-boolean function shouldn't be named Has*. This could be
> GetDevicePermission() or GetAllowedServices(), depending on whether we need to
> expose it to BluetoothDispatcherHost in content/. We probably will need to
> expose it, and in that case it needs a signature compatible with other
choosers'
> data so the result can go through the content::ChooserPermissionManager
> interface. So, probably GetDevicePermission(), returning a
> base::DictionaryValue.

This function is removed since it is not used by other function.

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_permission_context.h:51: void
OnGetAdapter(scoped_refptr<device::BluetoothAdapter> adapter);
On 2016/01/21 00:32:30, Jeffrey Yasskin wrote:
> I believe this can be private.

This function is removed since the permission object is constructed by
BluetoothDispatcherHost.

[Reilly Grant (use Gerrit), 2016-01-25 19:08:38]

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/profile...
chrome/browser/profiles/profile_impl.cc:50: #include
"chrome/browser/permissions/chooser_permission_manager.h"
On 2016/01/25 18:53:32, juncai wrote:
> On 2016/01/21 00:32:30, Jeffrey Yasskin wrote:
> > I think you don't need this #include.
> 
> Without this include, line:
> return ChooserPermissionManagerFactory::GetForProfile(this);
> will get compile error:
> error: cannot initialize return object of type
> 'content::ChooserPermissionManager *' with an rvalue of type
> 'ChooserPermissionManager *'

ChooserPermissionManagerFactory::GetForProfile should be modified to return a
content::ChooserPermissionManager. That is, move the cast from
::ChooserPermissionManager to content::ChooserPermissionManager into
chooser_permission_manager_factory.cc so that the include is only necessary
there.

[juncai, 2016-01-25 21:51:59]

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/1560263002/diff/180001/chrome/browser/profile...
chrome/browser/profiles/profile_impl.cc:50: #include
"chrome/browser/permissions/chooser_permission_manager.h"
On 2016/01/25 19:08:38, Reilly Grant wrote:
> On 2016/01/25 18:53:32, juncai wrote:
> > On 2016/01/21 00:32:30, Jeffrey Yasskin wrote:
> > > I think you don't need this #include.
> > 
> > Without this include, line:
> > return ChooserPermissionManagerFactory::GetForProfile(this);
> > will get compile error:
> > error: cannot initialize return object of type
> > 'content::ChooserPermissionManager *' with an rvalue of type
> > 'ChooserPermissionManager *'
> 
> ChooserPermissionManagerFactory::GetForProfile should be modified to return a
> content::ChooserPermissionManager. That is, move the cast from
> ::ChooserPermissionManager to content::ChooserPermissionManager into
> chooser_permission_manager_factory.cc so that the include is only necessary
> there.

Done.