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

Side by Side 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:

« 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 »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 <!DOCTYPE html>

	2 <title>Keyboard lock interface</title>
	whywhat 2017/04/13 02:34:36 nit: It's common to keep each test case in a singl nit: It's common to keep each test case in a single file as they may run in parallel and have dependencies (see my comment below about the check that a second request fails if there's one pending). Hzj_jie 2017/04/13 22:24:25 Oh, I misunderstood your meaning: in content_brows Show quoted text On 2017/04/13 02:34:36, whywhat wrote: > nit: It's common to keep each test case in a single file as they may run in > parallel and have dependencies (see my comment below about the check that a > second request fails if there's one pending). Oh, I misunderstood your meaning: in content_browsertests, several test cases can be merged together. But here, we prefer to execute them in parallel.
	3 <script src="/resources/testharness.js"></script>

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

	5 <script>

	6 'use strict';

	7

	8 test(() => {

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

	10 // functions;

	11 assert_true('requestKeyLock' in navigator);
	whywhat 2017/04/13 02:34:36 nit: you can add checks that these are both functi nit: you can add checks that these are both functions and even their return types by checking the result of typeof of their results. assert_equals(typeof(navigator.requestKeyLock), "function"); Hzj_jie 2017/04/13 22:24:25 Done. Show quoted text On 2017/04/13 02:34:36, whywhat wrote: > nit: you can add checks that these are both functions and even their return > types by checking the result of typeof of their results. > > assert_equals(typeof(navigator.requestKeyLock), "function"); Done.
	12 assert_true('cancelKeyLock' in navigator);

	13 }, 'Keyboard Lock IDL test');

	14

	15 promise_test((t) => {

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

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

	18

	19 return p1.then(() => {

	20 promise_rejects(t, null, p2,
	whywhat 2017/04/13 02:34:36 I tried your cl locally. If I change the Navigator 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. Hzj_jie 2017/04/13 22:24:25 This is out of my expectation: Promise.then() retu Show quoted text 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 Show quoted text 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.
	21 'requestKeyLock() should only be ' +

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

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

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

	25 });

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

	27

	28 promise_test(() => {

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

	30 .then(() => {})

	31 .catch((reason) => {

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

	33 });

	34 }, 'Keyboard Lock requestKeyLock test');

	35

	36 promise_test(() => {

	37 return navigator.requestKeyLock(['a', 'b'])
	whywhat 2017/04/13 02:34:36 This test still passes if I comment all the rest o 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.'); Hzj_jie 2017/04/13 22:24:25 After some investigations, the behavior of promise Show quoted text 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. whywhat 2017/04/13 22:40:24 Yeah, I was wondering that chaining should work. G Show quoted text 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!
	38 .then(() => {

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

	40 .then(() => {})

	41 .catch((reason) => {

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

	43 });

	44 })

	45 .catch((reason) => {

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

	47 });

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

	49

	50 test(() => {

	51 navigator.cancelKeyLock();

	52 }, 'Keyboard Lock cancelKeyLock test');

	53

	54 </script>

OLD	NEW