third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html - Issue 2805763004: [System-Keyboard-Lock] Forward navigator functions to RenderFrameHost

Unified Diff: third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html

Issue 2805763004: [System-Keyboard-Lock] Forward navigator functions to RenderFrameHost (Closed)

Patch Set: Resolve review comments Created 3 years, 8 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

« no previous file with comments | « third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json ('k') | third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html

diff --git a/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html b/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html

new file mode 100644

index 0000000000000000000000000000000000000000..08fda36d82234d411c18373fe277243dfec2b96b

--- /dev/null

+++ b/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html

@@ -0,0 +1,54 @@

+<!DOCTYPE html>

+<title>Keyboard lock interface</title>

whywhat 2017/04/13 02:34:36 nit: It's common to keep each test case in a singl

Hzj_jie 2017/04/13 22:24:25 Oh, I misunderstood your meaning: in content_brows

+<script src="/resources/testharness.js"></script>

+<script src="/resources/testharnessreport.js"></script>

+<script>

+'use strict';

+test(() => {

+ // NavigatorKeyboardLock implements both requestKeyLock() and cancelKeyLock()

+ // functions;

+ assert_true('requestKeyLock' in navigator);

whywhat 2017/04/13 02:34:36 nit: you can add checks that these are both functi

Hzj_jie 2017/04/13 22:24:25 Done.

+ assert_true('cancelKeyLock' in navigator);

+}, 'Keyboard Lock IDL test');

+promise_test((t) => {

+ const p1 = navigator.requestKeyLock(['a', 'b']);

+ const p2 = navigator.requestKeyLock(['c', 'd']);

+ return p1.then(() => {

+ promise_rejects(t, null, p2,

whywhat 2017/04/13 02:34:36 I tried your cl locally. If I change the Navigator

Hzj_jie 2017/04/13 22:24:25 This is out of my expectation: Promise.then() retu

On 2017/04/13 02:34:36, whywhat wrote: > I tried your cl locally. If I change the NavigatorKeyboardLock.cpp like below, > the test still passes: > > index 4069544f2637..2c22ca763590 100644 > --- a/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp > +++ b/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp > @@ -41,9 +41,10 @@ ScriptPromise NavigatorKeyboardLock::requestKeyLock( > const Vector<String>& keycodes) { > DCHECK(state); > if (request_keylock_resolver_) { > - return ScriptPromise::Reject( > - state, V8String(state->GetIsolate(), > - "Last requestKeyLock() has not finished yet.")); > + request_keylock_resolver_->Resolve(); > + // return ScriptPromise::Reject( > + // state, V8String(state->GetIsolate(), > + // "Last requestKeyLock() has not finished yet.")); > } > > if (!service_) { > @@ -89,6 +90,7 @@ void NavigatorKeyboardLock::cancelKeyLock(Navigator& > navigator) { > > void NavigatorKeyboardLock::LockRequestFinished(bool allowed, > const String& reason) { > + if (!request_keylock_resolver_) return; > DCHECK(request_keylock_resolver_); > if (allowed) > request_keylock_resolver_->Resolve(); > > Note that for promise_test() you return one promise and the test will pass if > this promise fulfills and fail if it rejects. Once it's settled, no further > callback code will have effect (as test.done() is called) so calling > promise_rejects from there doesn't have any effect AFAIK. > > I think you could just return promise_rejects(): > > promise_test((t) => { > const p1 = navigator.requestKeyLock(['a', 'b']); > const p2 = navigator.requestKeyLock(['c', 'd']); > return promise_rejects(t, null, p2); > }, 'Test that calling requestKeyLock() rejects before the previous promise was > settled'); > > However, as is, the next test would fail because it may be run before p1 is > resolved so its promise will get rejected too. > Having each test in a separate file fixes that.

This is out of my expectation: Promise.then() returns a new Promise, i.e. "return p1.then(() => ...)" returns a new Promise instance other than p1 which can only be resolved once the then() is executed. Maybe I should also "return promise_rejects(t, null, p2, ...);" from "p1.then(() => ...)" to ensure the "then" Promise won't be resolved until the "promise_rejects" Promise is resolved. And yes, you are correct, I wrote it like this to ensure both Promises p1 and p2 would be resolved before next test is being executed. But since your solution is pretty simple and clear, I am happy to use it.

whywhat 2017/04/13 22:40:24 Ack. Maybe returning promise_rejects() is all it t

On 2017/04/13 at 22:24:25, Hzj_jie wrote: > On 2017/04/13 02:34:36, whywhat wrote: > > I tried your cl locally. If I change the NavigatorKeyboardLock.cpp like below, > > the test still passes: > > > > index 4069544f2637..2c22ca763590 100644 > > --- a/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp > > +++ b/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp > > @@ -41,9 +41,10 @@ ScriptPromise NavigatorKeyboardLock::requestKeyLock( > > const Vector<String>& keycodes) { > > DCHECK(state); > > if (request_keylock_resolver_) { > > - return ScriptPromise::Reject( > > - state, V8String(state->GetIsolate(), > > - "Last requestKeyLock() has not finished yet.")); > > + request_keylock_resolver_->Resolve(); > > + // return ScriptPromise::Reject( > > + // state, V8String(state->GetIsolate(), > > + // "Last requestKeyLock() has not finished yet.")); > > } > > > > if (!service_) { > > @@ -89,6 +90,7 @@ void NavigatorKeyboardLock::cancelKeyLock(Navigator& > > navigator) { > > > > void NavigatorKeyboardLock::LockRequestFinished(bool allowed, > > const String& reason) { > > + if (!request_keylock_resolver_) return; > > DCHECK(request_keylock_resolver_); > > if (allowed) > > request_keylock_resolver_->Resolve(); > > > > Note that for promise_test() you return one promise and the test will pass if > > this promise fulfills and fail if it rejects. Once it's settled, no further > > callback code will have effect (as test.done() is called) so calling > > promise_rejects from there doesn't have any effect AFAIK. > > > > I think you could just return promise_rejects(): > > > > promise_test((t) => { > > const p1 = navigator.requestKeyLock(['a', 'b']); > > const p2 = navigator.requestKeyLock(['c', 'd']); > > return promise_rejects(t, null, p2); > > }, 'Test that calling requestKeyLock() rejects before the previous promise was > > settled'); > > > > However, as is, the next test would fail because it may be run before p1 is > > resolved so its promise will get rejected too. > > Having each test in a separate file fixes that. > > This is out of my expectation: Promise.then() returns a new Promise, i.e. "return p1.then(() => ...)" returns a new Promise instance other than p1 which can only be resolved once the then() is executed. Maybe I should also "return promise_rejects(t, null, p2, ...);" from "p1.then(() => ...)" to ensure the "then" Promise won't be resolved until the "promise_rejects" Promise is resolved. > > And yes, you are correct, I wrote it like this to ensure both Promises p1 and p2 would be resolved before next test is being executed. But since your solution is pretty simple and clear, I am happy to use it.

Ack. Maybe returning promise_rejects() is all it takes to fix your test then.

+ 'requestKeyLock() should only be ' +

+ 'executed if another request has finished.');

+ }).catch((reason) => {

+ assert_true(false, 'The first request should be allowed.');

+ });

+}, 'Keyboard Lock requestKeyLock twice concurrently test');

+promise_test(() => {

+ return navigator.requestKeyLock(['a', 'b'])

+ .then(() => {})

+ .catch((reason) => {

+ assert_true(false, 'requestKeyLock should succeed.');

+ });

+}, 'Keyboard Lock requestKeyLock test');

+promise_test(() => {

+ return navigator.requestKeyLock(['a', 'b'])

whywhat 2017/04/13 02:34:36 This test still passes if I comment all the rest o

Hzj_jie 2017/04/13 22:24:25 After some investigations, the behavior of promise

After some investigations, the behavior of promise_test() is explainable. As long as we return a Promise in the then() function, it won't be ignored. But if nothing is returned from then(), current Promise is resolved by design. I have applied your change to comment out request_keylock_resolver_ = nullptr line, and change the sequential test into return navigator.requestKeyLock(['a', 'b']) .then(() => { - navigator.requestKeyLock(['c', 'd']) + return navigator.requestKeyLock(['c', 'd']) .then(() => {}) .catch((reason) => { the test fails as expected. This document https://developers.google.com/web/fundamentals/getting-started/primers/promises explains the behavior of chaining promises, which can apply here.

whywhat 2017/04/13 22:40:24 Yeah, I was wondering that chaining should work. G

On 2017/04/13 at 22:24:25, Hzj_jie wrote: > On 2017/04/13 02:34:36, whywhat wrote: > > This test still passes if I comment all the rest out and modify the > > implementation like this: > > > > index 4069544f2637..fe74ebfba76c 100644 > > --- a/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp > > +++ b/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp > > @@ -94,7 +94,7 @@ void NavigatorKeyboardLock::LockRequestFinished(bool allowed, > > request_keylock_resolver_->Resolve(); > > else > > request_keylock_resolver_->Reject(reason); > > - request_keylock_resolver_ = nullptr; > > +// request_keylock_resolver_ = nullptr; > > } > > > > // static > > > > For similar reasons - as long as the first promise resolves, promise_test() > > marks the test as done and catch for the second promise doesn't fail the test. > > > > This should be an async_test that can be written like this (note t.step_func() > > usage as it's not a promise_test()): > > > > async_test((t) => { > > navigator.requestKeyLock([]).then(t.step_func(() => { > > navigator.requestKeyLock([]) > > .then(t.step_func_done()) > > .catch(t.unreached_func()); > > })) > > .catch(t.unreached_func()); > > }, 'Tests that second requestKeyLock succeeds once the promise returned by the > > first one is fulfilled.'); > > After some investigations, the behavior of promise_test() is explainable. As long as we return a Promise in the then() function, it won't be ignored. But if nothing is returned from then(), current Promise is resolved by design. > > I have applied your change to comment out request_keylock_resolver_ = nullptr line, and change the sequential test into > > return navigator.requestKeyLock(['a', 'b']) > .then(() => { > - navigator.requestKeyLock(['c', 'd']) > + return navigator.requestKeyLock(['c', 'd']) > .then(() => {}) > .catch((reason) => { > > the test fails as expected. > > This document https://developers.google.com/web/fundamentals/getting-started/primers/promises explains the behavior of chaining promises, which can apply here.

Yeah, I was wondering that chaining should work. Glad it does!

+ .then(() => {

+ navigator.requestKeyLock(['c', 'd'])

+ .then(() => {})

+ .catch((reason) => {

+ assert_true(false, 'requestKeyLock should succeed.');

+ });

+ })

+ .catch((reason) => {

+ assert_true(false, 'requestKeyLock should succeed.');

+ });

+}, 'Keyboard Lock requestKeyLock twice sequentially test');

+test(() => {

+ navigator.cancelKeyLock();

+}, 'Keyboard Lock cancelKeyLock test');

+</script>