Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
328663003:
IDL: restructure logic handling registration of methods (Closed)
|
Created:
6 years, 6 months ago by Jens Widell Modified:
6 years, 6 months ago CC:
blink-reviews, blink-reviews-bindings_chromium.org, arv+blink, abarth-chromium Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionIDL: restructure logic handling registration of methods
Methods are registered in one of three different ways on an interface's
prototype object;
1) via an entry in a static array,
2) via custom code in the interface's configure{{class}}Template(), or
3) via custom code in its installPerContextEnabledMethods();
depending on whether the method is static and its extended attributes.
E.g., [RuntimeEnabled] leads to 2 and [PerContextEnabled] leads to 3.
The old method of setting flags on each method signalling how it ought
to be registered, and filtering the single list of methods in the
templates based on these flags, was becoming seriously unwieldy for
complicated cases such as overloaded methods.
To address this, change the logic to instead filter methods into three
lists in the scripts, one for each type of registration. The templates
simply use a different list of methods when generating the different
code blocks.
For overloaded methods, take the attributes of all overloads into
account, instead of looking at only one of them (the first in IDL file
order, as it were.)
Also introduce a compile-time check that a set of overloaded methods
doesn't contain methods with conflicting extended attributes that affect
how the method ought to be registered.
BUG=339000
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176017
Patch Set 1 #
Total comments: 8
Patch Set 2 : #
Total comments: 3
Patch Set 3 : s/needs_/has_/ + check for conflicting ext attrs #
Total comments: 5
Patch Set 4 : address nits #Patch Set 5 : improve custom registration ext attrs check #
Total comments: 3
Patch Set 6 : rebased #
Messages
Total messages: 24 (0 generated)
PTAL These changes are extracted from the per-overload-RuntimeEnabled patch (and slightly modified from what I had done there.) Limited CG changes: http://pastebin.com/0LyNy9uU Essentially: - my Function.length fix kicks in and fixes .length on a few overloaded methods, - the order in the generated code changes in cases where overloads are spread out in the IDL file (unclear why they should be) or declared in other IDL files (partial interfaces.) https://codereview.chromium.org/328663003/diff/1/Source/bindings/scripts/v8_i... File Source/bindings/scripts/v8_interface.py (right): https://codereview.chromium.org/328663003/diff/1/Source/bindings/scripts/v8_i... Source/bindings/scripts/v8_interface.py:257: overloads['do_not_check_signature_any'] and I'm not 100 % sure about using "any" instead of "all" here, but it's correct for these cases that I'm at least somewhat familiar with: - Overloaded static methods: 'do_not_check_signature' is guaranteed false on all overloads, and thus 'do_not_check_signature_any' is false, and 'do_generate_method_configuration' is false. - Partially [RuntimeEnabled] overloaded methods: 'do_not_check_signature' will be true on those that aren't runtime enabled, and so 'do_not_check_signature_any' is true, and 'do_generate_method_configuration' is true (which is good; a method that is only partially runtime enabled is unconditionally configured.) Where this might be wrong would be overloads where only some overloads have any of the attributes [DoNotCheckSecurity], [DoNotCheckSignature], [NotEnumerable], [ReadOnly] or [Unforgeable]. Having overloads with different [NotEnumerable], [ReadOnly] or [Unforgeable] makes no sense I think, and the others I'm not really familiar with. Anyway: with the current set of overloads in current IDL files, this code does not change the generated code.
One question for haraken, otherwise fine. This is a technical CL that regularizes behavior, and the logic is fine for all real use cases. Some potential edge cases would break; these are unlikely, and not worth supporting if it's safe and would add lots of complexity, but I want to understand what could go wrong. https://codereview.chromium.org/328663003/diff/1/Source/bindings/scripts/v8_i... File Source/bindings/scripts/v8_interface.py (right): https://codereview.chromium.org/328663003/diff/1/Source/bindings/scripts/v8_i... Source/bindings/scripts/v8_interface.py:254: if 'overloads' in method: Comment: # For overloaded methods, only generate one accessor https://codereview.chromium.org/328663003/diff/1/Source/bindings/scripts/v8_i... Source/bindings/scripts/v8_interface.py:257: overloads['do_not_check_signature_any'] and haraken: Thoughts on do_not_check_signature on overloaded methods? I'm not clear on what check signature actually does, or why we're skipping it; are there security or type-checking implications? Jens: I think your code and reasoning are correct; could you put a comment somewhere that this is fiddly? (I think in generate_overloads; see below.) [DoNotCheckSecurity] is quite rare (only used in Location and Window) http://www.chromium.org/blink/webidl/blink-idl-extended-attributes#TOC-DoNotC... ...and [DoNotCheckSignature] is only used one place: (for what should be a stringifier in Window): http://www.chromium.org/blink/webidl/blink-idl-extended-attributes#TOC-Immuta... https://codereview.chromium.org/328663003/diff/1/Source/bindings/scripts/v8_i... Source/bindings/scripts/v8_interface.py:258: not overloads['per_context_enabled_function_all']) I'd slightly prefer |continue| to |else| (slightly flatter, clearly exceptional case). https://codereview.chromium.org/328663003/diff/1/Source/bindings/scripts/v8_i... Source/bindings/scripts/v8_interface.py:263: # Ignore any overload not handled by the case above Comment tweak: # Overloaded methods are handled above (only one accessor) https://codereview.chromium.org/328663003/diff/1/Source/bindings/scripts/v8_i... Source/bindings/scripts/v8_interface.py:394: 'do_not_check_signature_any': any(method.get('do_not_check_signature') Maybe a comment on this being potentially wrong if weird mix of overloads? https://codereview.chromium.org/328663003/diff/1/Source/bindings/templates/in... File Source/bindings/templates/interface.cpp (right): https://codereview.chromium.org/328663003/diff/1/Source/bindings/templates/in... Source/bindings/templates/interface.cpp:991: {% if not(method.overloads.do_not_check_signature_any if method.overloads else I think this can be: if not((method.overloads and method.overloads.do_not_check_signature_any) or method.overload_index or method.do_not_check_signature) ...which is rather more legible than a nested ternary!
BTW, thanks for digging into this and for splitting this off; these are exactly the details that cause pain and complexity!
https://codereview.chromium.org/328663003/diff/1/Source/bindings/scripts/v8_i... File Source/bindings/scripts/v8_interface.py (right): https://codereview.chromium.org/328663003/diff/1/Source/bindings/scripts/v8_i... Source/bindings/scripts/v8_interface.py:257: overloads['do_not_check_signature_any'] and On 2014/06/11 06:28:28, Nils Barth wrote: > haraken: > Thoughts on do_not_check_signature on overloaded methods? > I'm not clear on what check signature actually does, or why we're skipping it; > are there security or type-checking implications? > > Jens: I think your code and reasoning are correct; > could you put a comment somewhere that this is fiddly? > (I think in generate_overloads; see below.) > > [DoNotCheckSecurity] is quite rare (only used in Location and Window) > http://www.chromium.org/blink/webidl/blink-idl-extended-attributes#TOC-DoNotC... > > ...and [DoNotCheckSignature] is only used one place: > (for what should be a stringifier in Window): > http://www.chromium.org/blink/webidl/blink-idl-extended-attributes#TOC-Immuta... Good question. I think it doesn't make sense at all to specify [DoNotCheckSecurity] and/or [DoNotCheckSignature] on a part of overloaded methods (I cannot imagine use cases, and in fact there is no use case at the moment). We should specify [DoNotCheckSecurity] and/or [DoNotCheckSignature] on either all of the overloaded methods or none of the overloaded methods. Shall we add the check about it, instead of worrying about "any" or "all"?
On 2014/06/11 06:38:08, haraken wrote:
> I think it doesn't make sense at all to specify [DoNotCheckSecurity] and/or
> [DoNotCheckSignature] on a part of overloaded methods (I cannot imagine use
> cases, and in fact there is no use case at the moment). We should specify
> [DoNotCheckSecurity] and/or [DoNotCheckSignature] on either all of the
> overloaded methods or none of the overloaded methods.
>
> Shall we add the check about it, instead of worrying about "any" or "all"?
That's a much better answer.
Concretely:
* RuntimeEnabled *can* be specified differently, so could you *remove* this from
the
'do_not_check_signature' test, and treat it separately
* In v8_methods, could you make a tuple of the other extended attributes, like:
DO_NOT_CHECK_SIGNATURE_EXTENDED_ATTRIBUTES = (
'DoNotCheckSecurity',
'DoNotCheckSignature',
'NotEnumerable',
'ReadOnly',
'Unforgeable',
)
* In v8_interface, add a check on overloads that such ext attrs should
be either on all or none, raising an error otherwise?
(I.e., check each of these ext attrs individually.)
That seems much clearer.
On 2014/06/11 06:47:08, Nils Barth wrote:
> * In v8_methods, could you make a tuple of the other extended attributes,
like:
> DO_NOT_CHECK_SIGNATURE_EXTENDED_ATTRIBUTES = (
> 'DoNotCheckSecurity',
> 'DoNotCheckSignature',
> 'NotEnumerable',
> 'ReadOnly',
> 'Unforgeable',
> )
Is "do not check signature" a good name here at all? AFAICT, the actual meaning
is "do not generate custom registration code in configureFooTemplate()".
(This is about method['do_not_check_signature']. There is also
method['is_do_not_check_signature'], which simply reflects the
[DoNotCheckSignature] attribute, and is used separately in the templates.)
BTW, having dug into this code, I feel that we might want to restructure this a
bit more (in this CL or a follow-up): drop 'do_not_check_signature' and
'do_generate_method_configuration' and instead make one pass over all methods
that does
per_context_enabled_methods = []
configure_template_methods = []
method_configuration_methods = []
for method in methods:
if <is overload and not the primary one>:
continue
if <is per context enabled>:
per_context_enabled_methods.append(method)
continue
if <should go into configure template>:
configure_template_methods.append(method)
continue
method_configuration_methods.append(method)
and then make the templates use these derived method lists to generate code. I
believe the code will be much easier to follow then. No logic in the templates,
and we can put all the logic in one place in the script (to whatever degree we
want to.)
On 2014/06/11 07:16:09, Jens Lindström wrote: > On 2014/06/11 06:47:08, Nils Barth wrote: > > * In v8_methods, could you make a tuple of the other extended attributes, > like: > > DO_NOT_CHECK_SIGNATURE_EXTENDED_ATTRIBUTES = ( > > 'DoNotCheckSecurity', > > 'DoNotCheckSignature', > > 'NotEnumerable', > > 'ReadOnly', > > 'Unforgeable', > > ) > > Is "do not check signature" a good name here at all? AFAICT, the actual meaning > is "do not generate custom registration code in configureFooTemplate()". Good point; that's clearer! (...and avoids the conflict with is_do_not_check_signature) > BTW, having dug into this code, I feel that we might want to restructure this a > bit more (in this CL or a follow-up): drop 'do_not_check_signature' and > 'do_generate_method_configuration' and instead make one pass over all methods > that does Good point; that sounds a lot clearer, and I think fits fine in this CL. Style-wise, simple list filtering should go in Jinja, but this is a complex case (3 lists!), so we should do it in Python. Also, supporting this properly for overloads is getting really ugly, so your suggestion sounds much better - thanks!
On 2014/06/11 08:49:23, Nils Barth wrote: > On 2014/06/11 07:16:09, Jens Lindström wrote: > > On 2014/06/11 06:47:08, Nils Barth wrote: > > > * In v8_methods, could you make a tuple of the other extended attributes, > > like: > > > DO_NOT_CHECK_SIGNATURE_EXTENDED_ATTRIBUTES = ( > > > 'DoNotCheckSecurity', > > > 'DoNotCheckSignature', > > > 'NotEnumerable', > > > 'ReadOnly', > > > 'Unforgeable', > > > ) > > > > Is "do not check signature" a good name here at all? AFAICT, the actual > meaning > > is "do not generate custom registration code in configureFooTemplate()". > > Good point; that's clearer! > (...and avoids the conflict with is_do_not_check_signature) I ended up calling the global CUSTOM_REGISTRATION_EXTENDED_ATTRIBUTES, and the dictionary member 'needs_custom_registration'. Happy to change if you've got a better suggestion. > > BTW, having dug into this code, I feel that we might want to restructure this > a > > bit more (in this CL or a follow-up): drop 'do_not_check_signature' and > > 'do_generate_method_configuration' and instead make one pass over all methods > > that does > > Good point; that sounds a lot clearer, and I think fits fine in this CL. > > Style-wise, simple list filtering should go in Jinja, but this is a complex case > (3 lists!), so we should do it in Python. > > Also, supporting this properly for overloads is getting really ugly, > so your suggestion sounds much better - thanks! Done so now, while also splitting "runtime enabled" from "needs custom registration". That split, which I tried first, made the old approach with list filtering in the templates quite painful and illegible. https://codereview.chromium.org/328663003/diff/20001/Source/bindings/tests/re... File Source/bindings/tests/results/V8TestInterface.cpp (right): https://codereview.chromium.org/328663003/diff/20001/Source/bindings/tests/re... Source/bindings/tests/results/V8TestInterface.cpp:1499: prototypeTemplate->Set(v8AtomicString(isolate, "partial2StaticVoidMethod"), v8::FunctionTemplate::New(isolate, TestInterfaceImplementationV8Internal::partial2StaticVoidMethodMethodCallback, v8Undefined(), defaultSignature, 0)->GetFunction()); Note that partial2StaticVoidMethod() is still registered here. My patch now removes its registration from configureV8TestInterfaceTemplate(). That it was registered in two places, and that this place registers it on the prototype template rather than on the function template, mostly shows that [PerContextEnabled] couldn't and still can't be combined with other things that affect how a method is registered, I think.
https://codereview.chromium.org/328663003/diff/20001/Source/bindings/tests/re... File Source/bindings/tests/results/V8TestInterface.cpp (right): https://codereview.chromium.org/328663003/diff/20001/Source/bindings/tests/re... Source/bindings/tests/results/V8TestInterface.cpp:1499: prototypeTemplate->Set(v8AtomicString(isolate, "partial2StaticVoidMethod"), v8::FunctionTemplate::New(isolate, TestInterfaceImplementationV8Internal::partial2StaticVoidMethodMethodCallback, v8Undefined(), defaultSignature, 0)->GetFunction()); On 2014/06/11 09:38:13, Jens Lindström wrote: > Note that partial2StaticVoidMethod() is still registered here. My patch now > removes its registration from configureV8TestInterfaceTemplate(). Ow, ow. Agreed, this shows how easy this is to get wrong. (In practice there aren't any static members with [PerContextEnabled]; this test error is b/c of merging test cases (creating unused code paths) to minimize the number of test cases; we wanted to avoid a separate TestPartialInterface3.idl / TestPartialInterfacePerContextEnabled.idl file just for this case.) Anyway, [PerContextEnabled] is only used in 3 places: 2 methods in History 1 partial interface definition: WindowPagePopup ...so we don't want to have much complexity to support it, and treating it separately is certainly the right approach.
On 2014/06/11 09:38:14, Jens Lindström wrote: > I ended up calling the global CUSTOM_REGISTRATION_EXTENDED_ATTRIBUTES, and the > dictionary member 'needs_custom_registration'. Happy to change if you've got a > better suggestion. haraken: naming thoughts? We normally use is_ or has_ for prefixing boolean variables (only one case of needs_, needs_constructor_getter_callback, which perhaps should be changed to has_). has_ seems appropriate here. > Done so now, while also splitting "runtime enabled" from "needs custom > registration". That split, which I tried first, made the old approach with list > filtering in the templates quite painful and illegible. Thanks, that makes sense!
One meaningful code change, otherwise looks great (and rather simpler and more extensible). https://codereview.chromium.org/328663003/diff/20001/Source/bindings/scripts/... File Source/bindings/scripts/v8_methods.py (right): https://codereview.chromium.org/328663003/diff/20001/Source/bindings/scripts/... Source/bindings/scripts/v8_methods.py:150: v8_utilities.has_extended_attribute(method, CUSTOM_REGISTRATION_EXTENDED_ATTRIBUTES), nit: line break https://codereview.chromium.org/328663003/diff/40001/Source/bindings/scripts/... File Source/bindings/scripts/v8_interface.py (right): https://codereview.chromium.org/328663003/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/v8_interface.py:413: if not overloads[0].get('is_constructor'): nit: Normally we'd combine these into a single boolean expression to reduce nesting. (However, if replace body with loop as suggested in next comment, then this is fine.) https://codereview.chromium.org/328663003/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/v8_interface.py:414: if (common_value(overloads, 'is_do_not_check_security') is None or This is a bit indirect and hence fragile, going via the context values; could you instead directly test the extended attributes? Something like: overload_extended_attributes = [overload.extended_attributes for overload in overloads] for extended_attribute in v8_methods.CUSTOM_REGISTRATION_EXTENDED_ATTRIBUTES: if common_key(overload_extended_attributes, extended_attribute) is None: raise ValueError ...with an auxiliary common_key function which is just: if all(key in dict for dict in dicts): return True if all(key not in dict for dict in dicts)): return False return None How does this sound? https://codereview.chromium.org/328663003/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/v8_interface.py:417: raise ValueError('Overloads of %s have conflicting extended attributes' % overloads[0]['name']) Could you factor out a helper variable: name = overloads[0]['name'] ...at the top? https://codereview.chromium.org/328663003/diff/40001/Source/bindings/scripts/... File Source/bindings/scripts/v8_methods.py (right): https://codereview.chromium.org/328663003/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/v8_methods.py:149: 'has_custom_registration': is_static or alpha =P
BTW, could you update title and description, since CL has changed some? Thanks!
https://codereview.chromium.org/328663003/diff/40001/Source/bindings/scripts/... File Source/bindings/scripts/v8_interface.py (right): https://codereview.chromium.org/328663003/diff/40001/Source/bindings/scripts/... Source/bindings/scripts/v8_interface.py:414: if (common_value(overloads, 'is_do_not_check_security') is None or On 2014/06/11 10:10:28, Nils Barth wrote: > This is a bit indirect and hence fragile, going via the context values; > could you instead directly test the extended attributes? > Something like: > overload_extended_attributes = [overload.extended_attributes for overload in > overloads] > for extended_attribute in v8_methods.CUSTOM_REGISTRATION_EXTENDED_ATTRIBUTES: > if common_key(overload_extended_attributes, extended_attribute) is None: > raise ValueError > > ...with an auxiliary common_key function which is just: > if all(key in dict for dict in dicts): return True > if all(key not in dict for dict in dicts)): return False > return None > > How does this sound? Sounds good, except at this point I only have what is returned by generate_method(), and that doesn't include the actual extended attributes. I could make generate_method() include the extended attributes directly, but it feels a bit ugly since (as I understand it) it's a design principle to not expose the "raw" extended attributes to the templates. Other suggestions?
On 2014/06/11 09:48:43, Nils Barth wrote: > On 2014/06/11 09:38:14, Jens Lindström wrote: > > I ended up calling the global CUSTOM_REGISTRATION_EXTENDED_ATTRIBUTES, and the > > dictionary member 'needs_custom_registration'. Happy to change if you've got a > > better suggestion. > > haraken: naming thoughts? > We normally use is_ or has_ for prefixing boolean variables (only one case of > needs_, > needs_constructor_getter_callback, which perhaps should be changed to has_). > has_ seems appropriate here. has_ sounds good to me.
On 2014/06/11 10:20:57, Jens Lindström wrote: > Sounds good, except at this point I only have what is returned by > generate_method(), and that doesn't include the actual extended attributes. Ow, good point. > I could make generate_method() include the extended attributes directly, but it > feels a bit ugly since (as I understand it) it's a design principle to not > expose the "raw" extended attributes to the templates. Other suggestions? How about instead including just these attributes, via: 'custom_registration_extended_attributes': CUSTOM_REGISTRATION_EXTENDED_ATTRIBUTES.intersect(extended_attributes.iterkeys()), ...and then checking if these sets are all equal? Also, could you make CUSTOM_REGISTRATION_EXTENDED_ATTRIBUTES a frozenset instead of a tuple? (my bad)
On 2014/06/12 03:08:14, Nils Barth wrote: > How about instead including just these attributes, via: > 'custom_registration_extended_attributes': > > CUSTOM_REGISTRATION_EXTENDED_ATTRIBUTES.intersect(extended_attributes.iterkeys()), > ...and then checking if these sets are all equal? > > Also, could you make CUSTOM_REGISTRATION_EXTENDED_ATTRIBUTES a frozenset instead > of a tuple? > (my bad) Patch updated so.
On 2014/06/11 10:11:25, Nils Barth wrote: > BTW, could you update title and description, since CL has changed some? Updated. Do let me know if you had something else in mind, or think I've left something out (or have included too much.)
LGTM, thanks for revisions! https://codereview.chromium.org/328663003/diff/80001/Source/bindings/scripts/... File Source/bindings/scripts/v8_interface.py (right): https://codereview.chromium.org/328663003/diff/80001/Source/bindings/scripts/... Source/bindings/scripts/v8_interface.py:794: def common(dicts, f): Slick ;) Maybe overkill, but nice touch! https://codereview.chromium.org/328663003/diff/80001/Source/bindings/scripts/... File Source/bindings/scripts/v8_methods.py (right): https://codereview.chromium.org/328663003/diff/80001/Source/bindings/scripts/... Source/bindings/scripts/v8_methods.py:136: 'idl_type': idl_type.base_type, Thanks!
On 2014/06/12 09:35:14, Nils Barth wrote: > LGTM, thanks for revisions! Thanks! Will rebase and queue. https://codereview.chromium.org/328663003/diff/80001/Source/bindings/scripts/... File Source/bindings/scripts/v8_interface.py (right): https://codereview.chromium.org/328663003/diff/80001/Source/bindings/scripts/... Source/bindings/scripts/v8_interface.py:794: def common(dicts, f): On 2014/06/12 09:35:14, Nils Barth wrote: > Slick ;) > Maybe overkill, but nice touch! I realized I was essentially duplicating all of common_value() to implement common_key(), and felt a bit stupid doing so.
rebased
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/328663003/100001
LGTM
Message was sent while issue was closed.
Change committed as 176017 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
