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

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
Committed: https://crrev.com/fb032b1e3fec6f34883a1f432ff38cebb6875045
Cr-Commit-Position: refs/heads/master@{#423487}

[Zhiqiang Zhang (Slow), 2016-09-29 16:39:46]

zqzhang@chromium.org changed reviewers:
+ mkwst@chromium.org, rockot@chromium.org

[Zhiqiang Zhang (Slow), 2016-09-29 16:39:47]

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.

[Zhiqiang Zhang (Slow), 2016-09-29 19:54:43]

zqzhang@chromium.org changed reviewers:
+ pdr@chromium.org, tsepez@chromium.org
- mkwst@chromium.org

[Zhiqiang Zhang (Slow), 2016-09-29 19:54:43]

-mkwst
+tsepez,pdr

[Tom Sepez, 2016-09-29 20:13:09]

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

https://codereview.chromium.org/2379993003/diff/1/mojo/common/common_custom_t...
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).

[Zhiqiang Zhang (Slow), 2016-09-29 20:39:31]

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

https://codereview.chromium.org/2379993003/diff/1/mojo/common/common_custom_t...
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.

[Ken Rockot(use gerrit already), 2016-09-29 22:00:48]

lgtm

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

https://codereview.chromium.org/2379993003/diff/1/mojo/common/common_custom_t...
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/p...
File
third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraitsTest.cpp
(right):

https://codereview.chromium.org/2379993003/diff/1/third_party/WebKit/Source/p...
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.

[Tom Sepez, 2016-09-30 16:34:38]

LGTM per Ken's explanation.

[Zhiqiang Zhang (Slow), 2016-09-30 17:55:00]

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

[commit-bot: I haz the power, 2016-09-30 17:55:44]

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

[Zhiqiang Zhang (Slow), 2016-09-30 17:55:53]

PTAL. Addressed rockot's comments

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

https://codereview.chromium.org/2379993003/diff/1/third_party/WebKit/Source/p...
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 :)

[Ken Rockot(use gerrit already), 2016-09-30 18:02:53]

lgtm

[commit-bot: I haz the power, 2016-09-30 18:24:13]

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

[commit-bot: I haz the power, 2016-09-30 18:24:14]

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_...)

[Zhiqiang Zhang (Slow), 2016-09-30 19:12:44]

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

[commit-bot: I haz the power, 2016-09-30 19:13:58]

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-09-30 19:53:49]

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

[commit-bot: I haz the power, 2016-09-30 19:53:50]

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...)

[Zhiqiang Zhang (Slow), 2016-09-30 20:57:28]

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

[commit-bot: I haz the power, 2016-09-30 20:57:59]

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-09-30 21:45:47]

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

[commit-bot: I haz the power, 2016-09-30 21:45:48]

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

[Zhiqiang Zhang (Slow), 2016-09-30 21:57:53]

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

[commit-bot: I haz the power, 2016-09-30 21:58:25]

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-09-30 23:50:18]

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

[commit-bot: I haz the power, 2016-09-30 23:50:19]

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

[Zhiqiang Zhang (Slow), 2016-10-01 12:19:38]

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

[commit-bot: I haz the power, 2016-10-01 12:19:44]

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-10-01 12:27:32]

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

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

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

[Zhiqiang Zhang (Slow), 2016-10-01 13:09:30]

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

[commit-bot: I haz the power, 2016-10-01 13:09:42]

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-10-01 14:19:54]

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

[commit-bot: I haz the power, 2016-10-01 14:19:55]

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_...)

[Zhiqiang Zhang (Slow), 2016-10-03 10:24:50]

zqzhang@chromium.org changed reviewers:
+ jochen@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-03 10:24:51]

+jochen for EMEA WEbkit/Source/platform review

[Zhiqiang Zhang (Slow), 2016-10-03 22:59:29]

zqzhang@chromium.org changed reviewers:
+ tkent@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-03 22:59:30]

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

[tkent, 2016-10-03 23:15:26]

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

[tkent, 2016-10-03 23:15:49]

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

[haraken, 2016-10-04 02:19:19]

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

[haraken, 2016-10-04 02:19:20]

LGTM on my side

+esprehn for the String conversion.

[esprehn, 2016-10-04 06:12:59]

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

[esprehn, 2016-10-04 06:13:00]

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.

[Ken Rockot(use gerrit already), 2016-10-04 06:28:40]

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

https://codereview.chromium.org/2379993003/diff/120001/mojo/common/common_cus...
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/Sou...
File third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp
(right):

https://codereview.chromium.org/2379993003/diff/120001/third_party/WebKit/Sou...
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.

[Zhiqiang Zhang (Slow), 2016-10-04 09:28:05]

Description was changed from

==========
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
==========

to

==========
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
==========

[Zhiqiang Zhang (Slow), 2016-10-04 09:28:05]

zqzhang@chromium.org changed reviewers:
- jochen@chromium.org, pdr@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-04 10:07:19]

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

https://codereview.chromium.org/2379993003/diff/120001/mojo/common/common_cus...
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.

[Zhiqiang Zhang (Slow), 2016-10-04 13:21:13]

Patchset #8 (id:140001) has been deleted

[Zhiqiang Zhang (Slow), 2016-10-04 13:21:59]

PTAL. Addressed comments from esprehn and rockot.

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

https://codereview.chromium.org/2379993003/diff/120001/mojo/common/common_cus...
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/inter...
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/Sou...
File third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp
(right):

https://codereview.chromium.org/2379993003/diff/120001/third_party/WebKit/Sou...
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.

[Zhiqiang Zhang (Slow), 2016-10-04 13:25:14]

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

[commit-bot: I haz the power, 2016-10-04 13:25:24]

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-10-04 16:38:21]

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

[commit-bot: I haz the power, 2016-10-04 16:38:22]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-10-04 18:20:29]

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

https://codereview.chromium.org/2379993003/diff/160001/mojo/common/common_cus...
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/Sou...
File third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp
(right):

https://codereview.chromium.org/2379993003/diff/160001/third_party/WebKit/Sou...
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/Sou...
File third_party/WebKit/Source/platform/mojo/WebGeometryStructTraits.h (right):

https://codereview.chromium.org/2379993003/diff/160001/third_party/WebKit/Sou...
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.

[Ken Rockot(use gerrit already), 2016-10-04 18:23:44]

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

[Zhiqiang Zhang (Slow), 2016-10-04 19:41:25]

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

[commit-bot: I haz the power, 2016-10-04 19:41:50]

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

[Zhiqiang Zhang (Slow), 2016-10-04 20:06:22]

PTAL.

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

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

https://codereview.chromium.org/2379993003/diff/160001/mojo/common/common_cus...
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/Sou...
File third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp
(right):

https://codereview.chromium.org/2379993003/diff/160001/third_party/WebKit/Sou...
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/Sou...
File third_party/WebKit/Source/platform/mojo/WebGeometryStructTraits.h (right):

https://codereview.chromium.org/2379993003/diff/160001/third_party/WebKit/Sou...
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/Sou...
File third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp
(right):

https://codereview.chromium.org/2379993003/diff/180001/third_party/WebKit/Sou...
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()

[Zhiqiang Zhang (Slow), 2016-10-04 20:09:41]

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.

[commit-bot: I haz the power, 2016-10-04 20:21:15]

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

[commit-bot: I haz the power, 2016-10-04 20:21:16]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Zhiqiang Zhang (Slow), 2016-10-04 21:01:29]

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

[commit-bot: I haz the power, 2016-10-04 21:02:33]

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-10-05 00:08:05]

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

[commit-bot: I haz the power, 2016-10-05 00:08:06]

Dry run: This issue passed the CQ dry run.

[Zhiqiang Zhang (Slow), 2016-10-05 16:26:20]

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

[Ken Rockot(use gerrit already), 2016-10-05 16:30:19]

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!

[Ken Rockot(use gerrit already), 2016-10-05 16:34:06]

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.

[esprehn, 2016-10-05 17:38:53]

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/Sou...
File third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp
(right):

https://codereview.chromium.org/2379993003/diff/200001/third_party/WebKit/Sou...
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

[Ken Rockot(use gerrit already), 2016-10-05 17:47:56]

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/bindin...
File mojo/public/cpp/bindings/array_traits_carray.h (right):

https://codereview.chromium.org/2379993003/diff/200001/mojo/public/cpp/bindin...
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/Sou...
File third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp
(right):

https://codereview.chromium.org/2379993003/diff/200001/third_party/WebKit/Sou...
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.

[Ken Rockot(use gerrit already), 2016-10-05 17:48:30]

On 2016/10/05 at 17:47:56, Ken Rockot wrote:
> 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/bindin...
> File mojo/public/cpp/bindings/array_traits_carray.h (right):
> 
>
https://codereview.chromium.org/2379993003/diff/200001/mojo/public/cpp/bindin...
> 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/Sou...
> File third_party/WebKit/Source/platform/mojo/CommonCustomTypesStructTraits.cpp
(right):
> 
>
https://codereview.chromium.org/2379993003/diff/200001/third_party/WebKit/Sou...
> 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.

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.

[esprehn, 2016-10-05 18:14:25]

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?

[Ken Rockot(use gerrit already), 2016-10-05 18:19:16]

On Wed, Oct 5, 2016 at 11:14 AM, <esprehn@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/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-10-05 18:19:17]

On Wed, Oct 5, 2016 at 11:14 AM, <esprehn@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/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[esprehn, 2016-10-05 18:30:11]

On 2016/10/05 at 18:19:17, rockot wrote:
> On Wed, Oct 5, 2016 at 11:14 AM, <esprehn@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.
> 

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...

[esprehn, 2016-10-05 18:32:09]

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

[Zhiqiang Zhang (Slow), 2016-10-05 18:49:09]

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

[commit-bot: I haz the power, 2016-10-05 18:50:01]

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

[Ken Rockot(use gerrit already), 2016-10-05 19:04:55]

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.



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

> 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/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-10-05 19:04:55]

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.



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

> 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/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[esprehn, 2016-10-05 19:45:12]

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.

[esprehn, 2016-10-05 19:45:22]

lgtm

[esprehn, 2016-10-05 19:46:56]

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

[Zhiqiang Zhang (Slow), 2016-10-05 20:46:04]

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.

[Zhiqiang Zhang (Slow), 2016-10-05 20:46:12]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-05 20:46:12]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
haraken@chromium.org, rockot@chromium.org, esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2379993003/#ps240001
(title: "map to blink::IntSize")

[commit-bot: I haz the power, 2016-10-05 20:47:11]

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-10-05 23:00:29]

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

[commit-bot: I haz the power, 2016-10-05 23:00:31]

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...)

[Zhiqiang Zhang (Slow), 2016-10-05 23:13:13]

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.

[Zhiqiang Zhang (Slow), 2016-10-05 23:32:49]

On 2016/10/05 at 23:13:13, Zhiqiang Zhang wrote:
> 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.

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.

[Zhiqiang Zhang (Slow), 2016-10-05 23:51:40]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-05 23:51:41]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
esprehn@chromium.org, tsepez@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2379993003/#ps260001
(title: "mapping back to WebSize :(")

[commit-bot: I haz the power, 2016-10-05 23:52:15]

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-10-06 02:33:17]

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

[commit-bot: I haz the power, 2016-10-06 02:33:19]

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_androi...)

[Zhiqiang Zhang (Slow), 2016-10-06 08:40:09]

The CQ bit was checked by zqzhang@chromium.org

[commit-bot: I haz the power, 2016-10-06 08:40:28]

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-10-06 09:23:38]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-10-06 09:23:40]

Committed patchset #13 (id:260001)

[commit-bot: I haz the power, 2016-10-06 09:25:03]

Description was changed from

==========
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
==========

to

==========
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

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

[commit-bot: I haz the power, 2016-10-06 09:25:05]

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