Convert a few ppapi dev interfaces to IDL. NaCl has tests for these

Convert a few ppapi dev interfaces to IDL. NaCl has tests for these
dev interfaces, and we want it to go through IDL so that the PNaCl shim
will know about them. There are few others dev and private interfaces
not yet converted to IDL, but this is a start.

TEST= ./generator.py
BUG= http://code.google.com/p/chromium/issues/detail?id=89968

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110777

[jvoung - send to chromium..., 2011-11-17 18:47:56]

+ dmichael -- Is it okay to remove the 0.6 version of ppb_testing_dev, or are
there still dependencies?

[dmichael (off chromium), 2011-11-17 19:04:41]

It is okay to get rid of the 0.6 version of Testing (though you probably also
have to get rid of the part(s) of the code that use that macro; maybe
webkit/plugins/ppapi/plugin_module.cc).

PDF and Flash are still in a separate repo and roll DEPS to get the latest PPAPI
frequently; stuff like this only needs to stick around for a few days so PDF and
Flash can catch up. Published interfaces, of course, need to keep backwards
compat for...  a long time.

http://codereview.chromium.org/8586031/diff/3002/ppapi/api/dev/ppb_cursor_con...
File ppapi/api/dev/ppb_cursor_control_dev.idl (right):

http://codereview.chromium.org/8586031/diff/3002/ppapi/api/dev/ppb_cursor_con...
ppapi/api/dev/ppb_cursor_control_dev.idl:25: [in] PP_Point hot_spot);
nit: Why not use C++-style indentation?

(I saw somebody else doing this style in their IDL; if there's a good reason for
it, please let me know so I can stop telling people the wrong thing :-) )

http://codereview.chromium.org/8586031/diff/3002/ppapi/c/dev/ppb_cursor_contr...
File ppapi/c/dev/ppb_cursor_control_dev.h (right):

http://codereview.chromium.org/8586031/diff/3002/ppapi/c/dev/ppb_cursor_contr...
ppapi/c/dev/ppb_cursor_control_dev.h:20: #define PPB_CURSORCONTROL_DEV_INTERFACE
PPB_CURSORCONTROL_DEV_INTERFACE_0_4
This is a change to the macro for the interface string. We probably shouldn't
change it unnecessarily. There's some kind of 'macro' annotation you can use to
keep it the same.

noelallen: This came up elsewhere...  can we make the IDL tool put _ between
words that are CamelCase in the interface name? I like the CURSOR_CONTROL style
better, personally, and I think it would be ideal if the tool does it
automatically.

http://codereview.chromium.org/8586031/diff/3002/ppapi/c/dev/ppp_printing_dev.h
File ppapi/c/dev/ppp_printing_dev.h (right):

http://codereview.chromium.org/8586031/diff/3002/ppapi/c/dev/ppp_printing_dev...
ppapi/c/dev/ppp_printing_dev.h:45: PP_PRINTOUTPUTFORMAT_EMF = 1 << 3
yipe... we should probably still use unsigned literals here, since we're
shifting and these are used in a uint32_t bitfield (though it *probably* doesn't
matter in this case; not sure)

[jvoung - send to chromium..., 2011-11-17 20:51:34]

Thanks for the review!

re: 0.6 dev testing interface

Within webkit/plugins/ppapi/plugin_module.cc it refers to the version without
0_6 or 0_7.

Also, the original .h file doesn't seem to indicate how 0_6 is different for
0_7, so when I ported it to IDL, it could not tell the difference and seems to
"optimize out" 0_7.

I'll run a try job to check also. So far, grep hasn't showed anything explicitly
requesting 0_6.

http://codereview.chromium.org/8586031/diff/3002/ppapi/api/dev/ppb_cursor_con...
File ppapi/api/dev/ppb_cursor_control_dev.idl (right):

http://codereview.chromium.org/8586031/diff/3002/ppapi/api/dev/ppb_cursor_con...
ppapi/api/dev/ppb_cursor_control_dev.idl:25: [in] PP_Point hot_spot);
On 2011/11/17 19:04:41, dmichael wrote:
> nit: Why not use C++-style indentation?
> 
> (I saw somebody else doing this style in their IDL; if there's a good reason
for
> it, please let me know so I can stop telling people the wrong thing :-) )

Oh, I guess I got lucky and picked an example IDL file that happened to have
this style. From my unscientific survey, I would say it's about 50-50 in each
style. Should I change or leave it alone?

http://codereview.chromium.org/8586031/diff/3002/ppapi/c/dev/ppb_cursor_contr...
File ppapi/c/dev/ppb_cursor_control_dev.h (right):

http://codereview.chromium.org/8586031/diff/3002/ppapi/c/dev/ppb_cursor_contr...
ppapi/c/dev/ppb_cursor_control_dev.h:20: #define PPB_CURSORCONTROL_DEV_INTERFACE
PPB_CURSORCONTROL_DEV_INTERFACE_0_4
On 2011/11/17 19:04:41, dmichael wrote:
> This is a change to the macro for the interface string. We probably shouldn't
> change it unnecessarily. There's some kind of 'macro' annotation you can use
to
> keep it the same.
> 
> noelallen: This came up elsewhere...  can we make the IDL tool put _ between
> words that are CamelCase in the interface name? I like the CURSOR_CONTROL
style
> better, personally, and I think it would be ideal if the tool does it
> automatically.

Ah thanks, found the [macro="..."] option

http://codereview.chromium.org/8586031/diff/3002/ppapi/c/dev/ppp_printing_dev.h
File ppapi/c/dev/ppp_printing_dev.h (right):

http://codereview.chromium.org/8586031/diff/3002/ppapi/c/dev/ppp_printing_dev...
ppapi/c/dev/ppp_printing_dev.h:45: PP_PRINTOUTPUTFORMAT_EMF = 1 << 3
On 2011/11/17 19:04:41, dmichael wrote:
> yipe... we should probably still use unsigned literals here, since we're
> shifting and these are used in a uint32_t bitfield (though it *probably*
doesn't
> matter in this case; not sure)

Left shifts should be okay, right (no sign extension issues)?

[dmichael (off chromium), 2011-11-17 21:05:49]

http://codereview.chromium.org/8586031/diff/3002/ppapi/api/dev/ppb_cursor_con...
File ppapi/api/dev/ppb_cursor_control_dev.idl (right):

http://codereview.chromium.org/8586031/diff/3002/ppapi/api/dev/ppb_cursor_con...
ppapi/api/dev/ppb_cursor_control_dev.idl:25: [in] PP_Point hot_spot);
On 2011/11/17 20:51:34, jvoung wrote:
> On 2011/11/17 19:04:41, dmichael wrote:
> > nit: Why not use C++-style indentation?
> > 
> > (I saw somebody else doing this style in their IDL; if there's a good reason
> for
> > it, please let me know so I can stop telling people the wrong thing :-) )
> 
> Oh, I guess I got lucky and picked an example IDL file that happened to have
> this style. From my unscientific survey, I would say it's about 50-50 in each
> style. Should I change or leave it alone?
If we're ~50-50, and there's no good reason to do it the above way (Noel??), I'm
going to push for following C++ style for any CLs I review. Thanks for being
accommodating :-)

http://codereview.chromium.org/8586031/diff/3002/ppapi/c/dev/ppp_printing_dev.h
File ppapi/c/dev/ppp_printing_dev.h (right):

http://codereview.chromium.org/8586031/diff/3002/ppapi/c/dev/ppp_printing_dev...
ppapi/c/dev/ppp_printing_dev.h:45: PP_PRINTOUTPUTFORMAT_EMF = 1 << 3
On 2011/11/17 20:51:34, jvoung wrote:
> On 2011/11/17 19:04:41, dmichael wrote:
> > yipe... we should probably still use unsigned literals here, since we're
> > shifting and these are used in a uint32_t bitfield (though it *probably*
> doesn't
> > matter in this case; not sure)
> 
> Left shifts should be okay, right (no sign extension issues)?
Yeah, it should give the value you want. I was concerned about the type. enums
always have an integral type, but *what* integral type it is can depend on the
value. These are used in a context where uint32_t is desired (they're or-ed
together), so I'd rather the enum values have the right type. That said...  I
don't think we really know what type the enum uses in this case (though you can
force it by making it store a std::numeric_limits::max<uint32_t>(), for
example). So we probably have to cast to uint32_t where these are used anyway,
hence, it *probably* doesn't matter.

All that said, even if it works fine, I don't really see the point in changing
it from unsigned (the type we ultimately want) to signed.

[jvoung - send to chromium..., 2011-11-17 22:00:09]

http://codereview.chromium.org/8586031/diff/3002/ppapi/api/dev/ppb_cursor_con...
File ppapi/api/dev/ppb_cursor_control_dev.idl (right):

http://codereview.chromium.org/8586031/diff/3002/ppapi/api/dev/ppb_cursor_con...
ppapi/api/dev/ppb_cursor_control_dev.idl:25: [in] PP_Point hot_spot);
On 2011/11/17 21:05:49, dmichael wrote:
> On 2011/11/17 20:51:34, jvoung wrote:
> > On 2011/11/17 19:04:41, dmichael wrote:
> > > nit: Why not use C++-style indentation?
> > > 
> > > (I saw somebody else doing this style in their IDL; if there's a good
reason
> > for
> > > it, please let me know so I can stop telling people the wrong thing :-) )
> > 
> > Oh, I guess I got lucky and picked an example IDL file that happened to have
> > this style. From my unscientific survey, I would say it's about 50-50 in
each
> > style. Should I change or leave it alone?
> If we're ~50-50, and there's no good reason to do it the above way (Noel??),
I'm
> going to push for following C++ style for any CLs I review. Thanks for being
> accommodating :-)

Ok, done =)

http://codereview.chromium.org/8586031/diff/3002/ppapi/c/dev/ppp_printing_dev.h
File ppapi/c/dev/ppp_printing_dev.h (right):

http://codereview.chromium.org/8586031/diff/3002/ppapi/c/dev/ppp_printing_dev...
ppapi/c/dev/ppp_printing_dev.h:45: PP_PRINTOUTPUTFORMAT_EMF = 1 << 3
On 2011/11/17 21:05:49, dmichael wrote:
> On 2011/11/17 20:51:34, jvoung wrote:
> > On 2011/11/17 19:04:41, dmichael wrote:
> > > yipe... we should probably still use unsigned literals here, since we're
> > > shifting and these are used in a uint32_t bitfield (though it *probably*
> > doesn't
> > > matter in this case; not sure)
> > 
> > Left shifts should be okay, right (no sign extension issues)?
> Yeah, it should give the value you want. I was concerned about the type. enums
> always have an integral type, but *what* integral type it is can depend on the
> value. These are used in a context where uint32_t is desired (they're or-ed
> together), so I'd rather the enum values have the right type. That said...  I
> don't think we really know what type the enum uses in this case (though you
can
> force it by making it store a std::numeric_limits::max<uint32_t>(), for
> example). So we probably have to cast to uint32_t where these are used anyway,
> hence, it *probably* doesn't matter.
> 
> All that said, even if it works fine, I don't really see the point in changing
> it from unsigned (the type we ultimately want) to signed.

Done.

[jvoung - send to chromium..., 2011-11-17 22:06:57]

On 2011/11/17 22:00:09, jvoung wrote:
>
http://codereview.chromium.org/8586031/diff/3002/ppapi/api/dev/ppb_cursor_con...
> File ppapi/api/dev/ppb_cursor_control_dev.idl (right):
> 
>
http://codereview.chromium.org/8586031/diff/3002/ppapi/api/dev/ppb_cursor_con...
> ppapi/api/dev/ppb_cursor_control_dev.idl:25: [in] PP_Point hot_spot);
> On 2011/11/17 21:05:49, dmichael wrote:
> > On 2011/11/17 20:51:34, jvoung wrote:
> > > On 2011/11/17 19:04:41, dmichael wrote:
> > > > nit: Why not use C++-style indentation?
> > > > 
> > > > (I saw somebody else doing this style in their IDL; if there's a good
> reason
> > > for
> > > > it, please let me know so I can stop telling people the wrong thing :-)
)
> > > 
> > > Oh, I guess I got lucky and picked an example IDL file that happened to
have
> > > this style. From my unscientific survey, I would say it's about 50-50 in
> each
> > > style. Should I change or leave it alone?
> > If we're ~50-50, and there's no good reason to do it the above way (Noel??),
> I'm
> > going to push for following C++ style for any CLs I review. Thanks for being
> > accommodating :-)
> 
> Ok, done =)
> 
>
http://codereview.chromium.org/8586031/diff/3002/ppapi/c/dev/ppp_printing_dev.h
> File ppapi/c/dev/ppp_printing_dev.h (right):
> 
>
http://codereview.chromium.org/8586031/diff/3002/ppapi/c/dev/ppp_printing_dev...
> ppapi/c/dev/ppp_printing_dev.h:45: PP_PRINTOUTPUTFORMAT_EMF = 1 << 3
> On 2011/11/17 21:05:49, dmichael wrote:
> > On 2011/11/17 20:51:34, jvoung wrote:
> > > On 2011/11/17 19:04:41, dmichael wrote:
> > > > yipe... we should probably still use unsigned literals here, since we're
> > > > shifting and these are used in a uint32_t bitfield (though it *probably*
> > > doesn't
> > > > matter in this case; not sure)
> > > 
> > > Left shifts should be okay, right (no sign extension issues)?
> > Yeah, it should give the value you want. I was concerned about the type.
enums
> > always have an integral type, but *what* integral type it is can depend on
the
> > value. These are used in a context where uint32_t is desired (they're or-ed
> > together), so I'd rather the enum values have the right type. That said... 
I
> > don't think we really know what type the enum uses in this case (though you
> can
> > force it by making it store a std::numeric_limits::max<uint32_t>(), for
> > example). So we probably have to cast to uint32_t where these are used
anyway,
> > hence, it *probably* doesn't matter.
> > 
> > All that said, even if it works fine, I don't really see the point in
changing
> > it from unsigned (the type we ultimately want) to signed.
> 
> Done.

Ah... sorry please hold on the review. Some issues with non-typedef'ed enums
crept out.

[jvoung - send to chromium..., 2011-11-18 00:30:34]

On 2011/11/17 22:06:57, jvoung wrote:
> On 2011/11/17 22:00:09, jvoung wrote:
> >
>
http://codereview.chromium.org/8586031/diff/3002/ppapi/api/dev/ppb_cursor_con...
> > File ppapi/api/dev/ppb_cursor_control_dev.idl (right):
> > 
> >
>
http://codereview.chromium.org/8586031/diff/3002/ppapi/api/dev/ppb_cursor_con...
> > ppapi/api/dev/ppb_cursor_control_dev.idl:25: [in] PP_Point hot_spot);
> > On 2011/11/17 21:05:49, dmichael wrote:
> > > On 2011/11/17 20:51:34, jvoung wrote:
> > > > On 2011/11/17 19:04:41, dmichael wrote:
> > > > > nit: Why not use C++-style indentation?
> > > > > 
> > > > > (I saw somebody else doing this style in their IDL; if there's a good
> > reason
> > > > for
> > > > > it, please let me know so I can stop telling people the wrong thing
:-)
> )
> > > > 
> > > > Oh, I guess I got lucky and picked an example IDL file that happened to
> have
> > > > this style. From my unscientific survey, I would say it's about 50-50 in
> > each
> > > > style. Should I change or leave it alone?
> > > If we're ~50-50, and there's no good reason to do it the above way
(Noel??),
> > I'm
> > > going to push for following C++ style for any CLs I review. Thanks for
being
> > > accommodating :-)
> > 
> > Ok, done =)
> > 
> >
>
http://codereview.chromium.org/8586031/diff/3002/ppapi/c/dev/ppp_printing_dev.h
> > File ppapi/c/dev/ppp_printing_dev.h (right):
> > 
> >
>
http://codereview.chromium.org/8586031/diff/3002/ppapi/c/dev/ppp_printing_dev...
> > ppapi/c/dev/ppp_printing_dev.h:45: PP_PRINTOUTPUTFORMAT_EMF = 1 << 3
> > On 2011/11/17 21:05:49, dmichael wrote:
> > > On 2011/11/17 20:51:34, jvoung wrote:
> > > > On 2011/11/17 19:04:41, dmichael wrote:
> > > > > yipe... we should probably still use unsigned literals here, since
we're
> > > > > shifting and these are used in a uint32_t bitfield (though it
*probably*
> > > > doesn't
> > > > > matter in this case; not sure)
> > > > 
> > > > Left shifts should be okay, right (no sign extension issues)?
> > > Yeah, it should give the value you want. I was concerned about the type.
> enums
> > > always have an integral type, but *what* integral type it is can depend on
> the
> > > value. These are used in a context where uint32_t is desired (they're
or-ed
> > > together), so I'd rather the enum values have the right type. That said...

> I
> > > don't think we really know what type the enum uses in this case (though
you
> > can
> > > force it by making it store a std::numeric_limits::max<uint32_t>(), for
> > > example). So we probably have to cast to uint32_t where these are used
> anyway,
> > > hence, it *probably* doesn't matter.
> > > 
> > > All that said, even if it works fine, I don't really see the point in
> changing
> > > it from unsigned (the type we ultimately want) to signed.
> > 
> > Done.
> 
> Ah... sorry please hold on the review. Some issues with non-typedef'ed enums
> crept out.

Ok, I added a "notypedef" property to allow the header to have a "enum X {...};"
vs "typedef enum { ... } X;"  This property currently only makes sense for
enums. It seems structs are never typedef'ed (and thank goodness for that since
the PNaCl shim needs to know if something is a struct or not).

Alternatively we would end up changing the header and the one user of the header
that I found (may be more in other repos?) -- see patch set 8 for that version.

I'm guessing patch set 10 (not changing the enum) is what we want.

[dmichael (off chromium), 2011-11-18 03:32:58]

Yes...  typedefing enums is the convention we use almost everywhere, but if you
change it, it'll break the flash part of the official build on check-in. So I
agree, patch set 10 is good.

LGTM, thanks!

[commit-bot: I haz the power, 2011-11-18 19:53:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jvoung@google.com/8586031/10001

[commit-bot: I haz the power, 2011-11-18 21:22:17]

Try job failure for 8586031-10001 (retry) (retry) on linux_rel for step
"ui_tests".
It's a second try, previously, steps "test_shell_tests, ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...