mojo: Add deserialization to python structs.

mojo: Add deserialization to python structs.

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

Committed: https://crrev.com/ea8a6ef576c314506c4b3ee25544f6bcaa895fe1
Cr-Commit-Position: refs/heads/master@{#295478}

[sdefresne, 2014-09-18 12:56:22]

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

https://codereview.chromium.org/578263003/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/descriptor.py:409: return None
Don't you want to validate that the value is 0?

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

https://codereview.chromium.org/578263003/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/reflection.py:112: fields = {}
Why not just doing result._fields = {} here? Nothing else has access to result,
so even if an exception is raised, the only way to access result in an
inconsistent state would be to inspect the stack, which I don't expect anyone
sane to do.

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

https://codereview.chromium.org/578263003/diff/1/mojo/python/tests/bindings_s...
mojo/python/tests/bindings_serialization_deserialization_unittest.py:20:
bar_instance = sample_service_mojom.Bar()
nit: I think you don't test serialization/deserialization of float/double.

https://codereview.chromium.org/578263003/diff/1/mojo/python/tests/bindings_s...
mojo/python/tests/bindings_serialization_deserialization_unittest.py:59: if not
bool(bar1):
style: if not bar1:

When using an object in a boolean context, there is an implicit conversion to
bool type (well the same __function__ are tried as if the object was passed to
bool constructor), thus the bool(bar1) is not required.

https://codereview.chromium.org/578263003/diff/1/mojo/python/tests/bindings_s...
mojo/python/tests/bindings_serialization_deserialization_unittest.py:64:
properties = [ "alpha",
nit: you can use a tuple instead of a list here since this is immutable.

https://codereview.chromium.org/578263003/diff/1/mojo/python/tests/bindings_s...
mojo/python/tests/bindings_serialization_deserialization_unittest.py:69: for p
in properties:
You can extract this loop in a method.

BTW, if you don't mind using introspection, you can do something like:

properties = [ p for p in dir(bar1) if not p.startswith('__') or not
p.endswith('__') ]
for p in properties:
  ...

You can also deal with arrays by inspecting the values if you want.

https://codereview.chromium.org/578263003/diff/1/mojo/python/tests/bindings_s...
mojo/python/tests/bindings_serialization_deserialization_unittest.py:71: p2 =
getattr(bar1, p)
s/bar1/bar2/

https://codereview.chromium.org/578263003/diff/1/mojo/python/tests/bindings_s...
mojo/python/tests/bindings_serialization_deserialization_unittest.py:109:
"array_of_bools"
nit: missing trailing comma

https://codereview.chromium.org/578263003/diff/1/mojo/python/tests/bindings_s...
mojo/python/tests/bindings_serialization_deserialization_unittest.py:139:
self.assertTrue(_AreFooEquals(foo1, foo2))
Can you test failure to deserialization by passing an incorrect data?

[qsr, 2014-09-18 14:16:37]

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

https://codereview.chromium.org/578263003/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/descriptor.py:409: return None
On 2014/09/18 12:56:21, sdefresne wrote:
> Don't you want to validate that the value is 0?

 Not really. NoneType is not there to stay. But I have a couple of type that I
do not handle in the struct yet, mainly interfaces and interface request. So I
do not really care about this type.

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

https://codereview.chromium.org/578263003/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/reflection.py:112: fields = {}
On 2014/09/18 12:56:21, sdefresne wrote:
> Why not just doing result._fields = {} here? Nothing else has access to
result,
> so even if an exception is raised, the only way to access result in an
> inconsistent state would be to inspect the stack, which I don't expect anyone
> sane to do.

 Because pylint is a little dumb... If I try to access result._fields for
reading, it complains that I access the private member of an object... Here it
doesn't complain. As pylint is run as a preupload, I need it to be happy.

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

https://codereview.chromium.org/578263003/diff/1/mojo/python/tests/bindings_s...
mojo/python/tests/bindings_serialization_deserialization_unittest.py:20:
bar_instance = sample_service_mojom.Bar()
On 2014/09/18 12:56:22, sdefresne wrote:
> nit: I think you don't test serialization/deserialization of float/double.

 Added new test for this.

https://codereview.chromium.org/578263003/diff/1/mojo/python/tests/bindings_s...
mojo/python/tests/bindings_serialization_deserialization_unittest.py:59: if not
bool(bar1):
 Method doesn't exist anymore.

https://codereview.chromium.org/578263003/diff/1/mojo/python/tests/bindings_s...
mojo/python/tests/bindings_serialization_deserialization_unittest.py:64:
properties = [ "alpha",
On 2014/09/18 12:56:22, sdefresne wrote:
> nit: you can use a tuple instead of a list here since this is immutable.

 This doesn't exist anymore.

https://codereview.chromium.org/578263003/diff/1/mojo/python/tests/bindings_s...
mojo/python/tests/bindings_serialization_deserialization_unittest.py:69: for p
in properties:
 Changed equality checking by having a generic equality method.

https://codereview.chromium.org/578263003/diff/1/mojo/python/tests/bindings_s...
mojo/python/tests/bindings_serialization_deserialization_unittest.py:71: p2 =
getattr(bar1, p)
On 2014/09/18 12:56:22, sdefresne wrote:
> s/bar1/bar2/

 Not there anymore.

https://codereview.chromium.org/578263003/diff/1/mojo/python/tests/bindings_s...
mojo/python/tests/bindings_serialization_deserialization_unittest.py:109:
"array_of_bools"
On 2014/09/18 12:56:22, sdefresne wrote:
> nit: missing trailing comma

 Not there anymore.

https://codereview.chromium.org/578263003/diff/1/mojo/python/tests/bindings_s...
mojo/python/tests/bindings_serialization_deserialization_unittest.py:139:
self.assertTrue(_AreFooEquals(foo1, foo2))
On 2014/09/18 12:56:22, sdefresne wrote:
> Can you test failure to deserialization by passing an incorrect data?

Done.

[sdefresne, 2014-09-18 15:01:49]

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

https://codereview.chromium.org/578263003/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/descriptor.py:409: return None
On 2014/09/18 14:16:36, qsr wrote:
> On 2014/09/18 12:56:21, sdefresne wrote:
> > Don't you want to validate that the value is 0?
> 
>  Not really. NoneType is not there to stay. But I have a couple of type that I
> do not handle in the struct yet, mainly interfaces and interface request. So I
> do not really care about this type.

Acknowledged.

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

https://codereview.chromium.org/578263003/diff/1/mojo/public/python/mojo/bind...
mojo/public/python/mojo/bindings/reflection.py:112: fields = {}
On 2014/09/18 14:16:36, qsr wrote:
> On 2014/09/18 12:56:21, sdefresne wrote:
> > Why not just doing result._fields = {} here? Nothing else has access to
> result,
> > so even if an exception is raised, the only way to access result in an
> > inconsistent state would be to inspect the stack, which I don't expect
anyone
> > sane to do.
> 
>  Because pylint is a little dumb... If I try to access result._fields for
> reading, it complains that I access the private member of an object... Here it
> doesn't complain. As pylint is run as a preupload, I need it to be happy.

Acknowledged. You could do "result._fields = fields = {}". This is valid python
but people find it ugly, so I'm not recommending doing it here.

https://codereview.chromium.org/578263003/diff/20001/mojo/python/tests/bindin...
File mojo/python/tests/bindings_serialization_deserialization_unittest.py
(right):

https://codereview.chromium.org/578263003/diff/20001/mojo/python/tests/bindin...
mojo/python/tests/bindings_serialization_deserialization_unittest.py:23: def
_TestEquality(x, y):
This is much shorter and cleaner I think, thanks.

https://codereview.chromium.org/578263003/diff/20001/mojo/python/tests/bindin...
mojo/python/tests/bindings_serialization_deserialization_unittest.py:34: if
hasattr(x, '__iter__'):
You may want to use "hasattr(x, '__len__')" and change the error to "Sequences
are not the same size." as the iterator protocol does not require to support the
"len()" method (think about generator that are infinite).

Note that both tests (__len__ or __iter__) will also pass for dictionary, but
then you'll only end up comparing keys, not values. I don't think that mojo
supports map.

[qsr, 2014-09-18 15:11:36]

https://codereview.chromium.org/578263003/diff/20001/mojo/python/tests/bindin...
File mojo/python/tests/bindings_serialization_deserialization_unittest.py
(right):

https://codereview.chromium.org/578263003/diff/20001/mojo/python/tests/bindin...
mojo/python/tests/bindings_serialization_deserialization_unittest.py:23: def
_TestEquality(x, y):
On 2014/09/18 15:01:49, sdefresne wrote:
> This is much shorter and cleaner I think, thanks.

Acknowledged.

https://codereview.chromium.org/578263003/diff/20001/mojo/python/tests/bindin...
mojo/python/tests/bindings_serialization_deserialization_unittest.py:34: if
hasattr(x, '__iter__'):
 Changed for len. And yes, mojo doesn't support map at this point.

[sdefresne, 2014-09-18 15:13:56]

lgtm

[qsr, 2014-09-18 15:22:26]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-09-18 15:23:38]

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

[commit-bot: I haz the power, 2014-09-18 16:18:18]

Committed patchset #3 (id:40001) as b32765e60b3911df882c746cbdbccf66959efec0

[commit-bot: I haz the power, 2014-09-18 16:18:52]

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

[Elliot Glaysher, 2014-09-18 19:22:58]

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

The reason for reverting is: New unit tests fail.

https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests/builds/13695.