[Onion Soup] Move WebBluetoothImpl from //content/renderer/bluetooth to Blink's bluetooth module

[Onion Soup] Move WebBluetoothImpl from //content/renderer/bluetooth to Blink's bluetooth module

The Mojo WebBluetoothService can be used directly from Blink. This CL
moves the WebBluetoothImpl from //content/renderer/bluetooth to
//third_party/WebKit/Source/modules/bluetooth.

BUG=597053, 561879
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/c7a55bd0ff1e9816eccb7dc2d4c02ff825c4a7eb
Cr-Commit-Position: refs/heads/master@{#441194}

[juncai, 2016-12-10 00:00:50]

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

[commit-bot: I haz the power, 2016-12-10 00:01:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-10 00:07:31]

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

[commit-bot: I haz the power, 2016-12-10 00:07:32]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[juncai, 2016-12-10 01:31:26]

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

[commit-bot: I haz the power, 2016-12-10 01:33:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-10 02:35:23]

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

[commit-bot: I haz the power, 2016-12-10 02:35:24]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[juncai, 2016-12-12 18:43:11]

Description was changed from

==========
Web Bluetooth Onion Soup

Web Bluetooth Onion Soup.

BUG=597053
==========

to

==========
Web Bluetooth Onion Soup

Web Bluetooth Onion Soup.

BUG=597053, 561879
==========

[juncai, 2016-12-12 18:47:53]

Description was changed from

==========
Web Bluetooth Onion Soup

Web Bluetooth Onion Soup.

BUG=597053, 561879
==========

to

==========
Move WebBluetoothImpl from //content/renderer/bluetooth to into Blink's
bluetooth module

The Mojo WebBluetoothService can be used directly from Blink. This CL moves the
WebBluetoothImpl from //content/renderer/bluetooth to
//third_party/WebKit/Source/modules/bluetooth.

BUG=597053, 561879
==========

[juncai, 2016-12-12 18:48:08]

Description was changed from

==========
Move WebBluetoothImpl from //content/renderer/bluetooth to into Blink's
bluetooth module

The Mojo WebBluetoothService can be used directly from Blink. This CL moves the
WebBluetoothImpl from //content/renderer/bluetooth to
//third_party/WebKit/Source/modules/bluetooth.

BUG=597053, 561879
==========

to

==========
Move WebBluetoothImpl from //content/renderer/bluetooth to into Blink's
bluetooth module

The Mojo WebBluetoothService can be used directly from Blink. This CL
moves the WebBluetoothImpl from //content/renderer/bluetooth to
//third_party/WebKit/Source/modules/bluetooth.

BUG=597053, 561879
==========

[juncai, 2016-12-12 18:53:05]

Description was changed from

==========
Move WebBluetoothImpl from //content/renderer/bluetooth to into Blink's
bluetooth module

The Mojo WebBluetoothService can be used directly from Blink. This CL
moves the WebBluetoothImpl from //content/renderer/bluetooth to
//third_party/WebKit/Source/modules/bluetooth.

BUG=597053, 561879
==========

to

==========
Move WebBluetoothImpl from //content/renderer/bluetooth to Blink's bluetooth
module

The Mojo WebBluetoothService can be used directly from Blink. This CL
moves the WebBluetoothImpl from //content/renderer/bluetooth to
//third_party/WebKit/Source/modules/bluetooth.

BUG=597053, 561879
==========

[juncai, 2016-12-12 19:22:55]

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

[commit-bot: I haz the power, 2016-12-12 19:24:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 20:29:51]

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

[commit-bot: I haz the power, 2016-12-12 20:29:52]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[juncai, 2016-12-13 23:31:02]

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

[commit-bot: I haz the power, 2016-12-13 23:32:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 00:34:11]

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

[commit-bot: I haz the power, 2016-12-14 00:34:12]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[juncai, 2016-12-15 00:28:19]

Description was changed from

==========
Move WebBluetoothImpl from //content/renderer/bluetooth to Blink's bluetooth
module

The Mojo WebBluetoothService can be used directly from Blink. This CL
moves the WebBluetoothImpl from //content/renderer/bluetooth to
//third_party/WebKit/Source/modules/bluetooth.

BUG=597053, 561879
==========

to

==========
Move WebBluetoothImpl from //content/renderer/bluetooth to Blink's bluetooth
module

The Mojo WebBluetoothService can be used directly from Blink. This CL
moves the WebBluetoothImpl from //content/renderer/bluetooth to
//third_party/WebKit/Source/modules/bluetooth.

BUG=597053, 561879
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[juncai, 2016-12-15 00:28:29]

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

[commit-bot: I haz the power, 2016-12-15 00:29:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 00:37:38]

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

[commit-bot: I haz the power, 2016-12-15 00:37:38]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[juncai, 2016-12-15 01:37:43]

Description was changed from

==========
Move WebBluetoothImpl from //content/renderer/bluetooth to Blink's bluetooth
module

The Mojo WebBluetoothService can be used directly from Blink. This CL
moves the WebBluetoothImpl from //content/renderer/bluetooth to
//third_party/WebKit/Source/modules/bluetooth.

BUG=597053, 561879
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[Onion Soup] Move WebBluetoothImpl from //content/renderer/bluetooth to Blink's
bluetooth module

The Mojo WebBluetoothService can be used directly from Blink. This CL
moves the WebBluetoothImpl from //content/renderer/bluetooth to
//third_party/WebKit/Source/modules/bluetooth.

BUG=597053, 561879
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[juncai, 2016-12-15 01:38:53]

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

[juncai, 2016-12-15 01:38:54]

Please take a look.

[juncai, 2016-12-15 23:13:36]

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

[juncai, 2016-12-15 23:13:37]

esprehn@chromium.org: Please review this Onion Soup CL.

[esprehn, 2016-12-15 23:54:04]

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

[esprehn, 2016-12-15 23:54:05]

lgtm from my side, reillyg@ does this look legit to you?

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp (right):

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp:209: interfaceProvider
= Platform::current()->interfaceProvider();
Hmm, we probably need to have interfaceProvider() available for workers
eventually

[dougt, 2016-12-16 00:11:43]

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

[dougt, 2016-12-16 00:11:45]

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

https://codereview.chromium.org/2565913002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:3217:
web_bluetooth_service.get();
I would probably not have this local variable, can call .get() directly as it
would be clearer to the reader.

https://codereview.chromium.org/2565913002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:3227:
WebBluetoothServiceImpl* web_bluetooth_service_ptr) {
remove _ptr

https://codereview.chromium.org/2565913002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2565913002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:814:
WebBluetoothServiceImpl* web_bluetooth_service_ptr);
nit: drop _ptr in the param name.

https://codereview.chromium.org/2565913002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:997:
web_bluetooth_services_;
I think this needs a comment.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp (right):

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp:217: 
What should we do if interfaceProvider is null here?

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothAttributeInstanceMap.h
(right):

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothAttributeInstanceMap.h:36:
const String& deviceInstanceId);
I think you can drop the parameter names here. (see rest of file)

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothAttributeInstanceMap.h:53:
BluetoothRemoteGATTService*);
same.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:154:
resolver->reject(BluetoothError::take(resolver, static_cast<int>(result)));
It would be great if BluetoothError::take would accept a WebBluetoothResult so
that we can avoid the cast?

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:213:
resolver->reject(BluetoothError::take(resolver, static_cast<int>(result)));
same

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:245:
valueVector.append(value.bytes(), value.byteLength());
Do we need this change?

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:251:
CHECK(gatt()->connected());
this is a crash if we're not connected here. Should we instead handle this case?

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:286:
resolver->reject(BluetoothError::take(resolver, static_cast<int>(result)));
same as above.  Would be nice if ::take() accepted WebBluetoothResult.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp
(right):

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:198:
CHECK(m_device->gatt()->connected());
Another crash if we're not connected. What do you think?

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp
(right):

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:73:
DCHECK(characteristics);
I don't understand this check.  characteristics is optional here?

[Reilly Grant (use Gerrit), 2016-12-16 01:41:29]

https://codereview.chromium.org/2565913002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2565913002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:997:
web_bluetooth_services_;
On 2016/12/16 00:11:44, dougt wrote:
> I think this needs a comment.

You can use std::unordered_set<std::unique_ptr<WebBluetoothServiceImpl>> here
with the following trick to do removals:

void DeleteWebBluetoothService(WebBluetoothServiceImpl* impl) {
  std::unique_ptr<WebBluetoothServiceImpl> key(impl);
  auto it = web_bluetooth_services_.find(key);
  key.release();
  web_bluetooth_services_.erase(it);
}

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js
(right):

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js:237:
function assert_event_fires_after_promise(object, func, event, num_listeners,
ignore_event_promise_order) {
Please document this new parameter.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp (right):

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp:129:
mojom::blink::WebBluetoothScanFilter::New();
auto canonicalizedFilter = mojom::blink::WebBluetoothScanFilter::New();

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp:173:
resolver->reject(BluetoothError::take(resolver, static_cast<int>(result)));
Update BluetoothError to take a mojom::blink::WebBluetoothResult.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp:209: interfaceProvider
= Platform::current()->interfaceProvider();
On 2016/12/15 23:54:05, esprehn wrote:
> Hmm, we probably need to have interfaceProvider() available for workers
> eventually

WebBluetooth isn't available in workers so we should just fail to initialize the
service if !executionContext->isDocument().

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp:215:
interfaceProvider->getInterface(std::move(request));
interfaceProvider->getInterface(mojo::GetProxy(&m_service));

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp:217: 
On 2016/12/16 00:11:44, dougt wrote:
> What should we do if interfaceProvider is null here?

m_service remains null and we reject the promise with a DOMException.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp:236:
mojom::blink::WebBluetoothRequestDeviceOptions::New();
auto deviceOptions = mojom::blink::WebBluetoothRequestDeviceOptions::New();

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothAttributeInstanceMap.h
(right):

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothAttributeInstanceMap.h:36:
const String& deviceInstanceId);
On 2016/12/16 00:11:44, dougt wrote:
> I think you can drop the parameter names here. (see rest of file)

I'd prefer we leave the parameter names when they're all strings, ints or bools
as otherwise the header is useless as documentation.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.cpp (right):

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.cpp:89:
mojom::blink::WebBluetoothDeviceId::New();
auto deviceId = mojom::blink::WebBluetoothDeviceId::New();

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:134:
return;
This check is unnecessary. It is performed by
ScriptPromiseResolver::resolve/reject.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:195:
return;
This check is unnecessary. It is performed by
ScriptPromiseResolver::resolve/reject.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:269:
return;
This check is unnecessary. It is performed by
ScriptPromiseResolver::resolve/reject.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp
(right):

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:63:
return;
This check is unnecessary. It is performed by
ScriptPromiseResolver::resolve/reject.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:81:
createWebBluetoothDeviceIdPtr(device()->id());
Don't put "Ptr" in the variable name.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:111:
return;
This check is unnecessary. It is performed by
ScriptPromiseResolver::resolve/reject.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp
(right):

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:62:
return;
This check is unnecessary. It is performed by
ScriptPromiseResolver::resolve/reject.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothUtils.h (right):

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothUtils.h:13:
bluetooth::mojom::blink::UUIDPtr createUUIDPtr(const String& uuid);
Move this to BluetoothUUID::createMojoUuid().

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothUtils.h:15:
mojom::blink::WebBluetoothDeviceIdPtr createWebBluetoothDeviceIdPtr(
Move this to BluetoothDevice::createMojoDeviceId().

[scheib, 2016-12-16 06:33:17]

https://codereview.chromium.org/2565913002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2565913002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:997:
web_bluetooth_services_;
On 2016/12/16 01:41:28, Reilly Grant wrote:
> On 2016/12/16 00:11:44, dougt wrote:
> > I think this needs a comment.
> 
> You can use std::unordered_set<std::unique_ptr<WebBluetoothServiceImpl>> here
> with the following trick to do removals:
> 
> void DeleteWebBluetoothService(WebBluetoothServiceImpl* impl) {
>   std::unique_ptr<WebBluetoothServiceImpl> key(impl);
>   auto it = web_bluetooth_services_.find(key);
>   key.release();
>   web_bluetooth_services_.erase(it);
> }

;\ This is fairly hacky too.
How about find_if to remove:

    std::find_if(set.begin(), set.end(), 
      [to_remove](const std::unique_ptr<...>& i) -> bool { 
          return i.get() == to_remove; });

[esprehn, 2016-12-16 07:41:29]

https://codereview.chromium.org/2565913002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2565913002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:997:
web_bluetooth_services_;
On 2016/12/16 at 06:33:17, scheib wrote:
> On 2016/12/16 01:41:28, Reilly Grant wrote:
> > On 2016/12/16 00:11:44, dougt wrote:
> > > I think this needs a comment.
> > 
> > You can use std::unordered_set<std::unique_ptr<WebBluetoothServiceImpl>>
here
> > with the following trick to do removals:
> > 
> > void DeleteWebBluetoothService(WebBluetoothServiceImpl* impl) {
> >   std::unique_ptr<WebBluetoothServiceImpl> key(impl);
> >   auto it = web_bluetooth_services_.find(key);
> >   key.release();
> >   web_bluetooth_services_.erase(it);
> > }
> 
> ;\ This is fairly hacky too.
> How about find_if to remove:
> 
>     std::find_if(set.begin(), set.end(), 
>       [to_remove](const std::unique_ptr<...>& i) -> bool { 
>           return i.get() == to_remove; });

Thats bad because it's doing something linear for no reason other than to work
around the stl being terrible when you have a container of unique_ptr. We should
wrap the pattern of putting the object in a wrapper and then releasing it in a
function in the base STLUtils.h and use it when we need it. We could also use
our own containers that don't have this limitation (ex. wtf ones allow
comparison between ptr wrapped types).

[haraken, 2016-12-16 11:24:04]

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

[haraken, 2016-12-16 11:24:05]

Drive-by:

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp
(right):

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:63:
return;
On 2016/12/16 01:41:29, Reilly Grant wrote:
> This check is unnecessary. It is performed by
> ScriptPromiseResolver::resolve/reject.

Is it okay to call m_device->gatt()->setConnected(true) when the context is
detached?

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:111:
return;
On 2016/12/16 01:41:29, Reilly Grant wrote:
> This check is unnecessary. It is performed by
> ScriptPromiseResolver::resolve/reject.

Ditto. In general, it would be better to check this at the beginning of any
callback.

[juncai, 2016-12-17 01:17:30]

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

[commit-bot: I haz the power, 2016-12-17 01:17:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[juncai, 2016-12-17 01:18:53]

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

https://codereview.chromium.org/2565913002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:3217:
web_bluetooth_service.get();
On 2016/12/16 00:11:44, dougt wrote:
> I would probably not have this local variable, can call .get() directly as it
> would be clearer to the reader.

Done.

https://codereview.chromium.org/2565913002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:3227:
WebBluetoothServiceImpl* web_bluetooth_service_ptr) {
On 2016/12/16 00:11:44, dougt wrote:
> remove _ptr

Done.

https://codereview.chromium.org/2565913002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2565913002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:814:
WebBluetoothServiceImpl* web_bluetooth_service_ptr);
On 2016/12/16 00:11:44, dougt wrote:
> nit: drop _ptr in the param name.

Done.

https://codereview.chromium.org/2565913002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:997:
web_bluetooth_services_;
On 2016/12/16 07:41:29, esprehn wrote:
> On 2016/12/16 at 06:33:17, scheib wrote:
> > On 2016/12/16 01:41:28, Reilly Grant wrote:
> > > On 2016/12/16 00:11:44, dougt wrote:
> > > > I think this needs a comment.
> > > 
> > > You can use std::unordered_set<std::unique_ptr<WebBluetoothServiceImpl>>
> here
> > > with the following trick to do removals:
> > > 
> > > void DeleteWebBluetoothService(WebBluetoothServiceImpl* impl) {
> > >   std::unique_ptr<WebBluetoothServiceImpl> key(impl);
> > >   auto it = web_bluetooth_services_.find(key);
> > >   key.release();
> > >   web_bluetooth_services_.erase(it);
> > > }
> > 
> > ;\ This is fairly hacky too.
> > How about find_if to remove:
> > 
> >     std::find_if(set.begin(), set.end(), 
> >       [to_remove](const std::unique_ptr<...>& i) -> bool { 
> >           return i.get() == to_remove; });
> 
> Thats bad because it's doing something linear for no reason other than to work
> around the stl being terrible when you have a container of unique_ptr. We
should
> wrap the pattern of putting the object in a wrapper and then releasing it in a
> function in the base STLUtils.h and use it when we need it. We could also use
> our own containers that don't have this limitation (ex. wtf ones allow
> comparison between ptr wrapped types).

Use the same method that is used at:
https://cs.chromium.org/chromium/src/third_party/pdfium/fpdfsdk/javascript/ap...
https://cs.chromium.org/chromium/src/third_party/pdfium/third_party/base/stl_...
So added class FakeUniquePtr to //base/stl_util.h

Done.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js
(right):

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js:237:
function assert_event_fires_after_promise(object, func, event, num_listeners,
ignore_event_promise_order) {
On 2016/12/16 01:41:28, Reilly Grant wrote:
> Please document this new parameter.

Done.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp (right):

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp:129:
mojom::blink::WebBluetoothScanFilter::New();
On 2016/12/16 01:41:29, Reilly Grant wrote:
> auto canonicalizedFilter = mojom::blink::WebBluetoothScanFilter::New();

Done.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp:173:
resolver->reject(BluetoothError::take(resolver, static_cast<int>(result)));
On 2016/12/16 01:41:29, Reilly Grant wrote:
> Update BluetoothError to take a mojom::blink::WebBluetoothResult.

Done.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp:209: interfaceProvider
= Platform::current()->interfaceProvider();
On 2016/12/16 01:41:29, Reilly Grant wrote:
> On 2016/12/15 23:54:05, esprehn wrote:
> > Hmm, we probably need to have interfaceProvider() available for workers
> > eventually
> 
> WebBluetooth isn't available in workers so we should just fail to initialize
the
> service if !executionContext->isDocument().

Done.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp:215:
interfaceProvider->getInterface(std::move(request));
On 2016/12/16 01:41:28, Reilly Grant wrote:
> interfaceProvider->getInterface(mojo::GetProxy(&m_service));

Done.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp:236:
mojom::blink::WebBluetoothRequestDeviceOptions::New();
On 2016/12/16 01:41:29, Reilly Grant wrote:
> auto deviceOptions = mojom::blink::WebBluetoothRequestDeviceOptions::New();

Done.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothAttributeInstanceMap.h
(right):

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothAttributeInstanceMap.h:53:
BluetoothRemoteGATTService*);
On 2016/12/16 00:11:44, dougt wrote:
> same.

Based on reillyg@'s comment, keep the parameter names here.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.cpp (right):

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.cpp:89:
mojom::blink::WebBluetoothDeviceId::New();
On 2016/12/16 01:41:29, Reilly Grant wrote:
> auto deviceId = mojom::blink::WebBluetoothDeviceId::New();

Done.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:134:
return;
On 2016/12/16 01:41:29, Reilly Grant wrote:
> This check is unnecessary. It is performed by
> ScriptPromiseResolver::resolve/reject.

Keep this check if the callbacks do more than
ScriptPromiseResolver::resolve/reject.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:154:
resolver->reject(BluetoothError::take(resolver, static_cast<int>(result)));
On 2016/12/16 00:11:44, dougt wrote:
> It would be great if BluetoothError::take would accept a WebBluetoothResult so
> that we can avoid the cast?

Done.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:195:
return;
On 2016/12/16 01:41:29, Reilly Grant wrote:
> This check is unnecessary. It is performed by
> ScriptPromiseResolver::resolve/reject.

ditto.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:213:
resolver->reject(BluetoothError::take(resolver, static_cast<int>(result)));
On 2016/12/16 00:11:44, dougt wrote:
> same

Done.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:245:
valueVector.append(value.bytes(), value.byteLength());
On 2016/12/16 00:11:44, dougt wrote:
> Do we need this change?

I think so.
https://cs.chromium.org/chromium/src/out/Debug/gen/third_party/WebKit/public/...
The WebBluetoothService::RemoteCharacteristicWriteValue() takes a WTF::Vector as
a parameter instead of a WebVector.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:251:
CHECK(gatt()->connected());
On 2016/12/16 00:11:44, dougt wrote:
> this is a crash if we're not connected here. Should we instead handle this
case?

I think this line can just be removed since this case is already handled on line
220.

Done.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:269:
return;
On 2016/12/16 01:41:29, Reilly Grant wrote:
> This check is unnecessary. It is performed by
> ScriptPromiseResolver::resolve/reject.

ditto.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:286:
resolver->reject(BluetoothError::take(resolver, static_cast<int>(result)));
On 2016/12/16 00:11:44, dougt wrote:
> same as above.  Would be nice if ::take() accepted WebBluetoothResult.

Done.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp
(right):

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:63:
return;
On 2016/12/16 11:24:05, haraken wrote:
> On 2016/12/16 01:41:29, Reilly Grant wrote:
> > This check is unnecessary. It is performed by
> > ScriptPromiseResolver::resolve/reject.
> 
> Is it okay to call m_device->gatt()->setConnected(true) when the context is
> detached?

I think when context is detached, the m_device->gatt() may be garbage collected.
So it is necessary to have the above check.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:81:
createWebBluetoothDeviceIdPtr(device()->id());
On 2016/12/16 01:41:29, Reilly Grant wrote:
> Don't put "Ptr" in the variable name.

Done.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:111:
return;
On 2016/12/16 11:24:05, haraken wrote:
> On 2016/12/16 01:41:29, Reilly Grant wrote:
> > This check is unnecessary. It is performed by
> > ScriptPromiseResolver::resolve/reject.
> 
> Ditto. In general, it would be better to check this at the beginning of any
> callback.

For callbacks that do more than ScriptPromiseResolver::resolve/reject, keep this
check at the beginning of the callback.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:198:
CHECK(m_device->gatt()->connected());
On 2016/12/16 00:11:44, dougt wrote:
> Another crash if we're not connected. What do you think?

I think this line can just be removed since this case is already handled on line
188.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp
(right):

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:62:
return;
On 2016/12/16 01:41:29, Reilly Grant wrote:
> This check is unnecessary. It is performed by
> ScriptPromiseResolver::resolve/reject.

Keep this check if the callbacks do more than
ScriptPromiseResolver::resolve/reject.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:73:
DCHECK(characteristics);
On 2016/12/16 00:11:45, dougt wrote:
> I don't understand this check.  characteristics is optional here?

This DCHECK() is done inside the above if statement:
if (result == mojom::blink::WebBluetoothResult::SUCCESS)
So in this case, |characteristics| is supposed to be valid.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothUtils.h (right):

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothUtils.h:13:
bluetooth::mojom::blink::UUIDPtr createUUIDPtr(const String& uuid);
On 2016/12/16 01:41:29, Reilly Grant wrote:
> Move this to BluetoothUUID::createMojoUuid().

Done.

https://codereview.chromium.org/2565913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothUtils.h:15:
mojom::blink::WebBluetoothDeviceIdPtr createWebBluetoothDeviceIdPtr(
On 2016/12/16 01:41:29, Reilly Grant wrote:
> Move this to BluetoothDevice::createMojoDeviceId().

Done.

[juncai, 2016-12-17 01:37:22]

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

[commit-bot: I haz the power, 2016-12-17 01:37:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-17 03:38:53]

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

[commit-bot: I haz the power, 2016-12-17 03:38:54]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[Reilly Grant (use Gerrit), 2016-12-20 02:08:17]

Looking good.

https://codereview.chromium.org/2565913002/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2565913002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:3227:
base::FakeUniquePtr<WebBluetoothServiceImpl>(web_bluetooth_service));
Add a helper method base::MakeFakeUniquePtr() so that, like make_scoped_refptr,
you don't need to specify the type here.

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js
(right):

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js:239: //
ignore their relative order.
I think this explanation is backwards.

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothAttributeInstanceMap.cpp
(right):

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothAttributeInstanceMap.cpp:29:
deviceInstanceId, m_device.get());
.get() shouldn't be necessary here.

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp (right):

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp:15: error /*
Corresponds to WebBluetoothResult in web_bluetooth.mojom */) {
Same here.

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothError.h (right):

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothError.h:27: error /*
Corresponds to WebBluetoothResult in web_bluetooth.mojom */);
This comment is unnecessary now.

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:148:
if (!gatt()->RemoveFromActiveAlgorithms(resolver)) {
This check can be moved out to before you check result == SUCCESS.

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:205:
if (!gatt()->RemoveFromActiveAlgorithms(resolver)) {
Same here.

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:276:
if (!gatt()->RemoveFromActiveAlgorithms(resolver)) {
...and here.

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp
(right):

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:141:
if (!RemoveFromActiveAlgorithms(resolver)) {
This check can be hoisted out to above if (result == SUCCESS).

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:202:
bluetooth::mojom::blink::UUIDPtr uuid = nullptr;
No need to initialize to nullptr, that is the default.

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:204:
uuid = BluetoothUUID::createMojoUuid(servicesUUID);
I'm wondering whether it would be useful to just have createMojoUuid() return
nullptr if its input is empty.

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp
(right):

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:97:
if (!device()->gatt()->RemoveFromActiveAlgorithms(resolver)) {
This check can be hoisted.

[juncai, 2016-12-20 22:28:16]

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

[commit-bot: I haz the power, 2016-12-20 22:28:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[juncai, 2016-12-20 22:33:43]

https://codereview.chromium.org/2565913002/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2565913002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:3227:
base::FakeUniquePtr<WebBluetoothServiceImpl>(web_bluetooth_service));
On 2016/12/20 02:08:16, Reilly Grant wrote:
> Add a helper method base::MakeFakeUniquePtr() so that, like
make_scoped_refptr,
> you don't need to specify the type here.

Discussed offline with reillyg@, using base::FakeUniquePtr directly here seems a
better approach.

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js
(right):

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js:239: //
ignore their relative order.
On 2016/12/20 02:08:16, Reilly Grant wrote:
> I think this explanation is backwards.

Done.

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothAttributeInstanceMap.cpp
(right):

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothAttributeInstanceMap.cpp:29:
deviceInstanceId, m_device.get());
On 2016/12/20 02:08:17, Reilly Grant wrote:
> .get() shouldn't be necessary here.

Done.

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp (right):

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp:15: error /*
Corresponds to WebBluetoothResult in web_bluetooth.mojom */) {
On 2016/12/20 02:08:17, Reilly Grant wrote:
> Same here.

Done.

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothError.h (right):

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothError.h:27: error /*
Corresponds to WebBluetoothResult in web_bluetooth.mojom */);
On 2016/12/20 02:08:17, Reilly Grant wrote:
> This comment is unnecessary now.

Done.

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:148:
if (!gatt()->RemoveFromActiveAlgorithms(resolver)) {
On 2016/12/20 02:08:17, Reilly Grant wrote:
> This check can be moved out to before you check result == SUCCESS.

Done.

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:205:
if (!gatt()->RemoveFromActiveAlgorithms(resolver)) {
On 2016/12/20 02:08:17, Reilly Grant wrote:
> Same here.

Done.

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:276:
if (!gatt()->RemoveFromActiveAlgorithms(resolver)) {
On 2016/12/20 02:08:17, Reilly Grant wrote:
> ...and here.

Done.

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp
(right):

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:141:
if (!RemoveFromActiveAlgorithms(resolver)) {
On 2016/12/20 02:08:17, Reilly Grant wrote:
> This check can be hoisted out to above if (result == SUCCESS).

Done.

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:202:
bluetooth::mojom::blink::UUIDPtr uuid = nullptr;
On 2016/12/20 02:08:17, Reilly Grant wrote:
> No need to initialize to nullptr, that is the default.

Done.

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp:204:
uuid = BluetoothUUID::createMojoUuid(servicesUUID);
On 2016/12/20 02:08:17, Reilly Grant wrote:
> I'm wondering whether it would be useful to just have createMojoUuid() return
> nullptr if its input is empty.

Done.

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp
(right):

https://codereview.chromium.org/2565913002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:97:
if (!device()->gatt()->RemoveFromActiveAlgorithms(resolver)) {
On 2016/12/20 02:08:17, Reilly Grant wrote:
> This check can be hoisted.

Done.

[juncai, 2016-12-20 22:37:29]

juncai@chromium.org changed reviewers:
+ clamy@chromium.org, dcheng@chromium.org

[juncai, 2016-12-20 22:37:30]

dcheng@chromium.org: Please review changes in 
//base/stl_util.h

clamy@chromium.org: Please review changes in 
//content/browser/frame_host
//content/renderer

[Reilly Grant (use Gerrit), 2016-12-20 22:42:33]

lgtm

[commit-bot: I haz the power, 2016-12-21 01:53:58]

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

[commit-bot: I haz the power, 2016-12-21 01:54:00]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[clamy, 2016-12-21 16:03:10]

Thanks! One question below.

https://codereview.chromium.org/2565913002/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2565913002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:3215:
std::unique_ptr<WebBluetoothServiceImpl> web_bluetooth_service =
Why do we go from 1 WebBluetoothServiceImpl to several WebBluetoothServiceImpls?

[juncai, 2016-12-21 18:06:40]

https://codereview.chromium.org/2565913002/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2565913002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:3215:
std::unique_ptr<WebBluetoothServiceImpl> web_bluetooth_service =
On 2016/12/21 16:03:09, clamy wrote:
> Why do we go from 1 WebBluetoothServiceImpl to several
WebBluetoothServiceImpls?

The previous implementation is to have one WebBluetooth object:
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/modul...
which is provided to the frame by:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/ChromeClie...
And the WebBluetoothImpl implements the WebBluetooth:
https://cs.chromium.org/chromium/src/content/renderer/bluetooth/web_bluetooth...
So there is one web bluetooth service for the WebBluetooth object.
And the WebBluetooth object is accessed from BluetoothSupplement:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/blueto...

Now by doing onion soup, these extra layers are removed, so multiple web
bluetooth services are possible and each service corresponds to a Bluetooth
object, and the service is initialized when the Bluetooth:requestDevice()
function is called.

[juncai, 2016-12-21 21:53:42]

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

[commit-bot: I haz the power, 2016-12-21 21:54:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 01:58:47]

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

[commit-bot: I haz the power, 2016-12-22 01:58:49]

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

[dcheng, 2016-12-22 08:37:21]

https://codereview.chromium.org/2565913002/diff/160001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2565913002/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:3221: return
it.first->get();
I would suggest using a container of raw pointers and make
WebBluetoothServiceImpl owned by a strong binding. The dtor can clear itself
from RenderFrameHostImpl.

I think you're going to have a hard time convincing //base OWNERS to accept
FakeUniquePtr =)

https://codereview.chromium.org/2565913002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp (right):

https://codereview.chromium.org/2565913002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp:229:
convertRequestDeviceOptions(options, deviceOptions, exceptionState);
Is it possible to typemap WebBluetoothRequestDeviceOptions? I guess it's a bit
trickier here, since the conversion function also does validation, but we should
be able to separate the two bits of logic.

https://codereview.chromium.org/2565913002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp
(right):

https://codereview.chromium.org/2565913002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:76:
resolver->resolve(device()->getOrCreateBluetoothRemoteGATTCharacteristic(
Btw, I'd consider trying to shorten some of these method names (in a followup):
|device| is already a BluetoothDevice, so the method name can just be
getOrCreateRemoteGATTCharacteristic().

https://codereview.chromium.org/2565913002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:76:
resolver->resolve(device()->getOrCreateBluetoothRemoteGATTCharacteristic(
Though this is renderer-side code, it would be nice to use typemap rather than
writing marshalling code like this. Is there a reason we cache characteristics
on a device map?

https://codereview.chromium.org/2565913002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:137:
String characteristicsUUID) {
Nit: const String&

https://codereview.chromium.org/2565913002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:157:
uuid = BluetoothUUID::createMojoUuid(characteristicsUUID);
Is it possible to typemap this to String on the Blink side?

https://codereview.chromium.org/2565913002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:162:
wrapPersistent(this), m_serviceInstanceId, quantity,
wrapWeakPersistent(this)? If this object is going to be destroyed, there's no
point in invoking the callback.

https://codereview.chromium.org/2565913002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/DEPS (right):

https://codereview.chromium.org/2565913002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/DEPS:4:
"+device/bluetooth/public/interfaces",
esprehn, can we post the rules for Blink dependencies somewhere? It makes sense
that we'd allow this dependency now, but I'm a bit fuzzy on the actual rules.

[Reilly Grant (use Gerrit), 2016-12-22 16:23:14]

https://codereview.chromium.org/2565913002/diff/160001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2565913002/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:3221: return
it.first->get();
On 2016/12/22 08:37:20, dcheng wrote:
> I would suggest using a container of raw pointers and make
> WebBluetoothServiceImpl owned by a strong binding. The dtor can clear itself
> from RenderFrameHostImpl.
> 
> I think you're going to have a hard time convincing //base OWNERS to accept
> FakeUniquePtr =)

That method would suggest using the now removed STLDeleteContainerPointers to
free all the raw pointers in RenderFrameHostImpl's destructor.

FakeUniquePtr is already part of stl_util.h in pdfium.

https://codereview.chromium.org/2565913002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp
(right):

https://codereview.chromium.org/2565913002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:162:
wrapPersistent(this), m_serviceInstanceId, quantity,
On 2016/12/22 08:37:20, dcheng wrote:
> wrapWeakPersistent(this)? If this object is going to be destroyed, there's no
> point in invoking the callback.

But if script doesn't keep a reference to this object should its promise really
never get resolved?

[clamy, 2016-12-22 17:00:57]

https://codereview.chromium.org/2565913002/diff/160001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2565913002/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:3221: return
it.first->get();
On 2016/12/22 16:23:13, Reilly Grant wrote:
> On 2016/12/22 08:37:20, dcheng wrote:
> > I would suggest using a container of raw pointers and make
> > WebBluetoothServiceImpl owned by a strong binding. The dtor can clear itself
> > from RenderFrameHostImpl.
> > 
> > I think you're going to have a hard time convincing //base OWNERS to accept
> > FakeUniquePtr =)
> 
> That method would suggest using the now removed STLDeleteContainerPointers to
> free all the raw pointers in RenderFrameHostImpl's destructor.
> 
> FakeUniquePtr is already part of stl_util.h in pdfium.

Why not use a map with an explicit id as a key? This would also avoid issues
with pointer comparisons such as the allocator reusing the same pointer for
objects of the same type (which it is prone to do and has caused issues in the
past).

[juncai, 2016-12-22 21:49:26]

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

[commit-bot: I haz the power, 2016-12-22 21:49:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[juncai, 2016-12-22 22:15:40]

https://codereview.chromium.org/2565913002/diff/160001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2565913002/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:3221: return
it.first->get();
On 2016/12/22 08:37:20, dcheng wrote:
> I would suggest using a container of raw pointers and make
> WebBluetoothServiceImpl owned by a strong binding. The dtor can clear itself
> from RenderFrameHostImpl.
> 
> I think you're going to have a hard time convincing //base OWNERS to accept
> FakeUniquePtr =)

Changed it to use std::list<std::unique_ptr<WebBluetoothServiceImpl>>. And
removed the FakeUniquePtr at stl_util.h.

https://codereview.chromium.org/2565913002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp (right):

https://codereview.chromium.org/2565913002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/Bluetooth.cpp:229:
convertRequestDeviceOptions(options, deviceOptions, exceptionState);
On 2016/12/22 08:37:20, dcheng wrote:
> Is it possible to typemap WebBluetoothRequestDeviceOptions? I guess it's a bit
> trickier here, since the conversion function also does validation, but we
should
> be able to separate the two bits of logic.

Will try this in a follow-up CL, and filed a bug for it:
https://bugs.chromium.org/p/chromium/issues/detail?id=676702

https://codereview.chromium.org/2565913002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp
(right):

https://codereview.chromium.org/2565913002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:76:
resolver->resolve(device()->getOrCreateBluetoothRemoteGATTCharacteristic(
On 2016/12/22 08:37:20, dcheng wrote:
> Btw, I'd consider trying to shorten some of these method names (in a
followup):
> |device| is already a BluetoothDevice, so the method name can just be
> getOrCreateRemoteGATTCharacteristic().

Thanks! I filed a bug for this and will do it in a follow-up CL:
https://bugs.chromium.org/p/chromium/issues/detail?id=676703

https://codereview.chromium.org/2565913002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:76:
resolver->resolve(device()->getOrCreateBluetoothRemoteGATTCharacteristic(
On 2016/12/22 08:37:20, dcheng wrote:
> Though this is renderer-side code, it would be nice to use typemap rather than
> writing marshalling code like this. Is there a reason we cache characteristics
> on a device map?

Thanks! I will try this in a follow-up CL, and filed a bug for it:
https://bugs.chromium.org/p/chromium/issues/detail?id=676702

By caching the characteristics on a device map, users of Web Bluetooth can check
which characteristic a device has and use it to construct a GATT service.

https://codereview.chromium.org/2565913002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:137:
String characteristicsUUID) {
On 2016/12/22 08:37:20, dcheng wrote:
> Nit: const String&

Done.

https://codereview.chromium.org/2565913002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:157:
uuid = BluetoothUUID::createMojoUuid(characteristicsUUID);
On 2016/12/22 08:37:20, dcheng wrote:
> Is it possible to typemap this to String on the Blink side?

I am not sure about this, since the uuid is already a typemap to
device:BluetoothUUID:
https://cs.chromium.org/chromium/src/device/bluetooth/public/interfaces/uuid....

[commit-bot: I haz the power, 2016-12-23 00:03:10]

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

[commit-bot: I haz the power, 2016-12-23 00:03:11]

Dry run: This issue passed the CQ dry run.

[clamy, 2016-12-23 12:27:33]

Thanks! Lgtm.

[juncai, 2016-12-27 17:57:55]

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

[commit-bot: I haz the power, 2016-12-27 17:58:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-27 19:50:27]

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

[commit-bot: I haz the power, 2016-12-27 19:50:29]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-12-27 20:37:37]

https://codereview.chromium.org/2565913002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp
(right):

https://codereview.chromium.org/2565913002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:76:
resolver->resolve(device()->getOrCreateBluetoothRemoteGATTCharacteristic(
On 2016/12/22 22:15:40, juncai wrote:
> On 2016/12/22 08:37:20, dcheng wrote:
> > Though this is renderer-side code, it would be nice to use typemap rather
than
> > writing marshalling code like this. Is there a reason we cache
characteristics
> > on a device map?
> 
> Thanks! I will try this in a follow-up CL, and filed a bug for it:
> https://bugs.chromium.org/p/chromium/issues/detail?id=676702
> 
> By caching the characteristics on a device map, users of Web Bluetooth can
check
> which characteristic a device has and use it to construct a GATT service.

I'm just trying to understanding if the caching is a performance optimization or
something else.
If we just always typedmapped, it would reduce the amount of custom marshalling
code here, which is generally considered good.

https://codereview.chromium.org/2565913002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:157:
uuid = BluetoothUUID::createMojoUuid(characteristicsUUID);
On 2016/12/22 22:15:40, juncai wrote:
> On 2016/12/22 08:37:20, dcheng wrote:
> > Is it possible to typemap this to String on the Blink side?
> 
> I am not sure about this, since the uuid is already a typemap to
> device:BluetoothUUID:
>
https://cs.chromium.org/chromium/src/device/bluetooth/public/interfaces/uuid....

Chromium-side and Blink-side typemaps are registered in different places, so
this should be doable.

https://codereview.chromium.org/2565913002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:162:
wrapPersistent(this), m_serviceInstanceId, quantity,
On 2016/12/22 16:23:14, Reilly Grant wrote:
> On 2016/12/22 08:37:20, dcheng wrote:
> > wrapWeakPersistent(this)? If this object is going to be destroyed, there's
no
> > point in invoking the callback.
> 
> But if script doesn't keep a reference to this object should its promise
really
> never get resolved?

OK, fair enough--I'd expect that if you had an otherwise reference-less promise,
the promise should still be resolved.

https://codereview.chromium.org/2565913002/diff/200001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2565913002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:3236:
std::unique_ptr<WebBluetoothServiceImpl> web_bluetooth_service =
Nit: OK to use auto here.

https://codereview.chromium.org/2565913002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:3241:
web_bluetooth_services_.push_back(std::move(web_bluetooth_service));
That being said, I've read the code a bit more: I'm wondering why this isn't
just using strong binding? Why does the RFH need to maintain a list of these?

Is it because there are concerns about the lifetimes of the bluetooth service vs
the RFH?

[Reilly Grant (use Gerrit), 2016-12-27 20:45:47]

https://codereview.chromium.org/2565913002/diff/200001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2565913002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:3241:
web_bluetooth_services_.push_back(std::move(web_bluetooth_service));
On 2016/12/27 20:37:37, dcheng wrote:
> That being said, I've read the code a bit more: I'm wondering why this isn't
> just using strong binding? Why does the RFH need to maintain a list of these?
> 
> Is it because there are concerns about the lifetimes of the bluetooth service
vs
> the RFH?

Yes, this is a common pattern for Mojo service implementations that hold a
RenderFrameHost* (or RenderProcessHost*).

[juncai, 2016-12-27 21:50:40]

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

[commit-bot: I haz the power, 2016-12-27 21:50:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[juncai, 2016-12-27 21:55:49]

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

[juncai, 2016-12-27 21:55:50]

rockot@chromium.org: Please review changes in 
//mojo/public/tools/bindings/blink_bindings_configuration.gni

https://codereview.chromium.org/2565913002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp
(right):

https://codereview.chromium.org/2565913002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:76:
resolver->resolve(device()->getOrCreateBluetoothRemoteGATTCharacteristic(
On 2016/12/27 20:37:37, dcheng wrote:
> On 2016/12/22 22:15:40, juncai wrote:
> > On 2016/12/22 08:37:20, dcheng wrote:
> > > Though this is renderer-side code, it would be nice to use typemap rather
> than
> > > writing marshalling code like this. Is there a reason we cache
> characteristics
> > > on a device map?
> > 
> > Thanks! I will try this in a follow-up CL, and filed a bug for it:
> > https://bugs.chromium.org/p/chromium/issues/detail?id=676702
> > 
> > By caching the characteristics on a device map, users of Web Bluetooth can
> check
> > which characteristic a device has and use it to construct a GATT service.
> 
> I'm just trying to understanding if the caching is a performance optimization
or
> something else.
> If we just always typedmapped, it would reduce the amount of custom
marshalling
> code here, which is generally considered good.

Yes, it is a performance optimization.

https://codereview.chromium.org/2565913002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:157:
uuid = BluetoothUUID::createMojoUuid(characteristicsUUID);
On 2016/12/27 20:37:37, dcheng wrote:
> On 2016/12/22 22:15:40, juncai wrote:
> > On 2016/12/22 08:37:20, dcheng wrote:
> > > Is it possible to typemap this to String on the Blink side?
> > 
> > I am not sure about this, since the uuid is already a typemap to
> > device:BluetoothUUID:
> >
>
https://cs.chromium.org/chromium/src/device/bluetooth/public/interfaces/uuid....
> 
> Chromium-side and Blink-side typemaps are registered in different places, so
> this should be doable.

Thanks! I added the typemap for this.

Done.

https://codereview.chromium.org/2565913002/diff/200001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2565913002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:3236:
std::unique_ptr<WebBluetoothServiceImpl> web_bluetooth_service =
On 2016/12/27 20:37:37, dcheng wrote:
> Nit: OK to use auto here.

Done.

[Ken Rockot(use gerrit already), 2016-12-27 22:02:05]

mojo GNI rs lgtm

[commit-bot: I haz the power, 2016-12-27 23:15:16]

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

[commit-bot: I haz the power, 2016-12-27 23:15:17]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-12-29 08:58:07]

https://codereview.chromium.org/2565913002/diff/220001/mojo/public/tools/bind...
File mojo/public/tools/bindings/blink_bindings_configuration.gni (right):

https://codereview.chromium.org/2565913002/diff/220001/mojo/public/tools/bind...
mojo/public/tools/bindings/blink_bindings_configuration.gni:11:
"//third_party/WebKit/Source/modules/bluetooth/typemaps.gni",
This doesn't feel quite right: it feels like the Blink typemaps should be
controlled by just a few gnis in Blink itself, and the Blink bindings
configuration depends on that. We can sort that out in a followup though.

https://codereview.chromium.org/2565913002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothUUIDTraits.h (right):

https://codereview.chromium.org/2565913002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothUUIDTraits.h:21: *output =
WTF::String(reinterpret_cast<const UChar*>(view.storage()),
Can this just be:

return data.ReadUuid(output);

Also, Read() is usually out-of-lined as well, since it can end up potentially
inlining a fair amount of other code.

[juncai, 2016-12-29 20:28:53]

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

[commit-bot: I haz the power, 2016-12-29 20:29:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-29 22:19:41]

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

[commit-bot: I haz the power, 2016-12-29 22:19:42]

Dry run: This issue passed the CQ dry run.

[juncai, 2016-12-29 22:22:59]

https://codereview.chromium.org/2565913002/diff/220001/mojo/public/tools/bind...
File mojo/public/tools/bindings/blink_bindings_configuration.gni (right):

https://codereview.chromium.org/2565913002/diff/220001/mojo/public/tools/bind...
mojo/public/tools/bindings/blink_bindings_configuration.gni:11:
"//third_party/WebKit/Source/modules/bluetooth/typemaps.gni",
On 2016/12/29 08:58:06, dcheng wrote:
> This doesn't feel quite right: it feels like the Blink typemaps should be
> controlled by just a few gnis in Blink itself, and the Blink bindings
> configuration depends on that. We can sort that out in a followup though.

Done.

https://codereview.chromium.org/2565913002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothUUIDTraits.h (right):

https://codereview.chromium.org/2565913002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothUUIDTraits.h:21: *output =
WTF::String(reinterpret_cast<const UChar*>(view.storage()),
On 2016/12/29 08:58:06, dcheng wrote:
> Can this just be:
> 
> return data.ReadUuid(output);
> 
> Also, Read() is usually out-of-lined as well, since it can end up potentially
> inlining a fair amount of other code.

Done.

[dcheng, 2016-12-30 09:19:06]

mojo lgtm with comments addressed

https://codereview.chromium.org/2565913002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mojo/BluetoothUUID.typemap (right):

https://codereview.chromium.org/2565913002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mojo/BluetoothUUID.typemap:1: # Copyright
2016 The Chromium Authors. All rights reserved.
Nit: let's just call this Bluetooth.typemap, since there are other bluetooth
things we'll probably typemap.

https://codereview.chromium.org/2565913002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mojo/BluetoothUUIDTraits.h (right):

https://codereview.chromium.org/2565913002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mojo/BluetoothUUIDTraits.h:1: // Copyright
2016 The Chromium Authors. All rights reserved.
Name this file with StructTraits.h suffix instead of Traits.h, to make sure it's
covered by OWNERS checks.

[juncai, 2017-01-03 18:07:02]

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

[commit-bot: I haz the power, 2017-01-03 18:07:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-03 19:53:40]

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

[commit-bot: I haz the power, 2017-01-03 19:53:41]

Dry run: This issue passed the CQ dry run.

[juncai, 2017-01-03 19:57:43]

https://codereview.chromium.org/2565913002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mojo/BluetoothUUID.typemap (right):

https://codereview.chromium.org/2565913002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mojo/BluetoothUUID.typemap:1: # Copyright
2016 The Chromium Authors. All rights reserved.
On 2016/12/30 09:19:05, dcheng wrote:
> Nit: let's just call this Bluetooth.typemap, since there are other bluetooth
> things we'll probably typemap.

Done.

https://codereview.chromium.org/2565913002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mojo/BluetoothUUIDTraits.h (right):

https://codereview.chromium.org/2565913002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mojo/BluetoothUUIDTraits.h:1: // Copyright
2016 The Chromium Authors. All rights reserved.
On 2016/12/30 09:19:06, dcheng wrote:
> Name this file with StructTraits.h suffix instead of Traits.h, to make sure
it's
> covered by OWNERS checks.

Done.

[juncai, 2017-01-03 19:58:32]

The CQ bit was checked by juncai@chromium.org

[juncai, 2017-01-03 19:58:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
esprehn@chromium.org, reillyg@chromium.org, clamy@chromium.org,
rockot@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2565913002/#ps260001
(title: "renamed BluetoothUUID.typemap to Bluetooth.typemap")

[commit-bot: I haz the power, 2017-01-03 19:58:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-03 20:04:03]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1483473512846960,
"parent_rev": "2ed0338c86f418f4d7f72df3742d90eb9c1fc999", "commit_rev":
"3a553389ec23de4aed8be58c1dba5f76444f328c"}

[commit-bot: I haz the power, 2017-01-03 20:04:32]

Description was changed from

==========
[Onion Soup] Move WebBluetoothImpl from //content/renderer/bluetooth to Blink's
bluetooth module

The Mojo WebBluetoothService can be used directly from Blink. This CL
moves the WebBluetoothImpl from //content/renderer/bluetooth to
//third_party/WebKit/Source/modules/bluetooth.

BUG=597053, 561879
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[Onion Soup] Move WebBluetoothImpl from //content/renderer/bluetooth to Blink's
bluetooth module

The Mojo WebBluetoothService can be used directly from Blink. This CL
moves the WebBluetoothImpl from //content/renderer/bluetooth to
//third_party/WebKit/Source/modules/bluetooth.

BUG=597053, 561879
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2565913002
==========

[commit-bot: I haz the power, 2017-01-03 20:04:33]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2017-01-03 20:07:39]

Description was changed from

==========
[Onion Soup] Move WebBluetoothImpl from //content/renderer/bluetooth to Blink's
bluetooth module

The Mojo WebBluetoothService can be used directly from Blink. This CL
moves the WebBluetoothImpl from //content/renderer/bluetooth to
//third_party/WebKit/Source/modules/bluetooth.

BUG=597053, 561879
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2565913002
==========

to

==========
[Onion Soup] Move WebBluetoothImpl from //content/renderer/bluetooth to Blink's
bluetooth module

The Mojo WebBluetoothService can be used directly from Blink. This CL
moves the WebBluetoothImpl from //content/renderer/bluetooth to
//third_party/WebKit/Source/modules/bluetooth.

BUG=597053, 561879
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[commit-bot: I haz the power, 2017-01-03 20:07:41]

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