Mojo C++ bindings: make String16 and gfx::Size available in Blink (issue 2379993003 by zqzhang@chromium.org)

[zqz...@chromium.org]

Reviewers: Mike West, Ken Rockot
CL: https://codereview.chromium.org/2379993003/

Message:
mkwst@, rockot@, please review this patch.

I'm not very sure if I'm putting the files in the correct directory, please pay
attention when reviewing.

Description:
Mojo C++ bindings: make String16 and gfx::Size available in Blink

This CL adds C++ bindings for String16 and Size in Blink, which will
be used in a follow-up CL (https://codereview.chromium.org/2367393002/).

BUG=624136,649630

Affected files (+352, -7 lines):
M mojo/common/common_custom_types.mojom
M mojo/common/common_custom_types_struct_traits.h
M mojo/common/common_custom_types_struct_traits.cc
M mojo/common/common_custom_types_unittest.cc
M third_party/WebKit/Source/platform/BUILD.gn
A third_party/WebKit/Source/platform/mojo/CommonCustomTypes.typemap
A third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.h
A third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp
A third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraitsTest.cpp
M third_party/WebKit/Source/platform/mojo/DEPS
A + third_party/WebKit/Source/platform/mojo/WebGeometry.typemap
A third_party/WebKit/Source/platform/mojo/WebGeometryStructTraits.h
A third_party/WebKit/Source/platform/mojo/WebGeometryStructTraitsTest.cpp
M third_party/WebKit/Source/platform/mojo/blink_typemaps.gni

[zqz...@chromium.org]

-mkwst
+tsepez,pdr

https://codereview.chromium.org/2379993003/

[tse...@chromium.org]

https://codereview.chromium.org/2379993003/diff/1/mojo/common/common_custom_types.mojom
File mojo/common/common_custom_types.mojom (right):

https://codereview.chromium.org/2379993003/diff/1/mojo/common/common_custom_types.mojom#newcode27
mojo/common/common_custom_types.mojom:27: // Corresponds to
|blink::WebString| in
Wouldn't this violate DEPS, in some sense; I didn't think /mojo was
allowed to know about blink (base OK since mojom built on base).

https://codereview.chromium.org/2379993003/

[zqz...@chromium.org]

https://codereview.chromium.org/2379993003/diff/1/mojo/common/common_custom_types.mojom
File mojo/common/common_custom_types.mojom (right):

https://codereview.chromium.org/2379993003/diff/1/mojo/common/common_custom_types.mojom#newcode27
mojo/common/common_custom_types.mojom:27: // Corresponds to
|blink::WebString| in

On 2016/09/29 20:13:08, Tom Sepez wrote:
> Wouldn't this violate DEPS, in some sense; I didn't think /mojo was
allowed to
> know about blink (base OK since mojom built on base).

There some other types which has been mapped to blink structures, such
as:
String->WTF::String
Array->WTF::Vector

In general, my purpose is to send structures from WebKit/Source to
chromium directly, so we could avoid having a layer in content/renderer
to do the type conversion.

There's another example, which maps mojo Url to blink::KURL. See
"KURL.typemap"

I think the *.mojom-blink.* bindings are only used in Blink, which
serializes Blink structures for mojo communication, so it's probably OK?

Maybe rockot@ has better answer.

https://codereview.chromium.org/2379993003/

[roc...@chromium.org]

lgtm

https://codereview.chromium.org/2379993003/diff/1/mojo/common/common_custom_types.mojom
File mojo/common/common_custom_types.mojom (right):

https://codereview.chromium.org/2379993003/diff/1/mojo/common/common_custom_types.mojom#newcode27
mojo/common/common_custom_types.mojom:27: // Corresponds to
|blink::WebString| in

On 2016/09/29 at 20:39:31, Zhiqiang Zhang wrote:
> On 2016/09/29 20:13:08, Tom Sepez wrote:
> > Wouldn't this violate DEPS, in some sense; I didn't think /mojo was
allowed to
> > know about blink (base OK since mojom built on base).
>
> There some other types which has been mapped to blink structures, such
as:
> String->WTF::String
> Array->WTF::Vector
>
> In general, my purpose is to send structures from WebKit/Source to
chromium directly, so we could avoid having a layer in content/renderer
to do the type conversion.
>
> There's another example, which maps mojo Url to blink::KURL. See
"KURL.typemap"
>
> I think the *.mojom-blink.* bindings are only used in Blink, which
serializes Blink structures for mojo communication, so it's probably OK?
>
> Maybe rockot@ has better answer.

This is fine. The typemap which actually imposes DEPS is defined in
third_party/WebKit and only used by third_party/WebKit code.

It's a little weird to reference blink types even in comments, but I
think it's OK in this case, for clarity.

https://codereview.chromium.org/2379993003/diff/1/third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraitsTest.cpp
File
third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraitsTest.cpp
(right):

https://codereview.chromium.org/2379993003/diff/1/third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraitsTest.cpp#newcode70
third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraitsTest.cpp:70:
base::RunLoop runLoop;
nit: You could avoid needing a RunLoop by marking BounceString16 with
[Sync] in the mojom. Then you can just:

WebString response;
ptr->BounceString16(str, &response);
EXPECT_EQ(str, response);

This is OK to do since it's just a testing interface.

https://codereview.chromium.org/2379993003/

[tse...@chromium.org]

LGTM per Ken's explanation.

https://codereview.chromium.org/2379993003/

[zqz...@chromium.org]

PTAL. Addressed rockot's comments

https://codereview.chromium.org/2379993003/diff/1/third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraitsTest.cpp
File
third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraitsTest.cpp
(right):

https://codereview.chromium.org/2379993003/diff/1/third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraitsTest.cpp#newcode70
third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraitsTest.cpp:70:
base::RunLoop runLoop;

On 2016/09/29 22:00:48, Ken Rockot wrote:
> nit: You could avoid needing a RunLoop by marking BounceString16 with
[Sync] in
> the mojom. Then you can just:
>
> WebString response;
> ptr->BounceString16(str, &response);
> EXPECT_EQ(str, response);
>
> This is OK to do since it's just a testing interface.

You're right. Done :)

https://codereview.chromium.org/2379993003/

[roc...@chromium.org]

lgtm



https://codereview.chromium.org/2379993003/

[zqz...@chromium.org]

+jochen for EMEA WEbkit/Source/platform review

https://codereview.chromium.org/2379993003/

[zqz...@chromium.org]

+tkent for APAC Blink review. Hopefully we can get approval by tomorrow.

https://codereview.chromium.org/2379993003/

[tk...@chromium.org]

I'm not familiar with Mojo. haraken@, can you review this?


https://codereview.chromium.org/2379993003/

[har...@chromium.org]

LGTM on my side

+esprehn for the String conversion.


https://codereview.chromium.org/2379993003/

[esp...@chromium.org]

This patch doesn't look right it's round tripping all the strings through
Vectors, but the whole point of the wtf bindings is that we never need to do
that. We should be copying directly into the WTF::String which supports utf16
natively.

You should look at how yzshen@ did the String support. There should be no
Vectors involved at all.

https://codereview.chromium.org/2379993003/

[roc...@chromium.org]

https://codereview.chromium.org/2379993003/diff/120001/mojo/common/common_custom_types_struct_traits.cc
File mojo/common/common_custom_types_struct_traits.cc (right):

https://codereview.chromium.org/2379993003/diff/120001/mojo/common/common_custom_types_struct_traits.cc#newcode23
mojo/common/common_custom_types_struct_traits.cc:23: if
(!data.ReadData(&raw_data))
To esprehn@'s point, you can do:

mojo::ArrayDataView<int16_t> view;
data.GetDataDataView(&view);
out->assign(view.data(), view.size());

https://codereview.chromium.org/2379993003/diff/120001/third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp
File
third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp
(right):

https://codereview.chromium.org/2379993003/diff/120001/third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp#newcode21
third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp:21:
WTF::Vector<uint16_t> rawData(str16.length());
And similarly you can use ArrayDataView here as well to avoid another
copy.

https://codereview.chromium.org/2379993003/

[zqz...@chromium.org]

https://codereview.chromium.org/2379993003/diff/120001/mojo/common/common_custom_types_struct_traits.cc
File mojo/common/common_custom_types_struct_traits.cc (right):

https://codereview.chromium.org/2379993003/diff/120001/mojo/common/common_custom_types_struct_traits.cc#newcode23
mojo/common/common_custom_types_struct_traits.cc:23: if
(!data.ReadData(&raw_data))

On 2016/10/04 06:28:40, Ken Rockot wrote:
> To esprehn@'s point, you can do:
>
> mojo::ArrayDataView<int16_t> view;
> data.GetDataDataView(&view);
> out->assign(view.data(), view.size());
>

Hmm, seems like I can do something similar to
"mojo/public/cpp/bindings/string_traits_string16.h".
Maybe it's also better to put the struct traits file there?
Will send back for review after I finished.

https://codereview.chromium.org/2379993003/

[zqz...@chromium.org]

PTAL. Addressed comments from esprehn and rockot.

https://codereview.chromium.org/2379993003/diff/120001/mojo/common/common_custom_types_struct_traits.cc
File mojo/common/common_custom_types_struct_traits.cc (right):

https://codereview.chromium.org/2379993003/diff/120001/mojo/common/common_custom_types_struct_traits.cc#newcode23
mojo/common/common_custom_types_struct_traits.cc:23: if
(!data.ReadData(&raw_data))

On 2016/10/04 10:07:18, Zhiqiang Zhang wrote:
> On 2016/10/04 06:28:40, Ken Rockot wrote:
> > To esprehn@'s point, you can do:
> >
> > mojo::ArrayDataView<int16_t> view;
> > data.GetDataDataView(&view);
> > out->assign(view.data(), view.size());
> >
>
> Hmm, seems like I can do something similar to
> "mojo/public/cpp/bindings/string_traits_string16.h".
> Maybe it's also better to put the struct traits file there?
> Will send back for review after I finished.

OK, I decided not to do that.
I found that StringTraits are used for converting custom strings to/from
UTF8, which will bring extra overhead for base::string16.

Now I'm just avoiding making redundant copies as you suggested.

BTW, just FYI to rockot@. I guess string_traits_string16 should be
removed now? However I cannot remove it at the moment since I see a lot
of uses of StringTraits<base::string16> for implicitly mapping
base::string16 into mojo string (8-bit), which I'm not sure if it's
abusing or not. For example
https://cs.chromium.org/chromium/src/components/autofill/content/public/interfaces/autofill_types.mojom?rcl=0&l=95
autofill.mojom.FormData::name is mojo string but it's base::string16 in
autofill::FormData.

https://codereview.chromium.org/2379993003/diff/120001/third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp
File
third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp
(right):

https://codereview.chromium.org/2379993003/diff/120001/third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp#newcode21
third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp:21:
WTF::Vector<uint16_t> rawData(str16.length());

On 2016/10/04 06:28:40, Ken Rockot wrote:

> And similarly you can use ArrayDataView here as well to avoid another
copy.

Done.

https://codereview.chromium.org/2379993003/

[roc...@chromium.org]

https://codereview.chromium.org/2379993003/diff/160001/mojo/common/common_custom_types_struct_traits.cc
File mojo/common/common_custom_types_struct_traits.cc (right):

https://codereview.chromium.org/2379993003/diff/160001/mojo/common/common_custom_types_struct_traits.cc#newcode15
mojo/common/common_custom_types_struct_traits.cc:15: return
std::vector<uint16_t>(str.data(), str.data() + str.size());
Another reason to add mojo::ConstCArray (see other inline comment).

Static accessors in traits should always do as little work as possible.
You can use something like ConstCArray to provide a view into the
existing string.

https://codereview.chromium.org/2379993003/diff/160001/third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp
File
third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp
(right):

https://codereview.chromium.org/2379993003/diff/160001/third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp#newcode16
third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp:16:
StructTraits<mojo::common::mojom::String16DataView,
::blink::WebString>::data(
This is called twice during serialization. You should use SetUpContext
and TearDownContext if you need to do non-trivial setup to get data in a
useful form.

In this case though, I'm not sure it's needed. I don't think we should
allow for accidental use of an 8-bit WebString for a 16-bit string
field, so you should DCHECK(!strView.is8Bit). We should really avoid
ever accidentally doing transcoding for callers.

In the valid 16-bit case, I think we could add a mojo::ConstCArray<T>
type (see src/mojo/public/cpp/bindings/array_traits_carray.h) with the
necessary subset of ArrayTraits defined. Then you can simply return
something like:

return mojo::ConstCArray<uint16_t>(
reinterpret_cast<const uint16_t*>(strView.characters16()),
strView.length());

Then there's no need for extra copying or setup.

https://codereview.chromium.org/2379993003/diff/160001/third_party/WebKit/Source/platform/mojo/WebGeometryStructTraits.h
File third_party/WebKit/Source/platform/mojo/WebGeometryStructTraits.h
(right):

https://codereview.chromium.org/2379993003/diff/160001/third_party/WebKit/Source/platform/mojo/WebGeometryStructTraits.h#newcode17
third_party/WebKit/Source/platform/mojo/WebGeometryStructTraits.h:17:
static bool Read(gfx::mojom::blink::Size::DataView data,
nit: Probably should move this out-of-line.

https://codereview.chromium.org/2379993003/

[roc...@chromium.org]

Actually mojo::CArray should work as-is, since there's no reason T can't be a
const type.

https://codereview.chromium.org/2379993003/

[zqz...@chromium.org]

PTAL.

Addressed more comments from rockot@.
Left a comment for esprehn@

https://codereview.chromium.org/2379993003/diff/160001/mojo/common/common_custom_types_struct_traits.cc
File mojo/common/common_custom_types_struct_traits.cc (right):

https://codereview.chromium.org/2379993003/diff/160001/mojo/common/common_custom_types_struct_traits.cc#newcode15
mojo/common/common_custom_types_struct_traits.cc:15: return
std::vector<uint16_t>(str.data(), str.data() + str.size());

On 2016/10/04 at 18:20:28, Ken Rockot wrote:
> Another reason to add mojo::ConstCArray (see other inline comment).
>
> Static accessors in traits should always do as little work as
possible. You can use something like ConstCArray to provide a view into
the existing string.

Done.

https://codereview.chromium.org/2379993003/diff/160001/third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp
File
third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp
(right):

https://codereview.chromium.org/2379993003/diff/160001/third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp#newcode16
third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp:16:
StructTraits<mojo::common::mojom::String16DataView,
::blink::WebString>::data(

On 2016/10/04 at 18:20:29, Ken Rockot wrote:
> This is called twice during serialization. You should use SetUpContext
and TearDownContext if you need to do non-trivial setup to get data in a
useful form.
>
> In this case though, I'm not sure it's needed. I don't think we should
allow for accidental use of an 8-bit WebString for a 16-bit string
field, so you should DCHECK(!strView.is8Bit). We should really avoid
ever accidentally doing transcoding for callers.
>
> In the valid 16-bit case, I think we could add a mojo::ConstCArray<T>
type (see src/mojo/public/cpp/bindings/array_traits_carray.h) with the
necessary subset of ArrayTraits defined. Then you can simply return
something like:
>
> return mojo::ConstCArray<uint16_t>(
> reinterpret_cast<const uint16_t*>(strView.characters16()),
> strView.length());
>
> Then there's no need for extra copying or setup.

Thanks! Done.
BTW, I saw your other comment, and tried using mojo::CArray<const
uint16_t> but there's a static assert, so I'm adding mojo::ConstCArray.

The static assert is:
/array_serialization.h:119:3: error: static_assert failed "Incorrect
array serializer"

https://codereview.chromium.org/2379993003/diff/160001/third_party/WebKit/Source/platform/mojo/WebGeometryStructTraits.h
File third_party/WebKit/Source/platform/mojo/WebGeometryStructTraits.h
(right):

https://codereview.chromium.org/2379993003/diff/160001/third_party/WebKit/Source/platform/mojo/WebGeometryStructTraits.h#newcode17
third_party/WebKit/Source/platform/mojo/WebGeometryStructTraits.h:17:
static bool Read(gfx::mojom::blink::Size::DataView data,

On 2016/10/04 at 18:20:29, Ken Rockot wrote:
> nit: Probably should move this out-of-line.

Done.

https://codereview.chromium.org/2379993003/diff/180001/third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp
File
third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp
(right):

https://codereview.chromium.org/2379993003/diff/180001/third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp#newcode34
third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp:34:
out->ensure16Bit();
esprehn@, should this be fixed in WTF::String?

I found WTF::String is 8-bit when constructed using WTF::String(UChar*,
length) with length=0, because it calls StringImpl::empty() instead
StringImpl::empty16Bit()

https://codereview.chromium.org/2379993003/

[zqz...@chromium.org]

On 2016/10/04 at 20:06:22, Zhiqiang Zhang wrote:
> PTAL.
>
> Addressed more comments from rockot@.
> Left a comment for esprehn@

Also, I'm now mapping String16 to WTF::String instead of blink::WebString in
Blink. The reason is that WTF::String is more handy to do string operations.

https://codereview.chromium.org/2379993003/

[zqz...@chromium.org]

Ping!
esprehn@ & rockot@, are you OK with the patch now?

https://codereview.chromium.org/2379993003/

[roc...@chromium.org]

On 2016/10/05 at 16:26:20, zqzhang wrote:
> Ping!
> esprehn@ & rockot@, are you OK with the patch now?

Oops, sorry - yes, this indeed LGTM. Thanks!

https://codereview.chromium.org/2379993003/

[roc...@chromium.org]

On 2016/10/05 at 16:30:19, Ken Rockot wrote:
> On 2016/10/05 at 16:26:20, zqzhang wrote:
> > Ping!
> > esprehn@ & rockot@, are you OK with the patch now?
>
> Oops, sorry - yes, this indeed LGTM. Thanks!

And to the question you asked earlier which I forgot about... Yes I think we
should remove StringTraits<base::string16>. I think if a developer really wants
to use string16 somewhere, they should either explicitly convert to UTF8 before
sending over a message, or use mojo.common.mojom.String16 in their message
format.

https://codereview.chromium.org/2379993003/

[esp...@chromium.org]

That DCHECK is8Bit is scary, isn't that when we're sending a string through
mojo? That string could be 8 or 16 bit unless I'm not understanding the code.


https://codereview.chromium.org/2379993003/diff/200001/third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp
File
third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp
(right):

https://codereview.chromium.org/2379993003/diff/200001/third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp#newcode18
third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp:18:
DCHECK(!str.is8Bit());
What makes sure the string is 16bit here? I think you need to convert
here, ideally mojo would support writing directly into the output buffer
without the extra malloc and upconvert, but hopefully this is rare

https://codereview.chromium.org/2379993003/

[roc...@chromium.org]

Could be a CHECK instead if that would be less scary. I don't see a problem with
asserting on send, we just don't want to assert on Read.



https://codereview.chromium.org/2379993003/diff/200001/mojo/public/cpp/bindings/array_traits_carray.h
File mojo/public/cpp/bindings/array_traits_carray.h (right):

https://codereview.chromium.org/2379993003/diff/200001/mojo/public/cpp/bindings/array_traits_carray.h#newcode26
mojo/public/cpp/bindings/array_traits_carray.h:26: size_t size;
nit: These fields could be const size_t and const T* const

https://codereview.chromium.org/2379993003/diff/200001/third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp
File
third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp
(right):

https://codereview.chromium.org/2379993003/diff/200001/third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp#newcode18
third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp:18:
DCHECK(!str.is8Bit());

On 2016/10/05 at 17:38:52, esprehn wrote:
> What makes sure the string is 16bit here? I think you need to convert
here, ideally mojo would support writing directly into the output buffer
without the extra malloc and upconvert, but hopefully this is rare

What do you mean by writing directly into the output buffer? The wire
format of this type is fixed. It's a 16-bit string on the wire, always.

https://codereview.chromium.org/2379993003/

[roc...@chromium.org]

We could support what I think you're asking for by making a union-ified string
type that's either an 8-bit or a 16-bit string. I don't think we want that.

https://codereview.chromium.org/2379993003/

[esp...@chromium.org]

Actually can we step back here, why do we need a String16 type in the mojo IPC?
DOMString is WTF::String, and nearly every other IPC is sending utf8 as plain
mojo::String which is highly optimized to avoid copies in the common cases. Can
we just use std::string here directly and avoid adding this type?

https://codereview.chromium.org/2379993003/

[Ken Rockot]

On Wed, Oct 5, 2016 at 11:14 AM, <esp...@chromium.org> wrote:

Actually can we step back here, why do we need a String16 type in the mojo IPC?
DOMString is WTF::String, and nearly every other IPC is sending utf8 as plain
mojo::String which is highly optimized to avoid copies in the common cases. Can
we just use std::string here directly and avoid adding this type?

We do want people to prefer to use the standard mojom string type for their messages, but in some cases there are IPCs which currently use a 16-bit string on both ends. Maybe they shouldn't, but they do, and getting them to change seems like a separate issue. Using UTF8 over the wire in those cases would be a waste.

 

https://codereview.chromium.org/2379993003/

[esp...@chromium.org]

On 2016/10/05 at 18:19:17, rockot wrote:

> On Wed, Oct 5, 2016 at 11:14 AM, <esp...@chromium.org&gt; wrote:
>
> > Actually can we step back here, why do we need a String16 type in the mojo
> > IPC?
> > DOMString is WTF::String, and nearly every other IPC is sending utf8 as
> > plain
> > mojo::String which is highly optimized to avoid copies in the common
> > cases. Can
> > we just use std::string here directly and avoid adding this type?
> >
>
> We do want people to prefer to use the standard mojom string type for their
> messages, but in some cases there are IPCs which currently use a 16-bit
> string on both ends. Maybe they shouldn't, but they do, and getting them to
> change seems like a separate issue. Using UTF8 over the wire in those cases
> would be a waste.
>

In this case there's not utf16 in blink though, so the waste is the string
inflation before sending the IPC, having to convert from utf8 to string16 in the
browser process is not less wasteful than what's going on here inside blink.
That said I think this IPC is only from the browser -> renderer with the
metadata, so this code here that does WTF::String -> mojo's String16 is probably
unreachable in the blocked CL. I don't think any new IPCs should be using this
though, and that MediaMetadata API has 2016 copyrights all over it...

https://codereview.chromium.org/2379993003/

[esp...@chromium.org]

On 2016/10/05 at 18:30:11, esprehn wrote:
> ...

> >
>
> In this case there's not utf16 in blink though, so the waste is the string
inflation before sending the IPC, having to convert from utf8 to string16 in the
browser process is not less wasteful than what's going on here inside blink.
That said I think this IPC is only from the browser -> renderer with the
metadata, so this code here that does WTF::String -> mojo's String16 is probably
unreachable in the blocked CL. I don't think any new IPCs should be using this
though, and that MediaMetadata API has 2016 copyrights all over it...

Err *more wasteful*. :P

https://codereview.chromium.org/2379993003/

[Ken Rockot]

Ah, so 16-bit WebString is UCS-2 and base::string16 is UTF-16, got it. I think it makes sense for the wire format representation of mojo.common.mojom.String16 to be UTF-16 since that is strictly a more useful encoding.

If you want to allow use of this mojom type from Blink code, this CL seems like the correct way to typemap it in my opinion (though I suppose Blink the traits also need to do a UCS-2 to UTF-16 conversion now.)

If you don't want to allow this mojom type to be used by any Blink code, and it sounds like you're saying you don't, then adding a Blink typemap is a bad idea. I'm not fundamentally opposed to this policy, as I am a strong advocate of using UTF-8 wherever possible.

In any case I'd still like zqzhang@ to at least land the chromium typemap side of this, because the work is already done, and it leaves things strictly better than they are now, where we rely on legacy IPC serialization of base::string16.

[esp...@chromium.org]

lgtm



https://codereview.chromium.org/2379993003/

[esp...@chromium.org]

btw I think you probably want to map to IntSize, not WebSize for blink. WebSize
is for plumbing stuff out of blink.

https://codereview.chromium.org/2379993003/

[esp...@chromium.org]

I think this is fine to land, we should just avoid adding any new IPCs (that
aren't converting existing ones to mojo) that use String16.

https://codereview.chromium.org/2379993003/

[zqz...@chromium.org]

On 2016/10/05 at 19:46:56, esprehn wrote:
> btw I think you probably want to map to IntSize, not WebSize for blink.
WebSize is for plumbing stuff out of blink.

Done.

https://codereview.chromium.org/2379993003/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2379993003/240001


https://codereview.chromium.org/2379993003/

[commit-bot@chromium.org via codereview.chromium.org]

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_compile_dbg_ng/builds/270679)

https://codereview.chromium.org/2379993003/

[zqz...@chromium.org]

Hmm, interesting...
When mapping gfx::Size to blink::IntSize, it got link error on Windows.

I think it's because there are some mojo interfaces in Source/platform, and the
public_header in Geometry.typemap requires all generated mojom-blink.h/cc files
to include "IntSize.h", while these mojom-blink.o object files somehow contains
the definition of IntSize:: methods.

I will try remove the public_header = [ ".../IntSize.h" ] from Geometry.typemap
for now, and include the header in all required files. This could probably fix
the issue temporarily.

https://codereview.chromium.org/2379993003/

[zqz...@chromium.org]

Ahhh, it doesn't work... I'll have to map to WebSize for now.

One addition to the issue, it seems like "//cc/ipc/quads.mojom",
quads.mojom-blink.cc will include IntSize.h, and maybe it somehow has
BLINK_PLATFORM_IMPLEMENTATION on thus generating definitions in
quads.mojom-blink.o.

I'll file a bug for it.

https://codereview.chromium.org/2379993003/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2379993003/260001


https://codereview.chromium.org/2379993003/

[commit-bot@chromium.org via codereview.chromium.org]

Try jobs failed on following builders:

linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/155051)

https://codereview.chromium.org/2379993003/

[commit-bot@chromium.org via codereview.chromium.org]

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #13 (id:260001)

https://codereview.chromium.org/2379993003/

[commit-bot@chromium.org via codereview.chromium.org]

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

https://codereview.chromium.org/2379993003/