Implementation for feature policy - vibrate.

Implementation for feature policy - vibrate.
Disable Navigator.vibrate() unless enabled through feature policy.

Added tests with runtime flag is on

Also modified IsFeatureEnabledInFrame for the case EnableForSelf, should only be enabled when with same origin.

BUG=623682
Committed: https://crrev.com/11eca67f18be0dfbfcb1703a21abb454e5f85b05
Cr-Commit-Position: refs/heads/master@{#434423}

[lunalu1, 2016-11-09 21:18:26]

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

[commit-bot: I haz the power, 2016-11-09 21:18:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2016-11-09 21:36:49]

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

[commit-bot: I haz the power, 2016-11-09 21:37:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2016-11-09 21:38:57]

Patchset #1 (id:1) has been deleted

[lunalu1, 2016-11-09 21:39:49]

Description was changed from

==========
Implementation for feature policy - vibrate.
Disable Navigator.vibrate() unless enabled through feature policy.

BUG=
==========

to

==========
Implementation for feature policy - vibrate.
Disable Navigator.vibrate() unless enabled through feature policy.

BUG=623682
==========

[commit-bot: I haz the power, 2016-11-09 23:11:02]

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

[commit-bot: I haz the power, 2016-11-09 23:11:03]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[lunalu1, 2016-11-10 19:13:52]

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

[commit-bot: I haz the power, 2016-11-10 19:14:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2016-11-10 19:24:32]

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

[commit-bot: I haz the power, 2016-11-10 19:24:46]

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, 2016-11-10 20:56:21]

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

[commit-bot: I haz the power, 2016-11-10 20:56:22]

Dry run: This issue passed the CQ dry run.

[lunalu1, 2016-11-15 20:12:56]

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

[commit-bot: I haz the power, 2016-11-15 20:13: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, 2016-11-15 20:34:43]

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

[commit-bot: I haz the power, 2016-11-15 20:34:44]

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, 2016-11-15 21:40:24]

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

[lunalu1, 2016-11-15 21:40:25]

Hi Ian,

Could you please take a look at this CL?

Thanks

[iclelland, 2016-11-16 15:25:12]

This is looking good, thanks!

The test HTML file shouldn't be moved into the virtual test suite; that can stay
where it is, and we should leave the original -expected.txt file there as well.
That will ensure that we still run the test without the FeaturePolicy flag
turned on, too.

The virtual directory should just contain the -expected.txt file for when the
flag is turned on, it doesn't need to contain any test files of its own.

https://codereview.chromium.org/2492623002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp (right):

https://codereview.chromium.org/2492623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:90: } else if
(frame->isCrossOriginSubframe()) {
I think we could actually move this check into blink::isFeatureEnabledInFrame as
the default behaviour for any features of type EnableForSelf. Then we would only
need one check, whether or not RuntimeEnabledFeatures::featurePolicyEnabled() is
set.

(We might still want to do the runtime check to decide what message to display)

[iclelland, 2016-11-16 15:27:04]

https://codereview.chromium.org/2492623002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/vibrate_in_cross_origin_iframe_blocked-expected.txt
(right):

https://codereview.chromium.org/2492623002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/vibrate_in_cross_origin_iframe_blocked-expected.txt:2:
this frame.
You're going to want to un-wrap this line -- the output from the test will all
show up on one line, not broken into two like this.

[lunalu1, 2016-11-16 16:15:24]

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

[commit-bot: I haz the power, 2016-11-16 16:15:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2016-11-16 16:20:40]

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

[commit-bot: I haz the power, 2016-11-16 16:20:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2016-11-16 16:22:09]

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

[lunalu1, 2016-11-16 16:22:20]

Patchset #4 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-11-16 16:22:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2016-11-16 16:22:32]

Patchset #4 (id:100001) has been deleted

[lunalu1, 2016-11-16 16:23:51]

Patchset #4 (id:120001) has been deleted

[lunalu1, 2016-11-16 16:24:25]

Thanks. I made the changes based on your comments. Please take a look.

https://codereview.chromium.org/2492623002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/vibrate_in_cross_origin_iframe_blocked-expected.txt
(right):

https://codereview.chromium.org/2492623002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/vibrate_in_cross_origin_iframe_blocked-expected.txt:2:
this frame.
On 2016/11/16 15:27:04, iclelland wrote:
> You're going to want to un-wrap this line -- the output from the test will all
> show up on one line, not broken into two like this.

Done.

[lunalu1, 2016-11-16 18:41:32]

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

[commit-bot: I haz the power, 2016-11-16 18:42:35]

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, 2016-11-16 19:58:19]

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

[commit-bot: I haz the power, 2016-11-16 19:58:19]

Dry run: 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, 2016-11-16 20:47:41]

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

[commit-bot: I haz the power, 2016-11-16 20:48:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2016-11-16 20:50:07]

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

[commit-bot: I haz the power, 2016-11-16 20:50:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2016-11-16 20:51:32]

lunalu@chromium.org changed reviewers:
+ binlu@google.com

[lunalu1, 2016-11-16 20:51:33]

binlu@ FYI, we are removing cross-origin vibrate with navigator.vibrate() to
feature-policy

haraken@, could you please take a look at the following files:
  third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp
  third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp

Thanks

[lunalu1, 2016-11-16 21:02:21]

Patchset #4 (id:140001) has been deleted

[commit-bot: I haz the power, 2016-11-16 22:45:24]

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

[commit-bot: I haz the power, 2016-11-16 22:45:25]

Dry run: This issue passed the CQ dry run.

[raymes, 2016-11-16 23:21:07]

raymes@chromium.org changed reviewers:
+ raymes@chromium.org

[raymes, 2016-11-16 23:21:08]

Cool and exciting! :) 

Do we want to add a test which verifies that enabling via FP makes this work?

https://codereview.chromium.org/2492623002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp (right):

https://codereview.chromium.org/2492623002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:95: }
nit: Should we rework this so that only the error message is dependent on
whether FP is enabled? I think it would reuse a little more logic. e.g.

if (!isFeatureEnabledInFrame(blink::kVibrateFeature, frame)) {
  if (RuntimeEnabledFeatures::featurePolicyEnabled())
    frame->localDOMWindow()->printErrorMessage(...);
  else
    frame->localDOMWindow()->printErrorMessage(...);
  return false
}

[haraken, 2016-11-17 06:01:24]

bindings/ and modules/ LGTM, but please wait for an approval of raymes or
iclelland.

[lunalu1, 2016-11-17 19:51:23]

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

[commit-bot: I haz the power, 2016-11-17 19:52:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2016-11-17 21:12:43]

Description was changed from

==========
Implementation for feature policy - vibrate.
Disable Navigator.vibrate() unless enabled through feature policy.

BUG=623682
==========

to

==========
Implementation for feature policy - vibrate.
Disable Navigator.vibrate() unless enabled through feature policy.

Added tests with runtime flag on

Also modified IsFeatureEnabled for the case EnableForSelf, should only be
enabled when with same origin.

BUG=623682
==========

[lunalu1, 2016-11-17 21:13:38]

Description was changed from

==========
Implementation for feature policy - vibrate.
Disable Navigator.vibrate() unless enabled through feature policy.

Added tests with runtime flag on

Also modified IsFeatureEnabled for the case EnableForSelf, should only be
enabled when with same origin.

BUG=623682
==========

to

==========
Implementation for feature policy - vibrate.
Disable Navigator.vibrate() unless enabled through feature policy.

Added tests with runtime flag is on

Also modified IsFeatureEnabled for the case EnableForSelf, should only be
enabled when with same origin.

BUG=623682
==========

[lunalu1, 2016-11-17 21:14:53]

Description was changed from

==========
Implementation for feature policy - vibrate.
Disable Navigator.vibrate() unless enabled through feature policy.

Added tests with runtime flag is on

Also modified IsFeatureEnabled for the case EnableForSelf, should only be
enabled when with same origin.

BUG=623682
==========

to

==========
Implementation for feature policy - vibrate.
Disable Navigator.vibrate() unless enabled through feature policy.

Added tests with runtime flag is on

Also modified IsFeatureEnabledInFrame for the case EnableForSelf, should only be
enabled when with same origin.

BUG=623682
==========

[commit-bot: I haz the power, 2016-11-17 21:24:07]

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

[commit-bot: I haz the power, 2016-11-17 21:24:08]

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, 2016-11-17 21:45:42]

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

[lunalu1, 2016-11-17 21:46:17]

Patchset #5 (id:180001) has been deleted

[commit-bot: I haz the power, 2016-11-17 21:47:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2016-11-17 21:47:20]

Made the changes. Please take another look. Thanks

https://codereview.chromium.org/2492623002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp (right):

https://codereview.chromium.org/2492623002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:95: }
On 2016/11/16 23:21:07, raymes wrote:
> nit: Should we rework this so that only the error message is dependent on
> whether FP is enabled? I think it would reuse a little more logic. e.g.
> 
> if (!isFeatureEnabledInFrame(blink::kVibrateFeature, frame)) {
>   if (RuntimeEnabledFeatures::featurePolicyEnabled())
>     frame->localDOMWindow()->printErrorMessage(...);
>   else
>     frame->localDOMWindow()->printErrorMessage(...);
>   return false
> }

Done.

[commit-bot: I haz the power, 2016-11-17 23:17:41]

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

[commit-bot: I haz the power, 2016-11-17 23:17:42]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[iclelland, 2016-11-18 04:46:48]

I've added some comments to the tests -- The code looks good, and I think we
have the right tests in place, just need to get them to pass.

There is no -expected.txt for any of the tests, which I think is why they're
failing. We'll need to have:
virtual/feature-policy/http/tests/feature-policy/vibrate-disabled-expected.txt
virtual/feature-policy/http/tests/feature-policy/vibrate-enabledforall-expected.txt
virtual/feature-policy/http/tests/feature-policy/vibrate-enabledforself-expected.txt

(and the same without the "virtual/feature-policy/", for the regular layout
tests, and I think you can also delete
vibrate_in_cross_origin_iframe_blocked-expected.txt from this CL)

https://codereview.chromium.org/2492623002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/resources/feature-policy-vibrate-disabled.html
(right):

https://codereview.chromium.org/2492623002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/resources/feature-policy-vibrate-disabled.html:2:
<title>Feature-Policy Vibrate Disabled</title>
<title> should go inside of the <head> element

https://codereview.chromium.org/2492623002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/resources/feature-policy-vibrate-enabledforall.html
(right):

https://codereview.chromium.org/2492623002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/resources/feature-policy-vibrate-enabledforall.html:2:
<title>Feature-Policy Vibrate Enabled For All</title>
Ditto here.

https://codereview.chromium.org/2492623002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/vibrate-disabled.php
(right):

https://codereview.chromium.org/2492623002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/vibrate-disabled.php:13:
<script src="../../resources/testharness.js"></script>
The <script> tags should be inside of <html> (and should probably be inside of
the <head>, for consistency with the other files.)

https://codereview.chromium.org/2492623002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/vibrate-enabledforall.php
(right):

https://codereview.chromium.org/2492623002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/vibrate-enabledforall.php:13:
<script src="../../resources/testharness.js"></script>
Ditto here.

https://codereview.chromium.org/2492623002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/vibrate-enabledforself.php
(right):

https://codereview.chromium.org/2492623002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/vibrate-enabledforself.php:9:
Header("Feature-Policy: {\"vibrate\": [\"*\"]}");
Was this meant to be ["self"]?

https://codereview.chromium.org/2492623002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/vibrate-enabledforself.php:16:
<iframe id="f1"
src="resources/feature-policy-vibrate-enabledforself-same-origin.html">
Can we just use the feature-policy-vibrate-enabled.html file here? Then we
wouldn't need feature-policy-vibrate-enabledforself-same-origin.html, which as
far as I can tell, has the same functionality.

https://codereview.chromium.org/2492623002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/vibrate-enabledforself.php:17:
<iframe id="f2"
src="http://localhost:8000/resources/feature-policy-vibrate-enabledforself-cross-origin.html">
Same here -- I think that we could use feature-policy-vibrate-disabled.html
instead, and have the same effect.

[raymes, 2016-11-21 03:58:04]

lgtm 

I feel like we're going to want a really similar set of tests for all the
features we add which might start to get complicated/error prone. I wonder if we
can make the tests generic to make that possible? I don't know enough about
blink tests though - I guess we would have to parameterize the test somehow. Any
thoughts on this Ian? No need to do it in this CL though :)

https://codereview.chromium.org/2492623002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp (right):

https://codereview.chromium.org/2492623002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:79:
(feature.defaultPolicy == FeaturePolicy::FeatureDefault::EnableForAll ||
nit: outer brackets not needed

[iclelland, 2016-11-21 14:37:21]

On 2016/11/21 03:58:04, raymes wrote:
> lgtm 
> 
> I feel like we're going to want a really similar set of tests for all the
> features we add which might start to get complicated/error prone. I wonder if
we
> can make the tests generic to make that possible? I don't know enough about
> blink tests though - I guess we would have to parameterize the test somehow.
Any
> thoughts on this Ian? No need to do it in this CL though :)
> 

I think the tests are simple enough that there's not really *that* much
boilerplate. The point of these tests, I think, is to ensure that each feature
is actually turned off when it should be, and that it gets disabled in the right
way. That's probably going to be different for each feature -- some have console
messages, some may throw, or just disappear -- and I think that's going to end
up being the bulk of the tests.

Happy to revisit, though, after we have a few more of these in place :)

[lunalu1, 2016-11-21 15:51:54]

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

[commit-bot: I haz the power, 2016-11-21 15:52:27]

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, 2016-11-21 16:51:58]

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

[commit-bot: I haz the power, 2016-11-21 16:51:59]

Dry run: 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, 2016-11-21 20:54:22]

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

[commit-bot: I haz the power, 2016-11-21 20:55:04]

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, 2016-11-21 22:39:58]

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

[commit-bot: I haz the power, 2016-11-21 22:39:59]

Dry run: 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_...)

[iclelland, 2016-11-22 02:45:10]

Looks like it's almost there -- the test expectation files just need some
content now, and it should be good.

https://codereview.chromium.org/2492623002/diff/240001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/feature-policy/vibrate-disabled-expected.txt
(right):

https://codereview.chromium.org/2492623002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/feature-policy/vibrate-disabled-expected.txt:8:
false got true
Looks like these two lines need to be joined.

[lunalu1, 2016-11-22 15:28:23]

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

[commit-bot: I haz the power, 2016-11-22 15:28: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, 2016-11-22 17:14:36]

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

[commit-bot: I haz the power, 2016-11-22 17:14:37]

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, 2016-11-22 18:58:14]

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

[lunalu1, 2016-11-22 18:58:25]

Patchset #7 (id:240001) has been deleted

[lunalu1, 2016-11-22 18:58:40]

Patchset #7 (id:260001) has been deleted

[commit-bot: I haz the power, 2016-11-22 18:58:57]

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, 2016-11-22 19:24:39]

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

[commit-bot: I haz the power, 2016-11-22 19:24:40]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[lunalu1, 2016-11-22 20:56:48]

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

[lunalu1, 2016-11-22 20:56:59]

Patchset #7 (id:280001) has been deleted

[commit-bot: I haz the power, 2016-11-22 20:57:25]

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, 2016-11-22 22:44:05]

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

[commit-bot: I haz the power, 2016-11-22 22:44:06]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[lunalu1, 2016-11-22 22:59:48]

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

[lunalu1, 2016-11-22 22:59:57]

Patchset #7 (id:290001) has been deleted

[commit-bot: I haz the power, 2016-11-22 23:00:19]

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, 2016-11-23 00:20:41]

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

[commit-bot: I haz the power, 2016-11-23 00:20:43]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[lunalu1, 2016-11-23 00:23:49]

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

[lunalu1, 2016-11-23 00:24:16]

Patchset #6 (id:220001) has been deleted

[lunalu1, 2016-11-23 00:24:32]

Patchset #6 (id:310001) has been deleted

[commit-bot: I haz the power, 2016-11-23 00:24:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2016-11-23 00:25:03]

Changes have been made. Please take another look :)

Thanks

https://codereview.chromium.org/2492623002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/resources/feature-policy-vibrate-disabled.html
(right):

https://codereview.chromium.org/2492623002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/resources/feature-policy-vibrate-disabled.html:2:
<title>Feature-Policy Vibrate Disabled</title>
On 2016/11/18 04:46:48, iclelland wrote:
> <title> should go inside of the <head> element

Done.

https://codereview.chromium.org/2492623002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/resources/feature-policy-vibrate-enabledforall.html
(right):

https://codereview.chromium.org/2492623002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/resources/feature-policy-vibrate-enabledforall.html:2:
<title>Feature-Policy Vibrate Enabled For All</title>
On 2016/11/18 04:46:48, iclelland wrote:
> Ditto here.

Done.

https://codereview.chromium.org/2492623002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/vibrate-disabled.php
(right):

https://codereview.chromium.org/2492623002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/vibrate-disabled.php:13:
<script src="../../resources/testharness.js"></script>
On 2016/11/18 04:46:48, iclelland wrote:
> The <script> tags should be inside of <html> (and should probably be inside of
> the <head>, for consistency with the other files.)

Done.

https://codereview.chromium.org/2492623002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/vibrate-enabledforall.php
(right):

https://codereview.chromium.org/2492623002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/vibrate-enabledforall.php:13:
<script src="../../resources/testharness.js"></script>
On 2016/11/18 04:46:48, iclelland wrote:
> Ditto here.

Done.

https://codereview.chromium.org/2492623002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/vibrate-enabledforself.php
(right):

https://codereview.chromium.org/2492623002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/vibrate-enabledforself.php:9:
Header("Feature-Policy: {\"vibrate\": [\"*\"]}");
On 2016/11/18 04:46:48, iclelland wrote:
> Was this meant to be ["self"]?

Done.

https://codereview.chromium.org/2492623002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/vibrate-enabledforself.php:16:
<iframe id="f1"
src="resources/feature-policy-vibrate-enabledforself-same-origin.html">
On 2016/11/18 04:46:48, iclelland wrote:
> Can we just use the feature-policy-vibrate-enabled.html file here? Then we
> wouldn't need feature-policy-vibrate-enabledforself-same-origin.html, which as
> far as I can tell, has the same functionality.

Done.

https://codereview.chromium.org/2492623002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/vibrate-enabledforself.php:17:
<iframe id="f2"
src="http://localhost:8000/resources/feature-policy-vibrate-enabledforself-cross-origin.html">
On 2016/11/18 04:46:48, iclelland wrote:
> Same here -- I think that we could use feature-policy-vibrate-disabled.html
> instead, and have the same effect.

Done.

[commit-bot: I haz the power, 2016-11-23 02:02:42]

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

[commit-bot: I haz the power, 2016-11-23 02:02:43]

Dry run: This issue passed the CQ dry run.

[iclelland, 2016-11-23 19:13:03]

https://codereview.chromium.org/2492623002/diff/330001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/feature-policy/vibrate-disabled.php
(right):

https://codereview.chromium.org/2492623002/diff/330001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/feature-policy/vibrate-disabled.php:24:
<iframe id="f1" src="resources/feature-policy-vibrate-disabled.html">
The HTML spec says that these tags *must* be closed; I think that's why we're
only seeing one frame in the text output. End them with </iframe>, and the
output should start containing both frames.

Nit: If we have <html> and <head> tags in this file, we might as well put a
<body> tag around the iframes as well, to be explicit. (Same in the other two
.php files)

[lunalu1, 2016-11-24 16:41:48]

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

[commit-bot: I haz the power, 2016-11-24 16:42:02]

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, 2016-11-24 18:13:51]

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

[commit-bot: I haz the power, 2016-11-24 18:13:52]

Dry run: 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, 2016-11-24 19:59:17]

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

[lunalu1, 2016-11-24 19:59:19]

Patchset #7 (id:350001) has been deleted

[commit-bot: I haz the power, 2016-11-24 19:59: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, 2016-11-24 21:08:05]

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

[commit-bot: I haz the power, 2016-11-24 21:08:07]

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

[lunalu1, 2016-11-24 22:05:32]

Please take a final look before I land. Thanks

[iclelland, 2016-11-25 01:54:12]

LGTM -- the tests look good now, thanks!

[lunalu1, 2016-11-25 02:05:38]

The CQ bit was checked by lunalu@chromium.org

[lunalu1, 2016-11-25 02:05:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/2492623002/#ps370001
(title: "Update layout test result")

[commit-bot: I haz the power, 2016-11-25 02:05:52]

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, 2016-11-25 02:09:45]

CQ is committing da patch.

Bot data: {"patchset_id": 370001, "attempt_start_ts": 1480039538156610,
"parent_rev": "ba4b5c83249850856fe7543f212f5c5b72ce20cc", "commit_rev":
"ccdb50c732d95b53a63d82a082df8965b38056fd"}

[commit-bot: I haz the power, 2016-11-25 02:10:39]

Description was changed from

==========
Implementation for feature policy - vibrate.
Disable Navigator.vibrate() unless enabled through feature policy.

Added tests with runtime flag is on

Also modified IsFeatureEnabledInFrame for the case EnableForSelf, should only be
enabled when with same origin.

BUG=623682
==========

to

==========
Implementation for feature policy - vibrate.
Disable Navigator.vibrate() unless enabled through feature policy.

Added tests with runtime flag is on

Also modified IsFeatureEnabledInFrame for the case EnableForSelf, should only be
enabled when with same origin.

BUG=623682
==========

[commit-bot: I haz the power, 2016-11-25 02:10:41]

Committed patchset #7 (id:370001)

[commit-bot: I haz the power, 2016-11-25 02:12:17]

Description was changed from

==========
Implementation for feature policy - vibrate.
Disable Navigator.vibrate() unless enabled through feature policy.

Added tests with runtime flag is on

Also modified IsFeatureEnabledInFrame for the case EnableForSelf, should only be
enabled when with same origin.

BUG=623682
==========

to

==========
Implementation for feature policy - vibrate.
Disable Navigator.vibrate() unless enabled through feature policy.

Added tests with runtime flag is on

Also modified IsFeatureEnabledInFrame for the case EnableForSelf, should only be
enabled when with same origin.

BUG=623682

Committed: https://crrev.com/11eca67f18be0dfbfcb1703a21abb454e5f85b05
Cr-Commit-Position: refs/heads/master@{#434423}
==========

[commit-bot: I haz the power, 2016-11-25 02:12:19]

Patchset 7 (id:??) landed as
https://crrev.com/11eca67f18be0dfbfcb1703a21abb454e5f85b05
Cr-Commit-Position: refs/heads/master@{#434423}

[ojan, 2017-01-19 01:34:28]

ojan@chromium.org changed reviewers:
+ haraken@chromium.org, ojan@chromium.org

[ojan, 2017-01-19 01:34:30]

https://codereview.chromium.org/2492623002/diff/370001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp (right):

https://codereview.chromium.org/2492623002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:81:
!frame->isCrossOriginSubframe()));
Just to complicate things, it looks like we'll be loosening the vibrate
restrictions to disallow vibrate on subframes until the first user gesture in
that frame. So, I think this code will need to be a bit more complicated. I
think we might need EnabledForCrossOriginFramesAfterGesture or something. We'll
want the same thing for top-level-navigation by subframes. 

Whether we want to expose that to web devs via FeaturePolicy is a different
question. I would say no to start with. We can later consider it if we see
developer demand for it.

https://codereview.chromium.org/2492623002/diff/370001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp (right):

https://codereview.chromium.org/2492623002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:85:
frame->localDOMWindow()->printErrorMessage(
Should this print statement be part of FeaturePolicy itself, e.g. if the
top-level page disables something in a subframe and then that subframe uses it,
should we log to the console?