Mojo C++ bindings: switch the existing usage of StructTraits to use the new data view interface.

Mojo C++ bindings: switch the existing usage of StructTraits to use the new data view interface.

BUG=577686
Committed: https://crrev.com/2017646cf12650e54c22fab5195dfb225617a009
Cr-Commit-Position: refs/heads/master@{#393448}

[yzshen1, 2016-05-10 23:27:15]

yzshen@chromium.org changed reviewers:
+ jam@chromium.org, rockot@chromium.org

[yzshen1, 2016-05-10 23:27:15]

Hi, Ken and John.

Would you please take a look? Thanks!

One thing that I wish you could look at is how this affects StructTraits::Read()
implementations (and therefore affects our users).

Thanks!

[Ken Rockot(use gerrit already), 2016-05-11 15:25:07]

On 2016/05/10 at 23:27:15, yzshen wrote:
> Hi, Ken and John.
> 
> Would you please take a look? Thanks!
> 
> One thing that I wish you could look at is how this affects
StructTraits::Read() implementations (and therefore affects our users).

It does mean Read implementations become a bit more verbose, but I don't think
that's necessarily a bad thing. I like that it's explicit and easy to
understand.

The one complaint I have is with the is_null() checks in every Read method. I
wonder if it would be worth avoiding this by adding another StructTraits method
which initializes an output value from a null encoded struct. So if you have

struct Foo {
  Bar bar;
}

struct Baz {
  Bar? bar;
}

and you define

template <>
struct StructTraits<Bar, BarImpl> {
  // ...

  static void CreateFromNull(BarImpl* out) { /* ... */ }
}


then FooDataView::ReadBar can simply return failure if bar is null (and use
Bar's Read otherwise), while BazDataView::ReadBar can use Bar's CreateFromNull
if the field is null and otherwise fallback on Bar's Read.

I don't feel strongly about this since it won't lead to any less code being
written, it just makes the null handling requirement more obvious.

WDYT?

> 
> Thanks!

[yzshen1, 2016-05-11 15:57:21]

On 2016/05/11 15:25:07, Ken Rockot wrote:
> On 2016/05/10 at 23:27:15, yzshen wrote:
> > Hi, Ken and John.
> > 
> > Would you please take a look? Thanks!
> > 
> > One thing that I wish you could look at is how this affects
> StructTraits::Read() implementations (and therefore affects our users).
> 
> It does mean Read implementations become a bit more verbose, but I don't think
> that's necessarily a bad thing. I like that it's explicit and easy to
> understand.
> 
> The one complaint I have is with the is_null() checks in every Read method. I
> wonder if it would be worth avoiding this by adding another StructTraits
method
> which initializes an output value from a null encoded struct. So if you have
> 
> struct Foo {
>   Bar bar;
> }
> 
> struct Baz {
>   Bar? bar;
> }
> 
> and you define
> 
> template <>
> struct StructTraits<Bar, BarImpl> {
>   // ...
> 
>   static void CreateFromNull(BarImpl* out) { /* ... */ }
> }
> 
> 
> then FooDataView::ReadBar can simply return failure if bar is null (and use
> Bar's Read otherwise), while BazDataView::ReadBar can use Bar's CreateFromNull
> if the field is null and otherwise fallback on Bar's Read.

I think this is a good idea that worth considering. I already had such a member,
call SetToNull(), in ArrayTraits.

Currently StructTraits supports an optional IsNull(const T& value) member. If it
is not defined, T is always considered non-null. (GURL is one example.)
However, CreateFromNull()/SetToNull() won't be optional, though. For the GURL
case, this function needs to set GURL to empty.
That is because a url.mojom.Url may be sent from a different typemapping that
supports null url.mojom.Url (such as the standard JS/Java bindings).

Do you think this optional/non-optional mismatch a problem? I don't have a
strong opinion. The mismatch already exists in ArrayTraits, but users are much
less likely to define ArrayTraits themselves.

Moving the null check into FooDataView::ReadBar/BazDataView::ReadBar increases
the code size a little bit. That is probably negligible though.

> 
> I don't feel strongly about this since it won't lead to any less code being
> written, it just makes the null handling requirement more obvious.
> 
> WDYT?
> 
> > 
> > Thanks!

[Ken Rockot(use gerrit already), 2016-05-11 16:13:16]

On 2016/05/11 at 15:57:21, yzshen wrote:
> On 2016/05/11 15:25:07, Ken Rockot wrote:
> > On 2016/05/10 at 23:27:15, yzshen wrote:
> > > Hi, Ken and John.
> > > 
> > > Would you please take a look? Thanks!
> > > 
> > > One thing that I wish you could look at is how this affects
> > StructTraits::Read() implementations (and therefore affects our users).
> > 
> > It does mean Read implementations become a bit more verbose, but I don't
think
> > that's necessarily a bad thing. I like that it's explicit and easy to
> > understand.
> > 
> > The one complaint I have is with the is_null() checks in every Read method.
I
> > wonder if it would be worth avoiding this by adding another StructTraits
method
> > which initializes an output value from a null encoded struct. So if you have
> > 
> > struct Foo {
> >   Bar bar;
> > }
> > 
> > struct Baz {
> >   Bar? bar;
> > }
> > 
> > and you define
> > 
> > template <>
> > struct StructTraits<Bar, BarImpl> {
> >   // ...
> > 
> >   static void CreateFromNull(BarImpl* out) { /* ... */ }
> > }
> > 
> > 
> > then FooDataView::ReadBar can simply return failure if bar is null (and use
> > Bar's Read otherwise), while BazDataView::ReadBar can use Bar's
CreateFromNull
> > if the field is null and otherwise fallback on Bar's Read.
> 
> I think this is a good idea that worth considering. I already had such a
member, call SetToNull(), in ArrayTraits.
> 
> Currently StructTraits supports an optional IsNull(const T& value) member. If
it is not defined, T is always considered non-null. (GURL is one example.)
> However, CreateFromNull()/SetToNull() won't be optional, though. For the GURL
case, this function needs to set GURL to empty.
> That is because a url.mojom.Url may be sent from a different typemapping that
supports null url.mojom.Url (such as the standard JS/Java bindings).
> 
> Do you think this optional/non-optional mismatch a problem? I don't have a
strong opinion. The mismatch already exists in ArrayTraits, but users are much
less likely to define ArrayTraits themselves.
> 
> Moving the null check into FooDataView::ReadBar/BazDataView::ReadBar increases
the code size a little bit. That is probably negligible though.
> 
> > 
> > I don't feel strongly about this since it won't lead to any less code being
> > written, it just makes the null handling requirement more obvious.
> > 
> > WDYT?
> > 
> > > 
> > > Thanks!

OK - Let's leave it as-is for now unless John has a strong opinion about it.
LGTM

[yzshen1, 2016-05-11 17:33:38]

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

[yzshen1, 2016-05-11 17:33:38]

+Daniel for struct traits security review.

This CL change StructTraits::Read() to use a new type for the input data
(*DataView).

It updates the implementations to handle null input data properly.  The previous
code will crash if null is received.

[dcheng, 2016-05-11 18:04:22]

https://codereview.chromium.org/1966933002/diff/40001/url/mojo/origin_struct_...
File url/mojo/origin_struct_traits.h (right):

https://codereview.chromium.org/1966933002/diff/40001/url/mojo/origin_struct_...
url/mojo/origin_struct_traits.h:29: if (data.is_null()) {
Why/when would data be null? Why do we consider this to be a deserialization
success?

[yzshen1, 2016-05-11 18:18:25]

https://codereview.chromium.org/1966933002/diff/40001/url/mojo/origin_struct_...
File url/mojo/origin_struct_traits.h (right):

https://codereview.chromium.org/1966933002/diff/40001/url/mojo/origin_struct_...
url/mojo/origin_struct_traits.h:29: if (data.is_null()) {
On 2016/05/11 18:04:22, dcheng wrote:
> Why/when would data be null? Why do we consider this to be a deserialization
> success?

Imagine we have a mojo interface like this:

interface Foo {
  Send(url.mojom.Origin? origin);
};

The sender may send a null url.mojom.Origin. That is legitimate.

Based on the current StructTraits definition, url::Origin (the custom-mapped
type) is never converted to null url.mojom.Origin -- because it doesn't have a
IsNull() function defined (I will update our document to talk about things like
this).

So if the sender is using the custom-mapped url::Origin, it should never send a
null.

But other bindings may be able to send null, for example:
- the standard JS/Java bindings;
- or if the blink bindings use the standard generated
url::mojom::blink::OriginPtr.


You may wonder: what if the interface requires to a non-null Origin:
interface Foo {
  Send(url.mojom.Origin origin);  // non-nullable origin
};
In this case, a null value should be rejected, how can we return true here?

That is because a null |origin| will be rejected directly at the validation
stage. It won't reach this Read() method at all. So whenever we see a null in
this method, it is a legitimate input.


The fact that it takes so long to explain this behavior makes me think that it
is probably easier to understand if we take the approach that Ken and I
discussed in the previous replies of this code review.

WDYT? Thanks!

[jam, 2016-05-11 19:24:23]

lgtm

[dcheng, 2016-05-11 21:19:57]

How is code supposed to distinguish between a null Origin and a unique Origin? I
find it a bit surprising that we represent nullability with default-constructed
objects. I would have expected it to be wrapped in something like
std::unique_ptr or even base::Optional.

[Ken Rockot(use gerrit already), 2016-05-11 21:27:40]

On May 11, 2016 2:19 PM, <dcheng@chromium.org> wrote:
>
> How is code supposed to distinguish between a null Origin and a unique
Origin? I
> find it a bit surprising that we represent nullability with
default-constructed
> objects. I would have expected it to be wrapped in something like
> std::unique_ptr or even base::Optional.
>

If a type needs that level of indirection we can typemap it to std::
unique_ptr<T> instead of T. Not everything needs to support nullability
(and some things may have a null-equivalent state), so forcing the
indirection in all cases seems unfortunate. Lots of unnecessary heap
allocation.

> https://codereview.chromium.org/1966933002/

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

[dcheng, 2016-05-11 21:33:26]

On 2016/05/11 at 21:27:40, rockot wrote:
> On May 11, 2016 2:19 PM, <dcheng@chromium.org> wrote:
> >
> > How is code supposed to distinguish between a null Origin and a unique
> Origin? I
> > find it a bit surprising that we represent nullability with
> default-constructed
> > objects. I would have expected it to be wrapped in something like
> > std::unique_ptr or even base::Optional.
> >
> 
> If a type needs that level of indirection we can typemap it to std::
> unique_ptr<T> instead of T. Not everything needs to support nullability
> (and some things may have a null-equivalent state), so forcing the
> indirection in all cases seems unfortunate. Lots of unnecessary heap
> allocation.

Right, it's just not clear (or obvious) to me that a null origin should map to a
unique origin. I might write an interface like this:

interface Foo {
  UpdateOrigin(url.mojom.Origin? origin);
};

And maybe my implementation wants to avoid doing something if the origin is
null. But I can't actually distinguish between the caller not passing an origin
at all and a unique origin. That does not seem good.

The heap allocation shouldn't be imposed on everyone, just things that are
saying that want a nullable argument type. They've opted into complexity so it
seems fair that they will be more complex. Ideally, a non-nullable interface
impl will look something like this:

void FooImpl::UpdateOrigin(const url::Origin& origin) {
  // stuff
}

And a nullable one would look like this:

void FooImpl::UpdateOrigin(std::unique_ptr<url::Origin> origin) {
  if (origin) {
    // do stuff
  }
  // do other stuff
}

> 
> > https://codereview.chromium.org/1966933002/
> 
> -- 
> 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.
>

[Ken Rockot(use gerrit already), 2016-05-11 21:45:32]

On 2016/05/11 at 21:33:26, dcheng wrote:
> On 2016/05/11 at 21:27:40, rockot wrote:
> > On May 11, 2016 2:19 PM, <dcheng@chromium.org> wrote:
> > >
> > > How is code supposed to distinguish between a null Origin and a unique
> > Origin? I
> > > find it a bit surprising that we represent nullability with
> > default-constructed
> > > objects. I would have expected it to be wrapped in something like
> > > std::unique_ptr or even base::Optional.
> > >
> > 
> > If a type needs that level of indirection we can typemap it to std::
> > unique_ptr<T> instead of T. Not everything needs to support nullability
> > (and some things may have a null-equivalent state), so forcing the
> > indirection in all cases seems unfortunate. Lots of unnecessary heap
> > allocation.
> 
> Right, it's just not clear (or obvious) to me that a null origin should map to
a unique origin. I might write an interface like this:
> 
> interface Foo {
>   UpdateOrigin(url.mojom.Origin? origin);
> };
> 
> And maybe my implementation wants to avoid doing something if the origin is
null. But I can't actually distinguish between the caller not passing an origin
at all and a unique origin. That does not seem good.
> 
> The heap allocation shouldn't be imposed on everyone, just things that are
saying that want a nullable argument type. They've opted into complexity so it
seems fair that they will be more complex. Ideally, a non-nullable interface
impl will look something like this:
> 
> void FooImpl::UpdateOrigin(const url::Origin& origin) {
>   // stuff
> }
> 
> And a nullable one would look like this:
> 
> void FooImpl::UpdateOrigin(std::unique_ptr<url::Origin> origin) {
>   if (origin) {
>     // do stuff
>   }
>   // do other stuff
> }
> 
> > 
> > > https://codereview.chromium.org/1966933002/
> > 
> > -- 
> > 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.
> >

I see. Sorry, on phone and misinterpreted context. Will defer to yzshen

[yzshen1, 2016-05-11 23:29:48]

We don't support different type mapping for T and T? (nullable T) at the moment.
Both will be mapped to the same type, just like they are both mapped to the same
mojo::StructPtr<T> type when custom-typemapping is not specified.

The current assumption is: if there is no sensible way to convert a null Foo to
a CustomFoo instance, the user shouldn't typemap Foo to CustomFoo. (It is not a
problem introduced by this CL. This is always the assumption from the beginning.
This CL just avoids the crash on receiving a null value.)

Maybe supporting different type mapping for T and T? is a feature we can
consider. It certainly will complicate things further. I will need to think more
about it and get back to you later.

For this particular case, mapping null origin to unique origin seems acceptable,
origin.h says things like:
- Invalid and non-standard GURLs are parsed as unique origins.
- Unique origins all serialize to the string "null".

WDYT? Thanks!

[dcheng, 2016-05-11 23:35:30]

On 2016/05/11 at 23:29:48, yzshen wrote:
> We don't support different type mapping for T and T? (nullable T) at the
moment. Both will be mapped to the same type, just like they are both mapped to
the same mojo::StructPtr<T> type when custom-typemapping is not specified.

I think this part should be implicit in the generated bindings. For T, we keep
the existing behavior. For T?, we automatically wrap in std::unique_ptr<T>. If T
is not null, then we param.reset(new T) and delegate to the already-existing
mojo::StructPtr<T> to unpack into the heap-allocated T. Otherwise, param remains
null.

> 
> The current assumption is: if there is no sensible way to convert a null Foo
to a CustomFoo instance, the user shouldn't typemap Foo to CustomFoo. (It is not
a problem introduced by this CL. This is always the assumption from the
beginning. This CL just avoids the crash on receiving a null value.)

Whether something has a typemap or not and whether it's nullable seem
orthogonal. How is a nullable int represented today?

> 
> Maybe supporting different type mapping for T and T? is a feature we can
consider. It certainly will complicate things further. I will need to think more
about it and get back to you later.
> 
> For this particular case, mapping null origin to unique origin seems
acceptable, origin.h says things like:
> - Invalid and non-standard GURLs are parsed as unique origins.
> - Unique origins all serialize to the string "null".
> 
> WDYT? Thanks!

I still think it's confusing: there's a real difference between a "null" origin
on the web and wanting to logically pass no origin at all.

[Ken Rockot(use gerrit already), 2016-05-11 23:42:46]

On 2016/05/11 at 23:35:30, dcheng wrote:
> On 2016/05/11 at 23:29:48, yzshen wrote:
> > We don't support different type mapping for T and T? (nullable T) at the
moment. Both will be mapped to the same type, just like they are both mapped to
the same mojo::StructPtr<T> type when custom-typemapping is not specified.
> 
> I think this part should be implicit in the generated bindings. For T, we keep
the existing behavior. For T?, we automatically wrap in std::unique_ptr<T>. If T
is not null, then we param.reset(new T) and delegate to the already-existing
mojo::StructPtr<T> to unpack into the heap-allocated T. Otherwise, param remains
null.

I would much rather leave the decision up to individual type mappings than force
T? -> std::unique_ptr<T> on everyone. I don't understand how forcing this on
developers is an improvement. Especially since some typemaps will want to
explicitly always map to std::unique_ptr<T> (e.g. ui::Event wants polymorphic
deserialization). In that case an optional event field would be
std::unique_ptr<std::unique_ptr<T>>...

> 
> > 
> > The current assumption is: if there is no sensible way to convert a null Foo
to a CustomFoo instance, the user shouldn't typemap Foo to CustomFoo. (It is not
a problem introduced by this CL. This is always the assumption from the
beginning. This CL just avoids the crash on receiving a null value.)
> 
> Whether something has a typemap or not and whether it's nullable seem
orthogonal. How is a nullable int represented today?
> 
> > 
> > Maybe supporting different type mapping for T and T? is a feature we can
consider. It certainly will complicate things further. I will need to think more
about it and get back to you later.
> > 
> > For this particular case, mapping null origin to unique origin seems
acceptable, origin.h says things like:
> > - Invalid and non-standard GURLs are parsed as unique origins.
> > - Unique origins all serialize to the string "null".
> > 
> > WDYT? Thanks!
> 
> I still think it's confusing: there's a real difference between a "null"
origin on the web and wanting to logically pass no origin at all.

[Ken Rockot(use gerrit already), 2016-05-11 23:47:09]

On 2016/05/11 at 23:42:46, Ken Rockot wrote:
> On 2016/05/11 at 23:35:30, dcheng wrote:
> > On 2016/05/11 at 23:29:48, yzshen wrote:
> > > We don't support different type mapping for T and T? (nullable T) at the
moment. Both will be mapped to the same type, just like they are both mapped to
the same mojo::StructPtr<T> type when custom-typemapping is not specified.
> > 
> > I think this part should be implicit in the generated bindings. For T, we
keep the existing behavior. For T?, we automatically wrap in std::unique_ptr<T>.
If T is not null, then we param.reset(new T) and delegate to the
already-existing mojo::StructPtr<T> to unpack into the heap-allocated T.
Otherwise, param remains null.
> 
> I would much rather leave the decision up to individual type mappings than
force T? -> std::unique_ptr<T> on everyone. I don't understand how forcing this
on developers is an improvement. Especially since some typemaps will want to
explicitly always map to std::unique_ptr<T> (e.g. ui::Event wants polymorphic
deserialization). In that case an optional event field would be
std::unique_ptr<std::unique_ptr<T>>...

Doh. Sorry, after some more thought I have convinced myself that you're probably
right. The ui::Event use case is probably an exceptional one and it's not worth
optimizing for. In the worst case, we can always add a typemap option later
which opts out of T? -> unique_ptr wrapping.

> 
> > 
> > > 
> > > The current assumption is: if there is no sensible way to convert a null
Foo to a CustomFoo instance, the user shouldn't typemap Foo to CustomFoo. (It is
not a problem introduced by this CL. This is always the assumption from the
beginning. This CL just avoids the crash on receiving a null value.)
> > 
> > Whether something has a typemap or not and whether it's nullable seem
orthogonal. How is a nullable int represented today?
> > 
> > > 
> > > Maybe supporting different type mapping for T and T? is a feature we can
consider. It certainly will complicate things further. I will need to think more
about it and get back to you later.
> > > 
> > > For this particular case, mapping null origin to unique origin seems
acceptable, origin.h says things like:
> > > - Invalid and non-standard GURLs are parsed as unique origins.
> > > - Unique origins all serialize to the string "null".
> > > 
> > > WDYT? Thanks!
> > 
> > I still think it's confusing: there's a real difference between a "null"
origin on the web and wanting to logically pass no origin at all.

[yzshen1, 2016-05-12 00:06:01]

On 2016/05/11 23:35:30, dcheng wrote:
> On 2016/05/11 at 23:29:48, yzshen wrote:
> > We don't support different type mapping for T and T? (nullable T) at the
> moment. Both will be mapped to the same type, just like they are both mapped
to
> the same mojo::StructPtr<T> type when custom-typemapping is not specified.
> 
> I think this part should be implicit in the generated bindings. For T, we keep
> the existing behavior. For T?, we automatically wrap in std::unique_ptr<T>. If
T
> is not null, then we param.reset(new T) and delegate to the already-existing
> mojo::StructPtr<T> to unpack into the heap-allocated T. Otherwise, param
remains
> null.

Very likely people want more control over how nullable should be handled. For
example,
- std::unique_ptr<T>;
- base::Optional<T>;
- the same type if T itself supports null state (such as WTF::String and all
existing mojo generated standard struct/array/map/string/unions. Exactly the
same typemapping logic is used to support those "built-in" types.)


> 
> > 
> > The current assumption is: if there is no sensible way to convert a null Foo
> to a CustomFoo instance, the user shouldn't typemap Foo to CustomFoo. (It is
not
> a problem introduced by this CL. This is always the assumption from the
> beginning. This CL just avoids the crash on receiving a null value.)
> 
> Whether something has a typemap or not and whether it's nullable seem
> orthogonal. How is a nullable int represented today?

I feel that they are related. The custom type needs to be taken into account
when we consider how nullable should be represented. (Please see my example
above.)
There is no way to express a nullable int. nullable is a concept for only
objects and handles. That is why it wasn't called "optional". :)

> 
> > 
> > Maybe supporting different type mapping for T and T? is a feature we can
> consider. It certainly will complicate things further. I will need to think
more
> about it and get back to you later.
> > 
> > For this particular case, mapping null origin to unique origin seems
> acceptable, origin.h says things like:
> > - Invalid and non-standard GURLs are parsed as unique origins.
> > - Unique origins all serialize to the string "null".
> > 
> > WDYT? Thanks!
> 
> I still think it's confusing: there's a real difference between a "null"
origin
> on the web and wanting to logically pass no origin at all.

I can see your point. I will need to think more about it.

As I said, this CL doesn't introduce the problem. It avoids a possible crash at
the receiver end if some sender does send a null value.
Do you think we can separate it from solving the issue we discussed above? I
feel that it would be nice if I can do that separately because it will be a
pretty big feature to consider.

[dcheng, 2016-05-12 00:09:56]

On 2016/05/12 at 00:06:01, yzshen wrote:
> On 2016/05/11 23:35:30, dcheng wrote:
> > On 2016/05/11 at 23:29:48, yzshen wrote:
> > > We don't support different type mapping for T and T? (nullable T) at the
> > moment. Both will be mapped to the same type, just like they are both mapped
to
> > the same mojo::StructPtr<T> type when custom-typemapping is not specified.
> > 
> > I think this part should be implicit in the generated bindings. For T, we
keep
> > the existing behavior. For T?, we automatically wrap in std::unique_ptr<T>.
If T
> > is not null, then we param.reset(new T) and delegate to the already-existing
> > mojo::StructPtr<T> to unpack into the heap-allocated T. Otherwise, param
remains
> > null.
> 
> Very likely people want more control over how nullable should be handled. For
example,
> - std::unique_ptr<T>;
> - base::Optional<T>;
> - the same type if T itself supports null state (such as WTF::String and all
existing mojo generated standard struct/array/map/string/unions. Exactly the
same typemapping logic is used to support those "built-in" types.)
> 
> 
> > 
> > > 
> > > The current assumption is: if there is no sensible way to convert a null
Foo
> > to a CustomFoo instance, the user shouldn't typemap Foo to CustomFoo. (It is
not
> > a problem introduced by this CL. This is always the assumption from the
> > beginning. This CL just avoids the crash on receiving a null value.)
> > 
> > Whether something has a typemap or not and whether it's nullable seem
> > orthogonal. How is a nullable int represented today?
> 
> I feel that they are related. The custom type needs to be taken into account
when we consider how nullable should be represented. (Please see my example
above.)
> There is no way to express a nullable int. nullable is a concept for only
objects and handles. That is why it wasn't called "optional". :)

OK. Along with the WTF::String example, I'm now convinced.

> 
> > 
> > > 
> > > Maybe supporting different type mapping for T and T? is a feature we can
> > consider. It certainly will complicate things further. I will need to think
more
> > about it and get back to you later.
> > > 
> > > For this particular case, mapping null origin to unique origin seems
> > acceptable, origin.h says things like:
> > > - Invalid and non-standard GURLs are parsed as unique origins.
> > > - Unique origins all serialize to the string "null".
> > > 
> > > WDYT? Thanks!
> > 
> > I still think it's confusing: there's a real difference between a "null"
origin
> > on the web and wanting to logically pass no origin at all.
> 
> I can see your point. I will need to think more about it.
> 
> As I said, this CL doesn't introduce the problem. It avoids a possible crash
at the receiver end if some sender does send a null value.
> Do you think we can separate it from solving the issue we discussed above? I
feel that it would be nice if I can do that separately because it will be a
pretty big feature to consider.

I would say we just shouldn't handle it, so that people who try to make things
nullable that logically aren't will just crash =P

[dcheng, 2016-05-12 03:01:23]

On 2016/05/12 at 00:09:56, dcheng wrote:
> On 2016/05/12 at 00:06:01, yzshen wrote:
> > On 2016/05/11 23:35:30, dcheng wrote:
> > > On 2016/05/11 at 23:29:48, yzshen wrote:
> > > > We don't support different type mapping for T and T? (nullable T) at the
> > > moment. Both will be mapped to the same type, just like they are both
mapped to
> > > the same mojo::StructPtr<T> type when custom-typemapping is not specified.
> > > 
> > > I think this part should be implicit in the generated bindings. For T, we
keep
> > > the existing behavior. For T?, we automatically wrap in
std::unique_ptr<T>. If T
> > > is not null, then we param.reset(new T) and delegate to the
already-existing
> > > mojo::StructPtr<T> to unpack into the heap-allocated T. Otherwise, param
remains
> > > null.
> > 
> > Very likely people want more control over how nullable should be handled.
For example,
> > - std::unique_ptr<T>;
> > - base::Optional<T>;
> > - the same type if T itself supports null state (such as WTF::String and all
existing mojo generated standard struct/array/map/string/unions. Exactly the
same typemapping logic is used to support those "built-in" types.)
> > 
> > 
> > > 
> > > > 
> > > > The current assumption is: if there is no sensible way to convert a null
Foo
> > > to a CustomFoo instance, the user shouldn't typemap Foo to CustomFoo. (It
is not
> > > a problem introduced by this CL. This is always the assumption from the
> > > beginning. This CL just avoids the crash on receiving a null value.)
> > > 
> > > Whether something has a typemap or not and whether it's nullable seem
> > > orthogonal. How is a nullable int represented today?
> > 
> > I feel that they are related. The custom type needs to be taken into account
when we consider how nullable should be represented. (Please see my example
above.)
> > There is no way to express a nullable int. nullable is a concept for only
objects and handles. That is why it wasn't called "optional". :)
> 
> OK. Along with the WTF::String example, I'm now convinced.
> 
> > 
> > > 
> > > > 
> > > > Maybe supporting different type mapping for T and T? is a feature we can
> > > consider. It certainly will complicate things further. I will need to
think more
> > > about it and get back to you later.
> > > > 
> > > > For this particular case, mapping null origin to unique origin seems
> > > acceptable, origin.h says things like:
> > > > - Invalid and non-standard GURLs are parsed as unique origins.
> > > > - Unique origins all serialize to the string "null".
> > > > 
> > > > WDYT? Thanks!
> > > 
> > > I still think it's confusing: there's a real difference between a "null"
origin
> > > on the web and wanting to logically pass no origin at all.
> > 
> > I can see your point. I will need to think more about it.
> > 
> > As I said, this CL doesn't introduce the problem. It avoids a possible crash
at the receiver end if some sender does send a null value.
> > Do you think we can separate it from solving the issue we discussed above? I
feel that it would be nice if I can do that separately because it will be a
pretty big feature to consider.
> 
> I would say we just shouldn't handle it, so that people who try to make things
nullable that logically aren't will just crash =P

Another idea:
- If you have a T?, bindings call StructTraits::ReadNullable(), which could have
a null data view.
- If you have a T, bindings called StructTraits::Read(), which will never have a
null data view, using some SFINAE magic to fall back to ReadNullable().

[yzshen1, 2016-05-12 05:55:49]

On 2016/05/12 03:01:23, dcheng wrote:
> On 2016/05/12 at 00:09:56, dcheng wrote:
> > On 2016/05/12 at 00:06:01, yzshen wrote:
> > > On 2016/05/11 23:35:30, dcheng wrote:
> > > > On 2016/05/11 at 23:29:48, yzshen wrote:
> > > > > We don't support different type mapping for T and T? (nullable T) at
the
> > > > moment. Both will be mapped to the same type, just like they are both
> mapped to
> > > > the same mojo::StructPtr<T> type when custom-typemapping is not
specified.
> > > > 
> > > > I think this part should be implicit in the generated bindings. For T,
we
> keep
> > > > the existing behavior. For T?, we automatically wrap in
> std::unique_ptr<T>. If T
> > > > is not null, then we param.reset(new T) and delegate to the
> already-existing
> > > > mojo::StructPtr<T> to unpack into the heap-allocated T. Otherwise, param
> remains
> > > > null.
> > > 
> > > Very likely people want more control over how nullable should be handled.
> For example,
> > > - std::unique_ptr<T>;
> > > - base::Optional<T>;
> > > - the same type if T itself supports null state (such as WTF::String and
all
> existing mojo generated standard struct/array/map/string/unions. Exactly the
> same typemapping logic is used to support those "built-in" types.)
> > > 
> > > 
> > > > 
> > > > > 
> > > > > The current assumption is: if there is no sensible way to convert a
null
> Foo
> > > > to a CustomFoo instance, the user shouldn't typemap Foo to CustomFoo.
(It
> is not
> > > > a problem introduced by this CL. This is always the assumption from the
> > > > beginning. This CL just avoids the crash on receiving a null value.)
> > > > 
> > > > Whether something has a typemap or not and whether it's nullable seem
> > > > orthogonal. How is a nullable int represented today?
> > > 
> > > I feel that they are related. The custom type needs to be taken into
account
> when we consider how nullable should be represented. (Please see my example
> above.)
> > > There is no way to express a nullable int. nullable is a concept for only
> objects and handles. That is why it wasn't called "optional". :)
> > 
> > OK. Along with the WTF::String example, I'm now convinced.
> > 
> > > 
> > > > 
> > > > > 
> > > > > Maybe supporting different type mapping for T and T? is a feature we
can
> > > > consider. It certainly will complicate things further. I will need to
> think more
> > > > about it and get back to you later.
> > > > > 
> > > > > For this particular case, mapping null origin to unique origin seems
> > > > acceptable, origin.h says things like:
> > > > > - Invalid and non-standard GURLs are parsed as unique origins.
> > > > > - Unique origins all serialize to the string "null".
> > > > > 
> > > > > WDYT? Thanks!
> > > > 
> > > > I still think it's confusing: there's a real difference between a "null"
> origin
> > > > on the web and wanting to logically pass no origin at all.
> > > 
> > > I can see your point. I will need to think more about it.
> > > 
> > > As I said, this CL doesn't introduce the problem. It avoids a possible
crash
> at the receiver end if some sender does send a null value.
> > > Do you think we can separate it from solving the issue we discussed above?
I
> feel that it would be nice if I can do that separately because it will be a
> pretty big feature to consider.
> > 
> > I would say we just shouldn't handle it, so that people who try to make
things
> nullable that logically aren't will just crash =P

If some custom-mapped type CustomT really doesn't make sense to be converted
from null T data view, we can return false from StructTraits::Read(), which
means deserialization failure and closes the pipe. That seems better than
crashing to me. :) In other words, no allowing null becomes part of the
application-level validation, just like some non-null strings aren't valid
origin and will be rejected.

> 
> Another idea:
> - If you have a T?, bindings call StructTraits::ReadNullable(), which could
have
> a null data view.
> - If you have a T, bindings called StructTraits::Read(), which will never have
a
> null data view, using some SFINAE magic to fall back to ReadNullable().

This is fairly easy to do. (And I am doing a lot of similar things already. :D)
But I think this is not going to solve the problem here, unless I have
misunderstood your idea.
I can choose to call Read() and ReadNullable() for "url::mojom::Origin" and
"url::mojom::Origin?" respectively. As long as the two functions produce the
same output type url::Origin, however, it won't solve the problem. Because I
think what you are worried about is url::Origin is not able to precisely reflect
the meaning of "url::mojom::Origin?", right?

If the answer is "yes", then we have to choose a different result type for
ReadNullable(), e.g., std::unique_ptr<url::Origin>. And that basically means we
need two custom typemapping for the non-nullable and nullable variants.

Let's chat more about this tomorrow. 
Thanks for the input!

[yzshen1, 2016-05-12 23:10:06]

Hi, Daniel.

I have updated the CL according to our discussion. Please take another look.
Thanks!

[dcheng, 2016-05-13 04:12:34]

LGTM with one question.

https://codereview.chromium.org/1966933002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/string_traits_string_piece.h (right):

https://codereview.chromium.org/1966933002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/string_traits_string_piece.h:15: static bool
IsNull(const base::StringPiece& input) {
Usually, StringPiece should be passed by value. Any idea if it makes any codegen
difference here? Since this is a simple inlined template, maybe it's not needed?

[yzshen1, 2016-05-13 04:35:08]

Thanks!

https://codereview.chromium.org/1966933002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/string_traits_string_piece.h (right):

https://codereview.chromium.org/1966933002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/string_traits_string_piece.h:15: static bool
IsNull(const base::StringPiece& input) {
On 2016/05/13 04:12:34, dcheng wrote:
> Usually, StringPiece should be passed by value. Any idea if it makes any
codegen
> difference here? 
It still works if I change this method's signature to
static bool IsNull(base::StringPiece input);
But I think it is good to keep it consistent with other StructTraits, which all
use const& for the IsNull() method. WDYT?

> Since this is a simple inlined template, maybe it's not needed?
I am not quite sure which do you mean:
(1) the IsNull() method is not needed; or
(2) the whole StringTraits for base::StringPiece is not needed.

WRT (1): Yes, IsNull() could be omitted to mean "never null". I slightly prefer
to define either both IsNull() and SetToNull(), or none of them.

WRT (2): This StringTraits is useful. Without it you cannot return
base::StringPiece from a StructTraits field getter; or read a string from a
DataView input data as base::StringPiece directly. 

Line 39 of
https://codereview.chromium.org/1966933002/diff/80001/mojo/public/cpp/binding...

Line 17 of
https://codereview.chromium.org/1966933002/diff/80001/mojo/public/cpp/binding...

(I will definitely update our doc to talk about things like this. :))

[yzshen1, 2016-05-13 04:35:17]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2016-05-13 04:35:17]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1966933002/#ps80001
(title: "typeid() is not allowed :/")

[commit-bot: I haz the power, 2016-05-13 04:35:33]

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

[commit-bot: I haz the power, 2016-05-13 04:42:35]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-13 04:44:18]

Description was changed from

==========
Mojo C++ bindings: switch the existing usage of StructTraits to use the new data
view interface.

BUG=577686
==========

to

==========
Mojo C++ bindings: switch the existing usage of StructTraits to use the new data
view interface.

BUG=577686

Committed: https://crrev.com/2017646cf12650e54c22fab5195dfb225617a009
Cr-Commit-Position: refs/heads/master@{#393448}
==========

[commit-bot: I haz the power, 2016-05-13 04:44:19]

Patchset 5 (id:??) landed as
https://crrev.com/2017646cf12650e54c22fab5195dfb225617a009
Cr-Commit-Position: refs/heads/master@{#393448}

[dcheng, 2016-05-13 06:05:16]

https://codereview.chromium.org/1966933002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/string_traits_string_piece.h (right):

https://codereview.chromium.org/1966933002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/string_traits_string_piece.h:15: static bool
IsNull(const base::StringPiece& input) {
On 2016/05/13 at 04:35:08, yzshen1 wrote:
> On 2016/05/13 04:12:34, dcheng wrote:
> > Usually, StringPiece should be passed by value. Any idea if it makes any
codegen
> > difference here? 
> It still works if I change this method's signature to
> static bool IsNull(base::StringPiece input);
> But I think it is good to keep it consistent with other StructTraits, which
all use const& for the IsNull() method. WDYT?
> 
> > Since this is a simple inlined template, maybe it's not needed?
> I am not quite sure which do you mean:
> (1) the IsNull() method is not needed; or
> (2) the whole StringTraits for base::StringPiece is not needed.
> 
> WRT (1): Yes, IsNull() could be omitted to mean "never null". I slightly
prefer to define either both IsNull() and SetToNull(), or none of them.
> 
> WRT (2): This StringTraits is useful. Without it you cannot return
base::StringPiece from a StructTraits field getter; or read a string from a
DataView input data as base::StringPiece directly. 
> 
> Line 39 of
https://codereview.chromium.org/1966933002/diff/80001/mojo/public/cpp/binding...
> 
> Line 17 of
>
https://codereview.chromium.org/1966933002/diff/80001/mojo/public/cpp/binding...
> 
> (I will definitely update our doc to talk about things like this. :))

To clarify, my comment was only referring to using StringPiece instead of const
StringPiece& for the parameter name. More documentation is always good though =)