mojo: Adding struct's fields for python bindings

mojo: Adding struct's fields for python bindings

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

Committed: https://crrev.com/9c68b1e833e9192ad3a7299d0272de45a21199fd
Cr-Commit-Position: refs/heads/master@{#294177}

[qsr, 2014-09-09 08:20:43]

qsr@chromium.org changed reviewers:
+ sdefresne@chromium.org

[qsr, 2014-09-09 08:20:43]

+sdefresne

[sdefresne, 2014-09-09 09:24:45]

https://codereview.chromium.org/551053002/diff/20001/mojo/public/python/mojo/...
File mojo/public/python/mojo/bindings/descriptor.py (right):

https://codereview.chromium.org/551053002/diff/20001/mojo/public/python/mojo/...
mojo/public/python/mojo/bindings/descriptor.py:42: return 0
I think you want to do "value = 0" here, otherwise, the result may have an
inconsistent type when "value" is "None" versus when it is "0".

For BooleanType, GetDefaultValue(None) will return 0 but GetDefaultValue(0) will
return False. For FloatType, GetDefaultValue(None) will return 0, but
GetDefaultValue(0) will return 0.0.

I'm not sure this is really problematic or not, but since you're taking care to
convert the value to the correct type, I think it make sense to also have the
default value get the correct type.

https://codereview.chromium.org/551053002/diff/20001/mojo/public/python/mojo/...
mojo/public/python/mojo/bindings/descriptor.py:97: return unicode(value)
By default, if no encoding is specified, "unicode(value)" will assume that value
is in ascii which will cause exception to be raised if it has bytes above 0x7f.
Maybe you want to pass an encoding here (and make it configurable in the
constructor).

  if isinstance(value, str):
    return unicode(value, encoding='utf8')

https://codereview.chromium.org/551053002/diff/20001/mojo/public/python/mojo/...
mojo/public/python/mojo/bindings/descriptor.py:151: """Placeholder type, used
temporarly until all mojo types are handled."""
nit: temporarily

https://codereview.chromium.org/551053002/diff/20001/mojo/public/python/mojo/...
File mojo/public/python/mojo/bindings/reflection.py (right):

https://codereview.chromium.org/551053002/diff/20001/mojo/public/python/mojo/...
mojo/public/python/mojo/bindings/reflection.py:69: - 2 constants C1 and C2.
nit: when using a list, the usage is to end all lines by a comma or a
semi-colon, except for the last item that must end with a dot (unless the
sentence continue after the list).

https://codereview.chromium.org/551053002/diff/20001/mojo/public/python/mojo/...
mojo/public/python/mojo/bindings/reflection.py:91: def StructInit(self,
**kwargs):
nit: you could put this method at the top-level instead of declaring it in the
__new__ method as it does not capture anything.

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

https://codereview.chromium.org/551053002/diff/20001/mojo/python/tests/bindin...
mojo/python/tests/bindings_structs_unittest.py:152: # Structs
I don't see you testing nullable / non-nullable string/struct/... Is this an
omission or is it not yet supported?

[qsr, 2014-09-09 09:56:10]

https://chromiumcodereview.appspot.com/551053002/diff/20001/mojo/public/pytho...
File mojo/public/python/mojo/bindings/descriptor.py (right):

https://chromiumcodereview.appspot.com/551053002/diff/20001/mojo/public/pytho...
mojo/public/python/mojo/bindings/descriptor.py:42: return 0
On 2014/09/09 09:24:45, sdefresne wrote:
> I think you want to do "value = 0" here, otherwise, the result may have an
> inconsistent type when "value" is "None" versus when it is "0".
> 
> For BooleanType, GetDefaultValue(None) will return 0 but GetDefaultValue(0)
will
> return False. For FloatType, GetDefaultValue(None) will return 0, but
> GetDefaultValue(0) will return 0.0.

 BooleanType doesn't apply. It is not a numeric type. But 0.0 for FloatType
does, so modified.

> 
> I'm not sure this is really problematic or not, but since you're taking care
to
> convert the value to the correct type, I think it make sense to also have the
> default value get the correct type.

https://chromiumcodereview.appspot.com/551053002/diff/20001/mojo/public/pytho...
mojo/public/python/mojo/bindings/descriptor.py:97: return unicode(value)
On 2014/09/09 09:24:44, sdefresne wrote:
> By default, if no encoding is specified, "unicode(value)" will assume that
value
> is in ascii which will cause exception to be raised if it has bytes above
0x7f.
> Maybe you want to pass an encoding here (and make it configurable in the
> constructor).
> 
>   if isinstance(value, str):
>     return unicode(value, encoding='utf8')

 There is only one (two if you caount the nullable) instance of StringType, so I
cannot configure it.

 This conversion will happen when someone try to set a string value, I do not
really want to impose an encoding. So ascii is fine, and if it is not, the user
should give an unicode and do the conversion before setting the value. WDYT?

https://chromiumcodereview.appspot.com/551053002/diff/20001/mojo/public/pytho...
mojo/public/python/mojo/bindings/descriptor.py:151: """Placeholder type, used
temporarly until all mojo types are handled."""
On 2014/09/09 09:24:44, sdefresne wrote:
> nit: temporarily

Done.

https://chromiumcodereview.appspot.com/551053002/diff/20001/mojo/public/pytho...
File mojo/public/python/mojo/bindings/reflection.py (right):

https://chromiumcodereview.appspot.com/551053002/diff/20001/mojo/public/pytho...
mojo/public/python/mojo/bindings/reflection.py:69: - 2 constants C1 and C2.
On 2014/09/09 09:24:45, sdefresne wrote:
> nit: when using a list, the usage is to end all lines by a comma or a
> semi-colon, except for the last item that must end with a dot (unless the
> sentence continue after the list).

Done.

https://chromiumcodereview.appspot.com/551053002/diff/20001/mojo/public/pytho...
mojo/public/python/mojo/bindings/reflection.py:91: def StructInit(self,
**kwargs):
On 2014/09/09 09:24:45, sdefresne wrote:
> nit: you could put this method at the top-level instead of declaring it in the
> __new__ method as it does not capture anything.

Done.

https://chromiumcodereview.appspot.com/551053002/diff/20001/mojo/python/tests...
File mojo/python/tests/bindings_structs_unittest.py (right):

https://chromiumcodereview.appspot.com/551053002/diff/20001/mojo/python/tests...
mojo/python/tests/bindings_structs_unittest.py:152: # Structs
On 2014/09/09 09:24:45, sdefresne wrote:
> I don't see you testing nullable / non-nullable string/struct/... Is this an
> omission or is it not yet supported?

 nullable, non-nullable will only be tested at serialization/deserialization
time. As I do not handle this yet, this cannot be tested.

[sdefresne, 2014-09-09 12:22:48]

lgtm

https://chromiumcodereview.appspot.com/551053002/diff/20001/mojo/public/pytho...
File mojo/public/python/mojo/bindings/descriptor.py (right):

https://chromiumcodereview.appspot.com/551053002/diff/20001/mojo/public/pytho...
mojo/public/python/mojo/bindings/descriptor.py:97: return unicode(value)
On 2014/09/09 09:56:09, qsr wrote:
> On 2014/09/09 09:24:44, sdefresne wrote:
> > By default, if no encoding is specified, "unicode(value)" will assume that
> value
> > is in ascii which will cause exception to be raised if it has bytes above
> 0x7f.
> > Maybe you want to pass an encoding here (and make it configurable in the
> > constructor).
> > 
> >   if isinstance(value, str):
> >     return unicode(value, encoding='utf8')
> 
>  There is only one (two if you caount the nullable) instance of StringType, so
I
> cannot configure it.
> 
>  This conversion will happen when someone try to set a string value, I do not
> really want to impose an encoding. So ascii is fine, and if it is not, the
user
> should give an unicode and do the conversion before setting the value. WDYT?

SGTM. Maybe document that StringType store the value as a unicode, and that it
does the conversion using the default encoding if assigned a string instance.

https://chromiumcodereview.appspot.com/551053002/diff/20001/mojo/python/tests...
File mojo/python/tests/bindings_structs_unittest.py (right):

https://chromiumcodereview.appspot.com/551053002/diff/20001/mojo/python/tests...
mojo/python/tests/bindings_structs_unittest.py:152: # Structs
On 2014/09/09 09:56:09, qsr wrote:
> On 2014/09/09 09:24:45, sdefresne wrote:
> > I don't see you testing nullable / non-nullable string/struct/... Is this an
> > omission or is it not yet supported?
> 
>  nullable, non-nullable will only be tested at serialization/deserialization
> time. As I do not handle this yet, this cannot be tested.

SGTM.

https://chromiumcodereview.appspot.com/551053002/diff/40001/mojo/public/pytho...
File mojo/public/python/mojo/bindings/reflection.py (right):

https://chromiumcodereview.appspot.com/551053002/diff/40001/mojo/public/pytho...
mojo/public/python/mojo/bindings/reflection.py:5: """The meta classes used by
the mojo python bindings."""
nit: python documentation call them metaclasses, not sure what is the proper
spelling, leaving it to your appreciation.

https://chromiumcodereview.appspot.com/551053002/diff/40001/mojo/public/pytho...
mojo/public/python/mojo/bindings/reflection.py:71: - 1 int32 field named 'x'.
nit: why are you putting x in quote while leaving unquoted all other names (C1,
C2, ENUM1, ENUM2, V1, V2, value, ...)?

[qsr, 2014-09-09 12:44:26]

https://chromiumcodereview.appspot.com/551053002/diff/20001/mojo/public/pytho...
File mojo/public/python/mojo/bindings/descriptor.py (right):

https://chromiumcodereview.appspot.com/551053002/diff/20001/mojo/public/pytho...
mojo/public/python/mojo/bindings/descriptor.py:97: return unicode(value)
On 2014/09/09 12:22:48, sdefresne wrote:
> On 2014/09/09 09:56:09, qsr wrote:
> > On 2014/09/09 09:24:44, sdefresne wrote:
> > > By default, if no encoding is specified, "unicode(value)" will assume that
> > value
> > > is in ascii which will cause exception to be raised if it has bytes above
> > 0x7f.
> > > Maybe you want to pass an encoding here (and make it configurable in the
> > > constructor).
> > > 
> > >   if isinstance(value, str):
> > >     return unicode(value, encoding='utf8')
> > 
> >  There is only one (two if you caount the nullable) instance of StringType,
so
> I
> > cannot configure it.
> > 
> >  This conversion will happen when someone try to set a string value, I do
not
> > really want to impose an encoding. So ascii is fine, and if it is not, the
> user
> > should give an unicode and do the conversion before setting the value. WDYT?
> 
> SGTM. Maybe document that StringType store the value as a unicode, and that it
> does the conversion using the default encoding if assigned a string instance.

Done.

https://chromiumcodereview.appspot.com/551053002/diff/40001/mojo/public/pytho...
File mojo/public/python/mojo/bindings/reflection.py (right):

https://chromiumcodereview.appspot.com/551053002/diff/40001/mojo/public/pytho...
mojo/public/python/mojo/bindings/reflection.py:5: """The meta classes used by
the mojo python bindings."""
On 2014/09/09 12:22:48, sdefresne wrote:
> nit: python documentation call them metaclasses, not sure what is the proper
> spelling, leaving it to your appreciation.

 Changed.

https://chromiumcodereview.appspot.com/551053002/diff/40001/mojo/public/pytho...
mojo/public/python/mojo/bindings/reflection.py:71: - 1 int32 field named 'x'.
On 2014/09/09 12:22:48, sdefresne wrote:
> nit: why are you putting x in quote while leaving unquoted all other names
(C1,
> C2, ENUM1, ENUM2, V1, V2, value, ...)?

 Mainly because x was in lowercase, so harder to differentiate. Did quote
everything.

[qsr, 2014-09-10 13:16:33]

The CQ bit was checked by qsr@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/qsr@chromium.org/551053002/60001

[commit-bot: I haz the power, 2014-09-10 13:19:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-10 13:19:51]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[qsr, 2014-09-10 13:35:28]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-09-10 13:39:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/qsr@chromium.org/551053002/60001

[commit-bot: I haz the power, 2014-09-10 13:43:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-10 13:43:55]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[qsr, 2014-09-10 13:45:54]

Rebase

[qsr, 2014-09-10 13:51:39]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-09-10 13:51:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/qsr@chromium.org/551053002/80001

[commit-bot: I haz the power, 2014-09-10 15:44:33]

Committed patchset #5 (id:80001) as 0ecf01fc389046bc946f91f7e26180e004921e6d

[commit-bot: I haz the power, 2014-09-10 15:48:27]

Patchset 5 (id:??) landed as
https://crrev.com/9c68b1e833e9192ad3a7299d0272de45a21199fd
Cr-Commit-Position: refs/heads/master@{#294177}