[System-Keyboard-Lock] Forward navigator functions to RenderFrameHost (issue 2805763004 by zijiehe@chromium.org)

[ava...@chromium.org]

https://codereview.chromium.org/2805763004/diff/240001/third_party/WebKit/Source/core/frame/Navigator.idl
File third_party/WebKit/Source/core/frame/Navigator.idl (right):

https://codereview.chromium.org/2805763004/diff/240001/third_party/WebKit/Source/core/frame/Navigator.idl#newcode35
third_party/WebKit/Source/core/frame/Navigator.idl:35: void
cancelKeyLock();
Drive-by review: I don't think we often add new interfaces here
directly. Instead, new APIs are developed as modules (under
Source/modules/).
Just search for "partial interface Navigator" in .idl files in that
directory and you'll find many examples.

Also, as you don't have an approval to ship this API yet, you should
keep it behind the flag (look at RuntimeEnabledFeatures.json5 and
RuntimeEnabled= annotation in .idl files).

https://codereview.chromium.org/2805763004/

[zij...@chromium.org]

Reviewers: whywhat
CL: https://codereview.chromium.org/2805763004/

Message:

Thank you WhyWhat. I am moving the implementation out of Navigator class.
This feature may be different with other implementations, we will have a flag in
Chromium. So do we still need one in the blink, or should I just add a flag in
blink instead of Chromium?

Description:
[System-Keyboard-Lock] Forward navigator functions to RenderFrameHost

This change forwards navigator.requestKeyLock() and navigator.cancelKeyLock()
functions to the RenderFrameHostImpl.

System-Keyboard-Lock is a feature to detect the key presses which usually cannot
be received by the browser, and send them to the web page. It contains various
components to achieve the goal. This change is one of them, which receives
JavaScript (or Web Platform API in the design doc) function calls and forwards
them into RenderFrameHost. For detail, please refer to the design doc at,
https://docs.google.com/document/d/1T9gJHYdA1VGZ6QHQeOu0hOacWWWJ7yNEt1VDbLju4bs/edit#heading=h.cgwemqs2j4ta

W3C Working Draft: https://garykac.github.io/system-keyboard-lock/
Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/9pauQUAvrcw/lfbG7eunCAAJ

BUG=680809
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Affected files (+666, -2 lines):
M content/browser/frame_host/render_frame_host_impl.h
M content/browser/frame_host/render_frame_host_impl.cc
A content/browser/keyboard_lock/keyboard_lock_browsertest.cc
M content/common/frame.mojom
M content/renderer/render_frame_impl.h
M content/renderer/render_frame_impl.cc
M content/test/BUILD.gn
A content/test/data/keyboard_lock/cancel_keylock.html
A content/test/data/keyboard_lock/cancel_keylock.js
A content/test/data/keyboard_lock/request_keylock.html
A content/test/data/keyboard_lock/request_keylock.js
A content/test/data/keyboard_lock/request_keylock_twice.html
A content/test/data/keyboard_lock/request_keylock_twice.js
M third_party/WebKit/Source/core/frame/Navigator.h
M third_party/WebKit/Source/core/frame/Navigator.cpp
M third_party/WebKit/Source/core/frame/Navigator.idl
M third_party/WebKit/Source/core/loader/EmptyClients.h
M third_party/WebKit/Source/core/loader/EmptyClients.cpp
M third_party/WebKit/Source/core/page/ChromeClient.h
M third_party/WebKit/Source/platform/BUILD.gn
A third_party/WebKit/Source/platform/KeyboardLocker.h
A third_party/WebKit/Source/platform/KeyboardLocker.cpp
M third_party/WebKit/Source/web/BUILD.gn
M third_party/WebKit/Source/web/ChromeClientImpl.h
M third_party/WebKit/Source/web/ChromeClientImpl.cpp
A third_party/WebKit/Source/web/WebKeyLockRequestCompletionImpl.h
A third_party/WebKit/Source/web/WebKeyLockRequestCompletionImpl.cpp
M third_party/WebKit/public/BUILD.gn
M third_party/WebKit/public/web/WebFrameClient.h
A third_party/WebKit/public/web/WebKeyLockRequestCompletion.h

[ava...@chromium.org]

On 2017/04/10 at 21:08:44, zijiehe wrote:
> On 2017/04/10 04:32:45, whywhat wrote:
> >
https://codereview.chromium.org/2805763004/diff/240001/third_party/WebKit/Source/core/frame/Navigator.idl
> > File third_party/WebKit/Source/core/frame/Navigator.idl (right):
> >
> >
https://codereview.chromium.org/2805763004/diff/240001/third_party/WebKit/Source/core/frame/Navigator.idl#newcode35
> > third_party/WebKit/Source/core/frame/Navigator.idl:35: void cancelKeyLock();
> > Drive-by review: I don't think we often add new interfaces here directly.
> > Instead, new APIs are developed as modules (under Source/modules/).
> > Just search for "partial interface Navigator" in .idl files in that
directory
> > and you'll find many examples.
> >
> > Also, as you don't have an approval to ship this API yet, you should keep it
> > behind the flag (look at RuntimeEnabledFeatures.json5 and RuntimeEnabled=
> > annotation in .idl files).
>
> Thank you WhyWhat. I am moving the implementation out of Navigator class.
> This feature may be different with other implementations, we will have a flag
in Chromium. So do we still need one in the blink, or should I just add a flag
in blink instead of Chromium?

I don't think you can escape not having the flag in RuntimeEnabledFeatures.
Adding one is very easy - you add an entry with the name like SystemKeyboardLock
to the RuntimeEnabledFeatures.json5 file and add
[RuntimeEnabled=SystemKeyboardLock] in the .idl file for your
interfaces/methods.
The initial value would be 'testing' and when you have some backend for it
checked-in - you can change it to 'experimental'.
You only can change it to 'stable' when you get an approval to ship the API via
the Blink Intent to Ship process.

I believe someone familiar with the API should do the initial review and would
be a better guide for you through implementing the API in Blink. Someone from
the Intent to Implement email might be a good choice.

https://codereview.chromium.org/2805763004/

[ava...@chromium.org]

https://codereview.chromium.org/2805763004/

[zij...@chromium.org]

Oh, I am not escaping from adding a flag. I just want to ensure I am following a
regular routine, and the flags in blink can be easily enabled in test cases,
which is the most important way I verify the changes before the feature has been
fully finished.

https://codereview.chromium.org/2805763004/

[ava...@chromium.org]

Oh, sorry, I just meant having a chromium flag is not enough.

You can read more about RuntimeEnabledFeatures here:
https://www.chromium.org/blink/runtime-enabled-features

https://codereview.chromium.org/2805763004/

[zij...@chromium.org]

https://codereview.chromium.org/2805763004/diff/240001/third_party/WebKit/Source/core/frame/Navigator.idl
File third_party/WebKit/Source/core/frame/Navigator.idl (right):

https://codereview.chromium.org/2805763004/diff/240001/third_party/WebKit/Source/core/frame/Navigator.idl#newcode35
third_party/WebKit/Source/core/frame/Navigator.idl:35: void
cancelKeyLock();
On 2017/04/10 04:32:45, whywhat wrote:
> Drive-by review: I don't think we often add new interfaces here
directly.
> Instead, new APIs are developed as modules (under Source/modules/).
> Just search for "partial interface Navigator" in .idl files in that
directory
> and you'll find many examples.
>
> Also, as you don't have an approval to ship this API yet, you should
keep it
> behind the flag (look at RuntimeEnabledFeatures.json5 and
RuntimeEnabled=
> annotation in .idl files).

Done.

https://codereview.chromium.org/2805763004/

[dgla...@chromium.org]

Could the tests be layout tests?
https://chromium.googlesource.com/chromium/src/+/master/docs/testing/layout_tests.md

If so, this is how we would typically test web-exposed behaviors. Ideally, they
would be web-platform-tests.

https://codereview.chromium.org/2805763004/

[ava...@chromium.org]

I think in general you can avoid lots of ChromeClient plumbing by registering a
separate Mojo interface with FrameHost and acquiring it via GetInterfaceProvider
directly in NavigatorKeyboardLock.


https://codereview.chromium.org/2805763004/diff/260001/content/browser/keyboard_lock/keyboard_lock_browsertest.cc
File content/browser/keyboard_lock/keyboard_lock_browsertest.cc (right):

https://codereview.chromium.org/2805763004/diff/260001/content/browser/keyboard_lock/keyboard_lock_browsertest.cc#newcode50
content/browser/keyboard_lock/keyboard_lock_browsertest.cc:50: // TODO:
This test should receive "Succeeded" once the implementation in
nit: TODO(username) here and below

https://codereview.chromium.org/2805763004/diff/260001/content/common/frame.mojom
File content/common/frame.mojom (right):

https://codereview.chromium.org/2805763004/diff/260001/content/common/frame.mojom#newcode34
content/common/frame.mojom:34: RequestKeyLock(array<string> key_codes)
I think the intention here is for various APIs to live under their own
Mojo interfaces that FrameHost interface allows the renderer to acquire
via GetInterfaceProvider above - see RegisterMojoInterfaces in
RenderFrameHostImpl and how various mojo services registered there are
implemented.

https://codereview.chromium.org/2805763004/diff/260001/content/common/frame.mojom#newcode37
content/common/frame.mojom:37: CancelKeyLock() => ();
nit: I believe you can just omit => () if it's not used?

https://codereview.chromium.org/2805763004/diff/260001/content/renderer/render_frame_impl.cc
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2805763004/diff/260001/content/renderer/render_frame_impl.cc#newcode7
content/renderer/render_frame_impl.cc:7: #include <iostream>
nit: don't think this is used
out of curiosity, did you use it for logging and LOG(ERROR) or something
like that did not work for you?

https://codereview.chromium.org/2805763004/diff/260001/content/test/data/keyboard_lock/cancel_keylock.html
File content/test/data/keyboard_lock/cancel_keylock.html (right):

https://codereview.chromium.org/2805763004/diff/260001/content/test/data/keyboard_lock/cancel_keylock.html#newcode4
content/test/data/keyboard_lock/cancel_keylock.html:4: <script
type="text/javascript" src="cancel_keylock.js"></script>
I think you can just put all the test code into one .html file and call
different functions (like testCancelKeylock() and testRequestKeylock())
in the browser test.
There's no need to have separate .js files as they are not shared
between the tests and the html files are 99% the same.

https://codereview.chromium.org/2805763004/diff/260001/content/test/data/keyboard_lock/cancel_keylock.js
File content/test/data/keyboard_lock/cancel_keylock.js (right):

https://codereview.chromium.org/2805763004/diff/260001/content/test/data/keyboard_lock/cancel_keylock.js#newcode10
content/test/data/keyboard_lock/cancel_keylock.js:10:
document.getElementById('result').innerHTML = 'Failed: ' + exception;
I believe you could just return the string and examine it in
ExecuteScript without using dom automation.

https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/Source/modules/keyboard_lock/BUILD.gn
File third_party/WebKit/Source/modules/keyboard_lock/BUILD.gn (right):

https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/Source/modules/keyboard_lock/BUILD.gn#newcode1
third_party/WebKit/Source/modules/keyboard_lock/BUILD.gn:1: # Copyright
2017 The Chromium Authors. All rights reserved.
You may want to add an OWNERS file too?

https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
(right):

https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h#newcode47
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:47:
bool RetrieveChromeClient(ChromeClient*&, String&) const;
I don't think you need ChromeClient plumbing at all - you could just get
the service from here (see MediaSession as an example:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/mediasession/MediaSession.cpp?l=193)

https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/public/web/WebKeyLockRequestCompletion.h
File third_party/WebKit/public/web/WebKeyLockRequestCompletion.h
(right):

https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/public/web/WebKeyLockRequestCompletion.h#newcode1
third_party/WebKit/public/web/WebKeyLockRequestCompletion.h:1: /*
I believe the normal three line copyright header should suffice.

https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/public/web/WebKeyLockRequestCompletion.h#newcode46
third_party/WebKit/public/web/WebKeyLockRequestCompletion.h:46: virtual
void DidCompleteRequest(bool, const WebString&) = 0;
you could try using WebCallbacks which is commonly used to resolve
promises:
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebCallbacks.h
One example would be in the Presentation API implementation:
https://cs.chromium.org/webrtc/src/third_party/WebKit/Source/modules/presentation/PresentationAvailabilityCallbacks.cpp

https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/public/web/WebRuntimeFeatures.h
File third_party/WebKit/public/web/WebRuntimeFeatures.h (right):

https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/public/web/WebRuntimeFeatures.h#newcode149
third_party/WebKit/public/web/WebRuntimeFeatures.h:149: BLINK_EXPORT
static void EnableKeyboardLock(bool);
I don't think this is needed until you want to disable this for certain
platforms when shipped (like Chromecast and/or WebView).

https://codereview.chromium.org/2805763004/

[zij...@chromium.org]

https://codereview.chromium.org/2805763004/diff/260001/content/browser/keyboard_lock/keyboard_lock_browsertest.cc
File content/browser/keyboard_lock/keyboard_lock_browsertest.cc (right):

https://codereview.chromium.org/2805763004/diff/260001/content/browser/keyboard_lock/keyboard_lock_browsertest.cc#newcode50
content/browser/keyboard_lock/keyboard_lock_browsertest.cc:50: // TODO:
This test should receive "Succeeded" once the implementation in

On 2017/04/11 14:52:33, whywhat wrote:
> nit: TODO(username) here and below

Done.

https://codereview.chromium.org/2805763004/diff/260001/content/common/frame.mojom
File content/common/frame.mojom (right):

https://codereview.chromium.org/2805763004/diff/260001/content/common/frame.mojom#newcode34
content/common/frame.mojom:34: RequestKeyLock(array<string> key_codes)

On 2017/04/11 14:52:33, whywhat wrote:
> I think the intention here is for various APIs to live under their own
Mojo
> interfaces that FrameHost interface allows the renderer to acquire via
> GetInterfaceProvider above - see RegisterMojoInterfaces in
RenderFrameHostImpl
> and how various mojo services registered there are implemented.

Got you, thank you.
The code has been updated.

https://codereview.chromium.org/2805763004/diff/260001/content/common/frame.mojom#newcode37
content/common/frame.mojom:37: CancelKeyLock() => ();

On 2017/04/11 14:52:33, whywhat wrote:
> nit: I believe you can just omit => () if it's not used?

The plan is to let browser tell renderer the finish of this function,
though for now we do not actively export the result to the web page.
These APIs are still under discussing, some users require the callback
(promise v.s. void) of the navigator.cancelKeyLock(), so I would ensure
in most of the design, the signal is correctly posted.

https://codereview.chromium.org/2805763004/diff/260001/content/renderer/render_frame_impl.cc
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2805763004/diff/260001/content/renderer/render_frame_impl.cc#newcode7
content/renderer/render_frame_impl.cc:7: #include <iostream>

On 2017/04/11 14:52:33, whywhat wrote:
> nit: don't think this is used
> out of curiosity, did you use it for logging and LOG(ERROR) or
something like
> that did not work for you?

Sorry, it's for debugging purpose, and I forgot to remove it.

The change to this file should be removed entirely, as I am now using
mojo interface directly from NavigatorKeyboardLock class instead of
sending through various layers.

https://codereview.chromium.org/2805763004/diff/260001/content/test/data/keyboard_lock/cancel_keylock.html
File content/test/data/keyboard_lock/cancel_keylock.html (right):

https://codereview.chromium.org/2805763004/diff/260001/content/test/data/keyboard_lock/cancel_keylock.html#newcode4
content/test/data/keyboard_lock/cancel_keylock.html:4: <script
type="text/javascript" src="cancel_keylock.js"></script>

On 2017/04/11 14:52:33, whywhat wrote:
> I think you can just put all the test code into one .html file and
call
> different functions (like testCancelKeylock() and
testRequestKeylock()) in the
> browser test.
> There's no need to have separate .js files as they are not shared
between the
> tests and the html files are 99% the same.

Oh, I thought it's a common requirement to do so.

https://codereview.chromium.org/2805763004/diff/260001/content/test/data/keyboard_lock/cancel_keylock.js
File content/test/data/keyboard_lock/cancel_keylock.js (right):

https://codereview.chromium.org/2805763004/diff/260001/content/test/data/keyboard_lock/cancel_keylock.js#newcode10
content/test/data/keyboard_lock/cancel_keylock.js:10:
document.getElementById('result').innerHTML = 'Failed: ' + exception;

On 2017/04/11 14:52:33, whywhat wrote:
> I believe you could just return the string and examine it in
ExecuteScript
> without using dom automation.

It looks like I can merge these tests into one file, and call the
javascript functions in test case. So I would refector these tests.

https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/Source/modules/keyboard_lock/BUILD.gn
File third_party/WebKit/Source/modules/keyboard_lock/BUILD.gn (right):

https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/Source/modules/keyboard_lock/BUILD.gn#newcode1
third_party/WebKit/Source/modules/keyboard_lock/BUILD.gn:1: # Copyright
2017 The Chromium Authors. All rights reserved.

On 2017/04/11 14:52:33, whywhat wrote:
> You may want to add an OWNERS file too?

Done.

https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
(right):

https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h#newcode47
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:47:
bool RetrieveChromeClient(ChromeClient*&, String&) const;

On 2017/04/11 14:52:33, whywhat wrote:
> I don't think you need ChromeClient plumbing at all - you could just
get the
> service from here (see MediaSession as an example:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/mediasession/MediaSession.cpp?l=193)

Yes, I just found this solution: my solution seems very old fashion.
Most of the logic in WebKit have been simplified.

https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/public/web/WebKeyLockRequestCompletion.h
File third_party/WebKit/public/web/WebKeyLockRequestCompletion.h
(right):

https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/public/web/WebKeyLockRequestCompletion.h#newcode1
third_party/WebKit/public/web/WebKeyLockRequestCompletion.h:1: /*

On 2017/04/11 14:52:33, whywhat wrote:
> I believe the normal three line copyright header should suffice.

This file is also deprecated: WebKit/Source/modules/ can talk with
browser through service interface.

https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/public/web/WebKeyLockRequestCompletion.h#newcode46
third_party/WebKit/public/web/WebKeyLockRequestCompletion.h:46: virtual
void DidCompleteRequest(bool, const WebString&) = 0;

On 2017/04/11 14:52:33, whywhat wrote:
> you could try using WebCallbacks which is commonly used to resolve
promises:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebCallbacks.h
> One example would be in the Presentation API implementation:
>
https://cs.chromium.org/webrtc/src/third_party/WebKit/Source/modules/presentation/PresentationAvailabilityCallbacks.cpp

Done.

https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/public/web/WebRuntimeFeatures.h
File third_party/WebKit/public/web/WebRuntimeFeatures.h (right):

https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/public/web/WebRuntimeFeatures.h#newcode149
third_party/WebKit/public/web/WebRuntimeFeatures.h:149: BLINK_EXPORT
static void EnableKeyboardLock(bool);

On 2017/04/11 14:52:33, whywhat wrote:
> I don't think this is needed until you want to disable this for
certain
> platforms when shipped (like Chromecast and/or WebView).

Done.

https://codereview.chromium.org/2805763004/

[ava...@chromium.org]

Mostly nits and missing comments atm.

Based on the failing bots results, I think you need to update the
global-interface-listing layout tests that enumerate everything exposed to the
web (with the test features on; the ones under stable/ dir run without the flags
so you will only have to update them when shipping the api on by default).


https://codereview.chromium.org/2805763004/diff/320001/content/browser/frame_host/render_frame_host_impl.cc
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2805763004/diff/320001/content/browser/frame_host/render_frame_host_impl.cc#newcode18
content/browser/frame_host/render_frame_host_impl.cc:18: #include
"base/strings/utf_string_conversions.h"
nit: don't think you need this

https://codereview.chromium.org/2805763004/diff/320001/content/browser/keyboard_lock/keyboard_lock_browsertest.cc
File content/browser/keyboard_lock/keyboard_lock_browsertest.cc (right):

https://codereview.chromium.org/2805763004/diff/320001/content/browser/keyboard_lock/keyboard_lock_browsertest.cc#newcode25
content/browser/keyboard_lock/keyboard_lock_browsertest.cc:25: // This
browser test is aimed towards exercising the File System API bindings
nit: update comment

https://codereview.chromium.org/2805763004/diff/320001/content/browser/keyboard_lock/keyboard_lock_browsertest.cc#newcode65
content/browser/keyboard_lock/keyboard_lock_browsertest.cc:65: //
TODO(zijiehe): This test should receive "Succeeded" once the
implementation
nit: I'd say you have a layout test for this and the code that enforces
this behavior is also in Blink so testing the same behavior here maybe
redundant.

https://codereview.chromium.org/2805763004/diff/320001/content/common/frame.mojom
File content/common/frame.mojom (right):

https://codereview.chromium.org/2805763004/diff/320001/content/common/frame.mojom#newcode7
content/common/frame.mojom:7: import "mojo/common/string16.mojom";
nit: remove?

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/LayoutTests/VirtualTestSuites
File third_party/WebKit/LayoutTests/VirtualTestSuites (right):

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/LayoutTests/VirtualTestSuites#newcode403
third_party/WebKit/LayoutTests/VirtualTestSuites:403: "args":
["--enable-blink-features=KeyboardLock"]
As Philip explained on the blink-dev@ thread, I don't think you need to
pass the flag and use virtual test suite. All "test" and "experimental"
features are enabled when running wpt tests automatically.

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html
(right):

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html#newcode21
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html:21:
assert_true(false, 'requestKeyLock should only be executed if ' +
I think you should just use promise_rejects() here and below instead of
promise_test() if you need to verify that promise is rejected.

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html#newcode40
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html:40:
navigator.requestKeyLock(['c', 'd'])
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/remoteplayback/prompt-twice-throws.html

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

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
(right):

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode73
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:73:
if (!service_.get()) {
nit: one line blocks don't require {}

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode89
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:89:
if (allowed) {
nit: ditto

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode98
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:98:
// TODO(zijiehe): The response of CancelKeyLock may need to be forwarded
to
nit: you could not submit the unused code path until it's needed, you
can always add this plumbing in a follow up cl once the spec changes.

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
(right):

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h#newcode21
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:21:
class CORE_EXPORT NavigatorKeyboardLock final
nit: A class level comment would be nice to have (we don't have lots of
them in Blink for historic reasons but it's allowed and encouraged to
have as many comments as needed now)

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h#newcode29
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:29:
static ScriptPromise requestKeyLock(ScriptState*,
do you need both the static and non-static versions?
if yes, can one of them be private?

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h#newcode46
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:46:
// bool RetrieveChromeClient(ChromeClient*&, String&) const;
nit: remove with a blank line under it?

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl
(right):

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl#newcode5
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl:5:
partial interface Navigator {
nit: put a comment above the interface with at least a link to the spec
this idl implements.

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom
File
third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom
(right):

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom#newcode7
third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom:7:
interface KeyboardLockService {
nit: add some comments to the interface and methods about the params and
returns?

https://codereview.chromium.org/2805763004/

[ava...@chromium.org]

https://codereview.chromium.org/2805763004/diff/360001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
(right):

https://codereview.chromium.org/2805763004/diff/360001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode50
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:50:
GetSupplementable()->GetFrame()->GetInterfaceProvider()->GetInterface(
You need to check if GetFrame() returns null as frame can be in the
detached state.
That will fix some of the layout tests failing on this patchset right
now.

https://codereview.chromium.org/2805763004/

[zij...@chromium.org]

https://codereview.chromium.org/2805763004/diff/320001/content/browser/frame_host/render_frame_host_impl.cc
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2805763004/diff/320001/content/browser/frame_host/render_frame_host_impl.cc#newcode18
content/browser/frame_host/render_frame_host_impl.cc:18: #include
"base/strings/utf_string_conversions.h"

On 2017/04/12 16:02:12, whywhat wrote:
> nit: don't think you need this

Done.

https://codereview.chromium.org/2805763004/diff/320001/content/browser/keyboard_lock/keyboard_lock_browsertest.cc
File content/browser/keyboard_lock/keyboard_lock_browsertest.cc (right):

https://codereview.chromium.org/2805763004/diff/320001/content/browser/keyboard_lock/keyboard_lock_browsertest.cc#newcode25
content/browser/keyboard_lock/keyboard_lock_browsertest.cc:25: // This
browser test is aimed towards exercising the File System API bindings

On 2017/04/12 16:02:12, whywhat wrote:
> nit: update comment

Done.

https://codereview.chromium.org/2805763004/diff/320001/content/browser/keyboard_lock/keyboard_lock_browsertest.cc#newcode65
content/browser/keyboard_lock/keyboard_lock_browsertest.cc:65: //
TODO(zijiehe): This test should receive "Succeeded" once the
implementation

On 2017/04/12 16:02:12, whywhat wrote:
> nit: I'd say you have a layout test for this and the code that
enforces this
> behavior is also in Blink so testing the same behavior here maybe
redundant.

If it's not a must-have, I would remove this file and its associated js
/ html file from this change: the browser change has not been finished,
I cannot test the behavior other than "Failed: Unsupported" here.

Or maybe I can return "Succeeded" from RenderFrameHostImpl even it takes
no effect. What's the common practice for an unfinished feature like
this?

https://codereview.chromium.org/2805763004/diff/320001/content/common/frame.mojom
File content/common/frame.mojom (right):

https://codereview.chromium.org/2805763004/diff/320001/content/common/frame.mojom#newcode7
content/common/frame.mojom:7: import "mojo/common/string16.mojom";

On 2017/04/12 16:02:13, whywhat wrote:
> nit: remove?

Done.

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/LayoutTests/VirtualTestSuites
File third_party/WebKit/LayoutTests/VirtualTestSuites (right):

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/LayoutTests/VirtualTestSuites#newcode403
third_party/WebKit/LayoutTests/VirtualTestSuites:403: "args":
["--enable-blink-features=KeyboardLock"]

On 2017/04/12 16:02:13, whywhat wrote:
> As Philip explained on the blink-dev@ thread, I don't think you need
to pass the
> flag and use virtual test suite. All "test" and "experimental"
features are
> enabled when running wpt tests automatically.

Done.

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html
(right):

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html#newcode21
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html:21:
assert_true(false, 'requestKeyLock should only be executed if ' +

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.

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html#newcode40
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html:40:
navigator.requestKeyLock(['c', 'd'])

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/remoteplayback/prompt-twice-throws.html
>
> 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?type=cs&q=async_test+package:%5Echromium$&l=132)
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/testharness.js?rcl=e9b931b5db3e382b208607439ebc22ab0a7e5a68&l=523).
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.

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
(right):

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode73
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:73:
if (!service_.get()) {

On 2017/04/12 16:02:13, whywhat wrote:
> nit: one line blocks don't require {}

Done.

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode89
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:89:
if (allowed) {

On 2017/04/12 16:02:13, whywhat wrote:
> nit: ditto

Done.

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode98
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:98:
// TODO(zijiehe): The response of CancelKeyLock may need to be forwarded
to

On 2017/04/12 16:02:13, whywhat wrote:
> nit: you could not submit the unused code path until it's needed, you
can always
> add this plumbing in a follow up cl once the spec changes.

Done.

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
(right):

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h#newcode21
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:21:
class CORE_EXPORT NavigatorKeyboardLock final

On 2017/04/12 16:02:13, whywhat wrote:
> nit: A class level comment would be nice to have (we don't have lots
of them in
> Blink for historic reasons but it's allowed and encouraged to have as
many
> comments as needed now)

Done.

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h#newcode29
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:29:
static ScriptPromise requestKeyLock(ScriptState*,

On 2017/04/12 16:02:13, whywhat wrote:
> do you need both the static and non-static versions?
> if yes, can one of them be private?

I followed some other examples, which public both versions. I simply
thought it was a standard.
Updated.

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h#newcode46
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:46:
// bool RetrieveChromeClient(ChromeClient*&, String&) const;

On 2017/04/12 16:02:13, whywhat wrote:
> nit: remove with a blank line under it?

Sorry, I have not noticed this commented line.

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl
(right):

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl#newcode5
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl:5:
partial interface Navigator {

On 2017/04/12 16:02:13, whywhat wrote:
> nit: put a comment above the interface with at least a link to the
spec this idl
> implements.

The spec is still under discussion; should I use a bug?

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom
File
third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom
(right):

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom#newcode7
third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom:7:
interface KeyboardLockService {

On 2017/04/12 16:02:13, whywhat wrote:
> nit: add some comments to the interface and methods about the params
and
> returns?

Updated. But in Chrome, we never reject the request unless the web page
is out of security context. So I cannot give a very clear description
about the potential strings for the |reason| parameter.

https://codereview.chromium.org/2805763004/diff/360001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
(right):

https://codereview.chromium.org/2805763004/diff/360001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode50
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:50:
GetSupplementable()->GetFrame()->GetInterfaceProvider()->GetInterface(

On 2017/04/12 21:46:11, whywhat wrote:
> You need to check if GetFrame() returns null as frame can be in the
detached
> state.
> That will fix some of the layout tests failing on this patchset right
now.

Done.

https://codereview.chromium.org/2805763004/

[zij...@chromium.org]

I may be wrong, but it seems like the test failures in chromeos do not relate to
my change. The error message is TypeError: Cannot read property 'shadowRoot' of
null.

https://codereview.chromium.org/2805763004/

[ava...@chromium.org]

On 2017/04/13 at 01:17:29, zijiehe wrote:
> I may be wrong, but it seems like the test failures in chromeos do not relate
to my change. The error message is TypeError: Cannot read property 'shadowRoot'
of null.

Sounds like a flake. You can always retry.

https://codereview.chromium.org/2805763004/

[ava...@chromium.org]

So I patched your CL locally and tried to break the layout tests as I suspected
they won't work.

See the results and fix suggestions below - I verified that the tests I
suggested pass and fail as expected, at least on my machine :)

I don't think I'll have more comments once the layout tests are fixed and I'm
not an owner of any of these files so you might start picking owners in order to
submit once you fix the tests.

Presumably I'm not the owner because I could've missed something so you might
have to do a couple of more iterations.

https://codereview.chromium.org/2805763004/diff/320001/content/browser/keyboard_lock/keyboard_lock_browsertest.cc
File content/browser/keyboard_lock/keyboard_lock_browsertest.cc (right):

https://codereview.chromium.org/2805763004/diff/320001/content/browser/keyboard_lock/keyboard_lock_browsertest.cc#newcode65
content/browser/keyboard_lock/keyboard_lock_browsertest.cc:65: //
TODO(zijiehe): This test should receive "Succeeded" once the
implementation

On 2017/04/13 at 00:38:54, Hzj_jie wrote:
> On 2017/04/12 16:02:12, whywhat wrote:
> > nit: I'd say you have a layout test for this and the code that
enforces this
> > behavior is also in Blink so testing the same behavior here maybe
redundant.
>
> If it's not a must-have, I would remove this file and its associated
js / html file from this change: the browser change has not been
finished, I cannot test the behavior other than "Failed: Unsupported"
here.
>
> Or maybe I can return "Succeeded" from RenderFrameHostImpl even it
takes no effect. What's the common practice for an unfinished feature
like this?

I think it's okay to add browser tests later that would actually execute
the implementation part and remove this tests for now. It also makes
your change smaller :)

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl
(right):

https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl#newcode5
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl:5:
partial interface Navigator {

On 2017/04/13 at 00:38:54, Hzj_jie wrote:
> On 2017/04/12 16:02:13, whywhat wrote:
> > nit: put a comment above the interface with at least a link to the
spec this idl
> > implements.
>
> The spec is still under discussion; should I use a bug?

I thought you can just point to
https://garykac.github.io/system-keyboard-lock/
You can update it later if/when it moves.

https://codereview.chromium.org/2805763004/diff/380001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html
(right):

https://codereview.chromium.org/2805763004/diff/380001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html#newcode2
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html:2:
<title>Keyboard lock interface</title>
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).

https://codereview.chromium.org/2805763004/diff/380001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html#newcode11
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html:11:
assert_true('requestKeyLock' in navigator);
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");

https://codereview.chromium.org/2805763004/diff/380001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html#newcode20
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html:20:
promise_rejects(t, null, p2,
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.

https://codereview.chromium.org/2805763004/diff/380001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html#newcode37
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html:37:
return navigator.requestKeyLock(['a', 'b'])
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.');

https://codereview.chromium.org/2805763004/

[zij...@chromium.org]

Thank you Anton for the review and comments.
Add Philips and Avi to cover both blink and content changes.

https://codereview.chromium.org/2805763004/diff/320001/content/browser/keyboard_lock/keyboard_lock_browsertest.cc
File content/browser/keyboard_lock/keyboard_lock_browsertest.cc (right):

https://codereview.chromium.org/2805763004/diff/320001/content/browser/keyboard_lock/keyboard_lock_browsertest.cc#newcode65
content/browser/keyboard_lock/keyboard_lock_browsertest.cc:65: //
TODO(zijiehe): This test should receive "Succeeded" once the
implementation

On 2017/04/13 02:34:36, whywhat wrote:
> On 2017/04/13 at 00:38:54, Hzj_jie wrote:
> > On 2017/04/12 16:02:13, whywhat wrote:
> > > nit: put a comment above the interface with at least a link to the
spec this
> idl
> > > implements.
> >
> > The spec is still under discussion; should I use a bug?
>
> I thought you can just point to
https://garykac.github.io/system-keyboard-lock/
> You can update it later if/when it moves.

Done.

https://codereview.chromium.org/2805763004/diff/380001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html
(right):

https://codereview.chromium.org/2805763004/diff/380001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html#newcode2
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html:2:
<title>Keyboard lock interface</title>

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.

https://codereview.chromium.org/2805763004/diff/380001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html#newcode11
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html:11:
assert_true('requestKeyLock' in navigator);

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.

https://codereview.chromium.org/2805763004/diff/380001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html#newcode20
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html:20:
promise_rejects(t, null, p2,

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.

https://codereview.chromium.org/2805763004/diff/380001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html#newcode37
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html:37:
return navigator.requestKeyLock(['a', 'b'])

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.

https://codereview.chromium.org/2805763004/

[ava...@chromium.org]

non-owners lgtm

https://codereview.chromium.org/2805763004/diff/380001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html
(right):

https://codereview.chromium.org/2805763004/diff/380001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html#newcode20
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html:20:
promise_rejects(t, null, p2,

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

https://codereview.chromium.org/2805763004/diff/380001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html#newcode37
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html:37:
return navigator.requestKeyLock(['a', 'b'])

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

https://codereview.chromium.org/2805763004/

[a...@chromium.org]

content lgtm

https://codereview.chromium.org/2805763004/

[foo...@chromium.org]

Which files should I look at? Too many reviewers to guess :)

https://codereview.chromium.org/2805763004/

[ava...@chromium.org]

Philip, I think you could review everything in third_party/WebKit (esp. the
layout tests).
Daniel's LGTM is needed for the mojom file

I don't think any extra reviewers are necessary.

https://codereview.chromium.org/2805763004/

[zij...@chromium.org]

Sorry for that Philip.
Anton has reviewed and LGTMed all the changes in this CL.
Avi has LGTMed content/*.
Daniel, would you mind to have a look at the changes under
third_party/WebKit/public?
Philip, please have a look at the changes under third_party/WebKit/.

Thank you all.

https://codereview.chromium.org/2805763004/

[foo...@chromium.org]

I've looked at all of third_party/WebKit/, but it would be good if another owner
was added and review this as well, as well as future changes.

On the testing from, the added tests don't exercise the real purpose of this
feature, i.e. they don't test that key events are delivered that wouldn't be
without this feature. What would it take to test this fully using
web-platform-tests? Would ways to automate keyboard input be sufficient, or is
there also a connection to fullscreen that can't be seen in the shape of the
API?

Search for "web-platform-tests" in
https://docs.google.com/document/d/1vlTlsQKThwaX0-lj_iZbVTzyqY7LioqERU8DK3u3XjI/edit?usp=sharing
for what we'll be looking for in an Intent to Ship. At this point it's mostly to
understand exactly which features aren't getting full testing coverage, and not
a blocker.


https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json
File third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json#newcode89200
third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json:89200: +
"keyboard-lock/idl-KeyboardLock.html": [
This file shouldn't need to be modified, did you do it manually, and was
it necessary in order to get the test running?

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html#newcode9
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html:9:
// NavigatorKeyboardLock implements both requestKeyLock() and
cancelKeyLock()
NavigatorKeyboardLock is just the name of the Blink IDL file, in the
spec it's just a partial interface. So, you could
s/NavigatorKeyboardLock implements/Navigator has/, but better still to
write an idlharness.js test that will take care of the mere existence
tests. Here are some you can copy from:
https://github.com/w3c/web-platform-tests/blob/master/battery-status/battery-interface-idlharness.html
https://github.com/w3c/web-platform-tests/blob/master/gamepad/idlharness.html
https://github.com/w3c/web-platform-tests/blob/master/mediasession/idlharness.html
https://github.com/w3c/web-platform-tests/blob/master/vibration/idl.html
(a bit atypical)
https://github.com/w3c/web-platform-tests/blob/master/webauthn/interfaces.https.html

Because of [SecureContext], you will need to name it *.https.html

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html#newcode15
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html:15:
const p = navigator.requestKeyLock([]);
The argument is optional, omit it here so that TypeError is thrown if
it's incorrectly required by an implementation. (This is the bit of the
test that cannot be done using idlharness.js)

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockCancel.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockCancel.html
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockCancel.html#newcode2
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockCancel.html:2:
<title>Keyboard Lock - Interface</title>
Pleas exclude "test" from the filename, just methodName.https.html to
match the method under test would be OK. Or
navigator-methodName.https.html perhaps.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockCancel.html#newcode9
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockCancel.html:9:
navigator.cancelKeyLock();
https://garykac.github.io/system-keyboard-lock/ doesn't have a
cancelKeyLock() method, can you update the spec if you've all agreed to
the name change?

You could also assert that the return value is undefined so that if it's
changed to return a promise, this test also has to be updated.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockCancel.html#newcode10
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockCancel.html:10:
}, 'Keyboard Lock cancelKeyLock test');
Omit "test" from titles as well. Since this is the only test in the
file, you can move the title into <title>. Or omit <title>, in any case
two titles aren't useful.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockRequest.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockRequest.html
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockRequest.html#newcode9
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockRequest.html:9:

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

Ditto about this name not being in the spec.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockRequest.html#newcode10
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockRequest.html:10:
.then(() => {})
I think you can omit both the .then and the .catch line. If the returned
promise is resolved the test will pass, and if it's rejected the test
will fail, the promise_test machinery takes care of that.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockRequest.html#newcode14
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockRequest.html:14:
}, 'Keyboard Lock requestKeyLock test');
Ditto about having just one title in the file.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockTwoParallelRequests.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockTwoParallelRequests.html
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockTwoParallelRequests.html#newcode3
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockTwoParallelRequests.html:3:
Keyboard Lock - The second of two parallel requests should be rejected
The spec doesn't return a promise and thus doesn't describe this
behavior. Please update the spec to match what you intend to implement.
(It is of course OK for the implementation to be ahead of the spec
sometimes, but it creates the risk that they're never made to align
again, and should ideally come with plenty of TODOs that cannot be
missed before shipping.)

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockTwoSequentialRequests.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockTwoSequentialRequests.html
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockTwoSequentialRequests.html#newcode12
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockTwoSequentialRequests.html:12:
.then(() => {})
These bits and the outer .catch ought not be needed.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode45
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:45:
state, V8String(state->GetIsolate(),
To reject with a string is somewhat unusual I think. Please work out on
the spec side how it should be rejected, with TypeError or DOMException
of some sort.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode60
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:60:
state, V8String(state->GetIsolate(), "Browser is not connecting."));
Ditto about this rejection. You will still need a description string,
and "Browser is not connecting" is hard for web developers to understand
I'm afraid. Is the cause here unknown, under what circumstances could we
fail to create service_?

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode72
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:72:
if (!service_) {
Can you break out the shared code here into a private void
EnsureService()? (Name not important.)

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode82
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:82:
service_->CancelKeyLock(ConvertToBaseCallback(WTF::Bind([]() {})));
Why does service_->CancelKeyLock take a callback at all? Can you remove
it if it's not needed, or make it optional if it's needed somewhere
else?

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode96
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:96:
request_keylock_resolver_->Reject(reason);
Ditto about rejecting with a string.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h#newcode24
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:24:
: public GarbageCollectedFinalized<NavigatorKeyboardLock>,
Does this need to be finalized? Is it because of the service_ member?

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h#newcode29
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:29:
static NavigatorKeyboardLock& From(Navigator&);
Does ::From need to be public given that both of the methods below
already are?

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl#newcode7
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl:7:
// https://garykac.github.io/system-keyboard-lock/.
Just a link to the spec will suffice, most IDL files are like that.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl#newcode9
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl:9:
// Requests to receive a set of key codes
Please copy the spec'd IDL verbatim and make the minimal number of
changes necessary, which is the extra extended attributes.

This documentation would be good for the methods in the
NavigatorKeyboardLock.h file, perhaps.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl#newcode23
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl:23:
[RuntimeEnabled=KeyboardLock, CallWith=ScriptState] Promise<void>
requestKeyLock(DOMString[] keyCodes);
The argument should be "optional sequence<DOMString> keyCodes". Maybe
that actually doesn't make sense, though, what does it mean to request
no keys in particular?

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl#newcode27
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl:27:
// Once the web page is closed, the user agent implictly executes this
API.
Is such an implicit call anywhere in the code?

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/OWNERS
File third_party/WebKit/Source/modules/keyboard_lock/OWNERS (right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/OWNERS#newcode1
third_party/WebKit/Source/modules/keyboard_lock/OWNERS:1:
zij...@chromium.org
Just one entry in this file and no team/component seams like a problem,
and I suppose is why I was asked to review. Can you add at least a team
and component here, so that there's somebody responsible if you are on
vacation, switch projects, etc? If you can find another individual owner
right now, that'd be great too.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom
File
third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom#newcode14
third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom:14:
// The reason will only be provided if the request is rejected.
Does mojo support optional arguments? In that case just an optional
string argument would achieve the same thing, but I don't know what's
typical for mojo interfaces.

https://codereview.chromium.org/2805763004/

[har...@chromium.org]

I just scanned third_party/WebKit/. Yeah, foolip's comments need to be
addressed.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/OWNERS
File third_party/WebKit/Source/modules/keyboard_lock/OWNERS (right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/OWNERS#newcode1
third_party/WebKit/Source/modules/keyboard_lock/OWNERS:1:
zij...@chromium.org

On 2017/04/17 08:46:05, foolip_UTC7 wrote:
> Just one entry in this file and no team/component seams like a
problem, and I
> suppose is why I was asked to review. Can you add at least a team and
component
> here, so that there's somebody responsible if you are on vacation,
switch
> projects, etc? If you can find another individual owner right now,
that'd be
> great too.

+1 to adding TEAMS and COMPONENTS. Also we normally avoid adding a
person to OWNERS until she commits sufficient patches.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/public/platform/modules/keyboard_lock/OWNERS
File third_party/WebKit/public/platform/modules/keyboard_lock/OWNERS
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/public/platform/modules/keyboard_lock/OWNERS#newcode1
third_party/WebKit/public/platform/modules/keyboard_lock/OWNERS:1:
zij...@chromium.org

Ditto.

https://codereview.chromium.org/2805763004/

[dch...@chromium.org]

https://codereview.chromium.org/2805763004/diff/420001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc
File content/browser/keyboard_lock/keyboard_lock_service_impl.cc
(right):

https://codereview.chromium.org/2805763004/diff/420001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc#newcode19
content/browser/keyboard_lock/keyboard_lock_service_impl.cc:19:
impl->binding_.reset(new
mojo::Binding<blink::mojom::KeyboardLockService>(
Then here, you can call Bind(std::move(request))

https://codereview.chromium.org/2805763004/diff/420001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc#newcode26
content/browser/keyboard_lock/keyboard_lock_service_impl.cc:26: //
TODO(zijiehe): Implementation required.
Usually it's preferable not to land Mojo stubs--this ensures that the
subsequence changes that actually implement the stubs will be reviewed.
Would you be willing to split this up into two portions?

1) Implement the stubs for the Web API, along with any tests that can
land with it.
2) Land the CL with the Mojo changes, with the web portion actually
calling this?

https://codereview.chromium.org/2805763004/diff/420001/content/browser/keyboard_lock/keyboard_lock_service_impl.h
File content/browser/keyboard_lock/keyboard_lock_service_impl.h (right):

https://codereview.chromium.org/2805763004/diff/420001/content/browser/keyboard_lock/keyboard_lock_service_impl.h#newcode33
content/browser/keyboard_lock/keyboard_lock_service_impl.h:33:
std::unique_ptr<mojo::Binding<blink::mojom::KeyboardLockService>>
binding_;
Just use mojo::Binding<> directly, no need to have a unique_ptr
involved. When constructing, just use the single-argument constructor
and pass this.

https://codereview.chromium.org/2805763004/diff/440001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
(right):

https://codereview.chromium.org/2805763004/diff/440001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode89
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:89:
*error_message = String::FromUTF8("Current frame is detached.");
Nit: I think it's relatively rare to see FromUTF8() used; most callsites
just use the implicit String(const char*) constructor.

https://codereview.chromium.org/2805763004/

[zij...@chromium.org]

https://codereview.chromium.org/2805763004/diff/420001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc
File content/browser/keyboard_lock/keyboard_lock_service_impl.cc
(right):

https://codereview.chromium.org/2805763004/diff/420001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc#newcode19
content/browser/keyboard_lock/keyboard_lock_service_impl.cc:19:
impl->binding_.reset(new
mojo::Binding<blink::mojom::KeyboardLockService>(

On 2017/04/17 23:07:52, dcheng wrote:
> Then here, you can call Bind(std::move(request))

If the last sentence before this "Then here" is in the comment of
keyboard_lock_service_impl.h, this comment has been addressed.

https://codereview.chromium.org/2805763004/diff/420001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc#newcode26
content/browser/keyboard_lock/keyboard_lock_service_impl.cc:26: //
TODO(zijiehe): Implementation required.

On 2017/04/17 23:07:52, dcheng wrote:
> Usually it's preferable not to land Mojo stubs--this ensures that the
> subsequence changes that actually implement the stubs will be
reviewed. Would
> you be willing to split this up into two portions?
>
> 1) Implement the stubs for the Web API, along with any tests that can
land with
> it.
> 2) Land the CL with the Mojo changes, with the web portion actually
calling
> this?

I can surely do it, but AFAICT, this may not work well for this feature.
The service side change (to implement these implementations) is large,
and it seems not likely to be finished in a short time. In most of the
time, we would need to use the web api to test its partial finished
behavior.

Though I think the following changes do not depend on blink -- web page
will receive key events in the same way as other keys, I can still
involve you to the CLs if you prefer.

https://codereview.chromium.org/2805763004/diff/420001/content/browser/keyboard_lock/keyboard_lock_service_impl.h
File content/browser/keyboard_lock/keyboard_lock_service_impl.h (right):

https://codereview.chromium.org/2805763004/diff/420001/content/browser/keyboard_lock/keyboard_lock_service_impl.h#newcode33
content/browser/keyboard_lock/keyboard_lock_service_impl.h:33:
std::unique_ptr<mojo::Binding<blink::mojom::KeyboardLockService>>
binding_;

On 2017/04/17 23:07:52, dcheng wrote:
> Just use mojo::Binding<> directly, no need to have a unique_ptr
involved. When
> constructing, just use the single-argument constructor and pass this.

Done.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json
File third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json#newcode89200
third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json:89200: +
"keyboard-lock/idl-KeyboardLock.html": [

On 2017/04/17 08:46:04, foolip_UTC7 wrote:
> This file shouldn't need to be modified, did you do it manually, and
was it
> necessary in order to get the test running?

I used to believe tests not listed in this file won't be executed, and
yes, I have done it manually.
Should this file be automatically generated?

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html#newcode9
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html:9:
// NavigatorKeyboardLock implements both requestKeyLock() and
cancelKeyLock()

On 2017/04/17 08:46:04, foolip_UTC7 wrote:
> NavigatorKeyboardLock is just the name of the Blink IDL file, in the
spec it's
> just a partial interface. So, you could s/NavigatorKeyboardLock
> implements/Navigator has/, but better still to write an idlharness.js
test that
> will take care of the mere existence tests. Here are some you can copy
from:
>
https://github.com/w3c/web-platform-tests/blob/master/battery-status/battery-interface-idlharness.html
>
https://github.com/w3c/web-platform-tests/blob/master/gamepad/idlharness.html
>
https://github.com/w3c/web-platform-tests/blob/master/mediasession/idlharness.html
>
https://github.com/w3c/web-platform-tests/blob/master/vibration/idl.html
(a bit
> atypical)
>
https://github.com/w3c/web-platform-tests/blob/master/webauthn/interfaces.https.html
>
> Because of [SecureContext], you will need to name it *.https.html

Done.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html#newcode15
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html:15:
const p = navigator.requestKeyLock([]);

On 2017/04/17 08:46:04, foolip_UTC7 wrote:
> The argument is optional, omit it here so that TypeError is thrown if
it's
> incorrectly required by an implementation. (This is the bit of the
test that
> cannot be done using idlharness.js)

Done.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockCancel.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockCancel.html
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockCancel.html#newcode2
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockCancel.html:2:
<title>Keyboard Lock - Interface</title>

On 2017/04/17 08:46:04, foolip_UTC7 wrote:
> Pleas exclude "test" from the filename, just methodName.https.html to
match the
> method under test would be OK. Or navigator-methodName.https.html
perhaps.

Done.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockCancel.html#newcode9
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockCancel.html:9:
navigator.cancelKeyLock();

On 2017/04/17 08:46:04, foolip_UTC7 wrote:
> https://garykac.github.io/system-keyboard-lock/ doesn't have a
cancelKeyLock()
> method, can you update the spec if you've all agreed to the name
change?
>
> You could also assert that the return value is undefined so that if
it's changed
> to return a promise, this test also has to be updated.

Yes, Gary is working on changing the spec. There are also several other
changes required.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockCancel.html#newcode10
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockCancel.html:10:
}, 'Keyboard Lock cancelKeyLock test');

On 2017/04/17 08:46:04, foolip_UTC7 wrote:
> Omit "test" from titles as well. Since this is the only test in the
file, you
> can move the title into <title>. Or omit <title>, in any case two
titles aren't
> useful.

Done.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockRequest.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockRequest.html
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockRequest.html#newcode9
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockRequest.html:9:
return navigator.requestKeyLock(['a', 'b'])

On 2017/04/17 08:46:04, foolip_UTC7 wrote:
> Ditto about this name not being in the spec.

Yes, Gary is changing the spec.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockRequest.html#newcode10
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockRequest.html:10:
.then(() => {})

On 2017/04/17 08:46:04, foolip_UTC7 wrote:
> I think you can omit both the .then and the .catch line. If the
returned promise
> is resolved the test will pass, and if it's rejected the test will
fail, the
> promise_test machinery takes care of that.

Done.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockRequest.html#newcode14
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockRequest.html:14:
}, 'Keyboard Lock requestKeyLock test');

On 2017/04/17 08:46:04, foolip_UTC7 wrote:
> Ditto about having just one title in the file.

Done.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockTwoParallelRequests.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockTwoParallelRequests.html
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockTwoParallelRequests.html#newcode3
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockTwoParallelRequests.html:3:
Keyboard Lock - The second of two parallel requests should be rejected

On 2017/04/17 08:46:04, foolip_UTC7 wrote:
> The spec doesn't return a promise and thus doesn't describe this
behavior.
> Please update the spec to match what you intend to implement. (It is
of course
> OK for the implementation to be ahead of the spec sometimes, but it
creates the
> risk that they're never made to align again, and should ideally come
with plenty
> of TODOs that cannot be missed before shipping.)

I have added TODO in the idl file. Once we ship it, the flag will be
removed from idl file, so the TODOs won't be missed.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockTwoSequentialRequests.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockTwoSequentialRequests.html
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockTwoSequentialRequests.html#newcode12
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockTwoSequentialRequests.html:12:
.then(() => {})

On 2017/04/17 08:46:04, foolip_UTC7 wrote:
> These bits and the outer .catch ought not be needed.

Done.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode45
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:45:
state, V8String(state->GetIsolate(),

On 2017/04/17 08:46:04, foolip_UTC7 wrote:
> To reject with a string is somewhat unusual I think. Please work out
on the spec
> side how it should be rejected, with TypeError or DOMException of some
sort.

I am working with Gary to update the spec.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode60
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:60:
state, V8String(state->GetIsolate(), "Browser is not connecting."));

On 2017/04/17 08:46:05, foolip_UTC7 wrote:

> Ditto about this rejection. You will still need a description string,
and
> "Browser is not connecting" is hard for web developers to understand
I'm afraid.
> Is the cause here unknown, under what circumstances could we fail to
create
> service_?

I believe the spec won't cover the detail of exceptions here and the one
above: they are Chrome implementation detail. Maybe we can add a "Other
implementation limitations" error type in the spc and use similar
DOMException with different error strings.

For this case, I believe this could happen if the browser does not
provide these APIs.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode72
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:72:
if (!service_) {

On 2017/04/17 08:46:04, foolip_UTC7 wrote:
> Can you break out the shared code here into a private void
EnsureService()?
> (Name not important.)

Done.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode82
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:82:
service_->CancelKeyLock(ConvertToBaseCallback(WTF::Bind([]() {})));

On 2017/04/17 08:46:05, foolip_UTC7 wrote:

> Why does service_->CancelKeyLock take a callback at all? Can you
remove it if
> it's not needed, or make it optional if it's needed somewhere else?

We have also discussed whether the cancelKeyLock() function should
return a Promise<void> to tell the web page once this operation is
finished. But the decision has not been made.

I do not think mojo supports optional callback.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode96
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:96:
request_keylock_resolver_->Reject(reason);

On 2017/04/17 08:46:04, foolip_UTC7 wrote:
> Ditto about rejecting with a string.

TODO has been added.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h#newcode24
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:24:
: public GarbageCollectedFinalized<NavigatorKeyboardLock>,

On 2017/04/17 08:46:05, foolip_UTC7 wrote:

> Does this need to be finalized? Is it because of the service_ member?

I believe Member<ScriptPromiseResolver> needs to be finalized, right?

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h#newcode29
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:29:
static NavigatorKeyboardLock& From(Navigator&);

On 2017/04/17 08:46:05, foolip_UTC7 wrote:

> Does ::From need to be public given that both of the methods below
already are?

Done.
No, I used to believe this is a convention.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl#newcode7
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl:7:
// https://garykac.github.io/system-keyboard-lock/.

On 2017/04/17 08:46:05, foolip_UTC7 wrote:

> Just a link to the spec will suffice, most IDL files are like that.

Done.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl#newcode9
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl:9:
// Requests to receive a set of key codes

On 2017/04/17 08:46:05, foolip_UTC7 wrote:

> Please copy the spec'd IDL verbatim and make the minimal number of
changes
> necessary, which is the extra extended attributes.
>
> This documentation would be good for the methods in the
NavigatorKeyboardLock.h
> file, perhaps.

Done.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl#newcode23
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl:23:
[RuntimeEnabled=KeyboardLock, CallWith=ScriptState] Promise<void>
requestKeyLock(DOMString[] keyCodes);

On 2017/04/17 08:46:05, foolip_UTC7 wrote:

> The argument should be "optional sequence<DOMString> keyCodes". Maybe
that
> actually doesn't make sense, though, what does it mean to request no
keys in
> particular?

Passing in an empty keyCodes array will reserve all keys.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl#newcode27
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl:27:
// Once the web page is closed, the user agent implictly executes this
API.

On 2017/04/17 08:46:05, foolip_UTC7 wrote:

> Is such an implicit call anywhere in the code?

Yes, the logic is in browser process, which has not been implemented.
The "implicitly" here indicates the key lock status is session based,
and won't be kept by the user agent.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/OWNERS
File third_party/WebKit/Source/modules/keyboard_lock/OWNERS (right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/OWNERS#newcode1
third_party/WebKit/Source/modules/keyboard_lock/OWNERS:1:
zij...@chromium.org

On 2017/04/17 14:15:57, haraken wrote:
> On 2017/04/17 08:46:05, foolip_UTC7 wrote:
> > Just one entry in this file and no team/component seams like a
problem, and I
> > suppose is why I was asked to review. Can you add at least a team
and
> component
> > here, so that there's somebody responsible if you are on vacation,
switch
> > projects, etc? If you can find another individual owner right now,
that'd be
> > great too.
>
> +1 to adding TEAMS and COMPONENTS. Also we normally avoid adding a
person to
> OWNERS until she commits sufficient patches.

he.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/OWNERS#newcode1
third_party/WebKit/Source/modules/keyboard_lock/OWNERS:1:
zij...@chromium.org
On 2017/04/17 08:46:05, foolip_UTC7 wrote:
> Just one entry in this file and no team/component seams like a
problem, and I
> suppose is why I was asked to review. Can you add at least a team and
component
> here, so that there's somebody responsible if you are on vacation,
switch
> projects, etc? If you can find another individual owner right now,
that'd be
> great too.

Added COMPONENT: Services>Chromoting

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/public/platform/modules/keyboard_lock/OWNERS
File third_party/WebKit/public/platform/modules/keyboard_lock/OWNERS
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/public/platform/modules/keyboard_lock/OWNERS#newcode1
third_party/WebKit/public/platform/modules/keyboard_lock/OWNERS:1:
zij...@chromium.org

On 2017/04/17 14:15:57, haraken wrote:
>
> Ditto.

Done.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom
File
third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom#newcode14
third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom:14:
// The reason will only be provided if the request is rejected.

On 2017/04/17 08:46:05, foolip_UTC7 wrote:

> Does mojo support optional arguments? In that case just an optional
string
> argument would achieve the same thing, but I don't know what's typical
for mojo
> interfaces.

mojo supports optional array<string>?. But here we are not sending one
string argument, but a list of strings. Since we always need to do the
conversion in NavigatorKeyboardLock.cpp, using optional argument is not
necessary.

https://codereview.chromium.org/2805763004/diff/440001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
(right):

https://codereview.chromium.org/2805763004/diff/440001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode89
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:89:
*error_message = String::FromUTF8("Current frame is detached.");

On 2017/04/17 23:07:52, dcheng wrote:
> Nit: I think it's relatively rare to see FromUTF8() used; most
callsites just
> use the implicit String(const char*) constructor.

Done.

https://codereview.chromium.org/2805763004/diff/460001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html
(right):

https://codereview.chromium.org/2805763004/diff/460001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html#newcode1
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html:1:
<!doctype html>
It's weird: this test won't be executed on my machine.

https://codereview.chromium.org/2805763004/

[foo...@chromium.org]

Would you like to land these changes before the spec changes are made? It would
be preferable to have the spec for reference, or it's much harder to tell if
this implementation and the tests matches the spec. Making sure of that after
the fact is much harder, at least I would not come back to do it.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json
File third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json#newcode89200
third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json:89200: +
"keyboard-lock/idl-KeyboardLock.html": [

On 2017/04/18 02:26:07, Hzj_jie wrote:
> On 2017/04/17 08:46:04, foolip_UTC7 wrote:
> > This file shouldn't need to be modified, did you do it manually, and
was it
> > necessary in order to get the test running?
>
> I used to believe tests not listed in this file won't be executed, and
yes, I
> have done it manually.
> Should this file be automatically generated?

Before https://chromium-review.googlesource.com/c/447959/ there was just
a MANIFEST.json, and that had to be updated for the tests to actually
run. After that change, WPT_BASE_MANIFEST.json is updated on every
import, and the MANIFEST.json that's actually used is generated when the
tests are run. The WPT_BASE_MANIFEST.json is committed to make the
generation of MANIFEST.json much faster.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode60
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:60:
state, V8String(state->GetIsolate(), "Browser is not connecting."));

On 2017/04/18 02:26:08, Hzj_jie wrote:
> On 2017/04/17 08:46:05, foolip_UTC7 wrote:
> > Ditto about this rejection. You will still need a description
string, and
> > "Browser is not connecting" is hard for web developers to understand
I'm
> afraid.
> > Is the cause here unknown, under what circumstances could we fail to
create
> > service_?
>
> I believe the spec won't cover the detail of exceptions here and the
one above:
> they are Chrome implementation detail. Maybe we can add a "Other
implementation
> limitations" error type in the spc and use similar DOMException with
different
> error strings.
>
> For this case, I believe this could happen if the browser does not
provide these
> APIs.

When would this exception happen? It's possible it would be covered by a
spec-side statement like about the browsing context being discarded, or
the associated document not being fully active. This is the navigator
object, and it doesn't look like the spec fully defines when it's
replaced, so there's no concept of "active" that corresponds to the
GetFrame() checks we use in Navigator.cpp, so I've filed
https://github.com/whatwg/html/issues/2545

If there is any way for a web page to get a promise that is rejected on
this line, then it's web observable and should correspond to the spec in
some way. If it turns out that this is blocked on adding some concepts
to HTML, please file a spec bug for keyboard lock that links to it, and
add a TODO pointing to the keyboard lock issue here.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode82
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:82:
service_->CancelKeyLock(ConvertToBaseCallback(WTF::Bind([]() {})));

On 2017/04/18 02:26:08, Hzj_jie wrote:
> On 2017/04/17 08:46:05, foolip_UTC7 wrote:
> > Why does service_->CancelKeyLock take a callback at all? Can you
remove it if
> > it's not needed, or make it optional if it's needed somewhere else?
>
> We have also discussed whether the cancelKeyLock() function should
return a
> Promise<void> to tell the web page once this operation is finished.
But the
> decision has not been made.
>
> I do not think mojo supports optional callback.

Doesn't mojo support fire-and-forget operations, where you don't care
about the result?

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h#newcode24
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:24:
: public GarbageCollectedFinalized<NavigatorKeyboardLock>,

On 2017/04/18 02:26:08, Hzj_jie wrote:
> On 2017/04/17 08:46:05, foolip_UTC7 wrote:
> > Does this need to be finalized? Is it because of the service_
member?
>
> I believe Member<ScriptPromiseResolver> needs to be finalized, right?

That shouldn't mean that all the things that hold a reference to
ScriptPromiseResolver also need to be finalized. Can you try making this
not finalized to see if there are any compiler errors?

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl#newcode23
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl:23:
[RuntimeEnabled=KeyboardLock, CallWith=ScriptState] Promise<void>
requestKeyLock(DOMString[] keyCodes);

On 2017/04/18 02:26:08, Hzj_jie wrote:
> On 2017/04/17 08:46:05, foolip_UTC7 wrote:
> > The argument should be "optional sequence<DOMString> keyCodes".
Maybe that
> > actually doesn't make sense, though, what does it mean to request no
keys in
> > particular?
>
> Passing in an empty keyCodes array will reserve all keys.

OK. In that case it probably makes sense to add " = []" in the spec as
the default argument value. Otherwise the spec will have to say the same
thing in prose for both a missing argument and an empty array.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl#newcode27
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl:27:
// Once the web page is closed, the user agent implictly executes this
API.

On 2017/04/18 02:26:08, Hzj_jie wrote:
> On 2017/04/17 08:46:05, foolip_UTC7 wrote:
> > Is such an implicit call anywhere in the code?
>
> Yes, the logic is in browser process, which has not been implemented.
The
> "implicitly" here indicates the key lock status is session based, and
won't be
> kept by the user agent.

The spec should probably define precisely which events it is that causes
this to happen. In https://fullscreen.spec.whatwg.org/ it's "Whenever
the unloading document cleanup steps run with a document, fully exit
fullscreen document."

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom
File
third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom#newcode14
third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom:14:
// The reason will only be provided if the request is rejected.

On 2017/04/18 02:26:08, Hzj_jie wrote:
> On 2017/04/17 08:46:05, foolip_UTC7 wrote:
> > Does mojo support optional arguments? In that case just an optional
string
> > argument would achieve the same thing, but I don't know what's
typical for
> mojo
> > interfaces.
>
> mojo supports optional array<string>?. But here we are not sending one
string
> argument, but a list of strings. Since we always need to do the
conversion in
> NavigatorKeyboardLock.cpp, using optional argument is not necessary.

Acknowledged.

https://codereview.chromium.org/2805763004/diff/460001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html
(right):

https://codereview.chromium.org/2805763004/diff/460001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html#newcode1
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html:1:
<!doctype html>

On 2017/04/18 02:26:08, Hzj_jie wrote:
> It's weird: this test won't be executed on my machine.

Were you using run-webkit-tests? After running it and this test didn't
run, can you check if the generated MANIFEST.json includes this file?

https://codereview.chromium.org/2805763004/diff/460001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html#newcode6
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html:6:
<script src="../../../resources/testharness.js"></script>
Not sure, but this may be the reason that the test isn't run. Try
absolute paths here, /resources/testharness.js

https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.https.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.https.html
(right):

https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.https.html#newcode9
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.https.html:9:
// Navigator has both requestKeyLock() and cancelKeyLock() functions.
This should now be covered by the idlharness.js test. Can you fold what
remains of this test into the requestKeyLock() test?

https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html
(right):

https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html#newcode18
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html:18:
[SecureContext, RuntimeEnabled=KeyboardLock, CallWith=ScriptState]
Promise<void> requestKeyLock(optional sequence<DOMString> keyCodes);
Copy the IDL from the spec, not Blink's IDL files.
([RuntimeEnabled=KeyboardLock])

https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-cancelKeyLock.https.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-cancelKeyLock.https.html
(right):

https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-cancelKeyLock.https.html#newcode8
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-cancelKeyLock.https.html:8:
assert_true(navigator.cancelKeyLock() === undefined);
assert_equals(navigator.cancelKeyLock(), undefined);

Could also split into two lines so that the two kinds of failures
possible are on different lines, although one would be able to tell why
it failed anyway.

https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
(right):

https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h#newcode30
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:30:
// (https://www.w3.org/TR/uievents/#interface-keyboardevent) in string
format.
/TR/ links are usually stale snapshots, link to
https://w3c.github.io/uievents/

https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h#newcode39
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:39:
// is allowed; the second request will overwrite the key codes
reserved.
https://garykac.github.io/system-keyboard-lock/#requestSystemKeyboardLock
doesn't seem to say what happens with a second request. I see
https://github.com/garykac/system-keyboard-lock/issues/18, can you file
issues for this as well, and anything else in the implementation that
cannot be derived from the spec?

https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h#newcode46
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:46:
static ScriptPromise requestKeyLock(ScriptState*, Navigator&);
You should be able to remove this if the IDL has [] as the default
value.

https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Source/modules/keyboard_lock/OWNERS
File third_party/WebKit/Source/modules/keyboard_lock/OWNERS (right):

https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Source/modules/keyboard_lock/OWNERS#newcode1
third_party/WebKit/Source/modules/keyboard_lock/OWNERS:1:
zij...@chromium.org
How about gar...@chromium.org and jamie...@chromium.org? As editors
of the spec, they should be the best reviewers of this code after you.

https://codereview.chromium.org/2805763004/

[har...@chromium.org]

https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Source/modules/keyboard_lock/OWNERS
File third_party/WebKit/Source/modules/keyboard_lock/OWNERS (right):

https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Source/modules/keyboard_lock/OWNERS#newcode1
third_party/WebKit/Source/modules/keyboard_lock/OWNERS:1:
zij...@chromium.org

On 2017/04/18 05:10:57, foolip OOO on Wednesday wrote:
> How about mailto:gar...@chromium.org and
mailto:jamie...@chromium.org? As editors of the

> spec, they should be the best reviewers of this code after you.

modules/ have serious code health issues and we've been trying not to
add persons to OWNERS until they get familiar with Blink's
implementation. (I should document this somewhere.)

https://codereview.chromium.org/2805763004/

[zij...@chromium.org]

I have talked offline with Jamie, most of the issues should be addressed in the
spec, though we cannot finished it immediately because of the absent of
GaryKac@.
We are still struggling about whether the events are needed to indicate the
state changes. But it should not impact this change.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json
File third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json#newcode89200
third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json:89200: +
"keyboard-lock/idl-KeyboardLock.html": [

On 2017/04/18 05:10:57, foolip OOO on Wednesday wrote:

> On 2017/04/18 02:26:07, Hzj_jie wrote:
> > On 2017/04/17 08:46:04, foolip_UTC7 wrote:
> > > This file shouldn't need to be modified, did you do it manually,
and was it
> > > necessary in order to get the test running?
> >
> > I used to believe tests not listed in this file won't be executed,
and yes, I
> > have done it manually.
> > Should this file be automatically generated?
>
> Before https://chromium-review.googlesource.com/c/447959/ there was
just a
> MANIFEST.json, and that had to be updated for the tests to actually
run. After
> that change, WPT_BASE_MANIFEST.json is updated on every import, and
the
> MANIFEST.json that's actually used is generated when the tests are
run. The
> WPT_BASE_MANIFEST.json is committed to make the generation of
MANIFEST.json much
> faster.

Got you. Thank you for the explanation.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode60
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:60:
state, V8String(state->GetIsolate(), "Browser is not connecting."));

On 2017/04/18 05:10:57, foolip OOO on Wednesday wrote:

Sorry, I mean the browser process does not provide these APIs, but the
renderer process requires them, which is a Chrome specific
implementation detail.

TODO has been added.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode82
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:82:
service_->CancelKeyLock(ConvertToBaseCallback(WTF::Bind([]() {})));

On 2017/04/18 05:10:57, foolip OOO on Wednesday wrote:

> On 2017/04/18 02:26:08, Hzj_jie wrote:
> > On 2017/04/17 08:46:05, foolip_UTC7 wrote:
> > > Why does service_->CancelKeyLock take a callback at all? Can you
remove it
> if
> > > it's not needed, or make it optional if it's needed somewhere
else?
> >
> > We have also discussed whether the cancelKeyLock() function should
return a
> > Promise<void> to tell the web page once this operation is finished.
But the
> > decision has not been made.
> >
> > I do not think mojo supports optional callback.
>
> Doesn't mojo support fire-and-forget operations, where you don't care
about the
> result?

Yes, the new change uses no-callback operation. But mojo does not
support an optional callback. i.e. =>? is not supported in mojo.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h#newcode24
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:24:
: public GarbageCollectedFinalized<NavigatorKeyboardLock>,

On 2017/04/18 05:10:57, foolip OOO on Wednesday wrote:

> On 2017/04/18 02:26:08, Hzj_jie wrote:
> > On 2017/04/17 08:46:05, foolip_UTC7 wrote:
> > > Does this need to be finalized? Is it because of the service_
member?
> >
> > I believe Member<ScriptPromiseResolver> needs to be finalized,
right?
>
> That shouldn't mean that all the things that hold a reference to
> ScriptPromiseResolver also need to be finalized. Can you try making
this not
> finalized to see if there are any compiler errors?

Yes, it fires compiler error, but I have not remembered the correct
GC-required object. You were right, the |service_| needs to be GCed.

../../third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:23:1:
error: [blink-gc] Class 'NavigatorKeyboardLock' requires finalization.
class NavigatorKeyboardLock final
^
../../third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:71:3:
note: [blink-gc] Field 'service_' requiring finalization declared here:
mojom::blink::KeyboardLockServicePtr service_;

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl#newcode23
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl:23:
[RuntimeEnabled=KeyboardLock, CallWith=ScriptState] Promise<void>
requestKeyLock(DOMString[] keyCodes);

On 2017/04/18 05:10:57, foolip OOO on Wednesday wrote:

> On 2017/04/18 02:26:08, Hzj_jie wrote:
> > On 2017/04/17 08:46:05, foolip_UTC7 wrote:
> > > The argument should be "optional sequence<DOMString> keyCodes".
Maybe that
> > > actually doesn't make sense, though, what does it mean to request
no keys in
> > > particular?
> >
> > Passing in an empty keyCodes array will reserve all keys.
>
> OK. In that case it probably makes sense to add " = []" in the spec as
the
> default argument value. Otherwise the spec will have to say the same
thing in
> prose for both a missing argument and an empty array.

Updated.
TODOs have been added, and point to the corresponding issues.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl#newcode27
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl:27:
// Once the web page is closed, the user agent implictly executes this
API.

On 2017/04/18 05:10:57, foolip OOO on Wednesday wrote:

> On 2017/04/18 02:26:08, Hzj_jie wrote:
> > On 2017/04/17 08:46:05, foolip_UTC7 wrote:
> > > Is such an implicit call anywhere in the code?
> >
> > Yes, the logic is in browser process, which has not been
implemented. The
> > "implicitly" here indicates the key lock status is session based,
and won't be
> > kept by the user agent.
>
> The spec should probably define precisely which events it is that
causes this to
> happen. In https://fullscreen.spec.whatwg.org/ it's "Whenever the
unloading
> document cleanup steps run with a document, fully exit fullscreen
document."

Done.
TODO has been added, and point to the corresponding issue.

https://codereview.chromium.org/2805763004/diff/460001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html
(right):

https://codereview.chromium.org/2805763004/diff/460001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html#newcode1
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html:1:
<!doctype html>

On 2017/04/18 05:10:57, foolip OOO on Wednesday wrote:

> On 2017/04/18 02:26:08, Hzj_jie wrote:
> > It's weird: this test won't be executed on my machine.
>
> Were you using run-webkit-tests? After running it and this test didn't
run, can
> you check if the generated MANIFEST.json includes this file?

Done.

https://codereview.chromium.org/2805763004/diff/460001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html#newcode6
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html:6:
<script src="../../../resources/testharness.js"></script>

On 2017/04/18 05:10:57, foolip OOO on Wednesday wrote:

> Not sure, but this may be the reason that the test isn't run. Try
absolute paths
> here, /resources/testharness.js

Oh, yes, that's the reason. Updated.

Sorry, I verified this file on my local machine, I do not need to start
an http server by using relative URLs.

Another error is raised, requestKeyLock(sequence) calling operation with
this = null didn't throw TypeError.
But the logic is in the generated file from idl.

https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-cancelKeyLock.https.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-cancelKeyLock.https.html
(right):

https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-cancelKeyLock.https.html#newcode8
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-cancelKeyLock.https.html:8:
assert_true(navigator.cancelKeyLock() === undefined);

On 2017/04/18 05:10:57, foolip OOO on Wednesday wrote:

> assert_equals(navigator.cancelKeyLock(), undefined);
>
> Could also split into two lines so that the two kinds of failures
possible are
> on different lines, although one would be able to tell why it failed
anyway.

Done.

https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
(right):

https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h#newcode30
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:30:
// (https://www.w3.org/TR/uievents/#interface-keyboardevent) in string
format.

On 2017/04/18 05:10:57, foolip OOO on Wednesday wrote:

> /TR/ links are usually stale snapshots, link to
https://w3c.github.io/uievents/

Done.

https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h#newcode39
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:39:
// is allowed; the second request will overwrite the key codes
reserved.

On 2017/04/18 05:10:57, foolip OOO on Wednesday wrote:
>

https://garykac.github.io/system-keyboard-lock/#requestSystemKeyboardLock
> doesn't seem to say what happens with a second request. I see
> https://github.com/garykac/system-keyboard-lock/issues/18, can you
file issues
> for this as well, and anything else in the implementation that cannot
be derived
> from the spec?

Yes, I have updated the idl file to include all the TODOs.

https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h#newcode46
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:46:
static ScriptPromise requestKeyLock(ScriptState*, Navigator&);

On 2017/04/18 05:10:57, foolip OOO on Wednesday wrote:

> You should be able to remove this if the IDL has [] as the default
value.

Done.

https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Source/modules/keyboard_lock/OWNERS
File third_party/WebKit/Source/modules/keyboard_lock/OWNERS (right):

https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Source/modules/keyboard_lock/OWNERS#newcode1
third_party/WebKit/Source/modules/keyboard_lock/OWNERS:1:
zij...@chromium.org

On 2017/04/18 07:38:36, haraken wrote:
> On 2017/04/18 05:10:57, foolip OOO on Wednesday wrote:
> > How about mailto:gar...@chromium.org and
mailto:jamie...@chromium.org? As
> editors of the
> > spec, they should be the best reviewers of this code after you.
>
> modules/ have serious code health issues and we've been trying not to
add
> persons to OWNERS until they get familiar with Blink's implementation.
(I should
> document this somewhere.)

I am happy to entirely remove this OWNER file, if it's not a must-have.
I see 74 folders under modules, but only 66 of them have OWNERS file.
Otherwise, I am also OK to add GaryKac@ and JamieWalch@ to it, though
the COMPONENT: Services>Chromoting has covered them already.

https://codereview.chromium.org/2805763004/

[zij...@chromium.org]

Philip, do you happen to know why the idlharness test may fail? I cannot
reproduce it on my machine.

FAIL Navigator interface: operation requestKeyLock(sequence) assert_throws:
calling operation with this = null didn't throw TypeError function "function ()
{
fn.apply(obj, args);
}" did not throw

https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.https.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.https.html
(right):

https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.https.html#newcode9
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.https.html:9:
// Navigator has both requestKeyLock() and cancelKeyLock() functions.

On 2017/04/18 05:10:57, foolip_UTC7 wrote:
> This should now be covered by the idlharness.js test. Can you fold
what remains
> of this test into the requestKeyLock() test?

Done.

https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html
(right):

https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html#newcode18
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html:18:
[SecureContext, RuntimeEnabled=KeyboardLock, CallWith=ScriptState]

Promise<void> requestKeyLock(optional sequence<DOMString> keyCodes);

On 2017/04/18 05:10:57, foolip_UTC7 wrote:
> Copy the IDL from the spec, not Blink's IDL files.
> ([RuntimeEnabled=KeyboardLock])

Done.

https://codereview.chromium.org/2805763004/

[foo...@chromium.org]

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode60
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:60:
state, V8String(state->GetIsolate(), "Browser is not connecting."));

I'd still like to understand exactly when this can happen. If it could
be subsumed by a broader "if x then reject" in the spec, then it would
cease to be a Chrome-specific behavior.

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h#newcode24
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:24:
: public GarbageCollectedFinalized<NavigatorKeyboardLock>,

On 2017/04/19 00:45:54, Hzj_jie wrote:
> On 2017/04/18 05:10:57, foolip OOO on Wednesday wrote:
> > On 2017/04/18 02:26:08, Hzj_jie wrote:
> > > On 2017/04/17 08:46:05, foolip_UTC7 wrote:
> > > > Does this need to be finalized? Is it because of the service_
member?
> > >
> > > I believe Member<ScriptPromiseResolver> needs to be finalized,
right?
> >
> > That shouldn't mean that all the things that hold a reference to
> > ScriptPromiseResolver also need to be finalized. Can you try making
this not
> > finalized to see if there are any compiler errors?
>
> Yes, it fires compiler error, but I have not remembered the correct
GC-required
> object. You were right, the |service_| needs to be GCed.
>
>
../../third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:23:1:
> error: [blink-gc] Class 'NavigatorKeyboardLock' requires finalization.
> class NavigatorKeyboardLock final
> ^
>
../../third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:71:3:
> note: [blink-gc] Field 'service_' requiring finalization declared
here:
> mojom::blink::KeyboardLockServicePtr service_;

Thanks for confirming. We should avoid finalized wherever possible, but
here it's not.

https://codereview.chromium.org/2805763004/diff/460001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html
(right):

https://codereview.chromium.org/2805763004/diff/460001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html#newcode6
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html:6:
<script src="../../../resources/testharness.js"></script>

On 2017/04/19 00:45:55, Hzj_jie wrote:
> On 2017/04/18 05:10:57, foolip OOO on Wednesday wrote:
> > Not sure, but this may be the reason that the test isn't run. Try
absolute
> paths
> > here, /resources/testharness.js
>
> Oh, yes, that's the reason. Updated.
>
> Sorry, I verified this file on my local machine, I do not need to
start an http
> server by using relative URLs.
>

> Another error is raised, requestKeyLock(sequence) calling operation
with this =

> null didn't throw TypeError.
> But the logic is in the generated file from idl.

That is very strange, and sounds like a bug. Non-static methods should
require a this that's an instance of the right type (Navigator here).
This might be a bug with our generated bindings, can you run that part
of the test in a debugger and see where it returns from the
requestKeyLock call? Or just reading the generated code in
V8NavigatorKeyboardLock.cpp could reveal the reason.

https://codereview.chromium.org/2805763004/

[foo...@chromium.org]

https://codereview.chromium.org/2805763004/

[foo...@chromium.org]

On 2017/04/20 21:23:58, Hzj_jie wrote:
> Philip, do you happen to know why the idlharness test may fail? I cannot
> reproduce it on my machine.
>
> FAIL Navigator interface: operation requestKeyLock(sequence) assert_throws:
> calling operation with this = null didn't throw TypeError function "function
()
> {
> fn.apply(obj, args);
> }" did not throw

The code that generated this test is here:
https://github.com/w3c/web-platform-tests/blob/2f4bc7af7fbe4b74aec09310c028de31a85136d5/resources/idlharness.js#L1368

Can you try playing with navigator.requestKeyLock.call(null, []) and similar to
see what's going on? Probably it's a real bug.

https://codereview.chromium.org/2805763004/

[zij...@chromium.org]

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode60
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:60:
state, V8String(state->GetIsolate(), "Browser is not connecting."));

After discussion with Jamie, we believe here we should retry instead of
return an error to the web page.
But considering it's not a one-line change, and this change already took
so long time, I would prefer to do it in a separate change.

In the spec level, this should be covered by the statement of
rejections, something like, "User agent should only reject and discard
the request in following situations: 1. the request is not allowed by
the user. 2. there is already a pending request."

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
(right):

https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h#newcode24
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:24:
: public GarbageCollectedFinalized<NavigatorKeyboardLock>,

Done.

https://codereview.chromium.org/2805763004/diff/460001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html
(right):

https://codereview.chromium.org/2805763004/diff/460001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html#newcode6
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html:6:
<script src="../../../resources/testharness.js"></script>

On 2017/04/21 06:08:32, foolip_UTC7 wrote:
> On 2017/04/19 00:45:55, Hzj_jie wrote:
> > On 2017/04/18 05:10:57, foolip OOO on Wednesday wrote:
> > > Not sure, but this may be the reason that the test isn't run. Try
absolute
> > paths
> > > here, /resources/testharness.js
> >
> > Oh, yes, that's the reason. Updated.
> >
> > Sorry, I verified this file on my local machine, I do not need to
start an
> http
> > server by using relative URLs.
> >

> > Another error is raised, requestKeyLock(sequence) calling operation
with this
> =

> > null didn't throw TypeError.
> > But the logic is in the generated file from idl.
>
> That is very strange, and sounds like a bug. Non-static methods should
require a
> this that's an instance of the right type (Navigator here). This might
be a bug
> with our generated bindings, can you run that part of the test in a
debugger and
> see where it returns from the requestKeyLock call? Or just reading the
generated
> code in V8NavigatorKeyboardLock.cpp could reveal the reason.

The generated code looks correct, and both following pieces of code
-------------
<html>
<head>
<script>
navigator.requestKeyLock.call(null);
</script>
</head>
</html>
-------------
<html>
<head>
<script>
navigator.requestKeyLock.apply(null);
</script>
</head>
</html>
-------------
throws TypeError in promise as expected.

I have dig it a little bit deeper, the webidl2.js does not parse the
return type of requestKeyLock() function correctly or at least not as
the expectation of idlharness.js. (generic == null) So I have made a
change also in idlharness.js, and this test is passing.

https://codereview.chromium.org/2805763004/

[zij...@chromium.org]

Hi, Philip, is the fix in idlharness.js reasonable?

https://codereview.chromium.org/2805763004/

[foo...@chromium.org]

On 2017/04/24 18:45:14, Hzj_jie wrote:
> Hi, Philip, is the fix in idlharness.js reasonable?

It looks a bit odd with the idlType.idlType, but I'm not familiar enough with
idlharness.js or webidl2.js to say if this is the correct fix or a workaround.

How about landing the test with the failure, and then submitting the
idlharness.js change as a pull request to
https://github.com/w3c/web-platform-tests/blob/master/resources/idlharness.js?
Then I could poke the people who know this better, who don't work on Chromium.

https://codereview.chromium.org/2805763004/

[zij...@chromium.org]

On 2017/04/24 19:05:44, foolip_UTC7 wrote:
> On 2017/04/24 18:45:14, Hzj_jie wrote:
> > Hi, Philip, is the fix in idlharness.js reasonable?
>
> It looks a bit odd with the idlType.idlType, but I'm not familiar enough with
> idlharness.js or webidl2.js to say if this is the correct fix or a workaround.
>
> How about landing the test with the failure, and then submitting the
> idlharness.js change as a pull request to
> https://github.com/w3c/web-platform-tests/blob/master/resources/idlharness.js

> Then I could poke the people who know this better, who don't work on Chromium.

Oh, that's interesting. Is this file managed in github instead of Chromium?

I can use the same way as what has been done in media-capabilities tests to
avoid the failure.
I am working on it.

https://codereview.chromium.org/2805763004/

[foo...@chromium.org]

Yes, this is a shared test suite used by all browser vendors to some extent or
another. We're trying to use it as much as possible to help drive
interoperability.
https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_platform_tests.md
documents how it's imported and exported. It's supposed to be seamless so you
don't have to care about the boundary :)

https://codereview.chromium.org/2805763004/

[zij...@chromium.org]

Code has been updated.

https://codereview.chromium.org/2805763004/

[foo...@chromium.org]

third_party/WebKit/LayoutTests/ LGTM % nit.

Please also add keyboard-lock to W3CImportExpectations in this CL. That's how
owners are tracked, for now: https://crbug.com/713987


https://codereview.chromium.org/2805763004/diff/640001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-requestKeyLock.https.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-requestKeyLock.https.html
(right):

https://codereview.chromium.org/2805763004/diff/640001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-requestKeyLock.https.html#newcode9
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-requestKeyLock.https.html:9:
assert_equals(typeof(p.then), 'function');
I guess assert_true(p instanceof Promise) would cover both of these
statements and more?

https://codereview.chromium.org/2805763004/

[zij...@chromium.org]

Since Daniel is OOO, adding Elliott to this change.

https://codereview.chromium.org/2805763004/diff/640001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-requestKeyLock.https.html
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-requestKeyLock.https.html
(right):

https://codereview.chromium.org/2805763004/diff/640001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-requestKeyLock.https.html#newcode9
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-requestKeyLock.https.html:9:
assert_equals(typeof(p.then), 'function');

On 2017/04/25 06:34:15, foolip_UTC7 wrote:
> I guess assert_true(p instanceof Promise) would cover both of these
statements
> and more?

Done.

https://codereview.chromium.org/2805763004/

[har...@chromium.org]

From the implementation perspective, WebKit LGTM.




https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
(right):

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode79
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:79:
bool NavigatorKeyboardLock::EnsureServiceConnected(String*
error_message) {

I'd inline this method into the caller sites because once you want to
handle the exception properly, you anyway need to inline the logic in
the caller sites (e.g., requestKeyLock() rejects promises,
cancelKeyLock() returns with silence etc).

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
(right):

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h#newcode47
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:47:
// the web page is closed, the user agent implictly executes this API.

implicitly

https://codereview.chromium.org/2805763004/

[dch...@chromium.org]

https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc
File content/browser/keyboard_lock/keyboard_lock_service_impl.cc
(right):

https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc#newcode13
content/browser/keyboard_lock/keyboard_lock_service_impl.cc:13: :
binding_(this, std::move(request)) {}
Nit: #include <utility>

https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc#newcode21
content/browser/keyboard_lock/keyboard_lock_service_impl.cc:21: new
KeyboardLockServiceImpl(std::move(request));
Is this leaked? The binding isn't a strong binding, and I see no delete.

https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc#newcode27
content/browser/keyboard_lock/keyboard_lock_service_impl.cc:27: //
TODO(zijiehe): Implementation required.
For IPC reviews, it's generally beneficial to include the actual service
implementation--otherwise, it's hard to evaluate for the IPC security
review. Is there any possibility of doing that here?

https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboard_lock/keyboard_lock_service_impl.h
File content/browser/keyboard_lock/keyboard_lock_service_impl.h (right):

https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboard_lock/keyboard_lock_service_impl.h#newcode25
content/browser/keyboard_lock/keyboard_lock_service_impl.h:25: void
RequestKeyLock(const std::vector<std::string>& key_codes,
Nit: Add a comment like

// blink::mojom::KeyboardLockService overrides:

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
(right):

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode52
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:52:
if (!EnsureServiceConnected(&error_message)) {
The only possible failure is if the frame is detached. That check should
probably just be a separate helper, e.g.

if (RejectPromiseIfDetached(state))
return;

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode91
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:91:
if (!service_.get()) {
This block is unnecessary.

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom
File
third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom
(right):

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom#newcode14

third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom:14:
// The reason will only be provided if the request is rejected.

Is it necessary to return a freeform string here? Maybe the return value
should actually be a status enum?

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom#newcode19
third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom:19:
// callback indicates the finish of the processing only.
There's no callback -- is this comment correct?

https://codereview.chromium.org/2805763004/

[zij...@chromium.org]

https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc
File content/browser/keyboard_lock/keyboard_lock_service_impl.cc
(right):

https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc#newcode13
content/browser/keyboard_lock/keyboard_lock_service_impl.cc:13: :
binding_(this, std::move(request)) {}

On 2017/04/26 13:27:17, dcheng (OOO through May 2) wrote:
> Nit: #include <utility>

Done.

https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc#newcode21
content/browser/keyboard_lock/keyboard_lock_service_impl.cc:21: new
KeyboardLockServiceImpl(std::move(request));

On 2017/04/26 13:27:17, dcheng (OOO through May 2) wrote:
> Is this leaked? The binding isn't a strong binding, and I see no
delete.

I do not think so: the object is owned by binding_.

https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc#newcode27
content/browser/keyboard_lock/keyboard_lock_service_impl.cc:27: //
TODO(zijiehe): Implementation required.

On 2017/04/26 13:27:17, dcheng (OOO through May 2) wrote:
> For IPC reviews, it's generally beneficial to include the actual
service
> implementation--otherwise, it's hard to evaluate for the IPC security
review. Is
> there any possibility of doing that here?

I'd only say we still have bunch of changes needed to support this
callback, which is not likely to be able to be done in this change.

https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboard_lock/keyboard_lock_service_impl.h
File content/browser/keyboard_lock/keyboard_lock_service_impl.h (right):

https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboard_lock/keyboard_lock_service_impl.h#newcode25
content/browser/keyboard_lock/keyboard_lock_service_impl.h:25: void
RequestKeyLock(const std::vector<std::string>& key_codes,

On 2017/04/26 13:27:17, dcheng (OOO through May 2) wrote:
> Nit: Add a comment like
>
> // blink::mojom::KeyboardLockService overrides:

Done.

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
(right):

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode52
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:52:
if (!EnsureServiceConnected(&error_message)) {

On 2017/04/26 13:27:17, dcheng (OOO through May 2) wrote:
> The only possible failure is if the frame is detached. That check
should
> probably just be a separate helper, e.g.
>
> if (RejectPromiseIfDetached(state))
> return;

This comment is conflict with what Kentaro suggested.
I tend to agree if GetInterface() won't return nullptr, we should inline
the EnsureServiceConnected() function directly into requestKeyLock() and
cancelKeyLock() functions. But before I had a chance to talk with Daniel
directly, I would prefer to keep it as this to avoid any troubles caused
by misunderstanding.

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode79
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:79:
bool NavigatorKeyboardLock::EnsureServiceConnected(String*
error_message) {

On 2017/04/26 06:56:03, haraken wrote:
>
> I'd inline this method into the caller sites because once you want to
handle the
> exception properly, you anyway need to inline the logic in the caller
sites
> (e.g., requestKeyLock() rejects promises, cancelKeyLock() returns with
silence
> etc).
>

Please see my reply in Daniel's comment. I would keep this function
as-is for now, and talk with Daniel about the GetInterface() function
once he returned.

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode91
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:91:
if (!service_.get()) {

On 2017/04/26 13:27:17, dcheng (OOO through May 2) wrote:
> This block is unnecessary.

Since Daniel is OoO, and my understanding is GetInterface may still
return an empty service pointer, I would keep this check to make it
safer. We can update this logic once I talked with Daniel about the
detail.

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h
(right):

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h#newcode47
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:47:
// the web page is closed, the user agent implictly executes this API.

On 2017/04/26 06:56:03, haraken wrote:
>
> implicitly

Done.

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom
File
third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom
(right):

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom#newcode14
third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom:14:
// The reason will only be provided if the request is rejected.

On 2017/04/26 13:27:18, dcheng (OOO through May 2) wrote:
> Is it necessary to return a freeform string here? Maybe the return
value should
> actually be a status enum?

If we are only talking about the Chrome implementation, this function
will never false and the reason. So the design of the return values is
to ensure we are following the spec only.
I will add a comment here to ensure it has been updated once the spec
defines the error types.

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom#newcode19
third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom:19:
// callback indicates the finish of the processing only.

On 2017/04/26 13:27:18, dcheng (OOO through May 2) wrote:
> There's no callback -- is this comment correct?

Sorry, I forget to update the comment.

https://codereview.chromium.org/2805763004/

[dch...@chromium.org]

https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc
File content/browser/keyboard_lock/keyboard_lock_service_impl.cc
(right):

https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc#newcode21
content/browser/keyboard_lock/keyboard_lock_service_impl.cc:21: new
KeyboardLockServiceImpl(std::move(request));

On 2017/04/26 22:05:56, Hzj_jie wrote:
> On 2017/04/26 13:27:17, dcheng (OOO through May 2) wrote:
> > Is this leaked? The binding isn't a strong binding, and I see no
delete.
>
> I do not think so: the object is owned by binding_.

Talked offline and we agree that this needs to be cleaned up.

I would just recommend using mojo::MakeStrongBinding() for now; we can
change it later if it doesn't make sense as the implementation is filled
out.

https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc#newcode27
content/browser/keyboard_lock/keyboard_lock_service_impl.cc:27: //
TODO(zijiehe): Implementation required.

On 2017/04/26 22:05:56, Hzj_jie wrote:
> On 2017/04/26 13:27:17, dcheng (OOO through May 2) wrote:
> > For IPC reviews, it's generally beneficial to include the actual
service
> > implementation--otherwise, it's hard to evaluate for the IPC
security review.
> Is
> > there any possibility of doing that here?
>
> I'd only say we still have bunch of changes needed to support this
callback,
> which is not likely to be able to be done in this change.

Please include an IPC security reviewer on the followup CLs then.

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
(right):

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode52
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:52:
if (!EnsureServiceConnected(&error_message)) {

On 2017/04/26 22:05:56, Hzj_jie wrote:
> On 2017/04/26 13:27:17, dcheng (OOO through May 2) wrote:
> > The only possible failure is if the frame is detached. That check
should
> > probably just be a separate helper, e.g.
> >
> > if (RejectPromiseIfDetached(state))
> > return;
>
> This comment is conflict with what Kentaro suggested.
> I tend to agree if GetInterface() won't return nullptr, we should
inline the
> EnsureServiceConnected() function directly into requestKeyLock() and
> cancelKeyLock() functions. But before I had a chance to talk with
Daniel
> directly, I would prefer to keep it as this to avoid any troubles
caused by
> misunderstanding.

Oh I see; I missed that there's no promise being rejected for
cancelKeyLock. I agree with haraken@ then.

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode91
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:91:
if (!service_.get()) {

On 2017/04/26 22:05:56, Hzj_jie wrote:
> On 2017/04/26 13:27:17, dcheng (OOO through May 2) wrote:
> > This block is unnecessary.
>
> Since Daniel is OoO, and my understanding is GetInterface may still
return an
> empty service pointer, I would keep this check to make it safer. We
can update
> this logic once I talked with Daniel about the detail.

As discussed offline, this should work fine assuming the static
configuration is done correctly (i.e. this should be a DCHECK if it's
checked at all). It seems there might be an issue with the manifest
config still though, so let's see what the trybots say.

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom
File
third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom
(right):

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom#newcode14
third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom:14:
// The reason will only be provided if the request is rejected.

On 2017/04/26 22:05:56, Hzj_jie wrote:
> On 2017/04/26 13:27:18, dcheng (OOO through May 2) wrote:
> > Is it necessary to return a freeform string here? Maybe the return
value
> should
> > actually be a status enum?
>
> If we are only talking about the Chrome implementation, this function
will never
> false and the reason. So the design of the return values is to ensure
we are
> following the spec only.
> I will add a comment here to ensure it has been updated once the spec
defines
> the error types.

I think this shouldn't stop us from using an enum for now:
- We can define an enum for the return types (even if the Chrome
implementation currently can't fail, I imagine it can in the future).
Right now, it could just be SUCCESS and FAILED as the two entries.
- The mojo method itself should just return an enum status
- The mapping of status code -> string should happen in the browser. See
WebBluetooth for an example.

https://codereview.chromium.org/2805763004/

[zij...@chromium.org]

Though (some of ) the trybots are down, the latest change passes all the tests.

https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc
File content/browser/keyboard_lock/keyboard_lock_service_impl.cc
(right):

https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc#newcode21
content/browser/keyboard_lock/keyboard_lock_service_impl.cc:21: new
KeyboardLockServiceImpl(std::move(request));

On 2017/04/27 02:53:10, dcheng (OOO through May 2) wrote:
> On 2017/04/26 22:05:56, Hzj_jie wrote:
> > On 2017/04/26 13:27:17, dcheng (OOO through May 2) wrote:
> > > Is this leaked? The binding isn't a strong binding, and I see no
delete.
> >
> > I do not think so: the object is owned by binding_.
>
> Talked offline and we agree that this needs to be cleaned up.
>
> I would just recommend using mojo::MakeStrongBinding() for now; we can
change it
> later if it doesn't make sense as the implementation is filled out.

Done.

https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc#newcode27
content/browser/keyboard_lock/keyboard_lock_service_impl.cc:27: //
TODO(zijiehe): Implementation required.

On 2017/04/27 02:53:10, dcheng (OOO through May 2) wrote:
> On 2017/04/26 22:05:56, Hzj_jie wrote:
> > On 2017/04/26 13:27:17, dcheng (OOO through May 2) wrote:
> > > For IPC reviews, it's generally beneficial to include the actual
service
> > > implementation--otherwise, it's hard to evaluate for the IPC
security
> review.
> > Is
> > > there any possibility of doing that here?
> >
> > I'd only say we still have bunch of changes needed to support this
callback,
> > which is not likely to be able to be done in this change.
>
> Please include an IPC security reviewer on the followup CLs then.

Done.

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
(right):

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode52
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:52:
if (!EnsureServiceConnected(&error_message)) {

On 2017/04/27 02:53:10, dcheng (OOO through May 2) wrote:
> On 2017/04/26 22:05:56, Hzj_jie wrote:
> > On 2017/04/26 13:27:17, dcheng (OOO through May 2) wrote:
> > > The only possible failure is if the frame is detached. That check
should
> > > probably just be a separate helper, e.g.
> > >
> > > if (RejectPromiseIfDetached(state))
> > > return;
> >
> > This comment is conflict with what Kentaro suggested.
> > I tend to agree if GetInterface() won't return nullptr, we should
inline the
> > EnsureServiceConnected() function directly into requestKeyLock() and
> > cancelKeyLock() functions. But before I had a chance to talk with
Daniel
> > directly, I would prefer to keep it as this to avoid any troubles
caused by
> > misunderstanding.
>
> Oh I see; I missed that there's no promise being rejected for
cancelKeyLock. I
> agree with haraken@ then.

Since this change has been lasted for almost one month already, I would
prefer to submit this one first, and do the following upgrade in a new
change.
1. Change the return type of IPC function RequestKeyLock() into an
enumeration.
2. Remove the NavigatorKeyboardLock::EnsureServiceConnected() function.
3. Store the pending operations in NavigatorKeyboardLock, and retry
after connection has been generated.

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode91
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:91:
if (!service_.get()) {

On 2017/04/27 02:53:10, dcheng (OOO through May 2) wrote:
> On 2017/04/26 22:05:56, Hzj_jie wrote:
> > On 2017/04/26 13:27:17, dcheng (OOO through May 2) wrote:
> > > This block is unnecessary.
> >
> > Since Daniel is OoO, and my understanding is GetInterface may still
return an
> > empty service pointer, I would keep this check to make it safer. We
can update
> > this logic once I talked with Daniel about the detail.
>
> As discussed offline, this should work fine assuming the static
configuration is
> done correctly (i.e. this should be a DCHECK if it's checked at all).
It seems
> there might be an issue with the manifest config still though, so
let's see what
> the trybots say.

Done.

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom
File
third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom
(right):

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom#newcode14
third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom:14:
// The reason will only be provided if the request is rejected.

This will be done in a new change immediately.

https://codereview.chromium.org/2805763004/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2805763004/760001

https://codereview.chromium.org/2805763004/

[dch...@chromium.org]

ipc lgtm with comments addressed

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
(right):

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode52
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:52:
if (!EnsureServiceConnected(&error_message)) {

I'm OK with deferring 1 and 3, but 2 is easy enough to do and contained
to this CL, so please address 2.

https://codereview.chromium.org/2805763004/diff/760001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc
File content/browser/keyboard_lock/keyboard_lock_service_impl.cc
(right):

https://codereview.chromium.org/2805763004/diff/760001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc#newcode21
content/browser/keyboard_lock/keyboard_lock_service_impl.cc:21:
RenderFrameHost* render_frame_host,
Nit: this appears to be unused, will it be used in future CLs? Maybe
remove it from this CL and add it when it's actually used

https://codereview.chromium.org/2805763004/diff/760001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
(right):

https://codereview.chromium.org/2805763004/diff/760001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode91
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:91:
DCHECK(service_.get());
Nit: omit .get()

https://codereview.chromium.org/2805763004/

[zij...@chromium.org]

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
(right):

https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode52
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:52:
if (!EnsureServiceConnected(&error_message)) {

Done.

https://codereview.chromium.org/2805763004/diff/760001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc
File content/browser/keyboard_lock/keyboard_lock_service_impl.cc
(right):

https://codereview.chromium.org/2805763004/diff/760001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc#newcode21
content/browser/keyboard_lock/keyboard_lock_service_impl.cc:21:
RenderFrameHost* render_frame_host,

On 2017/04/28 02:24:49, dcheng (OOO through May 2) wrote:
> Nit: this appears to be unused, will it be used in future CLs? Maybe
remove it
> from this CL and add it when it's actually used

I believe this is not useful at all. (The keyboard lock host is a
singleton object, which cannot be referred from RenderFrameHost.)

https://codereview.chromium.org/2805763004/diff/760001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
(right):

https://codereview.chromium.org/2805763004/diff/760001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode91
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:91:
DCHECK(service_.get());

On 2017/04/28 02:24:49, dcheng (OOO through May 2) wrote:
> Nit: omit .get()

Done.

https://codereview.chromium.org/2805763004/

[dch...@chromium.org]

https://codereview.chromium.org/2805763004/diff/780001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
(right):

https://codereview.chromium.org/2805763004/diff/780001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode55
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:55:
// not connected.
This isn't a recoverable error -- once frame is null, it will always be
null.

IMO we should keep the:

if (!frame)
return false;
frame->GetInterfaceProvider()->GetInterface(...);
return true;

part in a helper. Then requestKeyLock() will look like:

if (!EnsureServiceConnected()) {
return ScriptPromise::Reject(state, ...);
}

and cancelKeyLock() will look like:

if (!EnsureServiceConnected())
return;

https://codereview.chromium.org/2805763004/

[zij...@chromium.org]

https://codereview.chromium.org/2805763004/diff/780001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
(right):

https://codereview.chromium.org/2805763004/diff/780001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode55
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:55:
// not connected.

On 2017/04/28 15:33:45, dcheng (OOO through May 2) wrote:
> This isn't a recoverable error -- once frame is null, it will always
be null.
>
> IMO we should keep the:
>
> if (!frame)
> return false;
> frame->GetInterfaceProvider()->GetInterface(...);
> return true;
>
> part in a helper. Then requestKeyLock() will look like:
>
> if (!EnsureServiceConnected()) {
> return ScriptPromise::Reject(state, ...);
> }
>
> and cancelKeyLock() will look like:
>
> if (!EnsureServiceConnected())
> return;

I do not really understand the insight: why would frame be null? I used
to believe it happens once the tab itself is detaching, but it looks
like my understanding is totally wrong.

https://codereview.chromium.org/2805763004/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch.

Follow status at:

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2805763004/820001

https://codereview.chromium.org/2805763004/

[commit-bot@chromium.org via codereview.chromium.org]

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #27 (id:820001) as
https://chromium.googlesource.com/chromium/src/+/6016778c99f93e8f91eec1d1f20fc44642d3bfde

https://codereview.chromium.org/2805763004/

[dch...@chromium.org]

Basically, frame will be null if the Document is not the active Document in a
browsing context. A browsing context can be top-level (like the main frame in a
tab) or it can be in a subframe (which can be detached by removing the iframe
element from the DOM). Once a Document is detached, it can never be reattached
to the DOM. For example, if you have an iframe and remove it from the DOM, and
then re-insert it, you get a new Frame / Document / Navigator / etc. I'm happy
to explain more when I'm back in the office, as frame/document lifetimes are
pretty complex.

https://codereview.chromium.org/2805763004/

[zij...@chromium.org]

Got you. We may talk about the detail later: I am curious whether the lifetime
of an iframe is also trackable, if it becomes complex, we may simply disable
this feature in iframe. (For security concern, I believe it's also a reasonable
choice.)

https://codereview.chromium.org/2805763004/