mojo: Starting serialization for python bindings.

mojo: Starting serialization for python bindings.

R=sdefresne@chromium.org,cmasone@chromium.org

Committed: https://crrev.com/4e395c5e4c9025f187c98fb38730858b5cfdcc65
Cr-Commit-Position: refs/heads/master@{#294846}

[sdefresne, 2014-09-15 08:46:55]

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
File mojo/public/python/mojo/bindings/descriptor.py (right):

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/descriptor.py:66: 
nit: only two blank lines

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/descriptor.py:69: class BooleanType(Type):
Are BooleanType really unserializable or did you forget making them so?

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/descriptor.py:92: raise NotImplementedError()
Why don't you implement this using struct.calcsize(self.GetTypeCode())?

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/descriptor.py:263: assert sub_type != TYPE_BOOL
Use "is not" instead of "!=" here as types are singleton in python.

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/descriptor.py:470: value = 0
This could be rewritten as "reduce(lambda x, y: x * 2 + y, booleans, 0)" but
maybe this is too compact...

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
File mojo/public/python/mojo/bindings/serialization.py (right):

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/serialization.py:69: """Returns the padding
necessary to align value with the given alignment."""
nit: this method could return a tuple padding_length, and padding code (i.e. 'x'
* padding_length)

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/serialization.py:76: return reduce(lambda x, y:
x+y, [len(x.descriptors) for x in groups])
nit: "reduce(lambda x, y: x+y, [...])" can be rewritten as "sum(...)".

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/serialization.py:95: end_alignment = index % 8
nit: you can use NeededPaddingForAlignment here.

https://codereview.chromium.org/570563002/diff/1/mojo/public/tools/bindings/g...
File mojo/public/tools/bindings/generators/mojom_python_generator.py (right):

https://codereview.chromium.org/570563002/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/mojom_python_generator.py:43:
_kind_to_typecode_for_array = {
nit: _kind_to_typecode_for_native_array

https://codereview.chromium.org/570563002/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/mojom_python_generator.py:134:
arguments.append('\'%s\'' %_kind_to_typecode_for_array[kind.kind])
nit: you can use '%r' instead of '\'%s\'' to get a quoted string, I think it is
cleaner.

https://codereview.chromium.org/570563002/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/mojom_python_generator.py:196: print byte
Are those two lines debug? Since the last line is "assert False", I would think
that this function is never called (or at least that you never tested it).
Should it be removed or else tested?

https://codereview.chromium.org/570563002/diff/1/mojo/python/tests/bindings_s...
File mojo/python/tests/bindings_serialization_deserialization_unittest.py
(right):

https://codereview.chromium.org/570563002/diff/1/mojo/python/tests/bindings_s...
mojo/python/tests/bindings_serialization_deserialization_unittest.py:47: class
SerializationDeserializationTest(unittest.TestCase):
The class only test serialization, is it incorrectly named or did you forget
about deserialization testing?

[qsr, 2014-09-15 11:01:53]

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
File mojo/public/python/mojo/bindings/descriptor.py (right):

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/descriptor.py:66: 
On 2014/09/15 08:46:54, sdefresne wrote:
> nit: only two blank lines

Done.

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/descriptor.py:69: class BooleanType(Type):
On 2014/09/15 08:46:54, sdefresne wrote:
> Are BooleanType really unserializable or did you forget making them so?

 You cannot serialize a boolean by itself, because boolean are packed into
bytes. Serialization is done via BooleanGroup

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/descriptor.py:92: raise NotImplementedError()
On 2014/09/15 08:46:54, sdefresne wrote:
> Why don't you implement this using struct.calcsize(self.GetTypeCode())?

 Refactored to take the type code in the constructor, and initialize the size
there too.

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/descriptor.py:263: assert sub_type != TYPE_BOOL
Except that TYPE_BOOL is an instance, not a type. Am I missing something stupid.
Anyway, changed that to check that sub_type is serializable.

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/descriptor.py:470: value = 0
On 2014/09/15 08:46:54, sdefresne wrote:
> This could be rewritten as "reduce(lambda x, y: x * 2 + y, booleans, 0)" but
> maybe this is too compact...

Done.

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
File mojo/public/python/mojo/bindings/serialization.py (right):

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/serialization.py:69: """Returns the padding
necessary to align value with the given alignment."""
On 2014/09/15 08:46:54, sdefresne wrote:
> nit: this method could return a tuple padding_length, and padding code (i.e.
'x'
> * padding_length)

 Not really. It is used that way only once, but I use it plenty of time to just
compute a position, and I do not want to generate strings and use memory in
those cases.

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/serialization.py:76: return reduce(lambda x, y:
x+y, [len(x.descriptors) for x in groups])
On 2014/09/15 08:46:54, sdefresne wrote:
> nit: "reduce(lambda x, y: x+y, [...])" can be rewritten as "sum(...)".

Done.

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/serialization.py:95: end_alignment = index % 8
On 2014/09/15 08:46:54, sdefresne wrote:
> nit: you can use NeededPaddingForAlignment here.

Done.

https://codereview.chromium.org/570563002/diff/1/mojo/public/tools/bindings/g...
File mojo/public/tools/bindings/generators/mojom_python_generator.py (right):

https://codereview.chromium.org/570563002/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/mojom_python_generator.py:43:
_kind_to_typecode_for_array = {
On 2014/09/15 08:46:54, sdefresne wrote:
> nit: _kind_to_typecode_for_native_array

Done.

https://codereview.chromium.org/570563002/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/mojom_python_generator.py:134:
arguments.append('\'%s\'' %_kind_to_typecode_for_array[kind.kind])
On 2014/09/15 08:46:54, sdefresne wrote:
> nit: you can use '%r' instead of '\'%s\'' to get a quoted string, I think it
is
> cleaner.

Done.

https://codereview.chromium.org/570563002/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/mojom_python_generator.py:196: print byte
On 2014/09/15 08:46:55, sdefresne wrote:
> Are those two lines debug? Since the last line is "assert False", I would
think
> that this function is never called (or at least that you never tested it).
> Should it be removed or else tested?

 The assert False is correct, the print was just for debugging, and should not
be there. Removed.

https://codereview.chromium.org/570563002/diff/1/mojo/python/tests/bindings_s...
File mojo/python/tests/bindings_serialization_deserialization_unittest.py
(right):

https://codereview.chromium.org/570563002/diff/1/mojo/python/tests/bindings_s...
mojo/python/tests/bindings_serialization_deserialization_unittest.py:47: class
SerializationDeserializationTest(unittest.TestCase):
On 2014/09/15 08:46:55, sdefresne wrote:
> The class only test serialization, is it incorrectly named or did you forget
> about deserialization testing?

 Deserialization is not implemented in this CL. Test for it will be added here.
I want to test both at the same time, because the easier way to do a non trivial
test of the process is starting with an entity, serialize it/ deserialize the
result and check that the result has the same fields as the initial one.

[sdefresne, 2014-09-15 11:14:46]

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
File mojo/public/python/mojo/bindings/descriptor.py (right):

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/descriptor.py:69: class BooleanType(Type):
On 2014/09/15 11:01:52, qsr wrote:
> On 2014/09/15 08:46:54, sdefresne wrote:
> > Are BooleanType really unserializable or did you forget making them so?
> 
>  You cannot serialize a boolean by itself, because boolean are packed into
> bytes. Serialization is done via BooleanGroup

OK, this make sense.

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/descriptor.py:263: assert sub_type != TYPE_BOOL
On 2014/09/15 11:01:53, qsr wrote:
> Except that TYPE_BOOL is an instance, not a type. Am I missing something
stupid.
> Anyway, changed that to check that sub_type is serializable.

Well it would have been a singleton nonetheless. Changing to checking that the
type is serializable is probably better. Good.

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
File mojo/public/python/mojo/bindings/serialization.py (right):

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/serialization.py:69: """Returns the padding
necessary to align value with the given alignment."""
On 2014/09/15 11:01:53, qsr wrote:
> On 2014/09/15 08:46:54, sdefresne wrote:
> > nit: this method could return a tuple padding_length, and padding code (i.e.
> 'x'
> > * padding_length)
> 
>  Not really. It is used that way only once, but I use it plenty of time to
just
> compute a position, and I do not want to generate strings and use memory in
> those cases.

Make sense, thank you for the justification.

https://codereview.chromium.org/570563002/diff/1/mojo/public/tools/bindings/g...
File mojo/public/tools/bindings/generators/mojom_python_generator.py (right):

https://codereview.chromium.org/570563002/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/mojom_python_generator.py:195: return
GetFieldDescriptor(packed_field)
This return in a for loop is strange. Do you only want to return a value for the
first item in byte.packed_fields? What about instead directly writing "return
GetFieldDescriptor(byte.packed_fields[0])" instead? This make it obvious. With
maybe an assertion that the list is not empty.

https://codereview.chromium.org/570563002/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/mojom_python_generator.py:196: print byte
On 2014/09/15 11:01:53, qsr wrote:
> On 2014/09/15 08:46:55, sdefresne wrote:
> > Are those two lines debug? Since the last line is "assert False", I would
> think
> > that this function is never called (or at least that you never tested it).
> > Should it be removed or else tested?
> 
>  The assert False is correct, the print was just for debugging, and should not
> be there. Removed.

Oh, you mean this line should not be reached? What about using the "assert
condition, error message" syntax to make it obvious this should not be reached.

https://codereview.chromium.org/570563002/diff/1/mojo/python/tests/bindings_s...
File mojo/python/tests/bindings_serialization_deserialization_unittest.py
(right):

https://codereview.chromium.org/570563002/diff/1/mojo/python/tests/bindings_s...
mojo/python/tests/bindings_serialization_deserialization_unittest.py:47: class
SerializationDeserializationTest(unittest.TestCase):
On 2014/09/15 11:01:53, qsr wrote:
> On 2014/09/15 08:46:55, sdefresne wrote:
> > The class only test serialization, is it incorrectly named or did you forget
> > about deserialization testing?
> 
>  Deserialization is not implemented in this CL. Test for it will be added
here.
> I want to test both at the same time, because the easier way to do a non
trivial
> test of the process is starting with an entity, serialize it/ deserialize the
> result and check that the result has the same fields as the initial one.

Make sense.

[qsr, 2014-09-15 11:42:00]

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
File mojo/public/python/mojo/bindings/descriptor.py (right):

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/descriptor.py:69: class BooleanType(Type):
On 2014/09/15 11:14:46, sdefresne wrote:
> On 2014/09/15 11:01:52, qsr wrote:
> > On 2014/09/15 08:46:54, sdefresne wrote:
> > > Are BooleanType really unserializable or did you forget making them so?
> > 
> >  You cannot serialize a boolean by itself, because boolean are packed into
> > bytes. Serialization is done via BooleanGroup
> 
> OK, this make sense.

Acknowledged.

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/descriptor.py:263: assert sub_type != TYPE_BOOL
On 2014/09/15 11:14:46, sdefresne wrote:
> On 2014/09/15 11:01:53, qsr wrote:
> > Except that TYPE_BOOL is an instance, not a type. Am I missing something
> stupid.
> > Anyway, changed that to check that sub_type is serializable.
> 
> Well it would have been a singleton nonetheless. Changing to checking that the
> type is serializable is probably better. Good.

Acknowledged.

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
File mojo/public/python/mojo/bindings/serialization.py (right):

https://codereview.chromium.org/570563002/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/serialization.py:69: """Returns the padding
necessary to align value with the given alignment."""
On 2014/09/15 11:14:46, sdefresne wrote:
> On 2014/09/15 11:01:53, qsr wrote:
> > On 2014/09/15 08:46:54, sdefresne wrote:
> > > nit: this method could return a tuple padding_length, and padding code
(i.e.
> > 'x'
> > > * padding_length)
> > 
> >  Not really. It is used that way only once, but I use it plenty of time to
> just
> > compute a position, and I do not want to generate strings and use memory in
> > those cases.
> 
> Make sense, thank you for the justification.

Acknowledged.

https://codereview.chromium.org/570563002/diff/1/mojo/public/tools/bindings/g...
File mojo/public/tools/bindings/generators/mojom_python_generator.py (right):

https://codereview.chromium.org/570563002/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/mojom_python_generator.py:195: return
GetFieldDescriptor(packed_field)
On 2014/09/15 11:14:46, sdefresne wrote:
> This return in a for loop is strange. Do you only want to return a value for
the
> first item in byte.packed_fields? What about instead directly writing "return
> GetFieldDescriptor(byte.packed_fields[0])" instead? This make it obvious. With
> maybe an assertion that the list is not empty.

 Refactored by asserting the list has a single element.

https://codereview.chromium.org/570563002/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/mojom_python_generator.py:196: print byte
 Removed, asserting the list has a single element instead.

https://codereview.chromium.org/570563002/diff/1/mojo/python/tests/bindings_s...
File mojo/python/tests/bindings_serialization_deserialization_unittest.py
(right):

https://codereview.chromium.org/570563002/diff/1/mojo/python/tests/bindings_s...
mojo/python/tests/bindings_serialization_deserialization_unittest.py:47: class
SerializationDeserializationTest(unittest.TestCase):
On 2014/09/15 11:14:46, sdefresne wrote:
> On 2014/09/15 11:01:53, qsr wrote:
> > On 2014/09/15 08:46:55, sdefresne wrote:
> > > The class only test serialization, is it incorrectly named or did you
forget
> > > about deserialization testing?
> > 
> >  Deserialization is not implemented in this CL. Test for it will be added
> here.
> > I want to test both at the same time, because the easier way to do a non
> trivial
> > test of the process is starting with an entity, serialize it/ deserialize
the
> > result and check that the result has the same fields as the initial one.
> 
> Make sense.

Acknowledged.

[sdefresne, 2014-09-15 14:44:50]

lgtm

[qsr, 2014-09-15 15:16:18]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-09-15 15:17:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/570563002/40001

[commit-bot: I haz the power, 2014-09-15 17:42:49]

Committed patchset #3 (id:40001) as 4f78dc6221862386a4af70a054f9c782194f2695

[commit-bot: I haz the power, 2014-09-15 17:48:38]

Patchset 3 (id:??) landed as
https://crrev.com/4e395c5e4c9025f187c98fb38730858b5cfdcc65
Cr-Commit-Position: refs/heads/master@{#294846}

[acolwell GONE FROM CHROMIUM, 2014-09-16 00:45:55]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/576603002/ by acolwell@chromium.org.

The reason for reverting is: This patch appears to be causing failures on the
Linux Tests bot.

http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests/builds/13627.