Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1541983003:
Force all experiment enabled checks to use ExperimentalFeatures (Closed)
DescriptionForce all experiment enabled checks to use ExperimentalFeatures
The original motivation for this CL was to add an overload
to the isApiEnabled() method, without an error message.
Like ExecutionContext.isSecureContext(), this allows
callers to check for enabled experiments, but not pass
an error message parameter, which would be ignored.
In adding the overload, it was determined that all callers should
be using the ExperimentalFeatures generated class
(with the exception of unit tests). As a result, the
Experiments class now just provides a private
implementation of the enabled check. Callers use the
ExperimentalFeatures class for the integrated check of
runtime-enabled + appropriate API key.
The motivation for this change was this CL to add the
[ExperimentEnabled] IDL attribute:
https://codereview.chromium.org/1531443003
BUG=551609
Committed: https://crrev.com/707a221a0a22c5a376dcb7979367b4ee1c314f77
Cr-Commit-Position: refs/heads/master@{#368471}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rebase #Patch Set 3 : Generated code uses private implementation method #Patch Set 4 : Fix link error under GN and rename entry method #
Total comments: 9
Patch Set 5 : Update comments to explain class design and code generation #
Total comments: 1
Messages
Total messages: 23 (6 generated)
chasej@chromium.org changed reviewers: + iclelland@chromium.org, rbyers@chromium.org
iclelland, rbyers, please take a look. This is a followup to iclelland's CL: https://codereview.chromium.org/1538663003/ rbyers, owner review needed for core/experiments/*.
https://codereview.chromium.org/1541983003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/templates/ExperimentalFeatures.cpp.tmpl (right): https://codereview.chromium.org/1541983003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/ExperimentalFeatures.cpp.tmpl:32: if (errorMessage) { See the comment in Experiments.cpp -- it seems wrong to have to include logic here for calling one of two methods, when they both just convert the reference back into a pointer and call into Experiments::isApiEnabledImpl. Can we just call it directly instead? https://codereview.chromium.org/1541983003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/experiments/Experiments.cpp (right): https://codereview.chromium.org/1541983003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/experiments/Experiments.cpp:67: bool Experiments::isApiEnabled(ExecutionContext* executionContext, const String& apiName) This method isn't used anywhere outside of the ExperimentalFeatures class -- do we need to maintain this version? Could we get away with just the impl here, and make it public to call it directly from ExperimentalFeatures::*impl? I think that would simplify those methods a bit. Alternately, if we're committed to *not* having regular code call the raw Experiments::isApiEnabled methods, then we could leave the impl private, and make blink::ExperimentalFeatures a friend; that would make sure that someone has to do something really obvious if they're going to bypass the ExperimentalFeatures::* methods.
https://codereview.chromium.org/1541983003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/templates/ExperimentalFeatures.cpp.tmpl (right): https://codereview.chromium.org/1541983003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/ExperimentalFeatures.cpp.tmpl:32: if (errorMessage) { On 2015/12/23 03:56:12, iclelland wrote: > See the comment in Experiments.cpp -- it seems wrong to have to include logic > here for calling one of two methods, when they both just convert the reference > back into a pointer and call into Experiments::isApiEnabledImpl. Can we just > call it directly instead? Yes, I agree it would be better to call directly into the impl. Response to your other comment has more detail. https://codereview.chromium.org/1541983003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/experiments/Experiments.cpp (right): https://codereview.chromium.org/1541983003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/experiments/Experiments.cpp:67: bool Experiments::isApiEnabled(ExecutionContext* executionContext, const String& apiName) On 2015/12/23 03:56:12, iclelland wrote: > This method isn't used anywhere outside of the ExperimentalFeatures class -- do > we need to maintain this version? Could we get away with just the impl here, > and make it public to call it directly from ExperimentalFeatures::*impl? I think > that would simplify those methods a bit. > > Alternately, if we're committed to *not* having regular code call the raw > Experiments::isApiEnabled methods, then we could leave the impl private, and > make blink::ExperimentalFeatures a friend; that would make sure that someone has > to do something really obvious if they're going to bypass the > ExperimentalFeatures::* methods. At this point, I don't believe there should be any regular code calling the raw isApiEnabled() methods. Other than unit tests, all the regular callers will need the RuntimeEnabled check included in the generated xxxEnabled methods. The public isApiEnabled() methods can be removed. I agree with keeping the impl private, to avoid inadvertent bypassing of ExperimentalFeatures::*. If not for the fact that ExperimentalFeatures is generated, I would say that Experiments::isApiEnabledImpl should be part of that class. To support code generation and unit tests, we could use the friend approach as you suggested. The other option is to make Experiments a base class for ExperimentalFeatures (subject to renaming). I believe that achieves the same goal of making it obvious if someone was to bypass ExperimentalFeatures::*. What are your thoughts on defining ExperimentalFeatures as friend vs making Experiments a base class?
On 2015/12/23 15:58:24, chasej wrote: > https://codereview.chromium.org/1541983003/diff/1/third_party/WebKit/Source/b... > File > third_party/WebKit/Source/build/scripts/templates/ExperimentalFeatures.cpp.tmpl > (right): > > https://codereview.chromium.org/1541983003/diff/1/third_party/WebKit/Source/b... > third_party/WebKit/Source/build/scripts/templates/ExperimentalFeatures.cpp.tmpl:32: > if (errorMessage) { > On 2015/12/23 03:56:12, iclelland wrote: > > See the comment in Experiments.cpp -- it seems wrong to have to include logic > > here for calling one of two methods, when they both just convert the reference > > back into a pointer and call into Experiments::isApiEnabledImpl. Can we just > > call it directly instead? > > Yes, I agree it would be better to call directly into the impl. Response to > your other comment has more detail. > > https://codereview.chromium.org/1541983003/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/experiments/Experiments.cpp (right): > > https://codereview.chromium.org/1541983003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/experiments/Experiments.cpp:67: bool > Experiments::isApiEnabled(ExecutionContext* executionContext, const String& > apiName) > On 2015/12/23 03:56:12, iclelland wrote: > > This method isn't used anywhere outside of the ExperimentalFeatures class -- > do > > we need to maintain this version? Could we get away with just the impl here, > > and make it public to call it directly from ExperimentalFeatures::*impl? I > think > > that would simplify those methods a bit. > > > > Alternately, if we're committed to *not* having regular code call the raw > > Experiments::isApiEnabled methods, then we could leave the impl private, and > > make blink::ExperimentalFeatures a friend; that would make sure that someone > has > > to do something really obvious if they're going to bypass the > > ExperimentalFeatures::* methods. > > At this point, I don't believe there should be any regular code calling the raw > isApiEnabled() methods. Other than unit tests, all the regular callers will need > the RuntimeEnabled check included in the generated xxxEnabled methods. The > public isApiEnabled() methods can be removed. I agree with keeping the impl > private, to avoid inadvertent bypassing of ExperimentalFeatures::*. > > If not for the fact that ExperimentalFeatures is generated, I would say that > Experiments::isApiEnabledImpl should be part of that class. To support code > generation and unit tests, we could use the friend approach as you suggested. > The other option is to make Experiments a base class for ExperimentalFeatures > (subject to renaming). I believe that achieves the same goal of making it > obvious if someone was to bypass ExperimentalFeatures::*. > > What are your thoughts on defining ExperimentalFeatures as friend vs making > Experiments a base class? My instincts are usually to avoid friend classes as much as possible, except for unit test drivers, but in this case I think it's warranted. If we make these protected and allow subclasses, then you can't know, just by looking at Experiments.cpp, whether there *are* any subclasses, or what they do. By making them private, we force ExperimentalFeatures to be explicitly named as a friend, and then if any other friends are added (which I don't think should happen), we ensure that they are similarly explicitly named here. I think that that method will go farther towards the goal of avoiding any abuse of the bare isApiEnabledImpl methods, and encourage the use of the generated methods as much as possible.
On 2016/01/05 16:02:25, iclelland wrote: > On 2015/12/23 15:58:24, chasej wrote: > > > https://codereview.chromium.org/1541983003/diff/1/third_party/WebKit/Source/b... > > File > > > third_party/WebKit/Source/build/scripts/templates/ExperimentalFeatures.cpp.tmpl > > (right): > > > > > https://codereview.chromium.org/1541983003/diff/1/third_party/WebKit/Source/b... > > > third_party/WebKit/Source/build/scripts/templates/ExperimentalFeatures.cpp.tmpl:32: > > if (errorMessage) { > > On 2015/12/23 03:56:12, iclelland wrote: > > > See the comment in Experiments.cpp -- it seems wrong to have to include > logic > > > here for calling one of two methods, when they both just convert the > reference > > > back into a pointer and call into Experiments::isApiEnabledImpl. Can we just > > > call it directly instead? > > > > Yes, I agree it would be better to call directly into the impl. Response to > > your other comment has more detail. > > > > > https://codereview.chromium.org/1541983003/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/experiments/Experiments.cpp (right): > > > > > https://codereview.chromium.org/1541983003/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/experiments/Experiments.cpp:67: bool > > Experiments::isApiEnabled(ExecutionContext* executionContext, const String& > > apiName) > > On 2015/12/23 03:56:12, iclelland wrote: > > > This method isn't used anywhere outside of the ExperimentalFeatures class -- > > do > > > we need to maintain this version? Could we get away with just the impl here, > > > > and make it public to call it directly from ExperimentalFeatures::*impl? I > > think > > > that would simplify those methods a bit. > > > > > > Alternately, if we're committed to *not* having regular code call the raw > > > Experiments::isApiEnabled methods, then we could leave the impl private, and > > > make blink::ExperimentalFeatures a friend; that would make sure that someone > > has > > > to do something really obvious if they're going to bypass the > > > ExperimentalFeatures::* methods. > > > > At this point, I don't believe there should be any regular code calling the > raw > > isApiEnabled() methods. Other than unit tests, all the regular callers will > need > > the RuntimeEnabled check included in the generated xxxEnabled methods. The > > public isApiEnabled() methods can be removed. I agree with keeping the impl > > private, to avoid inadvertent bypassing of ExperimentalFeatures::*. > > > > If not for the fact that ExperimentalFeatures is generated, I would say that > > Experiments::isApiEnabledImpl should be part of that class. To support code > > generation and unit tests, we could use the friend approach as you suggested. > > The other option is to make Experiments a base class for ExperimentalFeatures > > (subject to renaming). I believe that achieves the same goal of making it > > obvious if someone was to bypass ExperimentalFeatures::*. > > > > What are your thoughts on defining ExperimentalFeatures as friend vs making > > Experiments a base class? > > My instincts are usually to avoid friend classes as much as possible, except for > unit test drivers, but in this case I think it's warranted. If we make these > protected and allow subclasses, then you can't know, just by looking at > Experiments.cpp, whether there *are* any subclasses, or what they do. By making > them private, we force ExperimentalFeatures to be explicitly named as a friend, > and then if any other friends are added (which I don't think should happen), we > ensure that they are similarly explicitly named here. I think that that method > will go farther towards the goal of avoiding any abuse of the bare > isApiEnabledImpl methods, and encourage the use of the generated methods as much > as possible. FWIW I'm happy to leave API details like this to you - the owners of the experimental features API (as long as whatever you do is consistent with blink coding style). Either option sounds OK to me. Let me know once they two of you are happy with the CL and I'll do an OWNERS review.
https://codereview.chromium.org/1541983003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/ExperimentalFeatures.cpp.tmpl (right): https://codereview.chromium.org/1541983003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/ExperimentalFeatures.cpp.tmpl:23: {% endfor %} Why the separate loop? Couldn't the next function definition go before the {% endif %}? https://codereview.chromium.org/1541983003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/experiments/Experiments.cpp (right): https://codereview.chromium.org/1541983003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/experiments/Experiments.cpp:48: if (errorMessage) { Would the code be cleaner if you did the same thing that ExecutionContext::isSecureContext() does here -- if null, then reassign the errorMessage pointer to a new string, and let it get destroyed when the method returns? Then all of the "if(errorMessage)" blocks, and the ternary below, could be eliminated. https://codereview.chromium.org/1541983003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/experiments/ExperimentsTest.cpp (right): https://codereview.chromium.org/1541983003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/experiments/ExperimentsTest.cpp:114: EXPECT_EQ(("The provided key(s) are not valid for the 'This API does not exist' API."), errorMessage); Maybe make this a kExpectedErrorMessage constant at the top of the file, since it's very closely tied to kNonExistingAPIName.
Description was changed from ========== Add method overloads for experiment enabled checks without error message Some callers will check for enabled experiments, but not use any error message provided. Similar to ExecutionContext.isSecureContext(), this CL adds an overload for isApiEnabled() without an error message parameter. For example usage, see this CL to add the [ExperimentEnabled] IDL attribute: https://codereview.chromium.org/1531443003 As well, the code generation for ExperimentalFeatures will now produce corresponding overloads without an error message for each experiment. These generated methods call the appropriate overload of isApiEnabled(). BUG=551609 ========== to ========== Force all experiment enabled checks to use ExperimentalFeatures The original motivation for this CL was to add an overload to the isApiEnabled() method, without an error message. Like ExecutionContext.isSecureContext(), this allows callers to check for enabled experiments, but not pass an error message parameter, which would be ignored. In adding the overload, it was determined that all callers should be using the ExperimentalFeatures generated class (with the exception of unit tests). As a result, the Experiments class now just provides a private implementation of the enabled check. Callers use the ExperimentalFeatures class for the integrated check of runtime-enabled + appropriate API key. The motivation for this change was this CL to add the [ExperimentEnabled] IDL attribute: https://codereview.chromium.org/1531443003 BUG=551609 ==========
https://codereview.chromium.org/1541983003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/ExperimentalFeatures.cpp.tmpl (right): https://codereview.chromium.org/1541983003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/ExperimentalFeatures.cpp.tmpl:23: {% endfor %} On 2016/01/08 15:11:28, iclelland wrote: > Why the separate loop? Couldn't the next function definition go before the {% > endif %}? The separate for loops are to match the declaration order in ExperimentalFeatures.h. That is, the two xxxEnabled() methods for each feature are public, and declared first. Followed by the declaration of all the xxxEnabledImpl() private methods. https://codereview.chromium.org/1541983003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/experiments/Experiments.cpp (right): https://codereview.chromium.org/1541983003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/experiments/Experiments.cpp:48: if (errorMessage) { On 2016/01/08 15:11:28, iclelland wrote: > Would the code be cleaner if you did the same thing that > ExecutionContext::isSecureContext() does here -- if null, then reassign the > errorMessage pointer to a new string, and let it get destroyed when the method > returns? Then all of the "if(errorMessage)" blocks, and the ternary below, could > be eliminated. I did consider that the code is less clean with the extra "if (errormessage)" checks. I think it's a reasonable trade-off, to avoid the extra allocation for a string that would never be used. Of course, I haven't measured, so the benefit could be negligible. https://codereview.chromium.org/1541983003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/experiments/ExperimentsTest.cpp (right): https://codereview.chromium.org/1541983003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/experiments/ExperimentsTest.cpp:114: EXPECT_EQ(("The provided key(s) are not valid for the 'This API does not exist' API."), errorMessage); On 2016/01/08 15:11:28, iclelland wrote: > Maybe make this a kExpectedErrorMessage constant at the top of the file, since > it's very closely tied to kNonExistingAPIName. True, they are closely related. Given the expected error message is only used once, does it improve the readability of the test to define a constant? Is there any consensus on style for expected values in tests (that are used only once)?
https://codereview.chromium.org/1541983003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/experiments/Experiments.cpp (right): https://codereview.chromium.org/1541983003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/experiments/Experiments.cpp:48: if (errorMessage) { On 2016/01/08 17:37:06, chasej wrote: > On 2016/01/08 15:11:28, iclelland wrote: > > Would the code be cleaner if you did the same thing that > > ExecutionContext::isSecureContext() does here -- if null, then reassign the > > errorMessage pointer to a new string, and let it get destroyed when the method > > returns? Then all of the "if(errorMessage)" blocks, and the ternary below, > could > > be eliminated. > > I did consider that the code is less clean with the extra "if (errormessage)" > checks. I think it's a reasonable trade-off, to avoid the extra allocation for a > string that would never be used. Of course, I haven't measured, so the benefit > could be negligible. Well, if you get to the point where you're calling isSecureContext(), it's going to allocate that string anyway, so the only potential performance hit is in the error case.
Everything else is minor style quibbles, I think. This LGTM, whichever way they're resolved. https://codereview.chromium.org/1541983003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/ExperimentalFeatures.cpp.tmpl (right): https://codereview.chromium.org/1541983003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/ExperimentalFeatures.cpp.tmpl:23: {% endfor %} On 2016/01/08 17:37:06, chasej wrote: > On 2016/01/08 15:11:28, iclelland wrote: > > Why the separate loop? Couldn't the next function definition go before the {% > > endif %}? > > The separate for loops are to match the declaration order in > ExperimentalFeatures.h. That is, the two xxxEnabled() methods for each feature > are public, and declared first. Followed by the declaration of all the > xxxEnabledImpl() private methods. Acknowledged. https://codereview.chromium.org/1541983003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/experiments/ExperimentsTest.cpp (right): https://codereview.chromium.org/1541983003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/experiments/ExperimentsTest.cpp:114: EXPECT_EQ(("The provided key(s) are not valid for the 'This API does not exist' API."), errorMessage); On 2016/01/08 17:37:06, chasej wrote: > On 2016/01/08 15:11:28, iclelland wrote: > > Maybe make this a kExpectedErrorMessage constant at the top of the file, since > > it's very closely tied to kNonExistingAPIName. > > True, they are closely related. Given the expected error message is only used > once, does it improve the readability of the test to define a constant? Is there > any consensus on style for expected values in tests (that are used only once)? I think it makes it a little clearer where the magic string value comes from, but I don't know about any definitive style arguments here. I'm really fine either way.
On 2016/01/08 17:50:10, iclelland wrote: > https://codereview.chromium.org/1541983003/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/experiments/Experiments.cpp (right): > > https://codereview.chromium.org/1541983003/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/experiments/Experiments.cpp:48: if (errorMessage) > { > On 2016/01/08 17:37:06, chasej wrote: > > On 2016/01/08 15:11:28, iclelland wrote: > > > Would the code be cleaner if you did the same thing that > > > ExecutionContext::isSecureContext() does here -- if null, then reassign the > > > errorMessage pointer to a new string, and let it get destroyed when the > method > > > returns? Then all of the "if(errorMessage)" blocks, and the ternary below, > > could > > > be eliminated. > > > > I did consider that the code is less clean with the extra "if (errormessage)" > > checks. I think it's a reasonable trade-off, to avoid the extra allocation for > a > > string that would never be used. Of course, I haven't measured, so the benefit > > could be negligible. > > Well, if you get to the point where you're calling isSecureContext(), it's going > to allocate that string anyway, so the only potential performance hit is in the > error case. As our investigation shows, Document overrides both overloads of isSecureContext(), and does not allocate a string for the method without an error message. So, it seems that whenever this is used in a document context, it will actually avoid a string allocation. I know you added l-g-t-m anyway, but just wanted to record the discussion.
rbyers, please take a look. Ian and I have reached consensus on this CL, so we're looking for an OWNERS review from you.
Description was changed from ========== Force all experiment enabled checks to use ExperimentalFeatures The original motivation for this CL was to add an overload to the isApiEnabled() method, without an error message. Like ExecutionContext.isSecureContext(), this allows callers to check for enabled experiments, but not pass an error message parameter, which would be ignored. In adding the overload, it was determined that all callers should be using the ExperimentalFeatures generated class (with the exception of unit tests). As a result, the Experiments class now just provides a private implementation of the enabled check. Callers use the ExperimentalFeatures class for the integrated check of runtime-enabled + appropriate API key. The motivation for this change was this CL to add the [ExperimentEnabled] IDL attribute: https://codereview.chromium.org/1531443003 BUG=551609 ========== to ========== Force all experiment enabled checks to use ExperimentalFeatures The original motivation for this CL was to add an overload to the isApiEnabled() method, without an error message. Like ExecutionContext.isSecureContext(), this allows callers to check for enabled experiments, but not pass an error message parameter, which would be ignored. In adding the overload, it was determined that all callers should be using the ExperimentalFeatures generated class (with the exception of unit tests). As a result, the Experiments class now just provides a private implementation of the enabled check. Callers use the ExperimentalFeatures class for the integrated check of runtime-enabled + appropriate API key. The motivation for this change was this CL to add the [ExperimentEnabled] IDL attribute: https://codereview.chromium.org/1531443003 BUG=551609 ==========
LGTM with nit https://codereview.chromium.org/1541983003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/1541983003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:23: // api_name=API_NAME is used to integrate the feature with the experimental framework, so that the feature can be nit: perhaps this should be renamed to make the relationship to the EF clearer (eg: "experimental_feature_name")? Eg. do we need to be concerned that developers may blindly copy some other entry without realizing they were exposing their API to the web via the experimental framework?
On 2016/01/08 21:20:34, Rick Byers wrote: > LGTM with nit > > https://codereview.chromium.org/1541983003/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): > > https://codereview.chromium.org/1541983003/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:23: // > api_name=API_NAME is used to integrate the feature with the experimental > framework, so that the feature can be > nit: perhaps this should be renamed to make the relationship to the EF clearer > (eg: "experimental_feature_name")? Eg. do we need to be concerned that > developers may blindly copy some other entry without realizing they were > exposing their API to the web via the experimental framework? That's a good point, and I like the suggested name of "experimental_feature_name". We'll address in a separate CL, as part of a larger renaming effort. The renaming is tracked in a new issue: https://code.google.com/p/chromium/issues/detail?id=575808
The CQ bit was checked by chasej@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1541983003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1541983003/80001
On 2016/01/08 21:43:00, chasej wrote: > On 2016/01/08 21:20:34, Rick Byers wrote: > > LGTM with nit > > > > > https://codereview.chromium.org/1541983003/diff/80001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): > > > > > https://codereview.chromium.org/1541983003/diff/80001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:23: // > > api_name=API_NAME is used to integrate the feature with the experimental > > framework, so that the feature can be > > nit: perhaps this should be renamed to make the relationship to the EF clearer > > (eg: "experimental_feature_name")? Eg. do we need to be concerned that > > developers may blindly copy some other entry without realizing they were > > exposing their API to the web via the experimental framework? > > That's a good point, and I like the suggested name of > "experimental_feature_name". We'll address in a separate CL, as part of a > larger renaming effort. The renaming is tracked in a new issue: > https://code.google.com/p/chromium/issues/detail?id=575808 SGTM, thanks. Hope you don't end up having to rename your whole project - "the trial feature framework" perhaps, or maybe "selectively enabled speculative features" ;-)
Message was sent while issue was closed.
Description was changed from ========== Force all experiment enabled checks to use ExperimentalFeatures The original motivation for this CL was to add an overload to the isApiEnabled() method, without an error message. Like ExecutionContext.isSecureContext(), this allows callers to check for enabled experiments, but not pass an error message parameter, which would be ignored. In adding the overload, it was determined that all callers should be using the ExperimentalFeatures generated class (with the exception of unit tests). As a result, the Experiments class now just provides a private implementation of the enabled check. Callers use the ExperimentalFeatures class for the integrated check of runtime-enabled + appropriate API key. The motivation for this change was this CL to add the [ExperimentEnabled] IDL attribute: https://codereview.chromium.org/1531443003 BUG=551609 ========== to ========== Force all experiment enabled checks to use ExperimentalFeatures The original motivation for this CL was to add an overload to the isApiEnabled() method, without an error message. Like ExecutionContext.isSecureContext(), this allows callers to check for enabled experiments, but not pass an error message parameter, which would be ignored. In adding the overload, it was determined that all callers should be using the ExperimentalFeatures generated class (with the exception of unit tests). As a result, the Experiments class now just provides a private implementation of the enabled check. Callers use the ExperimentalFeatures class for the integrated check of runtime-enabled + appropriate API key. The motivation for this change was this CL to add the [ExperimentEnabled] IDL attribute: https://codereview.chromium.org/1531443003 BUG=551609 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Force all experiment enabled checks to use ExperimentalFeatures The original motivation for this CL was to add an overload to the isApiEnabled() method, without an error message. Like ExecutionContext.isSecureContext(), this allows callers to check for enabled experiments, but not pass an error message parameter, which would be ignored. In adding the overload, it was determined that all callers should be using the ExperimentalFeatures generated class (with the exception of unit tests). As a result, the Experiments class now just provides a private implementation of the enabled check. Callers use the ExperimentalFeatures class for the integrated check of runtime-enabled + appropriate API key. The motivation for this change was this CL to add the [ExperimentEnabled] IDL attribute: https://codereview.chromium.org/1531443003 BUG=551609 ========== to ========== Force all experiment enabled checks to use ExperimentalFeatures The original motivation for this CL was to add an overload to the isApiEnabled() method, without an error message. Like ExecutionContext.isSecureContext(), this allows callers to check for enabled experiments, but not pass an error message parameter, which would be ignored. In adding the overload, it was determined that all callers should be using the ExperimentalFeatures generated class (with the exception of unit tests). As a result, the Experiments class now just provides a private implementation of the enabled check. Callers use the ExperimentalFeatures class for the integrated check of runtime-enabled + appropriate API key. The motivation for this change was this CL to add the [ExperimentEnabled] IDL attribute: https://codereview.chromium.org/1531443003 BUG=551609 Committed: https://crrev.com/707a221a0a22c5a376dcb7979367b4ee1c314f77 Cr-Commit-Position: refs/heads/master@{#368471} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/707a221a0a22c5a376dcb7979367b4ee1c314f77 Cr-Commit-Position: refs/heads/master@{#368471} |
