Allow vibrate in cross-origin iframes with user gesture.

Allow vibrate in cross-origin iframes with user gesture.

This is to fix the issue of breaking some use cases of vibrate in
cross-origin iframes. Once the iframe has ever received a user gesture,
it will be allowed to call vibrate.

See more context:
https://github.com/WICG/interventions/issues/25

BUG=683938
Review-Url: https://codereview.chromium.org/2642263006
Cr-Commit-Position: refs/heads/master@{#445631}
Committed: https://chromium.googlesource.com/chromium/src/+/8f4a5209d0e6306060e901f89f345517e0df6219

[Bin Lu, 2017-01-21 08:03:16]

The CQ bit was checked by binlu@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-21 08:03:24]

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

[Bin Lu, 2017-01-21 08:03:56]

Description was changed from

==========
Allow vibrate in cross-origin iframes with user gesture.

This is to fix the issue of breaking some use cases of vibrate in
cross-origin iframes. Once the iframe has ever received a user gesture,
it will be allowed to call vibrate.

See more context:
https://github.com/WICG/interventions/issues/25

BUG=625044
==========

to

==========
Allow vibrate in cross-origin iframes with user gesture.

This is to fix the issue of breaking some use cases of vibrate in
cross-origin iframes. Once the iframe has ever received a user gesture,
it will be allowed to call vibrate.

See more context:
https://github.com/WICG/interventions/issues/25

BUG=625044
==========

[Bin Lu, 2017-01-21 08:03:56]

binlu@google.com changed reviewers:
+ ojan@chromium.org, rbyers@chromium.org

[Bin Lu, 2017-01-21 08:04:38]

Hi Rick and Ojan, Does this LG to you?

[commit-bot: I haz the power, 2017-01-21 09:21:11]

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

[commit-bot: I haz the power, 2017-01-21 09:21:12]

Dry run: This issue passed the CQ dry run.

[ojan, 2017-01-22 23:02:54]

https://codereview.chromium.org/2642263006/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp (right):

https://codereview.chromium.org/2642263006/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:96:
(!frame->isCrossOriginSubframe() || frame->hasReceivedUserGesture())));
I don't think you want this here. This is the generic FeaturePolicy
implementation, which will eventually be exposed to web authors. This check
should go in NavigatorVibration.cpp for now. If we find it's more general, we
might want a helper function later.

[Bin Lu, 2017-01-23 03:50:50]

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

[Bin Lu, 2017-01-23 03:51:40]

The CQ bit was checked by binlu@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 03:51:51]

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

[Bin Lu, 2017-01-23 03:57:17]

Aha, I thought we want this for all features, but any way I moved the change to
NavigatorVibration.cpp, so that it won't affect the generic FeaturePolicy logic.
@Ojan, is this what you have in mind?

But now there are two different places deciding whether vibrate is allowed or
not: NavigatorVibration vs FeaturePolicy, and when FeaturePolicy is turned on
(not yet), the logic would be inconsistent.
+lunalu@ as FYI: Before FeaturePolicy is turned on, please make sure to change
the FeaturePolicy logic for vibrate to make it consistent with the new user
gesture logic.

Thanks,
Bin

On 2017/01/22 23:02:54, ojan wrote:
>
https://codereview.chromium.org/2642263006/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp
(right):
> 
>
https://codereview.chromium.org/2642263006/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:96:
> (!frame->isCrossOriginSubframe() || frame->hasReceivedUserGesture())));
> I don't think you want this here. This is the generic FeaturePolicy
> implementation, which will eventually be exposed to web authors. This check
> should go in NavigatorVibration.cpp for now. If we find it's more general, we
> might want a helper function later.

[Bin Lu, 2017-01-23 05:23:24]

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

[commit-bot: I haz the power, 2017-01-23 05:39:02]

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

[commit-bot: I haz the power, 2017-01-23 05:39: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_...)

[Bin Lu, 2017-01-23 06:18:16]

The CQ bit was checked by binlu@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 06:18:31]

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-01-23 07:35:12]

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

[commit-bot: I haz the power, 2017-01-23 07:35:13]

Dry run: This issue passed the CQ dry run.

[Rick Byers, 2017-01-23 15:42:14]

I defer to iclelland@ on how best to unify the logic with Feature Policy.  But
otherwise (with a couple nits) this looks reasonable to me.

I think we should plan to merge this to the Chrome 57 branch.  Unfortunately I
believe it's too late for Chrome 56.

https://codereview.chromium.org/2642263006/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/resources/cross-origin-iframe-for-vibrate-with-user-gesture-allowed.html
(right):

https://codereview.chromium.org/2642263006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/resources/cross-origin-iframe-for-vibrate-with-user-gesture-allowed.html:24:
test(function () {
This tests that virbrate is allowed when triggered by a user gesture, but we're
implementing something weaker: any time after a user gesture has occurred in the
frame.  To cover this can you convert it into an async test and invoke vibrate
in a requestAnimationFrame callback?

To be really thorough we might want to call vibrate 3 times:
 1. before the mousedown
 2. during the 'click' event
 3. in the rAF following the click event

And verify that #1 fails but #2 and #3 succeed.  Off the top of my head it seems
like it should be easy to do that but I might be wrong.  I'll leave it up to you
- min-bar for me is that we're testing case #3.

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

https://codereview.chromium.org/2642263006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:96: "iframe
if there is no user activation on the iframe: "
nit to wording - this sounds like a warning about a pending change.  How about:
"Blocked call to navigator.vibrate inside a cross-origin iframe because the
frame has never been activated by the user".

[iclelland, 2017-01-23 16:11:38]

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

https://codereview.chromium.org/2642263006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:84: if
(RuntimeEnabledFeatures::featurePolicyEnabled() &&
This version of the logic lgtm, thanks!

I'll clean this up some once we resolve the other issues around feature policy
and default heuristics, but this looks like a good solution in the interim.

[Bin Lu, 2017-01-23 16:25:56]

Description was changed from

==========
Allow vibrate in cross-origin iframes with user gesture.

This is to fix the issue of breaking some use cases of vibrate in
cross-origin iframes. Once the iframe has ever received a user gesture,
it will be allowed to call vibrate.

See more context:
https://github.com/WICG/interventions/issues/25

BUG=625044
==========

to

==========
Allow vibrate in cross-origin iframes with user gesture.

This is to fix the issue of breaking some use cases of vibrate in
cross-origin iframes. Once the iframe has ever received a user gesture,
it will be allowed to call vibrate.

See more context:
https://github.com/WICG/interventions/issues/25

BUG=683938
==========

[lunalu1, 2017-01-23 18:04:16]

logic lgtm

[Bin Lu, 2017-01-23 20:14:14]

The CQ bit was checked by binlu@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 20:15:06]

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

[Bin Lu, 2017-01-23 20:18:19]

Thanks all for the review.

@Rick, modified to test vibrate 3 times in a x-origin iframe:  
1. blocked before user gesture; 
2. succeed during the click event; 
3. succeed after user click.

Thanks,
Bin

On 2017/01/23 18:04:16, loonybear wrote:
> logic lgtm

[Bin Lu, 2017-01-23 20:18:40]

https://codereview.chromium.org/2642263006/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/resources/cross-origin-iframe-for-vibrate-with-user-gesture-allowed.html
(right):

https://codereview.chromium.org/2642263006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/resources/cross-origin-iframe-for-vibrate-with-user-gesture-allowed.html:24:
test(function () {
On 2017/01/23 15:42:14, Rick Byers wrote:
> This tests that virbrate is allowed when triggered by a user gesture, but
we're
> implementing something weaker: any time after a user gesture has occurred in
the
> frame.  To cover this can you convert it into an async test and invoke vibrate
> in a requestAnimationFrame callback?
> 
> To be really thorough we might want to call vibrate 3 times:
>  1. before the mousedown
>  2. during the 'click' event
>  3. in the rAF following the click event
> 
> And verify that #1 fails but #2 and #3 succeed.  Off the top of my head it
seems
> like it should be easy to do that but I might be wrong.  I'll leave it up to
you
> - min-bar for me is that we're testing case #3.

Done.

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

https://codereview.chromium.org/2642263006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:84: if
(RuntimeEnabledFeatures::featurePolicyEnabled() &&
On 2017/01/23 16:11:38, iclelland wrote:
> This version of the logic lgtm, thanks!
> 
> I'll clean this up some once we resolve the other issues around feature policy
> and default heuristics, but this looks like a good solution in the interim.

Acknowledged.

https://codereview.chromium.org/2642263006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:96: "iframe
if there is no user activation on the iframe: "
On 2017/01/23 15:42:14, Rick Byers wrote:
> nit to wording - this sounds like a warning about a pending change.  How
about:
> "Blocked call to navigator.vibrate inside a cross-origin iframe because the
> frame has never been activated by the user".

Done.

[Rick Byers, 2017-01-23 20:23:27]

LGTM

[Rick Byers, 2017-01-23 20:23:27]

LGTM

[commit-bot: I haz the power, 2017-01-23 20:24:48]

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

[commit-bot: I haz the power, 2017-01-23 20:24:49]

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

[Bin Lu, 2017-01-23 20:32:01]

The CQ bit was checked by binlu@google.com

[Bin Lu, 2017-01-23 20:32:01]

The patchset sent to the CQ was uploaded after l-g-t-m from lunalu@chromium.org,
iclelland@chromium.org
Link to the patchset: https://codereview.chromium.org/2642263006/#ps60001
(title: "git cl try")

[commit-bot: I haz the power, 2017-01-23 20:33:08]

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-01-23 23:01:44]

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

[commit-bot: I haz the power, 2017-01-23 23:01:45]

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

[Bin Lu, 2017-01-23 23:52:38]

The CQ bit was checked by binlu@google.com

[commit-bot: I haz the power, 2017-01-23 23:53:19]

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-01-24 03:38:41]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485215558948770,
"parent_rev": "4c6b3b0f11b62e069d9d84ba99dbe94a45a5c622", "commit_rev":
"8f4a5209d0e6306060e901f89f345517e0df6219"}

[commit-bot: I haz the power, 2017-01-24 03:39:17]

Description was changed from

==========
Allow vibrate in cross-origin iframes with user gesture.

This is to fix the issue of breaking some use cases of vibrate in
cross-origin iframes. Once the iframe has ever received a user gesture,
it will be allowed to call vibrate.

See more context:
https://github.com/WICG/interventions/issues/25

BUG=683938
==========

to

==========
Allow vibrate in cross-origin iframes with user gesture.

This is to fix the issue of breaking some use cases of vibrate in
cross-origin iframes. Once the iframe has ever received a user gesture,
it will be allowed to call vibrate.

See more context:
https://github.com/WICG/interventions/issues/25

BUG=683938

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

[commit-bot: I haz the power, 2017-01-24 03:39:18]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/8f4a5209d0e6306060e901f89f34...