bluetooth: browser-side implementation of writeValue.

bluetooth: browser-side implementation of writeValue.

Chrome spec repo update: https://github.com/WebBluetoothChrome/web-bluetooth/pull/12

This is the second of three patches to implement writeValue():
 [1] http://crrev.com/1212943006 (Blink-side interface)
 [2] This patch.
 [3] http://crrev.com/1218803008 (Blink-side implementation)

BUG=507415
Committed: https://crrev.com/af85f60db9f8a89b2b4446e47c73f7cc44dcf4c6
Cr-Commit-Position: refs/heads/master@{#337912}

[ortuno, 2015-07-06 23:05:15]

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

[ortuno, 2015-07-06 23:05:16]

jyasskin: PTAL

[Jeffrey Yasskin, 2015-07-07 00:31:42]

lgtm

https://codereview.chromium.org/1217983004/diff/1/content/child/bluetooth/blu...
File content/child/bluetooth/bluetooth_dispatcher.cc (right):

https://codereview.chromium.org/1217983004/diff/1/content/child/bluetooth/blu...
content/child/bluetooth/bluetooth_dispatcher.cc:246: std::vector<uint8_t>
value(byteLength);
You can initialize this with "std::vector<uint8_t> value(bytes, bytes +
byteLength);"

[ortuno, 2015-07-07 00:34:23]

On 2015/07/07 at 00:31:42, jyasskin wrote:
> lgtm
> 
>
https://codereview.chromium.org/1217983004/diff/1/content/child/bluetooth/blu...
> File content/child/bluetooth/bluetooth_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/1217983004/diff/1/content/child/bluetooth/blu...
> content/child/bluetooth/bluetooth_dispatcher.cc:246: std::vector<uint8_t>
value(byteLength);
> You can initialize this with "std::vector<uint8_t> value(bytes, bytes +
byteLength);"

Ahh I spent quite a bit of time looking for another way to initialize this. Do
you have a link to this constructor in the documentation?

[ortuno, 2015-07-07 00:39:44]

On 2015/07/07 at 00:34:23, ortuno wrote:
> On 2015/07/07 at 00:31:42, jyasskin wrote:
> > lgtm
> > 
> >
https://codereview.chromium.org/1217983004/diff/1/content/child/bluetooth/blu...
> > File content/child/bluetooth/bluetooth_dispatcher.cc (right):
> > 
> >
https://codereview.chromium.org/1217983004/diff/1/content/child/bluetooth/blu...
> > content/child/bluetooth/bluetooth_dispatcher.cc:246: std::vector<uint8_t>
value(byteLength);
> > You can initialize this with "std::vector<uint8_t> value(bytes, bytes +
byteLength);"
> 
> Ahh I spent quite a bit of time looking for another way to initialize this. Do
you have a link to this constructor in the documentation?

Is it (4) here: http://en.cppreference.com/w/cpp/container/vector/vector ?

[Jeffrey Yasskin, 2015-07-07 00:47:57]

On 2015/07/07 00:39:44, ortuno wrote:
> On 2015/07/07 at 00:34:23, ortuno wrote:
> > On 2015/07/07 at 00:31:42, jyasskin wrote:
> > > lgtm
> > > 
> > >
>
https://codereview.chromium.org/1217983004/diff/1/content/child/bluetooth/blu...
> > > File content/child/bluetooth/bluetooth_dispatcher.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/1217983004/diff/1/content/child/bluetooth/blu...
> > > content/child/bluetooth/bluetooth_dispatcher.cc:246: std::vector<uint8_t>
> value(byteLength);
> > > You can initialize this with "std::vector<uint8_t> value(bytes, bytes +
> byteLength);"
> > 
> > Ahh I spent quite a bit of time looking for another way to initialize this.
Do
> you have a link to this constructor in the documentation?
> 
> Is it (4) here: http://en.cppreference.com/w/cpp/container/vector/vector ?

Yep.

[ortuno, 2015-07-07 15:41:21]

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

[ortuno, 2015-07-07 15:41:22]

@scheib: PTAL

[scheib, 2015-07-07 19:28:08]

LGTM

[ortuno, 2015-07-07 19:31:22]

ortuno@chromium.org changed reviewers:
+ asvitkine@chromium.org, nick@chromium.org, tsepez@chromium.org

[ortuno, 2015-07-07 19:31:23]

@nick: PTAL bad_message.h
@asvitkine: PTAL at histograms.xml
@tsepez: PTAL at content/common/bluetooth/bluetooth_messages.h

[ortuno, 2015-07-07 20:03:20]

ortuno@chromium.org changed reviewers:
+ palmer@chromium.org
- tsepez@chromium.org

[ortuno, 2015-07-07 20:03:20]

@palmer: tsepez is OOO would you mind taking a look at
content/common/bluetooth/bluetooth_messages.h?

[ncarter (slow), 2015-07-07 21:10:22]

bad_message.h and histograms.xml lgtm (I'm not an owner for the latter)

[Alexei Svitkine (slow), 2015-07-07 21:13:03]

histograms lgtm

[palmer, 2015-07-07 21:37:17]

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

https://codereview.chromium.org/1217983004/diff/40001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:460: // Kill the renderer
Nit: This comment is superfluous.

https://codereview.chromium.org/1217983004/diff/40001/content/common/bluetoot...
File content/common/bluetooth/bluetooth_messages.h (right):

https://codereview.chromium.org/1217983004/diff/40001/content/common/bluetoot...
content/common/bluetooth/bluetooth_messages.h:190: std::string /* error_message
*/)
It's generally best (smaller messages, faster) to send an error code enum and
let the recipient translate the enum value into a string at its appropriate UI
layer. The concern is more acute when the message is renderer -> browser (we
don't like to let renderers control what data is in browser memory any more than
necessary), but it's still a good practice regardless of message direction.

[ortuno, 2015-07-07 22:01:52]

Thanks!

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

https://codereview.chromium.org/1217983004/diff/40001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:460: // Kill the renderer
On 2015/07/07 at 21:37:16, palmer wrote:
> Nit: This comment is superfluous.

Done.

https://codereview.chromium.org/1217983004/diff/40001/content/common/bluetoot...
File content/common/bluetooth/bluetooth_messages.h (right):

https://codereview.chromium.org/1217983004/diff/40001/content/common/bluetoot...
content/common/bluetooth/bluetooth_messages.h:190: std::string /* error_message
*/)
On 2015/07/07 at 21:37:17, palmer wrote:
> It's generally best (smaller messages, faster) to send an error code enum and
let the recipient translate the enum value into a string at its appropriate UI
layer. The concern is more acute when the message is renderer -> browser (we
don't like to let renderers control what data is in browser memory any more than
necessary), but it's still a good practice regardless of message direction.

I considered that pattern, but there are 25 different error messages and having
such a big enum on both sides seemed unnecessary. I'm happy to use an enum
instead if you think it's necessary. Since this would be a non trivial
refactoring would it be OK to do it in a follow up CL?

[ortuno, 2015-07-08 00:29:52]

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

[ortuno, 2015-07-08 00:29:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyasskin@chromium.org, nick@chromium.org, asvitkine@chromium.org,
scheib@chromium.org
Link to the patchset: https://codereview.chromium.org/1217983004/#ps60001
(title: "Remove comment")

[commit-bot: I haz the power, 2015-07-08 00:39:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1217983004/60001

[commit-bot: I haz the power, 2015-07-08 01:08:31]

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

[commit-bot: I haz the power, 2015-07-08 01:08:32]

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

(exceeded global retry quota)

[ortuno, 2015-07-08 14:04:22]

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

[commit-bot: I haz the power, 2015-07-08 14:04:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1217983004/60001

[commit-bot: I haz the power, 2015-07-08 15:10:00]

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

[commit-bot: I haz the power, 2015-07-08 15:10:01]

Dry run: This issue passed the CQ dry run.

[Jeffrey Yasskin, 2015-07-08 15:29:37]

On 2015/07/07 22:01:52, ortuno wrote:
> Thanks!
> 
>
https://codereview.chromium.org/1217983004/diff/40001/content/browser/bluetoo...
> File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):
> 
>
https://codereview.chromium.org/1217983004/diff/40001/content/browser/bluetoo...
> content/browser/bluetooth/bluetooth_dispatcher_host.cc:460: // Kill the
renderer
> On 2015/07/07 at 21:37:16, palmer wrote:
> > Nit: This comment is superfluous.
> 
> Done.
> 
>
https://codereview.chromium.org/1217983004/diff/40001/content/common/bluetoot...
> File content/common/bluetooth/bluetooth_messages.h (right):
> 
>
https://codereview.chromium.org/1217983004/diff/40001/content/common/bluetoot...
> content/common/bluetooth/bluetooth_messages.h:190: std::string /*
error_message
> */)
> On 2015/07/07 at 21:37:17, palmer wrote:
> > It's generally best (smaller messages, faster) to send an error code enum
and
> let the recipient translate the enum value into a string at its appropriate UI
> layer. The concern is more acute when the message is renderer -> browser (we
> don't like to let renderers control what data is in browser memory any more
than
> necessary), but it's still a good practice regardless of message direction.
> 
> I considered that pattern, but there are 25 different error messages and
having
> such a big enum on both sides seemed unnecessary. I'm happy to use an enum
> instead if you think it's necessary. Since this would be a non trivial
> refactoring would it be OK to do it in a follow up CL?

Having a lot of messages wouldn't be a good reason to avoid the enum. I was
thinking we should send a string because some of the messages might be dynamic
(saying things like "Device with address X isn't connectable"), but a) that
would need to go into the "unsanitized_message" parameter of the DOMException
constructor, while existing messages go into the sanitized_message parameter,
and b) we could send any information the renderer doesn't already have in
explicit extra response parameters, exactly when we figure out what needs to be
formatted into the message string.

So +1 to palmer's suggestion, but also +1 to doing this in a separate patch,
since it affects many more IPCs than just the ones Gio's adding in this patch.

[ortuno, 2015-07-08 19:56:40]

@palmer: ping?

[palmer, 2015-07-08 21:06:10]

LGTM on the condition that you switch to the enum approach in a second CL
(please CC me on that/on the bug for it). Thanks!

[ortuno, 2015-07-08 21:26:51]

The CQ bit was checked by ortuno@chromium.org

[commit-bot: I haz the power, 2015-07-08 21:29:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1217983004/60001

[commit-bot: I haz the power, 2015-07-08 21:35:29]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-07-08 21:36:29]

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