Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2766213002:
Update feature list (Closed)
DescriptionUpdate feature list
Update the list to match the spec draft
https://rawgit.com/clelland/feature-policy/update-spec/index.html#feature
I changed the order of the Enums following the spec.
I kept previously defined features, but replace usermedia
by camera, speaker, and microphone.
BUG=666765
Review-Url: https://codereview.chromium.org/2766213002
Cr-Commit-Position: refs/heads/master@{#460184}
Committed: https://chromium.googlesource.com/chromium/src/+/0725ef5c886c6386de8a5dbca0700ba79e7dace3
Patch Set 1 : Initial Impl #
Total comments: 2
Patch Set 2 : Codereview: nit #
Total comments: 1
Patch Set 3 : Codereview: nit (replace draft by real spec link) #
Total comments: 2
Patch Set 4 : Codereview: nit #
Messages
Total messages: 33 (25 generated)
The CQ bit was checked by lunalu@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...
lunalu@chromium.org changed reviewers: + iclelland@chromium.org
Hi Ian, could you PTAL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_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 lunalu@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: 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
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
Description was changed from ========== Update feature list Update the list to match the spec draft https://rawgit.com/clelland/feature-policy/update-spec/index.html#feature BUG=666765 ========== to ========== Update feature list Update the list to match the spec draft https://rawgit.com/clelland/feature-policy/update-spec/index.html#feature I changed the order of the Enums following the spec. I kept previously defined features, but replace usermedia by camera, speaker, and microphone. BUG=666765 ==========
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Patchset #1 (id:20001) has been deleted
Dry run: 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
Dry run: This issue passed the CQ dry run.
Since we're re-ordering the enum fields here in any case, do you think there's a good way to break them up? I feel like we could easily have three blocks of features: - Features which are in the spec and shipped (or ready to ship) -- fullscreen, vibrate, paymentrequest - Features which are in the spec, but either not implemented or not shipped -- camera, eme, geolocation, etc. - Features which are not in the spec yet -- docwrite, cookie, etc. Both of the last two are the features behind the FeaturePolicyExperimentalFeatures flag. https://codereview.chromium.org/2766213002/diff/40001/content/common/feature_... File content/common/feature_policy/feature_policy.cc (right): https://codereview.chromium.org/2766213002/diff/40001/content/common/feature_... content/common/feature_policy/feature_policy.cc:210: {blink::WebFeaturePolicyFeature::DocumentCookie, Can you put a comment above this line -- something like "The features below this point are only available when FeaturePolicyExperimentalFeatures is enabled."? https://codereview.chromium.org/2766213002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebFeaturePolicy.h (right): https://codereview.chromium.org/2766213002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebFeaturePolicy.h:23: Eme, EncryptedMedia would be a more understandable name, I think.
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
PTASL
Dry run: 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
Dry run: This issue passed the CQ dry run.
LGTM with one comment nit https://codereview.chromium.org/2766213002/diff/40002/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebFeaturePolicy.h (right): https://codereview.chromium.org/2766213002/diff/40002/third_party/WebKit/publ... third_party/WebKit/public/platform/WebFeaturePolicy.h:20: // https://rawgit.com/clelland/feature-policy/update-spec/index.html#defined-fea... This can be https://wicg.github.io/feature-policy/#defined-features now, the new spec is published there.
lunalu@chromium.org changed reviewers: + rbyers@chromium.org
Hi Rick, could you please owner approve third_party/WebKit/Source/platform/ Thanks
WebKit LGTM with nit https://codereview.chromium.org/2766213002/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebFeaturePolicy.h (right): https://codereview.chromium.org/2766213002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebFeaturePolicy.h:16: // TODO(iclelland): Link to the spec where the behaviour for each of these is nit: can you remove the TODO now? https://codereview.chromium.org/2766213002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebFeaturePolicy.h:30: // Below are the features currently implemented in Blink: I was initially confused by this (if the others aren't implemented in blink, why mention them at all?). But now I see the "feature policy experimental features" flag. Maybe change this whole section just to say something like: "Features are defined in https://wicg.github.io/feature-policy/#defined-features. Many of these are still under development in blink behind the featurePolicyExperimentalFeatures flag, see getWebFeaturePolicyFeature()." Then you don't have to worry about keeping the comment here in-sync with the code in getWebFeaturePolicyFeature. But if you want to explicitly list the 3 that are ready, that's fine with me too (I can see the argument for it). I just wouldn't bother listing out the rest since it's redundant with the list below.
The CQ bit was checked by lunalu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from iclelland@chromium.org, rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/2766213002/#ps80001 (title: "Codereview: nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490722034828500,
"parent_rev": "295190866360ccb6ed6c31e07d8587442c6e3cfd", "commit_rev":
"0725ef5c886c6386de8a5dbca0700ba79e7dace3"}
Message was sent while issue was closed.
Description was changed from ========== Update feature list Update the list to match the spec draft https://rawgit.com/clelland/feature-policy/update-spec/index.html#feature I changed the order of the Enums following the spec. I kept previously defined features, but replace usermedia by camera, speaker, and microphone. BUG=666765 ========== to ========== Update feature list Update the list to match the spec draft https://rawgit.com/clelland/feature-policy/update-spec/index.html#feature I changed the order of the Enums following the spec. I kept previously defined features, but replace usermedia by camera, speaker, and microphone. BUG=666765 Review-Url: https://codereview.chromium.org/2766213002 Cr-Commit-Position: refs/heads/master@{#460184} Committed: https://chromium.googlesource.com/chromium/src/+/0725ef5c886c6386de8a5dbca070... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/0725ef5c886c6386de8a5dbca070... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
