Pepper: Autogenerate thunk for PPB_TrueTypeFont.

Pepper: Autogenerate thunk for PPB_TrueTypeFont.

This makes a few changes to IDL annotations:
  * 'singleton_resource' is now 'singleton' and can be per-function or per-interface.
  * There is a new 'API' annotation that describes when a specific API should be used for a function.
  * There is a 'thunk_include' annotation for when a header file must be included in the thunk output, but we can't guess it.

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

[teravest, 2013-04-10 16:29:06]

bbudge for TrueTypeFont changes and annotations
dmichael for ppapi OWNERS
yzshen for content/renderer/pepper/OWNERS

[teravest, 2013-04-10 16:59:20]

bbudge,
As a quick aside, if IsFont() was renamed to IsTrueTypeFont(), we wouldn't need
that funky [is_function] annotation. Not sure if that's something you can
change, though.

[yzshen1, 2013-04-10 17:00:48]

https://codereview.chromium.org/13820003/diff/5001/ppapi/api/dev/ppb_truetype...
File ppapi/api/dev/ppb_truetype_font_dev.idl (right):

https://codereview.chromium.org/13820003/diff/5001/ppapi/api/dev/ppb_truetype...
ppapi/api/dev/ppb_truetype_font_dev.idl:12:
[generate_thunk,thunk_include="ppapi/thunk/ppb_truetype_font_singleton_api.h"]
Does it make sense to deduce the include file name and omit the "thunk_include"
part? I mean, we could decide to include ppb_truetype_font_singleton_api.h when
we see [singleton,api=PPB_TrueTypeFont_Singleton_API].

https://codereview.chromium.org/13820003/diff/5001/ppapi/api/dev/ppb_truetype...
ppapi/api/dev/ppb_truetype_font_dev.idl:176: [is_function]
(1) Maybe we should change its name to IsTrueTypeFont() to make it conform to
the naming rule? I think we don't need the label in that case, right?

(2) If we decide not to do (1), maybe we could use a more descriptive label? For
example, 'resource_type_tester'. When I saw 'is_function', I thought it means
'this is a function'.

https://codereview.chromium.org/13820003/diff/5001/ppapi/api/ppb_gamepad.idl
File ppapi/api/ppb_gamepad.idl (right):

https://codereview.chromium.org/13820003/diff/5001/ppapi/api/ppb_gamepad.idl#...
ppapi/api/ppb_gamepad.idl:93: [singleton]
I understand that sometimes per-function annotation is necessary. However, I
think for most of interfaces, their methods are either all singleton or all
non-singleton. In that case, it would be more concise to do an interface-level
annotation. Does that make sense?

https://codereview.chromium.org/13820003/diff/5001/ppapi/thunk/ppb_truetype_f...
File ppapi/thunk/ppb_truetype_font_dev_thunk.cc (right):

https://codereview.chromium.org/13820003/diff/5001/ppapi/thunk/ppb_truetype_f...
ppapi/thunk/ppb_truetype_font_dev_thunk.cc:29: return
enter.SetResult(enter.functions()->GetFontFamilies(
Nice catch. :)

[teravest, 2013-04-10 17:06:53]

On Wed, Apr 10, 2013 at 11:00 AM,  <yzshen@chromium.org> wrote:
>
>
https://codereview.chromium.org/13820003/diff/5001/ppapi/api/dev/ppb_truetype...
> File ppapi/api/dev/ppb_truetype_font_dev.idl (right):
>
>
https://codereview.chromium.org/13820003/diff/5001/ppapi/api/dev/ppb_truetype...
> ppapi/api/dev/ppb_truetype_font_dev.idl:12:
>
[generate_thunk,thunk_include="ppapi/thunk/ppb_truetype_font_singleton_api.h"]
> Does it make sense to deduce the include file name and omit the
> "thunk_include" part? I mean, we could decide to include
> ppb_truetype_font_singleton_api.h when we see
> [singleton,api=PPB_TrueTypeFont_Singleton_API].

I wasn't sure I could come up with a rule to figure it out.
I left a TODO for myself to try to find a way to do this, but I don't
think I can reliably guess the underscore location(s) without renaming
some files.

>
>
https://codereview.chromium.org/13820003/diff/5001/ppapi/api/dev/ppb_truetype...
> ppapi/api/dev/ppb_truetype_font_dev.idl:176: [is_function]
> (1) Maybe we should change its name to IsTrueTypeFont() to make it
> conform to the naming rule? I think we don't need the label in that
> case, right?

I'd love to. Waiting to hear if bbudge is cool with this.

>
> (2) If we decide not to do (1), maybe we could use a more descriptive
> label? For example, 'resource_type_tester'. When I saw 'is_function', I
> thought it means 'this is a function'.

Agreed. If it has to stay as IsFont(), I'll rename it to resource_type_tester.

>
> https://codereview.chromium.org/13820003/diff/5001/ppapi/api/ppb_gamepad.idl
> File ppapi/api/ppb_gamepad.idl (right):
>
>
https://codereview.chromium.org/13820003/diff/5001/ppapi/api/ppb_gamepad.idl#...
> ppapi/api/ppb_gamepad.idl:93: [singleton]
> I understand that sometimes per-function annotation is necessary.
> However, I think for most of interfaces, their methods are either all
> singleton or all non-singleton. In that case, it would be more concise
> to do an interface-level annotation. Does that make sense?

Sure. I'll make it so "singleton" can annotate a method or an interface.
If an interface is annotated as "singleton", then all of its methods
will get the "singleton" behavior.

>
>
https://codereview.chromium.org/13820003/diff/5001/ppapi/thunk/ppb_truetype_f...
> File ppapi/thunk/ppb_truetype_font_dev_thunk.cc (right):
>
>
https://codereview.chromium.org/13820003/diff/5001/ppapi/thunk/ppb_truetype_f...
> ppapi/thunk/ppb_truetype_font_dev_thunk.cc:29: return
> enter.SetResult(enter.functions()->GetFontFamilies(
> Nice catch. :)
>
> https://codereview.chromium.org/13820003/

[bbudge, 2013-04-10 17:38:17]

LGTM

https://codereview.chromium.org/13820003/diff/5001/ppapi/api/dev/ppb_truetype...
File ppapi/api/dev/ppb_truetype_font_dev.idl (right):

https://codereview.chromium.org/13820003/diff/5001/ppapi/api/dev/ppb_truetype...
ppapi/api/dev/ppb_truetype_font_dev.idl:176: [is_function]
I think renaming to IsTrueTypeFont is a good idea. Hopefully we follow the
convention everywhere else so we wouldn't need this annotation. I doubt there
are many users of this API.
On 2013/04/10 17:00:48, yzshen1 wrote:
> (1) Maybe we should change its name to IsTrueTypeFont() to make it conform to
> the naming rule? I think we don't need the label in that case, right?
> 
> (2) If we decide not to do (1), maybe we could use a more descriptive label?
For
> example, 'resource_type_tester'. When I saw 'is_function', I thought it means
> 'this is a function'.

[bbudge, 2013-04-10 17:40:37]

https://codereview.chromium.org/13820003/diff/5001/ppapi/thunk/ppb_truetype_f...
File ppapi/thunk/ppb_truetype_font_dev_thunk.cc (right):

https://codereview.chromium.org/13820003/diff/5001/ppapi/thunk/ppb_truetype_f...
ppapi/thunk/ppb_truetype_font_dev_thunk.cc:29: return
enter.SetResult(enter.functions()->GetFontFamilies(
You might have to rebase - I added enter.SetResult in a follow on patch.
On 2013/04/10 17:00:48, yzshen1 wrote:
> Nice catch. :)

[teravest, 2013-04-10 18:11:09]

On Wed, Apr 10, 2013 at 11:40 AM,  <bbudge@chromium.org> wrote:
>
>
https://codereview.chromium.org/13820003/diff/5001/ppapi/thunk/ppb_truetype_f...
> File ppapi/thunk/ppb_truetype_font_dev_thunk.cc (right):
>
>
https://codereview.chromium.org/13820003/diff/5001/ppapi/thunk/ppb_truetype_f...
> ppapi/thunk/ppb_truetype_font_dev_thunk.cc:29: return
> enter.SetResult(enter.functions()->GetFontFamilies(
> You might have to rebase - I added enter.SetResult in a follow on patch.

Done.

>
> On 2013/04/10 17:00:48, yzshen1 wrote:
>>
>> Nice catch. :)
>
>
> https://codereview.chromium.org/13820003/

[teravest, 2013-04-10 18:12:06]

On Wed, Apr 10, 2013 at 11:06 AM, Justin TerAvest <teravest@chromium.org>
wrote:
> On Wed, Apr 10, 2013 at 11:00 AM,  <yzshen@chromium.org> wrote:
>>
>>
https://codereview.chromium.org/13820003/diff/5001/ppapi/api/dev/ppb_truetype...
>> File ppapi/api/dev/ppb_truetype_font_dev.idl (right):
>>
>>
https://codereview.chromium.org/13820003/diff/5001/ppapi/api/dev/ppb_truetype...
>> ppapi/api/dev/ppb_truetype_font_dev.idl:12:
>>
[generate_thunk,thunk_include="ppapi/thunk/ppb_truetype_font_singleton_api.h"]
>> Does it make sense to deduce the include file name and omit the
>> "thunk_include" part? I mean, we could decide to include
>> ppb_truetype_font_singleton_api.h when we see
>> [singleton,api=PPB_TrueTypeFont_Singleton_API].
>
> I wasn't sure I could come up with a rule to figure it out.
> I left a TODO for myself to try to find a way to do this, but I don't
> think I can reliably guess the underscore location(s) without renaming
> some files.
>
>>
>>
https://codereview.chromium.org/13820003/diff/5001/ppapi/api/dev/ppb_truetype...
>> ppapi/api/dev/ppb_truetype_font_dev.idl:176: [is_function]
>> (1) Maybe we should change its name to IsTrueTypeFont() to make it
>> conform to the naming rule? I think we don't need the label in that
>> case, right?
>
> I'd love to. Waiting to hear if bbudge is cool with this.

Changed to IsTrueTypeFont. Hooray.

>
>>
>> (2) If we decide not to do (1), maybe we could use a more descriptive
>> label? For example, 'resource_type_tester'. When I saw 'is_function', I
>> thought it means 'this is a function'.
>
> Agreed. If it has to stay as IsFont(), I'll rename it to
resource_type_tester.
>
>>
>> https://codereview.chromium.org/13820003/diff/5001/ppapi/api/ppb_gamepad.idl
>> File ppapi/api/ppb_gamepad.idl (right):
>>
>>
https://codereview.chromium.org/13820003/diff/5001/ppapi/api/ppb_gamepad.idl#...
>> ppapi/api/ppb_gamepad.idl:93: [singleton]
>> I understand that sometimes per-function annotation is necessary.
>> However, I think for most of interfaces, their methods are either all
>> singleton or all non-singleton. In that case, it would be more concise
>> to do an interface-level annotation. Does that make sense?
>
> Sure. I'll make it so "singleton" can annotate a method or an interface.
> If an interface is annotated as "singleton", then all of its methods
> will get the "singleton" behavior.

Done.

>
>>
>>
https://codereview.chromium.org/13820003/diff/5001/ppapi/thunk/ppb_truetype_f...
>> File ppapi/thunk/ppb_truetype_font_dev_thunk.cc (right):
>>
>>
https://codereview.chromium.org/13820003/diff/5001/ppapi/thunk/ppb_truetype_f...
>> ppapi/thunk/ppb_truetype_font_dev_thunk.cc:29: return
>> enter.SetResult(enter.functions()->GetFontFamilies(
>> Nice catch. :)
>>
>> https://codereview.chromium.org/13820003/

[dmichael (off chromium), 2013-04-10 18:30:36]

lgtm

[yzshen1, 2013-04-10 18:33:06]

LGTM

Thanks!

[commit-bot: I haz the power, 2013-04-10 19:40:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/13820003/14002

[commit-bot: I haz the power, 2013-04-10 19:40:49]

Presubmit check for 13820003-14002 failed and returned exit status 1.

INFO:root:Found 13 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/ppapi/PRESUBMIT.py

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit Warnings **
Missing PPAPI header, no change or skipped generation?

***************
ppapi/api/ppb_gamepad.idl
***************

Presubmit checks took 4.8s to calculate.