Use array<uint8> rather than string to pass BroadcastChannel messages.

Use array<uint8> rather than string to pass BroadcastChannel messages.

Even though SerializedScriptValue normally serializes values to a WTF::String,
the string isn't actually a valid unicode string. Because of that it can't
be safely passed over mojo as a string (invalid strings get silently converted
to null). This changes the mojo interface to use array<uint8> instead, thereby
making sure arbitrary values get passed across correctly.

BUG=630203
Committed: https://crrev.com/af71dd08b3a335dd78e541e03c379e99a83f57a7
Cr-Commit-Position: refs/heads/master@{#407048}

[Marijn Kruisselbrink, 2016-07-21 17:11:23]

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

[commit-bot: I haz the power, 2016-07-21 17:11:42]

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

[Marijn Kruisselbrink, 2016-07-21 17:15:01]

mek@chromium.org changed reviewers:
+ dcheng@chromium.org, rockot@chromium.org

[Marijn Kruisselbrink, 2016-07-21 17:15:01]

rocket: to sanity check that array<uint8> is indeed the canonical way in mojo to
pass binary data
dcheng: for mojom OWNERS

[Ken Rockot(use gerrit already), 2016-07-21 17:21:35]

lgtm

optionally since you're changing this mojom and the API surface is still very
small, you could remove use_new_wrapper_types = false from the public/interfaces
BUILD.gn, and then the generated bindings will use less stupid types (e.g.
WebVector/std::vector and WebString/std::string) instead of mojo::Array and
mojo::String.

[Ken Rockot(use gerrit already), 2016-07-21 17:23:43]

On 2016/07/21 at 17:21:35, Ken Rockot wrote:
> lgtm
> 
> optionally since you're changing this mojom and the API surface is still very
small, you could remove use_new_wrapper_types = false from the public/interfaces
BUILD.gn, and then the generated bindings will use less stupid types (e.g.
WebVector/std::vector and WebString/std::string) instead of mojo::Array and
mojo::String.

Errr mojo::WTFArray / WTFString for the blink types

[Marijn Kruisselbrink, 2016-07-21 17:28:34]

On 2016/07/21 at 17:23:43, rockot wrote:
> On 2016/07/21 at 17:21:35, Ken Rockot wrote:

> > optionally since you're changing this mojom and the API surface is still
very small, you could remove use_new_wrapper_types = false from the
public/interfaces BUILD.gn, and then the generated bindings will use less stupid
types (e.g. WebVector/std::vector and WebString/std::string) instead of
mojo::Array and mojo::String.
> 
> Errr mojo::WTFArray / WTFString for the blink types

I would, except that there are 14 other mojom files part of the same target
(pretty much all the mojom files that blink exports).

[Ken Rockot(use gerrit already), 2016-07-21 17:30:16]

On 2016/07/21 at 17:28:34, mek wrote:
> On 2016/07/21 at 17:23:43, rockot wrote:
> > On 2016/07/21 at 17:21:35, Ken Rockot wrote:
> 
> > > optionally since you're changing this mojom and the API surface is still
very small, you could remove use_new_wrapper_types = false from the
public/interfaces BUILD.gn, and then the generated bindings will use less stupid
types (e.g. WebVector/std::vector and WebString/std::string) instead of
mojo::Array and mojo::String.
> > 
> > Errr mojo::WTFArray / WTFString for the blink types
> 
> I would, except that there are 14 other mojom files part of the same target
(pretty much all the mojom files that blink exports).

Ah, sorry, nevermind then. We probably shouldn't have one monolithic target for
all mojom in blink, but that's a separate issue.

[commit-bot: I haz the power, 2016-07-21 18:30:45]

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

[commit-bot: I haz the power, 2016-07-21 18:30:46]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-07-22 02:33:03]

mojom lgtm

https://codereview.chromium.org/2171843002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp
(right):

https://codereview.chromium.org/2171843002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp:69:
mojoData.appendVector(data);
It's sad that this results in copying the data now, especially since we're just
going from char to uint8_t...

[Marijn Kruisselbrink, 2016-07-22 03:23:12]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2016-07-22 03:23:13]

https://codereview.chromium.org/2171843002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp
(right):

https://codereview.chromium.org/2171843002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp:69:
mojoData.appendVector(data);
On 2016/07/22 at 02:33:03, dcheng wrote:
> It's sad that this results in copying the data now, especially since we're
just going from char to uint8_t...

Yeah... but reinterpret_casting a Vector didn't seem like a great idea either. I
don't think this copy is really going to matter too much for the overall
performance anyway.

[commit-bot: I haz the power, 2016-07-22 03:24:05]

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

[dcheng, 2016-07-22 03:26:22]

On 2016/07/22 03:23:13, Marijn Kruisselbrink wrote:
>
https://codereview.chromium.org/2171843002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp
> (right):
> 
>
https://codereview.chromium.org/2171843002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp:69:
> mojoData.appendVector(data);
> On 2016/07/22 at 02:33:03, dcheng wrote:
> > It's sad that this results in copying the data now, especially since we're
> just going from char to uint8_t...
> 
> Yeah... but reinterpret_casting a Vector didn't seem like a great idea either.
I
> don't think this copy is really going to matter too much for the overall
> performance anyway.

Yeah, a reinterpret_cast would be highly dubious. I just think we should
probably be uniform about using uint8_t to mean a collection of bytes. It's the
inconsistency that gets us into trouble.

[commit-bot: I haz the power, 2016-07-22 03:52:37]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-07-22 03:54:48]

Description was changed from

==========
Use array<uint8> rather than string to pass BroadcastChannel messages.

Even though SerializedScriptValue normally serializes values to a WTF::String,
the string isn't actually a valid unicode string. Because of that it can't
be safely passed over mojo as a string (invalid strings get silently converted
to null). This changes the mojo interface to use array<uint8> instead, thereby
making sure arbitrary values get passed across correctly.

BUG=630203
==========

to

==========
Use array<uint8> rather than string to pass BroadcastChannel messages.

Even though SerializedScriptValue normally serializes values to a WTF::String,
the string isn't actually a valid unicode string. Because of that it can't
be safely passed over mojo as a string (invalid strings get silently converted
to null). This changes the mojo interface to use array<uint8> instead, thereby
making sure arbitrary values get passed across correctly.

BUG=630203

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

[commit-bot: I haz the power, 2016-07-22 03:54:50]

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

[esprehn, 2016-07-23 08:53:49]

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

[esprehn, 2016-07-23 08:53:50]

https://codereview.chromium.org/2171843002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp
(right):

https://codereview.chromium.org/2171843002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp:66:
Vector<char> data;
you want to call reserveInitialCapacity on both of these vectors to avoid
resizes and copies. :)

[Marijn Kruisselbrink, 2016-07-23 19:14:18]

https://codereview.chromium.org/2171843002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp
(right):

https://codereview.chromium.org/2171843002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp:66:
Vector<char> data;
On 2016/07/23 at 08:53:50, esprehn wrote:
> you want to call reserveInitialCapacity on both of these vectors to avoid
resizes and copies. :)

Not sure what resized and copies that would avoid?
SerializedScriptValue::toWireBytes resizes the output vector appropriately
before adding anything to it, and similarly Vector::appendVector resizes before
appending all the data. So both these vectors will only get resized once anyway.
Maybe it would make sense to add a
SerializedScriptValue::toWireBytes(Vector<uint8_t>&) overload to avoid having to
go through the extra Vector<char>, as that's a copy that really shouldn't be
necessary.