Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
474173002:
IDL: Use IdlNullableType wrapper to represent nullable types (Closed)
|
Created:
6 years, 4 months ago by Jens Widell Modified:
6 years, 4 months ago CC:
blink-reviews, blink-reviews-bindings_chromium.org, arv+blink, abarth-chromium Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionIDL: Use IdlNullableType wrapper to represent nullable types
Replaces the |is_nullable| flag previously stored in IdlTypeBase.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180564
Patch Set 1 #
Total comments: 8
Patch Set 2 : alternative approach #
Total comments: 9
Patch Set 3 : cleanup #
Messages
Total messages: 21 (0 generated)
PTAL I'm not convinced this is a good change overall. Nullable types are in most cases treated the same as their inner type. Representing nullable as a flag on all types means all you need to do is ignore it. With this change, we need to explicitly make nullable be treated the same as their inner type, which mostly means we can forget to do it in some place and have a nullable type act as if it was different from its inner type by accident. https://codereview.chromium.org/474173002/diff/1/Source/bindings/tests/result... File Source/bindings/tests/results/V8TestObject.cpp (right): https://codereview.chromium.org/474173002/diff/1/Source/bindings/tests/result... Source/bindings/tests/results/V8TestObject.cpp:7418: TONATIVE_VOID_EXCEPTIONSTATE_INTERNAL(defaultStringArg, toByteString(info[0], exceptionState), exceptionState); This seems a correct change; there's only one toByteString() and it requires an ExceptionState parameter. I don't know what part of the patch caused it...
bashi-san: What do you think about this?
LGTM. https://codereview.chromium.org/474173002/diff/1/Source/bindings/scripts/idl_... File Source/bindings/scripts/idl_definitions.py (right): https://codereview.chromium.org/474173002/diff/1/Source/bindings/scripts/idl_... Source/bindings/scripts/idl_definitions.py:802: element_type = type_node_to_type(sequence_child) Nit: element_type => sequence_type (or remove element_type) https://codereview.chromium.org/474173002/diff/1/Source/bindings/scripts/idl_... File Source/bindings/scripts/idl_types.py (right): https://codereview.chromium.org/474173002/diff/1/Source/bindings/scripts/idl_... Source/bindings/scripts/idl_types.py:393: return self.inner_type.is_basic_type Just a question about Python: Even if we have the above __getattr__, do we need to define is_basic_type, is_callback_function etc?
On 2014/08/15 07:36:43, haraken wrote: > bashi-san: What do you think about this? I think this is LGTM, but as haraken-san said, it would be great if we can avoid lots of 'self.inner_type.foobar' fallbacks.
https://codereview.chromium.org/474173002/diff/1/Source/bindings/scripts/idl_... File Source/bindings/scripts/idl_definitions.py (right): https://codereview.chromium.org/474173002/diff/1/Source/bindings/scripts/idl_... Source/bindings/scripts/idl_definitions.py:802: element_type = type_node_to_type(sequence_child) On 2014/08/15 07:43:57, haraken wrote: > > Nit: element_type => sequence_type (or remove element_type) It *is* the element type of the sequence (i.e. sequence_type.element_type). But dropping the local variable would be fine. https://codereview.chromium.org/474173002/diff/1/Source/bindings/scripts/idl_... File Source/bindings/scripts/idl_types.py (right): https://codereview.chromium.org/474173002/diff/1/Source/bindings/scripts/idl_... Source/bindings/scripts/idl_types.py:393: return self.inner_type.is_basic_type On 2014/08/15 07:43:57, haraken wrote: > > Just a question about Python: Even if we have the above __getattr__, do we need > to define is_basic_type, is_callback_function etc? Yes, __getattr__ is called as a last resort, if the attribute wasn't found by any other lookup mechanism. Thus we need to explicitly override everything that IdlTypeBase defines. If we didn't inherit IdlTypeBase, we could have just __getattr__, but then extending IdlTypeBase in v8_types.py would no longer work.
https://codereview.chromium.org/474173002/diff/1/Source/bindings/scripts/idl_... File Source/bindings/scripts/idl_types.py (right): https://codereview.chromium.org/474173002/diff/1/Source/bindings/scripts/idl_... Source/bindings/scripts/idl_types.py:393: return self.inner_type.is_basic_type On 2014/08/15 08:00:09, Jens Widell wrote: > On 2014/08/15 07:43:57, haraken wrote: > > > > Just a question about Python: Even if we have the above __getattr__, do we > need > > to define is_basic_type, is_callback_function etc? > > Yes, __getattr__ is called as a last resort, if the attribute wasn't found by > any other lookup mechanism. Thus we need to explicitly override everything that > IdlTypeBase defines. > > If we didn't inherit IdlTypeBase, we could have just __getattr__, but then > extending IdlTypeBase in v8_types.py would no longer work. Makes sense.
On 2014/08/15 07:56:51, bashi1 wrote: > On 2014/08/15 07:36:43, haraken wrote: > > bashi-san: What do you think about this? > > I think this is LGTM, but as haraken-san said, it would be great if we can avoid > lots of 'self.inner_type.foobar' fallbacks. For the record, I totally agree that avoiding all the manual relaying of attribute accesses in IdlNullableType would be great. I just couldn't quite come up with a nice way to do it.
I can't come up with a good way to avoid manual relaying either... so still LGTM. https://codereview.chromium.org/474173002/diff/1/Source/bindings/scripts/v8_d... File Source/bindings/scripts/v8_dictionary.py (right): https://codereview.chromium.org/474173002/diff/1/Source/bindings/scripts/v8_d... Source/bindings/scripts/v8_dictionary.py:9: import copy nit: Could you remove this?
On 2014/08/15 15:08:25, bashi1 wrote:
> I can't come up with a good way to avoid manual relaying either...
I came up with something, that could certainly be seen as quite hacky. Uploaded
as PS#2.
The approach is to drop all fallback "return None/False" in IdlTypeBase, and
instead implement them as a __getattr__() that returns None for anything that
one of the sub-types implements. (We could check if the property name starts
with "is_" and return False instead then, but it I can't think of a particularly
good reason to do so.)
This in turn will allow IdlNullableType's __getattr__() to relay to the inner
class everything that IdlNullableType doesn't implement itself (i.e.
is_nullable, name and resolve_typedefs.)
I think it works, so the question is if we want it. It's a bit hacky, but
eliminates most of the maintenance burden of syncing the property sets of
IdlTypeBase, Idl{,Array,Sequence}Type and IdlNullableType.
PTAL?
https://codereview.chromium.org/474173002/diff/1/Source/bindings/scripts/v8_d...
File Source/bindings/scripts/v8_dictionary.py (right):
https://codereview.chromium.org/474173002/diff/1/Source/bindings/scripts/v8_d...
Source/bindings/scripts/v8_dictionary.py:9: import copy
On 2014/08/15 15:08:25, bashi1 wrote:
> nit: Could you remove this?
Done.
On 2014/08/15 15:08:25, bashi1 wrote:
> I can't come up with a good way to avoid manual relaying either...
I came up with something, that could certainly be seen as quite hacky. Uploaded
as PS#2.
The approach is to drop all fallback "return None/False" in IdlTypeBase, and
instead implement them as a __getattr__() that returns None for anything that
one of the sub-types implements. (We could check if the property name starts
with "is_" and return False instead then, but it I can't think of a particularly
good reason to do so.)
This in turn will allow IdlNullableType's __getattr__() to relay to the inner
class everything that IdlNullableType doesn't implement itself (i.e.
is_nullable, name and resolve_typedefs.)
I think it works, so the question is if we want it. It's a bit hacky, but
eliminates most of the maintenance burden of syncing the property sets of
IdlTypeBase, Idl{,Array,Sequence}Type and IdlNullableType.
PTAL?
On 2014/08/19 11:37:00, Jens Widell wrote:
> On 2014/08/15 15:08:25, bashi1 wrote:
> > I can't come up with a good way to avoid manual relaying either...
>
> I came up with something, that could certainly be seen as quite hacky.
Uploaded
> as PS#2.
>
> The approach is to drop all fallback "return None/False" in IdlTypeBase, and
> instead implement them as a __getattr__() that returns None for anything that
> one of the sub-types implements. (We could check if the property name starts
> with "is_" and return False instead then, but it I can't think of a
particularly
> good reason to do so.)
>
> This in turn will allow IdlNullableType's __getattr__() to relay to the inner
> class everything that IdlNullableType doesn't implement itself (i.e.
> is_nullable, name and resolve_typedefs.)
>
> I think it works, so the question is if we want it. It's a bit hacky, but
> eliminates most of the maintenance burden of syncing the property sets of
> IdlTypeBase, Idl{,Array,Sequence}Type and IdlNullableType.
>
> PTAL?
Thanks for the update.
I'd slightly prefer the previous version (because the new version looks a bit
too hacky), but I'll delegate the decision to you and bashi-san.
Hi Jens, PS #2 looks great: it removes a huge amount of boilerplate, and makes the code much more extensible and maintainable. To the "hacky" point: defaulting to None actually makes our Idl*Type classes accord with Jinja variables, which default to None, so it improves consistency, and is the same solution to the same problem (lots of possible keys, don't want to define them all explicitly). https://codereview.chromium.org/474173002/diff/40001/Source/bindings/scripts/... File Source/bindings/scripts/idl_types.py (right): https://codereview.chromium.org/474173002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/idl_types.py:109: def __getattr__(self, name): Rather than hacky, how about considering this "elegant"? Also, I think it's fine to just have this default to None, so this can be: # Default undefined attributes to None (analogous to Jinja variables). # This allows us to not define default properties in the base class, # and allows us to relay __getattr__ in IdlNullableType to the inner type. return None This does lose us compile-time errors for typos (caught by test cases instead, which is what we have for Jinja), but removes the asymmetry of properties vs. member variables. https://codereview.chromium.org/474173002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/idl_types.py:121: if not name.startswith('_'): Do you need this? __getattr__ is only called if an attribute is not found otherwise, so this isn't overriding __str__, for instance. (I appreciate the caution and desire for safety, but I'm not sure this actually helps.)
Thank you for uploading alternative approach. I don't have a strong opinion here, but if my understanding is correct, I think we can avoid adding property relays in v8_types.py. If this is true, I slightly prefer PS#2. https://codereview.chromium.org/474173002/diff/40001/Source/bindings/scripts/... File Source/bindings/scripts/v8_types.py (right): https://codereview.chromium.org/474173002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/v8_types.py:249: IdlNullableType.native_array_element_type = property( If we take this approach, can we remove this?
BTW, per bashi, I think you can remove the other relays, right? https://codereview.chromium.org/474173002/diff/40001/Source/bindings/scripts/... File Source/bindings/scripts/v8_types.py (right): https://codereview.chromium.org/474173002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/v8_types.py:78: IdlNullableType.is_typed_array_element_type = property( Remove? https://codereview.chromium.org/474173002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/v8_types.py:86: IdlNullableType.is_wrapper_type = property( Remove? https://codereview.chromium.org/474173002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/v8_types.py:400: IdlNullableType.includes_for_type = property( Remove?
Oh, and LGTM ;)
> BTW, per bashi, I think you can remove the other relays, right? oh, then I'd much prefer the patch set 2 :)
Thanks all. PS#3 simplifies IdlTypeBase.__getattr__() per Nils' suggestions, makes some other minor changes to idl_types.py, and removes the added relays in v8_types.py, and also some other superfluous IdlTypeBase extensions there that were there just to make properties default to None or False. https://codereview.chromium.org/474173002/diff/40001/Source/bindings/scripts/... File Source/bindings/scripts/idl_types.py (right): https://codereview.chromium.org/474173002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/idl_types.py:109: def __getattr__(self, name): On 2014/08/19 13:44:14, Nils Barth (inactive) wrote: > Rather than hacky, how about considering this "elegant"? Done. :-) https://codereview.chromium.org/474173002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/idl_types.py:121: if not name.startswith('_'): On 2014/08/19 13:44:14, Nils Barth (inactive) wrote: > Do you need this? > __getattr__ is only called if an attribute is not found otherwise, > so this isn't overriding __str__, for instance. > > (I appreciate the caution and desire for safety, but I'm not sure this actually > helps.) What I meant to avoid with this was Python looking for some __x__, expecting either a valid value or an AttributeError. But I don't know that Python has any problem with invalid values, and in my testing it doesn't seem like it does.
(Latest is very clean, thanks!) https://codereview.chromium.org/474173002/diff/40001/Source/bindings/scripts/... File Source/bindings/scripts/idl_types.py (right): https://codereview.chromium.org/474173002/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/idl_types.py:121: if not name.startswith('_'): On 2014/08/19 14:30:12, Jens Widell wrote: > What I meant to avoid with this was Python looking for some __x__, expecting > either a valid value or an AttributeError. But I don't know that Python has any > problem with invalid values, and in my testing it doesn't seem like it does. Thanks for explaining; AFAICT Python doesn't need this: it will use __ attributes if present (*not* calling __getattr__, which is only used for usual attribute access, not the special access of __ attributes), but otherwise proceed fine without; details of course at: https://docs.python.org/2/reference/datamodel.html
The CQ bit was checked by jl@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jl@opera.com/474173002/120001
Message was sent while issue was closed.
Committed patchset #3 (120001) as 180564 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
