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: Add external/wpt tests 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:

« third_party/WebKit/LayoutTests/VirtualTestSuites ('K') | « third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json ('k') | third_party/WebKit/LayoutTests/virtual/keyboard_lock/external/wpt/keyboard_lock/README.txt » ('j') | third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h » ('J')
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>

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

	12 assert_true('cancelKeyLock' in navigator);

	13 }, 'Keyboard Lock IDL test');

	14

	15 promise_test(() => {

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

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

	18

	19 return Promise.all([p1, p2])

	20 .then(() => {

	21 assert_true(false, 'requestKeyLock should only be executed if ' +
	whywhat 2017/04/12 16:02:13 I think you should just use promise_rejects() here I think you should just use promise_rejects() here and below instead of promise_test() if you need to verify that promise is rejected. Hzj_jie 2017/04/13 00:38:54 Done. Show quoted text On 2017/04/12 16:02:13, whywhat wrote: > I think you should just use promise_rejects() here and below instead of > promise_test() if you need to verify that promise is rejected. Done.
	22 'another request has finished.');

	23 })

	24 .catch((reason) => {});

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

	26

	27 promise_test(() => {

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

	29 .then(() => {})

	30 .catch((reason) => {

	31 // TODO(zijiehe): The Promise should be resolved once the function has

	32 // been implemented.

	33 assert_true(true, 'requestKeyLock should succeed.');

	34 });

	35 }, 'Keyboard Lock requestKeyLock test');

	36

	37 promise_test(() => {

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

	39 .then(() => {

	40 navigator.requestKeyLock(['c', 'd'])
	whywhat 2017/04/12 16:02:13 I'm not sure this is a correct usage of promise_te I'm not sure this is a correct usage of promise_test - it will just check that the first promise you return will not reject and will resolve probably without executing the then()/catch() clauses. You should probably use async_test and t.unreached_func() for then() handlers like in the example I think I gave: https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/media/rem... If you verified the test passes and fails correctly locally if you pretend to reject and resolve in your implementation, then it's okay to leave as is :) Also, if your implementation does return the rejection reason, you should probably verify the reason value (e.g. DOMException type and so on). Hzj_jie 2017/04/13 00:38:54 Yes, I was a little bit confusing about the async_ Show quoted text On 2017/04/12 16:02:13, whywhat wrote: > I'm not sure this is a correct usage of promise_test - it will just check that > the first promise you return will not reject and will resolve probably without > executing the then()/catch() clauses. You should probably use async_test and > t.unreached_func() for then() handlers like in the example I think I gave: > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/media/rem... > > If you verified the test passes and fails correctly locally if you pretend to > reject and resolve in your implementation, then it's okay to leave as is :) > > Also, if your implementation does return the rejection reason, you should > probably verify the reason value (e.g. DOMException type and so on). Yes, I was a little bit confusing about the async_test and promise_test, but the example of promise_test (https://cs.chromium.org/chromium/src/docs/testing/writing_layout_tests.md?typ...) shows it accepts a Promise object. And indeed a promise_test is an async_test (https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources...). As long as the test passes, I think current solution seems clean and readable. The spec has not actively talked about the reason we should return from the rejected Promise. I have updated the implementation in browser layer to return a true instead of false with "Unsupported" to avoid the misleading information from this change. P.S. I tried to use internals, but the logic in renderer process (NavigatorKeyboardLock) is too simple (just forwarding the requests), mocking its behavior does not really help to test its correctness.
	41 .then(() => {})

	42 .catch((reason) => {

	43 // TODO(zijiehe): The Promise should be resolved once the function

	44 // has been implemented.

	45 assert_true(true, 'requestKeyLock should succeed.');

	46 });

	47 })

	48 .catch((reason) => {

	49 // TODO(zijiehe): The Promise should be resolved once the function has

	50 // been implemented.

	51 assert_true(true, 'requestKeyLock should succeed.');

	52 });

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

	54

	55 test(() => {

	56 navigator.cancelKeyLock();

	57 }, 'Keyboard Lock cancelKeyLock test');

	58

	59 </script>

OLD	NEW