Remove navigator.vibrate without user gesture.

Remove navigator.vibrate without user gesture.

Calls to navigator.vibrate will immediately return 'false' if user
hasn't tapped on the frame or any embedded frame yet.

The implementation is behind an experimental flag.

The Intent to Deprecate thread:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/QGcpjisxtik

BUG=704650
Review-Url: https://codereview.chromium.org/2778693004
Cr-Commit-Position: refs/heads/master@{#465618}
Committed: https://chromium.googlesource.com/chromium/src/+/c5b59119ca57398e87a692c9ea836efee2627966

[Bin Lu, 2017-03-27 16:38:27]

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

[commit-bot: I haz the power, 2017-03-27 16:38:57]

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-03-27 16:39:10]

Description was changed from

==========
Remove navigator.vibrate without user gesture.

Calls to navigator.vibrate will immediately return 'false' if user
hasn't tapped on the frame or any embedded frame yet.

BUG=704650
==========

to

==========
Remove navigator.vibrate without user gesture.

Calls to navigator.vibrate will immediately return 'false' if user
hasn't tapped on the frame or any embedded frame yet.

BUG=704650
==========

[Bin Lu, 2017-03-27 16:39:10]

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

[Bin Lu, 2017-03-27 16:40:28]

Does this look good? If yes, I will send out the "intent to implement and
remove" email to blink-dev. Thanks.

[commit-bot: I haz the power, 2017-03-27 16:41:50]

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

[commit-bot: I haz the power, 2017-03-27 16:41:51]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Bin Lu, 2017-03-27 16:45:10]

Description was changed from

==========
Remove navigator.vibrate without user gesture.

Calls to navigator.vibrate will immediately return 'false' if user
hasn't tapped on the frame or any embedded frame yet.

BUG=704650
==========

to

==========
Remove navigator.vibrate without user gesture.

Calls to navigator.vibrate will immediately return 'false' if user
hasn't tapped on the frame or any embedded frame yet.

Also changed to use hasReceivedUserGesture (from processingUserGesture) for
metrics collecting, to be consistent with the implementation & better measure
the metrics on vibrate blocking.

BUG=704650
==========

[Bin Lu, 2017-03-28 20:13:11]

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

[commit-bot: I haz the power, 2017-03-28 20:13:43]

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-03-28 20:16:49]

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

[commit-bot: I haz the power, 2017-03-28 20:16:50]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Bin Lu, 2017-03-28 21:05:34]

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

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

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-03-28 21:06:40]

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

[commit-bot: I haz the power, 2017-03-28 21:06:41]

Dry run: CQ has no permission to schedule in bucket
master.tryserver.chromium.linux

[Bin Lu, 2017-03-28 21:12:15]

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

[commit-bot: I haz the power, 2017-03-28 21:13:00]

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-03-28 21:13:26]

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

[commit-bot: I haz the power, 2017-03-28 21:13:27]

Dry run: CQ has no permission to schedule in bucket
master.tryserver.chromium.linux

[Bin Lu, 2017-03-28 21:49:11]

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

[commit-bot: I haz the power, 2017-03-28 21:49:32]

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

[binlu, 2017-03-28 21:50:24]

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

[commit-bot: I haz the power, 2017-03-28 21:50:30]

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-03-28 23:23:16]

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

[commit-bot: I haz the power, 2017-03-28 23:23:17]

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

[binlu, 2017-03-29 15:23:52]

Description was changed from

==========
Remove navigator.vibrate without user gesture.

Calls to navigator.vibrate will immediately return 'false' if user
hasn't tapped on the frame or any embedded frame yet.

Also changed to use hasReceivedUserGesture (from processingUserGesture) for
metrics collecting, to be consistent with the implementation & better measure
the metrics on vibrate blocking.

BUG=704650
==========

to

==========
Remove navigator.vibrate without user gesture.

Calls to navigator.vibrate will immediately return 'false' if user
hasn't tapped on the frame or any embedded frame yet.

Also changed to use hasReceivedUserGesture (from processingUserGesture) for
metrics collecting, to be consistent with the implementation & better measure
the metrics on vibrate blocking.

The Intent to Deprecate thread:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/QGcpjisxtik

BUG=704650
==========

[Bin Lu, 2017-03-29 16:45:12]

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

[commit-bot: I haz the power, 2017-03-29 16:45:46]

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

[Rick Byers, 2017-03-29 18:01:40]

Looks good overall.

You should probably add some explicit tests for this behavior though.  Eg.
verify that tapping inside a frame allows the top level to call vibrate
successfully without warning.

https://codereview.chromium.org/2778693004/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/vibration/vibration-durations-expected.txt
(right):

https://codereview.chromium.org/2778693004/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vibration/vibration-durations-expected.txt:1:
CONSOLE WARNING: line 1: A call to navigator.vibrate will soon require user tap
on the frame or any embedded
frame:https://www.chromestatus.com/feature/5644273861001216.
I think you want to change the test to simulate a user gesture, since that's
really the codepath the tests intend to validate.  Then we can avoid adding all
these warnings to the expected output.

Ideally you'd have at least one test which intentionally doesn't have a gesture
to test the error message and eventual failure.

Note that often we'll de-dupe warnings to prevent a frequently called API from
generating an infinite number.  The risk here seems low with vibrate so it's
probably better as-is, but thought it was worth mentioning.

https://codereview.chromium.org/2778693004/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vibration/vibration-durations-expected.txt:2:
CONSOLE WARNING: line 17: A call to navigator.vibrate will soon require user tap
on the frame or any embedded
frame:https://www.chromestatus.com/feature/5644273861001216.
Why are we getting this version of the warning when status=experimental features
are enabled by default for LayoutTests (outside virtual/stable)?  Shouldn't we
be getting the "has been blocked" warning?

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

https://codereview.chromium.org/2778693004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:85:
frame->domWindow()->printErrorMessage(
Can you please have all these messages use InterventionMessageSource?  It's
somewhat new so not used everywhere yet, but we're trying to get all these to be
consistently exposed in the Lighthouse auditing tool.  We'll have to either add
a source arg to printErrorMessage or (perhaps better) just use a single
addMessage call here with some variables for the target and message string.

[commit-bot: I haz the power, 2017-03-29 18:39:04]

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

[commit-bot: I haz the power, 2017-03-29 18:39:05]

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-04-06 17:21:12]

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

[Bin Lu, 2017-04-06 17:27:18]

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

[commit-bot: I haz the power, 2017-04-06 17:28: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, 2017-04-06 18:40:31]

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

[Bin Lu, 2017-04-06 18:40:32]

Description was changed from

==========
Remove navigator.vibrate without user gesture.

Calls to navigator.vibrate will immediately return 'false' if user
hasn't tapped on the frame or any embedded frame yet.

Also changed to use hasReceivedUserGesture (from processingUserGesture) for
metrics collecting, to be consistent with the implementation & better measure
the metrics on vibrate blocking.

The Intent to Deprecate thread:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/QGcpjisxtik

BUG=704650
==========

to

==========
Remove navigator.vibrate without user gesture.

Calls to navigator.vibrate will immediately return 'false' if user
hasn't tapped on the frame or any embedded frame yet.

The implementation is behind an experimental flag.

The Intent to Deprecate thread:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/QGcpjisxtik

BUG=704650
==========

[commit-bot: I haz the power, 2017-04-06 18:40:32]

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

[jochen (gone - plz use gerrit), 2017-04-07 06:58:21]

you will also need a member of the metrics team to review the histograms.xml
change

[binlu, 2017-04-07 12:04:14]

binlu@chromium.org changed reviewers:
+ jwd@chromium.org

[binlu, 2017-04-07 12:06:03]

Added jwd@ from the metrics team.
Thanks jochen@ for the reminder.

On 2017/04/07 06:58:21, jochen wrote:
> you will also need a member of the metrics team to review the histograms.xml
> change

[jwd, 2017-04-10 21:43:20]

Metrics LGTM

[Bin Lu, 2017-04-11 20:47:07]

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

[commit-bot: I haz the power, 2017-04-11 20:47:38]

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-04-11 20:51:23]

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

[commit-bot: I haz the power, 2017-04-11 20:51:24]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Bin Lu, 2017-04-11 21:47:06]

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

[commit-bot: I haz the power, 2017-04-11 21:47:48]

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-04-11 21:53:01]

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

[commit-bot: I haz the power, 2017-04-11 21:53:44]

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

[binlu, 2017-04-11 21:54:25]

I've refactored the tests. PTAL.

Also I'm still not sure if
third_party/WebKit/LayoutTests/vibration/vibration-iframe.html is working
correctly. Any idea?

https://codereview.chromium.org/2778693004/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/vibration/vibration-durations-expected.txt
(right):

https://codereview.chromium.org/2778693004/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vibration/vibration-durations-expected.txt:1:
CONSOLE WARNING: line 1: A call to navigator.vibrate will soon require user tap
on the frame or any embedded
frame:https://www.chromestatus.com/feature/5644273861001216.

Done:
(1) Made changes to the tests to simulate user click via adding a
user-gesture-utils.js file in  third_party/WebKit/LayoutTests/resources.
(2) Added another test to test the cases of before/during/after user click on
the top page:
http/tests/security/vibration/vibrate-on-top-page-before-during-after-user-gesture.html

Any pointer on how to de-dupe warnings (even if we don't need it for this
change)?


On 2017/03/29 18:01:40, Rick Byers wrote:
> I think you want to change the test to simulate a user gesture, since that's
> really the codepath the tests intend to validate.  Then we can avoid adding
all
> these warnings to the expected output.
> 
> Ideally you'd have at least one test which intentionally doesn't have a
gesture
> to test the error message and eventual failure.
> 
> Note that often we'll de-dupe warnings to prevent a frequently called API from
> generating an infinite number.  The risk here seems low with vibrate so it's
> probably better as-is, but thought it was worth mentioning.
>

https://codereview.chromium.org/2778693004/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vibration/vibration-durations-expected.txt:2:
CONSOLE WARNING: line 17: A call to navigator.vibrate will soon require user tap
on the frame or any embedded
frame:https://www.chromestatus.com/feature/5644273861001216.
Is my understanding right that it says "content_shell will not enable
experimental/test features by default ..." according to
https://www.chromium.org/blink/runtime-enabled-features.

Anyway, I changed the experiment feature to FEATURE_ENABLED_BY_DEFAULT (from
XXX_DISABLED_BY_DEFAULT), and so we see the "has been blocked" warning.
Please let me know if I'm doing it wrong. Thanks.


On 2017/03/29 18:01:40, Rick Byers wrote:
> Why are we getting this version of the warning when status=experimental
features
> are enabled by default for LayoutTests (outside virtual/stable)?  Shouldn't we
> be getting the "has been blocked" warning?

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

https://codereview.chromium.org/2778693004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:85:
frame->domWindow()->printErrorMessage(
On 2017/03/29 18:01:40, Rick Byers wrote:
> Can you please have all these messages use InterventionMessageSource?  It's
> somewhat new so not used everywhere yet, but we're trying to get all these to
be
> consistently exposed in the Lighthouse auditing tool.  We'll have to either
add
> a source arg to printErrorMessage or (perhaps better) just use a single
> addMessage call here with some variables for the target and message string.

Done.

[commit-bot: I haz the power, 2017-04-11 23:57:30]

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

[commit-bot: I haz the power, 2017-04-11 23:57:31]

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

[binlu, 2017-04-12 14:28:53]

binlu@chromium.org changed reviewers:
+ mustaq@chromium.org

[binlu, 2017-04-12 14:45:27]

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

[lunalu1, 2017-04-12 14:47:47]

The layout tests for feature policy vibrate are failing is because now vibrate
is behind a different flag (FeaturePolicyExperimetal instead of FeaturePolicy).
I will move the tests to the correct directory.
Sorry about that!

[lunalu1, 2017-04-12 17:35:50]

Once https://codereview.chromium.org/2814153002 is landed, all feature policy
vibrate tests should be passing.

[mustaq, 2017-04-13 14:28:34]

Few comments while you wait for the feature-policy CL to land...

https://codereview.chromium.org/2778693004/diff/220001/chrome/browser/flag_de...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2778693004/diff/220001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.cc:2494: "Blocking the Vibration API if there
has no user gesture received on "
Nit: s/Blocking/Block/

https://codereview.chromium.org/2778693004/diff/220001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/resources/user-gesture-utils.js (right):

https://codereview.chromium.org/2778693004/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/resources/user-gesture-utils.js:6: function
simulateUserClick(elementID, event) {
The role of |event| parameter is unclear. Since this is unused for now, let's
remove it.

https://codereview.chromium.org/2778693004/diff/220001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/vibration/resources/vibrate-from-iframe.html
(right):

https://codereview.chromium.org/2778693004/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/vibration/resources/vibrate-from-iframe.html:8:
/*function startTest(event) {
Used code.

[Bin Lu, 2017-04-13 15:02:32]

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

[commit-bot: I haz the power, 2017-04-13 15:03:09]

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-04-13 15:04:46]

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

[commit-bot: I haz the power, 2017-04-13 15:05:16]

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

[binlu, 2017-04-13 15:11:11]

Thanks for the reviews. Fixed the issues. PTAL.

I'm hoping that this CL could catch up with the Branch date for M59 which is
today.

https://codereview.chromium.org/2778693004/diff/220001/chrome/browser/flag_de...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2778693004/diff/220001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.cc:2494: "Blocking the Vibration API if there
has no user gesture received on "
On 2017/04/13 14:28:34, mustaq wrote:
> Nit: s/Blocking/Block/

Done.

https://codereview.chromium.org/2778693004/diff/220001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/resources/user-gesture-utils.js (right):

https://codereview.chromium.org/2778693004/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/resources/user-gesture-utils.js:6: function
simulateUserClick(elementID, event) {
On 2017/04/13 14:28:34, mustaq wrote:
> The role of |event| parameter is unclear. Since this is unused for now, let's
> remove it.

It's actually used in some test files, e.g.,
https://codereview.chromium.org/2778693004/diff/260001/third_party/WebKit/Lay...
https://codereview.chromium.org/2778693004/diff/260001/third_party/WebKit/Lay...

https://codereview.chromium.org/2778693004/diff/220001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/vibration/resources/vibrate-from-iframe.html
(right):

https://codereview.chromium.org/2778693004/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/vibration/resources/vibrate-from-iframe.html:8:
/*function startTest(event) {
On 2017/04/13 14:28:34, mustaq wrote:
> Used code.

Done.

[mustaq, 2017-04-13 15:58:37]

https://codereview.chromium.org/2778693004/diff/220001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/resources/user-gesture-utils.js (right):

https://codereview.chromium.org/2778693004/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/resources/user-gesture-utils.js:6: function
simulateUserClick(elementID, event) {
On 2017/04/13 15:11:11, binlu wrote:
> On 2017/04/13 14:28:34, mustaq wrote:
> > The role of |event| parameter is unclear. Since this is unused for now,
let's
> > remove it.
> 
> It's actually used in some test files, e.g.,
>
https://codereview.chromium.org/2778693004/diff/260001/third_party/WebKit/Lay...
>
https://codereview.chromium.org/2778693004/diff/260001/third_party/WebKit/Lay...

I see, you have passed the missing frame coordinates now, thanks.

But since simulateUserClick() is a repeatedly used utility function, I would
like to simplify the interface to:
  simulateUserClick(x,y).
For the subframe case, the caller would use the frame coordinates appropriately.

[binlu, 2017-04-13 17:05:44]

https://codereview.chromium.org/2778693004/diff/220001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/resources/user-gesture-utils.js (right):

https://codereview.chromium.org/2778693004/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/resources/user-gesture-utils.js:6: function
simulateUserClick(elementID, event) {
On 2017/04/13 15:58:37, mustaq wrote:
> On 2017/04/13 15:11:11, binlu wrote:
> > On 2017/04/13 14:28:34, mustaq wrote:
> > > The role of |event| parameter is unclear. Since this is unused for now,
> let's
> > > remove it.
> > 
> > It's actually used in some test files, e.g.,
> >
>
https://codereview.chromium.org/2778693004/diff/260001/third_party/WebKit/Lay...
> >
>
https://codereview.chromium.org/2778693004/diff/260001/third_party/WebKit/Lay...
> 
> I see, you have passed the missing frame coordinates now, thanks.
> 
> But since simulateUserClick() is a repeatedly used utility function, I would
> like to simplify the interface to:
>   simulateUserClick(x,y).
> For the subframe case, the caller would use the frame coordinates
appropriately.

Good point. Done.

[mustaq, 2017-04-13 19:05:04]

Have a few more comments on tests & a clarification question otherwise looks
fine.

https://codereview.chromium.org/2778693004/diff/280001/chrome/browser/flag_de...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2778693004/diff/280001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.cc:2494: "Block the Vibration API if there has
no user gesture received on "
Nit: "... if no user gesture is received on ..."

https://codereview.chromium.org/2778693004/diff/280001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/vibration/resources/vibrate-in-iframe-with-user-gesture-allowed.html
(right):

https://codereview.chromium.org/2778693004/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/vibration/resources/vibrate-in-iframe-with-user-gesture-allowed.html:6:
var test = async_test(
The async_test label seems redundant with the assert labels. I suggest nuking
the "var test = async_test..." line, and change each
  test.step(f(assert(condition,label))
into
  test(f(assert(condition),label)
which makes the test outcome a bit more granular. See
mouse-pointer-chorded-buttons.html as an example.

https://codereview.chromium.org/2778693004/diff/280001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/vibration/resources/vibrate-with-user-gesture-allowed.html
(right):

https://codereview.chromium.org/2778693004/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/vibration/resources/vibrate-with-user-gesture-allowed.html:15:
eventSender.mouseUp();
Nuke eventSender.{mouseMoveTo,mosueDown,mouseUp} calls.

https://codereview.chromium.org/2778693004/diff/280001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/vibration/vibrate-on-top-page-before-during-after-user-gesture.html
(right):

https://codereview.chromium.org/2778693004/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/vibration/vibrate-on-top-page-before-during-after-user-gesture.html:3:
<script src="/resources/testharness.js"></script>
Can you please add a <title> to describe the test here, as in the other tests?

https://codereview.chromium.org/2778693004/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/vibration/vibrate-on-top-page-before-during-after-user-gesture.html:7:
var test = async_test(
Ditto about async_test.

https://codereview.chromium.org/2778693004/diff/280001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/vibration/resources/vibrate-from-iframe.html
(right):

https://codereview.chromium.org/2778693004/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/vibration/resources/vibrate-from-iframe.html:2:
<script src="../../resources/testharness.js"></script>
Add a <title>?

https://codereview.chromium.org/2778693004/diff/280001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/vibration/vibration-patterns.html (right):

https://codereview.chromium.org/2778693004/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/vibration/vibration-patterns.html:13: // See:
https://www.chromestatus.com/feature/5644273861001216.
The chromestatus links here seem redundant because the same link is there in the
main code as well as in console messages. Please remove all such links from
*html comments*.

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

https://codereview.chromium.org/2778693004/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:94: if
(!frame->HasReceivedUserGesture()) {
Clarification question: why you don't want this to depend on
featurePolicyEnabled() now?

[Bin Lu, 2017-04-13 19:50:18]

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

[commit-bot: I haz the power, 2017-04-13 19:50:47]

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

[binlu, 2017-04-13 19:55:27]

Thanks for the review. PTAL.

https://codereview.chromium.org/2778693004/diff/280001/chrome/browser/flag_de...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2778693004/diff/280001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.cc:2494: "Block the Vibration API if there has
no user gesture received on "
On 2017/04/13 19:05:04, mustaq wrote:
> Nit: "... if no user gesture is received on ..."

Done.

https://codereview.chromium.org/2778693004/diff/280001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/vibration/resources/vibrate-in-iframe-with-user-gesture-allowed.html
(right):

https://codereview.chromium.org/2778693004/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/vibration/resources/vibrate-in-iframe-with-user-gesture-allowed.html:6:
var test = async_test(
On 2017/04/13 19:05:04, mustaq wrote:
> The async_test label seems redundant with the assert labels. I suggest nuking
> the "var test = async_test..." line, and change each
>   test.step(f(assert(condition,label))
> into
>   test(f(assert(condition),label)
> which makes the test outcome a bit more granular. See
> mouse-pointer-chorded-buttons.html as an example.

Done.

https://codereview.chromium.org/2778693004/diff/280001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/vibration/resources/vibrate-with-user-gesture-allowed.html
(right):

https://codereview.chromium.org/2778693004/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/vibration/resources/vibrate-with-user-gesture-allowed.html:15:
eventSender.mouseUp();
On 2017/04/13 19:05:04, mustaq wrote:
> Nuke eventSender.{mouseMoveTo,mosueDown,mouseUp} calls.

Done.

https://codereview.chromium.org/2778693004/diff/280001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/vibration/vibrate-on-top-page-before-during-after-user-gesture.html
(right):

https://codereview.chromium.org/2778693004/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/vibration/vibrate-on-top-page-before-during-after-user-gesture.html:3:
<script src="/resources/testharness.js"></script>
On 2017/04/13 19:05:04, mustaq wrote:
> Can you please add a <title> to describe the test here, as in the other tests?

Done.

https://codereview.chromium.org/2778693004/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/vibration/vibrate-on-top-page-before-during-after-user-gesture.html:7:
var test = async_test(
On 2017/04/13 19:05:04, mustaq wrote:
> Ditto about async_test.

Done.

https://codereview.chromium.org/2778693004/diff/280001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/vibration/vibration-patterns.html (right):

https://codereview.chromium.org/2778693004/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/vibration/vibration-patterns.html:13: // See:
https://www.chromestatus.com/feature/5644273861001216.
On 2017/04/13 19:05:04, mustaq wrote:
> The chromestatus links here seem redundant because the same link is there in
the
> main code as well as in console messages. Please remove all such links from
> *html comments*.

Will remove them shortly.

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

https://codereview.chromium.org/2778693004/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:94: if
(!frame->HasReceivedUserGesture()) {
On 2017/04/13 19:05:04, mustaq wrote:
> Clarification question: why you don't want this to depend on
> featurePolicyEnabled() now?

FeaturePolicy is being turned on by default for other features (but not
vibrate), and also the check will run before the user gesture check.

[commit-bot: I haz the power, 2017-04-13 19:56:06]

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

[commit-bot: I haz the power, 2017-04-13 19:56:07]

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

[Bin Lu, 2017-04-13 20:12:18]

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

[commit-bot: I haz the power, 2017-04-13 20:12:48]

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-04-13 20:17:30]

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

[commit-bot: I haz the power, 2017-04-13 20:17:31]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  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_...)

[Bin Lu, 2017-04-13 20:27:09]

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

[commit-bot: I haz the power, 2017-04-13 20:27:36]

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-04-13 20:30:55]

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

[commit-bot: I haz the power, 2017-04-13 20:31:25]

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-04-13 20:33:51]

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

[commit-bot: I haz the power, 2017-04-13 20:34:25]

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-04-13 22:47:11]

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

[commit-bot: I haz the power, 2017-04-13 22:47:42]

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-04-14 00:39:25]

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

[commit-bot: I haz the power, 2017-04-14 00:39:27]

Dry run: This issue passed the CQ dry run.

[binlu, 2017-04-14 00:43:56]

The CQ bit was checked by binlu@chromium.org

[binlu, 2017-04-14 00:43:56]

The patchset sent to the CQ was uploaded after l-g-t-m from jwd@chromium.org
Link to the patchset: https://codereview.chromium.org/2778693004/#ps380001
(title: "skip mojo test for vibrate with feature policy")

[commit-bot: I haz the power, 2017-04-14 00:44:31]

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

[ojan, 2017-04-14 00:52:43]

https://codereview.chromium.org/2778693004/diff/380001/content/public/common/...
File content/public/common/content_features.cc (right):

https://codereview.chromium.org/2778693004/diff/380001/content/public/common/...
content/public/common/content_features.cc:231: "VibrateRequiresUserGesture",
base::FEATURE_ENABLED_BY_DEFAULT};
This isn't actually hooked up to the RuntimeEnabledFeature here I think. See
WebRuntimeFeatures.cpp and runtime_features.cc for examples of how to do the
plumbing.

[commit-bot: I haz the power, 2017-04-14 00:59:22]

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

[commit-bot: I haz the power, 2017-04-14 00:59:24]

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

[Rick Byers, 2017-04-14 01:09:13]

This LGTM except for the problem Ojan pointed out where the about:flags page
isn't actually hooked up.  Personally I feel that's overkill (there's already
the "enable experimental web platform features" flag in about:flags someone can
use to turn this on along with all other status=experimental things).  So I'd
suggest just reverting all the changes outside WebKit (in which case my feedback
is a "nit" and you're free to land).

Or if you prefer to have the flags entry, then you'll need to wire it up and
double check with Ojan or I (or another OWNER of the code).

https://codereview.chromium.org/2778693004/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/vibration/vibration-durations-expected.txt
(right):

https://codereview.chromium.org/2778693004/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vibration/vibration-durations-expected.txt:1:
CONSOLE WARNING: line 1: A call to navigator.vibrate will soon require user tap
on the frame or any embedded
frame:https://www.chromestatus.com/feature/5644273861001216.
On 2017/04/11 21:54:25, binlu wrote:
> 
> Done:
> (1) Made changes to the tests to simulate user click via adding a
> user-gesture-utils.js file in  third_party/WebKit/LayoutTests/resources.
> (2) Added another test to test the cases of before/during/after user click on
> the top page:
>
http/tests/security/vibration/vibrate-on-top-page-before-during-after-user-gesture.html
> 
> Any pointer on how to de-dupe warnings (even if we don't need it for this
> change)?

You could use the machinery in Deprecation.cpp (which relies on UseCounter.cpp
to keep track of the usage seen per Page since the last page load).

> 
> 
> On 2017/03/29 18:01:40, Rick Byers wrote:
> > I think you want to change the test to simulate a user gesture, since that's
> > really the codepath the tests intend to validate.  Then we can avoid adding
> all
> > these warnings to the expected output.
> > 
> > Ideally you'd have at least one test which intentionally doesn't have a
> gesture
> > to test the error message and eventual failure.
> > 
> > Note that often we'll de-dupe warnings to prevent a frequently called API
from
> > generating an infinite number.  The risk here seems low with vibrate so it's
> > probably better as-is, but thought it was worth mentioning.
> > 
>

https://codereview.chromium.org/2778693004/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/vibration/vibration-durations-expected.txt:2:
CONSOLE WARNING: line 17: A call to navigator.vibrate will soon require user tap
on the frame or any embedded
frame:https://www.chromestatus.com/feature/5644273861001216.
On 2017/04/11 21:54:25, binlu wrote:
> Is my understanding right that it says "content_shell will not enable
> experimental/test features by default ..." according to
> https://www.chromium.org/blink/runtime-enabled-features.

That page is confusing, sorry.  Running layout tests DOES run with the
--dump-render-tree flag (although it's actually called --run-layout-test now). 
So LayoutTests have status=test (and experimental) enabled by default except
inside the virtual/stable virtual test suite.

> Anyway, I changed the experiment feature to FEATURE_ENABLED_BY_DEFAULT (from
> XXX_DISABLED_BY_DEFAULT), and so we see the "has been blocked" warning.
> Please let me know if I'm doing it wrong. Thanks.

Have you verified that running your Chrome build manually you don't now get the
experimental behavior enabled?  You don't want to accidentally ship this just
yet ;-).  The tests should be testing what we expect to ship in M60 and you
should manually verify that (other than warnings) things behave as before in
M59.  If necessary we can add tests to the 'stable' testsuite to also test the
M59 behavior but I don't think it's worth worrying about.

> On 2017/03/29 18:01:40, Rick Byers wrote:
> > Why are we getting this version of the warning when status=experimental
> features
> > are enabled by default for LayoutTests (outside virtual/stable)?  Shouldn't
we
> > be getting the "has been blocked" warning?
>

[Bin Lu, 2017-04-14 01:11:37]

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

[commit-bot: I haz the power, 2017-04-14 01:12:19]

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

[binlu, 2017-04-14 01:16:35]

https://codereview.chromium.org/2778693004/diff/380001/content/public/common/...
File content/public/common/content_features.cc (right):

https://codereview.chromium.org/2778693004/diff/380001/content/public/common/...
content/public/common/content_features.cc:231: "VibrateRequiresUserGesture",
base::FEATURE_ENABLED_BY_DEFAULT};
On 2017/04/14 00:52:43, ojan wrote:
> This isn't actually hooked up to the RuntimeEnabledFeature here I think. See
> WebRuntimeFeatures.cpp and runtime_features.cc for examples of how to do the
> plumbing.

Done. It's weird that the change was there before but dropped somehow during
various rebases:
https://codereview.chromium.org/2778693004/diff/60001/content/child/runtime_f...
just added it back: 
https://codereview.chromium.org/2778693004/diff/400001/content/child/runtime_...

[commit-bot: I haz the power, 2017-04-14 01:25:30]

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

[commit-bot: I haz the power, 2017-04-14 01:25:32]

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_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Bin Lu, 2017-04-14 01:28:16]

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

[commit-bot: I haz the power, 2017-04-14 01:28:50]

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-04-14 03:13:09]

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

[commit-bot: I haz the power, 2017-04-14 03:13:10]

Dry run: This issue passed the CQ dry run.

[binlu, 2017-04-14 04:38:21]

Thanks for the reviews!

On 2017/04/14 01:09:13, Rick Byers wrote:
> This LGTM except for the problem Ojan pointed out where the about:flags page
> isn't actually hooked up.  Personally I feel that's overkill (there's already
> the "enable experimental web platform features" flag in about:flags someone
can
> use to turn this on along with all other status=experimental things).  So I'd
> suggest just reverting all the changes outside WebKit (in which case my
feedback
> is a "nit" and you're free to land).
> 
> Or if you prefer to have the flags entry, then you'll need to wire it up and
> double check with Ojan or I (or another OWNER of the code).
I prefer to have the flags entry, and have wired it up:
https://codereview.chromium.org/2778693004/diff/420001/content/child/runtime_...
Offline chatted with Rick, and submitting the CL now to catch up with the branch
of M59.
If the submission doesn't, will try to merge it into M59 early next week.

> 
>
https://codereview.chromium.org/2778693004/diff/80001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/vibration/vibration-durations-expected.txt
> (right):
> 
>
https://codereview.chromium.org/2778693004/diff/80001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/vibration/vibration-durations-expected.txt:1:
> CONSOLE WARNING: line 1: A call to navigator.vibrate will soon require user
tap
> on the frame or any embedded
> frame:https://www.chromestatus.com/feature/5644273861001216.
> On 2017/04/11 21:54:25, binlu wrote:
> > 
> > Done:
> > (1) Made changes to the tests to simulate user click via adding a
> > user-gesture-utils.js file in  third_party/WebKit/LayoutTests/resources.
> > (2) Added another test to test the cases of before/during/after user click
on
> > the top page:
> >
>
http/tests/security/vibration/vibrate-on-top-page-before-during-after-user-gesture.html
> > 
> > Any pointer on how to de-dupe warnings (even if we don't need it for this
> > change)?
> 
> You could use the machinery in Deprecation.cpp (which relies on UseCounter.cpp
> to keep track of the usage seen per Page since the last page load).
Got it. Thanks for the pointer!

> 
> > 
> > 
> > On 2017/03/29 18:01:40, Rick Byers wrote:
> > > I think you want to change the test to simulate a user gesture, since
that's
> > > really the codepath the tests intend to validate.  Then we can avoid
adding
> > all
> > > these warnings to the expected output.
> > > 
> > > Ideally you'd have at least one test which intentionally doesn't have a
> > gesture
> > > to test the error message and eventual failure.
> > > 
> > > Note that often we'll de-dupe warnings to prevent a frequently called API
> from
> > > generating an infinite number.  The risk here seems low with vibrate so
it's
> > > probably better as-is, but thought it was worth mentioning.
> > > 
> >
> 
>
https://codereview.chromium.org/2778693004/diff/80001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/vibration/vibration-durations-expected.txt:2:
> CONSOLE WARNING: line 17: A call to navigator.vibrate will soon require user
tap
> on the frame or any embedded
> frame:https://www.chromestatus.com/feature/5644273861001216.
> On 2017/04/11 21:54:25, binlu wrote:
> > Is my understanding right that it says "content_shell will not enable
> > experimental/test features by default ..." according to
> > https://www.chromium.org/blink/runtime-enabled-features.
> 
> That page is confusing, sorry.  Running layout tests DOES run with the
> --dump-render-tree flag (although it's actually called --run-layout-test now).

> So LayoutTests have status=test (and experimental) enabled by default except
> inside the virtual/stable virtual test suite.
Got it. Thanks for the explanation.

> 
> > Anyway, I changed the experiment feature to FEATURE_ENABLED_BY_DEFAULT (from
> > XXX_DISABLED_BY_DEFAULT), and so we see the "has been blocked" warning.
> > Please let me know if I'm doing it wrong. Thanks.
> 
> Have you verified that running your Chrome build manually you don't now get
the
> experimental behavior enabled?  You don't want to accidentally ship this just
> yet ;-).  The tests should be testing what we expect to ship in M60 and you
> should manually verify that (other than warnings) things behave as before in
> M59.  If necessary we can add tests to the 'stable' testsuite to also test the
> M59 behavior but I don't think it's worth worrying about.
Verified, after changing ENABLED_BY_DEFAULT to DISABLED_BY_DEFAULT.

> 
> > On 2017/03/29 18:01:40, Rick Byers wrote:
> > > Why are we getting this version of the warning when status=experimental
> > features
> > > are enabled by default for LayoutTests (outside virtual/stable)? 
Shouldn't
> we
> > > be getting the "has been blocked" warning?
> >

[binlu, 2017-04-14 04:39:01]

The CQ bit was checked by binlu@chromium.org

[binlu, 2017-04-14 04:39:02]

The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org,
jwd@chromium.org
Link to the patchset: https://codereview.chromium.org/2778693004/#ps440001
(title: "address the comments.")

[commit-bot: I haz the power, 2017-04-14 04:39:21]

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-04-14 04:46:59]

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

[commit-bot: I haz the power, 2017-04-14 04:47:00]

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

[Bin Lu, 2017-04-14 04:51:03]

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

[commit-bot: I haz the power, 2017-04-14 04:51:18]

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

[blink-reviews, 2017-04-14 04:53:03]

The presubmit failed due to "Missing LGTM from an OWNER for these files":
content/child/runtime_features.cc
content/public/common/content_features.cc
content/public/common/content_features.h

jochen@, Could you help do the owner review for those files? Thanks.

On Fri, Apr 14, 2017 at 12:47 AM, commit-bot@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> Try jobs failed on following builders:
> chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
> http://build.chromium.org/p/tryserver.chromium.linux/
> builders/chromium_presubmit/builds/411338)
>
> https://codereview.chromium.org/2778693004/
>

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

[chromium-reviews, 2017-04-14 04:53:03]

The presubmit failed due to "Missing LGTM from an OWNER for these files":
content/child/runtime_features.cc
content/public/common/content_features.cc
content/public/common/content_features.h

jochen@, Could you help do the owner review for those files? Thanks.

On Fri, Apr 14, 2017 at 12:47 AM, commit-bot@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> Try jobs failed on following builders:
> chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
> http://build.chromium.org/p/tryserver.chromium.linux/
> builders/chromium_presubmit/builds/411338)
>
> https://codereview.chromium.org/2778693004/
>

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

[commit-bot: I haz the power, 2017-04-14 07:56:33]

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

[commit-bot: I haz the power, 2017-04-14 07:56:34]

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

[Bin Lu, 2017-04-18 03:44:42]

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

[commit-bot: I haz the power, 2017-04-18 03:44:59]

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-04-18 03:47:35]

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

[commit-bot: I haz the power, 2017-04-18 03:47:36]

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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[binlu, 2017-04-18 04:45:56]

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

[commit-bot: I haz the power, 2017-04-18 04:46:21]

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-04-18 04:49:36]

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

[commit-bot: I haz the power, 2017-04-18 04:49:38]

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

[Bin Lu, 2017-04-18 05:19:41]

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

[commit-bot: I haz the power, 2017-04-18 05:19: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, 2017-04-18 07:13:58]

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

[commit-bot: I haz the power, 2017-04-18 07:13:59]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-04-18 15:10:09]

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

https://codereview.chromium.org/2778693004/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:94: if
(!frame->HasReceivedUserGesture()) {
should this be done via the Deprecation class
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Dep...)

That would automatically include a target milestone and produce use counter data

[Bin Lu, 2017-04-18 15:47:43]

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

[commit-bot: I haz the power, 2017-04-18 15:48:11]

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

[binlu, 2017-04-18 15:48:30]

https://codereview.chromium.org/2778693004/diff/280001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/vibration/resources/vibrate-from-iframe.html
(right):

https://codereview.chromium.org/2778693004/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/vibration/resources/vibrate-from-iframe.html:2:
<script src="../../resources/testharness.js"></script>
On 2017/04/13 19:05:04, mustaq wrote:
> Add a <title>?

Done.

https://codereview.chromium.org/2778693004/diff/280001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/vibration/vibration-patterns.html (right):

https://codereview.chromium.org/2778693004/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/vibration/vibration-patterns.html:13: // See:
https://www.chromestatus.com/feature/5644273861001216.
On 2017/04/13 19:55:27, binlu wrote:
> On 2017/04/13 19:05:04, mustaq wrote:
> > The chromestatus links here seem redundant because the same link is there in
> the
> > main code as well as in console messages. Please remove all such links from
> > *html comments*.
> 
> Will remove them shortly.

Done.

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

https://codereview.chromium.org/2778693004/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:94: if
(!frame->HasReceivedUserGesture()) {
On 2017/04/18 15:10:08, jochen wrote:
> should this be done via the Deprecation class
>
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Dep...)
> 
> That would automatically include a target milestone and produce use counter
data

Done.

[jochen (gone - plz use gerrit), 2017-04-18 15:51:10]

you'll also need to modify histograms.xml for the new use counter (there's a
script that does that in that directory)

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

https://codereview.chromium.org/2778693004/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:99: message =
can you add a UseCounter here as well?

https://codereview.chromium.org/2778693004/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:105: message
=
same here?

[commit-bot: I haz the power, 2017-04-18 15:53:00]

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

[commit-bot: I haz the power, 2017-04-18 15:53:01]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  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_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_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_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Bin Lu, 2017-04-18 16:12:01]

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

[commit-bot: I haz the power, 2017-04-18 16:12:23]

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

[binlu, 2017-04-18 16:13:00]

Thanks Jochen for the review!

On 2017/04/18 15:51:10, jochen wrote:
> you'll also need to modify histograms.xml for the new use counter (there's a
> script that does that in that directory)
Done.

[binlu, 2017-04-18 16:14:43]

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

https://codereview.chromium.org/2778693004/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:99: message =
On 2017/04/18 15:51:10, jochen wrote:
> can you add a UseCounter here as well?

Those metrics are already collected in CollectHistogramMetrics.

https://codereview.chromium.org/2778693004/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:105: message
=
On 2017/04/18 15:51:10, jochen wrote:
> same here?

ditto.

[commit-bot: I haz the power, 2017-04-18 16:16:16]

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

[commit-bot: I haz the power, 2017-04-18 16:16:18]

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...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-04-18 18:38:10]

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

[commit-bot: I haz the power, 2017-04-18 18:38:47]

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

[jochen (gone - plz use gerrit), 2017-04-18 19:21:19]

lgtm

[commit-bot: I haz the power, 2017-04-18 20:46:29]

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

[commit-bot: I haz the power, 2017-04-18 20:46:31]

Dry run: This issue passed the CQ dry run.

[binlu, 2017-04-18 21:01:15]

The CQ bit was checked by binlu@chromium.org

[binlu, 2017-04-18 21:01:15]

The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org,
jwd@chromium.org
Link to the patchset: https://codereview.chromium.org/2778693004/#ps540001
(title: "rebase")

[commit-bot: I haz the power, 2017-04-18 21:01:50]

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-04-18 21:12:08]

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

[commit-bot: I haz the power, 2017-04-18 21:12:10]

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

[lunalu1, 2017-04-18 21:12:45]

lgtm

[binlu, 2017-04-19 00:21:52]

The CQ bit was checked by binlu@chromium.org

[binlu, 2017-04-19 00:21:53]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
rbyers@chromium.org, jwd@chromium.org, lunalu@chromium.org
Link to the patchset: https://codereview.chromium.org/2778693004/#ps580001
(title: "remove deleted file")

[commit-bot: I haz the power, 2017-04-19 00:22:47]

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-04-19 03:00:07]

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

[commit-bot: I haz the power, 2017-04-19 03:00:09]

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

[Bin Lu, 2017-04-19 03:21:23]

The CQ bit was checked by binlu@google.com

[commit-bot: I haz the power, 2017-04-19 03:21:46]

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-04-19 05:48:15]

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

[commit-bot: I haz the power, 2017-04-19 05:48:17]

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

[Bin Lu, 2017-04-19 11:39:47]

The CQ bit was checked by binlu@google.com

[commit-bot: I haz the power, 2017-04-19 11:40:06]

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-04-19 11:47:08]

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

[commit-bot: I haz the power, 2017-04-19 11:47:10]

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

[Bin Lu, 2017-04-19 13:22:23]

The CQ bit was checked by binlu@google.com

[Bin Lu, 2017-04-19 13:22:24]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
rbyers@chromium.org, jwd@chromium.org, lunalu@chromium.org
Link to the patchset: https://codereview.chromium.org/2778693004/#ps600001
(title: "rebase")

[commit-bot: I haz the power, 2017-04-19 13:22:46]

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-04-19 15:38:52]

CQ is committing da patch.

Bot data: {"patchset_id": 600001, "attempt_start_ts": 1492608143448740,
"parent_rev": "ff6a1da8d92c61fe4e8f15d1ee070faa4cb60e40", "commit_rev":
"95b15909d2610cf35f90bd3f63d41050376de804"}

[commit-bot: I haz the power, 2017-04-19 15:39:09]

CQ is committing da patch.

Bot data: {"patchset_id": 600001, "attempt_start_ts": 1492608143448740,
"parent_rev": "6e935156eb8bfd135d843804d2c62b2e3d742945", "commit_rev":
"c5b59119ca57398e87a692c9ea836efee2627966"}

[commit-bot: I haz the power, 2017-04-19 15:39:25]

Description was changed from

==========
Remove navigator.vibrate without user gesture.

Calls to navigator.vibrate will immediately return 'false' if user
hasn't tapped on the frame or any embedded frame yet.

The implementation is behind an experimental flag.

The Intent to Deprecate thread:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/QGcpjisxtik

BUG=704650
==========

to

==========
Remove navigator.vibrate without user gesture.

Calls to navigator.vibrate will immediately return 'false' if user
hasn't tapped on the frame or any embedded frame yet.

The implementation is behind an experimental flag.

The Intent to Deprecate thread:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/QGcpjisxtik

BUG=704650

Review-Url: https://codereview.chromium.org/2778693004
Cr-Commit-Position: refs/heads/master@{#465618}
Committed:
https://chromium.googlesource.com/chromium/src/+/c5b59119ca57398e87a692c9ea83...
==========

[commit-bot: I haz the power, 2017-04-19 15:39:27]

Committed patchset #31 (id:600001) as
https://chromium.googlesource.com/chromium/src/+/c5b59119ca57398e87a692c9ea83...

[foolip, 2017-06-16 08:00:39]

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

[foolip, 2017-06-16 08:00:42]

https://codereview.chromium.org/2778693004/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/Deprecation.cpp (right):

https://codereview.chromium.org/2778693004/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/Deprecation.cpp:422: M60,
"5644273861001216");
We're now at M61, can this be removed and merged back?

[binlu, 2017-06-20 15:55:59]

On 2017/06/16 08:00:42, foolip wrote:
>
https://codereview.chromium.org/2778693004/diff/600001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/frame/Deprecation.cpp (right):
> 
>
https://codereview.chromium.org/2778693004/diff/600001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/Deprecation.cpp:422: M60,
> "5644273861001216");
> We're now at M61, can this be removed and merged back?

The deprecation message is shown only when VibrateRequiresUserGestureEnabled is
false, and so it won't be triggered in M60+ since that flag is true in M60+.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/vibrat...
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/vibrat...

If it's okay, I'd prefer to keep it until M60 is released and we are certain
that we don't revert this change.

[foolip, 2017-06-20 20:53:45]

On 2017/06/20 15:55:59, binlu wrote:
> On 2017/06/16 08:00:42, foolip wrote:
> >
>
https://codereview.chromium.org/2778693004/diff/600001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/frame/Deprecation.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2778693004/diff/600001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/frame/Deprecation.cpp:422: M60,
> > "5644273861001216");
> > We're now at M61, can this be removed and merged back?
> 
> The deprecation message is shown only when VibrateRequiresUserGestureEnabled
is
> false, and so it won't be triggered in M60+ since that flag is true in M60+.
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/vibrat...
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/vibrat...
> 
> If it's okay, I'd prefer to keep it until M60 is released and we are certain
> that we don't revert this change.

Sure, that's fine, I thought this had missed the branch cut as happens from time
to time with removals.