Implement URL and Origin typemaps/struct traits for blink.

Implement URL and Origin typemaps/struct traits for blink.

Committed: https://crrev.com/8919fe48df2ae876444abfd8068bcb817c63a25e
Cr-Commit-Position: refs/heads/master@{#396952}

[Marijn Kruisselbrink, 2016-05-25 01:03:39]

mek@chromium.org changed reviewers:
+ esprehn@chromium.org, rockot@chromium.org, yzshen@chromium.org

[Marijn Kruisselbrink, 2016-05-25 01:03:39]

Pretty much just copied from the content-side typemaps/struct traits for urls
and origins.
(hopefully I'm not duplicating anybody else's work)

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

generally lg % namespace issue (inline) and a gyp bikeshed.

I think it's going to be weird no matter how we address generating blink
bindings for something in //url. The options seem to be the thing you've done
here (define the rule in gyp near the mojom), or to define the rule in
third_party/WebKit gyp.

More generally I see the latter policy as "define bindings rules at the lowest
layer they can be used." Existing cases are only for testing, so it makes sense
that the blink gyp rules live near the test mojom. The only consumers of blink
Url bindings are in third_party/WebKit, so I'd sort of prefer we move the rule
to somewhere in there.

Having said all that, I don't feel strongly about this, not least of all because
gyp will disappear someday soon. WDYT?

https://codereview.chromium.org/2000253006/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/mojo/KURLStructTraits.h (right):

https://codereview.chromium.org/2000253006/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mojo/KURLStructTraits.h:15: struct
StructTraits<url::mojom::blink::Url, blink::KURL> {
You will need to qualify blink::KURL here for now. This is unfortunate, and it's
a result of the fact that we generate some bindings (for internal control
messages) in the mojo:: namespace. The blink variant of those introduces a
mojo::blink namespace.

We can move them to mojo::internal to avoid stuff like this in the future.

[Marijn Kruisselbrink, 2016-05-25 16:13:02]

On 2016/05/25 at 15:09:20, rockot wrote:
> generally lg % namespace issue (inline) and a gyp bikeshed.
> 
> I think it's going to be weird no matter how we address generating blink
bindings for something in //url. The options seem to be the thing you've done
here (define the rule in gyp near the mojom), or to define the rule in
third_party/WebKit gyp.
> 
> More generally I see the latter policy as "define bindings rules at the lowest
layer they can be used." Existing cases are only for testing, so it makes sense
that the blink gyp rules live near the test mojom. The only consumers of blink
Url bindings are in third_party/WebKit, so I'd sort of prefer we move the rule
to somewhere in there.
> 
> Having said all that, I don't feel strongly about this, not least of all
because gyp will disappear someday soon. WDYT?

On one hand it indeed seems more sensible to have the gyp rules for blink Url
bindings in //third_party/WebKit. But the GN rules for these same bindings will
still be auto-generated in //url anyway, so moving the gyp rules would be
somewhat inconsistent. And yeah, hopefully gyp is going to be gone in the near
future anyway, so it doesn't matter too much.

>
https://codereview.chromium.org/2000253006/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/mojo/KURLStructTraits.h (right):
> 
>
https://codereview.chromium.org/2000253006/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/mojo/KURLStructTraits.h:15: struct
StructTraits<url::mojom::blink::Url, blink::KURL> {
> You will need to qualify blink::KURL here for now. This is unfortunate, and
it's a result of the fact that we generate some bindings (for internal control
messages) in the mojo:: namespace. The blink variant of those introduces a
mojo::blink namespace.

Fixed.

[Ken Rockot(use gerrit already), 2016-05-25 16:14:39]

lgtm

On 2016/05/25 at 16:13:02, mek wrote:
> On 2016/05/25 at 15:09:20, rockot wrote:
> > generally lg % namespace issue (inline) and a gyp bikeshed.
> > 
> > I think it's going to be weird no matter how we address generating blink
bindings for something in //url. The options seem to be the thing you've done
here (define the rule in gyp near the mojom), or to define the rule in
third_party/WebKit gyp.
> > 
> > More generally I see the latter policy as "define bindings rules at the
lowest layer they can be used." Existing cases are only for testing, so it makes
sense that the blink gyp rules live near the test mojom. The only consumers of
blink Url bindings are in third_party/WebKit, so I'd sort of prefer we move the
rule to somewhere in there.
> > 
> > Having said all that, I don't feel strongly about this, not least of all
because gyp will disappear someday soon. WDYT?
> 
> On one hand it indeed seems more sensible to have the gyp rules for blink Url
bindings in //third_party/WebKit. But the GN rules for these same bindings will
still be auto-generated in //url anyway, so moving the gyp rules would be
somewhat inconsistent. And yeah, hopefully gyp is going to be gone in the near
future anyway, so it doesn't matter too much.
> 
> >
https://codereview.chromium.org/2000253006/diff/1/third_party/WebKit/Source/p...
> > File third_party/WebKit/Source/platform/mojo/KURLStructTraits.h (right):
> > 
> >
https://codereview.chromium.org/2000253006/diff/1/third_party/WebKit/Source/p...
> > third_party/WebKit/Source/platform/mojo/KURLStructTraits.h:15: struct
StructTraits<url::mojom::blink::Url, blink::KURL> {
> > You will need to qualify blink::KURL here for now. This is unfortunate, and
it's a result of the fact that we generate some bindings (for internal control
messages) in the mojo:: namespace. The blink variant of those introduces a
mojo::blink namespace.
> 
> Fixed.

[yzshen1, 2016-05-25 16:43:15]

https://codereview.chromium.org/2000253006/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/mojo/KURLStructTraits.h (right):

https://codereview.chromium.org/2000253006/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mojo/KURLStructTraits.h:15: struct
StructTraits<url::mojom::blink::Url, blink::KURL> {
On 2016/05/25 15:09:20, Ken Rockot wrote:
> You will need to qualify blink::KURL here for now. This is unfortunate, and
it's
> a result of the fact that we generate some bindings (for internal control
> messages) in the mojo:: namespace. The blink variant of those introduces a
> mojo::blink namespace.
> 
> We can move them to mojo::internal to avoid stuff like this in the future.

Ah, that seems unfortunate. Moving those things into mojo::internal will require
fixing some bindings code/template, because we already have mojo::internal::
defined, too. The easiest way is to move them into something like
mojo.interface_control (we already have mojo.pipe_control for some other control
messages).

https://codereview.chromium.org/2000253006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/BUILD.gn (right):

https://codereview.chromium.org/2000253006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/BUILD.gn:307:
"//url/mojo:url_mojom_origin_blink_cpp_sources",
These seem sad.
Hmm... Wish we had a way to avoid these.

https://codereview.chromium.org/2000253006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mojo/KURLSecurityOriginTest.cpp (right):

https://codereview.chromium.org/2000253006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mojo/KURLSecurityOriginTest.cpp:17: explicit
UrlTestImpl(mojo::InterfaceRequest<url::mojom::blink::UrlTest> request)
nit: in case you didn't know, you could use url::mojom::blink::UrlTestRequest as
a shorter name

https://codereview.chromium.org/2000253006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mojo/SecurityOriginStructTraits.h
(right):

https://codereview.chromium.org/2000253006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mojo/SecurityOriginStructTraits.h:44: // the
values provided to 'UnsafelyCreateOriginWithoutNormalization' were
Is this comment up to date? I didn't see
'UnsafelyCreateOriginWithoutNormalization' used in blink::SecurityOrigin.

[Marijn Kruisselbrink, 2016-05-25 16:55:21]

https://codereview.chromium.org/2000253006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mojo/KURLSecurityOriginTest.cpp (right):

https://codereview.chromium.org/2000253006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mojo/KURLSecurityOriginTest.cpp:17: explicit
UrlTestImpl(mojo::InterfaceRequest<url::mojom::blink::UrlTest> request)
On 2016/05/25 at 16:43:15, yzshen1 wrote:
> nit: in case you didn't know, you could use url::mojom::blink::UrlTestRequest
as a shorter name

Good point. I just copied this from the existing test in //url/mojo, but using
the shorter name might make sense.

https://codereview.chromium.org/2000253006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mojo/SecurityOriginStructTraits.h
(right):

https://codereview.chromium.org/2000253006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mojo/SecurityOriginStructTraits.h:44: // the
values provided to 'UnsafelyCreateOriginWithoutNormalization' were
On 2016/05/25 at 16:43:15, yzshen1 wrote:
> Is this comment up to date? I didn't see
'UnsafelyCreateOriginWithoutNormalization' used in blink::SecurityOrigin.

Fixed

[yzshen1, 2016-05-25 17:00:00]

On 2016/05/25 16:55:21, Marijn Kruisselbrink wrote:
>
https://codereview.chromium.org/2000253006/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/mojo/KURLSecurityOriginTest.cpp
(right):
> 
>
https://codereview.chromium.org/2000253006/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/mojo/KURLSecurityOriginTest.cpp:17:
explicit
> UrlTestImpl(mojo::InterfaceRequest<url::mojom::blink::UrlTest> request)
> On 2016/05/25 at 16:43:15, yzshen1 wrote:
> > nit: in case you didn't know, you could use
url::mojom::blink::UrlTestRequest
> as a shorter name
> 
> Good point. I just copied this from the existing test in //url/mojo, but using
> the shorter name might make sense.
> 
>
https://codereview.chromium.org/2000253006/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/mojo/SecurityOriginStructTraits.h
> (right):
> 
>
https://codereview.chromium.org/2000253006/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/mojo/SecurityOriginStructTraits.h:44: //
the
> values provided to 'UnsafelyCreateOriginWithoutNormalization' were
> On 2016/05/25 at 16:43:15, yzshen1 wrote:
> > Is this comment up to date? I didn't see
> 'UnsafelyCreateOriginWithoutNormalization' used in blink::SecurityOrigin.
> 
> Fixed

LGTM (you also need a security OWNER to review this change)

[esprehn, 2016-05-25 17:06:27]

https://codereview.chromium.org/2000253006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mojo/KURLSecurityOriginTest.cpp (right):

https://codereview.chromium.org/2000253006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mojo/KURLSecurityOriginTest.cpp:15: class
UrlTestImpl : public url::mojom::blink::UrlTest {
don't indent nested namespace functions

https://codereview.chromium.org/2000253006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mojo/KURLSecurityOriginTest.cpp:17: explicit
UrlTestImpl(url::mojom::blink::UrlTestRequest request)
Where are things like the generated TestRequest stuff documented?

https://codereview.chromium.org/2000253006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mojo/KURLStructTraits.h (right):

https://codereview.chromium.org/2000253006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mojo/KURLStructTraits.h:16: static
WTF::String url(const ::blink::KURL& r)
what calls this? Nothing in this file seems to use it. This matches some
variable defined in the mojom? Which one?

Also don't abbreviate in blink. what is |r| ?

https://codereview.chromium.org/2000253006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mojo/SecurityOriginStructTraits.h
(right):

https://codereview.chromium.org/2000253006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mojo/SecurityOriginStructTraits.h:15: static
WTF::String scheme(const RefPtr<::blink::SecurityOrigin>& r)
ditto, don't use single letter vars

https://codereview.chromium.org/2000253006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mojo/SecurityOriginStructTraits.h:36:
WTF::String scheme, host;
one line per var

https://codereview.chromium.org/2000253006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mojo/SecurityOriginStructTraits.h:45: if
(!data.unique() && (*out)->isUnique())
does the contract for these say it's okay to still assign a value to |out| even
when returning false? Where are these documented? :)

[Marijn Kruisselbrink, 2016-05-25 17:35:08]

https://codereview.chromium.org/2000253006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mojo/KURLSecurityOriginTest.cpp (right):

https://codereview.chromium.org/2000253006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mojo/KURLSecurityOriginTest.cpp:15: class
UrlTestImpl : public url::mojom::blink::UrlTest {
On 2016/05/25 at 17:06:27, esprehn wrote:
> don't indent nested namespace functions

I never quite understood what "nested namespaces with the same scope" meant in
the blink style guide ("The contents of an outermost namespace block (and any
nested namespaces with the same scope) should not be indented. The contents of
other nested namespaces should be indented."), but assumed that it meant to
differentiate between "namespace a { namespace b {} class foo {}; }" and
"namespace a { namespace b { class foo {}; } }" where in the first one things
inside b should be indented, while in the second one it shouldn't. Maybe my
understanding of the style guide is wrong though (although this does seem to
match what the clang format rules for blink do).

Anyway, fixed.

https://codereview.chromium.org/2000253006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mojo/KURLSecurityOriginTest.cpp:17: explicit
UrlTestImpl(url::mojom::blink::UrlTestRequest request)
On 2016/05/25 at 17:06:27, esprehn wrote:
> Where are things like the generated TestRequest stuff documented?

https://www.chromium.org/developers/design-documents/mojo#TOC-Bindings seems to
document at least some of this stuff (and in particular mentions the generated
FooRequest typedef).

https://codereview.chromium.org/2000253006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mojo/KURLStructTraits.h (right):

https://codereview.chromium.org/2000253006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mojo/KURLStructTraits.h:16: static
WTF::String url(const ::blink::KURL& r)
On 2016/05/25 at 17:06:27, esprehn wrote:
> what calls this? Nothing in this file seems to use it. This matches some
variable defined in the mojom? Which one?

This matches the "url" field in the url.mojom.Url struct. StructTraits are
documented at
https://www.chromium.org/developers/design-documents/mojo/type-mapping#TOC-St...


> Also don't abbreviate in blink. what is |r| ?

renamed it to |blinkUrl|.

https://codereview.chromium.org/2000253006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mojo/SecurityOriginStructTraits.h
(right):

https://codereview.chromium.org/2000253006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mojo/SecurityOriginStructTraits.h:15: static
WTF::String scheme(const RefPtr<::blink::SecurityOrigin>& r)
On 2016/05/25 at 17:06:27, esprehn wrote:
> ditto, don't use single letter vars

Fixed

https://codereview.chromium.org/2000253006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mojo/SecurityOriginStructTraits.h:36:
WTF::String scheme, host;
On 2016/05/25 at 17:06:27, esprehn wrote:
> one line per var

Fixed

https://codereview.chromium.org/2000253006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mojo/SecurityOriginStructTraits.h:45: if
(!data.unique() && (*out)->isUnique())
On 2016/05/25 at 17:06:27, esprehn wrote:
> does the contract for these say it's okay to still assign a value to |out|
even when returning false? Where are these documented? :)

Documented at
https://www.chromium.org/developers/design-documents/mojo/type-mapping#TOC-St...
It's not explicit about if it is okay to assign a value to |out| when returning
false, but returning false will result in the connection being closed and
whatever message was being deserialized to not be processed, so it seems
reasonable that it would be okay (also it is what existing StructTraits do).

[Marijn Kruisselbrink, 2016-05-25 18:31:20]

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

[Marijn Kruisselbrink, 2016-05-25 18:31:21]

+dcheng for IPC OWNERS

[Marijn Kruisselbrink, 2016-05-26 20:04:10]

esprehn/dcheng: ping

[esprehn, 2016-05-26 20:06:00]

lgtm w/ headers fixed

https://codereview.chromium.org/2000253006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mojo/KURLStructTraits.h (right):

https://codereview.chromium.org/2000253006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mojo/KURLStructTraits.h:8: #include
"platform/weborigin/KURL.h"
missing WTFString.h

https://codereview.chromium.org/2000253006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mojo/SecurityOriginStructTraits.h
(right):

https://codereview.chromium.org/2000253006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mojo/SecurityOriginStructTraits.h:8: #include
"platform/weborigin/SecurityOrigin.h"
ditto

[dcheng, 2016-05-26 20:15:15]

mojo lgtm

https://codereview.chromium.org/2000253006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mojo/KURLSecurityOriginTest.cpp (right):

https://codereview.chromium.org/2000253006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mojo/KURLSecurityOriginTest.cpp:15: class
UrlTestImpl : public url::mojom::blink::UrlTest {
On 2016/05/25 at 17:35:08, Marijn Kruisselbrink wrote:
> On 2016/05/25 at 17:06:27, esprehn wrote:
> > don't indent nested namespace functions
> 
> I never quite understood what "nested namespaces with the same scope" meant in
the blink style guide ("The contents of an outermost namespace block (and any
nested namespaces with the same scope) should not be indented. The contents of
other nested namespaces should be indented."), but assumed that it meant to
differentiate between "namespace a { namespace b {} class foo {}; }" and
"namespace a { namespace b { class foo {}; } }" where in the first one things
inside b should be indented, while in the second one it shouldn't. Maybe my
understanding of the style guide is wrong though (although this does seem to
match what the clang format rules for blink do).
> 
> Anyway, fixed.

Basically, just think of it as namespaces never introduce indents. You might be
able to find some edge cases where this isn't true, but as a general rule of
thumb, this is safe to follow.

https://codereview.chromium.org/2000253006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mojo/KURLSecurityOriginTest.cpp (right):

https://codereview.chromium.org/2000253006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mojo/KURLSecurityOriginTest.cpp:53: for
(size_t i = 0; i < arraysize(serializeCases); i++) {
Thanks to the magic of C++11, you can just write:

  for (const char* testCase : serializeCases) {
    KURL input(KURL(), testCase);
    // ...
  }

[Marijn Kruisselbrink, 2016-05-26 20:52:43]

https://codereview.chromium.org/2000253006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mojo/KURLSecurityOriginTest.cpp (right):

https://codereview.chromium.org/2000253006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mojo/KURLSecurityOriginTest.cpp:15: class
UrlTestImpl : public url::mojom::blink::UrlTest {
On 2016/05/26 at 20:15:15, dcheng wrote:
> On 2016/05/25 at 17:35:08, Marijn Kruisselbrink wrote:
> > On 2016/05/25 at 17:06:27, esprehn wrote:
> > > don't indent nested namespace functions
> > 
> > I never quite understood what "nested namespaces with the same scope" meant
in the blink style guide ("The contents of an outermost namespace block (and any
nested namespaces with the same scope) should not be indented. The contents of
other nested namespaces should be indented."), but assumed that it meant to
differentiate between "namespace a { namespace b {} class foo {}; }" and
"namespace a { namespace b { class foo {}; } }" where in the first one things
inside b should be indented, while in the second one it shouldn't. Maybe my
understanding of the style guide is wrong though (although this does seem to
match what the clang format rules for blink do).
> > 
> > Anyway, fixed.
> 
> Basically, just think of it as namespaces never introduce indents. You might
be able to find some edge cases where this isn't true, but as a general rule of
thumb, this is safe to follow.

Yes, that's the chromium styleguide. The blink/webkit styleguide are explicitly
not like that (even the example in the blink styleguide has the contents of
"NestedNamespace" indented). But yeah, this definitely seems to be the most
often ignored rule in the blink styleguide. But probably not worth fixing the
blink styleguide to match reality since we hopefully won't have to deal with
separate styles for much longer anyway.

https://codereview.chromium.org/2000253006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mojo/KURLSecurityOriginTest.cpp (right):

https://codereview.chromium.org/2000253006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mojo/KURLSecurityOriginTest.cpp:53: for
(size_t i = 0; i < arraysize(serializeCases); i++) {
On 2016/05/26 at 20:15:15, dcheng wrote:
> Thanks to the magic of C++11, you can just write:
> 
>   for (const char* testCase : serializeCases) {
>     KURL input(KURL(), testCase);
>     // ...
>   }

Good point, not sure why the non-blink version of this test didn't do it that
way.

https://codereview.chromium.org/2000253006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mojo/KURLStructTraits.h (right):

https://codereview.chromium.org/2000253006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mojo/KURLStructTraits.h:8: #include
"platform/weborigin/KURL.h"
On 2016/05/26 at 20:06:00, esprehn wrote:
> missing WTFString.h

Done

https://codereview.chromium.org/2000253006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mojo/SecurityOriginStructTraits.h
(right):

https://codereview.chromium.org/2000253006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mojo/SecurityOriginStructTraits.h:8: #include
"platform/weborigin/SecurityOrigin.h"
On 2016/05/26 at 20:06:00, esprehn wrote:
> ditto

Done

[Marijn Kruisselbrink, 2016-05-26 20:53:10]

mek@chromium.org changed reviewers:
+ brettw@chromium.org

[Marijn Kruisselbrink, 2016-05-26 20:53:10]

+brettw for url/url.gyp OWNERS

[brettw, 2016-05-27 20:15:04]

https://codereview.chromium.org/2000253006/diff/80001/url/url.gyp
File url/url.gyp (right):

https://codereview.chromium.org/2000253006/diff/80001/url/url.gyp#newcode137
url/url.gyp:137: 'target_name': 'url_interfaces_mojom_for_blink',
Can this have a comment about where the GN code is? I don't understand these
mojo things, but you can see the other targets here have comments like:
  # GN version: //url:url_features

[Marijn Kruisselbrink, 2016-05-27 20:29:02]

https://codereview.chromium.org/2000253006/diff/80001/url/url.gyp
File url/url.gyp (right):

https://codereview.chromium.org/2000253006/diff/80001/url/url.gyp#newcode137
url/url.gyp:137: 'target_name': 'url_interfaces_mojom_for_blink',
On 2016/05/27 at 20:15:04, brettw wrote:
> Can this have a comment about where the GN code is? I don't understand these
mojo things, but you can see the other targets here have comments like:
>   # GN version: //url:url_features

url_features seems to be the only target that has such a comment, none of the
other targets do.

Also I'm not sure if it really makes sense to try to invent such a mapping for
these mojo targets, as there really isn't any kind of 1:1 mapping. For example
the closest equivalent to this target on the GN side would be the union of
//url/mojo:url_mojom_gurl_blink and //url/mojo::url_mojom_origin_blink, but
those target names don't actually exist anywhere in any of the .gn files
(instead they are generated by the same rules as the non-blink versions). And to
actually use mojo bindings in some other target you'd need to do something very
different in gyp compared to gn anyway.

[Marijn Kruisselbrink, 2016-05-27 20:35:56]

https://codereview.chromium.org/2000253006/diff/80001/url/url.gyp
File url/url.gyp (right):

https://codereview.chromium.org/2000253006/diff/80001/url/url.gyp#newcode137
url/url.gyp:137: 'target_name': 'url_interfaces_mojom_for_blink',
On 2016/05/27 at 20:29:02, Marijn Kruisselbrink wrote:
> On 2016/05/27 at 20:15:04, brettw wrote:
> > Can this have a comment about where the GN code is? I don't understand these
mojo things, but you can see the other targets here have comments like:
> >   # GN version: //url:url_features
> 
> url_features seems to be the only target that has such a comment, none of the
other targets do.
> 
> Also I'm not sure if it really makes sense to try to invent such a mapping for
these mojo targets, as there really isn't any kind of 1:1 mapping. For example
the closest equivalent to this target on the GN side would be the union of
//url/mojo:url_mojom_gurl_blink and //url/mojo::url_mojom_origin_blink, but
those target names don't actually exist anywhere in any of the .gn files
(instead they are generated by the same rules as the non-blink versions). And to
actually use mojo bindings in some other target you'd need to do something very
different in gyp compared to gn anyway.

But maybe having a non-perfect match comment is still better than nothing, so
added some GN version comments.

[Marijn Kruisselbrink, 2016-05-31 17:14:36]

brettw: ping?

[brettw, 2016-05-31 20:14:30]

LGTM (we should have the GN comments, the fact that the other ones were missing
it was a mistake, and they don't have to be perfect, just to tell people where
to look generally).

[Marijn Kruisselbrink, 2016-05-31 20:16:08]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2016-05-31 20:16:09]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
yzshen@chromium.org, esprehn@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2000253006/#ps120001
(title: "try to fix gyp rules")

[commit-bot: I haz the power, 2016-05-31 20:16:28]

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

[commit-bot: I haz the power, 2016-05-31 22:24:24]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-05-31 22:25:57]

Description was changed from

==========
Implement URL and Origin typemaps/struct traits for blink.
==========

to

==========
Implement URL and Origin typemaps/struct traits for blink.

Committed: https://crrev.com/8919fe48df2ae876444abfd8068bcb817c63a25e
Cr-Commit-Position: refs/heads/master@{#396952}
==========

[commit-bot: I haz the power, 2016-05-31 22:25:58]

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