Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12315074:
Fix problem with non void return types in extension API IDL. (Closed)
|
Created:
7 years, 10 months ago by benwells Modified:
7 years, 10 months ago CC:
chromium-reviews, chrome-apps-reviews-syd_chromium.org, asargent_no_longer_on_chrome Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix problem with non void return types in extension API IDL.
This also changes the dart generator to use these types.
BUG=144301
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184762
Patch Set 1 #Patch Set 2 : Have another go #
Total comments: 8
Patch Set 3 : Feedback #
Total comments: 2
Patch Set 4 : asargent's feedback #
Messages
Total messages: 18 (0 generated)
kalman: this is sasha's change https://codereview.chromium.org/12316031/ with the test failures removed. The problem was that now that idl functions have return types in the schema, they are validated. As some functions return null this was interpreted as the value not being present. Hack solution is to make all idl return types optional. It would be better if this only happened for object types, but I'm not sure how to find this out. I also fixed a typo in some of the dart generation. That stuff really needs those tests sorted out. That's my next stop on this dart app journey.
On 2013/02/25 06:54:01, benwells wrote: > kalman: this is sasha's change https://codereview.chromium.org/12316031/ with > the test failures removed. > > The problem was that now that idl functions have return types in the schema, > they are validated. As some functions return null this was interpreted as the > value not being present. > > Hack solution is to make all idl return types optional. It would be better if > this only happened for object types, but I'm not sure how to find this out. > > I also fixed a typo in some of the dart generation. That stuff really needs > those tests sorted out. That's my next stop on this dart app journey. Oops, still has problems, don't review yet.
Should be fixed now.
FYI sasha's test change got reverted, but I can't remember if it got re-landed. https://codereview.chromium.org/12315074/diff/5002/tools/json_schema_compiler... File tools/json_schema_compiler/idl_schema.py (right): https://codereview.chromium.org/12315074/diff/5002/tools/json_schema_compiler... tools/json_schema_compiler/idl_schema.py:105: if return_type.get('type', '') == 'object' or '$ref' in return_type: the second argument to .get is unnecessary, it's implicitly None. https://codereview.chromium.org/12315074/diff/5002/tools/json_schema_compiler... tools/json_schema_compiler/idl_schema.py:106: return_type['optional'] = True; Yes it would be much preferable to fix this by annotating the IDL files directly for functions that can return null. I imagine this would involve turning static FileEntry getEntryById(DOMString id); into static FileEntry? getEntryById(DOMString id); I had a quick look at the ppapi source to see if it supports it, but I can't really tell. Best would just be to do it an see I suppose, but looking at this file it seem like it'd just be a matter of changing the condition to return_type.GetProperty('OPTIONAL'). In that case I'd prefer doing it that way. But if it involves changing the ppapi compiler... what you have is fine, but we should add a comment. https://codereview.chromium.org/12315074/diff/5002/tools/json_schema_compiler... tools/json_schema_compiler/idl_schema.py:245: properties['additionalProperties']['type'] = 'any' I think we're going to want a way to actually annotate objects like this as belonging to the web platform, not an extensions/app schema, but we can do that later. +asargent
+asargent for real
https://codereview.chromium.org/12315074/diff/5002/tools/json_schema_compiler... File tools/json_schema_compiler/idl_schema.py (right): https://codereview.chromium.org/12315074/diff/5002/tools/json_schema_compiler... tools/json_schema_compiler/idl_schema.py:105: if return_type.get('type', '') == 'object' or '$ref' in return_type: On 2013/02/25 18:00:43, kalman wrote: > the second argument to .get is unnecessary, it's implicitly None. Done. https://codereview.chromium.org/12315074/diff/5002/tools/json_schema_compiler... tools/json_schema_compiler/idl_schema.py:106: return_type['optional'] = True; On 2013/02/25 18:00:43, kalman wrote: > Yes it would be much preferable to fix this by annotating the IDL files directly > for functions that can return null. I imagine this would involve turning > > static FileEntry getEntryById(DOMString id); > > into > > static FileEntry? getEntryById(DOMString id); > > I had a quick look at the ppapi source to see if it supports it, but I can't > really tell. Best would just be to do it an see I suppose, but looking at this > file it seem like it'd just be a matter of changing the condition to > return_type.GetProperty('OPTIONAL'). > > In that case I'd prefer doing it that way. Yep, that would be nicer. The idl parser doesn't seem to like it though. I've added a comment. > > But if it involves changing the ppapi compiler... what you have is fine, but we > should add a comment. https://codereview.chromium.org/12315074/diff/5002/tools/json_schema_compiler... tools/json_schema_compiler/idl_schema.py:245: properties['additionalProperties']['type'] = 'any' On 2013/02/25 18:00:43, kalman wrote: > I think we're going to want a way to actually annotate objects like this as > belonging to the web platform, not an extensions/app schema, but we can do that > later. > > +asargent Sounds good.
lgtm https://codereview.chromium.org/12315074/diff/5002/tools/json_schema_compiler... File tools/json_schema_compiler/idl_schema.py (right): https://codereview.chromium.org/12315074/diff/5002/tools/json_schema_compiler... tools/json_schema_compiler/idl_schema.py:245: properties['additionalProperties']['type'] = 'any' On 2013/02/26 04:19:07, benwells wrote: > On 2013/02/25 18:00:43, kalman wrote: > > I think we're going to want a way to actually annotate objects like this as > > belonging to the web platform, not an extensions/app schema, but we can do > that > > later. > > > > +asargent > > Sounds good. which I suppose would just be adding support for [instanceof=Foo] annotations?
On 2013/02/26 05:31:58, kalman wrote: > lgtm > > https://codereview.chromium.org/12315074/diff/5002/tools/json_schema_compiler... > File tools/json_schema_compiler/idl_schema.py (right): > > https://codereview.chromium.org/12315074/diff/5002/tools/json_schema_compiler... > tools/json_schema_compiler/idl_schema.py:245: > properties['additionalProperties']['type'] = 'any' > On 2013/02/26 04:19:07, benwells wrote: > > On 2013/02/25 18:00:43, kalman wrote: > > > I think we're going to want a way to actually annotate objects like this as > > > belonging to the web platform, not an extensions/app schema, but we can do > > that > > > later. > > > > > > +asargent > > > > Sounds good. > > which I suppose would just be adding support for [instanceof=Foo] annotations? Not sure. The instanceof stuff doesn't seem to do much in idl land, for validation anyway, that I am aware of. I know testing for instanceof FileEntry in javascript doesn't work like it does with other types. Assuming that is the case for all webkit defined types, the instanceof=foo stuff seems not that useful (except documentation).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benwells@chromium.org/12315074/2016
lgtm https://codereview.chromium.org/12315074/diff/5002/tools/json_schema_compiler... File tools/json_schema_compiler/idl_schema.py (right): https://codereview.chromium.org/12315074/diff/5002/tools/json_schema_compiler... tools/json_schema_compiler/idl_schema.py:245: properties['additionalProperties']['type'] = 'any' On 2013/02/26 05:31:58, kalman wrote: > On 2013/02/26 04:19:07, benwells wrote: > > On 2013/02/25 18:00:43, kalman wrote: > > > I think we're going to want a way to actually annotate objects like this as > > > belonging to the web platform, not an extensions/app schema, but we can do > > that > > > later. > > > > > > +asargent > > > > Sounds good. > > which I suppose would just be adding support for [instanceof=Foo] annotations? I haven't dealt with it much; my understanding was that the instanceOf idiom is for DOM elements, and maps to instanceof in the JS schema validation. If it doesn't work then maybe that's unintended, but I don't really know.
LGTM https://codereview.chromium.org/12315074/diff/2016/tools/json_schema_compiler... File tools/json_schema_compiler/idl_schema.py (right): https://codereview.chromium.org/12315074/diff/2016/tools/json_schema_compiler... tools/json_schema_compiler/idl_schema.py:101: if self.node.GetProperty('TYPEREF') not in ['void', None]: tiny nit: I think we typically prefer ('void', None) to ['void', None] for "in" expressions where the set of items is static https://codereview.chromium.org/12315074/diff/2016/tools/json_schema_compiler... tools/json_schema_compiler/idl_schema.py:106: # Instead we infer any object return values to be optional. I can whip up a quick CL to add this support to the idl parser - it's just a matter of supporting a question mark appearing after a type: static Foo? myFunc(); Then in the parse tree, in the list of properties for myFunc, there will be TYPEREF: Foo and OPTIONAL: True. While I'm there I could also add support for array type return values, so you could do: static long[] myFunc2(); and even optional array types, like: static long[]? myFunc3(); It's probably fine to still land this as you currently have it - just add a TODO comment here saying that we'll want to switch this to looking for an OPTIONAL entry in the properties.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benwells@chromium.org/12315074/11003
On 2013/02/26 06:19:47, Antony Sargent wrote: > LGTM > > https://codereview.chromium.org/12315074/diff/2016/tools/json_schema_compiler... > File tools/json_schema_compiler/idl_schema.py (right): > > https://codereview.chromium.org/12315074/diff/2016/tools/json_schema_compiler... > tools/json_schema_compiler/idl_schema.py:101: if > self.node.GetProperty('TYPEREF') not in ['void', None]: > tiny nit: I think we typically prefer ('void', None) to ['void', None] for "in" > expressions where the set of items is static > > https://codereview.chromium.org/12315074/diff/2016/tools/json_schema_compiler... > tools/json_schema_compiler/idl_schema.py:106: # Instead we infer any object > return values to be optional. > I can whip up a quick CL to add this support to the idl parser - it's just a > matter of supporting a question mark appearing after a type: > > static Foo? myFunc(); > > Then in the parse tree, in the list of properties for myFunc, there will be > TYPEREF: Foo and OPTIONAL: True. > > While I'm there I could also add support for array type return values, so you > could do: > > static long[] myFunc2(); > > and even optional array types, like: > > static long[]? myFunc3(); > > > It's probably fine to still land this as you currently have it - just add a TODO > comment here saying that we'll want to switch this to looking for an OPTIONAL > entry in the properties. ok, added TODO and fixed nit.
Retried try job too often on mac_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benwells@chromium.org/12315074/11003
Retried try job too often on mac_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benwells@chromium.org/12315074/11003
Message was sent while issue was closed.
Change committed as 184762 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
