Add a Pepper IDL generator to the JSON schema compiler.

Add a Pepper IDL generator to the JSON schema compiler.

BUG=326867
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240297

[not at google - send to devlin, 2013-12-09 23:30:58]

structure lgtm, just a few nitty python comments. I didn't look at the actual
code that gets generated in any way.

https://codereview.chromium.org/101483003/diff/100001/tools/json_schema_compi...
File tools/json_schema_compiler/ppapi_generator.py (right):

https://codereview.chromium.org/101483003/diff/100001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator.py:20: class
_PpapiGeneratorBase(object):
docstring

https://codereview.chromium.org/101483003/diff/100001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator.py:33: raise jinja2_error
this will reset the stack trace. if you want the original stack trace then you
need to assign jing2_error to sys.exc_info() IIRC not e.

https://codereview.chromium.org/101483003/diff/100001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator.py:49:
self.jinja_environment.filters['needs_optional_array'] = (
nit: self.jinja_environment.filters.update({
  'ppapi_type': ...
})

https://codereview.chromium.org/101483003/diff/100001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator.py:62: 'name': self._namespace.name,
nit: only 2 indent for object declarations not 4. elsewhere too.

https://codereview.chromium.org/101483003/diff/100001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator.py:82: for function in
self._namespace.functions.values():
itervalue)

and iterfoo() for everywhere else it makes sense to (most of these)

https://codereview.chromium.org/101483003/diff/100001/tools/json_schema_compi...
File tools/json_schema_compiler/ppapi_generator_test.py (right):

https://codereview.chromium.org/101483003/diff/100001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator_test.py:40:
self.assertLess(typename_to_index['MyType1'], typename_to_index['MyType2'])
assertLess is python 2.7 which has been a problem in the past (for windows,
which uses 2.6), though I saw a post saying we were going to upgrade that. just
a warning. this is probably ok to leave for now.

[Sam McNally, 2013-12-10 09:41:15]

https://codereview.chromium.org/101483003/diff/100001/tools/json_schema_compi...
File tools/json_schema_compiler/ppapi_generator.py (right):

https://codereview.chromium.org/101483003/diff/100001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator.py:20: class
_PpapiGeneratorBase(object):
On 2013/12/09 23:30:58, kalman wrote:
> docstring

Done.

https://codereview.chromium.org/101483003/diff/100001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator.py:33: raise jinja2_error
On 2013/12/09 23:30:58, kalman wrote:
> this will reset the stack trace. if you want the original stack trace then you
> need to assign jing2_error to sys.exc_info() IIRC not e.

Done.

https://codereview.chromium.org/101483003/diff/100001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator.py:49:
self.jinja_environment.filters['needs_optional_array'] = (
On 2013/12/09 23:30:58, kalman wrote:
> nit: self.jinja_environment.filters.update({
>   'ppapi_type': ...
> })

Done.

https://codereview.chromium.org/101483003/diff/100001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator.py:62: 'name': self._namespace.name,
On 2013/12/09 23:30:58, kalman wrote:
> nit: only 2 indent for object declarations not 4. elsewhere too.

Done.

https://codereview.chromium.org/101483003/diff/100001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator.py:82: for function in
self._namespace.functions.values():
On 2013/12/09 23:30:58, kalman wrote:
> itervalue)
> 
> and iterfoo() for everywhere else it makes sense to (most of these)

Done.

[yzshen1, 2013-12-10 21:43:01]

Thanks, Sam!

This mostly looks good.

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
File tools/json_schema_compiler/compiler.py (right):

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
tools/json_schema_compiler/compiler.py:42: is_ppapi_dev=False):
FYI: very soon, pepper will switch to a new 'dev' design: no _dev suffix or dev/
folder, but only have [channel=dev] annotation for versions.

Please see the design:
https://docs.google.com/a/google.com/document/d/1Q660kK72_230gxnqtgQavMcDT5_g...

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
File tools/json_schema_compiler/ppapi_generator.py (right):

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator.py:73: c = code.Code()
nit: maybe consider a more descriptive variable name?

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator.py:87: 'year':
datetime.date.today().year,
nit: Please note that when we modify an existing file, we don't change the year
information.
For example, in 2012 we run this script to create ppb_foo.idl, and in 2013 if we
run the script to update it, it is better to not change the year in the
copyright text.

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator.py:143: def
_RegisterTypeDependency(self, type_, depender, optional, array):
I noticed that you use 'type_' in quite some places, is it intended to have the
trailing underscore?

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
File tools/json_schema_compiler/ppapi_generator_test.py (right):

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator_test.py:111:
self.assertEqual('PP_Idl_pepper_MyType0', self.generator.FormatParamType(
I think the type of |struct_single| should be PP_IdlPepper_MyType0.

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator_test.py:117:
'PP_Optional_Idl_pepper_MyType0', self.generator.FormatParamType(
And here maybe it is better to be "PP_IdlPepper_Optional_MyType0".

"PP" is the namespace of pepper.
"IdlPepper" is the namespace of all types for idl_pepper.
I think it is better to put them before "optional".

What do you think?

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator_test.py:170:
self.assertEqual('PP_IDL_PEPPER_ENUMTYPE_NAME1',
I think it is better to use PP_IDLPEPPER_ENUMTYPE_NAME1 (removing the underscore
between IDL and PEPPER).

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
File tools/json_schema_compiler/templates/ppapi/idl.template (right):

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
tools/json_schema_compiler/templates/ppapi/idl.template:36: M33 = 0.1
[no need to make change right now]
The version information we probably need to define manually somewhere, maybe in
the apps IDL.

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
tools/json_schema_compiler/templates/ppapi/idl.template:59: typedef uint32_t {{
event | ppapi_type }}(
Event callback will return void.

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
tools/json_schema_compiler/templates/ppapi/idl.template:63: [inout] {{ param |
format_param_type }} {{ param.unix_name }}
I think other params are all [in].

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
File tools/json_schema_compiler/test/idl_pepper.idl (right):

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
tools/json_schema_compiler/test/idl_pepper.idl:87: static void function12(MyType
arg);
is it intentional to have the same func name as the previous one?

[yzshen1, 2013-12-11 01:10:34]

Adding Justin who has worked on Pepper auto-gen tools.

And I think we probably would like to make the generator output new-style dev
APIs. What do you think Justin?

[Sam McNally, 2013-12-11 08:02:38]

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
File tools/json_schema_compiler/ppapi_generator.py (right):

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator.py:73: c = code.Code()
On 2013/12/10 21:43:02, yzshen1 wrote:
> nit: maybe consider a more descriptive variable name?

Done.

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator.py:87: 'year':
datetime.date.today().year,
On 2013/12/10 21:43:02, yzshen1 wrote:
> nit: Please note that when we modify an existing file, we don't change the
year
> information.
> For example, in 2012 we run this script to create ppb_foo.idl, and in 2013 if
we
> run the script to update it, it is better to not change the year in the
> copyright text.

Added a TODO. As long as the first generated file is checked in next year we'll
have plenty of time to fix it :)

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator.py:143: def
_RegisterTypeDependency(self, type_, depender, optional, array):
On 2013/12/10 21:43:02, yzshen1 wrote:
> I noticed that you use 'type_' in quite some places, is it intended to have
the
> trailing underscore?

Yes. This is to avoid clashing with the global "type" type.

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
File tools/json_schema_compiler/ppapi_generator_test.py (right):

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator_test.py:111:
self.assertEqual('PP_Idl_pepper_MyType0', self.generator.FormatParamType(
On 2013/12/10 21:43:02, yzshen1 wrote:
> I think the type of |struct_single| should be PP_IdlPepper_MyType0.

Done.

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator_test.py:117:
'PP_Optional_Idl_pepper_MyType0', self.generator.FormatParamType(
On 2013/12/10 21:43:02, yzshen1 wrote:
> And here maybe it is better to be "PP_IdlPepper_Optional_MyType0".
> 
> "PP" is the namespace of pepper.
> "IdlPepper" is the namespace of all types for idl_pepper.
> I think it is better to put them before "optional".
> 
> What do you think?

Done.

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator_test.py:170:
self.assertEqual('PP_IDL_PEPPER_ENUMTYPE_NAME1',
On 2013/12/10 21:43:02, yzshen1 wrote:
> I think it is better to use PP_IDLPEPPER_ENUMTYPE_NAME1 (removing the
underscore
> between IDL and PEPPER).

Done.

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
File tools/json_schema_compiler/templates/ppapi/idl.template (right):

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
tools/json_schema_compiler/templates/ppapi/idl.template:36: M33 = 0.1
On 2013/12/10 21:43:02, yzshen1 wrote:
> [no need to make change right now]
> The version information we probably need to define manually somewhere, maybe
in
> the apps IDL.

Added a TODO.

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
tools/json_schema_compiler/templates/ppapi/idl.template:59: typedef uint32_t {{
event | ppapi_type }}(
On 2013/12/10 21:43:02, yzshen1 wrote:
> Event callback will return void.

Done.

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
tools/json_schema_compiler/templates/ppapi/idl.template:63: [inout] {{ param |
format_param_type }} {{ param.unix_name }}
On 2013/12/10 21:43:02, yzshen1 wrote:
> I think other params are all [in].

Done.

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
File tools/json_schema_compiler/test/idl_pepper.idl (right):

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
tools/json_schema_compiler/test/idl_pepper.idl:87: static void function12(MyType
arg);
On 2013/12/10 21:43:02, yzshen1 wrote:
> is it intentional to have the same func name as the previous one?

Done.

[teravest, 2013-12-11 16:32:57]

lgtm

This looks okay to me, though I'm no expert at reading jinja2 templates.

https://codereview.chromium.org/101483003/diff/160001/tools/json_schema_compi...
File tools/json_schema_compiler/ppapi_generator.py (right):

https://codereview.chromium.org/101483003/diff/160001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator.py:15: import jinja2
Shouldn't jinja2 always be available to be imported from third_party?

[Sam McNally, 2013-12-11 22:35:19]

https://codereview.chromium.org/101483003/diff/160001/tools/json_schema_compi...
File tools/json_schema_compiler/ppapi_generator.py (right):

https://codereview.chromium.org/101483003/diff/160001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator.py:15: import jinja2
On 2013/12/11 16:32:57, teravest wrote:
> Shouldn't jinja2 always be available to be imported from third_party?

Done.

[yzshen1, 2013-12-11 22:55:35]

LGTM

Thanks!

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
File tools/json_schema_compiler/ppapi_generator.py (right):

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator.py:87: 'year':
datetime.date.today().year,
On 2013/12/11 08:02:38, Sam McNally wrote:
> On 2013/12/10 21:43:02, yzshen1 wrote:
> > nit: Please note that when we modify an existing file, we don't change the
> year
> > information.
> > For example, in 2012 we run this script to create ppb_foo.idl, and in 2013
if
> we
> > run the script to update it, it is better to not change the year in the
> > copyright text.
> 
> Added a TODO. As long as the first generated file is checked in next year
we'll
> have plenty of time to fix it :)

Indeed!

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator.py:143: def
_RegisterTypeDependency(self, type_, depender, optional, array):
(I don't know.) Is it a common practice to use "type_"? If not we could consider
a different name such as "data_type".

On 2013/12/11 08:02:38, Sam McNally wrote:
> On 2013/12/10 21:43:02, yzshen1 wrote:
> > I noticed that you use 'type_' in quite some places, is it intended to have
> the
> > trailing underscore?
> 
> Yes. This is to avoid clashing with the global "type" type.

[Sam McNally, 2013-12-11 23:02:20]

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
File tools/json_schema_compiler/ppapi_generator.py (right):

https://codereview.chromium.org/101483003/diff/120001/tools/json_schema_compi...
tools/json_schema_compiler/ppapi_generator.py:143: def
_RegisterTypeDependency(self, type_, depender, optional, array):
type_ is used in the rest of the JSON schema compiler.

On 2013/12/11 22:55:35, yzshen1 wrote:
> (I don't know.) Is it a common practice to use "type_"? If not we could
consider
> a different name such as "data_type".
> 
> On 2013/12/11 08:02:38, Sam McNally wrote:
> > On 2013/12/10 21:43:02, yzshen1 wrote:
> > > I noticed that you use 'type_' in quite some places, is it intended to
have
> > the
> > > trailing underscore?
> > 
> > Yes. This is to avoid clashing with the global "type" type.
>

[commit-bot: I haz the power, 2013-12-11 23:04:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/101483003/200001

[commit-bot: I haz the power, 2013-12-12 00:30:58]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-12-12 02:36:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/101483003/220001

[commit-bot: I haz the power, 2013-12-12 05:03:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/101483003/240001

[commit-bot: I haz the power, 2013-12-12 13:28:09]

Change committed as 240297