bluetooth: Move requestDevice to mojo

bluetooth: Move requestDevice to mojo

Changes in order of importance and complexity:
1. Move most of the logic of BluetoothDispatcherHost::OnRequestDeviceImpl to a new
   class called BluetoothDeviceProvider.
2. Introduce BluetoothAdapterFactoryWrapper to manage adapters and their observers.
   This class allows us to set the adapter from tests. It will be removed once we
   no longer need to swap adapters to change an adapter's behavior.
3. Fix BluetoothBlacklist and BluetoothAllowedDevicesMap to use mojo types.
4. Remove content/common/bluetooth and content/renderer/bluetooth/bluetooth_dispatcher
   \o/

BUG=508771
Committed: https://crrev.com/b6374dd8ae9d5d1b4d99bfbebb45e9adddc45a24
Cr-Commit-Position: refs/heads/master@{#396369}

[ortuno, 2016-05-11 18:18:58]

Description was changed from

==========
bluetooth: (WIP) Request Device

BUG=
==========

to

==========
bluetooth: Move requestDevice to mojo

Changes in order of importance and complexity:
1. Move most of the logic of BluetoothDispatcherHost::OnRequestDeviceImpl to a
new
   class called BluetoothDeviceProvider.
2. Introduce BluetoothAdapterFactoryWrapper to manage adapters and their
observers.
   This class allows us to set the adapter from tests. It will be removed once
we
   no longer need to swap adapters to change an adapter's behavior.
3. Fix BluetoothBlacklist and BluetoothAllowedDevicesMap to use mojo types.
4. Remove content/common/bluetooth and
content/renderer/bluetooth/bluetooth_dispatcher
   \o/


BUG=508771
==========

[ortuno, 2016-05-11 19:34:05]

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

[ortuno, 2016-05-11 19:34:06]

jyasskin: PTAL

[Jeffrey Yasskin, 2016-05-13 04:42:00]

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_adapter_factory_wrapper.h (right):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_adapter_factory_wrapper.h:24: class
CONTENT_EXPORT BluetoothAdapterFactoryWrapper final {
"FactoryWrapper" is also an awkward suffix. Still thinking about better
alternatives.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_adapter_factory_wrapper.h:34: // acquired
the adapter in the past we add |observer| as an observer to that
Be consistent about "we" vs "it".

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_adapter_factory_wrapper.h:46:
device::BluetoothAdapter* GetAdapter(
Would it make sense to pass the adapter back in the AcquireAdapter callback?

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_adapter_factory_wrapper.h:80: int
scan_duration_;
This should be a base::TimeDelta.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_allowed_devices_map.h (right):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map.h:37: // unique origins
generate the same hash, unique origins are not supported.
Does "not supported" mean they'll CHECK-fail, return "", or something else?

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc
(right):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:35: const
blink::mojom::WebBluetoothRequestDeviceOptionsPtr& GetEmptyOptions() {
For tests, you can just access the field directly:

class BluetoothAllowedDevicesMapTest : public testing::Test {
 protected:
  BluetoothAllowedDevicesMapTest() 
    : empty_options_(blink::mojom::WebBluetoothRequestDeviceOptions::New()) {
  }

  const blink::mojom::WebBluetoothRequestDeviceOptionsPtr empty_options_;
};

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_blacklist_unittest.cc (right):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_blacklist_unittest.cc:249:
BluetoothUUID("0000").canonical_value());
Maybe write a local Canonicalize() function to get these back on a single line.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_device_provider.cc (right):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_provider.cc:56: return
(filter->name.is_null() && filter->name_prefix.is_null() &&
I'd rather a series of 'if' statements, but this is ok, and way more correct
than the old implementation.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_provider.cc:123: new
device::BluetoothDiscoveryFilter(
This can now be

  auto discovery_filter = base::MakeUnique<device::BluetoothDiscoveryFilter>(
      device::BluetoothDiscoveryFilter::TRANSPORT_DUAL);

No need to change it, but it's a bit shorter.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_provider.cc:207:
CHECK(!HasEmptyOrInvalidFilter(options->filters));
We shouldn't CHECK that the renderer is behaving well, since that crashes the
whole browser on failure. Just kill the renderer. You can either
ReceivedBadMessage(render_frame_host_->GetProcess()) or find a way to do that
with the mojo pipe.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_provider.cc:212: if
(BluetoothBlacklist::Get().IsExcluded(options_->filters)) {
The comment on this function says that the UUIDs must be valid, but
HasEmptyOrInvalidFilter() doesn't check for UUID validity. We'll be able to get
rid of the check once https://crbug.com/606446 is fixed, but we should be sure
not to risk security problems until then.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_device_provider.h (right):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_provider.h:27: // Class that
interacts with a chooser and starts a bluetooth discovery session.
Can this class manage more than one successive GetDevice() call, or does it need
to be re-instantiated for each one?

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_provider.h:28: class
BluetoothDeviceProvider final {
I'm not totally happy with this name, but I don't have a great suggestion. Maybe
BluetoothDeviceChooserManager? or ...Controller?

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_provider.h:38: // |scan_duration| is
how long will a discovery session be active for.
Nit: "is how long a discovery session will be active." "for" at the end is ok
but unnecessary.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_provider.h:41: int scan_duration);
Lengths of time should be base::TimeDelta so that the units are built in:
https://code.google.com/p/chromium/codesearch/#chromium/src/base/time/time.h

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_provider.h:44: // This function
performs the following checks before starting a discovery
If this function is called again before the previous call runs the success or
error callback, what happens?

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_provider.h:63: // Adds a device to
the chooser.
Must this be called only while a GetDevice() is active?

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_provider.h:66: // Stops the current
discovery session and notifies the chooser
It's odd that this class would need something external to tell it that the
adapter has been turned on or off, but would be able to stop a discovery session
on its own.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_provider.h:117: // The last discovery
session to be started.
So this doesn't get nulled out when no discovery session is active?

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.h (left):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.h:5: #ifndef
CONTENT_BROWSER_BLUETOOTH_BLUETOOTH_DISPATCHER_HOST_H_
Any idea why this is showing as a modification instead of a deletion in
Rietveld?

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_metrics.cc (right):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.cc:62: filter->services.size());
I'm worried about these unchecked dereferences. It's true that they're not a
security concern since they'd just dereference null, but nothing in the mojom
interface guarantees that the renderer is always sending non-null values here.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (left):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:200: ClearState();
Is removing this going to bring https://crbug.com/604318 back?

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:662:
base::Passed(options->Clone()), callback),
Instead of cloning the options, would it make sense to have GetDevice() pass
them on to the success callback?

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:706:
blink::mojom::WebBluetoothDevicePtr device_ptr =
I'd probably create this just before it's used for the first time, so it's
clearer that it doesn't accumulate data through the whole function.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:710:
GetBluetoothAdapterFactoryWrapper()->GetAdapter(this)->GetDevice(
Consider wrapping the "GetBluetoothAdapterFactoryWrapper()->GetAdapter(this)"
into a "GetAdapter()" member function.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:717:
device_provider_.reset();
Can you reset this at the beginning of the function so we can't accidentally
return without destroying it?

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:750: // Errors are
recorded by the BluetoothDeviceProvider that called this
Or just "by *device_provider_."?

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.h (right):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.h:191: CacheQueryResult
QueryCacheForDevice(const std::string& device_id);
Comment this function like the other two.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2735:
BluetoothAdapterFactoryWrapper*
You can just return a BluetoothAdapterFactoryWrapper& since it's guaranteed to
be present.

https://codereview.chromium.org/1922923002/diff/200001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/bluetooth/WebRequestDeviceOptions.h
(right):

https://codereview.chromium.org/1922923002/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/WebRequestDeviceOptions.h:25:

Remove this blank line.

https://codereview.chromium.org/1922923002/diff/200001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/1922923002/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:78:
array<WebBluetoothScanFilter> filters;
I don't suppose there's a way to say that the elements in the array are never
null? That'd save a bunch of code in bluetooth_blacklist_unittest.cc
initializing members.

[ortuno, 2016-05-13 20:04:23]

Patchset #12 (id:220001) has been deleted

[ortuno, 2016-05-13 20:09:43]

Patchset #12 (id:240001) has been deleted

[ortuno, 2016-05-13 20:11:18]

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_adapter_factory_wrapper.h (right):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_adapter_factory_wrapper.h:24: class
CONTENT_EXPORT BluetoothAdapterFactoryWrapper final {
On 2016/05/13 at 04:41:58, Jeffrey Yasskin wrote:
> "FactoryWrapper" is also an awkward suffix. Still thinking about better
alternatives.

BluetoothAdapterProvider? BluetoothAdapterManager?

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_adapter_factory_wrapper.h:34: // acquired
the adapter in the past we add |observer| as an observer to that
On 2016/05/13 at 04:41:58, Jeffrey Yasskin wrote:
> Be consistent about "we" vs "it".

Done.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_adapter_factory_wrapper.h:46:
device::BluetoothAdapter* GetAdapter(
On 2016/05/13 at 04:41:58, Jeffrey Yasskin wrote:
> Would it make sense to pass the adapter back in the AcquireAdapter callback?

Done.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_adapter_factory_wrapper.h:80: int
scan_duration_;
On 2016/05/13 at 04:41:58, Jeffrey Yasskin wrote:
> This should be a base::TimeDelta.

Done.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_allowed_devices_map.h (right):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map.h:37: // unique origins
generate the same hash, unique origins are not supported.
On 2016/05/13 at 04:41:58, Jeffrey Yasskin wrote:
> Does "not supported" mean they'll CHECK-fail, return "", or something else?

Added comment.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc
(right):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:35: const
blink::mojom::WebBluetoothRequestDeviceOptionsPtr& GetEmptyOptions() {
On 2016/05/13 at 04:41:58, Jeffrey Yasskin wrote:
> For tests, you can just access the field directly:
> 
> class BluetoothAllowedDevicesMapTest : public testing::Test {
>  protected:
>   BluetoothAllowedDevicesMapTest() 
>     : empty_options_(blink::mojom::WebBluetoothRequestDeviceOptions::New()) {
>   }
> 
>   const blink::mojom::WebBluetoothRequestDeviceOptionsPtr empty_options_;
> };

Done.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_blacklist_unittest.cc (right):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_blacklist_unittest.cc:249:
BluetoothUUID("0000").canonical_value());
On 2016/05/13 at 04:41:58, Jeffrey Yasskin wrote:
> Maybe write a local Canonicalize() function to get these back on a single
line.

Done.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_device_provider.cc (right):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_provider.cc:56: return
(filter->name.is_null() && filter->name_prefix.is_null() &&
On 2016/05/13 at 04:41:58, Jeffrey Yasskin wrote:
> I'd rather a series of 'if' statements, but this is ok, and way more correct
than the old implementation.

Changed to ifs.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_provider.cc:123: new
device::BluetoothDiscoveryFilter(
On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> This can now be
> 
>   auto discovery_filter = base::MakeUnique<device::BluetoothDiscoveryFilter>(
>       device::BluetoothDiscoveryFilter::TRANSPORT_DUAL);
> 
> No need to change it, but it's a bit shorter.

Nice. Done.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_provider.cc:207:
CHECK(!HasEmptyOrInvalidFilter(options->filters));
On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> We shouldn't CHECK that the renderer is behaving well, since that crashes the
whole browser on failure. Just kill the renderer. You can either
ReceivedBadMessage(render_frame_host_->GetProcess()) or find a way to do that
with the mojo pipe.

Done. This required passing in the WebBluetoothService though :(

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_provider.cc:212: if
(BluetoothBlacklist::Get().IsExcluded(options_->filters)) {
On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> The comment on this function says that the UUIDs must be valid, but
HasEmptyOrInvalidFilter() doesn't check for UUID validity. We'll be able to get
rid of the check once https://crbug.com/606446 is fixed, but we should be sure
not to risk security problems until then.

Added a check for UUID validity to IsEmptyOrInvalidFilter and also added
HasInvalidOptionalServices function to check for UUID validity there.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_device_provider.h (right):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_provider.h:27: // Class that
interacts with a chooser and starts a bluetooth discovery session.
On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> Can this class manage more than one successive GetDevice() call, or does it
need to be re-instantiated for each one?

Added comment.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_provider.h:28: class
BluetoothDeviceProvider final {
On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> I'm not totally happy with this name, but I don't have a great suggestion.
Maybe BluetoothDeviceChooserManager? or ...Controller?

Changed to BluetoothDeviceChooserController.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_provider.h:38: // |scan_duration| is
how long will a discovery session be active for.
On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> Nit: "is how long a discovery session will be active." "for" at the end is ok
but unnecessary.

Done.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_provider.h:41: int scan_duration);
On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> Lengths of time should be base::TimeDelta so that the units are built in:
https://code.google.com/p/chromium/codesearch/#chromium/src/base/time/time.h

Done.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_provider.h:44: // This function
performs the following checks before starting a discovery
On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> If this function is called again before the previous call runs the success or
error callback, what happens?

It will DCHECK. Added comment.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_provider.h:63: // Adds a device to
the chooser.
On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> Must this be called only while a GetDevice() is active?

Added comment.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_provider.h:66: // Stops the current
discovery session and notifies the chooser
On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> It's odd that this class would need something external to tell it that the
adapter has been turned on or off, but would be able to stop a discovery session
on its own.

I agree it's a bit weird. The way I think about it is that this class is allowed
to talk to the adapter, but the adapter is not allowed to talk to this class.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_provider.h:117: // The last discovery
session to be started.
On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> So this doesn't get nulled out when no discovery session is active?

We could null it when we stop it but we would also have to null it when the
adapter stops it for some reason and for that we need to override another
Observer function. Do you think we should do it?

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.h (left):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.h:5: #ifndef
CONTENT_BROWSER_BLUETOOTH_BLUETOOTH_DISPATCHER_HOST_H_
On 2016/05/13 at 04:41:58, Jeffrey Yasskin wrote:
> Any idea why this is showing as a modification instead of a deletion in
Rietveld?

Maybe because I used this file as a base for bluetooth_adapter_factory_wrapper?
It shows as deletion on the new UI.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_metrics.cc (right):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.cc:62: filter->services.size());
On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> I'm worried about these unchecked dereferences. It's true that they're not a
security concern since they'd just dereference null, but nothing in the mojom
interface guarantees that the renderer is always sending non-null values here.

Mojo actually guarantees that members won't be null if you marked the type as
non-nullable in the mojom file i.e. no question mark:

// .mojom
array<string?> nullable

array<string> non_nullable

For the first one mojo will construct an ArrayValidateParams[1] object with the
in_element_is_nullable param set to true and for the second one it will
construct an ArrayValidateParams object with is_element_is_nullable set to
false. Then it calls ValidateArray which ends up calling ValidateElement with
returns true or false depending on ArrayValidateParams, which causes the pipe to
close.

You can follow some of that code in [2], which is the generated code for
array<WebBluetoothRemoteGATTCharacteristic>.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/thir...

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (left):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:200: ClearState();
On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> Is removing this going to bring https://crbug.com/604318 back?

Noup! That bug happened because the adapter in BluetoothDispatcherHost was
destroyed before notify sessions in WebBluetoothServiceImpl were destroyed. Now
that we destroy the adapter after we stop notify session in ClearState(), we
shouldn't see that error. Now that I think about it we do destroy the adapter
before we stop notify session in the destructor. Fixed that.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:662:
base::Passed(options->Clone()), callback),
On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> Instead of cloning the options, would it make sense to have GetDevice() pass
them on to the success callback?

Done.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:706:
blink::mojom::WebBluetoothDevicePtr device_ptr =
On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> I'd probably create this just before it's used for the first time, so it's
clearer that it doesn't accumulate data through the whole function.

Done.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:710:
GetBluetoothAdapterFactoryWrapper()->GetAdapter(this)->GetDevice(
On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> Consider wrapping the "GetBluetoothAdapterFactoryWrapper()->GetAdapter(this)"
into a "GetAdapter()" member function.

Done.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:717:
device_provider_.reset();
On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> Can you reset this at the beginning of the function so we can't accidentally
return without destroying it?

Done.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:750: // Errors are
recorded by the BluetoothDeviceProvider that called this
On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> Or just "by *device_provider_."?

Done.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.h (right):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.h:191: CacheQueryResult
QueryCacheForDevice(const std::string& device_id);
On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> Comment this function like the other two.

Done.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2735:
BluetoothAdapterFactoryWrapper*
On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> You can just return a BluetoothAdapterFactoryWrapper& since it's guaranteed to
be present.

Done.

https://codereview.chromium.org/1922923002/diff/200001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/bluetooth/WebRequestDeviceOptions.h
(right):

https://codereview.chromium.org/1922923002/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/WebRequestDeviceOptions.h:25:

On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> Remove this blank line.

Done.

https://codereview.chromium.org/1922923002/diff/200001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/1922923002/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:78:
array<WebBluetoothScanFilter> filters;
On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> I don't suppose there's a way to say that the elements in the array are never
null? That'd save a bunch of code in bluetooth_blacklist_unittest.cc
initializing members.

There is! It's the way it's written now haha. If we allowed null members it
would look like so: array<WebBluetoothScanFilter?>. According to rockot as long
as you don't add the question mark mojo guarantees the value will never be null.

[ortuno, 2016-05-13 20:11:25]

The CQ bit was checked by ortuno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-13 20:11:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1922923002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1922923002/260001

[commit-bot: I haz the power, 2016-05-13 20:15:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-13 20:15:40]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[ortuno, 2016-05-13 21:25:51]

The CQ bit was checked by ortuno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-13 21:26:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1922923002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1922923002/280001

[Jeffrey Yasskin, 2016-05-13 21:36:56]

LGTM, thanks!

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_adapter_factory_wrapper.h (right):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_adapter_factory_wrapper.h:24: class
CONTENT_EXPORT BluetoothAdapterFactoryWrapper final {
On 2016/05/13 20:11:17, ortuno wrote:
> On 2016/05/13 at 04:41:58, Jeffrey Yasskin wrote:
> > "FactoryWrapper" is also an awkward suffix. Still thinking about better
> alternatives.
> 
> BluetoothAdapterProvider? BluetoothAdapterManager?

It does actually wrap the factory. We could add this sort of feature to the
factory itself, but probably not in this change.

I think keep the name as you have it for this CL, especially since this class
should go away once we make the adapters more programmable.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_metrics.cc (right):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.cc:62: filter->services.size());
On 2016/05/13 20:11:18, ortuno wrote:
> On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> > I'm worried about these unchecked dereferences. It's true that they're not a
> security concern since they'd just dereference null, but nothing in the mojom
> interface guarantees that the renderer is always sending non-null values here.
> 
> Mojo actually guarantees that members won't be null if you marked the type as
> non-nullable in the mojom file i.e. no question mark:
> 
> // .mojom
> array<string?> nullable
> 
> array<string> non_nullable
> 
> For the first one mojo will construct an ArrayValidateParams[1] object with
the
> in_element_is_nullable param set to true and for the second one it will
> construct an ArrayValidateParams object with is_element_is_nullable set to
> false. Then it calls ValidateArray which ends up calling ValidateElement with
> returns true or false depending on ArrayValidateParams, which causes the pipe
to
> close.
> 
> You can follow some of that code in [2], which is the generated code for
> array<WebBluetoothRemoteGATTCharacteristic>.
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi...
> [2]
>
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/thir...

Oh, I see. It just doesn't change the type of the elements, so it's all a
runtime guarantee instead of a compile-time check. :( Oh well.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2735:
BluetoothAdapterFactoryWrapper*
On 2016/05/13 20:11:18, ortuno wrote:
> On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> > You can just return a BluetoothAdapterFactoryWrapper& since it's guaranteed
to
> be present.
> 
> Done.

You did this in WebBluetoothServiceImpl but not here.

https://codereview.chromium.org/1922923002/diff/280001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_device_chooser_controller.h (right):

https://codereview.chromium.org/1922923002/diff/280001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.h:78: // Stops the
current discovery session and notifies the chooser
On 2016/05/13 20:11:17, ortuno wrote:
> On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> > It's odd that this class would need something external to tell it that the
> adapter has been turned on or off, but would be able to stop a discovery
session
> on its own.
> 
> I agree it's a bit weird. The way I think about it is that this class is
allowed
> to talk to the adapter, but the adapter is not allowed to talk to this class.

Mhmm. Another way to do it would be to just make this class observe the adapter,
but I do see the benefit of having that all go through the
WebBluetoothServiceImpl. No need to change anything.

https://codereview.chromium.org/1922923002/diff/280001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.h:130: // The last
discovery session to be started.
On 2016/05/13 20:11:17, ortuno wrote:
> On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> > So this doesn't get nulled out when no discovery session is active?
> 
> We could null it when we stop it but we would also have to null it when the
> adapter stops it for some reason and for that we need to override another
> Observer function. Do you think we should do it?

Looking at the implementation, you do actually null it out any time we call
StopDiscoverySession() (because that's what std::move() does on a unique_ptr),
so the only ambiguity is when the system stops discovery. We should be reporting
that to the chooser anyway, so we can probably clear discovery_session_ at the
same time.

However, I don't think you need to do that now. Add a TODO for
https://crbug.com/611852?

https://codereview.chromium.org/1922923002/diff/280001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/1922923002/diff/280001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:750: // Errors are
recorded the *device_chooser_controller_.
Nit: "recorded by the"

https://codereview.chromium.org/1922923002/diff/280001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.h (right):

https://codereview.chromium.org/1922923002/diff/280001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.h:125: // Should only be
run after we've acquired an adapter from
This comment is now redundant with the parameter.

[ortuno, 2016-05-13 22:14:39]

Thanks!

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_adapter_factory_wrapper.h (right):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_adapter_factory_wrapper.h:24: class
CONTENT_EXPORT BluetoothAdapterFactoryWrapper final {
On 2016/05/13 at 21:36:55, Jeffrey Yasskin wrote:
> On 2016/05/13 20:11:17, ortuno wrote:
> > On 2016/05/13 at 04:41:58, Jeffrey Yasskin wrote:
> > > "FactoryWrapper" is also an awkward suffix. Still thinking about better
> > alternatives.
> > 
> > BluetoothAdapterProvider? BluetoothAdapterManager?
> 
> It does actually wrap the factory. We could add this sort of feature to the
factory itself, but probably not in this change.
> 
> I think keep the name as you have it for this CL, especially since this class
should go away once we make the adapters more programmable.

Ack.

https://codereview.chromium.org/1922923002/diff/200001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1922923002/diff/200001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2735:
BluetoothAdapterFactoryWrapper*
On 2016/05/13 at 21:36:55, Jeffrey Yasskin wrote:
> On 2016/05/13 20:11:18, ortuno wrote:
> > On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> > > You can just return a BluetoothAdapterFactoryWrapper& since it's
guaranteed to
> > be present.
> > 
> > Done.
> 
> You did this in WebBluetoothServiceImpl but not here.

Oops. Done.

https://codereview.chromium.org/1922923002/diff/280001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_device_chooser_controller.h (right):

https://codereview.chromium.org/1922923002/diff/280001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.h:78: // Stops the
current discovery session and notifies the chooser
On 2016/05/13 at 21:36:55, Jeffrey Yasskin wrote:
> On 2016/05/13 20:11:17, ortuno wrote:
> > On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> > > It's odd that this class would need something external to tell it that the
> > adapter has been turned on or off, but would be able to stop a discovery
session
> > on its own.
> > 
> > I agree it's a bit weird. The way I think about it is that this class is
allowed
> > to talk to the adapter, but the adapter is not allowed to talk to this
class.
> 
> Mhmm. Another way to do it would be to just make this class observe the
adapter, but I do see the benefit of having that all go through the
WebBluetoothServiceImpl. No need to change anything.

Ack.

https://codereview.chromium.org/1922923002/diff/280001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.h:130: // The last
discovery session to be started.
On 2016/05/13 at 21:36:55, Jeffrey Yasskin wrote:
> On 2016/05/13 20:11:17, ortuno wrote:
> > On 2016/05/13 at 04:41:59, Jeffrey Yasskin wrote:
> > > So this doesn't get nulled out when no discovery session is active?
> > 
> > We could null it when we stop it but we would also have to null it when the
> > adapter stops it for some reason and for that we need to override another
> > Observer function. Do you think we should do it?
> 
> Looking at the implementation, you do actually null it out any time we call
StopDiscoverySession() (because that's what std::move() does on a unique_ptr),
so the only ambiguity is when the system stops discovery. We should be reporting
that to the chooser anyway, so we can probably clear discovery_session_ at the
same time.
> 
> However, I don't think you need to do that now. Add a TODO for
https://crbug.com/611852?

Done. Thanks for opening the issue!

https://codereview.chromium.org/1922923002/diff/280001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/1922923002/diff/280001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:750: // Errors are
recorded the *device_chooser_controller_.
On 2016/05/13 at 21:36:55, Jeffrey Yasskin wrote:
> Nit: "recorded by the"

Done.

https://codereview.chromium.org/1922923002/diff/280001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.h (right):

https://codereview.chromium.org/1922923002/diff/280001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.h:125: // Should only be
run after we've acquired an adapter from
On 2016/05/13 at 21:36:55, Jeffrey Yasskin wrote:
> This comment is now redundant with the parameter.

Done.

[ortuno, 2016-05-13 22:14:43]

The CQ bit was checked by ortuno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-13 22:15:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1922923002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1922923002/300001

[ortuno, 2016-05-13 22:17:14]

ortuno@chromium.org changed reviewers:
+ palmer@chromium.org, rockot@chromium.org

[ortuno, 2016-05-13 22:17:15]

palmer: PTAL at content/common and
third_party/WebKit/public/platform/modules/bluetooth/
rockot: PTAL at mojo bits in
third_party/WebKit/public/platform/modules/bluetooth/, content/renderer/ and
content/brower/bluetooth/web_bluetooth_service_impl.h/cc

[commit-bot: I haz the power, 2016-05-14 00:52:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-14 00:52:31]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-05-19 17:59:10]

Sorry I dropped this somehow. LGTM

[ortuno, 2016-05-23 16:26:58]

palmer: ping?

[palmer, 2016-05-23 19:26:00]

https://codereview.chromium.org/1922923002/diff/300001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/1922923002/diff/300001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:92:
array<string> uuids;
I know I sound like a broken record :) but is there any way to represent these
types (device IDs, UUIDs) with more specific types, whose constructors assert
sanity?

[ortuno, 2016-05-23 20:11:06]

https://codereview.chromium.org/1922923002/diff/300001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/1922923002/diff/300001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:92:
array<string> uuids;
On 2016/05/23 at 19:26:00, palmer wrote:
> I know I sound like a broken record :) but is there any way to represent these
types (device IDs, UUIDs) with more specific types, whose constructors assert
sanity?

I'm currently working on a follow up patch to use BluetoothUUID instead of just
strings: http://crbug.com/606446

Currently there is nothing of the sorts for device id. We could introduce one
which would help http://crbug.com/577962 but the only validation we can do is
regarding the string's format: length and last two characters. jyasskin and I
agree that it is something we should do but that the payoff isn't that high.

[dcheng, 2016-05-23 21:00:11]

On 2016/05/23 at 20:11:06, ortuno wrote:
>
https://codereview.chromium.org/1922923002/diff/300001/third_party/WebKit/pub...
> File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):
> 
>
https://codereview.chromium.org/1922923002/diff/300001/third_party/WebKit/pub...
> third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:92:
array<string> uuids;
> On 2016/05/23 at 19:26:00, palmer wrote:
> > I know I sound like a broken record :) but is there any way to represent
these types (device IDs, UUIDs) with more specific types, whose constructors
assert sanity?
> 
> I'm currently working on a follow up patch to use BluetoothUUID instead of
just strings: http://crbug.com/606446
> 
> Currently there is nothing of the sorts for device id. We could introduce one
which would help http://crbug.com/577962 but the only validation we can do is
regarding the string's format: length and last two characters. jyasskin and I
agree that it is something we should do but that the payoff isn't that high.

Why do we have to pass around device UUIDs at all? Does a UUID represent one
unique device?

[ortuno, 2016-05-23 21:08:20]

On 2016/05/23 at 21:00:11, dcheng wrote:
> On 2016/05/23 at 20:11:06, ortuno wrote:
> >
https://codereview.chromium.org/1922923002/diff/300001/third_party/WebKit/pub...
> > File
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):
> > 
> >
https://codereview.chromium.org/1922923002/diff/300001/third_party/WebKit/pub...
> > third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:92:
array<string> uuids;
> > On 2016/05/23 at 19:26:00, palmer wrote:
> > > I know I sound like a broken record :) but is there any way to represent
these types (device IDs, UUIDs) with more specific types, whose constructors
assert sanity?
> > 
> > I'm currently working on a follow up patch to use BluetoothUUID instead of
just strings: http://crbug.com/606446
> > 
> > Currently there is nothing of the sorts for device id. We could introduce
one which would help http://crbug.com/577962 but the only validation we can do
is regarding the string's format: length and last two characters. jyasskin and I
agree that it is something we should do but that the payoff isn't that high.
> 
> Why do we have to pass around device UUIDs at all? Does a UUID represent one
unique device?

In this specific line of code we are passing a list of the device's service
UUIDs. Further up we pass a "device id" which is indeed a unique identifier for
that device for that origin. See:
https://webbluetoothcg.github.io/web-bluetooth/#remote-device-identifiers

[palmer, 2016-05-23 21:19:48]

> I'm currently working on a follow up patch to use BluetoothUUID instead of
just
> strings: http://crbug.com/606446
> 
> Currently there is nothing of the sorts for device id. We could introduce one
> which would help http://crbug.com/577962 but the only validation we can do is
> regarding the string's format: length and last two characters. jyasskin and I
> agree that it is something we should do but that the payoff isn't that high.

Sure, but it's good to do that validation. Are you currently doing it?

[ortuno, 2016-05-23 22:01:56]

On 2016/05/23 at 21:19:48, palmer wrote:
> > I'm currently working on a follow up patch to use BluetoothUUID instead of
just
> > strings: http://crbug.com/606446
> > 
> > Currently there is nothing of the sorts for device id. We could introduce
one
> > which would help http://crbug.com/577962 but the only validation we can do
is
> > regarding the string's format: length and last two characters. jyasskin and
I
> > agree that it is something we should do but that the payoff isn't that high.
> 
> Sure, but it's good to do that validation. Are you currently doing it?

Not for the format specifically. Though, if a renderer tries to perform any
actions with an Id for which we haven't granted permission, we crash the
renderer. Anyway, I've marked the issue as blocking our Mojo move and will
address it after the BluetoothUUID issue.

[palmer, 2016-05-23 22:19:41]

> Not for the format specifically. Though, if a renderer tries to perform any
> actions with an Id for which we haven't granted permission, we crash the
> renderer. Anyway, I've marked the issue as blocking our Mojo move and will
> address it after the BluetoothUUID issue.

OK, LGTM.

[ortuno, 2016-05-23 22:47:28]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-05-23 22:47:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyasskin@chromium.org, rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/1922923002/#ps320001
(title: "Merge")

[commit-bot: I haz the power, 2016-05-23 22:48:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1922923002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1922923002/320001

[commit-bot: I haz the power, 2016-05-23 23:02:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-23 23:02:39]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[ortuno, 2016-05-24 15:17:37]

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

[ortuno, 2016-05-24 15:17:38]

nick: PTAL at the following files:

content/browser/renderer_host/render_process_host_impl.h
content/browser/renderer_host/render_process_host_impl.cc
content/renderer/render_frame_impl.cc
content/renderer/render_thread_impl.h
content/renderer/render_thread_impl.cc
content/test/layouttest_support.cc

[ncarter (slow), 2016-05-25 21:19:18]

lgtm with a test suggestion and one style fix

https://codereview.chromium.org/1922923002/diff/320001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1922923002/diff/320001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2737: return
bluetooth_adapter_factory_wrapper_;
Given that you've removed the reset-on-process-relaunch behavior, make sure you
test that bluetooth works properly after a crash.

The following repro steps could help (and would make a fine browsertest):
 1. Load a test page and interact with bluetooth.
 2. Using the task manager, kill the process, producing a sad tab.
 3. Hit F5 to reload the page.

https://codereview.chromium.org/1922923002/diff/320001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/1922923002/diff/320001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.h:264:
BluetoothAdapterFactoryWrapper& GetBluetoothAdapterFactoryWrapper();
This ought to be a pointer, not a ref.

[Jeffrey Yasskin, 2016-05-25 21:31:02]

https://codereview.chromium.org/1922923002/diff/320001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/1922923002/diff/320001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.h:264:
BluetoothAdapterFactoryWrapper& GetBluetoothAdapterFactoryWrapper();
On 2016/05/25 21:19:18, ncarter wrote:
> This ought to be a pointer, not a ref.

How come? Your comment is often a misunderstanding of the "no non-const
reference arguments" rule at
https://google.github.io/styleguide/cppguide.html#Reference_Arguments, which
doesn't say anything about return types, but you may have another reason.

I prefer a reference return because it's never null.

[ortuno, 2016-05-26 19:51:49]

The CQ bit was checked by ortuno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-26 19:52:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1922923002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1922923002/340001

[ortuno, 2016-05-26 20:31:01]

https://codereview.chromium.org/1922923002/diff/320001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1922923002/diff/320001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2737: return
bluetooth_adapter_factory_wrapper_;
On 2016/05/25 at 21:19:18, ncarter wrote:
> Given that you've removed the reset-on-process-relaunch behavior, make sure
you test that bluetooth works properly after a crash.
> 
> The following repro steps could help (and would make a fine browsertest):
>  1. Load a test page and interact with bluetooth.
>  2. Using the task manager, kill the process, producing a sad tab.
>  3. Hit F5 to reload the page.

Done.

https://codereview.chromium.org/1922923002/diff/320001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/1922923002/diff/320001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.h:264:
BluetoothAdapterFactoryWrapper& GetBluetoothAdapterFactoryWrapper();
On 2016/05/25 at 21:31:02, Jeffrey Yasskin wrote:
> On 2016/05/25 21:19:18, ncarter wrote:
> > This ought to be a pointer, not a ref.
> 
> How come? Your comment is often a misunderstanding of the "no non-const
reference arguments" rule at
https://google.github.io/styleguide/cppguide.html#Reference_Arguments, which
doesn't say anything about return types, but you may have another reason.
> 
> I prefer a reference return because it's never null.

I agree with jyasskin. The reference indicates users of the function that this
value will never be null.

[ncarter (slow), 2016-05-26 21:47:35]

https://codereview.chromium.org/1922923002/diff/320001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/1922923002/diff/320001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.h:264:
BluetoothAdapterFactoryWrapper& GetBluetoothAdapterFactoryWrapper();
On 2016/05/26 20:31:01, ortuno wrote:
> On 2016/05/25 at 21:31:02, Jeffrey Yasskin wrote:
> > On 2016/05/25 21:19:18, ncarter wrote:
> > > This ought to be a pointer, not a ref.
> > 
> > How come? Your comment is often a misunderstanding of the "no non-const
> reference arguments" rule at
> https://google.github.io/styleguide/cppguide.html#Reference_Arguments, which
> doesn't say anything about return types, but you may have another reason.
> > 
> > I prefer a reference return because it's never null.
> 
> I agree with jyasskin. The reference indicates users of the function that this
> value will never be null.

I agree with you, that it isn't prohibited by the style guide. It struck me as
odd because the "ref means it can't be null" convention is uncommon in content/.
For example, this class has a bunch of pointer-valued function that can never be
null. They don't use references to indicate this. Doing it here seems
inconsistent with the surrounding code in content/browser.

I see:

https://goo.gl/6o1YMQ 487 pointer valued functions
https://goo.gl/W2jnWo 43 ref-valued functions
https://goo.gl/4pWAmQ of which 32 are const ref

Leaving 11 cases where we do this, vs 487 cases where we don't.

Having said that, this is the sort of style debate I absolutely loathe. You can
do it if it's really important to you.

[ncarter (slow), 2016-05-26 22:02:34]

https://codereview.chromium.org/1922923002/diff/320001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/1922923002/diff/320001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.h:264:
BluetoothAdapterFactoryWrapper& GetBluetoothAdapterFactoryWrapper();
On 2016/05/26 21:47:35, ncarter wrote:
> On 2016/05/26 20:31:01, ortuno wrote:
> > On 2016/05/25 at 21:31:02, Jeffrey Yasskin wrote:
> > > On 2016/05/25 21:19:18, ncarter wrote:
> > > > This ought to be a pointer, not a ref.
> > > 
> > > How come? Your comment is often a misunderstanding of the "no non-const
> > reference arguments" rule at
> > https://google.github.io/styleguide/cppguide.html#Reference_Arguments, which
> > doesn't say anything about return types, but you may have another reason.
> > > 
> > > I prefer a reference return because it's never null.
> > 
> > I agree with jyasskin. The reference indicates users of the function that
this
> > value will never be null.
> 
> I agree with you, that it isn't prohibited by the style guide. It struck me as
> odd because the "ref means it can't be null" convention is uncommon in
content/.
> For example, this class has a bunch of pointer-valued function that can never
be
> null. They don't use references to indicate this. Doing it here seems
> inconsistent with the surrounding code in content/browser.
> 
> I see:
> 
> https://goo.gl/6o1YMQ 487 pointer valued functions
> https://goo.gl/W2jnWo 43 ref-valued functions
> https://goo.gl/4pWAmQ of which 32 are const ref
> 
> Leaving 11 cases where we do this, vs 487 cases where we don't.
> 
> Having said that, this is the sort of style debate I absolutely loathe. You
can
> do it if it's really important to you.

Whoops, that last bit possibly came across too passive-agressive. What I mean
is: if you're not swayed by the consistency argument, then go ahead with the
non-const ref: the style guide is the ultimate arbiter here, and it says this is
legal.

[commit-bot: I haz the power, 2016-05-26 23:17:37]

Dry run: None

[ortuno, 2016-05-26 23:19:01]

The CQ bit was checked by ortuno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-26 23:19:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1922923002/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1922923002/360001

[commit-bot: I haz the power, 2016-05-27 01:30:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-27 01:30:39]

Dry run: This issue passed the CQ dry run.

[commit-bot: I haz the power, 2016-05-27 01:30:55]

Dry run: None

[ortuno, 2016-05-27 02:53:23]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-05-27 02:53:25]

Thanks!

https://codereview.chromium.org/1922923002/diff/320001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/1922923002/diff/320001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.h:264:
BluetoothAdapterFactoryWrapper& GetBluetoothAdapterFactoryWrapper();
On 2016/05/26 at 22:02:34, ncarter wrote:
> On 2016/05/26 21:47:35, ncarter wrote:
> > On 2016/05/26 20:31:01, ortuno wrote:
> > > On 2016/05/25 at 21:31:02, Jeffrey Yasskin wrote:
> > > > On 2016/05/25 21:19:18, ncarter wrote:
> > > > > This ought to be a pointer, not a ref.
> > > > 
> > > > How come? Your comment is often a misunderstanding of the "no non-const
> > > reference arguments" rule at
> > > https://google.github.io/styleguide/cppguide.html#Reference_Arguments,
which
> > > doesn't say anything about return types, but you may have another reason.
> > > > 
> > > > I prefer a reference return because it's never null.
> > > 
> > > I agree with jyasskin. The reference indicates users of the function that
this
> > > value will never be null.
> > 
> > I agree with you, that it isn't prohibited by the style guide. It struck me
as
> > odd because the "ref means it can't be null" convention is uncommon in
content/.
> > For example, this class has a bunch of pointer-valued function that can
never be
> > null. They don't use references to indicate this. Doing it here seems
> > inconsistent with the surrounding code in content/browser.
> > 
> > I see:
> > 
> > https://goo.gl/6o1YMQ 487 pointer valued functions
> > https://goo.gl/W2jnWo 43 ref-valued functions
> > https://goo.gl/4pWAmQ of which 32 are const ref
> > 
> > Leaving 11 cases where we do this, vs 487 cases where we don't.
> > 
> > Having said that, this is the sort of style debate I absolutely loathe. You
can
> > do it if it's really important to you.
> 
> Whoops, that last bit possibly came across too passive-agressive. What I mean
is: if you're not swayed by the consistency argument, then go ahead with the
non-const ref: the style guide is the ultimate arbiter here, and it says this is
legal.

Consistency makes sense. I've changed the function to return a pointer.

[ortuno, 2016-05-27 02:53:25]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
jyasskin@chromium.org, nick@chromium.org, rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/1922923002/#ps360001
(title: "Change ref to pointer")

[commit-bot: I haz the power, 2016-05-27 02:53:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1922923002/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1922923002/360001

[commit-bot: I haz the power, 2016-05-27 03:04:19]

Committed patchset #17 (id:360001)

[commit-bot: I haz the power, 2016-05-27 03:06:05]

Description was changed from

==========
bluetooth: Move requestDevice to mojo

Changes in order of importance and complexity:
1. Move most of the logic of BluetoothDispatcherHost::OnRequestDeviceImpl to a
new
   class called BluetoothDeviceProvider.
2. Introduce BluetoothAdapterFactoryWrapper to manage adapters and their
observers.
   This class allows us to set the adapter from tests. It will be removed once
we
   no longer need to swap adapters to change an adapter's behavior.
3. Fix BluetoothBlacklist and BluetoothAllowedDevicesMap to use mojo types.
4. Remove content/common/bluetooth and
content/renderer/bluetooth/bluetooth_dispatcher
   \o/


BUG=508771
==========

to

==========
bluetooth: Move requestDevice to mojo

Changes in order of importance and complexity:
1. Move most of the logic of BluetoothDispatcherHost::OnRequestDeviceImpl to a
new
   class called BluetoothDeviceProvider.
2. Introduce BluetoothAdapterFactoryWrapper to manage adapters and their
observers.
   This class allows us to set the adapter from tests. It will be removed once
we
   no longer need to swap adapters to change an adapter's behavior.
3. Fix BluetoothBlacklist and BluetoothAllowedDevicesMap to use mojo types.
4. Remove content/common/bluetooth and
content/renderer/bluetooth/bluetooth_dispatcher
   \o/

BUG=508771

Committed: https://crrev.com/b6374dd8ae9d5d1b4d99bfbebb45e9adddc45a24
Cr-Commit-Position: refs/heads/master@{#396369}
==========

[commit-bot: I haz the power, 2016-05-27 03:06:07]

Patchset 17 (id:??) landed as
https://crrev.com/b6374dd8ae9d5d1b4d99bfbebb45e9adddc45a24
Cr-Commit-Position: refs/heads/master@{#396369}