Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1531443003:
[bindings] Implement an ExperimentEnabled IDL extended attribute. (Closed)
|
Created:
5 years ago by Daniel Nishi Modified:
4 years, 11 months ago CC:
chromium-reviews, blink-reviews, blink-reviews-bindings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[bindings] Implement an ExperimentEnabled IDL extended attribute.
[ExperimentEnabled] acts similarly to [RuntimeEnabled], except the
exposure depends on the origin being a secure origin that has a valid
key for the experiment. The goal is to support feature detection in JS code.
BUG=563563
Committed: https://crrev.com/aa1c4ced03fe5890aa8cd2e330b939ee3accf02a
Cr-Commit-Position: refs/heads/master@{#367432}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Less hacky and less lines. #
Total comments: 1
Patch Set 3 : Global generation fix. #Patch Set 4 : Remove 'return undefined and print a message' on constructors. #
Total comments: 16
Patch Set 5 : #Patch Set 6 : More sharing. #
Total comments: 2
Patch Set 7 : Install interfaces/attributes/methods unconditionally. Use the generated ExperimentalFeature functi… #
Total comments: 20
Patch Set 8 : Comments addressed. Much less added generations. #
Total comments: 8
Patch Set 9 : Addressing comments. #Messages
Total messages: 36 (9 generated)
dhnishi@chromium.org changed reviewers: + haraken@chromium.org
+chasej for CC. haraken: PTAL at the bindings changes. Thank you very much!
If we have [ExperimentEnabled] on window.xxx, do we really need to hide the xxx from the window object when the experiment is disabled? The requirement is adding a lot of complexity to this CL (in particular, isExperimentallyEnabled). Wouldn't the following behavior be acceptable? - window.xxx works as a valid web API if the experiment is enabled. - window.xxx exists but returns undefined with a kind error message if the experiment is disabled. https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/IDLExtendedAttributes.txt (right): https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/IDLExtendedAttributes.txt:56: ExperimentEnabled=* I'd rename "ExperimentEnabled" to "APIExperimentEnabled". We're mixing "experiment", "experiments", "experimental_api". We should unify the names. My suggestion is to use "APIExperiment" consistently. https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right): https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/v8_interface.py:69: 'core/experiments/Experiments.h', I feel that "Experiments" sounds a bit too general. "APIExperiments"? https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/scripts/v8_methods.py (right): https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/v8_methods.py:142: 'experimental_api_name': v8_utilities.experimental_api_name(method), # [ExperimentEnabled] Move this up to above the TODO comment. https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/templates/attributes.cpp (right): https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/templates/attributes.cpp:15: String errorMessage; What is this errorMessage for? https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/templates/attributes.cpp:16: if (!Experiments::isApiEnabled(currentExecutionContext(info.GetIsolate()), "{{ attribute.experimental_api_name }}", errorMessage)) { Do you have a CL to implement the isAPIEnabled? I'm fine with landing this CL without Experiments.h but want to see how it looks. https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/templates/attributes.cpp:478: 'is%sExperimentallyEnabled' % attribute.name if attribute.experimental_api_name else 'v8ConstructorAttributeGetter' %} Avoid using "if ... else ... if ... else". https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/templates/constants.cpp (right): https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/templates/constants.cpp:39: String constantsErrorMessage; What is the constantsErrorMessage for? https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/templates/constants.cpp:63: {% for constant in experimental_only_constants %} Why do we need to distinguish experimental_only_constants from experimental_enabled_constants? Are you assuming that [ExperimentEnabled] and [RuntimeEnabled] can be used at the same time? https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right): https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/templates/interface_base.cpp:293: More bindings work will be needed to have this function perfectly. #} Yeah, this is hacky... This needs to be addressed before landing this CL.
Patchset #2 (id:20001) has been deleted
dhnishi@chromium.org changed reviewers: + chasej@chromium.org
chasej@: I've added a method to experiments.h which doesn't require an error message string, in order to avoid instantiating error message strings when they would get dropped. PTAL when you get a chance. https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/IDLExtendedAttributes.txt (right): https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/IDLExtendedAttributes.txt:56: ExperimentEnabled=* On 2015/12/16 02:16:41, haraken wrote: > > I'd rename "ExperimentEnabled" to "APIExperimentEnabled". > > We're mixing "experiment", "experiments", "experimental_api". We should unify > the names. My suggestion is to use "APIExperiment" consistently. > Done. Now "APIExperiment" and "api_experiment" are used. https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right): https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/v8_interface.py:69: 'core/experiments/Experiments.h', On 2015/12/16 02:16:41, haraken wrote: > > I feel that "Experiments" sounds a bit too general. "APIExperiments"? core/experiments/* is already landed by a different author, but I can look into if the owners are open to changing the name. https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/scripts/v8_methods.py (right): https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/scripts/v8_methods.py:142: 'experimental_api_name': v8_utilities.experimental_api_name(method), # [ExperimentEnabled] On 2015/12/16 02:16:41, haraken wrote: > > Move this up to above the TODO comment. Done. https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/templates/attributes.cpp (right): https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/templates/attributes.cpp:15: String errorMessage; On 2015/12/16 02:16:41, haraken wrote: > > What is this errorMessage for? It explains why an experiment may have failed. When the error is unnecessary, I've changed the patch to not take a useless error string. https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/templates/attributes.cpp:16: if (!Experiments::isApiEnabled(currentExecutionContext(info.GetIsolate()), "{{ attribute.experimental_api_name }}", errorMessage)) { On 2015/12/16 02:16:41, haraken wrote: > > Do you have a CL to implement the isAPIEnabled? > > I'm fine with landing this CL without Experiments.h but want to see how it > looks. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/templates/attributes.cpp:478: 'is%sExperimentallyEnabled' % attribute.name if attribute.experimental_api_name else 'v8ConstructorAttributeGetter' %} On 2015/12/16 02:16:41, haraken wrote: > > Avoid using "if ... else ... if ... else". I've removed this by eliminating is%sExperimentallyEnabled. https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/templates/constants.cpp (right): https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/templates/constants.cpp:39: String constantsErrorMessage; On 2015/12/16 02:16:41, haraken wrote: > > What is the constantsErrorMessage for? Removed. https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/templates/constants.cpp:63: {% for constant in experimental_only_constants %} On 2015/12/16 02:16:42, haraken wrote: > > Why do we need to distinguish experimental_only_constants from > experimental_enabled_constants? > > Are you assuming that [ExperimentEnabled] and [RuntimeEnabled] can be used at > the same time? Yes. This is being done so that they interact properly together. https://codereview.chromium.org/1531443003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/templates/attributes.cpp (right): https://codereview.chromium.org/1531443003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/templates/attributes.cpp:219: {% if attribute.api_experiment_name %} I've replaced the is%sExperimentallyEnabled with this. This implements the return undefined and give a friendly message about why it failed concept.
On 2015/12/16 21:42:03, Daniel Nishi wrote: > chasej@: I've added a method to experiments.h which doesn't require an error > message string, in order to avoid instantiating error message strings when they > would get dropped. PTAL when you get a chance. > > https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... > File third_party/WebKit/Source/bindings/IDLExtendedAttributes.txt (right): > > https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... > third_party/WebKit/Source/bindings/IDLExtendedAttributes.txt:56: > ExperimentEnabled=* > On 2015/12/16 02:16:41, haraken wrote: > > > > I'd rename "ExperimentEnabled" to "APIExperimentEnabled". > > > > We're mixing "experiment", "experiments", "experimental_api". We should unify > > the names. My suggestion is to use "APIExperiment" consistently. > > > > Done. Now "APIExperiment" and "api_experiment" are used. > > https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... > File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right): > > https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... > third_party/WebKit/Source/bindings/scripts/v8_interface.py:69: > 'core/experiments/Experiments.h', > On 2015/12/16 02:16:41, haraken wrote: > > > > I feel that "Experiments" sounds a bit too general. "APIExperiments"? > > core/experiments/* is already landed by a different author, but I can look into > if the owners are open to changing the name. > > https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... > File third_party/WebKit/Source/bindings/scripts/v8_methods.py (right): > > https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... > third_party/WebKit/Source/bindings/scripts/v8_methods.py:142: > 'experimental_api_name': v8_utilities.experimental_api_name(method), # > [ExperimentEnabled] > On 2015/12/16 02:16:41, haraken wrote: > > > > Move this up to above the TODO comment. > > Done. > > https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... > File third_party/WebKit/Source/bindings/templates/attributes.cpp (right): > > https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... > third_party/WebKit/Source/bindings/templates/attributes.cpp:15: String > errorMessage; > On 2015/12/16 02:16:41, haraken wrote: > > > > What is this errorMessage for? > > It explains why an experiment may have failed. > > When the error is unnecessary, I've changed the patch to not take a useless > error string. > > https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... > third_party/WebKit/Source/bindings/templates/attributes.cpp:16: if > (!Experiments::isApiEnabled(currentExecutionContext(info.GetIsolate()), "{{ > attribute.experimental_api_name }}", errorMessage)) { > On 2015/12/16 02:16:41, haraken wrote: > > > > Do you have a CL to implement the isAPIEnabled? > > > > I'm fine with landing this CL without Experiments.h but want to see how it > > looks. > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... > third_party/WebKit/Source/bindings/templates/attributes.cpp:478: > 'is%sExperimentallyEnabled' % attribute.name if attribute.experimental_api_name > else 'v8ConstructorAttributeGetter' %} > On 2015/12/16 02:16:41, haraken wrote: > > > > Avoid using "if ... else ... if ... else". > > I've removed this by eliminating is%sExperimentallyEnabled. > > https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... > File third_party/WebKit/Source/bindings/templates/constants.cpp (right): > > https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... > third_party/WebKit/Source/bindings/templates/constants.cpp:39: String > constantsErrorMessage; > On 2015/12/16 02:16:41, haraken wrote: > > > > What is the constantsErrorMessage for? > > Removed. > > https://codereview.chromium.org/1531443003/diff/1/third_party/WebKit/Source/b... > third_party/WebKit/Source/bindings/templates/constants.cpp:63: {% for constant > in experimental_only_constants %} > On 2015/12/16 02:16:42, haraken wrote: > > > > Why do we need to distinguish experimental_only_constants from > > experimental_enabled_constants? > > > > Are you assuming that [ExperimentEnabled] and [RuntimeEnabled] can be used at > > the same time? > > Yes. This is being done so that they interact properly together. > > https://codereview.chromium.org/1531443003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/bindings/templates/attributes.cpp (right): > > https://codereview.chromium.org/1531443003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/templates/attributes.cpp:219: {% if > attribute.api_experiment_name %} > I've replaced the is%sExperimentallyEnabled with this. This implements the > return undefined and give a friendly message about why it failed concept. Thanks for the update! I'm happy to resume reviewing this CL once the discussion in blink-dev@ has settled :)
Patchset #4 (id:80001) has been deleted
This patch removes the case of "return undefined, but the field still exists." https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right): https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/interface_base.cpp:386: {% filter experimental_framework_runtime_enabled(attribute.api_experiment_name) %} If the ExperimentalFramework runtime flag is enabled (which it currently is not), this will install all [APIExperimentalFramework] interface constructors. If the ExperimentalFramework runtime flag is not enabled, this will install no [APIExperimentalFramework] interfaces constructors. These are the two cases that will affect the DOMWindow's top level attributes.
https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/IDLExtendedAttributes.txt (right): https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/IDLExtendedAttributes.txt:37: APIExperimentEnabled=* Alphabetical order. https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/v8_attributes.py (right): https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_attributes.py:509: context['needs_constructor_getter_callback'] = context['measure_as'] or context['deprecate_as'] or context['api_experiment_name'] Can we remove this now because we've decided to unconditionally expose experimentally-enabled interfaces? https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/v8_utilities.py (right): https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_utilities.py:368: def is_experimental_api(interface): is_experimental_api => is_api_experiment_enabled https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right): https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/interface_base.cpp:440: method.api_experiment_name) %} It's not really nice that we have to duplicate code between runtime_enabled and api_experiment_enabled. Would it be possible to just reuse the implementation for runtime_enabled? The only difference between runtime_enabled and api_experiment_enabled is whether we need to use isAPIExperimentEnabled or not. https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/methods.cpp (right): https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp:421: method.api_experiment_name) %} Ditto. Want to avoid the code duplication. https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp:715: method.api_experiment_name) %} Ditto. Want to avoid the code duplication. https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/experiments/Experiments.h (right): https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/experiments/Experiments.h:28: static bool isApiEnabledWithoutMessage(ExecutionContext*, const String& apiName); I'd prefer: - isAPIEnabled (two args version) calls isAPIEnabledInternal - isAPIEnabled (three args version) calls isAPIEnabledInternal
Thanks a ton for the quick review. Please take another look when you get a chance. https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/IDLExtendedAttributes.txt (right): https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/IDLExtendedAttributes.txt:37: APIExperimentEnabled=* On 2015/12/18 02:35:51, haraken wrote: > > Alphabetical order. Done, I think. https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/v8_attributes.py (right): https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_attributes.py:509: context['needs_constructor_getter_callback'] = context['measure_as'] or context['deprecate_as'] or context['api_experiment_name'] On 2015/12/18 02:35:51, haraken wrote: > > Can we remove this now because we've decided to unconditionally expose > experimentally-enabled interfaces? Yes and done. https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/v8_utilities.py (right): https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_utilities.py:368: def is_experimental_api(interface): On 2015/12/18 02:35:51, haraken wrote: > > is_experimental_api => is_api_experiment_enabled Done. https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right): https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/interface_base.cpp:440: method.api_experiment_name) %} On 2015/12/18 02:35:51, haraken wrote: > > It's not really nice that we have to duplicate code between runtime_enabled and > api_experiment_enabled. Would it be possible to just reuse the implementation > for runtime_enabled? The only difference between runtime_enabled and > api_experiment_enabled is whether we need to use isAPIExperimentEnabled or not. In code_generator_v8, I've shared the implementation between runtime_enabled and api_experiment_enabled. I've also shared the logic regarding the method.overloads when possible. https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/methods.cpp (right): https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp:421: method.api_experiment_name) %} On 2015/12/18 02:35:51, haraken wrote: > > Ditto. Want to avoid the code duplication. Hm. The (not x_all and x) logic on this particular one is dense enough that I'm not sure I can solve the duplication without overcomplicating it. Unlike the other two, it doesn't share the method.overloads check. https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp:715: method.api_experiment_name) %} On 2015/12/18 02:35:51, haraken wrote: > > Ditto. Want to avoid the code duplication. Done in the same manner w/ sharing the method.overloads logic and sharing logic between the filters. https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/experiments/Experiments.h (right): https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/experiments/Experiments.h:28: static bool isApiEnabledWithoutMessage(ExecutionContext*, const String& apiName); On 2015/12/18 02:35:51, haraken wrote: > > I'd prefer: > > - isAPIEnabled (two args version) calls isAPIEnabledInternal > - isAPIEnabled (three args version) calls isAPIEnabledInternal > Done.
https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/methods.cpp (right): https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/methods.cpp:421: method.api_experiment_name) %} On 2015/12/18 05:11:42, Daniel Nishi wrote: > On 2015/12/18 02:35:51, haraken wrote: > > > > Ditto. Want to avoid the code duplication. > > Hm. The (not x_all and x) logic on this particular one is dense enough that I'm > not sure I can solve the duplication without overcomplicating it. Unlike the > other two, it doesn't share the method.overloads check. BTW, do we really need to differentiate [APIExperimentEnabled] from [RuntimeEnabled]? From the perspective that it is a mechanism to conditionally enable DOM features, these two are the same thing. Just an idea but would it be possible to implement the API-experiment on top of the existing mechanism of RuntimeEnabledFeatures? For example, you can introduce status=api-experiment to RuntimeEnabledFeature.in. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
On 2015/12/18 05:20:03, haraken wrote: > https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/bindings/templates/methods.cpp (right): > > https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/templates/methods.cpp:421: > method.api_experiment_name) %} > On 2015/12/18 05:11:42, Daniel Nishi wrote: > > On 2015/12/18 02:35:51, haraken wrote: > > > > > > Ditto. Want to avoid the code duplication. > > > > Hm. The (not x_all and x) logic on this particular one is dense enough that > I'm > > not sure I can solve the duplication without overcomplicating it. Unlike the > > other two, it doesn't share the method.overloads check. > > BTW, do we really need to differentiate [APIExperimentEnabled] from > [RuntimeEnabled]? From the perspective that it is a mechanism to conditionally > enable DOM features, these two are the same thing. > > Just an idea but would it be possible to implement the API-experiment on top of > the existing mechanism of RuntimeEnabledFeatures? For example, you can introduce > status=api-experiment to RuntimeEnabledFeature.in. > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Conceptually, I think of [RuntimeEnabled] as being set globally during runtime. (i.e. if I turn on the flag or run Chromium with a certain command line flag, the feature is enabled in the browser.) The existing statuses convey that a feature is enabled, for example, for all tests (test), or for all pages when the flag is enabled (experimental), or for everything in the stable release (stable). [ExperimentEnabled] conveys that it will be using the more dynamic Experimental Framework, and all of the caveats that go with that. Keeping them separate also avoids any risk of overloading the term "experimental" when talking about runtime features, especially because "experimental" is a status for RuntimeEnabledFeatures.
On 2015/12/18 17:56:39, Daniel Nishi wrote: > On 2015/12/18 05:20:03, haraken wrote: > > > https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/bindings/templates/methods.cpp (right): > > > > > https://codereview.chromium.org/1531443003/diff/100001/third_party/WebKit/Sou... > > third_party/WebKit/Source/bindings/templates/methods.cpp:421: > > method.api_experiment_name) %} > > On 2015/12/18 05:11:42, Daniel Nishi wrote: > > > On 2015/12/18 02:35:51, haraken wrote: > > > > > > > > Ditto. Want to avoid the code duplication. > > > > > > Hm. The (not x_all and x) logic on this particular one is dense enough that > > I'm > > > not sure I can solve the duplication without overcomplicating it. Unlike the > > > other two, it doesn't share the method.overloads check. > > > > BTW, do we really need to differentiate [APIExperimentEnabled] from > > [RuntimeEnabled]? From the perspective that it is a mechanism to conditionally > > enable DOM features, these two are the same thing. > > > > Just an idea but would it be possible to implement the API-experiment on top > of > > the existing mechanism of RuntimeEnabledFeatures? For example, you can > introduce > > status=api-experiment to RuntimeEnabledFeature.in. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Conceptually, I think of [RuntimeEnabled] as being set globally during runtime. > (i.e. if I turn on the flag or run Chromium with a certain command line flag, > the feature is enabled in the browser.) The existing statuses convey that a > feature is enabled, for example, for all tests (test), or for all pages when the > flag is enabled (experimental), or for everything in the stable release > (stable). > > [ExperimentEnabled] conveys that it will be using the more dynamic Experimental > Framework, and all of the caveats that go with that. > > Keeping them separate also avoids any risk of overloading the term > "experimental" when talking about runtime features, especially because > "experimental" is a status for RuntimeEnabledFeatures. I agree that the two are conceptually different features. What I'm saying is that the two are very similar in that they are features to enable or disable DOM interfaces/methods/attributes. So we could share the implementation more.
I understand. I've changed the patch to no use an additional filter and re-use more code from runtime_enabled_features. https://codereview.chromium.org/1531443003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right): https://codereview.chromium.org/1531443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/interface_base.cpp:386: {% for attribute in attributes This catches the case of APIExperimentEnabled, but not RuntimeEnabled. I believe I missed this case above.
Thanks for the update. One question (I should have noticed this earlier...): installDOMClassTemplate installs DOM attributes/methods on a FunctionTemplate. The FunctionTemplate is cached on V8PerIsolateData.h (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). This means that the FunctionTemplate is per isolate, not per context. Then the following scenario can happen: 1) You are in a frame F. At the first time you access the window object, the FunctionTemplate of the window interface is instantiated. At that point you validate the origin of the frame F and install xxx on the FunctionTemplate (by using installDOMClassTemplate). window.xxx is now available. 2) You call into a function in a child frame F2, which has a different origin from F. The FunctionTemplate of the window interface cached on the V8PerIsolateData is used. window.xxx is available. This is not expected. This behavior is no problem for [RuntimeEnabled] because in case of [RuntimeEnabled] we want to enable/disable the feature per isolate. However, the behavior is a problem for [APIExperimentEnabled] because we want to enable/disable the feature per context. One option is to make the FunctionTemplate per context, but that wouldn't be acceptable for start-up performance... (We've moved the FunctionTemplate from per-context to per-isolate to optimize the performance of V8 initialization.) Am I understanding correctly?
On 2015/12/21 23:46:33, haraken wrote: > Thanks for the update. > > One question (I should have noticed this earlier...): > > installDOMClassTemplate installs DOM attributes/methods on a FunctionTemplate. > The FunctionTemplate is cached on V8PerIsolateData.h > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). > This means that the FunctionTemplate is per isolate, not per context. Then the > following scenario can happen: > > 1) You are in a frame F. At the first time you access the window object, the > FunctionTemplate of the window interface is instantiated. At that point you > validate the origin of the frame F and install xxx on the FunctionTemplate (by > using installDOMClassTemplate). window.xxx is now available. > > 2) You call into a function in a child frame F2, which has a different origin > from F. The FunctionTemplate of the window interface cached on the > V8PerIsolateData is used. window.xxx is available. This is not expected. > > This behavior is no problem for [RuntimeEnabled] because in case of > [RuntimeEnabled] we want to enable/disable the feature per isolate. However, the > behavior is a problem for [APIExperimentEnabled] because we want to > enable/disable the feature per context. > > One option is to make the FunctionTemplate per context, but that wouldn't be > acceptable for start-up performance... (We've moved the FunctionTemplate from > per-context to per-isolate to optimize the performance of V8 initialization.) > > Am I understanding correctly? I think you are understanding correctly. Given that information, I think we would have a problem when it comes to frames sharing the isolate. Would this pose an immediate problem, given that the window.[API] API experiments are exposed unconditionally currently?
On 2015/12/22 00:16:55, Daniel Nishi wrote: > On 2015/12/21 23:46:33, haraken wrote: > > Thanks for the update. > > > > One question (I should have noticed this earlier...): > > > > installDOMClassTemplate installs DOM attributes/methods on a FunctionTemplate. > > The FunctionTemplate is cached on V8PerIsolateData.h > > > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). > > This means that the FunctionTemplate is per isolate, not per context. Then the > > following scenario can happen: > > > > 1) You are in a frame F. At the first time you access the window object, the > > FunctionTemplate of the window interface is instantiated. At that point you > > validate the origin of the frame F and install xxx on the FunctionTemplate (by > > using installDOMClassTemplate). window.xxx is now available. > > > > 2) You call into a function in a child frame F2, which has a different origin > > from F. The FunctionTemplate of the window interface cached on the > > V8PerIsolateData is used. window.xxx is available. This is not expected. > > > > This behavior is no problem for [RuntimeEnabled] because in case of > > [RuntimeEnabled] we want to enable/disable the feature per isolate. However, > the > > behavior is a problem for [APIExperimentEnabled] because we want to > > enable/disable the feature per context. > > > > One option is to make the FunctionTemplate per context, but that wouldn't be > > acceptable for start-up performance... (We've moved the FunctionTemplate from > > per-context to per-isolate to optimize the performance of V8 initialization.) > > > > Am I understanding correctly? > > I think you are understanding correctly. Given that information, I think we > would have a problem when it comes to frames sharing the isolate. > > Would this pose an immediate problem, given that the window.[API] API > experiments are exposed unconditionally currently? For window.xxx, it doesn't pose an immediate problem because we've decided to expose the interface on window unconditionally. For other interface.xxx, it poses the problem. Actually isAPIExperimentEnabled check does any meaningful work because whether the interface.xxx is exposed or not may have been determined by another context in the same isolate. In other words, I'm afraid that the concept of "exposing APIs per context depending on its origin" makes zero sense with the current implementation :/
On 2015/12/22 01:23:55, haraken wrote: > On 2015/12/22 00:16:55, Daniel Nishi wrote: > > On 2015/12/21 23:46:33, haraken wrote: > > > Thanks for the update. > > > > > > One question (I should have noticed this earlier...): > > > > > > installDOMClassTemplate installs DOM attributes/methods on a > FunctionTemplate. > > > The FunctionTemplate is cached on V8PerIsolateData.h > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). > > > This means that the FunctionTemplate is per isolate, not per context. Then > the > > > following scenario can happen: > > > > > > 1) You are in a frame F. At the first time you access the window object, the > > > FunctionTemplate of the window interface is instantiated. At that point you > > > validate the origin of the frame F and install xxx on the FunctionTemplate > (by > > > using installDOMClassTemplate). window.xxx is now available. > > > > > > 2) You call into a function in a child frame F2, which has a different > origin > > > from F. The FunctionTemplate of the window interface cached on the > > > V8PerIsolateData is used. window.xxx is available. This is not expected. > > > > > > This behavior is no problem for [RuntimeEnabled] because in case of > > > [RuntimeEnabled] we want to enable/disable the feature per isolate. However, > > the > > > behavior is a problem for [APIExperimentEnabled] because we want to > > > enable/disable the feature per context. > > > > > > One option is to make the FunctionTemplate per context, but that wouldn't be > > > acceptable for start-up performance... (We've moved the FunctionTemplate > from > > > per-context to per-isolate to optimize the performance of V8 > initialization.) > > > > > > Am I understanding correctly? > > > > I think you are understanding correctly. Given that information, I think we > > would have a problem when it comes to frames sharing the isolate. > > > > Would this pose an immediate problem, given that the window.[API] API > > experiments are exposed unconditionally currently? > > For window.xxx, it doesn't pose an immediate problem because we've decided to > expose the interface on window unconditionally. > > For other interface.xxx, it poses the problem. Actually isAPIExperimentEnabled > check does any meaningful work because whether the interface.xxx is exposed or s/does/does not do/ > not may have been determined by another context in the same isolate. > > In other words, I'm afraid that the concept of "exposing APIs per context > depending on its origin" makes zero sense with the current implementation :/
https://codereview.chromium.org/1531443003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/experiments/Experiments.h (right): https://codereview.chromium.org/1531443003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/experiments/Experiments.h:28: static bool isApiEnabled(ExecutionContext*, const String& apiName); Makes sense to add this overload without an error message. The addition of an errorMessage parameter was based on ExecutionContext.isSecureContext(), which provides an overload without the parameter. Note that crrev.com/1538663003/ has landed, which added code generation to provide explicit ExperimentalFeatures::xxxEnabled() checks for every experiment. The bindings code should use those generated methods, rather than calling isApiEnabled() directly. Additionally, the generated "xxxEnabled()" method will first check if the corresponding runtime enabled feature. I believe this could simplify the logic for the bindings code: - If [ExperimentEnabled] and [RuntimeEnabled] attributes are found, for the *same* name, the [RuntimeEnabled] attribute can be ignored. The [ExperimentEnabled] attribute can take precedence. The generated methods do not currently provide an overload without an error message. I'll add that in a separate CL. This also raises the point of naming and potential confusion between [RuntimeEnabled] and [ExperimentEnabled]. That's a larger issue that goes beyond the bindings code. We'll need to address separately, so I'm going to start a thread for discussion.
On 2015/12/22 01:23:55, haraken wrote: > On 2015/12/22 00:16:55, Daniel Nishi wrote: > > On 2015/12/21 23:46:33, haraken wrote: > > > Thanks for the update. > > > > > > One question (I should have noticed this earlier...): > > > > > > installDOMClassTemplate installs DOM attributes/methods on a > FunctionTemplate. > > > The FunctionTemplate is cached on V8PerIsolateData.h > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). > > > This means that the FunctionTemplate is per isolate, not per context. Then > the > > > following scenario can happen: > > > > > > 1) You are in a frame F. At the first time you access the window object, the > > > FunctionTemplate of the window interface is instantiated. At that point you > > > validate the origin of the frame F and install xxx on the FunctionTemplate > (by > > > using installDOMClassTemplate). window.xxx is now available. > > > > > > 2) You call into a function in a child frame F2, which has a different > origin > > > from F. The FunctionTemplate of the window interface cached on the > > > V8PerIsolateData is used. window.xxx is available. This is not expected. > > > > > > This behavior is no problem for [RuntimeEnabled] because in case of > > > [RuntimeEnabled] we want to enable/disable the feature per isolate. However, > > the > > > behavior is a problem for [APIExperimentEnabled] because we want to > > > enable/disable the feature per context. > > > > > > One option is to make the FunctionTemplate per context, but that wouldn't be > > > acceptable for start-up performance... (We've moved the FunctionTemplate > from > > > per-context to per-isolate to optimize the performance of V8 > initialization.) > > > > > > Am I understanding correctly? > > > > I think you are understanding correctly. Given that information, I think we > > would have a problem when it comes to frames sharing the isolate. > > > > Would this pose an immediate problem, given that the window.[API] API > > experiments are exposed unconditionally currently? > > For window.xxx, it doesn't pose an immediate problem because we've decided to > expose the interface on window unconditionally. > > For other interface.xxx, it poses the problem. Actually isAPIExperimentEnabled > check does any meaningful work because whether the interface.xxx is exposed or > not may have been determined by another context in the same isolate. > > In other words, I'm afraid that the concept of "exposing APIs per context > depending on its origin" makes zero sense with the current implementation :/ Just brainstorming here, correct me if I'm wrong. Because we cache the template, could we do something along the lines of what I had in Patch Set #2, which determines if we actually want to return the template (or undefined) when the accessor for it is called? This allows us to install the template unconditionally, check the calling context to ensure the origin every time the interface is accessed, and avoid the problem of FunctionTemplate caching the template for non-enabled iFrame contexts because it is intended to be installed all the time. Do you think this solution adequately resolves the problem?
On 2015/12/22 18:25:51, Daniel Nishi wrote: > On 2015/12/22 01:23:55, haraken wrote: > > On 2015/12/22 00:16:55, Daniel Nishi wrote: > > > On 2015/12/21 23:46:33, haraken wrote: > > > > Thanks for the update. > > > > > > > > One question (I should have noticed this earlier...): > > > > > > > > installDOMClassTemplate installs DOM attributes/methods on a > > FunctionTemplate. > > > > The FunctionTemplate is cached on V8PerIsolateData.h > > > > > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). > > > > This means that the FunctionTemplate is per isolate, not per context. Then > > the > > > > following scenario can happen: > > > > > > > > 1) You are in a frame F. At the first time you access the window object, > the > > > > FunctionTemplate of the window interface is instantiated. At that point > you > > > > validate the origin of the frame F and install xxx on the FunctionTemplate > > (by > > > > using installDOMClassTemplate). window.xxx is now available. > > > > > > > > 2) You call into a function in a child frame F2, which has a different > > origin > > > > from F. The FunctionTemplate of the window interface cached on the > > > > V8PerIsolateData is used. window.xxx is available. This is not expected. > > > > > > > > This behavior is no problem for [RuntimeEnabled] because in case of > > > > [RuntimeEnabled] we want to enable/disable the feature per isolate. > However, > > > the > > > > behavior is a problem for [APIExperimentEnabled] because we want to > > > > enable/disable the feature per context. > > > > > > > > One option is to make the FunctionTemplate per context, but that wouldn't > be > > > > acceptable for start-up performance... (We've moved the FunctionTemplate > > from > > > > per-context to per-isolate to optimize the performance of V8 > > initialization.) > > > > > > > > Am I understanding correctly? > > > > > > I think you are understanding correctly. Given that information, I think we > > > would have a problem when it comes to frames sharing the isolate. > > > > > > Would this pose an immediate problem, given that the window.[API] API > > > experiments are exposed unconditionally currently? > > > > For window.xxx, it doesn't pose an immediate problem because we've decided to > > expose the interface on window unconditionally. > > > > For other interface.xxx, it poses the problem. Actually isAPIExperimentEnabled > > check does any meaningful work because whether the interface.xxx is exposed or > > not may have been determined by another context in the same isolate. > > > > In other words, I'm afraid that the concept of "exposing APIs per context > > depending on its origin" makes zero sense with the current implementation :/ > > Just brainstorming here, correct me if I'm wrong. > > Because we cache the template, could we do something along the lines of what I > had in Patch Set #2, which determines if we actually want to return the template > (or undefined) when the accessor for it is called? This allows us to install the > template unconditionally, check the calling context to ensure the origin every > time the interface is accessed, and avoid the problem of FunctionTemplate > caching the template for non-enabled iFrame contexts because it is intended to > be installed all the time. > > Do you think this solution adequately resolves the problem? Just to confirm: Your proposal is: - Install the DOM interfaces/attributes/methods unconditionally. The FunctionTemplate contains the DOM interfaces/attributes/methods even if the API experiment is disabled. - Do the isAPIEnabled check every time the DOM interface/attribute/method is accessed. We return undefined if the API experiment is disabled. I think it will work, although the isAPIEnabled check might be a bit too heavy. I'm fine with going with the approach at the moment. We can worry about the performance issue when it becomes a real issue.
Patchset #7 (id:160001) has been deleted
Okay, there are some significant changes. 1. We are now unconditionally installing the experimental attributes/methods/constants/etc. The generated code for the ExperimentalFeatures.cpp checks the runtime enabled status, so I have made the runtime-enabled-function-name to be None to stop it from generating runtime_enabled filter guards for experimental features. 2. We are now using the generated experimental features bindings code. 3. Due to the nature of the generated experimental features code, we no longer have a case where something is experimental, but not runtime enabled. This is because the RuntimeEnabledFeatures.in is used to generate the experimental check functions. I have removed all tests which tested experimental + not runtime enabled. haraken@ and chasej@: PTAL
Thanks for the update! We're getting pretty close. Mostly LG. https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/v8_attributes.py (right): https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_attributes.py:108: 'api_experiment_enabled': v8_utilities.api_experiment_enabled_function(attribute), # [APIExperimentEnabled] Alphabetical order. https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right): https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_interface.py:614: 'api_experiment_inherited_enabled': v8_utilities.api_experiment_enabled_function(interface), # [APIExperimentEnabled] api_experiment_inherited_enabled => api_experiment_enabled_per_interface ? https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/v8_utilities.py (right): https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_utilities.py:390: def is_api_experiment_enabled(interface): interface => definition_or_member ? https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_utilities.py:401: if not feature_name or not experiment_name: Why do we need to check 'not feature_name'? Aren't we just ignoring the [RuntimeEnabled] if [APIExperimentEnabled] is specified at the same time? https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_utilities.py:416: # If an experiment is on the method/attribute, it overrides the runtime experiment => API experiment ("experiment" sounds too generic.) https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/api_experiment.cpp (right): https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/api_experiment.cpp:1: {% macro check_api_experiment_internal(errorName, experiment_name) %} Could you move this macro to conversions.cpp (and avoid introducing this file)? Actually conversions.cpp should be renamed to utilities.cpp... (You don't need to rename it in this CL.) https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/api_experiment.cpp:6: Document& document = *toDocument(currentExecutionContext(info.GetIsolate())); Avoid calling currentExecutionContext twice. https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/attributes.cpp (right): https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/attributes.cpp:14: {% filter trim %} Why is trim needed? https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl (right): https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl:544: [APIExperimentEnabled=FeatureName2, RuntimeEnabled=FeatureName1] void partiallyExperimentEnabledOverloadedVoidMethod(TestInterface testInterfaceArg); If both [APIExperimentEnabled] and [RuntimeEnabled] are specified, the [RuntimeEnabled] is just ignored, right? If so, can we assert in the IDL compiler that it is invalid to specify the two IDL attributes at the same time (and remove these test cases)? https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/tests/results/core/V8ArrayBuffer.cpp (right): https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8ArrayBuffer.cpp:17: #include "core/inspector/ConsoleMessage.h" Can we include these files only when necessary?
Patchset #8 (id:200001) has been deleted
Patchset #8 (id:220001) has been deleted
https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/v8_attributes.py (right): https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_attributes.py:108: 'api_experiment_enabled': v8_utilities.api_experiment_enabled_function(attribute), # [APIExperimentEnabled] On 2015/12/30 00:25:32, haraken wrote: > > Alphabetical order. Done. https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right): https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_interface.py:614: 'api_experiment_inherited_enabled': v8_utilities.api_experiment_enabled_function(interface), # [APIExperimentEnabled] On 2015/12/30 00:25:32, haraken wrote: > > api_experiment_inherited_enabled => api_experiment_enabled_per_interface ? Done. https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/v8_utilities.py (right): https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_utilities.py:390: def is_api_experiment_enabled(interface): On 2015/12/30 00:25:32, haraken wrote: > > interface => definition_or_member ? Done. https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_utilities.py:401: if not feature_name or not experiment_name: On 2015/12/30 00:25:32, haraken wrote: > > Why do we need to check 'not feature_name'? Aren't we just ignoring the > [RuntimeEnabled] if [APIExperimentEnabled] is specified at the same time? https://codereview.chromium.org/1538663003/ uses the FeatureName to generate the ExperimentalFeatures::%sEnabled function. Without a feature name, we don't know what to use. https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_utilities.py:416: # If an experiment is on the method/attribute, it overrides the runtime On 2015/12/30 00:25:32, haraken wrote: > > experiment => API experiment > > ("experiment" sounds too generic.) Done. https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/api_experiment.cpp (right): https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/api_experiment.cpp:1: {% macro check_api_experiment_internal(errorName, experiment_name) %} On 2015/12/30 00:25:32, haraken wrote: > > Could you move this macro to conversions.cpp (and avoid introducing this file)? > > Actually conversions.cpp should be renamed to utilities.cpp... (You don't need > to rename it in this CL.) Done. https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/api_experiment.cpp:6: Document& document = *toDocument(currentExecutionContext(info.GetIsolate())); On 2015/12/30 00:25:32, haraken wrote: > > Avoid calling currentExecutionContext twice. Done. https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/attributes.cpp (right): https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/attributes.cpp:14: {% filter trim %} On 2015/12/30 00:25:32, haraken wrote: > > Why is trim needed? I was having spacing issues because the macro was returning nothing sometimes, causing the "| indent| to leave 4 whitespaces, which caused a presubmit error. By adding an if-statement guard around the macro, I've avoided the problem and fixed the whitespace issues that the trim caused. https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl (right): https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl:544: [APIExperimentEnabled=FeatureName2, RuntimeEnabled=FeatureName1] void partiallyExperimentEnabledOverloadedVoidMethod(TestInterface testInterfaceArg); On 2015/12/30 00:25:33, haraken wrote: > > If both [APIExperimentEnabled] and [RuntimeEnabled] are specified, the > [RuntimeEnabled] is just ignored, right? > > If so, can we assert in the IDL compiler that it is invalid to specify the two > IDL attributes at the same time (and remove these test cases)? The [RuntimeEnabled] is used to generate the ExperimentalFeatures::%sEnabled function, so we need to have both specified. I was unaware of this until I looked at the generation patch that landed. https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/tests/results/core/V8ArrayBuffer.cpp (right): https://codereview.chromium.org/1531443003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/tests/results/core/V8ArrayBuffer.cpp:17: #include "core/inspector/ConsoleMessage.h" On 2015/12/30 00:25:33, haraken wrote: > > Can we include these files only when necessary? Done.
Mostly look good. The final round of comments... https://codereview.chromium.org/1531443003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/v8_attributes.py (right): https://codereview.chromium.org/1531443003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_attributes.py:520: context['needs_constructor_getter_callback'] = context['measure_as'] or context['deprecate_as'] or context['api_experiment_name'] Do we need this change? https://codereview.chromium.org/1531443003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/v8_methods.py (right): https://codereview.chromium.org/1531443003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_methods.py:136: 'api_experiment_enabled_per_interface': v8_utilities.api_experiment_enabled_function(interface), # [APIExperimentEnabled] BTW, is it valid that [APIExperimentEnabled] is specified on both interface and attribute/method? If it is invalid (or not worth introducing the complexity), we can just assert it. Then we don't need to prepare two variables (i.e., api_experiment_enabled and api_experiment_enabled_per_interface). https://codereview.chromium.org/1531443003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/utilities.cpp (right): https://codereview.chromium.org/1531443003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/utilities.cpp:64: document.addConsoleMessage(ConsoleMessage::create(JSMessageSource, ErrorMessageLevel, {{errorName}})); toDocument(executionContextFromIsolate).addConsoleMessage https://codereview.chromium.org/1531443003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/utilities.cpp:72: ExecutionContext* executionContextFromIsolate = currentExecutionContext({{isolate}}); executionContextFromIsolate => executionContext (or currentExecutionContext)
https://codereview.chromium.org/1531443003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/v8_attributes.py (right): https://codereview.chromium.org/1531443003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_attributes.py:520: context['needs_constructor_getter_callback'] = context['measure_as'] or context['deprecate_as'] or context['api_experiment_name'] On 2015/12/31 12:47:22, haraken wrote: > > Do we need this change? This is needed so that the check_api_experiment's code is generated for constructors. https://codereview.chromium.org/1531443003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/scripts/v8_methods.py (right): https://codereview.chromium.org/1531443003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/scripts/v8_methods.py:136: 'api_experiment_enabled_per_interface': v8_utilities.api_experiment_enabled_function(interface), # [APIExperimentEnabled] On 2015/12/31 12:47:22, haraken wrote: > > BTW, is it valid that [APIExperimentEnabled] is specified on both interface and > attribute/method? If it is invalid (or not worth introducing the complexity), we > can just assert it. Then we don't need to prepare two variables (i.e., > api_experiment_enabled and api_experiment_enabled_per_interface). It's valid. It's possible that different API experiments will be enabled at both levels, so we'll need to check both. https://codereview.chromium.org/1531443003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/templates/utilities.cpp (right): https://codereview.chromium.org/1531443003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/utilities.cpp:64: document.addConsoleMessage(ConsoleMessage::create(JSMessageSource, ErrorMessageLevel, {{errorName}})); On 2015/12/31 12:47:22, haraken wrote: > > toDocument(executionContextFromIsolate).addConsoleMessage Done. https://codereview.chromium.org/1531443003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/templates/utilities.cpp:72: ExecutionContext* executionContextFromIsolate = currentExecutionContext({{isolate}}); On 2015/12/31 12:47:22, haraken wrote: > > executionContextFromIsolate => executionContext (or currentExecutionContext) Done.
Thanks, LGTM
The CQ bit was checked by dhnishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1531443003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1531443003/260001
Message was sent while issue was closed.
Committed patchset #9 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== [bindings] Implement an ExperimentEnabled IDL extended attribute. [ExperimentEnabled] acts similarly to [RuntimeEnabled], except the exposure depends on the origin being a secure origin that has a valid key for the experiment. The goal is to support feature detection in JS code. BUG=563563 ========== to ========== [bindings] Implement an ExperimentEnabled IDL extended attribute. [ExperimentEnabled] acts similarly to [RuntimeEnabled], except the exposure depends on the origin being a secure origin that has a valid key for the experiment. The goal is to support feature detection in JS code. BUG=563563 Committed: https://crrev.com/aa1c4ced03fe5890aa8cd2e330b939ee3accf02a Cr-Commit-Position: refs/heads/master@{#367432} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/aa1c4ced03fe5890aa8cd2e330b939ee3accf02a Cr-Commit-Position: refs/heads/master@{#367432} |
