New Mojom Parser: Serialization.

New Mojom Parser: Serialization.

R=azani@chromium.org, mattr@google.com
BUG=#461

This is the final major piece of the mojom parser--the logic to serialize the MojomDescriptor to a Mojo message.

Note that currently the use of Mojo serialization requires using cgo and linking against native librarires.
This also means that serialization_test.go cannot be run as a pure go test the way the other tests are run but
rather must be run using the "go_test_binary" target. This target will build a test binary in
<out_dir>/obj/mojom/mojom_parser/serialization_test which needs to be executed separately.

Here is a summary of the files changed:

- mojo/go/rules.gni: I updated the "go_test_binary" target to be closer to the "go_binary" target

- get_test_list.py: I added a new category of test for serialization_test since on the one hand it cannot
be run as part of the Pure Go unit tests and on the other hand it would be confusing and misleading to
run it under the heading Go system tests.

- mojom/mojom_parser/generated: This directory contains generated Go code that we are checking in. It is the
Go bindings for mojom_types.mojom and mojom_files.mojom.

- serialization.go: This is the actual serialization logic.

- serialization_test.go: A test of the serialization logic.

Committed: https://chromium.googlesource.com/external/mojo/+/3708f59648870ffd4ab7f062e75f9d7ac7375449

[rudominer, 2015-11-03 22:34:03]

Description was changed from

==========
New Mojom Parser: Serialization.

BUG=
==========

to

==========
New Mojom Parser: Serialization.

R=mattr@google.com, azani@chromium.org
BUG=#461
==========

[rudominer, 2015-11-03 22:34:03]

rudominer@chromium.org changed reviewers:
+ azani@chromium.org, mattr@google.com

[rudominer, 2015-11-03 22:57:41]

Description was changed from

==========
New Mojom Parser: Serialization.

R=mattr@google.com, azani@chromium.org
BUG=#461
==========

to

==========
New Mojom Parser: Serialization.

R=mattr@google.com, azani@chromium.org
BUG=#461

This is the final major piece of the mojom parser--the logic to serialize the
MojomDescriptor to a Mojo message.

Note that currently the use of Mojo serialization requires using cgo and linking
against native librarires. This also means that serialization_test.go cannot be
run as a pure go test the way the other tests are run but rather must be run
using the "go_test_binary" target. This target will build a
test binary in <out_dir>/obj/mojom/mojom_parser/serialization_test which needs
to be executed separately.

Here is a summary of the files changed:

- mojo/go/rules.gni: I updated the "go_test_binary" target to be closer to the
"go_binary" target

- get_test_list.py: I added a new category of test for serialization_test since
on the one hand it cannot be run as part of the Pure Go unit tests and on the
other hand it would be confusing and misleading to run it under the heading Go
system tests.

- mojom/mojom_parser/generated: This directory contains generated Go code that
we are checking in. It is the Go bindings for mojom_types.mojom and
mojom_files.mojom.

- serialization.go: This is the actual serialization logic.

- serialization_test.go: A test of the serialization logic.
==========

[rudominer, 2015-11-03 22:59:42]

Description was changed from

==========
New Mojom Parser: Serialization.

R=mattr@google.com, azani@chromium.org
BUG=#461

This is the final major piece of the mojom parser--the logic to serialize the
MojomDescriptor to a Mojo message.

Note that currently the use of Mojo serialization requires using cgo and linking
against native librarires. This also means that serialization_test.go cannot be
run as a pure go test the way the other tests are run but rather must be run
using the "go_test_binary" target. This target will build a
test binary in <out_dir>/obj/mojom/mojom_parser/serialization_test which needs
to be executed separately.

Here is a summary of the files changed:

- mojo/go/rules.gni: I updated the "go_test_binary" target to be closer to the
"go_binary" target

- get_test_list.py: I added a new category of test for serialization_test since
on the one hand it cannot be run as part of the Pure Go unit tests and on the
other hand it would be confusing and misleading to run it under the heading Go
system tests.

- mojom/mojom_parser/generated: This directory contains generated Go code that
we are checking in. It is the Go bindings for mojom_types.mojom and
mojom_files.mojom.

- serialization.go: This is the actual serialization logic.

- serialization_test.go: A test of the serialization logic.
==========

to

==========
New Mojom Parser: Serialization.

R=mattr@google.com, azani@chromium.org
BUG=#461

This is the final major piece of the mojom parser--the logic to serialize the
MojomDescriptor to a Mojo message.

Note that currently the use of Mojo serialization requires using cgo and linking
against native librarires. 
This also means that serialization_test.go cannot be run as a pure go test the
way the other tests are run but 
rather must be run using the "go_test_binary" target. This target will build a
test binary in 
<out_dir>/obj/mojom/mojom_parser/serialization_test which needs to be executed
separately.

Here is a summary of the files changed:

- mojo/go/rules.gni: I updated the "go_test_binary" target to be closer to the
"go_binary" target

- get_test_list.py: I added a new category of test for serialization_test since
on the one hand it cannot 
be run as part of the Pure Go unit tests and on the other hand it would be
confusing and misleading to 
run it under the heading Go system tests.

- mojom/mojom_parser/generated: This directory contains generated Go code that
we are checking in. It is the 
Go bindings for mojom_types.mojom and mojom_files.mojom.

- serialization.go: This is the actual serialization logic.

- serialization_test.go: A test of the serialization logic.
==========

[rudominer, 2015-11-03 23:03:33]

Alex and Matt, ptal.

Note that serialization_test.go is not complete. I will continue working on this
test as you review the rest.

You will see some TODOs sprinkled throughout which indicate tasks I plan to
finish in later CLs.

thanks,
Mitch

[rudominer, 2015-11-05 18:42:46]

Patchset #9 (id:160001) has been deleted

[rudominer, 2015-11-05 18:43:11]

Patchset #7 (id:120001) has been deleted

[rudominer, 2015-11-05 18:43:38]

Patchset #6 (id:100001) has been deleted

[rudominer, 2015-11-05 18:44:14]

Patchset #6 (id:140001) has been deleted

[rudominer, 2015-11-05 18:49:01]

Alex & Matt, serialization_test.go is now ready for you to look at.

In the course of creating this test I made two small changes to to the code:
- I changed the scheme for generating type and value keys. 
(See mojom_descriptor.go) Previously I was using successive integers for keys.
But this makes serialization_test very awkward because every time I add a new
type or value all the keys change. So I switched to a new strategy: I use the
reversed fqn.
- I added the ability for the serializer to omit line and column numbers. These
also made serialization_test very awkward.

[azani, 2015-11-05 21:20:46]

just a nit really.

https://codereview.chromium.org/1421193003/diff/180001/mojo/tools/get_test_li...
File mojo/tools/get_test_list.py (right):

https://codereview.chromium.org/1421193003/diff/180001/mojo/tools/get_test_li...
mojo/tools/get_test_list.py:127: # Go mojom parser serialization tests:
Maybe fold this under mojom_parser tests. It is a bit confusing since part of
the point of mojom is the creation of serialization code.

[rudominer, 2015-11-05 21:47:31]

Thanks Alex.

OK Matt whenever you have some time...
This should be the last one.

https://codereview.chromium.org/1421193003/diff/180001/mojo/tools/get_test_li...
File mojo/tools/get_test_list.py (right):

https://codereview.chromium.org/1421193003/diff/180001/mojo/tools/get_test_li...
mojo/tools/get_test_list.py:127: # Go mojom parser serialization tests:
On 2015/11/05 21:20:46, azani wrote:
> Maybe fold this under mojom_parser tests. It is a bit confusing since part of
> the point of mojom is the creation of serialization code.

Done.

[azani, 2015-11-05 21:58:55]

lgtm

[rudominer, 2015-11-09 17:48:29]

Patchset #8 (id:220001) has been deleted

[rudominer, 2015-11-09 17:50:07]

Alex, I added a README.md file to the |generated| directory. ptal. (We probably
will want to eventually add a script to automate what I describe there but I'm
not ready to do that and so the README is helpful.)

Matt, do you think you will have time to take a look today?

thanks,
Mitch

[mattr, 2015-11-10 19:26:49]

Dropped a few comments, feel free to ignore if you want.  Sorry for being so
slow, we've been firefighting.

https://codereview.chromium.org/1421193003/diff/260001/mojom/mojom_parser/moj...
File mojom/mojom_parser/mojom/mojom_descriptor.go (right):

https://codereview.chromium.org/1421193003/diff/260001/mojom/mojom_parser/moj...
mojom/mojom_parser/mojom/mojom_descriptor.go:479: // for the reversal is
pragmatic: When debugging we can tell whether a string
Reversing seems an odd way to mark the name, you could just add a prefix or
something.

https://codereview.chromium.org/1421193003/diff/260001/mojom/mojom_parser/ser...
File mojom/mojom_parser/serialization/serialization.go (right):

https://codereview.chromium.org/1421193003/diff/260001/mojom/mojom_parser/ser...
mojom/mojom_parser/serialization/serialization.go:93:
*(file.DeclaredMojomObjects.Interfaces) =
append(*(file.DeclaredMojomObjects.Interfaces), intrfc.TypeKey())
Why do we have a pointer to a slice?  Slices are already nilable and have a
separate zero.  Anyway I'm sure there's a reason.

https://codereview.chromium.org/1421193003/diff/260001/mojom/mojom_parser/ser...
mojom/mojom_parser/serialization/serialization.go:555: func newString(s string)
*string {
I don't understand this function.  In Go strings are immutable so it shouldn't
be necessary to copy, and actually what you are doing is not copying but
creating a new string header only.  What are you actually trying to accomplish?

https://codereview.chromium.org/1421193003/diff/260001/mojom/mojom_parser/ser...
File mojom/mojom_parser/serialization/serialization_test.go (right):

https://codereview.chromium.org/1421193003/diff/260001/mojom/mojom_parser/ser...
mojom/mojom_parser/serialization/serialization_test.go:35: startTestCase :=
func(moduleNameSpace string) {
I found this structure a bit confusing.  You could also do:

func defineTestCase(moduleNamespace string, body func(*testCase)) {
  // do the stuff from startTestCase
  body(newTestCase)
  // do the stuff from endTestCase
}

then pass it a function roughly corresponding to the block you've defined for
the existing test case.

[azani, 2015-11-10 22:46:07]

https://codereview.chromium.org/1421193003/diff/260001/mojom/mojom_parser/ser...
File mojom/mojom_parser/serialization/serialization.go (right):

https://codereview.chromium.org/1421193003/diff/260001/mojom/mojom_parser/ser...
mojom/mojom_parser/serialization/serialization.go:310: case mojom.ArrayTypeRef:
AFAICT, from running this code, you should expect *mojom.ArrayTypeRef. Same for
at least mojom.MapTypeRef and potentially others.

[rudominer, 2015-11-10 23:29:37]

Thanks Matt. I addressed your comments. ptal.

https://codereview.chromium.org/1421193003/diff/260001/mojom/mojom_parser/moj...
File mojom/mojom_parser/mojom/mojom_descriptor.go (right):

https://codereview.chromium.org/1421193003/diff/260001/mojom/mojom_parser/moj...
mojom/mojom_parser/mojom/mojom_descriptor.go:479: // for the reversal is
pragmatic: When debugging we can tell whether a string
On 2015/11/10 19:26:49, mattr wrote:
> Reversing seems an odd way to mark the name, you could just add a prefix or
> something.

Good idea. Thanks. Done.

https://codereview.chromium.org/1421193003/diff/260001/mojom/mojom_parser/ser...
File mojom/mojom_parser/serialization/serialization.go (right):

https://codereview.chromium.org/1421193003/diff/260001/mojom/mojom_parser/ser...
mojom/mojom_parser/serialization/serialization.go:93:
*(file.DeclaredMojomObjects.Interfaces) =
append(*(file.DeclaredMojomObjects.Interfaces), intrfc.TypeKey())
On 2015/11/10 19:26:49, mattr wrote:
> Why do we have a pointer to a slice?  Slices are already nilable and have a
> separate zero.  Anyway I'm sure there's a reason.

The Go code that defines a variable as a pointer to a slice is not hand-written
but is generated from Mojom. I didn't write the code that does the generation
but I think I can guess why it was done this way. It is because in Mojom there
is a distinction between an array of strings (array<string>) and a nullable
array of strings (array<string>?). The former gets translated to a string slice
and the latter to a pointer to a string slice.

https://codereview.chromium.org/1421193003/diff/260001/mojom/mojom_parser/ser...
mojom/mojom_parser/serialization/serialization.go:310: case mojom.ArrayTypeRef:
On 2015/11/10 22:46:07, azani wrote:
> AFAICT, from running this code, you should expect *mojom.ArrayTypeRef. Same
for
> at least mojom.MapTypeRef and potentially others.

OK, thanks for testing it. I will fix in a follow-up cl.

https://codereview.chromium.org/1421193003/diff/260001/mojom/mojom_parser/ser...
mojom/mojom_parser/serialization/serialization.go:555: func newString(s string)
*string {
On 2015/11/10 19:26:49, mattr wrote:
> I don't understand this function.  In Go strings are immutable so it shouldn't
> be necessary to copy, and actually what you are doing is not copying but
> creating a new string header only.  What are you actually trying to
accomplish?

The explanation for this peculiarity is similar to the previous one.

I added the following comment to clarify:
// newString is a convenience function for creating a pointer to a 
// string whose value  is the specified string. It is necessary to 
// create pointers to strings because that is how the Mojom type |string?| 
// (i.e. nullable string) is represented in the Mojom Go bindings.

https://codereview.chromium.org/1421193003/diff/260001/mojom/mojom_parser/ser...
File mojom/mojom_parser/serialization/serialization_test.go (right):

https://codereview.chromium.org/1421193003/diff/260001/mojom/mojom_parser/ser...
mojom/mojom_parser/serialization/serialization_test.go:35: startTestCase :=
func(moduleNameSpace string) {
On 2015/11/10 19:26:49, mattr wrote:
> I found this structure a bit confusing.  You could also do:
> 
> func defineTestCase(moduleNamespace string, body func(*testCase)) {
>   // do the stuff from startTestCase
>   body(newTestCase)
>   // do the stuff from endTestCase
> }
> 
> then pass it a function roughly corresponding to the block you've defined for
> the existing test case.

I have refactored the test to try to make it more readable. I did not do exactly
as you suggested but I did
extract all of the local functions into real defined functions.

[rudominer, 2015-11-10 23:39:47]

I already have an lgtm from azani@ and mattr@ gave me an lgtm over hangouts so I
am going to land now.

[rudominer, 2015-11-10 23:40:28]

Committed patchset #11 (id:300001) manually as
3708f59648870ffd4ab7f062e75f9d7ac7375449 (presubmit successful).

[mattr, 2015-11-10 23:43:40]

https://codereview.chromium.org/1421193003/diff/260001/mojom/mojom_parser/ser...
File mojom/mojom_parser/serialization/serialization.go (right):

https://codereview.chromium.org/1421193003/diff/260001/mojom/mojom_parser/ser...
mojom/mojom_parser/serialization/serialization.go:555: func newString(s string)
*string {
On 2015/11/10 23:29:36, rudominer wrote:
> On 2015/11/10 19:26:49, mattr wrote:
> > I don't understand this function.  In Go strings are immutable so it
shouldn't
> > be necessary to copy, and actually what you are doing is not copying but
> > creating a new string header only.  What are you actually trying to
> accomplish?
> 
> The explanation for this peculiarity is similar to the previous one.
> 
> I added the following comment to clarify:
> // newString is a convenience function for creating a pointer to a 
> // string whose value  is the specified string. It is necessary to 
> // create pointers to strings because that is how the Mojom type |string?| 
> // (i.e. nullable string) is represented in the Mojom Go bindings.

Sure, but you should just do 

return &s

In which case there is really no need for the function.

[rudominer, 2015-11-11 00:18:11]

On 2015/11/10 23:43:40, mattr wrote:
>
https://codereview.chromium.org/1421193003/diff/260001/mojom/mojom_parser/ser...
> File mojom/mojom_parser/serialization/serialization.go (right):
> 
>
https://codereview.chromium.org/1421193003/diff/260001/mojom/mojom_parser/ser...
> mojom/mojom_parser/serialization/serialization.go:555: func newString(s
string)
> *string {
> On 2015/11/10 23:29:36, rudominer wrote:
> > On 2015/11/10 19:26:49, mattr wrote:
> > > I don't understand this function.  In Go strings are immutable so it
> shouldn't
> > > be necessary to copy, and actually what you are doing is not copying but
> > > creating a new string header only.  What are you actually trying to
> > accomplish?
> > 
> > The explanation for this peculiarity is similar to the previous one.
> > 
> > I added the following comment to clarify:
> > // newString is a convenience function for creating a pointer to a 
> > // string whose value  is the specified string. It is necessary to 
> > // create pointers to strings because that is how the Mojom type |string?| 
> > // (i.e. nullable string) is represented in the Mojom Go bindings.
> 
> Sure, but you should just do 
> 
> return &s
> 
> In which case there is really no need for the function.

The function is necessary because you cannot take the address of a string value
that is not an lvalue such as a literal or the return value from a function.
I have changed the implementation of the function as you suggests in
https://codereview.chromium.org/1409063008/.