Enable Feature Policy without experimental features or fullscreen

Enable Feature Policy without experimental features or fullscreen

Intent to ship: https://groups.google.com/a/chromium.org/forum/?hl=en#!searchin/blink-dev/intent$20to$20ship$20feature$20policy/blink-dev/uKO1CwiY3ts/62vV7xmaCQAJ

BUG=623682
patch from issue 2864723002 at patchset 20001 (http://crrev.com/2864723002#ps20001)

Review-Url: https://codereview.chromium.org/2873433002
Cr-Commit-Position: refs/heads/master@{#471329}
Committed: https://chromium.googlesource.com/chromium/src/+/8f64c57f0ae067e658a1d903fbbf5cbdb44e135e

[lunalu1, 2017-05-08 18:24:00]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 18:24:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-08 19:04:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-08 19:04:57]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[lunalu1, 2017-05-08 19:36:02]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[lunalu1, 2017-05-08 19:36:20]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 19:37:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2017-05-08 19:50:52]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 19:51:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-08 21:28:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-08 21:28:28]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lunalu1, 2017-05-09 14:44:36]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[lunalu1, 2017-05-09 14:44:58]

lunalu@chromium.org changed reviewers:
+ iclelland@chromium.org

[lunalu1, 2017-05-09 14:44:59]

Hi Ian, PTAL

[commit-bot: I haz the power, 2017-05-09 14:45:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 14:50:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 14:50:45]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lunalu1, 2017-05-09 14:57:40]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 14:58:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 15:03:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 15:03:52]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[lunalu1, 2017-05-09 15:10:04]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[lunalu1, 2017-05-09 15:10:06]

Patchset #5 (id:80001) has been deleted

[lunalu1, 2017-05-09 15:10:11]

Patchset #4 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-05-09 15:10:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 17:30:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 17:30:15]

Dry run: This issue passed the CQ dry run.

[iclelland, 2017-05-09 17:58:45]

Thanks for taking care of this, Luna! This looks really good; I've added a
couple of comments below.

https://codereview.chromium.org/2873433002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/2873433002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:66: // TODO(lunalu): Re-enable
fullscreen in feature policy once all fullscreen
As mentioned offline, I don't really like this, even for a temporary measure --
and it highlights the fact that we're still doing lots of tests inside the
calling code to see whether featurepolicy wants to handle a feature. Here it's
just if (RuntimeEnabledFeatures::featurePolicyEnabled()), but in vibrate code,
it's (RuntimeEnabledFeatures::featurePolicyEnabled() &&
RuntimeEnabledFeatures::featurePolicyExperimentalFeaturesEnabled()), and in
other code, it could be different, and change whenever we promote a feature from
experimental to stable.

Maybe the right thing to do is have a method in FeaturePolicy.cpp that says
whether FP is currently handling a given feature. Then we can centralize the
featurePolicyEnabled and featurePolicyExperimentalFeaturesEnabled logic, and put
the special case for Fullscreen right there. WDYT?

https://codereview.chromium.org/2873433002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
(right):

https://codereview.chromium.org/2873433002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:133:
default_feature_name_map.Set("payment", WebFeaturePolicyFeature::kPayment);
This change looks unintentional; I'd prefer to not see it show up in the git
revision log, since nothing significant actually changed in this function.

[lunalu1, 2017-05-09 19:07:17]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[lunalu1, 2017-05-09 19:07:31]

Done. PTAL

https://codereview.chromium.org/2873433002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
(right):

https://codereview.chromium.org/2873433002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:133:
default_feature_name_map.Set("payment", WebFeaturePolicyFeature::kPayment);
On 2017/05/09 17:58:45, iclelland wrote:
> This change looks unintentional; I'd prefer to not see it show up in the git
> revision log, since nothing significant actually changed in this function.

Done.

[commit-bot: I haz the power, 2017-05-09 19:07:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[iclelland, 2017-05-09 19:14:19]

https://codereview.chromium.org/2873433002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
(right):

https://codereview.chromium.org/2873433002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:131: bool
IsSupportedInFeaturePolicy(String feature) {
Would it be possible to make this take a WebFeaturePolicyFeature, rather than a
string? I expect it would be much faster to test against, and less prone to
error when calling.

[lunalu1, 2017-05-09 19:52:44]

Done. PTAL

[iclelland, 2017-05-09 20:34:38]

On 2017/05/09 19:52:44, loonybear wrote:
> Done. PTAL

Thanks! LGTM

[lunalu1, 2017-05-09 20:52:29]

lunalu@chromium.org changed reviewers:
+ foolip@chromium.org, pfeldman@chromium.org

[lunalu1, 2017-05-09 20:52:30]

Hi pfeldman@, could you PTAL at:
content/browser/site_per_process_browsertest.cc
content/public/common/content_features.cc

Hi foolip@, could you PTAL at everything else?

Thanks

[pfeldman, 2017-05-09 21:10:00]

lgtm

[foolip, 2017-05-10 15:32:25]

lgtm % question

https://codereview.chromium.org/2873433002/diff/140001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2873433002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:3386: # Feature Policy changes
same-origin allowpaymentrequest behaviour, tests need updating
Should these tests not be updated in this CL?

[foolip, 2017-05-10 15:40:50]

Can you link to the Intent to Ship in the description too?

[lunalu1, 2017-05-10 15:47:49]

Description was changed from

==========
Enable Feature Policy without experimental features or fullscreen

BUG=623682

patch from issue 2864723002 at patchset 20001
(http://crrev.com/2864723002#ps20001)
==========

to

==========
Enable Feature Policy without experimental features or fullscreen

Intent to ship:
https://groups.google.com/a/chromium.org/forum/?hl=en#!searchin/blink-dev/int...

BUG=623682

patch from issue 2864723002 at patchset 20001
(http://crrev.com/2864723002#ps20001)
==========

[lunalu1, 2017-05-10 15:48:01]

The CQ bit was checked by lunalu@chromium.org

[commit-bot: I haz the power, 2017-05-10 15:48:36]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 17:11:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 17:11:04]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lunalu1, 2017-05-10 18:06:13]

The CQ bit was checked by lunalu@chromium.org

[lunalu1, 2017-05-10 18:06:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
iclelland@chromium.org, foolip@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2873433002/#ps160001
(title: "Fix test failure (fullscreen is still behind FeaturePolicy flag and
only disabled in-place of IsSup…")

[commit-bot: I haz the power, 2017-05-10 18:07:29]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 20:14:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 20:14:51]

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_...)

[lunalu1, 2017-05-11 18:12:44]

The CQ bit was checked by lunalu@chromium.org

[commit-bot: I haz the power, 2017-05-11 18:14:02]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 19:21:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 19:21:05]

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_...)

[lunalu1, 2017-05-12 14:28:37]

The CQ bit was checked by lunalu@chromium.org

[commit-bot: I haz the power, 2017-05-12 14:28:59]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 16:38:29]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1494599317737490,
"parent_rev": "f14c1e33e651019be3557f43121aff4a445c00b2", "commit_rev":
"8f64c57f0ae067e658a1d903fbbf5cbdb44e135e"}

[commit-bot: I haz the power, 2017-05-12 16:38:38]

Description was changed from

==========
Enable Feature Policy without experimental features or fullscreen

Intent to ship:
https://groups.google.com/a/chromium.org/forum/?hl=en#!searchin/blink-dev/int...

BUG=623682

patch from issue 2864723002 at patchset 20001
(http://crrev.com/2864723002#ps20001)
==========

to

==========
Enable Feature Policy without experimental features or fullscreen

Intent to ship:
https://groups.google.com/a/chromium.org/forum/?hl=en#!searchin/blink-dev/int...

BUG=623682

patch from issue 2864723002 at patchset 20001
(http://crrev.com/2864723002#ps20001)

Review-Url: https://codereview.chromium.org/2873433002
Cr-Commit-Position: refs/heads/master@{#471329}
Committed:
https://chromium.googlesource.com/chromium/src/+/8f64c57f0ae067e658a1d903fbbf...
==========

[commit-bot: I haz the power, 2017-05-12 16:38:42]

Committed patchset #7 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/8f64c57f0ae067e658a1d903fbbf...