Add a path for content/ to open and control a Bluetooth chooser dialog.

Add a path for content/ to open and control a Bluetooth chooser dialog.

This obsoletes several histogram values, and some of the corner-case error states won't be well tested until I add the ability for layout tests to manage dialog states.

2nd of 3 patches:
1. Add errors and prepare tests. (https://codereview.chromium.org/1293593003/)
2. Wire up the chooser on the Chrome side. (This patch)
3. Update the test assertions and remove now-unused errors. (https://codereview.chromium.org/1284143006/)

BUG=500989, 517237
Committed: https://crrev.com/5e7aec4faea108b6e64ee6a4712e77ca5b43e3c7
Cr-Commit-Position: refs/heads/master@{#345541}

[Jeffrey Yasskin, 2015-08-14 22:59:34]

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

[Jeffrey Yasskin, 2015-08-17 16:30:48]

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

[ortuno, 2015-08-17 21:12:59]

Mostly questions.

https://codereview.chromium.org/1286063002/diff/60001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1286063002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:138:
discovery_session->Stop(base::Bind(&base::DoNothing),
Should we at least log or histogram the result? It might point to some issue
with the lower level implementation.

https://codereview.chromium.org/1286063002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:153:
base::Bind(&BluetoothDispatcherHost::StopDeviceDiscovery, this),
Shouldn't you pass a weak pointer to the timer?

https://codereview.chromium.org/1286063002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:197:
base::Bind(&BluetoothDispatcherHost::StopDeviceDiscovery, this));
Same here. Wouldn't a weak pointer be better?

https://codereview.chromium.org/1286063002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:220: void
AddFilteredDevice(const device::BluetoothDevice& device) {
Why do you pass the device by reference if you already had a pointer to it?

https://codereview.chromium.org/1286063002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:253:
session->chooser->ShowDiscovering(BluetoothChooser::DiscoveryState::IDLE);
See below. ShowDiscovering could remove the session which would invalidate the
iterator.

https://codereview.chromium.org/1286063002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:280:
session->AddFilteredDevice(*device);
AddFilteredDevice could end up removing the session. Wouldn't that invalidate
the iterator here? Or is the PostTask there to make sure
OnRequestDeviceCancelled is called after this loop ends?

https://codereview.chromium.org/1286063002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:358: this, chooser_id,
render_frame_host->GetLastCommittedURL().GetOrigin());
If you call requestDevice() from an iframe would it show its parent's origin or
its origin?

https://codereview.chromium.org/1286063002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:369:
session->chooser->SetAdapterPresence(
Since this call could close the dialog shouldn't you check that the session is
still there after this call?

You check in the callbacks but trying to start a discovery session with an off
adapter would pollute the logs unnecessarily.

https://codereview.chromium.org/1286063002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:667:
CHECK(session->chooser) << "Shouldn't close the dialog twice.";
Do you need this CHECK? You would get a null pointer dereference in the next
line.

https://codereview.chromium.org/1286063002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1286063002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1941: if (delegate_) {
Why do you have to check if the delegate is there? Also what about:

return delegate_ ?
  delegate_->RunBluetoothChooser(observer, chooser_id, origin) : nullptr;

https://codereview.chromium.org/1286063002/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1286063002/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:699: if (delegate_)
What about using the ternary operator here as well?

https://codereview.chromium.org/1286063002/diff/60001/content/public/browser/...
File content/public/browser/bluetooth_chooser.h (right):

https://codereview.chromium.org/1286063002/diff/60001/content/public/browser/...
content/public/browser/bluetooth_chooser.h:49: virtual void
ShowDiscovering(DiscoveryState state) = 0;
Hmm not sure about the name of this function. What about ShowDiscoveringDialog
or Message?

[Jeffrey Yasskin, 2015-08-17 23:38:16]

I also fixed some comments, and reordered some things to try to make the dialog
flow better.

https://codereview.chromium.org/1286063002/diff/60001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1286063002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:138:
discovery_session->Stop(base::Bind(&base::DoNothing),
On 2015/08/17 21:12:58, ortuno wrote:
> Should we at least log or histogram the result? It might point to some issue
> with the lower level implementation.

We should, but I think not here. I've just filed https://crbug.com/521780.

https://codereview.chromium.org/1286063002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:153:
base::Bind(&BluetoothDispatcherHost::StopDeviceDiscovery, this),
On 2015/08/17 21:12:58, ortuno wrote:
> Shouldn't you pass a weak pointer to the timer?

Good catch: I need to explicitly pass an unretained pointer instead of letting
Bind() take a strong reference. I don't need to use a WeakPtr because Timer
guarantees it won't call back after is destructor starts.

Because Timer always posts tasks to a TaskRunner, it can't fire tasks during a
destructor, so the declaration order in the class doesn't matter either.

https://codereview.chromium.org/1286063002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:197:
base::Bind(&BluetoothDispatcherHost::StopDeviceDiscovery, this));
On 2015/08/17 21:12:58, ortuno wrote:
> Same here. Wouldn't a weak pointer be better?

Switched to Unretained here too.

https://codereview.chromium.org/1286063002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:220: void
AddFilteredDevice(const device::BluetoothDevice& device) {
On 2015/08/17 21:12:58, ortuno wrote:
> Why do you pass the device by reference if you already had a pointer to it?

To be clear that |device| can't be null inside this function. DeviceAdded()
provides a pointer to allow callees to mutate the device, because of our
no-nonconst-references rule.

https://codereview.chromium.org/1286063002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:253:
session->chooser->ShowDiscovering(BluetoothChooser::DiscoveryState::IDLE);
On 2015/08/17 21:12:58, ortuno wrote:
> See below. ShowDiscovering could remove the session which would invalidate the
> iterator.

See below.

https://codereview.chromium.org/1286063002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:280:
session->AddFilteredDevice(*device);
On 2015/08/17 21:12:58, ortuno wrote:
> AddFilteredDevice could end up removing the session. Wouldn't that invalidate
> the iterator here? Or is the PostTask there to make sure
> OnRequestDeviceCancelled is called after this loop ends?

Yep, the PostTask there makes sure the object is only destroyed at the top level
of an event loop, rather than inside an iteration in one of these functions.

https://codereview.chromium.org/1286063002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:358: this, chooser_id,
render_frame_host->GetLastCommittedURL().GetOrigin());
On 2015/08/17 21:12:58, ortuno wrote:
> If you call requestDevice() from an iframe would it show its parent's origin
or
> its origin?

Right now, it would show the frame's origin, since this is using the
RenderFrameHost. We need to figure out how to interact with the [permission]
attribute to let top-level frames prevent their sub-frames from using bluetooth:
https://crbug.com/518042.

https://codereview.chromium.org/1286063002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:369:
session->chooser->SetAdapterPresence(
On 2015/08/17 21:12:58, ortuno wrote:
> Since this call could close the dialog shouldn't you check that the session is
> still there after this call?

Yielding to the event loop in CloseDialog() makes sure the session isn't
destroyed before this function completes. However, I do need to check that the
*chooser* is still present after each call that might close the dialog. Our
tests are awesome and caught when I missed this.

> You check in the callbacks but trying to start a discovery session with an off
> adapter would pollute the logs unnecessarily.

Good point; I'll return early for that. I've also moved this after the
GetDevices() call, to make sure we don't miss any devices once the adapter is
powered back on.

https://codereview.chromium.org/1286063002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:667:
CHECK(session->chooser) << "Shouldn't close the dialog twice.";
On 2015/08/17 21:12:58, ortuno wrote:
> Do you need this CHECK? You would get a null pointer dereference in the next
> line.

session->chooser.reset() works if session->chooser==nullptr:
https://code.google.com/p/chromium/codesearch/#chromium/src/base/memory/scope...

https://codereview.chromium.org/1286063002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1286063002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1941: if (delegate_) {
On 2015/08/17 21:12:59, ortuno wrote:
> Why do you have to check if the delegate is there?

Looking at other uses of delegate_, I don't. Thanks!

https://codereview.chromium.org/1286063002/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1286063002/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:699: if (delegate_)
On 2015/08/17 21:12:59, ortuno wrote:
> What about using the ternary operator here as well?

In general, I find uses of the ternary operator harder to read than the
equivalent if(). There are some places for it, but I avoid it when it doesn't
make anything else simpler.

https://codereview.chromium.org/1286063002/diff/60001/content/public/browser/...
File content/public/browser/bluetooth_chooser.h (right):

https://codereview.chromium.org/1286063002/diff/60001/content/public/browser/...
content/public/browser/bluetooth_chooser.h:49: virtual void
ShowDiscovering(DiscoveryState state) = 0;
On 2015/08/17 21:12:59, ortuno wrote:
> Hmm not sure about the name of this function. What about ShowDiscoveringDialog
> or Message?

I'm not sure of it either, but I don't like your suggestions any better. The
chooser has a couple states:

* Discovering:
https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_Chooser...
* Finished discovering; either there are devices
(https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_Chooser...)
or there aren't
(https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_Chooser...).
* Discovery couldn't start: no mocks for this, but they should probably resemble
https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_Chooser...
or
https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/072015_Bluetoo....

"ShowDiscovering" was an attempt to express that we're asking the chooser to
show the user the state of device discovery. "ShowDiscoveryState" might be
better? The chooser doesn't necessarily show a dialog or a message in response,
so I don't want either of those in the function name.

Any other suggestions?

[ortuno, 2015-08-18 00:21:55]

LGTM

https://codereview.chromium.org/1286063002/diff/60001/content/public/browser/...
File content/public/browser/bluetooth_chooser.h (right):

https://codereview.chromium.org/1286063002/diff/60001/content/public/browser/...
content/public/browser/bluetooth_chooser.h:49: virtual void
ShowDiscovering(DiscoveryState state) = 0;
On 2015/08/17 23:38:16, Jeffrey Yasskin wrote:
> On 2015/08/17 21:12:59, ortuno wrote:
> > Hmm not sure about the name of this function. What about
ShowDiscoveringDialog
> > or Message?
> 
> I'm not sure of it either, but I don't like your suggestions any better. The
> chooser has a couple states:
> 
> * Discovering:
>
https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_Chooser...
> * Finished discovering; either there are devices
>
(https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_Chooser...)
> or there aren't
>
(https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_Chooser...).
> * Discovery couldn't start: no mocks for this, but they should probably
resemble
>
https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_Chooser...
> or
>
https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/072015_Bluetoo....
> 
> "ShowDiscovering" was an attempt to express that we're asking the chooser to
> show the user the state of device discovery. "ShowDiscoveryState" might be
> better? The chooser doesn't necessarily show a dialog or a message in
response,
> so I don't want either of those in the function name.
> 
> Any other suggestions?

I like ShowDiscoveryState :)

https://codereview.chromium.org/1286063002/diff/80001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1286063002/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:703: if
(!base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, callback)) {
Can you add a comment explaining why this is used?

[Jeffrey Yasskin, 2015-08-18 21:13:20]

jyasskin@chromium.org changed reviewers:
+ askvitkine@chromium.org, jam@chromium.org
- finnur@chromium.org, scheib@chromium.org

[Jeffrey Yasskin, 2015-08-18 21:13:20]

Alexei, please look at histograms.xml.
John, please look at content/*. Do you want us to add ourselves as per-file
owners of bluetooth_chooser.h?

Thanks,
Jeffrey

https://codereview.chromium.org/1286063002/diff/80001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1286063002/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:703: if
(!base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, callback)) {
On 2015/08/18 00:21:55, ortuno wrote:
> Can you add a comment explaining why this is used?

Done.

[Jeffrey Yasskin, 2015-08-18 21:14:52]

+Nick too, but I'd like to check this in before he gets back on Aug 24.

[Jeffrey Yasskin, 2015-08-20 15:37:30]

Ping. John, are you the right person to review content/*, or should I find
someone else?

[Alexei Svitkine (slow), 2015-08-24 17:18:03]

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

[Alexei Svitkine (slow), 2015-08-24 17:18:04]

lgtm

[Jeffrey Yasskin, 2015-08-25 15:25:41]

jyasskin@chromium.org changed reviewers:
- askvitkine@chromium.org

[jam, 2015-08-25 15:49:21]

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

https://codereview.chromium.org/1286063002/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:375: session->chooser =
render_frame_host->RunBluetoothChooser(
instead of going through
RenderFrameHost->RenderFrameHostDelegate->WebContents->WebContentsDelegate, how
about just

WebContents::FromRenderFrameHost(render_frame_host)->GetDelegate()?

https://codereview.chromium.org/1286063002/diff/120001/content/public/browser...
File content/public/browser/bluetooth_chooser.h (right):

https://codereview.chromium.org/1286063002/diff/120001/content/public/browser...
content/public/browser/bluetooth_chooser.h:28: // those knobs will cause new
functions to be called here.
are you certain that we will add more here? i.e. because if you're not sure, it
would be simpler (and more consistent with how we're trying to move the API to
use callbacks instead of single-method interfaces) to combine the two methods
above into one callback

https://codereview.chromium.org/1286063002/diff/120001/content/public/browser...
content/public/browser/bluetooth_chooser.h:53: // Tells the chooser that a
device is no longer available. The chooser should
nit: blank line before

https://codereview.chromium.org/1286063002/diff/120001/content/public/browser...
content/public/browser/bluetooth_chooser.h:60: void CallDialogCancelled() {
observer_->DialogCancelled(chooser_id_); }
this is against the content style guide:
https://www.chromium.org/developers/content-module/content-api

"content/public should contain only interfaces, structs and (rarely) static
functions"

if you used a callback here, you can bound the id to it

[Jeffrey Yasskin, 2015-08-25 17:13:14]

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

https://codereview.chromium.org/1286063002/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:375: session->chooser =
render_frame_host->RunBluetoothChooser(
On 2015/08/25 15:49:21, jam wrote:
> instead of going through
> RenderFrameHost->RenderFrameHostDelegate->WebContents->WebContentsDelegate,
how
> about just
> 
> WebContents::FromRenderFrameHost(render_frame_host)->GetDelegate()?

Thanks; I didn't realize that route existed.

https://codereview.chromium.org/1286063002/diff/120001/content/public/browser...
File content/public/browser/bluetooth_chooser.h (right):

https://codereview.chromium.org/1286063002/diff/120001/content/public/browser...
content/public/browser/bluetooth_chooser.h:28: // those knobs will cause new
functions to be called here.
On 2015/08/25 15:49:21, jam wrote:
> are you certain that we will add more here? i.e. because if you're not sure,
it
> would be simpler (and more consistent with how we're trying to move the API to
> use callbacks instead of single-method interfaces) to combine the two methods
> above into one callback 

Yeah, we have at least "enable the adapter", "start scanning again", and "visit
the help url" in the mocks. Those are all no-argument functions, so we could
have a single callback that takes a big enum saying what happened, with one
optional device_id for the "Selected" option. Is that better?

https://codereview.chromium.org/1286063002/diff/120001/content/public/browser...
content/public/browser/bluetooth_chooser.h:53: // Tells the chooser that a
device is no longer available. The chooser should
On 2015/08/25 15:49:21, jam wrote:
> nit: blank line before

Done.

https://codereview.chromium.org/1286063002/diff/120001/content/public/browser...
content/public/browser/bluetooth_chooser.h:60: void CallDialogCancelled() {
observer_->DialogCancelled(chooser_id_); }
On 2015/08/25 15:49:21, jam wrote:
> this is against the content style guide:
> https://www.chromium.org/developers/content-module/content-api
> 
> "content/public should contain only interfaces, structs and (rarely) static
> functions"
> 
> if you used a callback here, you can bound the id to it

This seems in line with other interfaces in content/public that include a small
amount of behavior to make their implementations easier to use. e.g.
RenderViewObserver and WebContentsObserver. But I'm willing to move the logic to
subclasses anyway. Do the member variables need to move too?

I'll wait to do that until we decide whether to change the observer to a
callback.

[Jeffrey Yasskin, 2015-08-25 20:53:27]

Patchset #9 (id:160001) has been deleted

[Jeffrey Yasskin, 2015-08-25 20:55:58]

Ready for another review.

https://codereview.chromium.org/1286063002/diff/120001/content/public/browser...
File content/public/browser/bluetooth_chooser.h (right):

https://codereview.chromium.org/1286063002/diff/120001/content/public/browser...
content/public/browser/bluetooth_chooser.h:28: // those knobs will cause new
functions to be called here.
On 2015/08/25 17:13:13, Jeffrey Yasskin wrote:
> On 2015/08/25 15:49:21, jam wrote:
> > are you certain that we will add more here? i.e. because if you're not sure,
> it
> > would be simpler (and more consistent with how we're trying to move the API
to
> > use callbacks instead of single-method interfaces) to combine the two
methods
> > above into one callback 
> 
> Yeah, we have at least "enable the adapter", "start scanning again", and
"visit
> the help url" in the mocks. Those are all no-argument functions, so we could
> have a single callback that takes a big enum saying what happened, with one
> optional device_id for the "Selected" option. Is that better?

Switched to a callback.

https://codereview.chromium.org/1286063002/diff/120001/content/public/browser...
content/public/browser/bluetooth_chooser.h:60: void CallDialogCancelled() {
observer_->DialogCancelled(chooser_id_); }
On 2015/08/25 17:13:14, Jeffrey Yasskin wrote:
> On 2015/08/25 15:49:21, jam wrote:
> > this is against the content style guide:
> > https://www.chromium.org/developers/content-module/content-api
> > 
> > "content/public should contain only interfaces, structs and (rarely) static
> > functions"
> > 
> > if you used a callback here, you can bound the id to it
> 
> This seems in line with other interfaces in content/public that include a
small
> amount of behavior to make their implementations easier to use. e.g.
> RenderViewObserver and WebContentsObserver. But I'm willing to move the logic
to
> subclasses anyway. Do the member variables need to move too?
> 
> I'll wait to do that until we decide whether to change the observer to a
> callback.

Moot, now that there's a callback.

[Alexei Svitkine (slow), 2015-08-25 20:59:23]

still lgtm

https://codereview.chromium.org/1286063002/diff/180001/content/browser/blueto...
File content/browser/bluetooth/first_device_bluetooth_chooser.h (right):

https://codereview.chromium.org/1286063002/diff/180001/content/browser/blueto...
content/browser/bluetooth/first_device_bluetooth_chooser.h:20:
FirstDeviceBluetoothChooser(const EventHandler& event_handler);
explicit

https://codereview.chromium.org/1286063002/diff/180001/content/browser/blueto...
content/browser/bluetooth/first_device_bluetooth_chooser.h:23: //
BluetoothChooser implementation:
Nit: For new code, I think the convention is just:

// BluetoothChooser:

https://codereview.chromium.org/1286063002/diff/180001/content/browser/blueto...
content/browser/bluetooth/first_device_bluetooth_chooser.h:32: };
DISALLOW_COPY_AND_ASSIGN()?

[Jeffrey Yasskin, 2015-08-25 21:04:50]

https://codereview.chromium.org/1286063002/diff/180001/content/browser/blueto...
File content/browser/bluetooth/first_device_bluetooth_chooser.h (right):

https://codereview.chromium.org/1286063002/diff/180001/content/browser/blueto...
content/browser/bluetooth/first_device_bluetooth_chooser.h:20:
FirstDeviceBluetoothChooser(const EventHandler& event_handler);
On 2015/08/25 20:59:23, Alexei Svitkine (OOO Aug15-24) wrote:
> explicit

Done.

https://codereview.chromium.org/1286063002/diff/180001/content/browser/blueto...
content/browser/bluetooth/first_device_bluetooth_chooser.h:23: //
BluetoothChooser implementation:
On 2015/08/25 20:59:23, Alexei Svitkine (OOO Aug15-24) wrote:
> Nit: For new code, I think the convention is just:
> 
> // BluetoothChooser:

Done.

https://codereview.chromium.org/1286063002/diff/180001/content/browser/blueto...
content/browser/bluetooth/first_device_bluetooth_chooser.h:32: };
On 2015/08/25 20:59:23, Alexei Svitkine (OOO Aug15-24) wrote:
> DISALLOW_COPY_AND_ASSIGN()?

Done.

[jam, 2015-08-25 23:45:51]

lgtm

https://codereview.chromium.org/1286063002/diff/200001/content/public/browser...
File content/public/browser/bluetooth_chooser.h (right):

https://codereview.chromium.org/1286063002/diff/200001/content/public/browser...
content/public/browser/bluetooth_chooser.h:42: // |observer| must outlive
|this|. |chooser_id| is propagated to the Observer
nit: no more observer

https://codereview.chromium.org/1286063002/diff/200001/content/public/browser...
File content/public/browser/web_contents_delegate.cc (right):

https://codereview.chromium.org/1286063002/diff/200001/content/public/browser...
content/public/browser/web_contents_delegate.cc:155: return make_scoped_ptr(new
FirstDeviceBluetoothChooser(event_handler));
nit: convention is for WCD default implementation to return null, then
webcontentsimpl can create FirstDeviceBluetoothChooser or whatever it wants if
it sees null.

[Jeffrey Yasskin, 2015-08-26 00:19:01]

https://codereview.chromium.org/1286063002/diff/200001/content/public/browser...
File content/public/browser/bluetooth_chooser.h (right):

https://codereview.chromium.org/1286063002/diff/200001/content/public/browser...
content/public/browser/bluetooth_chooser.h:42: // |observer| must outlive
|this|. |chooser_id| is propagated to the Observer
On 2015/08/25 23:45:50, jam wrote:
> nit: no more observer

Whoops, thanks.

https://codereview.chromium.org/1286063002/diff/200001/content/public/browser...
File content/public/browser/web_contents_delegate.cc (right):

https://codereview.chromium.org/1286063002/diff/200001/content/public/browser...
content/public/browser/web_contents_delegate.cc:155: return make_scoped_ptr(new
FirstDeviceBluetoothChooser(event_handler));
On 2015/08/25 23:45:50, jam wrote:
> nit: convention is for WCD default implementation to return null, then
> webcontentsimpl can create FirstDeviceBluetoothChooser or whatever it wants if
> it sees null.

'k. BluetoothDispatcherHost can do it.

[scheib, 2015-08-26 01:02:14]

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

[scheib, 2015-08-26 01:02:15]

LGTM, but noticed:

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

https://codereview.chromium.org/1286063002/diff/220001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:719: // asked
comment wrapping

[Jeffrey Yasskin, 2015-08-26 01:11:11]

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

https://codereview.chromium.org/1286063002/diff/220001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:719: // asked
On 2015/08/26 01:02:15, scheib wrote:
> comment wrapping

Fixed.

[Jeffrey Yasskin, 2015-08-26 01:12:34]

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

[commit-bot: I haz the power, 2015-08-26 01:12:58]

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

[commit-bot: I haz the power, 2015-08-26 02:37:07]

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

[commit-bot: I haz the power, 2015-08-26 02:37:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Jeffrey Yasskin, 2015-08-26 04:08:03]

The CQ bit was checked by jyasskin@chromium.org

[Jeffrey Yasskin, 2015-08-26 04:08:03]

The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org,
asvitkine@chromium.org, jam@chromium.org, scheib@chromium.org
Link to the patchset: https://codereview.chromium.org/1286063002/#ps240001
(title: "Fix comment wrapping.")

[commit-bot: I haz the power, 2015-08-26 04:08:11]

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

[commit-bot: I haz the power, 2015-08-26 04:46:56]

Committed patchset #12 (id:240001)

[commit-bot: I haz the power, 2015-08-26 04:47:36]

Patchset 12 (id:??) landed as
https://crrev.com/5e7aec4faea108b6e64ee6a4712e77ca5b43e3c7
Cr-Commit-Position: refs/heads/master@{#345541}