Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11417010:
Add support for generating thunk source from IDL. (Closed)
|
Created:
8 years, 1 month ago by teravest Modified:
8 years, 1 month ago CC:
chromium-reviews Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdd support for generating thunk source from IDL.
This introduces a few new IDL attributes:
generate_thunk - Enables thunk generation for an IDL file.
create_func - Overrides the guessed create function name.
on_failure - Overrides the default return value on failure.
report_errors - Allows error reporting to be disabled.
By using these attributes, we can generate _thunk.cc files for many IDL
files.
I'll send CLs for moving the thunks separately, as I found it tiring to
review them all in a big lump. I have PPB_Widget_Dev here as an example.
BUG=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168450
Patch Set 1 #Patch Set 2 : Removed an unnecessary change #
Total comments: 26
Patch Set 3 : Handled Noel's feedback for patch set 2. #Patch Set 4 : Fixed dmichael's comments. #
Total comments: 2
Patch Set 5 : minor style update #Patch Set 6 : presubmit: Allow IDL generate_thunk updates #Patch Set 7 : #Messages
Total messages: 14 (0 generated)
I'm able to generate about 30 _thunk.cc files from IDL with this change, but I figured it would be easier to review the generated thunks one by one. Let me know if I should add other reviewers.
Appart from mising LF, the generator changes LGTM. I'll assume dmichael or someone else on chrome/pepper side is verifying the expected emitted wrappers. One other change you should look into is ppapi/PRESUBMIT.py. There's code in there to verify that the .cc and .idl file are not out of sync. This will be complicated by your [generate_thunk] option. However I don't think that needs to block this CL and I'm happy to discuss possibilities offline. https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_c_hea... File ppapi/generators/idl_c_header.py (left): https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_c_hea... ppapi/generators/idl_c_header.py:24: Option('out', 'List of output files', default='') Thanks, I've been meaning to do this. https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_c_hea... ppapi/generators/idl_c_header.py:254: Make sure to leave blank EoF. I've been having windows patch failures when this is missing. The failure appears on the next CL to touch the end of the file. https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_c_pro... File ppapi/generators/idl_c_proto.py (left): https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_c_pro... ppapi/generators/idl_c_proto.py:695: Make sure to leave blank line to prevent windows patch issue. https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_thunk.py File ppapi/generators/idl_thunk.py (right): https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_thunk... ppapi/generators/idl_thunk.py:35: """Returns the base name for an interface, given the filename.""" FYI: Interface name does not always match source file name unfortunately. Usually this is a mismatch spacing. The property 'macro' renames the interface. For example: ppb_graphics_3d.idl generates the macro: PPB_GRAPHICS_3D_INTERFACE However it generates the interface struct PPB_Graphics3D Since you appear to only use this for generating filenames, I think you are okay, but wanted to make sure you didn't have an invalid assumption here. https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/test_thun... File ppapi/generators/test_thunk/std_types.idl (left): https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/test_thun... ppapi/generators/test_thunk/std_types.idl:11: This change can generate a warning. It's expected that all idl file should have a copyright notice followed by a file doc.
I'm not sure what to do for presubmit.py; I do get this error when making this change: ** Presubmit Warnings ** Missing PPAPI header, no change or skipped generation? *************** ppapi/c/dev/ppb_widget_dev.idl *************** ...but I think it's fine to give a warning when someone is adding "[generate_thunk]" and other annotations to the IDL without changing the header. Presumably I could ignore the warnings for those changes and commit anyway? https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_c_hea... File ppapi/generators/idl_c_header.py (left): https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_c_hea... ppapi/generators/idl_c_header.py:254: On 2012/11/15 22:03:18, noelallen1 wrote: > Make sure to leave blank EoF. I've been having windows patch failures when this > is missing. The failure appears on the next CL to touch the end of the file. Done. https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_c_pro... File ppapi/generators/idl_c_proto.py (left): https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_c_pro... ppapi/generators/idl_c_proto.py:695: On 2012/11/15 22:03:18, noelallen1 wrote: > Make sure to leave blank line to prevent windows patch issue. Done. https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_thunk.py File ppapi/generators/idl_thunk.py (right): https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_thunk... ppapi/generators/idl_thunk.py:35: """Returns the base name for an interface, given the filename.""" On 2012/11/15 22:03:18, noelallen1 wrote: > FYI: Interface name does not always match source file name unfortunately. > Usually this is a mismatch spacing. The property 'macro' renames the interface. > For example: > > ppb_graphics_3d.idl generates the macro: > PPB_GRAPHICS_3D_INTERFACE > However it generates the interface > struct PPB_Graphics3D > > Since you appear to only use this for generating filenames, I think you are > okay, but wanted to make sure you didn't have an invalid assumption here. I changed the method name to _GetBaseFileName to make this easier to read (and less error prone if the code is changed in the future). Thanks. https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/test_thun... File ppapi/generators/test_thunk/std_types.idl (left): https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/test_thun... ppapi/generators/test_thunk/std_types.idl:11: On 2012/11/15 22:03:18, noelallen1 wrote: > This change can generate a warning. It's expected that all idl file should have > a copyright notice followed by a file doc. Thanks for letting me know; I've added a comment here.
https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_c_pro... File ppapi/generators/idl_c_proto.py (right): https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_c_pro... ppapi/generators/idl_c_proto.py:586: while args[0][0] == ')': Can you maybe put an example of the kind of code you're splitting? Before & after? I think that would help with understanding the code. As-is, the ')' check is slightly non-obvious (my best guess is that if you see "()", you're looking left for another "(" to break on.) https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_c_pro... ppapi/generators/idl_c_proto.py:599: space = '%s ' % tab indent might be a better name than space? https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_c_pro... ppapi/generators/idl_c_proto.py:607: lines.append(line.rstrip()) I think I'd flip the if and the else, since in the current order, by the time you get here, you forget what the original condition was. https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_thunk.py File ppapi/generators/idl_thunk.py (right): https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_thunk... ppapi/generators/idl_thunk.py:35: """Returns the base name for an interface, given the filename.""" Maybe explain what "base name" means? Just an example would suffice, I think. https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_thunk... ppapi/generators/idl_thunk.py:168: args - List of arguments for the member function maybe mention that it's a list of 2-element lists, where the first is the type, second is the name? Or is that just the convention everywhere in the IDL stuff? If it's not the convention... making it a small object with 2 named properties might make the code more readable... e.g., args[len(args) - 1].type ...seems more obvious than... args[len(args) - 1][0] https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/test_thun... File ppapi/generators/test_thunk/std_types.idl (right): https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/test_thun... ppapi/generators/test_thunk/std_types.idl:52: PP_Var; since this is a little different from ppapi/api/std_types.idl, maybe it should have a different name, like basic_test_types.idl or something. https://codereview.chromium.org/11417010/diff/2001/ppapi/thunk/ppb_widget_thu... File ppapi/thunk/ppb_widget_thunk.cc (right): https://codereview.chromium.org/11417010/diff/2001/ppapi/thunk/ppb_widget_thu... ppapi/thunk/ppb_widget_thunk.cc:4: */ How hard would it be to do a C++ style copyright header? https://codereview.chromium.org/11417010/diff/2001/ppapi/thunk/ppb_widget_thu... ppapi/thunk/ppb_widget_thunk.cc:28: const struct PP_Rect* rect, since this is C++, the struct is superfluous. If this is just easier because it's what shows up in the C headers, then that's fine.
https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_c_pro... File ppapi/generators/idl_c_proto.py (right): https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_c_pro... ppapi/generators/idl_c_proto.py:586: while args[0][0] == ')': On 2012/11/16 18:24:44, dmichael wrote: > Can you maybe put an example of the kind of code you're splitting? Before & > after? I think that would help with understanding the code. As-is, the ')' check > is slightly non-obvious (my best guess is that if you see "()", you're looking > left for another "(" to break on.) Sure. Comment added. https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_c_pro... ppapi/generators/idl_c_proto.py:599: space = '%s ' % tab On 2012/11/16 18:24:44, dmichael wrote: > indent might be a better name than space? Done. I had left this unchanged from the previous code. https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_c_pro... ppapi/generators/idl_c_proto.py:607: lines.append(line.rstrip()) On 2012/11/16 18:24:44, dmichael wrote: > I think I'd flip the if and the else, since in the current order, by the time > you get here, you forget what the original condition was. Done. https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_thunk.py File ppapi/generators/idl_thunk.py (right): https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_thunk... ppapi/generators/idl_thunk.py:35: """Returns the base name for an interface, given the filename.""" On 2012/11/16 18:24:44, dmichael wrote: > Maybe explain what "base name" means? Just an example would suffice, I think. Examples added. https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_thunk... ppapi/generators/idl_thunk.py:168: args - List of arguments for the member function On 2012/11/16 18:24:44, dmichael wrote: > maybe mention that it's a list of 2-element lists, where the first is the type, > second is the name? Or is that just the convention everywhere in the IDL stuff? > > If it's not the convention... making it a small object with 2 named properties > might make the code more readable... e.g., > args[len(args) - 1].type > ...seems more obvious than... > args[len(args) - 1][0] It's actually a list of 4 element tuples, which is just what the IDL code uses to pass everything around. I agree that a type would be better, but I didn't want to do a lot of refactoring of the existing IDL code; I can do that if you'd like. For now, I added a comment. https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/test_thun... File ppapi/generators/test_thunk/std_types.idl (right): https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/test_thun... ppapi/generators/test_thunk/std_types.idl:52: PP_Var; On 2012/11/16 18:24:44, dmichael wrote: > since this is a little different from ppapi/api/std_types.idl, maybe it should > have a different name, like basic_test_types.idl or something. Done. https://codereview.chromium.org/11417010/diff/2001/ppapi/thunk/ppb_widget_thu... File ppapi/thunk/ppb_widget_thunk.cc (right): https://codereview.chromium.org/11417010/diff/2001/ppapi/thunk/ppb_widget_thu... ppapi/thunk/ppb_widget_thunk.cc:4: */ On 2012/11/16 18:24:44, dmichael wrote: > How hard would it be to do a C++ style copyright header? Done. I changed the "From...modified..." to be C++ style as well. https://codereview.chromium.org/11417010/diff/2001/ppapi/thunk/ppb_widget_thu... ppapi/thunk/ppb_widget_thunk.cc:28: const struct PP_Rect* rect, On 2012/11/16 18:24:44, dmichael wrote: > since this is C++, the struct is superfluous. If this is just easier because > it's what shows up in the C headers, then that's fine. This is easiest because it's what IDL gives me for the type. I debated stripping the 'struct ' off, but didn't see a good reason.
Still LGTM. https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_thunk.py File ppapi/generators/idl_thunk.py (right): https://codereview.chromium.org/11417010/diff/2001/ppapi/generators/idl_thunk... ppapi/generators/idl_thunk.py:168: args - List of arguments for the member function I've been meaning to make that change for a while. It's is lots of places so I don't think this is the correct CL for it. On 2012/11/16 18:52:36, Justin TerAvest wrote: > On 2012/11/16 18:24:44, dmichael wrote: > > maybe mention that it's a list of 2-element lists, where the first is the > type, > > second is the name? Or is that just the convention everywhere in the IDL > stuff? > > > > If it's not the convention... making it a small object with 2 named > properties > > might make the code more readable... e.g., > > args[len(args) - 1].type > > ...seems more obvious than... > > args[len(args) - 1][0] > > It's actually a list of 4 element tuples, which is just what the IDL code uses > to pass everything around. > > I agree that a type would be better, but I didn't want to do a lot of > refactoring of the existing IDL code; I can do that if you'd like. > > For now, I added a comment.
lgtm https://codereview.chromium.org/11417010/diff/8001/ppapi/generators/idl_thunk.py File ppapi/generators/idl_thunk.py (right): https://codereview.chromium.org/11417010/diff/8001/ppapi/generators/idl_thunk... ppapi/generators/idl_thunk.py:280: out.Write('%s\n' % cgen.Copyright(cright_node, True)) suggestion: "cpp_style=True", just so it's more self-documenting
https://codereview.chromium.org/11417010/diff/8001/ppapi/generators/idl_thunk.py File ppapi/generators/idl_thunk.py (right): https://codereview.chromium.org/11417010/diff/8001/ppapi/generators/idl_thunk... ppapi/generators/idl_thunk.py:280: out.Write('%s\n' % cgen.Copyright(cright_node, True)) On 2012/11/16 20:37:02, dmichael wrote: > suggestion: "cpp_style=True", just so it's more self-documenting Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/teravest@chromium.org/11417010/4003
Presubmit check for 11417010-4003 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change has an associated bug, add BUG=[bug number]. ** Presubmit Warnings ** Missing PPAPI header, no change or skipped generation? *************** ppapi/c/dev/ppb_widget_dev.idl *************** Presubmit checks took 2.7s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/teravest@chromium.org/11417010/8003
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/teravest@chromium.org/11417010/8003
Change committed as 168450 |
