Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
299203002:
Support per-overload [RuntimeEnabled] extended attribute (Closed)
|
Created:
6 years, 7 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: support per-overload [RuntimeEnabled] extended attribute
If different overloads of a method have different [RuntimeEnabled]
extended attributes, check the runtime flags as part of overload
resolution instead of when configuring the prototype object template.
If the runtime feature is disabled, the overload resolution algorithm
will act as if the method did not exist.
If all overloads have the same [RuntimeEnabled] extended attribute,
it is checked when configured the prototype object template as for
regular non-overloaded methods.
BUG=339000
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176028
Patch Set 1 #
Total comments: 18
Patch Set 2 : #
Total comments: 1
Patch Set 3 : rebased #Patch Set 4 : refactor runtime_enabled() filtering #
Total comments: 2
Patch Set 5 : rebased #Patch Set 6 : rebased #
Total comments: 9
Patch Set 7 : address nits #Patch Set 8 : move comment #
Total comments: 6
Messages
Total messages: 34 (0 generated)
PTAL. This affects some existing methods in CanvasRenderingContext2D: fill, stroke, clip, isPointInPath, isPointInStroke and drawImage, by making some runtime enabled overloads actually runtime enabled. They used to be unconditionally enabled, as far as I can tell. The drawImage overloads depend on ExperimentalCanvasFeatures (status=test) and the rest on Path2D (status=stable). In the drawImage case this means I'm actually removing some functionality that was (unintentionally) shipped and now can't even be enabled via about:flags. However, I'm told there's no way to create objects implementing the ImageBitmap interface without the Window.createImageBitmap method, which also depends on ExperimentalCanvasFeatures, and is properly unavailable when it's disabled. This means it wasn't possible to call these overloads either way. junov: Can you confirm that "removing" these drawImage(ImageBitmap, ...) overloads is safe?
https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/v8_i... File Source/bindings/scripts/v8_interface.py (right): https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/v8_i... Source/bindings/scripts/v8_interface.py:709: # Stop here unless this overload (and not all overloads) is runtime A simpler fix here would of course be to just remove the 'return' here and let the compiler eliminate the resulting dead code that would be produced in cases where no [RuntimeEnabled] is involved and there are string+number overloads: if (true) { foo1Method(info); // string return; } if (true) { foo2Method(info); // number return; }
LGTM, with a few style and comment points. Thanks, this is a long-desired feature! You might want to announce on [blink-dev] as an FYI, since this comes up often. https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/code... File Source/bindings/scripts/code_generator_v8.py (right): https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/code... Source/bindings/scripts/code_generator_v8.py:188: if len(lines) > 1: It's fine to always use a { } block, so you can eliminate this test: CG simplicity is more important than generated code polish. https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/code... Source/bindings/scripts/code_generator_v8.py:189: fmt = '%(indent)sif (%(fnname)s()) {\n %(code)s\n%(indent)s}\n' Could you use string concatenation and split the string across lines, as in existing code? (It's really hard to eyeball the multi-line code.) https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/code... Source/bindings/scripts/code_generator_v8.py:192: return fmt % { Could you use str.format instead of % with a dict? (Formally this is b/c using % with a dict is dynamic, while str.format has a static list of keywords.) Actually, if you eliminate the if clause, I think you don't need formatting, and can use basically the existing code, just with the split/join. https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/v8_i... File Source/bindings/scripts/v8_interface.py (right): https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/v8_i... Source/bindings/scripts/v8_interface.py:357: runtime_enabled_features = set(method.get('runtime_enabled_function') You don't need dict.get() here, do you? https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/v8_i... Source/bindings/scripts/v8_interface.py:359: if len(runtime_enabled_features) > 1: Shouldn't you use the utility common_value() function instead? https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/v8_i... Source/bindings/scripts/v8_interface.py:360: # More than one different value for [RuntimeEnabled] among the Could you clarify that this also works for *missing* values? E.g., f(); [RuntimeEnabled=Foo] f(long x); You have a test case, but None makes it a bit weird. https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/v8_i... Source/bindings/scripts/v8_interface.py:709: # Stop here unless this overload (and not all overloads) is runtime On 2014/05/23 14:55:16, Jens Lindström wrote: > A simpler fix here would of course be to just remove the 'return' here and let > the compiler eliminate the resulting dead code that would be produced in cases > where no [RuntimeEnabled] is involved and there are string+number overloads: > > if (true) { > foo1Method(info); // string > return; > } > if (true) { > foo2Method(info); // number > return; > } That's fine to do: CG simplicity is a higher priority than generated source code legibility, per: http://www.chromium.org/developers/design-documents/idl-compiler#TOC-Goals Could you remove the early returns here and below, and add a note about the dead code elimination in the comment beginning: # (Perform automatic type conversion, in order. Something like: "We rely on compiler dead code elimination to remove redundant branches."
https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/v8_i... File Source/bindings/scripts/v8_interface.py (right): https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/v8_i... Source/bindings/scripts/v8_interface.py:357: runtime_enabled_features = set(method.get('runtime_enabled_function') On 2014/05/27 06:33:20, Nils Barth wrote: > You don't need dict.get() here, do you? Needed for overloaded constructors currently; generate_constructor() doesn't set 'runtime_enabled_function'. Maybe it should (i.e. maybe we should support runtime enabled constructor overloads too) and either way, it could of course at least set it to None to allow us to assume it's there. Which do you prefer? :-) https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/v8_i... Source/bindings/scripts/v8_interface.py:359: if len(runtime_enabled_features) > 1: On 2014/05/27 06:33:20, Nils Barth wrote: > Shouldn't you use the utility common_value() function instead? It returns None both if there is no common value and when the common value is None. I handle those two cases differently here. https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/v8_i... Source/bindings/scripts/v8_interface.py:360: # More than one different value for [RuntimeEnabled] among the On 2014/05/27 06:33:20, Nils Barth wrote: > Could you clarify that this also works for *missing* values? > > E.g., > f(); > [RuntimeEnabled=Foo] f(long x); > > You have a test case, but None makes it a bit weird. Yeah, I'm essentially treating None as any other value for the RuntimeEnabled attribute. In your example, 'runtime_enabled_features' is set([None, 'Foo']) which looks like more than one different value and triggers per-overload handling. I can add a comment about it. I could also rewrite the code to handle None explicitly, if you like.
Thanks for comments; get() is fine, but we should make the treatment uniform with [DeprecateAs] and [MeasureAs] (suggestion below). https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/v8_i... File Source/bindings/scripts/v8_interface.py (right): https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/v8_i... Source/bindings/scripts/v8_interface.py:357: runtime_enabled_features = set(method.get('runtime_enabled_function') On 2014/05/27 06:46:20, Jens Lindström wrote: > On 2014/05/27 06:33:20, Nils Barth wrote: > > You don't need dict.get() here, do you? > > Needed for overloaded constructors currently; generate_constructor() doesn't set > 'runtime_enabled_function'. Maybe it should (i.e. maybe we should support > runtime enabled constructor overloads too) and either way, it could of course at > least set it to None to allow us to assume it's there. > > Which do you prefer? :-) Oh, good point. get() is a bit better, since having long lists of default None/False for the constructor context is fragile and gross. Thanks! https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/v8_i... Source/bindings/scripts/v8_interface.py:359: if len(runtime_enabled_features) > 1: On 2014/05/27 06:46:20, Jens Lindström wrote: > On 2014/05/27 06:33:20, Nils Barth wrote: > > Shouldn't you use the utility common_value() function instead? > > It returns None both if there is no common value and when the common value is > None. I handle those two cases differently here. Oh, good point. I'd prefer consistency with the other [DeprecateAs] and [MeasureAs] code, however you do it. Those treat it by having a separate *_all_as variable and branching in Jinja, instead of changing the variables in Python. I think you can handle these uniformly via something like: Python: 'runtime_enabled_function_all': common_value(overloads, 'runtime_enabled_function'), Jinja: {% filter runtime_enabled(not overloads.runtime_enabled_function_all and method.runtime_enabled_function) %} WDYT? https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/v8_i... Source/bindings/scripts/v8_interface.py:360: # More than one different value for [RuntimeEnabled] among the On 2014/05/27 06:46:20, Jens Lindström wrote: > Yeah, I'm essentially treating None as any other value for the RuntimeEnabled > attribute. In your example, 'runtime_enabled_features' is set([None, 'Foo']) > which looks like more than one different value and triggers per-overload > handling. > > I can add a comment about it. I could also rewrite the code to handle None > explicitly, if you like. None is confusing enough that I'd appreciate a comment, if you'd be so kind :) (The value of no value is itself a value. It's so Zen.) I think you can avoid special-casing None by using a more Jinja-y style, as outlined above.
https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/v8_i... File Source/bindings/scripts/v8_interface.py (right): https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/v8_i... Source/bindings/scripts/v8_interface.py:359: if len(runtime_enabled_features) > 1: On 2014/05/27 07:24:17, Nils Barth wrote: > Jinja: > {% filter runtime_enabled(not overloads.runtime_enabled_function_all and > method.runtime_enabled_function) %} > > WDYT? That works nicely in the overload_resolution_method() macro, but in the configure_class_template block, where the method is registered, I don't have the 'overloads' dict accessible. Almost, though. It's stored on the last method, and the method registration is generated for the first method: {% if not method.overload_index or method.overload_index == 1 %} {# For overloaded methods, only generate one accessor #} I could change that condition to {% if not method.overload_index or method.overloads %} I think, and then use {% filter runtime_enabled(method.overloads.runtime_enabled_function_all) %} to make the registration runtime enabled. Good, you think? (I haven't tested it.)
https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/v8_i... File Source/bindings/scripts/v8_interface.py (right): https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/v8_i... Source/bindings/scripts/v8_interface.py:359: if len(runtime_enabled_features) > 1: On 2014/05/27 07:39:17, Jens Lindström wrote: > On 2014/05/27 07:24:17, Nils Barth wrote: > > Jinja: > > {% filter runtime_enabled(not overloads.runtime_enabled_function_all and > > method.runtime_enabled_function) %} > > > > WDYT? > > That works nicely in the overload_resolution_method() macro, but in the > configure_class_template block, where the method is registered, I don't have the > 'overloads' dict accessible. Almost, though. It's stored on the last method, and > the method registration is generated for the first method: > > {% if not method.overload_index or method.overload_index == 1 %} > {# For overloaded methods, only generate one accessor #} > > I could change that condition to > > {% if not method.overload_index or method.overloads %} > > I think, and then use > > {% filter runtime_enabled(method.overloads.runtime_enabled_function_all) %} > > to make the registration runtime enabled. > > Good, you think? (I haven't tested it.) But that of course doesn't work for non-overloaded methods.
Thanks for tweaking! Bit confused about what issues you're running into... https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/v8_i... File Source/bindings/scripts/v8_interface.py (right): https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/v8_i... Source/bindings/scripts/v8_interface.py:359: if len(runtime_enabled_features) > 1: On 2014/05/27 07:39:17, Jens Lindström wrote: > Good, you think? (I haven't tested it.) That sounds great! https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/v8_i... Source/bindings/scripts/v8_interface.py:359: if len(runtime_enabled_features) > 1: On 2014/05/27 07:42:15, Jens Lindström wrote: > But that of course doesn't work for non-overloaded methods. ...why not? (confused)
On 2014/05/27 07:50:42, Nils Barth wrote: > https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/v8_i... > Source/bindings/scripts/v8_interface.py:359: if len(runtime_enabled_features) > > 1: > On 2014/05/27 07:42:15, Jens Lindström wrote: > > But that of course doesn't work for non-overloaded methods. > > ...why not? (confused) Because method.overloads.runtime_enabled_function_all would only be valid for overloaded methods. For non-overloaded methods, we'd still want to use method.runtime_enabled_function as before. And we can't use {% filter runtime_enabled(method.overloads.runtime_enabled_function_all or method.runtime_enabled_function) %} since we specifically don't want to use method.runtime_enabled_function on the first (or last) overload here.
On 2014/05/27 07:55:31, Jens Lindström wrote: > On 2014/05/27 07:50:42, Nils Barth wrote: > > > https://codereview.chromium.org/299203002/diff/1/Source/bindings/scripts/v8_i... > > Source/bindings/scripts/v8_interface.py:359: if len(runtime_enabled_features) > > > > 1: > > On 2014/05/27 07:42:15, Jens Lindström wrote: > > > But that of course doesn't work for non-overloaded methods. > > > > ...why not? (confused) > > Because method.overloads.runtime_enabled_function_all would only be valid for > overloaded methods. For non-overloaded methods, we'd still want to use > method.runtime_enabled_function as before. We can distinguish overloaded methods via overload_index (and last one via overloads), right? ...so there should be a way... > And we can't use > > {% filter runtime_enabled(method.overloads.runtime_enabled_function_all or > method.runtime_enabled_function) %} > > since we specifically don't want to use method.runtime_enabled_function on the > first (or last) overload here. Hmm...can't we use something like: {% set runtime_enabled_function = method.overloads.runtime_enabled_function_all if method.overloads else method.runtime_enabled_function %} ?
On 2014/05/27 08:02:12, Nils Barth wrote:
> Hmm...can't we use something like:
> {% set runtime_enabled_function =
method.overloads.runtime_enabled_function_all
> if method.overloads else
> method.runtime_enabled_function %}
> ?
That ought to work.
I'm too unfamiliar with Jinja to come up with such fancy solutions, evidently.
:-)
On 2014/05/27 08:04:58, Jens Lindström wrote:
> On 2014/05/27 08:02:12, Nils Barth wrote:
> > Hmm...can't we use something like:
> > {% set runtime_enabled_function =
> method.overloads.runtime_enabled_function_all
> > if method.overloads else
> > method.runtime_enabled_function %}
> > ?
>
> That ought to work.
>
> I'm too unfamiliar with Jinja to come up with such fancy solutions, evidently.
> :-)
That's what I'm here for ;)
(...and if this didn't work, we could put much the same logic in Python.)
Generally we tend to add extra variables for special cases instead of modifying
them, since it's confusing enough as is, and we're trying to keep it pretty
functional.
On 2014/05/27 07:39:16, Jens Lindström wrote:
> {% if not method.overload_index or method.overload_index == 1 %}
> {# For overloaded methods, only generate one accessor #}
>
> I could change that condition to
>
> {% if not method.overload_index or method.overloads %}
If the first and last method overloads have different
'number_of_required_or_variadic_arguments', this changes the 'length' property
value on the resulting Function object, I think. (It's passed in as the 'length'
argument to v8::FunctionTemplate::New() in the install_custom_signature()
macro.)
It feels a bit arbitrary to use either overload's value rather than one that is
calculated somehow from the set of overloads, but I guess the first overload is
likely the one with fewest arguments (seems natural to order the definitions
such in the IDL) so if a minimum is correct, this was correct by accident, and I
might break it now.
Aside from this, the 'runtime_enabled_function_all' variant works fine.
On 2014/05/27 08:17:07, Jens Lindström wrote: > If the first and last method overloads have different > 'number_of_required_or_variadic_arguments', this changes the 'length' property > value on the resulting Function object, I think. (It's passed in as the 'length' > argument to v8::FunctionTemplate::New() in the install_custom_signature() > macro.) > > It feels a bit arbitrary to use either overload's value rather than one that is > calculated somehow from the set of overloads, but I guess the first overload is > likely the one with fewest arguments (seems natural to order the definitions > such in the IDL) so if a minimum is correct, this was correct by accident, and I > might break it now. > > Aside from this, the 'runtime_enabled_function_all' variant works fine. Good catch! To keep this CL simple(-ish), could you fix the length in a separate CL, actually calculating it as a min? (...and finding the spec for this?) Thanks!
On 2014/05/27 08:19:01, Nils Barth wrote: > To keep this CL simple(-ish), could you fix the length in a separate CL, > actually calculating it as a min? > (...and finding the spec for this?) Yeah, I'll look up what the spec says and fix separately. (Thanks for great reviewing, BTW!)
On 2014/05/27 08:22:10, Jens Lindström wrote: > (Thanks for great reviewing, BTW!) (#^.^#) Thanks for kind words and revisions!
Updated to address review comments. Also change how 'do_not_check_signature' and 'do_generate_method_configuration' are handled on overloaded methods. This was necessary to avoid unintended and incorrect CG changes, in turn caused by using the last rather than the first overload to generate the shared code. These flags were handled incorrectly, I think. They were checked only on the first (in IDL order) overload, but applied to all overloads. Changing the order in the IDL file could have unexpected CG effects due to this. This CL now tries to handle them in a way that takes all overloads into account. nbarth@: I'll go ahead and ignore your first LGTM; it feels like I've changed the CL completely since then. :-) https://codereview.chromium.org/299203002/diff/20001/Source/bindings/tests/re... File Source/bindings/tests/results/V8TestObject.cpp (right): https://codereview.chromium.org/299203002/diff/20001/Source/bindings/tests/re... Source/bindings/tests/results/V8TestObject.cpp:9237: {"overloadedMethodA", TestObjectV8Internal::overloadedMethodAMethodCallback, 0, 2}, These length changes are incorrect and will go away again with https://codereview.chromium.org/296403007/.
Additional change in patch set 4: move runtime_enabled() filtering up to where conditional() filtering is done in configure_class_template; around all other per-method code instead of multiple times around the inner-most per-method code. Done to make the template less verbose. Affects CG to generate a little less code, but I don't think it has any noteworthy effect on what the compiler outputs in the end.
https://codereview.chromium.org/299203002/diff/60001/Source/bindings/template... File Source/bindings/templates/interface.cpp (right): https://codereview.chromium.org/299203002/diff/60001/Source/bindings/template... Source/bindings/templates/interface.cpp:995: {% filter runtime_enabled(method.overloads.runtime_enabled_function_all if method.overloads else This is wrong if all overloads are runtime enabled, but by more than one feature flag. You could end up with a method registered that supports zero overloads and thus always fails when called. (And traces of experimental features could potentially be shipped in the form of useless stub methods by mistake.) Don't know how likely it is that this would ever happen. Should we add a FIXME somewhere in v8_interface.py and raise an exception if such a set of overloads is ever defined?
> junov: Can you confirm that "removing" these drawImage(ImageBitmap, ...) > overloads is safe? confirmed.
https://codereview.chromium.org/299203002/diff/60001/Source/bindings/template... File Source/bindings/templates/interface.cpp (right): https://codereview.chromium.org/299203002/diff/60001/Source/bindings/template... Source/bindings/templates/interface.cpp:995: {% filter runtime_enabled(method.overloads.runtime_enabled_function_all if method.overloads else On 2014/06/04 11:20:28, Jens Lindström wrote: > This is wrong if all overloads are runtime enabled, but by more than one feature > flag. You could end up with a method registered that supports zero overloads and > thus always fails when called. (And traces of experimental features could > potentially be shipped in the form of useless stub methods by mistake.) > > Don't know how likely it is that this would ever happen. Should we add a FIXME > somewhere in v8_interface.py and raise an exception if such a set of overloads > is ever defined? It occurs to me that the "Function.length is dynamic" compile-time check that issue 296403007 adds is such a check, somewhat unintentionally.
Rebased onto r176017 (method registration fixes), which made this CL rather more trivial.
LGTM; thanks for splitting off the technical issues into another CL! https://codereview.chromium.org/299203002/diff/100001/Source/bindings/templat... File Source/bindings/templates/interface.cpp (left): https://codereview.chromium.org/299203002/diff/100001/Source/bindings/templat... Source/bindings/templates/interface.cpp:1004: {% filter runtime_enabled(method.runtime_enabled_function) %} Thanks, this is a lot nicer. https://codereview.chromium.org/299203002/diff/100001/Source/bindings/templat... File Source/bindings/templates/interface.cpp (right): https://codereview.chromium.org/299203002/diff/100001/Source/bindings/templat... Source/bindings/templates/interface.cpp:991: {% filter runtime_enabled(method.overloads.runtime_enabled_function_all if method.overloads else Could you wrap this line? https://codereview.chromium.org/299203002/diff/100001/Source/bindings/templat... File Source/bindings/templates/methods.cpp (right): https://codereview.chromium.org/299203002/diff/100001/Source/bindings/templat... Source/bindings/templates/methods.cpp:353: {# 10. If i = d, then: #} Could you move this comment up? https://codereview.chromium.org/299203002/diff/100001/Source/bindings/templat... Source/bindings/templates/methods.cpp:354: {% filter runtime_enabled(not overloads.runtime_enabled_function_all and method.runtime_enabled_function) %} wrap? https://codereview.chromium.org/299203002/diff/100001/Source/bindings/tests/i... File Source/bindings/tests/idls/TestObject.idl (right): https://codereview.chromium.org/299203002/diff/100001/Source/bindings/tests/i... Source/bindings/tests/idls/TestObject.idl:487: void partiallyRuntimeEnabledOverloadedVoidMethod(long longArg); Do you want a test case with multiple feature names, possibly overlapping? E.g., [RE=A] [RE=B] or: [RE=A] [RE=A&B] Not necessary if we're not supporting this, but if you're explicitly supporting this case, a test makes that clearer.
BTW, once this lands, could you post an announcement on blink-dev, as this has been a frequently requested feature and people may wish to use it? (Plus, gives a bit more visibility to the great work you're doing.) Thanks!
https://codereview.chromium.org/299203002/diff/100001/Source/bindings/templat... File Source/bindings/templates/methods.cpp (right): https://codereview.chromium.org/299203002/diff/100001/Source/bindings/templat... Source/bindings/templates/methods.cpp:353: {# 10. If i = d, then: #} On 2014/06/12 11:45:06, Nils Barth wrote: > Could you move this comment up? How far up? :-) https://codereview.chromium.org/299203002/diff/100001/Source/bindings/tests/i... File Source/bindings/tests/idls/TestObject.idl (right): https://codereview.chromium.org/299203002/diff/100001/Source/bindings/tests/i... Source/bindings/tests/idls/TestObject.idl:487: void partiallyRuntimeEnabledOverloadedVoidMethod(long longArg); On 2014/06/12 11:45:06, Nils Barth wrote: > Do you want a test case with multiple feature names, possibly overlapping? > E.g., > [RE=A] > [RE=B] > or: > [RE=A] > [RE=A&B] > Not necessary if we're not supporting this, but if you're > explicitly supporting this case, a test makes that clearer. Adding another partiallyRuntimeEnabledOverloadedVoidMethod() overload with a different [RuntimeEnabled] is cheap enough to add, and might be somewhat meaningful, but in practice doesn't exercise any additional code in the current implementation. Changing the [RuntimeEnabled] on one of the runtimeEnabledOverloadedVoidMethod() overloads is not allowed; it triggers the "Function.length depends on runtime flags" check. Makes some sense, since if both runtime flags are off, there should be no method object and thus it's length is arguably undefined.
https://codereview.chromium.org/299203002/diff/100001/Source/bindings/templat... File Source/bindings/templates/methods.cpp (right): https://codereview.chromium.org/299203002/diff/100001/Source/bindings/templat... Source/bindings/templates/methods.cpp:353: {# 10. If i = d, then: #} On 2014/06/12 12:04:38, Jens Lindström wrote: > On 2014/06/12 11:45:06, Nils Barth wrote: > > Could you move this comment up? > > How far up? :-) I love it when I say "Jump" and you reply "How high?" How about right above the |case| line? (Consistent with other usage: comment immediately before core.) https://codereview.chromium.org/299203002/diff/100001/Source/bindings/tests/i... File Source/bindings/tests/idls/TestObject.idl (right): https://codereview.chromium.org/299203002/diff/100001/Source/bindings/tests/i... Source/bindings/tests/idls/TestObject.idl:487: void partiallyRuntimeEnabledOverloadedVoidMethod(long longArg); On 2014/06/12 12:04:38, Jens Lindström wrote: > Adding another partiallyRuntimeEnabledOverloadedVoidMethod() overload with a > different [RuntimeEnabled] is cheap enough to add, and might be somewhat > meaningful, but in practice doesn't exercise any additional code in the current > implementation. > > Changing the [RuntimeEnabled] on one of the runtimeEnabledOverloadedVoidMethod() > overloads is not allowed; it triggers the "Function.length depends on runtime > flags" check. Makes some sense, since if both runtime flags are off, there > should be no method object and thus it's length is arguably undefined. Thanks for explanation; current tests are fine then!
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/299203002/160001
LGTM https://codereview.chromium.org/299203002/diff/160001/Source/bindings/tests/r... File Source/bindings/tests/results/V8TestObject.cpp (right): https://codereview.chromium.org/299203002/diff/160001/Source/bindings/tests/r... Source/bindings/tests/results/V8TestObject.cpp:6918: if (true) { This branch will never hit. Shall we use a better type so that all branches hit? https://codereview.chromium.org/299203002/diff/160001/Source/bindings/tests/r... Source/bindings/tests/results/V8TestObject.cpp:7167: if (true) { Ditto. https://codereview.chromium.org/299203002/diff/160001/Source/bindings/tests/r... Source/bindings/tests/results/V8TestObject.cpp:7339: if (true) { Ditto.
https://codereview.chromium.org/299203002/diff/160001/Source/bindings/tests/r... File Source/bindings/tests/results/V8TestObject.cpp (right): https://codereview.chromium.org/299203002/diff/160001/Source/bindings/tests/r... Source/bindings/tests/results/V8TestObject.cpp:6918: if (true) { On 2014/06/12 13:32:57, haraken wrote: > > This branch will never hit. Shall we use a better type so that all branches hit? Avoid overloading between DOMString/enum and numeric, you mean? We don't want to do that, I think, since that particular case has special handling that we ought to test. That special handling can be observed here as the IsNumber() branch. If only one of DOMString and long was supported, there would just be a single 'if (true)' branch. (We don't really need to test it three times, of course.)
https://codereview.chromium.org/299203002/diff/160001/Source/bindings/tests/r... File Source/bindings/tests/results/V8TestObject.cpp (right): https://codereview.chromium.org/299203002/diff/160001/Source/bindings/tests/r... Source/bindings/tests/results/V8TestObject.cpp:6918: if (true) { On 2014/06/12 13:40:07, Jens Lindström wrote: > On 2014/06/12 13:32:57, haraken wrote: > > > > This branch will never hit. Shall we use a better type so that all branches > hit? > > Avoid overloading between DOMString/enum and numeric, you mean? We don't want to > do that, I think, since that particular case has special handling that we ought > to test. > > That special handling can be observed here as the IsNumber() branch. If only one > of DOMString and long was supported, there would just be a single 'if (true)' > branch. > > (We don't really need to test it three times, of course.) ah, makes sense!
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/11750)
Message was sent while issue was closed.
Change committed as 176028
Message was sent while issue was closed.
https://codereview.chromium.org/299203002/diff/160001/Source/bindings/tests/r... File Source/bindings/tests/results/V8TestObject.cpp (right): https://codereview.chromium.org/299203002/diff/160001/Source/bindings/tests/r... Source/bindings/tests/results/V8TestObject.cpp:6918: if (true) { On 2014/06/12 13:32:57, haraken wrote: > This branch will never hit. Shall we use a better type so that all branches hit? This code is intentional, to keep the CG simpler. This generated code is dead code, which is eliminated by the compiler, so there's no impact on object code, but generating this anyway avoids having to special-case this logic. I initially had special cases that returned early, but the logic got more complicated once we supported more cases, so Jens is removing it. (This code is generated whenever there are 2+ types that support automatic type conversion at the distinguishing argument of an overload.) We *do* have special cases when it makes a difference though (e.g., skip an IsNumber check if it's immediately followed by an unconditional clause). The comment in the CG is: # (Perform automatic type conversion, in order. If any of these match, # that’s the end, and no other tests are needed.) To keep this code simple, # we rely on the C++ compiler's dead code elimination to deal with the # redundancy if both cases below trigger. See v8_interface.py in this CL for details: https://codereview.chromium.org/299203002/diff/160001/Source/bindings/scripts... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
