Generate Pepper IDL for OpenGLES2

Generate Pepper IDL for OpenGLES2

Previously, we were directly generating header files for
OpenGLES2 interface. This is not aligned with other Pepper
APIs. The header file generated from the generated IDL file
should be compatible with the previous one. To keep OpenGLES2
interfaces in struct namespace, this change introduces
"force_struct_namespace" attribute to the generator script.

TEST=trybots
BUG=411799
Committed: https://crrev.com/7bba553d64fdc63b33b976054411ac2827c339fb
Cr-Commit-Position: refs/heads/master@{#293904}

[hamaji, 2014-09-04 08:26:08]

hamaji@chromium.org changed reviewers:
+ teravest@chromium.org, vmiura@chromium.org

[hamaji, 2014-09-04 08:26:09]

Let me describe the motivation of this change. Our team is generating
files from Pepper headers for two purposes:

1. Generate mock definitions for Pepper APIs to test our code base
without real Pepper implementations.
2. Generate header files which fill floating point ABI gaps on
ARM. Chrome is built with hard ABI but we need to use softfp ABI.

Currently, we are parsing Pepper header files but this approach is
obviously fragile. Once this patch has been landed, we should be able
to generate the above things from IDLs hopefully re-using Pepper's IDL
generator.


I'm not sure if the "struct PPB_OpenGLES2" => "PPB_OpenGLES2"
change is OK. Suggestions are welcomed, thanks!

[teravest, 2014-09-04 15:30:51]

On 2014/09/04 08:26:09, hamaji wrote:
> Let me describe the motivation of this change. Our team is generating
> files from Pepper headers for two purposes:
> 
> 1. Generate mock definitions for Pepper APIs to test our code base
> without real Pepper implementations.
> 2. Generate header files which fill floating point ABI gaps on
> ARM. Chrome is built with hard ABI but we need to use softfp ABI.
> 
> Currently, we are parsing Pepper header files but this approach is
> obviously fragile. Once this patch has been landed, we should be able
> to generate the above things from IDLs hopefully re-using Pepper's IDL
> generator.
> 
> 
> I'm not sure if the "struct PPB_OpenGLES2" => "PPB_OpenGLES2"
> change is OK. Suggestions are welcomed, thanks!

I don't think the "struct PPB_OpenGLES2" => "PPB_OpenGLES2" change is OK.
Any code that explicitly specifies struct in the type (all C code, some C++
code) will fail to compile, as that type is moved out of the struct namespace.

Here's an example that I think would break, from searching around:
 
http://www.kayru.org/articles/arcadeshooter-nacl-postmortem/minimal_nacl_gles...


The only thing I can think to do here is to add a "force_struct_namespace"
annotation to the IDL, which would force PPB_OpenGLES2 to get its own duplicated
struct declaration instead of using a typedef.

Otherwise, the rest of this change seems fine.

[hamaji, 2014-09-05 06:20:17]

On 2014/09/04 15:30:51, teravest wrote:
> On 2014/09/04 08:26:09, hamaji wrote:
> > Let me describe the motivation of this change. Our team is generating
> > files from Pepper headers for two purposes:
> > 
> > 1. Generate mock definitions for Pepper APIs to test our code base
> > without real Pepper implementations.
> > 2. Generate header files which fill floating point ABI gaps on
> > ARM. Chrome is built with hard ABI but we need to use softfp ABI.
> > 
> > Currently, we are parsing Pepper header files but this approach is
> > obviously fragile. Once this patch has been landed, we should be able
> > to generate the above things from IDLs hopefully re-using Pepper's IDL
> > generator.
> > 
> > 
> > I'm not sure if the "struct PPB_OpenGLES2" => "PPB_OpenGLES2"
> > change is OK. Suggestions are welcomed, thanks!
> 
> I don't think the "struct PPB_OpenGLES2" => "PPB_OpenGLES2" change is OK.
> Any code that explicitly specifies struct in the type (all C code, some C++
> code) will fail to compile, as that type is moved out of the struct namespace.
> 
> Here's an example that I think would break, from searching around:
>  
>
http://www.kayru.org/articles/arcadeshooter-nacl-postmortem/minimal_nacl_gles...
> 
> 
> The only thing I can think to do here is to add a "force_struct_namespace"
> annotation to the IDL, which would force PPB_OpenGLES2 to get its own
duplicated
> struct declaration instead of using a typedef.
> 
> Otherwise, the rest of this change seems fine.

Thanks for the suggestion, done!

[teravest, 2014-09-05 15:50:37]

teravest@chromium.org changed reviewers:
+ dmichael@chromium.org

[teravest, 2014-09-05 15:50:37]

+dmichael

Dave had some ideas about getting fancy with typedefs to prevent declaring the
struct twice in a header. I'll let him take a look and chime in.

[dmichael (off chromium), 2014-09-05 18:46:09]

tl;dr, I now like Justin's approach, and we should probably go forward with
that.

For completeness and in case you care, I'll describe my alternate approach. I
was going to suggest putting every API version in as a struct and a typedef.
This would still have been behind a special annotation. E.g.:

typedef struct PPB_Foo {
  // ...
} PPB_Foo;
typedef PPB_Foo PPB_Foo_1_0

Then, if PPB_Foo got reved to 1.1, it would look like this:
typedef struct PPB_Foo {
  // new one
} PPB_Foo;
typedef PPB_Foo PPB_Foo_1_1

typedef struct PPB_Foo_1_0 {
  // old one
} PPB_Foo_1_0;

The only advantage here is you don't have to duplicate each API struct. Like
Justin's approach, it wouldn't break code compatibility. For any code using a
specific version (e.g. PPB_Foo_1_0), they could always use the typedef version.
They could use the struct version if it was not the newest. So it's kind of
weird and confusing to use in that way. I suggested this because so many of our
API headers are important references for developers, and so we don't want to
clutter them too much with unnecessary stuff.

However, after talking with Justin about it, I'm convinced that most GLES2
developers probably don't use this directly, but instead use the OpenGLES2
versions of the functions, so we should do the simplest thing that works here
without worrying about the header being a little verbose.

Hence...  Justin's approach is probably the right one (especially if it's
simpler to implement than the above suggestion).

Now back to your regularly scheduled review.

https://codereview.chromium.org/538903002/diff/80001/ppapi/generators/idl_c_p...
File ppapi/generators/idl_c_proto.py (right):

https://codereview.chromium.org/538903002/diff/80001/ppapi/generators/idl_c_p...
ppapi/generators/idl_c_proto.py:592: # Duplicate the definition to put it in
struct namespace.
I would elaborate that this is for legacy APIs like PPB_OpenGLES2 so that we
wouldn't break code compatibility, and it should not be used for new APIs. Also
please reference the bug.

[hamaji, 2014-09-08 07:34:05]

Done. I'm fine with either way. As Dave said by himself, I feel Dave's option is
a bit weird because people cannot use struct PPB_Foo_X_X for the latest version.

If you care the dup in the header files, another would be not defining versioned
structs at all. But I assume this idea is even worse than the two ideas.

https://codereview.chromium.org/538903002/diff/80001/ppapi/generators/idl_c_p...
File ppapi/generators/idl_c_proto.py (right):

https://codereview.chromium.org/538903002/diff/80001/ppapi/generators/idl_c_p...
ppapi/generators/idl_c_proto.py:592: # Duplicate the definition to put it in
struct namespace.
On 2014/09/05 18:46:08, dmichael wrote:
> I would elaborate that this is for legacy APIs like PPB_OpenGLES2 so that we
> wouldn't break code compatibility, and it should not be used for new APIs.
Also
> please reference the bug.

Done. Ah, I have not filed the bug yet. I filed one and added it to here and
BUG= line.

[teravest, 2014-09-08 14:26:35]

lgtm

[vmiura, 2014-09-08 18:23:00]

lgtm for gpu/command_buffer/build_gles2_cmd_buffer.py

[hamaji, 2014-09-09 02:56:29]

The CQ bit was checked by hamaji@chromium.org

[commit-bot: I haz the power, 2014-09-09 03:06:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hamaji@chromium.org/538903002/100001

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

Committed patchset #6 (id:100001) as 9309780570564fb2b214d8873c54ccf209e39890

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

Patchset 6 (id:??) landed as
https://crrev.com/7bba553d64fdc63b33b976054411ac2827c339fb
Cr-Commit-Position: refs/heads/master@{#293904}