Mojo C++ bindings: support mapping mojo string to WTF::String.

Mojo C++ bindings: support mapping mojo string to WTF::String.

The generator now supports a "--for_blink" flag. When it is specified, the generator will map mojo string to WTF::String.

BUG=583738
Committed: https://crrev.com/791f9b24ca1758ced39235bda1a4e0411801309c
Cr-Commit-Position: refs/heads/master@{#381091}

[yzshen1, 2016-02-29 21:16:30]

yzshen@chromium.org changed reviewers:
+ esprehn@chromium.org, haraken@chromium.org, rockot@chromium.org,
yukishiino@chromium.org, yutak@chromium.org

[yzshen1, 2016-02-29 21:16:31]

Hi,

Would you please take a look? Thanks!

Ken: everything
Kentaro: 	
  - mojo/public/cpp/bindings/DEPS: OWNER review for adding deps.
  - mojo/public/cpp/bindings/lib/wtf_string_serialization.{h,cc}
  - mojo/public/cpp/bindings/tests/wtf_types_unittest.cc
Others: FYI and welcome to comment if you are interested, of course.

[haraken, 2016-03-01 00:40:54]

https://codereview.chromium.org/1751563002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/wtf_string_serialization.cc (right):

https://codereview.chromium.org/1751563002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/wtf_string_serialization.cc:18:
DCHECK(input.is8Bit());

From the perspective of Blink, in common cases we don't know if a given string
is a 8-bit string or a 16-bit string. In that sense, this restriction is not
really embedder-friendly :)

Would it make sense to add conversion code from a 16-bit string to a 8-bit
string here?

[yzshen1, 2016-03-01 00:51:53]

https://codereview.chromium.org/1751563002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/wtf_string_serialization.cc (right):

https://codereview.chromium.org/1751563002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/wtf_string_serialization.cc:18:
DCHECK(input.is8Bit());
On 2016/03/01 00:40:54, haraken wrote:
> 
> From the perspective of Blink, in common cases we don't know if a given string
> is a 8-bit string or a 16-bit string. In that sense, this restriction is not
> really embedder-friendly :)
> 
> Would it make sense to add conversion code from a 16-bit string to a 8-bit
> string here?

If the receiver doesn't care whether the string was originally 16 bit or not,
then it should be doable -- the receiver always gets a 8-bit string, even if the
data is deserialized as a WTF::String.

Another choice is to require the caller of mojo interface to do the
check/conversion explicitly before passing WTF::String to mojo interfaces (which
is what I had in mind). Because according to mojom spec, mojo strings are
8-bits. The caller is responsible to use valid arguments to make calls.

WDYT? Thanks!

[haraken, 2016-03-01 01:56:46]

https://codereview.chromium.org/1751563002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/wtf_string_serialization.cc (right):

https://codereview.chromium.org/1751563002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/wtf_string_serialization.cc:18:
DCHECK(input.is8Bit());
On 2016/03/01 00:51:53, yzshen1 wrote:
> On 2016/03/01 00:40:54, haraken wrote:
> > 
> > From the perspective of Blink, in common cases we don't know if a given
string
> > is a 8-bit string or a 16-bit string. In that sense, this restriction is not
> > really embedder-friendly :)
> > 
> > Would it make sense to add conversion code from a 16-bit string to a 8-bit
> > string here?
> 
> If the receiver doesn't care whether the string was originally 16 bit or not,
> then it should be doable -- the receiver always gets a 8-bit string, even if
the
> data is deserialized as a WTF::String.
> 
> Another choice is to require the caller of mojo interface to do the
> check/conversion explicitly before passing WTF::String to mojo interfaces
(which
> is what I had in mind). Because according to mojom spec, mojo strings are
> 8-bits. The caller is responsible to use valid arguments to make calls.
> 
> WDYT? Thanks!
> 

Elliott: Do you have any preference on this?

[esprehn, 2016-03-01 02:03:32]

https://codereview.chromium.org/1751563002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/wtf_string_serialization.cc (right):

https://codereview.chromium.org/1751563002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/wtf_string_serialization.cc:18:
DCHECK(input.is8Bit());
wtf::String is 16 bit for many cases, the 8 bit strings are an optimization for
latin1 and ascii. The mojo 8bit strings mean something else entirely, they're
utf8, which isn't the same.

I don't think this code makes sense, you're asserting that the string is all
latin1?

[Ken Rockot(use gerrit already), 2016-03-01 02:06:37]

change in general LG to me but I'll hold off stamping until the blink string
questions are resolved

https://codereview.chromium.org/1751563002/diff/20001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/mojom.gni (right):

https://codereview.chromium.org/1751563002/diff/20001/mojo/public/tools/bindi...
mojo/public/tools/bindings/mojom.gni:68: #   use_wtf_types (optional, C++ only)
nit: How about more generically something like for_blink? I would like to use
the same flag to generate blink JS bindings eventually

[yzshen1, 2016-03-01 17:32:02]

https://codereview.chromium.org/1751563002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/wtf_string_serialization.cc (right):

https://codereview.chromium.org/1751563002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/wtf_string_serialization.cc:18:
DCHECK(input.is8Bit());
On 2016/03/01 02:03:32, esprehn wrote:
> wtf::String is 16 bit for many cases, the 8 bit strings are an optimization
for
> latin1 and ascii. The mojo 8bit strings mean something else entirely, they're
> utf8, which isn't the same.
> 
> I don't think this code makes sense, you're asserting that the string is all
> latin1?

Mojo string is 8 bit but not enforced to be utf8. (We talked about enforcing it
before but didn't do it as of now)

IMO, this CL tries to solve the problem of using mojo interfaces in blink, not
trying to change mojo spec to fit blink. (Not saying that shouldn't be
considered, but seems beyond the scope of the CL.)

So if we have a mojo interface:
interface Foo {
  SendString(string bar);
};

|bar| here is 8-bit according to mojo spec. We map |bar| to WTF::String just in
blink; the other side is free to map it to a different type, such as
mojo::String.

So when a user pass a WTF::String to a mojo interface, this string should
conform to mojo's requirement. As we mentioned above, two choices: (1) convert
it to 8-bit if it is not (question: what conversion function should I use if we
take this approach?); (2) assume it is the user's responsibility to do the
necessary conversion before making the call and [D]CHECK.

WDYT?

[yzshen1, 2016-03-01 18:42:46]

https://codereview.chromium.org/1751563002/diff/20001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/mojom.gni (right):

https://codereview.chromium.org/1751563002/diff/20001/mojo/public/tools/bindi...
mojo/public/tools/bindings/mojom.gni:68: #   use_wtf_types (optional, C++ only)
On 2016/03/01 02:06:37, Ken Rockot wrote:
> nit: How about more generically something like for_blink? I would like to use
> the same flag to generate blink JS bindings eventually

Sounds good. Done.

[darin (slow to review), 2016-03-01 19:13:29]

It makes me nervous to not ensure that string in mojom is always UTF-8. We
may end up with subtle encoding bugs otherwise. Maybe we should validate
strings in debug builds?

-Darin
On Mar 1, 2016 10:42 AM, <yzshen@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1751563002/diff/20001/mojo/public/tools/bindi...
> File mojo/public/tools/bindings/mojom.gni (right):
>
>
>
https://codereview.chromium.org/1751563002/diff/20001/mojo/public/tools/bindi...
> mojo/public/tools/bindings/mojom.gni:68: # use_wtf_types (optional,
> C++ only)
> On 2016/03/01 02:06:37, Ken Rockot wrote:
> > nit: How about more generically something like for_blink? I would like
> to use
> > the same flag to generate blink JS bindings eventually
>
> Sounds good. Done.
>
> https://codereview.chromium.org/1751563002/
>

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

[yzshen1, 2016-03-01 19:23:09]

On 2016/03/01 19:13:29, darin (slow to review) wrote:
> It makes me nervous to not ensure that string in mojom is always UTF-8. We
> may end up with subtle encoding bugs otherwise. Maybe we should validate
> strings in debug builds?

Yeah. Originally the mojom spec said that strings should be UTF-8, but we never
enforced that.
I filed a bug and assigned to myself:
https://bugs.chromium.org/p/chromium/issues/detail?id=591125

Thanks!

> 
> -Darin
> On Mar 1, 2016 10:42 AM, <mailto:yzshen@chromium.org> wrote:
> 
> >
> >
> >
>
https://codereview.chromium.org/1751563002/diff/20001/mojo/public/tools/bindi...
> > File mojo/public/tools/bindings/mojom.gni (right):
> >
> >
> >
>
https://codereview.chromium.org/1751563002/diff/20001/mojo/public/tools/bindi...
> > mojo/public/tools/bindings/mojom.gni:68: # use_wtf_types (optional,
> > C++ only)
> > On 2016/03/01 02:06:37, Ken Rockot wrote:
> > > nit: How about more generically something like for_blink? I would like
> > to use
> > > the same flag to generate blink JS bindings eventually
> >
> > Sounds good. Done.
> >
> > https://codereview.chromium.org/1751563002/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

[esprehn, 2016-03-01 19:51:14]

I don't want mojo::String conversion costs except at the IPC boundary (when you
actually call Send()), it leaks string conversions down into blink which we've
mostly be immune to today. I was hoping mojo::String would just be a wrapper
around wtf::String, which is either 16bit unicode or 8bit latin1. Down inside
the Send() call as it was copied into the channel it would use the
wtf::StringUTF8Adaptor to convert to utf8 for the wire format.

[Ken Rockot(use gerrit already), 2016-03-01 20:10:43]

I think doing the conversion to/from UTF8 at the serialization boundary
makes sense, as it's what all Blink consumers will want and expect.

Doing this is not entirely trivial though... because we completely
preallocate message buffers before serializing, we need to compute the size
of the message ahead of time. Are there any existing utilities for
computing the hypothetical UTF8 size of a WTF::String without doing any
alloc or copy?

On Tue, Mar 1, 2016 at 11:51 AM, <esprehn@chromium.org> wrote:

> I don't want mojo::String conversion costs except at the IPC boundary
> (when you
> actually call Send()), it leaks string conversions down into blink which
> we've
> mostly be immune to today. I was hoping mojo::String would just be a
> wrapper
> around wtf::String, which is either 16bit unicode or 8bit latin1. Down
> inside
> the Send() call as it was copied into the channel it would use the
> wtf::StringUTF8Adaptor to convert to utf8 for the wire format.
>
> https://codereview.chromium.org/1751563002/
>

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

[yzshen1, 2016-03-01 20:21:06]

On 2016/03/01 19:51:14, esprehn wrote:
> I don't want mojo::String conversion costs except at the IPC boundary (when
you
> actually call Send()), it leaks string conversions down into blink which we've
> mostly be immune to today. I was hoping mojo::String would just be a wrapper
> around wtf::String, which is either 16bit unicode or 8bit latin1. Down inside
> the Send() call as it was copied into the channel it would use the
> wtf::StringUTF8Adaptor to convert to utf8 for the wire format.

Please note that we are not planning to use mojo::String at all if we generate
bindings for blink.
So there is no conversion between mojo::String and WTF::String. We directly
convert between WTF::String and the "wire format" (payload bytes of mojo
messages).

I am fine with doing conversion to utf8 before sending (which is the
serialization code in wtf_string_serialization). But we need to get a upper
bound of how much space it needs before actually doing the conversion so that we
can compute the overall message size (as Ken pointed out).

[esprehn, 2016-03-01 20:51:10]

On 2016/03/01 at 20:21:06, yzshen wrote:
> On 2016/03/01 19:51:14, esprehn wrote:
> > I don't want mojo::String conversion costs except at the IPC boundary (when
you
> > actually call Send()), it leaks string conversions down into blink which
we've
> > mostly be immune to today. I was hoping mojo::String would just be a wrapper
> > around wtf::String, which is either 16bit unicode or 8bit latin1. Down
inside
> > the Send() call as it was copied into the channel it would use the
> > wtf::StringUTF8Adaptor to convert to utf8 for the wire format.
> 
> Please note that we are not planning to use mojo::String at all if we generate
bindings for blink.
> So there is no conversion between mojo::String and WTF::String. We directly
convert between WTF::String and the "wire format" (payload bytes of mojo
messages).
> 
> I am fine with doing conversion to utf8 before sending (which is the
serialization code in wtf_string_serialization). But we need to get a upper
bound of how much space it needs before actually doing the conversion so that we
can compute the overall message size (as Ken pointed out).

There's no way to do that without actually doing the conversion. How will you
handle sending string16 in the browser process?

I think you need your serializer to be two pass, walk the data structure once
creating the StringUTF8Adapter objects, and figure out the size, then walk it
again and actually serialize.

[esprehn, 2016-03-01 20:53:29]

On 2016/03/01 at 20:51:10, esprehn wrote:
> On 2016/03/01 at 20:21:06, yzshen wrote:
> > On 2016/03/01 19:51:14, esprehn wrote:
> > > I don't want mojo::String conversion costs except at the IPC boundary
(when you
> > > actually call Send()), it leaks string conversions down into blink which
we've
> > > mostly be immune to today. I was hoping mojo::String would just be a
wrapper
> > > around wtf::String, which is either 16bit unicode or 8bit latin1. Down
inside
> > > the Send() call as it was copied into the channel it would use the
> > > wtf::StringUTF8Adaptor to convert to utf8 for the wire format.
> > 
> > Please note that we are not planning to use mojo::String at all if we
generate bindings for blink.
> > So there is no conversion between mojo::String and WTF::String. We directly
convert between WTF::String and the "wire format" (payload bytes of mojo
messages).
> > 
> > I am fine with doing conversion to utf8 before sending (which is the
serialization code in wtf_string_serialization). But we need to get a upper
bound of how much space it needs before actually doing the conversion so that we
can compute the overall message size (as Ken pointed out).
> 
> There's no way to do that without actually doing the conversion. How will you
handle sending string16 in the browser process?
> 
> I think you need your serializer to be two pass, walk the data structure once
creating the StringUTF8Adapter objects, and figure out the size, then walk it
again and actually serialize.

And reusing the adapters inside the serializer, perhaps referencing them by
position in the serialization process.

[yzshen1, 2016-03-01 21:42:32]

I had a chat with Elliot. It turned out that WTF::String doesn't support utf8
representation, when it is 8-bit, it means latin1.
So it is not feasible to ask the user to do the conversion before making the
mojo calls. (We chatted about using WTF::CString as the mapping type of mojo
strings. And it seems clear that it is not a good idea.)

So I am thinking to have a context object which is passed to
GetSerializedSize_() and collect StringUTF8Adapter objects which will be used
later in Serialize_().

I will update the CL. Thanks!

[yzshen1, 2016-03-03 20:18:48]

Hi, reviewers.

This CL now can deal with 16-bit WTF::Strings. PTAL. Thanks!

[Ken Rockot(use gerrit already), 2016-03-04 00:26:05]

LGTM!

[haraken, 2016-03-04 04:04:50]

https://codereview.chromium.org/1751563002/diff/100001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/wtf_string_serialization.cc (right):

https://codereview.chromium.org/1751563002/diff/100001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_string_serialization.cc:37:
std::queue<UTF8AdaptorWithPointer> utf8_adaptors_;

Just help me understand: Why do we need to queue the UTF8AdaptorWithPointers?

[esprehn, 2016-03-04 04:14:48]

Hmmm, this means allowing usage of WTF out in mojo which is very strange, I
guess we can do it though.

https://codereview.chromium.org/1751563002/diff/100001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/wtf_string_serialization.cc (right):

https://codereview.chromium.org/1751563002/diff/100001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_string_serialization.cc:24:
WTF::StringUTF8Adaptor utf8_adaptor;
I'd rather you just stored the StringUTF8Adaptor. Note that this isn't generally
considered to be public API from webkit either. Are we sure we want WTF code in
mojo/ instead of adding a mojo directory to third_party/WebKit and keeping our
stuff there?

https://codereview.chromium.org/1751563002/diff/100001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_string_serialization.cc:37:
std::queue<UTF8AdaptorWithPointer> utf8_adaptors_;
Can we use a std::vector instead? std::deque is not especially efficient, but
maybe it's okay here.

https://codereview.chromium.org/1751563002/diff/100001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_string_serialization.cc:76:
CHECK_EQ(utf8_adaptors.front().original_input, &input);
Is this really worth the overhead?

https://codereview.chromium.org/1751563002/diff/100001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/tests/wtf_types_unittest.cc (right):

https://codereview.chromium.org/1751563002/diff/100001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/wtf_types_unittest.cc:15: #include
"third_party/WebKit/Source/wtf/Partitions.h"
this really isn't public API...

[esprehn, 2016-03-04 04:16:09]

fwiw instead of checking that the original_string is the same, I'd check that
the value of the string converted and value in the adaptor list are the same.
The probability that you have the same bytes, but the wrong string is super low.

[yzshen1, 2016-03-04 17:28:49]

> fwiw instead of checking that the original_string is the same, I'd check that
> the value of the string converted and value in the adaptor list are the same.
> The probability that you have the same bytes, but the wrong string is super
low.

Please see the updated CL and see whether that makes sense.

https://codereview.chromium.org/1751563002/diff/100001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/wtf_string_serialization.cc (right):

https://codereview.chromium.org/1751563002/diff/100001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_string_serialization.cc:24:
WTF::StringUTF8Adaptor utf8_adaptor;
On 2016/03/04 04:14:48, esprehn wrote:
> I'd rather you just stored the StringUTF8Adaptor. Note that this isn't
generally
> considered to be public API from webkit either. Are we sure we want WTF code
in
> mojo/ instead of adding a mojo directory to third_party/WebKit and keeping our
> stuff there?

If we move the serialization code of WTF::String into WebKit, we may have a
circular dependency issue: the code depends on some internal stuff of mojo
(mojo::internal::Buffer/String_Data/SerializationContext); and mojo depends on
the WTF::String serialization code. I think it probably doesn't worth the effort
to re-arrange things to make that work.

The WTF-friendly bindings should only be used in WebKit, so
WTF::StringUTF8Adaptor technically is not exposed to the outside. :)

WDYT? Thanks!

https://codereview.chromium.org/1751563002/diff/100001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_string_serialization.cc:37:
std::queue<UTF8AdaptorWithPointer> utf8_adaptors_;
On 2016/03/04 04:04:50, haraken wrote:
> 
> Just help me understand: Why do we need to queue the UTF8AdaptorWithPointers?

Serialization consists of two passes: the first uses GetSerializedSize_() to
calculate the size; the second uses Serialize_() to do actual serialization.

Imagine we have such a struct:
struct Foo {
  string a;
  array<string> b;
  map<string, string> c;
};

When serializing Foo, we call GetSerializedSize_() for all the members (some of
them are strings) recursively to compute the total size of Foo; and then we call
Serialize_() recursively in the same order.

If a WTF::String needs to be converted to UTF8, we don't want to do that twice.
Therefore, we store a StringUTF8Adaptor in the first pass, so that we can reuse
it in the second pass.

https://codereview.chromium.org/1751563002/diff/100001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_string_serialization.cc:37:
std::queue<UTF8AdaptorWithPointer> utf8_adaptors_;
On 2016/03/04 04:14:48, esprehn wrote:
> Can we use a std::vector instead? std::deque is not especially efficient, but
> maybe it's okay here.

I think std::queue is more efficient than std::vector in this case, because all
I need is push to the back and remove from the front.

https://codereview.chromium.org/1751563002/diff/100001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_string_serialization.cc:76:
CHECK_EQ(utf8_adaptors.front().original_input, &input);
On 2016/03/04 04:14:48, esprehn wrote:
> Is this really worth the overhead?

Here we compare the string pointers, not the string contents. So the overhead
should be negligible. I changed the type of |original_input| to uintptr_t to be
more clear.  I have also changed this to DCHECK.

This is used as a sanity check to make sure that we never mismatch
StringUTF8Adaptor and String.

https://codereview.chromium.org/1751563002/diff/100001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/tests/wtf_types_unittest.cc (right):

https://codereview.chromium.org/1751563002/diff/100001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/wtf_types_unittest.cc:15: #include
"third_party/WebKit/Source/wtf/Partitions.h"
On 2016/03/04 04:14:48, esprehn wrote:
> this really isn't public API...

I feel that it is not too bad because I only used this for testing.

For non-testing code, the WTF-friendly bindings should only be used inside
WebKit, so we don't have to worry about this.

WDYT? Thanks!

[please use gerrit instead, 2016-03-04 18:36:49]

Description was changed from

==========
Mojo C++ bindings: support mapping mojo string to WTF::String.

The generator now supports a "--use_wtf_types" flag. When it is specified, the
generator will map mojo string to WTF::String.

BUG=583738
==========

to

==========
Mojo C++ bindings: support mapping mojo string to WTF::String.

The generator now supports a "--use_wtf_types" flag. When it is specified, the
generator will map mojo string to WTF::String.

BUG=583738
==========

[yzshen1, 2016-03-04 22:56:27]

Description was changed from

==========
Mojo C++ bindings: support mapping mojo string to WTF::String.

The generator now supports a "--use_wtf_types" flag. When it is specified, the
generator will map mojo string to WTF::String.

BUG=583738
==========

to

==========
Mojo C++ bindings: support mapping mojo string to WTF::String.

The generator now supports a "--for_blink" flag. When it is specified, the
generator will map mojo string to WTF::String.

BUG=583738
==========

[haraken, 2016-03-07 02:04:29]

https://codereview.chromium.org/1751563002/diff/100001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/wtf_string_serialization.cc (right):

https://codereview.chromium.org/1751563002/diff/100001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_string_serialization.cc:37:
std::queue<UTF8AdaptorWithPointer> utf8_adaptors_;
On 2016/03/04 17:28:49, yzshen1 wrote:
> On 2016/03/04 04:04:50, haraken wrote:
> > 
> > Just help me understand: Why do we need to queue the
UTF8AdaptorWithPointers?
> 
> Serialization consists of two passes: the first uses GetSerializedSize_() to
> calculate the size; the second uses Serialize_() to do actual serialization.
> 
> Imagine we have such a struct:
> struct Foo {
>   string a;
>   array<string> b;
>   map<string, string> c;
> };
> 
> When serializing Foo, we call GetSerializedSize_() for all the members (some
of
> them are strings) recursively to compute the total size of Foo; and then we
call
> Serialize_() recursively in the same order.
> 
> If a WTF::String needs to be converted to UTF8, we don't want to do that
twice.
> Therefore, we store a StringUTF8Adaptor in the first pass, so that we can
reuse
> it in the second pass.

Thanks, makes sense. Maybe worth adding a comment?

https://codereview.chromium.org/1751563002/diff/140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/tests/wtf_types_unittest.cc (right):

https://codereview.chromium.org/1751563002/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/wtf_types_unittest.cc:47: void SetUp() override {
WTF::Partitions::initialize(nullptr); }

You'll need to call WTF::Partitions::initialize WTFVectorsTest, WTFHashMapTest
etc... which is nasty.

Would it be possible to introduce a parent class like WTFTest and call
blink::initializeWithoutV8/shutdownWithoutV8 in WTFTest::SetUp/TearDown?

[yzshen1, 2016-03-07 18:26:47]

Thanks! PTAL.

https://codereview.chromium.org/1751563002/diff/100001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/wtf_string_serialization.cc (right):

https://codereview.chromium.org/1751563002/diff/100001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_string_serialization.cc:37:
std::queue<UTF8AdaptorWithPointer> utf8_adaptors_;
On 2016/03/07 02:04:29, haraken wrote:
> On 2016/03/04 17:28:49, yzshen1 wrote:
> > On 2016/03/04 04:04:50, haraken wrote:
> > > 
> > > Just help me understand: Why do we need to queue the
> UTF8AdaptorWithPointers?
> > 
> > Serialization consists of two passes: the first uses GetSerializedSize_() to
> > calculate the size; the second uses Serialize_() to do actual serialization.
> > 
> > Imagine we have such a struct:
> > struct Foo {
> >   string a;
> >   array<string> b;
> >   map<string, string> c;
> > };
> > 
> > When serializing Foo, we call GetSerializedSize_() for all the members (some
> of
> > them are strings) recursively to compute the total size of Foo; and then we
> call
> > Serialize_() recursively in the same order.
> > 
> > If a WTF::String needs to be converted to UTF8, we don't want to do that
> twice.
> > Therefore, we store a StringUTF8Adaptor in the first pass, so that we can
> reuse
> > it in the second pass.
> 
> Thanks, makes sense. Maybe worth adding a comment?

Done.

https://codereview.chromium.org/1751563002/diff/140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/tests/wtf_types_unittest.cc (right):

https://codereview.chromium.org/1751563002/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/wtf_types_unittest.cc:47: void SetUp() override {
WTF::Partitions::initialize(nullptr); }
On 2016/03/07 02:04:29, haraken wrote:
> 
> You'll need to call WTF::Partitions::initialize WTFVectorsTest, WTFHashMapTest
> etc... which is nasty.
> 
> Would it be possible to introduce a parent class like WTFTest and call
> blink::initializeWithoutV8/shutdownWithoutV8 in WTFTest::SetUp/TearDown?

Done. PTAL

[haraken, 2016-03-07 23:54:17]

https://codereview.chromium.org/1751563002/diff/180001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/tests/wtf_test_base.cc (right):

https://codereview.chromium.org/1751563002/diff/180001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/wtf_test_base.cc:36: g_wtf_initialized = true;

Can we use blink::initializeWithoutV8/shutdownWithoutV8?

The reason I want to avoid directly calling WTF:: functions is that they are not
public APIs. It is a layering violation :/

[yzshen1, 2016-03-08 00:17:09]

https://codereview.chromium.org/1751563002/diff/180001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/tests/wtf_test_base.cc (right):

https://codereview.chromium.org/1751563002/diff/180001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/wtf_test_base.cc:36: g_wtf_initialized = true;
On 2016/03/07 23:54:17, haraken wrote:
> 
> Can we use blink::initializeWithoutV8/shutdownWithoutV8?
> 
> The reason I want to avoid directly calling WTF:: functions is that they are
not
> public APIs. It is a layering violation :/

I tried it, but that requires to link against the "web" target which is way
bigger than wtf. :/
IMO, this is a test class that tests exactly WTF stuff. It doesn't seem too bad
to use WTF initialization directly. WDYT? Thanks!

[haraken, 2016-03-08 00:29:29]

https://codereview.chromium.org/1751563002/diff/180001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/tests/wtf_test_base.cc (right):

https://codereview.chromium.org/1751563002/diff/180001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/wtf_test_base.cc:36: g_wtf_initialized = true;
On 2016/03/08 00:17:09, yzshen1 wrote:
> On 2016/03/07 23:54:17, haraken wrote:
> > 
> > Can we use blink::initializeWithoutV8/shutdownWithoutV8?
> > 
> > The reason I want to avoid directly calling WTF:: functions is that they are
> not
> > public APIs. It is a layering violation :/
> 
> I tried it, but that requires to link against the "web" target which is way
> bigger than wtf. :/
> IMO, this is a test class that tests exactly WTF stuff. It doesn't seem too
bad
> to use WTF initialization directly. WDYT? Thanks!

Then can we add a public API (in public/platform/) to initialize only WTF and
platform? (assuming it's acceptable to link against the wtf and platform
target.)

[yzshen1, 2016-03-08 00:48:27]

On 2016/03/08 00:29:29, haraken wrote:
>
https://codereview.chromium.org/1751563002/diff/180001/mojo/public/cpp/bindin...
> File mojo/public/cpp/bindings/tests/wtf_test_base.cc (right):
> 
>
https://codereview.chromium.org/1751563002/diff/180001/mojo/public/cpp/bindin...
> mojo/public/cpp/bindings/tests/wtf_test_base.cc:36: g_wtf_initialized = true;
> On 2016/03/08 00:17:09, yzshen1 wrote:
> > On 2016/03/07 23:54:17, haraken wrote:
> > > 
> > > Can we use blink::initializeWithoutV8/shutdownWithoutV8?
> > > 
> > > The reason I want to avoid directly calling WTF:: functions is that they
are
> > not
> > > public APIs. It is a layering violation :/
> > 
> > I tried it, but that requires to link against the "web" target which is way
> > bigger than wtf. :/
> > IMO, this is a test class that tests exactly WTF stuff. It doesn't seem too
> bad
> > to use WTF initialization directly. WDYT? Thanks!
> 
> Then can we add a public API (in public/platform/) to initialize only WTF and
> platform? (assuming it's acceptable to link against the wtf and platform
> target.)

WTF types are not public API exposed from blink, right? It seem a little weird
to expose a public API to initialize WTF, and then only use it for a testing
target.

The WTF-friendly bindings is used by blink internally. So the corresponding
testing code should probably be considered "internal" too.

WDYT? Thanks!


WDYT? Thanks!

[haraken, 2016-03-08 00:58:52]

On 2016/03/08 00:48:27, yzshen1 wrote:
> On 2016/03/08 00:29:29, haraken wrote:
> >
>
https://codereview.chromium.org/1751563002/diff/180001/mojo/public/cpp/bindin...
> > File mojo/public/cpp/bindings/tests/wtf_test_base.cc (right):
> > 
> >
>
https://codereview.chromium.org/1751563002/diff/180001/mojo/public/cpp/bindin...
> > mojo/public/cpp/bindings/tests/wtf_test_base.cc:36: g_wtf_initialized =
true;
> > On 2016/03/08 00:17:09, yzshen1 wrote:
> > > On 2016/03/07 23:54:17, haraken wrote:
> > > > 
> > > > Can we use blink::initializeWithoutV8/shutdownWithoutV8?
> > > > 
> > > > The reason I want to avoid directly calling WTF:: functions is that they
> are
> > > not
> > > > public APIs. It is a layering violation :/
> > > 
> > > I tried it, but that requires to link against the "web" target which is
way
> > > bigger than wtf. :/
> > > IMO, this is a test class that tests exactly WTF stuff. It doesn't seem
too
> > bad
> > > to use WTF initialization directly. WDYT? Thanks!
> > 
> > Then can we add a public API (in public/platform/) to initialize only WTF
and
> > platform? (assuming it's acceptable to link against the wtf and platform
> > target.)
> 
> WTF types are not public API exposed from blink, right? It seem a little weird
> to expose a public API to initialize WTF, and then only use it for a testing
> target.
> 
> The WTF-friendly bindings is used by blink internally. So the corresponding
> testing code should probably be considered "internal" too.
> 
> WDYT? Thanks!
> 
> 
> WDYT? Thanks!

Hmm, once you support a type conversion for HeapVector, HeapHashMap etc, you'll
anyway need to link the mojo tests with platform/ because HeapVector and
HeapHashMap are defined in platform/. Also you'll need to initialize Oilpan's GC
in platform/ to use HeapVector etc.

So I think implementing initializePlatformAndWTF/shutdownPlatformAndWTF in
public/platform/ would make more sense. Eventually
initializePlatformAndWTF/shutdownPlatformAndWTF should be unified with
initializeWithoutV8/shutdownWithoutV8.

+Elliott

[yzshen1, 2016-03-08 02:04:05]

On 2016/03/08 00:58:52, haraken wrote:
> On 2016/03/08 00:48:27, yzshen1 wrote:
> > On 2016/03/08 00:29:29, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/1751563002/diff/180001/mojo/public/cpp/bindin...
> > > File mojo/public/cpp/bindings/tests/wtf_test_base.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1751563002/diff/180001/mojo/public/cpp/bindin...
> > > mojo/public/cpp/bindings/tests/wtf_test_base.cc:36: g_wtf_initialized =
> true;
> > > On 2016/03/08 00:17:09, yzshen1 wrote:
> > > > On 2016/03/07 23:54:17, haraken wrote:
> > > > > 
> > > > > Can we use blink::initializeWithoutV8/shutdownWithoutV8?
> > > > > 
> > > > > The reason I want to avoid directly calling WTF:: functions is that
they
> > are
> > > > not
> > > > > public APIs. It is a layering violation :/
> > > > 
> > > > I tried it, but that requires to link against the "web" target which is
> way
> > > > bigger than wtf. :/
> > > > IMO, this is a test class that tests exactly WTF stuff. It doesn't seem
> too
> > > bad
> > > > to use WTF initialization directly. WDYT? Thanks!
> > > 
> > > Then can we add a public API (in public/platform/) to initialize only WTF
> and
> > > platform? (assuming it's acceptable to link against the wtf and platform
> > > target.)
> > 
> > WTF types are not public API exposed from blink, right? It seem a little
weird
> > to expose a public API to initialize WTF, and then only use it for a testing
> > target.
> > 
> > The WTF-friendly bindings is used by blink internally. So the corresponding
> > testing code should probably be considered "internal" too.
> > 
> > WDYT? Thanks!
> > 
> > 
> > WDYT? Thanks!
> 
> Hmm, once you support a type conversion for HeapVector, HeapHashMap etc,
you'll
> anyway need to link the mojo tests with platform/ because HeapVector and
> HeapHashMap are defined in platform/. Also you'll need to initialize Oilpan's
GC
> in platform/ to use HeapVector etc.

I am not aware of any plan to support HeapVector/HeapHashMap/etc. Would you
please share with me more details about that?
In my current plan, the binding generator always maps mojo array (map, string)
to a specific type for blink.

If it turns out that you need to be able to switch between multiple types, you
probably want the support of specifying custom mapping. Currently we only have
custom type mapping for structs.
I think it is probably something that we can consider later, right?

I would be happy to have a meeting with you to talk about this. Please let me
know.

> So I think implementing initializePlatformAndWTF/shutdownPlatformAndWTF in
> public/platform/ would make more sense. Eventually
> initializePlatformAndWTF/shutdownPlatformAndWTF should be unified with
> initializeWithoutV8/shutdownWithoutV8.

I don't think I know as much as you do about the initialization stuff, although
it still seems a little odd to expose a public API for a single testing target.

Could we consider this as a non blocking issue for this CL?  :) I would be happy
to switch to use any API that is more suitable as soon as it is available. It
should be a very small change.

WDYT? Thanks!


> 
> +Elliott

[haraken, 2016-03-08 02:36:15]

On 2016/03/08 02:04:05, yzshen1 wrote:
> On 2016/03/08 00:58:52, haraken wrote:
> > On 2016/03/08 00:48:27, yzshen1 wrote:
> > > On 2016/03/08 00:29:29, haraken wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1751563002/diff/180001/mojo/public/cpp/bindin...
> > > > File mojo/public/cpp/bindings/tests/wtf_test_base.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1751563002/diff/180001/mojo/public/cpp/bindin...
> > > > mojo/public/cpp/bindings/tests/wtf_test_base.cc:36: g_wtf_initialized =
> > true;
> > > > On 2016/03/08 00:17:09, yzshen1 wrote:
> > > > > On 2016/03/07 23:54:17, haraken wrote:
> > > > > > 
> > > > > > Can we use blink::initializeWithoutV8/shutdownWithoutV8?
> > > > > > 
> > > > > > The reason I want to avoid directly calling WTF:: functions is that
> they
> > > are
> > > > > not
> > > > > > public APIs. It is a layering violation :/
> > > > > 
> > > > > I tried it, but that requires to link against the "web" target which
is
> > way
> > > > > bigger than wtf. :/
> > > > > IMO, this is a test class that tests exactly WTF stuff. It doesn't
seem
> > too
> > > > bad
> > > > > to use WTF initialization directly. WDYT? Thanks!
> > > > 
> > > > Then can we add a public API (in public/platform/) to initialize only
WTF
> > and
> > > > platform? (assuming it's acceptable to link against the wtf and platform
> > > > target.)
> > > 
> > > WTF types are not public API exposed from blink, right? It seem a little
> weird
> > > to expose a public API to initialize WTF, and then only use it for a
testing
> > > target.
> > > 
> > > The WTF-friendly bindings is used by blink internally. So the
corresponding
> > > testing code should probably be considered "internal" too.
> > > 
> > > WDYT? Thanks!
> > > 
> > > 
> > > WDYT? Thanks!
> > 
> > Hmm, once you support a type conversion for HeapVector, HeapHashMap etc,
> you'll
> > anyway need to link the mojo tests with platform/ because HeapVector and
> > HeapHashMap are defined in platform/. Also you'll need to initialize
Oilpan's
> GC
> > in platform/ to use HeapVector etc.
> 
> I am not aware of any plan to support HeapVector/HeapHashMap/etc. Would you
> please share with me more details about that?
> In my current plan, the binding generator always maps mojo array (map, string)
> to a specific type for blink.

HeapVector/HeapHashMap is a GC-managed version of the Vector/HashMap.
HeapVector/HeapHashMap inherits from Vector/HashMap, so it would be fairly easy
to support them once we need them.

> If it turns out that you need to be able to switch between multiple types, you
> probably want the support of specifying custom mapping. Currently we only have
> custom type mapping for structs.
> I think it is probably something that we can consider later, right?
> 
> I would be happy to have a meeting with you to talk about this. Please let me
> know.
> 
> > So I think implementing initializePlatformAndWTF/shutdownPlatformAndWTF in
> > public/platform/ would make more sense. Eventually
> > initializePlatformAndWTF/shutdownPlatformAndWTF should be unified with
> > initializeWithoutV8/shutdownWithoutV8.
> 
> I don't think I know as much as you do about the initialization stuff,
although
> it still seems a little odd to expose a public API for a single testing
target.
> 
> Could we consider this as a non blocking issue for this CL?  :) I would be
happy
> to switch to use any API that is more suitable as soon as it is available. It
> should be a very small change.
> 
> WDYT? Thanks!

I'd like to hear Elliott's opinion here.

FWIW, the initialization issue is not that trivial as you think. We've caused a
bunch of breakages because the initialization/shutdown of the chromium-side
tests were not appropriately handled. I can easily imagine that the mojo tests
start failing once they start depending on something (e.g., something in
platform) because the initialization is not enough. That's why I want to use
initializeWithoutV8/shutdownWithoutV8 as much as possible.

[yzshen1, 2016-03-08 04:01:35]

On 2016/03/08 02:36:15, haraken wrote:
> On 2016/03/08 02:04:05, yzshen1 wrote:
> > On 2016/03/08 00:58:52, haraken wrote:
> > > On 2016/03/08 00:48:27, yzshen1 wrote:
> > > > On 2016/03/08 00:29:29, haraken wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1751563002/diff/180001/mojo/public/cpp/bindin...
> > > > > File mojo/public/cpp/bindings/tests/wtf_test_base.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1751563002/diff/180001/mojo/public/cpp/bindin...
> > > > > mojo/public/cpp/bindings/tests/wtf_test_base.cc:36: g_wtf_initialized
=
> > > true;
> > > > > On 2016/03/08 00:17:09, yzshen1 wrote:
> > > > > > On 2016/03/07 23:54:17, haraken wrote:
> > > > > > > 
> > > > > > > Can we use blink::initializeWithoutV8/shutdownWithoutV8?
> > > > > > > 
> > > > > > > The reason I want to avoid directly calling WTF:: functions is
that
> > they
> > > > are
> > > > > > not
> > > > > > > public APIs. It is a layering violation :/
> > > > > > 
> > > > > > I tried it, but that requires to link against the "web" target which
> is
> > > way
> > > > > > bigger than wtf. :/
> > > > > > IMO, this is a test class that tests exactly WTF stuff. It doesn't
> seem
> > > too
> > > > > bad
> > > > > > to use WTF initialization directly. WDYT? Thanks!
> > > > > 
> > > > > Then can we add a public API (in public/platform/) to initialize only
> WTF
> > > and
> > > > > platform? (assuming it's acceptable to link against the wtf and
platform
> > > > > target.)
> > > > 
> > > > WTF types are not public API exposed from blink, right? It seem a little
> > weird
> > > > to expose a public API to initialize WTF, and then only use it for a
> testing
> > > > target.
> > > > 
> > > > The WTF-friendly bindings is used by blink internally. So the
> corresponding
> > > > testing code should probably be considered "internal" too.
> > > > 
> > > > WDYT? Thanks!
> > > > 
> > > > 
> > > > WDYT? Thanks!
> > > 
> > > Hmm, once you support a type conversion for HeapVector, HeapHashMap etc,
> > you'll
> > > anyway need to link the mojo tests with platform/ because HeapVector and
> > > HeapHashMap are defined in platform/. Also you'll need to initialize
> Oilpan's
> > GC
> > > in platform/ to use HeapVector etc.
> > 
> > I am not aware of any plan to support HeapVector/HeapHashMap/etc. Would you
> > please share with me more details about that?
> > In my current plan, the binding generator always maps mojo array (map,
string)
> > to a specific type for blink.
> 
> HeapVector/HeapHashMap is a GC-managed version of the Vector/HashMap.
> HeapVector/HeapHashMap inherits from Vector/HashMap, so it would be fairly
easy
> to support them once we need them.

Unfortunately, the issue of supporting multiple types is not that trivial as you
think.
For example, if I have the following mojom interface:

// mojom:
interface Foo {
  GetBar() => (array<int32_t> bar);
  GetQux() => (array<int32_t> qux);
};

How should I let the generator know if I would like to generate bindings that 
map |bar| to a WTF::Vector, while |qux| to a HeapVector?
There must be ways to annotate what type to be used, if there are more than one
candidate. Are we going to special case something, or extending the current
struct type mapping mechanism?

So if there is such feature requests, please raise them early. That will help to
avoid unexpected delay or last-minute design changes. :)

> 
> > If it turns out that you need to be able to switch between multiple types,
you
> > probably want the support of specifying custom mapping. Currently we only
have
> > custom type mapping for structs.
> > I think it is probably something that we can consider later, right?
> > 
> > I would be happy to have a meeting with you to talk about this. Please let
me
> > know.
> > 
> > > So I think implementing initializePlatformAndWTF/shutdownPlatformAndWTF in
> > > public/platform/ would make more sense. Eventually
> > > initializePlatformAndWTF/shutdownPlatformAndWTF should be unified with
> > > initializeWithoutV8/shutdownWithoutV8.
> > 
> > I don't think I know as much as you do about the initialization stuff,
> although
> > it still seems a little odd to expose a public API for a single testing
> target.
> > 
> > Could we consider this as a non blocking issue for this CL?  :) I would be
> happy
> > to switch to use any API that is more suitable as soon as it is available.
It
> > should be a very small change.
> > 
> > WDYT? Thanks!
> 
> I'd like to hear Elliott's opinion here.
> 
> FWIW, the initialization issue is not that trivial as you think. We've caused
a
> bunch of breakages because the initialization/shutdown of the chromium-side
> tests were not appropriately handled. I can easily imagine that the mojo tests
> start failing once they start depending on something (e.g., something in
> platform) because the initialization is not enough. That's why I want to use
> initializeWithoutV8/shutdownWithoutV8 as much as possible.

[haraken, 2016-03-08 04:10:03]

On 2016/03/08 04:01:35, yzshen1 wrote:
> On 2016/03/08 02:36:15, haraken wrote:
> > On 2016/03/08 02:04:05, yzshen1 wrote:
> > > On 2016/03/08 00:58:52, haraken wrote:
> > > > On 2016/03/08 00:48:27, yzshen1 wrote:
> > > > > On 2016/03/08 00:29:29, haraken wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1751563002/diff/180001/mojo/public/cpp/bindin...
> > > > > > File mojo/public/cpp/bindings/tests/wtf_test_base.cc (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1751563002/diff/180001/mojo/public/cpp/bindin...
> > > > > > mojo/public/cpp/bindings/tests/wtf_test_base.cc:36:
g_wtf_initialized
> =
> > > > true;
> > > > > > On 2016/03/08 00:17:09, yzshen1 wrote:
> > > > > > > On 2016/03/07 23:54:17, haraken wrote:
> > > > > > > > 
> > > > > > > > Can we use blink::initializeWithoutV8/shutdownWithoutV8?
> > > > > > > > 
> > > > > > > > The reason I want to avoid directly calling WTF:: functions is
> that
> > > they
> > > > > are
> > > > > > > not
> > > > > > > > public APIs. It is a layering violation :/
> > > > > > > 
> > > > > > > I tried it, but that requires to link against the "web" target
which
> > is
> > > > way
> > > > > > > bigger than wtf. :/
> > > > > > > IMO, this is a test class that tests exactly WTF stuff. It doesn't
> > seem
> > > > too
> > > > > > bad
> > > > > > > to use WTF initialization directly. WDYT? Thanks!
> > > > > > 
> > > > > > Then can we add a public API (in public/platform/) to initialize
only
> > WTF
> > > > and
> > > > > > platform? (assuming it's acceptable to link against the wtf and
> platform
> > > > > > target.)
> > > > > 
> > > > > WTF types are not public API exposed from blink, right? It seem a
little
> > > weird
> > > > > to expose a public API to initialize WTF, and then only use it for a
> > testing
> > > > > target.
> > > > > 
> > > > > The WTF-friendly bindings is used by blink internally. So the
> > corresponding
> > > > > testing code should probably be considered "internal" too.
> > > > > 
> > > > > WDYT? Thanks!
> > > > > 
> > > > > 
> > > > > WDYT? Thanks!
> > > > 
> > > > Hmm, once you support a type conversion for HeapVector, HeapHashMap etc,
> > > you'll
> > > > anyway need to link the mojo tests with platform/ because HeapVector and
> > > > HeapHashMap are defined in platform/. Also you'll need to initialize
> > Oilpan's
> > > GC
> > > > in platform/ to use HeapVector etc.
> > > 
> > > I am not aware of any plan to support HeapVector/HeapHashMap/etc. Would
you
> > > please share with me more details about that?
> > > In my current plan, the binding generator always maps mojo array (map,
> string)
> > > to a specific type for blink.
> > 
> > HeapVector/HeapHashMap is a GC-managed version of the Vector/HashMap.
> > HeapVector/HeapHashMap inherits from Vector/HashMap, so it would be fairly
> easy
> > to support them once we need them.
> 
> Unfortunately, the issue of supporting multiple types is not that trivial as
you
> think.
> For example, if I have the following mojom interface:
> 
> // mojom:
> interface Foo {
>   GetBar() => (array<int32_t> bar);
>   GetQux() => (array<int32_t> qux);
> };
> 
> How should I let the generator know if I would like to generate bindings that 
> map |bar| to a WTF::Vector, while |qux| to a HeapVector?
> There must be ways to annotate what type to be used, if there are more than
one
> candidate. Are we going to special case something, or extending the current
> struct type mapping mechanism?
> 
> So if there is such feature requests, please raise them early. That will help
to
> avoid unexpected delay or last-minute design changes. :)

Yeah, I considered this a bit more and concluded that it won't happen that we
want to pass HeapVector/HeapHashMap to Mojo types. HeapVector/HeapHashMap are
containers that hold references to GC-managed objects in Blink. It would be
already invalid to pass (and serialize/deserialize) such references to outside
Blink. Sorry about the noise :)

[esprehn, 2016-03-10 19:50:10]

The more I think about this the more I think we really want this code to be
generated into blink, and the shims to live in WTF itself. Blink should have a
target that invokes the code generator and then runs the tests?

Having another init path in Platform just for mojo tests seems very strange.

[yzshen1, 2016-03-10 20:38:44]

On 2016/03/10 19:50:10, esprehn wrote:
> The more I think about this the more I think we really want this code to be
> generated into blink,
Do you mean the generated code such as
"//mojo/public/interfaces/bindings/tests:test_wtf_types_blink"?

> and the shims to live in WTF itself.
By shims you mean things like mojo::WTFArray? That is something we could
consider.

Even doing that, I think the serialization logic for things like
WTF::String/mojo::WTFArray has to live in mojo/public/cpp/bindings/lib.
I have listed the reasons in the other CL:
- otherwise we will have circular dependencies (mojo depends on WTF, WTF depends
on mojo)
- the serialization logic uses mojo::internal things.
- I would like template specialization of things like ValueTraits/etc. to stay
in one place.

> Blink should have a
> target that invokes the code generator and then runs the tests?
> 
> Having another init path in Platform just for mojo tests seems very strange.

Because it tests mostly the serialization logic, I wish it could stay as close
to the code as possible, i.e., in mojo/.

If you are all strongly against the current way of initialization, I could use
initializeWithoutV8/shutdownWithoutV8 instead, which should address your
concerns. It is just a little unfortunate that I have to link in so much stuff.
:) Please let me know.

Thanks!

[haraken, 2016-03-10 23:57:11]

On 2016/03/10 20:38:44, yzshen1 wrote:
> On 2016/03/10 19:50:10, esprehn wrote:
> > The more I think about this the more I think we really want this code to be
> > generated into blink,
> Do you mean the generated code such as
> "//mojo/public/interfaces/bindings/tests:test_wtf_types_blink"?
> 
> > and the shims to live in WTF itself.
> By shims you mean things like mojo::WTFArray? That is something we could
> consider.
> 
> Even doing that, I think the serialization logic for things like
> WTF::String/mojo::WTFArray has to live in mojo/public/cpp/bindings/lib.
> I have listed the reasons in the other CL:
> - otherwise we will have circular dependencies (mojo depends on WTF, WTF
depends
> on mojo)
> - the serialization logic uses mojo::internal things.
> - I would like template specialization of things like ValueTraits/etc. to stay
> in one place.
> 
> > Blink should have a
> > target that invokes the code generator and then runs the tests?
> > 
> > Having another init path in Platform just for mojo tests seems very strange.
> 
> Because it tests mostly the serialization logic, I wish it could stay as close
> to the code as possible, i.e., in mojo/.
> 
> If you are all strongly against the current way of initialization, I could use
> initializeWithoutV8/shutdownWithoutV8 instead, which should address your
> concerns. It is just a little unfortunate that I have to link in so much
stuff.
> :) Please let me know.

Regarding the initialization path, I'm now removing
initializeWithoutV8/shutdownWithoutV8 so that you can just call
Platform::initialize/shutdown
(https://bugs.chromium.org/p/chromium/issues/detail?id=593269).

[haraken, 2016-03-11 09:08:09]

On 2016/03/10 23:57:11, haraken wrote:
> On 2016/03/10 20:38:44, yzshen1 wrote:
> > On 2016/03/10 19:50:10, esprehn wrote:
> > > The more I think about this the more I think we really want this code to
be
> > > generated into blink,
> > Do you mean the generated code such as
> > "//mojo/public/interfaces/bindings/tests:test_wtf_types_blink"?
> > 
> > > and the shims to live in WTF itself.
> > By shims you mean things like mojo::WTFArray? That is something we could
> > consider.
> > 
> > Even doing that, I think the serialization logic for things like
> > WTF::String/mojo::WTFArray has to live in mojo/public/cpp/bindings/lib.
> > I have listed the reasons in the other CL:
> > - otherwise we will have circular dependencies (mojo depends on WTF, WTF
> depends
> > on mojo)
> > - the serialization logic uses mojo::internal things.
> > - I would like template specialization of things like ValueTraits/etc. to
stay
> > in one place.
> > 
> > > Blink should have a
> > > target that invokes the code generator and then runs the tests?
> > > 
> > > Having another init path in Platform just for mojo tests seems very
strange.
> > 
> > Because it tests mostly the serialization logic, I wish it could stay as
close
> > to the code as possible, i.e., in mojo/.
> > 
> > If you are all strongly against the current way of initialization, I could
use
> > initializeWithoutV8/shutdownWithoutV8 instead, which should address your
> > concerns. It is just a little unfortunate that I have to link in so much
> stuff.
> > :) Please let me know.
> 
> Regarding the initialization path, I'm now removing
> initializeWithoutV8/shutdownWithoutV8 so that you can just call
> Platform::initialize/shutdown
> (https://bugs.chromium.org/p/chromium/issues/detail?id=593269).

Once I land https://codereview.chromium.org/1785163002/, you can use
Platform::initialize/shutdown.

[yzshen1, 2016-03-11 19:08:59]

Thanks Kentaro!

After some more thoughts, I made the Mojo WTF-related tests a
source_set/static_library, and directly linked that into
blink_platfrom_unittests.
Please see:
- third_party/WebKit/Source/platform/BUILD.gn
- third_party/WebKit/Source/platform/blink_platform_tests.gyp

I think that probably is the most standard way to do the tests. WDYT? Thanks!

[esprehn, 2016-03-11 22:29:56]

On 2016/03/11 at 19:08:59, yzshen wrote:
> Thanks Kentaro!
> 
> After some more thoughts, I made the Mojo WTF-related tests a
source_set/static_library, and directly linked that into
blink_platfrom_unittests.
> Please see:
> - third_party/WebKit/Source/platform/BUILD.gn
> - third_party/WebKit/Source/platform/blink_platform_tests.gyp
> 
> I think that probably is the most standard way to do the tests. WDYT? Thanks!

I think that seems good, I don't want to block this much longer. We have many
people waiting for WTF based mojo in blink.

[haraken, 2016-03-13 16:21:07]

Sorry for blocking your work with the testing issues and thanks for being
persistent!

LGTM.

https://codereview.chromium.org/1751563002/diff/220001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/wtf_string_serialization.cc (right):

https://codereview.chromium.org/1751563002/diff/220001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_string_serialization.cc:89: #if DCHECK_IS_ON()

Remove this macro?

[yzshen1, 2016-03-14 06:36:27]

Thanks Kentaro for your suggestions. :)

https://codereview.chromium.org/1751563002/diff/220001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/wtf_string_serialization.cc (right):

https://codereview.chromium.org/1751563002/diff/220001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_string_serialization.cc:89: #if DCHECK_IS_ON()
On 2016/03/13 16:21:07, haraken wrote:
> 
> Remove this macro?

I had to add this #if because otherwise some bots were unhappy. It seems on
those bots DCHECK is evaluated even if DCHECK_IS_ON() is false.

[yzshen1, 2016-03-14 06:36:46]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2016-03-14 06:36:47]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/1751563002/#ps220001
(title: " ")

[commit-bot: I haz the power, 2016-03-14 06:36:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1751563002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1751563002/220001

[commit-bot: I haz the power, 2016-03-14 06:43:42]

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

[commit-bot: I haz the power, 2016-03-14 06:43:43]

Try jobs failed on following builders:
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)

[yzshen1, 2016-03-14 20:48:35]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2016-03-14 20:48:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/1751563002/#ps260001
(title: "sync & rebase")

[commit-bot: I haz the power, 2016-03-14 20:49:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1751563002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1751563002/260001

[commit-bot: I haz the power, 2016-03-14 22:26:34]

Description was changed from

==========
Mojo C++ bindings: support mapping mojo string to WTF::String.

The generator now supports a "--for_blink" flag. When it is specified, the
generator will map mojo string to WTF::String.

BUG=583738
==========

to

==========
Mojo C++ bindings: support mapping mojo string to WTF::String.

The generator now supports a "--for_blink" flag. When it is specified, the
generator will map mojo string to WTF::String.

BUG=583738
==========

[commit-bot: I haz the power, 2016-03-14 22:26:36]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-03-14 22:28:36]

Description was changed from

==========
Mojo C++ bindings: support mapping mojo string to WTF::String.

The generator now supports a "--for_blink" flag. When it is specified, the
generator will map mojo string to WTF::String.

BUG=583738
==========

to

==========
Mojo C++ bindings: support mapping mojo string to WTF::String.

The generator now supports a "--for_blink" flag. When it is specified, the
generator will map mojo string to WTF::String.

BUG=583738

Committed: https://crrev.com/791f9b24ca1758ced39235bda1a4e0411801309c
Cr-Commit-Position: refs/heads/master@{#381091}
==========

[commit-bot: I haz the power, 2016-03-14 22:28:37]

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

[loyso (OOO), 2016-03-15 02:10:06]

Is this CL related to linux_blink_rel failures on trybots?


https://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/...
./blink_platform_unittests: error while loading shared libraries:
libmojo_public_test_support.so: cannot open shared object file: No such file or
directory

[yzshen1, 2016-03-15 02:41:50]

Thanks for informing me of this!

Yes, it is related.

I am now looking at it. If I cannot fix it within, say, half an hour, I
will revert. WDYT? Thanks!

On Mon, Mar 14, 2016 at 7:10 PM, <loyso@chromium.org> wrote:

> Is this CL related to linux_blink_rel failures on trybots?
>
>
>
>
https://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/...
> ./blink_platform_unittests: error while loading shared libraries:
> libmojo_public_test_support.so: cannot open shared object file: No such
> file or
> directory
>
> https://codereview.chromium.org/1751563002/
>

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

[yzshen1, 2016-03-15 03:10:11]

I have landed a fix for it: https://codereview.chromium.org/1800843003/

I will keep an eye on the bots.

On Mon, Mar 14, 2016 at 7:41 PM, Yuzhu Shen <yzshen@chromium.org> wrote:

> Thanks for informing me of this!
>
> Yes, it is related.
>
> I am now looking at it. If I cannot fix it within, say, half an hour, I
> will revert. WDYT? Thanks!
>
> On Mon, Mar 14, 2016 at 7:10 PM, <loyso@chromium.org> wrote:
>
>> Is this CL related to linux_blink_rel failures on trybots?
>>
>>
>>
>>
https://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/...
>> ./blink_platform_unittests: error while loading shared libraries:
>> libmojo_public_test_support.so: cannot open shared object file: No such
>> file or
>> directory
>>
>> https://codereview.chromium.org/1751563002/
>>
>
>

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

[Nico, 2016-06-06 14:27:46]

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

[Nico, 2016-06-06 14:27:48]

I agree with Elliot that it looks very surprising that code not in blink depends
on blink internals like wtf. mojo is the only project doing this:
https://cs.chromium.org/search/?q=wtf+file:gyp+-file:third_party/webkit&sq=pa...

Was this discussed with platform-architecture-dev somewhere? Neither BUG= nor CL
description nor an in-code comment point at any discussion about this.

https://codereview.chromium.org/1751563002/diff/260001/mojo/mojo_public.gyp
File mojo/mojo_public.gyp (right):

https://codereview.chromium.org/1751563002/diff/260001/mojo/mojo_public.gyp#n...
mojo/mojo_public.gyp:222: 'clang_warning_flags_unset': [ '-Wglobal-constructors'
],
this looks incorrect. why do you need this?

[yzshen1, 2016-06-06 19:33:04]

On 2016/06/06 14:27:48, Nico wrote:
> I agree with Elliot that it looks very surprising that code not in blink
depends
> on blink internals like wtf. mojo is the only project doing this:
>
https://cs.chromium.org/search/?q=wtf+file:gyp+-file:third_party/webkit&sq=pa...
> 
> Was this discussed with platform-architecture-dev somewhere? Neither BUG= nor
CL
> description nor an in-code comment point at any discussion about this.
> 

The code referred to wtf in mojo is only used in blink, but not anywhere outside
of it.
It was discussed in person with Kentaro and Elliot.

There is a refactoring in progress that will allow us to eventually move
wtf-related code into blink.
https://bugs.chromium.org/p/chromium/issues/detail?id=577686

I have also filed bug crbug.com/617718 explicitly talking about removing this
dependency.

> https://codereview.chromium.org/1751563002/diff/260001/mojo/mojo_public.gyp
> File mojo/mojo_public.gyp (right):
> 
>
https://codereview.chromium.org/1751563002/diff/260001/mojo/mojo_public.gyp#n...
> mojo/mojo_public.gyp:222: 'clang_warning_flags_unset': [
'-Wglobal-constructors'
> ],
> this looks incorrect. why do you need this?

That is because gtest cannot compile with this flag. I have uploaded a CL to
unset the flag only for unittests.

Please review: https://codereview.chromium.org/2038413003/

Thanks!