Add mojo struct traits for GURL so that it can be sent over mojoms.

Add mojo struct traits for GURL so that it can be sent over mojoms.

The motivation for adding a structtraits for GURL, as opposed to using paramtraits, is that most probably we'll be passing urls to languages other than C++.

BUG=586194
Committed: https://crrev.com/baaeb815d807659fc7fe9b8cba2cc3b4d20018bf
Cr-Commit-Position: refs/heads/master@{#379128}

[jam, 2016-03-03 03:17:25]

Description was changed from

==========
Add mojo struct traits for GURL so that it can be sent over mojoms.

The motivation for adding a struct traits for GURL, as opposed to using
paramtraits, is that most probably we'll be passing urls to languages other than
C++.

BUG=586194
==========

to

==========
Add mojo struct traits for GURL so that it can be sent over mojoms.

The motivation for adding a structtraits for GURL, as opposed to using
paramtraits, is that most probably we'll be passing urls to languages other than
C++.

BUG=586194
==========

[jam, 2016-03-03 05:01:55]

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

[Ken Rockot(use gerrit already), 2016-03-03 05:23:28]

https://codereview.chromium.org/1760643004/diff/40001/url/mojo/url.mojom
File url/mojo/url.mojom (right):

https://codereview.chromium.org/1760643004/diff/40001/url/mojo/url.mojom#newc...
url/mojo/url.mojom:5: module url;
I think this is fine as is, but IMHO we should really be consistent with mojom
namespaces (i know, i know). I also think if we do that, the outer namespace
should match between the mojom type and the typemapped type. So because in this
case GURL is in the global namespace, I would argue this should just be module
mojom.

I would also personally prefer URL over Url, but I see that's against style
guide recommendations...

[Ken Rockot(use gerrit already), 2016-03-03 05:23:33]

lgtm

[jam, 2016-03-03 16:09:03]

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

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

+dcheng as security reviewer
Daniel: this adds a mojo URL type that is really a passthrough for GURL.
url/mojo/url_gurl.h contains the security sensitive code that converts from a
string to GURL. it's copied from the GURL<>chrome ipc code in
content/public/common/common_param_traits.cc

https://codereview.chromium.org/1760643004/diff/40001/url/mojo/url.mojom
File url/mojo/url.mojom (right):

https://codereview.chromium.org/1760643004/diff/40001/url/mojo/url.mojom#newc...
url/mojo/url.mojom:5: module url;
On 2016/03/03 05:23:28, Ken Rockot wrote:
> I think this is fine as is, but IMHO we should really be consistent with mojom
> namespaces (i know, i know). I also think if we do that, the outer namespace
> should match between the mojom type and the typemapped type. So because in
this
> case GURL is in the global namespace, I would argue this should just be module
> mojom.

ah I forgot, added mojom.
GURL is in global namespace because it's before we had the convention of code in
src\foo should be in foo namespace, and no one bothered to update it.

> 
> I would also personally prefer URL over Url, but I see that's against style
> guide recommendations...

ditto

[dcheng, 2016-03-03 16:23:53]

https://codereview.chromium.org/1760643004/diff/60001/url/mojo/url_gurl.h
File url/mojo/url_gurl.h (right):

https://codereview.chromium.org/1760643004/diff/60001/url/mojo/url_gurl.h#new...
url/mojo/url_gurl.h:1: // Copyright 2016 The Chromium Authors. All rights
reserved.
Can we have some sort of naming convention for files that have StructTraits (and
maybe TypeConverters?) like we do for IPC::ParamTraits? url_struct_traits.*
maybe?

https://codereview.chromium.org/1760643004/diff/60001/url/mojo/url_gurl.h#new...
url/mojo/url_gurl.h:13: const size_t kMaxURLChars = 2 * 1024 * 1024;
Nit: Strictly speaking, this should be kMaxUrlChars now =/

https://codereview.chromium.org/1760643004/diff/60001/url/mojo/url_gurl.h#new...
url/mojo/url_gurl.h:17: static base::StringPiece url(const GURL& r) {
Is there documentation on how StructTraits work? How does stuff that deduce the
type know that it needs to call StructTraits<T, U>::url() and not
StructTraits<T, U>::string()? Or am I completely misunderstanding how this
works?

[dcheng, 2016-03-03 16:24:32]

(Also, out of the scope of this immediate CL, but what's the relationship
between type converters and struct traits? Is there any overlap?)

[yzshen1, 2016-03-03 16:32:27]

On 2016/03/03 16:24:32, dcheng wrote:
> (Also, out of the scope of this immediate CL, but what's the relationship
> between type converters and struct traits? Is there any overlap?)

When we get a struct from a message, the bindings convert it from the "wire
format" to "wrapper type".

Custom type mapping (which uses struct traits) basically says "use this custom
wrapper type instead of the standard mojo wrapper type".
Type converter is used to convert "standard mojo wrapper type" to another type. 
It is easier to write type converters, but there is one more conversion in the
middle.

(Maybe we should revisit the design of type converters.)

[Ken Rockot(use gerrit already), 2016-03-03 16:33:53]

There should probably never be overlap. In general, TypeConverters are
inefficient since they take real Mojo structures and convert them to some
other type. StructTraits on the other hand teach the bindings layer how to
serialize or deserialize a Mojo structure's wire format directly from/to an
arbitrary type.

There is no real magic to the type deduction, it's all generated code. i.e.
if you define a struct in mojom with a string field foo, any StructTraits
mapping to that type must provide a static foo(input) that returns a
base::StringPiece.

Likewise you have to explicitly declare the type mappings you want at
bindings generation time. So if you apply the gurl typemap to a mojom
interface message which takes a url.mojom.Url argument, it will generate a
C++ interface signature which takes a const GURL&, and the generated
serialization code will expect to find a complete
mojo::StructTraits<url::mojom::Url, GURL> definition when encoding/decoding
messages


On Thu, Mar 3, 2016 at 8:24 AM, <dcheng@chromium.org> wrote:

> (Also, out of the scope of this immediate CL, but what's the relationship
> between type converters and struct traits? Is there any overlap?)
>
> https://codereview.chromium.org/1760643004/
>

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

[jam, 2016-03-03 21:25:12]

Ken: I think struct traits is something we need proper documentation for,
especially since we'll probably start adding those at a rapid pace soon.

one issue I ran into is that I first had the return type as string instead of
stringpiece, which led to it not working although it compiled. It would be nice
if there was a generated comment that would show a stub struct traits that one
could copy & paste.

https://codereview.chromium.org/1760643004/diff/60001/url/mojo/url_gurl.h
File url/mojo/url_gurl.h (right):

https://codereview.chromium.org/1760643004/diff/60001/url/mojo/url_gurl.h#new...
url/mojo/url_gurl.h:1: // Copyright 2016 The Chromium Authors. All rights
reserved.
On 2016/03/03 16:23:53, dcheng wrote:
> Can we have some sort of naming convention for files that have StructTraits
(and
> maybe TypeConverters?) like we do for IPC::ParamTraits? url_struct_traits.*
> maybe?

sure that seems reasonable, renamed

https://codereview.chromium.org/1760643004/diff/60001/url/mojo/url_gurl.h#new...
url/mojo/url_gurl.h:13: const size_t kMaxURLChars = 2 * 1024 * 1024;
On 2016/03/03 16:23:53, dcheng wrote:
> Nit: Strictly speaking, this should be kMaxUrlChars now =/

Done.

https://codereview.chromium.org/1760643004/diff/60001/url/mojo/url_gurl.h#new...
url/mojo/url_gurl.h:17: static base::StringPiece url(const GURL& r) {
On 2016/03/03 16:23:53, dcheng wrote:
> Is there documentation on how StructTraits work? How does stuff that deduce
the
> type know that it needs to call StructTraits<T, U>::url() and not
> StructTraits<T, U>::string()? Or am I completely misunderstanding how this
> works?

Looks like Ken already replied.

[dcheng, 2016-03-03 21:37:37]

lgtm with comments addressed

https://codereview.chromium.org/1760643004/diff/80001/url/mojo/OWNERS
File url/mojo/OWNERS (right):

https://codereview.chromium.org/1760643004/diff/80001/url/mojo/OWNERS#newcode3
url/mojo/OWNERS:3: per-file url_gurl_struct_traits_unittest.h=set noparent
Change this to per-file *_struct_traits.* (I think it's OK for this rule to not
catch the unit test, since changes to the unit test will probably involve
StructTraits changes)

https://codereview.chromium.org/1760643004/diff/80001/url/mojo/url_gurl_struc...
File url/mojo/url_gurl_struct_traits.h (right):

https://codereview.chromium.org/1760643004/diff/80001/url/mojo/url_gurl_struc...
url/mojo/url_gurl_struct_traits.h:17: static base::StringPiece url(const GURL&
r) {
I don't think I understand how the magic name for this is determined, but that
shouldn't block this review.

https://codereview.chromium.org/1760643004/diff/80001/url/mojo/url_gurl_struc...
File url/mojo/url_gurl_struct_traits_unittest.cc (right):

https://codereview.chromium.org/1760643004/diff/80001/url/mojo/url_gurl_struc...
url/mojo/url_gurl_struct_traits_unittest.cc:16: : binding_(this,
std::move(request)) {
Nit: #include <utility> for std::move

[yzshen1, 2016-03-03 21:47:55]

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

[yzshen1, 2016-03-03 21:47:56]

https://codereview.chromium.org/1760643004/diff/120001/url/mojo/url_gurl_stru...
File url/mojo/url_gurl_struct_traits_unittest.cc (right):

https://codereview.chromium.org/1760643004/diff/120001/url/mojo/url_gurl_stru...
url/mojo/url_gurl_struct_traits_unittest.cc:68: EXPECT_TRUE(proxy->Bounce(input,
&output));
Hmm.. this test case should result in message pipe disconnection because of the
illegal input, right?
In that case, the returning value should be false, instead of returning true and
receive a connection error later.

I will fix the sync call behavior. (No need to block this CL on that, I will
update this test after the fix.)
Sorry. :)

[jam, 2016-03-03 21:48:56]

The CQ bit was checked by jam@chromium.org

[jam, 2016-03-03 21:48:57]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1760643004/#ps120001
(title: "review comments")

[jam, 2016-03-03 21:49:47]

https://codereview.chromium.org/1760643004/diff/80001/url/mojo/OWNERS
File url/mojo/OWNERS (right):

https://codereview.chromium.org/1760643004/diff/80001/url/mojo/OWNERS#newcode3
url/mojo/OWNERS:3: per-file url_gurl_struct_traits_unittest.h=set noparent
On 2016/03/03 21:37:36, dcheng wrote:
> Change this to per-file *_struct_traits.* (I think it's OK for this rule to
not
> catch the unit test, since changes to the unit test will probably involve
> StructTraits changes)

yep, i realized that in a self review and was uploading a fixed version :)

https://codereview.chromium.org/1760643004/diff/80001/url/mojo/url_gurl_struc...
File url/mojo/url_gurl_struct_traits.h (right):

https://codereview.chromium.org/1760643004/diff/80001/url/mojo/url_gurl_struc...
url/mojo/url_gurl_struct_traits.h:17: static base::StringPiece url(const GURL&
r) {
On 2016/03/03 21:37:36, dcheng wrote:
> I don't think I understand how the magic name for this is determined, but that
> shouldn't block this review.

agree we need better documentation

https://codereview.chromium.org/1760643004/diff/80001/url/mojo/url_gurl_struc...
File url/mojo/url_gurl_struct_traits_unittest.cc (right):

https://codereview.chromium.org/1760643004/diff/80001/url/mojo/url_gurl_struc...
url/mojo/url_gurl_struct_traits_unittest.cc:16: : binding_(this,
std::move(request)) {
On 2016/03/03 21:37:36, dcheng wrote:
> Nit: #include <utility> for std::move

Done.

https://codereview.chromium.org/1760643004/diff/120001/url/mojo/url_gurl_stru...
File url/mojo/url_gurl_struct_traits_unittest.cc (right):

https://codereview.chromium.org/1760643004/diff/120001/url/mojo/url_gurl_stru...
url/mojo/url_gurl_struct_traits_unittest.cc:68: EXPECT_TRUE(proxy->Bounce(input,
&output));
On 2016/03/03 21:47:56, yzshen1 wrote:
> Hmm.. this test case should result in message pipe disconnection because of
the
> illegal input, right?
> In that case, the returning value should be false, instead of returning true
and
> receive a connection error later.
> 
> I will fix the sync call behavior. (No need to block this CL on that, I will
> update this test after the fix.)
> Sorry. :)

thanks for noticing that, yes that'd be great.

[commit-bot: I haz the power, 2016-03-03 21:49:52]

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

[commit-bot: I haz the power, 2016-03-03 23:12:05]

Description was changed from

==========
Add mojo struct traits for GURL so that it can be sent over mojoms.

The motivation for adding a structtraits for GURL, as opposed to using
paramtraits, is that most probably we'll be passing urls to languages other than
C++.

BUG=586194
==========

to

==========
Add mojo struct traits for GURL so that it can be sent over mojoms.

The motivation for adding a structtraits for GURL, as opposed to using
paramtraits, is that most probably we'll be passing urls to languages other than
C++.

BUG=586194
==========

[commit-bot: I haz the power, 2016-03-03 23:12:06]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-03 23:13:31]

Description was changed from

==========
Add mojo struct traits for GURL so that it can be sent over mojoms.

The motivation for adding a structtraits for GURL, as opposed to using
paramtraits, is that most probably we'll be passing urls to languages other than
C++.

BUG=586194
==========

to

==========
Add mojo struct traits for GURL so that it can be sent over mojoms.

The motivation for adding a structtraits for GURL, as opposed to using
paramtraits, is that most probably we'll be passing urls to languages other than
C++.

BUG=586194

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

[commit-bot: I haz the power, 2016-03-03 23:13:32]

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

[yzshen1, 2016-03-09 00:01:52]

https://codereview.chromium.org/1760643004/diff/120001/url/mojo/url_gurl_stru...
File url/mojo/url_gurl_struct_traits_unittest.cc (right):

https://codereview.chromium.org/1760643004/diff/120001/url/mojo/url_gurl_stru...
url/mojo/url_gurl_struct_traits_unittest.cc:68: EXPECT_TRUE(proxy->Bounce(input,
&output));
On 2016/03/03 21:49:47, jam wrote:
> On 2016/03/03 21:47:56, yzshen1 wrote:
> > Hmm.. this test case should result in message pipe disconnection because of
> the
> > illegal input, right?
> > In that case, the returning value should be false, instead of returning true
> and
> > receive a connection error later.
> > 
> > I will fix the sync call behavior. (No need to block this CL on that, I will
> > update this test after the fix.)
> > Sorry. :)
> 
> thanks for noticing that, yes that'd be great.

I have looked into this issue and found that the sync call returns success
because the invalid input doesn't reach the the message pipe. Before sending,
the bindings call StructTraits::url() to serialize the input. This method
returns an empty string for any invalid input. Therefore, the data passed over
the message pipe is actually valid. (So the sync call logic is working as
expected in this case.)

It depends on which behavior we think that makes more sense. The current
StructTraits will automatically serialize invalid input as empty string. If we
want to disconnect the message pipe on invalid input instead, we need to remove
the validation code from StructTraits::url().