Block navigator.vibrate in cross-domain iframe.

Block navigator.vibrate in cross-domain iframe.
Intent to implement and ship: https://groups.google.com/a/chromium.org/d/msg/blink-dev/7iVcwNcO3xw/WQSkkuk5BQAJ

BUG=625044
Committed: https://crrev.com/73c8d462f16d232661175c790bf476f4cda24874
Cr-Commit-Position: refs/heads/master@{#421873}

[Bin Lu, 2016-09-19 04:53:21]

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

[commit-bot: I haz the power, 2016-09-19 04:53:26]

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-09-19 05:02:17]

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

[commit-bot: I haz the power, 2016-09-19 05:02:18]

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

[Bin Lu, 2016-09-19 05:02:22]

Description was changed from

==========
Block navigator.vibrate in cross-domain iframe.

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

to

==========
Block navigator.vibrate in cross-domain iframe.
Intent to implement and ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/7iVcwNcO3xw/WQSkkuk5...

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

[Bin Lu, 2016-09-19 05:02:23]

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

[Bin Lu, 2016-09-19 05:06:36]

Hi Ojan and Nate,

Is this the right way to do it? If yes, I will add test in
third_party/WebKit/LayoutTests/http/tests/security/. 

Thanks,
Bin

[Bin Lu, 2016-09-19 05:29:03]

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

[commit-bot: I haz the power, 2016-09-19 05:29:13]

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

[Bin Lu, 2016-09-19 05:46:44]

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

[commit-bot: I haz the power, 2016-09-19 05:46:49]

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

[commit-bot: I haz the power, 2016-09-19 05:55:20]

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

[commit-bot: I haz the power, 2016-09-19 05:55:21]

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

[Bin Lu, 2016-09-19 06:29:18]

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

[commit-bot: I haz the power, 2016-09-19 06:29:26]

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-09-19 07:46:34]

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

[commit-bot: I haz the power, 2016-09-19 07:46:35]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2016-09-19 19:33:29]

That looks right to me. Just a nitpick about the error message.

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

https://codereview.chromium.org/2354433002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:87: "The
cross-domain iframe is disallowed to call navigator.vibrate:
https://www.chromestatus.com/feature/5682658461876224.");
"A cross-origin iframe may not call navigator.vibrate" is a little shorter and
clearer IMO.

[Bin Lu, 2016-09-20 03:09:06]

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

[commit-bot: I haz the power, 2016-09-20 03:09: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, 2016-09-20 04:42:12]

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

[commit-bot: I haz the power, 2016-09-20 04:42:12]

Dry run: This issue passed the CQ dry run.

[Bin Lu, 2016-09-20 05:29:06]

Done. Thanks, Nate!

On 2016/09/19 19:33:29, Nate Chapin wrote:
> That looks right to me. Just a nitpick about the error message.
> 
>
https://codereview.chromium.org/2354433002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp
(right):
> 
>
https://codereview.chromium.org/2354433002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:87: "The
> cross-domain iframe is disallowed to call navigator.vibrate:
> https://www.chromestatus.com/feature/5682658461876224.%22);
> "A cross-origin iframe may not call navigator.vibrate" is a little shorter and
> clearer IMO.

[Nate Chapin, 2016-09-20 18:32:18]

Code looks good, but the test should probably be included in this CL.

[ojan, 2016-09-20 22:57:47]

I keep going back and forth on whether we should return false or remove the API
from frames where it's not enabled. 

Given that feature policy and site engagement aren't quite ready to ship, I
think we should move forward with this patch (once it gets a test). But, please
put a TODO in the code like:

// TODO(binlu): Once FeaturePolicy is ready, exploring using it to remove the
API instead of
// having it return false.

[Bin Lu, 2016-09-21 08:02:35]

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

[commit-bot: I haz the power, 2016-09-21 08:02: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, 2016-09-21 14:50:54]

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

[Bin Lu, 2016-09-21 14:51:07]

The CQ bit was unchecked by binlu@google.com

[Bin Lu, 2016-09-21 14:51:31]

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

[commit-bot: I haz the power, 2016-09-21 14:51: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, 2016-09-21 16:08:51]

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

[commit-bot: I haz the power, 2016-09-21 16:08:51]

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, 2016-09-23 06:13:30]

binlu@google.com changed reviewers:
+ nate chapin，mvanouwerkerk@chromium.org
- japhet@chromium.org

[Bin Lu, 2016-09-23 06:14:14]

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

[commit-bot: I haz the power, 2016-09-23 06:14:27]

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

[Bin Lu, 2016-09-23 06:15:33]

Done. Added test too although not sure if it's correct (since this is the first
test I wrote a layout test in Chrome :-).


On 2016/09/20 22:57:47, ojan wrote:
> I keep going back and forth on whether we should return false or remove the
API
> from frames where it's not enabled. 
> 
> Given that feature policy and site engagement aren't quite ready to ship, I
> think we should move forward with this patch (once it gets a test). But,
please
> put a TODO in the code like:
> 
> // TODO(binlu): Once FeaturePolicy is ready, exploring using it to remove the
> API instead of
> // having it return false.

[Bin Lu, 2016-09-23 06:46:14]

binlu@google.com changed reviewers:
+ japhet@chromium.org, mvanouwerkerk@chromium.org
- nate chapin，mvanouwerkerk@chromium.org

[commit-bot: I haz the power, 2016-09-23 07:28:44]

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

[commit-bot: I haz the power, 2016-09-23 07:28:45]

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, 2016-09-28 16:23:41]

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

[commit-bot: I haz the power, 2016-09-28 16:24:04]

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

[Bin Lu, 2016-09-28 16:28:06]

Fixed the tests. PTAL. 
There is an empty file in the CL:
"...//vibrate_in_same_origin_iframe_allowed-expected.txt".
It's supposed to be removal, but when I try, it says "-expected.txt was
missing".
Please let me know if there is a way to remove it and make the test work.
Thanks.


On 2016/09/23 06:15:33, Bin Lu wrote:
> Done. Added test too although not sure if it's correct (since this is the
first
> test I wrote a layout test in Chrome :-).
> 
> 
> On 2016/09/20 22:57:47, ojan wrote:
> > I keep going back and forth on whether we should return false or remove the
> API
> > from frames where it's not enabled. 
> > 
> > Given that feature policy and site engagement aren't quite ready to ship, I
> > think we should move forward with this patch (once it gets a test). But,
> please
> > put a TODO in the code like:
> > 
> > // TODO(binlu): Once FeaturePolicy is ready, exploring using it to remove
the
> > API instead of
> > // having it return false.

[commit-bot: I haz the power, 2016-09-28 18:06:54]

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

[commit-bot: I haz the power, 2016-09-28 18:06:55]

Dry run: This issue passed the CQ dry run.

[ojan, 2016-09-28 21:06:21]

The CQ bit was checked by ojan@chromium.org

[ojan, 2016-09-28 21:06:22]

lgtm

[commit-bot: I haz the power, 2016-09-28 21:06:43]

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

[ojan, 2016-09-28 21:07:48]

The CQ bit was unchecked by ojan@chromium.org

[ojan, 2016-09-28 21:08:52]

https://codereview.chromium.org/2354433002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/resources/same-origin-iframe-for-vibrate-allowed.html
(right):

https://codereview.chromium.org/2354433002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/resources/same-origin-iframe-for-vibrate-allowed.html:10:
assert_true(navigator.vibrate(200));
Acutally...wait, does this not output a PASS line or something in the
-expected.txt. I'm less familiar with test_harness.js.

[ojan, 2016-09-28 21:12:34]

On 2016/09/28 at 21:08:52, ojan wrote:
>
https://codereview.chromium.org/2354433002/diff/160001/third_party/WebKit/Lay...
> File
third_party/WebKit/LayoutTests/http/tests/security/resources/same-origin-iframe-for-vibrate-allowed.html
(right):
> 
>
https://codereview.chromium.org/2354433002/diff/160001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/http/tests/security/resources/same-origin-iframe-for-vibrate-allowed.html:10:
assert_true(navigator.vibrate(200));
> Acutally...wait, does this not output a PASS line or something in the
-expected.txt. I'm less familiar with test_harness.js.

Hmmm...I guess it doesn't. I thought we didn't require the -expected.txt file in
that case.

[ojan, 2016-09-28 21:13:38]

+tkent, can you comment on what the right thing to do for -expected.txt files
for tests that use testharness.js and just have passing asserts?

In the meantime, I think we can land this patch and fix it up later if this is
doing the wrong thing.

[ojan, 2016-09-28 21:13:49]

The CQ bit was checked by ojan@chromium.org

[commit-bot: I haz the power, 2016-09-28 21:14: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, 2016-09-28 21:24:39]

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

[commit-bot: I haz the power, 2016-09-28 21:24:41]

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, 2016-09-28 21:33:49]

Hi Michael, could you please help do the owner review for
Source/modules/vibration/?

I'm see ** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp

Thanks,
Bin

On 2016/09/28 21:24:41, commit-bot: I haz the power 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_presub...)

[tkent, 2016-09-28 23:25:34]

On 2016/09/28 at 21:13:38, ojan wrote:
> +tkent, can you comment on what the right thing to do for -expected.txt files
for tests that use testharness.js and just have passing asserts?

In general, it's right.  We don't check in such -expected.txt.
However, it doesn't mean -expected.txt becomes empty.  I think tests in the
latest patch set don't print assert_*() in the top-level document, and
-expected.txts don't contain iframe content, or tests finish before loading
iframe content.  I'm afraid these tests don't make sense at this moment.

[Michael van Ouwerkerk, 2016-09-29 13:36:16]

lgtm with nit

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

https://codereview.chromium.org/2354433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:89: "A
cross-origin iframe may not call navigator.vibrate:
https://www.chromestatus.com/feature/5682658461876224.");
nit: Has this message been discussed? Technically, you can _call_ it, it just
doesn't do anything.

[Bin Lu, 2016-09-29 16:44:30]

> nit: Has this message been discussed? Technically, you can _call_ it, it just
> doesn't do anything.
Good point. Changed it to "A call of navigator.vibrate will be no-op inside
cross-origin iframes".
Please let me know if there is any better option. Thanks.


On 2016/09/29 13:36:16, Michael van Ouwerkerk wrote:
> lgtm with nit
> 
>
https://codereview.chromium.org/2354433002/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp
(right):
> 
>
https://codereview.chromium.org/2354433002/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp:89: "A
> cross-origin iframe may not call navigator.vibrate:
> https://www.chromestatus.com/feature/5682658461876224.%22);
> nit: Has this message been discussed? Technically, you can _call_ it, it just
> doesn't do anything.

[Bin Lu, 2016-09-29 16:46:39]

On 2016/09/28 23:25:34, tkent wrote:
> On 2016/09/28 at 21:13:38, ojan wrote:
> > +tkent, can you comment on what the right thing to do for -expected.txt
files
> for tests that use testharness.js and just have passing asserts?
> 
> In general, it's right.  We don't check in such -expected.txt.
> However, it doesn't mean -expected.txt becomes empty.  I think tests in the
> latest patch set don't print assert_*() in the top-level document, and
> -expected.txts don't contain iframe content, or tests finish before loading
> iframe content.  I'm afraid these tests don't make sense at this moment.

The -expected.txt does have an output of the error message for the blocked case,
and only the -expected.txt for the allowed case is empty.

[Bin Lu, 2016-09-29 16:46:46]

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

[commit-bot: I haz the power, 2016-09-29 16:47:11]

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-09-29 18:14:15]

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

[commit-bot: I haz the power, 2016-09-29 18:14:16]

Dry run: This issue passed the CQ dry run.

[Bin Lu, 2016-09-29 18:16:51]

The CQ bit was checked by binlu@google.com

[Bin Lu, 2016-09-29 18:16:52]

The patchset sent to the CQ was uploaded after l-g-t-m from ojan@chromium.org,
mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/2354433002/#ps180001
(title: "Change the error message to: "A call of navigator.vibrate will be no-op
inside cross-origin iframes"")

[commit-bot: I haz the power, 2016-09-29 18:17:29]

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

[commit-bot: I haz the power, 2016-09-29 18:25:22]

Description was changed from

==========
Block navigator.vibrate in cross-domain iframe.
Intent to implement and ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/7iVcwNcO3xw/WQSkkuk5...

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

to

==========
Block navigator.vibrate in cross-domain iframe.
Intent to implement and ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/7iVcwNcO3xw/WQSkkuk5...

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

[commit-bot: I haz the power, 2016-09-29 18:25:23]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-29 18:27:53]

Description was changed from

==========
Block navigator.vibrate in cross-domain iframe.
Intent to implement and ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/7iVcwNcO3xw/WQSkkuk5...

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

to

==========
Block navigator.vibrate in cross-domain iframe.
Intent to implement and ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/7iVcwNcO3xw/WQSkkuk5...

BUG=625044

Committed: https://crrev.com/73c8d462f16d232661175c790bf476f4cda24874
Cr-Commit-Position: refs/heads/master@{#421873}
==========

[commit-bot: I haz the power, 2016-09-29 18:27:55]

Patchset 10 (id:??) landed as
https://crrev.com/73c8d462f16d232661175c790bf476f4cda24874
Cr-Commit-Position: refs/heads/master@{#421873}

[tkent, 2016-09-29 23:24:29]

On 2016/09/29 at 16:46:39, binlu wrote:
> > In general, it's right.  We don't check in such -expected.txt.
> > However, it doesn't mean -expected.txt becomes empty.  I think tests in the
> > latest patch set don't print assert_*() in the top-level document, and
> > -expected.txts don't contain iframe content, or tests finish before loading
> > iframe content.  I'm afraid these tests don't make sense at this moment.
> 
> The -expected.txt does have an output of the error message for the blocked
case, and only the -expected.txt for the allowed case is empty.

ok, then we can find behavior changes by the tests.
However, The tests still wrong.  They have test() and assert_*(), but
-expected.txts don't contain neither PASS nor FAIL. We should run test() in the
top-level document.