Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2505663002:
Add unit tests for testing Feature Policy features in frames, and for testing isFeatureEnabledForOr… (Closed)
DescriptionAdd additional unit tests for Feature Policy.
This adds tests for the blink::isFeatureEnabledInFrame and FeaturePolicy::isFeatureEnabledForOrigin APIs
BUG=623682
Committed: https://crrev.com/f600666a8e1bf5f0227ace0f5a9c8ca75bb7eeab
Cr-Commit-Position: refs/heads/master@{#432991}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Nit #
Messages
Total messages: 27 (12 generated)
The CQ bit was checked by iclelland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add unit tests for testing Feature Policy features in frames, and for testing isFeatureEnabledForOrigin BUG= ========== to ========== Add additional unit tests for Feature Policy. This adds tests for the blink::isFeatureEnabledInFrame and FeaturePolicy::isFeatureEnabledForOrigin APIs BUG=623682 ==========
iclelland@chromium.org changed reviewers: + mkwst@chromium.org, raymes@chromium.org
+r raymes -- Can you take a look at the new tests? +r mkwst -- Can you PTAL as owner as well? (build file, new friend in FeaturePolicy.h) This extends the test coverage of the FeaturePolicy code; with this , we have pretty good coverage of the in-frame policy behaviour. Future CLs will add tests for the FrameLoader, browser tests for cross-process policy replication, and layout tests for specific features.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FeaturePolicyInFrameTest.cpp (right): https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FeaturePolicyInFrameTest.cpp:115: EXPECT_FALSE(isFeatureEnabledInFrame(kDefaultOffFeature, frame())); Hmm I would have expected this to be true? Where will the feature's status be checked when feature policy is disabled? https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp (right): https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:702: TEST_F(FeaturePolicyTest, TestFeatureEnabledForOrigin) { Hmm - is this not covered by other tests? https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:708: // and disabled for origin c. nit: C
https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FeaturePolicyInFrameTest.cpp (right): https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FeaturePolicyInFrameTest.cpp:115: EXPECT_FALSE(isFeatureEnabledInFrame(kDefaultOffFeature, frame())); On 2016/11/16 02:55:39, raymes wrote: > Hmm I would have expected this to be true? Where will the feature's status be > checked when feature policy is disabled? isFeatureEnabledInFrame will use the default state of a feature to decide what to do if we haven't turned FeaturePolicy on through runtime flags. Default-on and default-self features will are enabled; default-off features are disabled. (See https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...) We could switch it to enable everything -- but the point is academic, really, since we don't actually have any default-off features in Chromium, and we won't until we ship FP, as far as I know. (They would necessarily be feature-policy-opt-in-only APIs) https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp (right): https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:702: TEST_F(FeaturePolicyTest, TestFeatureEnabledForOrigin) { On 2016/11/16 02:55:39, raymes wrote: > Hmm - is this not covered by other tests? Only implicitly; feature policy inheritance would break in subtle ways if this method ever broke, and I'd like to have a test that pinpoints the problem in that case. The isFeatureEnabled method, which is tested many times above, and calls isFeatureEnabledForOrigin) only tests a policy against its own origin, so we don't have test coverage of the method that way either.
https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FeaturePolicyInFrameTest.cpp (right): https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FeaturePolicyInFrameTest.cpp:115: EXPECT_FALSE(isFeatureEnabledInFrame(kDefaultOffFeature, frame())); On 2016/11/16 04:41:04, iclelland wrote: > On 2016/11/16 02:55:39, raymes wrote: > > Hmm I would have expected this to be true? Where will the feature's status be > > checked when feature policy is disabled? > > isFeatureEnabledInFrame will use the default state of a feature to decide what > to do if we haven't turned FeaturePolicy on through runtime flags. Default-on > and default-self features will are enabled; default-off features are disabled. > (See > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...) > > We could switch it to enable everything -- but the point is academic, really, > since we don't actually have any default-off features in Chromium, and we won't > until we ship FP, as far as I know. (They would necessarily be > feature-policy-opt-in-only APIs) I guess the question is what happens if we do ship features that are off-by-default, and FP-only, and then one day we have to turn FP off (I'm imagining a situation where it's shipped, but still under Finch control.) Would we want to immediately enable those features in all contexts?
lgtm besides some minor thoughts https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FeaturePolicyInFrameTest.cpp (right): https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FeaturePolicyInFrameTest.cpp:115: EXPECT_FALSE(isFeatureEnabledInFrame(kDefaultOffFeature, frame())); On 2016/11/16 14:03:45, iclelland wrote: > On 2016/11/16 04:41:04, iclelland wrote: > > On 2016/11/16 02:55:39, raymes wrote: > > > Hmm I would have expected this to be true? Where will the feature's status > be > > > checked when feature policy is disabled? > > > > isFeatureEnabledInFrame will use the default state of a feature to decide what > > to do if we haven't turned FeaturePolicy on through runtime flags. Default-on > > and default-self features will are enabled; default-off features are disabled. > > (See > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...) > > > > We could switch it to enable everything -- but the point is academic, really, > > since we don't actually have any default-off features in Chromium, and we > won't > > until we ship FP, as far as I know. (They would necessarily be > > feature-policy-opt-in-only APIs) > I guess the question is what happens if we do ship features that are > off-by-default, and FP-only, and then one day we have to turn FP off (I'm > imagining a situation where it's shipped, but still under Finch control.) Would > we want to immediately enable those features in all contexts? Right - I guess it depends whether the notion of "off-by-default" is one which is only relevant for feature policy. If we had an off-by-default feature, would there be a way to enable it other than feature policy? I guess I would lean toward changing isFeatureEnabledInFrame to just return true if feature policy is disabled for now (and it can be made more complicated later if needed). But I also agree that it's not a huge point :) https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp (right): https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:702: TEST_F(FeaturePolicyTest, TestFeatureEnabledForOrigin) { On 2016/11/16 04:41:04, iclelland wrote: > On 2016/11/16 02:55:39, raymes wrote: > > Hmm - is this not covered by other tests? > > Only implicitly; feature policy inheritance would break in subtle ways if this > method ever broke, and I'd like to have a test that pinpoints the problem in > that case. > > The isFeatureEnabled method, which is tested many times above, and calls > isFeatureEnabledForOrigin) only tests a policy against its own origin, so we > don't have test coverage of the method that way either. Ah ok that makes sense! https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:708: // and disabled for origin c. On 2016/11/16 02:55:39, raymes wrote: > nit: C nit: C
lgtm besides some minor thoughts
On Thu, 17 Nov 2016 at 10:10 <raymes@chromium.org> wrote: > lgtm besides some minor thoughts > > > > > https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/frame/FeaturePolicyInFrameTest.cpp > (right): > > > https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/frame/FeaturePolicyInFrameTest.cpp:115: > EXPECT_FALSE(isFeatureEnabledInFrame(kDefaultOffFeature, frame())); > On 2016/11/16 14:03:45, iclelland wrote: > > On 2016/11/16 04:41:04, iclelland wrote: > > > On 2016/11/16 02:55:39, raymes wrote: > > > > Hmm I would have expected this to be true? Where will the > feature's status > > be > > > > checked when feature policy is disabled? > > > > > > isFeatureEnabledInFrame will use the default state of a feature to > decide what > > > to do if we haven't turned FeaturePolicy on through runtime flags. > Default-on > > > and default-self features will are enabled; default-off features are > disabled. > > > (See > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... > ) > > > > > > We could switch it to enable everything -- but the point is > academic, really, > > > since we don't actually have any default-off features in Chromium, > and we > > won't > > > until we ship FP, as far as I know. (They would necessarily be > > > feature-policy-opt-in-only APIs) > > I guess the question is what happens if we do ship features that are > > off-by-default, and FP-only, and then one day we have to turn FP off > (I'm > > imagining a situation where it's shipped, but still under Finch > control.) Would > > we want to immediately enable those features in all contexts? > > Right - I guess it depends whether the notion of "off-by-default" is one > which is only relevant for feature policy. If we had an off-by-default > feature, would there be a way to enable it other than feature policy? > > I guess I would lean toward changing isFeatureEnabledInFrame to just > return true if feature policy is disabled for now (and it can be made > more complicated later if needed). But I also agree that it's not a huge > point :) > I just noticed it got more complicated in https://codereview.chromium.org/2492623002 so it's probably ok :) > > > > https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/p... > File > third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp > (right): > > > https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/p... > > third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:702: > TEST_F(FeaturePolicyTest, TestFeatureEnabledForOrigin) { > On 2016/11/16 04:41:04, iclelland wrote: > > On 2016/11/16 02:55:39, raymes wrote: > > > Hmm - is this not covered by other tests? > > > > Only implicitly; feature policy inheritance would break in subtle ways > if this > > method ever broke, and I'd like to have a test that pinpoints the > problem in > > that case. > > > > The isFeatureEnabled method, which is tested many times above, and > calls > > isFeatureEnabledForOrigin) only tests a policy against its own origin, > so we > > don't have test coverage of the method that way either. > > Ah ok that makes sense! > > > > https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/p... > > third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:708: > // and disabled for origin c. > On 2016/11/16 02:55:39, raymes wrote: > > nit: C > > nit: C > > https://codereview.chromium.org/2505663002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Thu, 17 Nov 2016 at 10:10 <raymes@chromium.org> wrote: > lgtm besides some minor thoughts > > > > > https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/frame/FeaturePolicyInFrameTest.cpp > (right): > > > https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/frame/FeaturePolicyInFrameTest.cpp:115: > EXPECT_FALSE(isFeatureEnabledInFrame(kDefaultOffFeature, frame())); > On 2016/11/16 14:03:45, iclelland wrote: > > On 2016/11/16 04:41:04, iclelland wrote: > > > On 2016/11/16 02:55:39, raymes wrote: > > > > Hmm I would have expected this to be true? Where will the > feature's status > > be > > > > checked when feature policy is disabled? > > > > > > isFeatureEnabledInFrame will use the default state of a feature to > decide what > > > to do if we haven't turned FeaturePolicy on through runtime flags. > Default-on > > > and default-self features will are enabled; default-off features are > disabled. > > > (See > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... > ) > > > > > > We could switch it to enable everything -- but the point is > academic, really, > > > since we don't actually have any default-off features in Chromium, > and we > > won't > > > until we ship FP, as far as I know. (They would necessarily be > > > feature-policy-opt-in-only APIs) > > I guess the question is what happens if we do ship features that are > > off-by-default, and FP-only, and then one day we have to turn FP off > (I'm > > imagining a situation where it's shipped, but still under Finch > control.) Would > > we want to immediately enable those features in all contexts? > > Right - I guess it depends whether the notion of "off-by-default" is one > which is only relevant for feature policy. If we had an off-by-default > feature, would there be a way to enable it other than feature policy? > > I guess I would lean toward changing isFeatureEnabledInFrame to just > return true if feature policy is disabled for now (and it can be made > more complicated later if needed). But I also agree that it's not a huge > point :) > I just noticed it got more complicated in https://codereview.chromium.org/2492623002 so it's probably ok :) > > > > https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/p... > File > third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp > (right): > > > https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/p... > > third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:702: > TEST_F(FeaturePolicyTest, TestFeatureEnabledForOrigin) { > On 2016/11/16 04:41:04, iclelland wrote: > > On 2016/11/16 02:55:39, raymes wrote: > > > Hmm - is this not covered by other tests? > > > > Only implicitly; feature policy inheritance would break in subtle ways > if this > > method ever broke, and I'd like to have a test that pinpoints the > problem in > > that case. > > > > The isFeatureEnabled method, which is tested many times above, and > calls > > isFeatureEnabledForOrigin) only tests a policy against its own origin, > so we > > don't have test coverage of the method that way either. > > Ah ok that makes sense! > > > > https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/p... > > third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:708: > // and disabled for origin c. > On 2016/11/16 02:55:39, raymes wrote: > > nit: C > > nit: C > > https://codereview.chromium.org/2505663002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> I just noticed it got more complicated in https://codereview.chromium.org/2492623002 so it's probably ok :) Thanks :) https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp (right): https://codereview.chromium.org/2505663002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:708: // and disabled for origin c. On 2016/11/16 23:10:14, raymes wrote: > On 2016/11/16 02:55:39, raymes wrote: > > nit: C > > nit: C Done.
LGTM! Thank you. :)
The CQ bit was checked by iclelland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2505663002/#ps20001 (title: "Nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by iclelland@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add additional unit tests for Feature Policy. This adds tests for the blink::isFeatureEnabledInFrame and FeaturePolicy::isFeatureEnabledForOrigin APIs BUG=623682 ========== to ========== Add additional unit tests for Feature Policy. This adds tests for the blink::isFeatureEnabledInFrame and FeaturePolicy::isFeatureEnabledForOrigin APIs BUG=623682 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add additional unit tests for Feature Policy. This adds tests for the blink::isFeatureEnabledInFrame and FeaturePolicy::isFeatureEnabledForOrigin APIs BUG=623682 ========== to ========== Add additional unit tests for Feature Policy. This adds tests for the blink::isFeatureEnabledInFrame and FeaturePolicy::isFeatureEnabledForOrigin APIs BUG=623682 Committed: https://crrev.com/f600666a8e1bf5f0227ace0f5a9c8ca75bb7eeab Cr-Commit-Position: refs/heads/master@{#432991} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/f600666a8e1bf5f0227ace0f5a9c8ca75bb7eeab Cr-Commit-Position: refs/heads/master@{#432991} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
