Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2805763004:
[System-Keyboard-Lock] Forward navigator functions to RenderFrameHost (Closed)
|
Created:
3 years, 8 months ago by Hzj_jie Modified:
3 years, 7 months ago CC:
Aaron Boodman, abarth-chromium, blink-reviews, blink-reviews-api_chromium.org, blink-reviews-frames_chromium.org, chromium-reviews, creis+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, dglazkov+blink, jam, kinuko+watch, Łukasz Anforowicz, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, Jamie, garykac Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
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
Review-Url: https://codereview.chromium.org/2805763004
Cr-Commit-Position: refs/heads/master@{#468158}
Committed: https://chromium.googlesource.com/chromium/src/+/6016778c99f93e8f91eec1d1f20fc44642d3bfde
Patch Set 1 #
Total comments: 2
Patch Set 2 : Resolve review comments #
Total comments: 22
Patch Set 3 : Resolve review comments - Without removing deprecated logic #Patch Set 4 : Remove useless files #Patch Set 5 : Add external/wpt tests #
Total comments: 34
Patch Set 6 : Resolve review comments #
Total comments: 2
Patch Set 7 : Resolve review comments #
Total comments: 10
Patch Set 8 : Resolve review comments #Patch Set 9 : Synchronize latest changes #
Total comments: 75
Patch Set 10 : Resolve review comments #
Total comments: 2
Patch Set 11 : Sync latest changes #
Total comments: 7
Patch Set 12 : Resolve review comments #
Total comments: 15
Patch Set 13 : Resolve review comments #Patch Set 14 : Missing comments #Patch Set 15 : Resolve review comments #Patch Set 16 : Fix media-capabilities idlharness failure as well #Patch Set 17 : Sync latest changes to resolve broken merge #Patch Set 18 : Resolve review comments #
Total comments: 2
Patch Set 19 : Resolve review comments #
Total comments: 32
Patch Set 20 : Resolve review comments and update the location of the spec #Patch Set 21 : Sync latest changes #Patch Set 22 : Add manifest #Patch Set 23 : Resolve more review comments #Patch Set 24 : Update manifest #
Total comments: 4
Patch Set 25 : Resolve review comments #
Total comments: 2
Patch Set 26 : Resolve review comments #Patch Set 27 : Sync latest changes #Messages
Total messages: 270 (214 generated)
Description was changed from ========== [System-Keyboard-Lock] BUG=680809 ========== to ========== [System-Keyboard-Lock] BUG=680809 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== [System-Keyboard-Lock] BUG=680809 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== [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/1T9gJHYdA1VGZ6QHQeOu0hOacWWWJ7yNEt1VDbLju4... 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/lf... BUG=680809 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #1 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:80001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:120001) has been deleted
Patchset #1 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:200001) has been deleted
Patchset #3 (id:180001) has been deleted
Patchset #2 (id:160001) has been deleted
Patchset #1 (id:140001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:220001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
avayvod@chromium.org changed reviewers: + avayvod@chromium.org
https://codereview.chromium.org/2805763004/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/Navigator.idl (right): https://codereview.chromium.org/2805763004/diff/240001/third_party/WebKit/Sou... 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).
On 2017/04/10 04:32:45, whywhat wrote: > https://codereview.chromium.org/2805763004/diff/240001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/frame/Navigator.idl (right): > > https://codereview.chromium.org/2805763004/diff/240001/third_party/WebKit/Sou... > 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?
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/Sou... > > File third_party/WebKit/Source/core/frame/Navigator.idl (right): > > > > https://codereview.chromium.org/2805763004/diff/240001/third_party/WebKit/Sou... > > 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.
avayvod@chromium.org changed reviewers: + garykac@chromium.org
On 2017/04/10 21:22:14, whywhat wrote: > 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/Sou... > > > File third_party/WebKit/Source/core/frame/Navigator.idl (right): > > > > > > > https://codereview.chromium.org/2805763004/diff/240001/third_party/WebKit/Sou... > > > 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. 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.
On 2017/04/10 at 21:34:38, zijiehe wrote: > On 2017/04/10 21:22:14, whywhat wrote: > > 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/Sou... > > > > File third_party/WebKit/Source/core/frame/Navigator.idl (right): > > > > > > > > > > https://codereview.chromium.org/2805763004/diff/240001/third_party/WebKit/Sou... > > > > 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. > > 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. 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
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2805763004/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/Navigator.idl (right): https://codereview.chromium.org/2805763004/diff/240001/third_party/WebKit/Sou... 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.
Could the tests be layout tests? https://chromium.googlesource.com/chromium/src/+/master/docs/testing/layout_t... If so, this is how we would typically test web-exposed behaviors. Ideally, they would be web-platform-tests.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
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/keyboa... File content/browser/keyboard_lock/keyboard_lock_browsertest.cc (right): https://codereview.chromium.org/2805763004/diff/260001/content/browser/keyboa... 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.m... File content/common/frame.mojom (right): https://codereview.chromium.org/2805763004/diff/260001/content/common/frame.m... 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.m... 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/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2805763004/diff/260001/content/renderer/rende... 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/keyb... File content/test/data/keyboard_lock/cancel_keylock.html (right): https://codereview.chromium.org/2805763004/diff/260001/content/test/data/keyb... 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/keyb... File content/test/data/keyboard_lock/cancel_keylock.js (right): https://codereview.chromium.org/2805763004/diff/260001/content/test/data/keyb... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/BUILD.gn (right): https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/Sou... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h (right): https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/Sou... 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/medias...) https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebKeyLockRequestCompletion.h (right): https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/pub... 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/pub... 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/WebCa... One example would be in the Presentation API implementation: https://cs.chromium.org/webrtc/src/third_party/WebKit/Source/modules/presenta... https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebRuntimeFeatures.h (right): https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/pub... 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).
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2805763004/diff/260001/content/browser/keyboa... File content/browser/keyboard_lock/keyboard_lock_browsertest.cc (right): https://codereview.chromium.org/2805763004/diff/260001/content/browser/keyboa... 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.m... File content/common/frame.mojom (right): https://codereview.chromium.org/2805763004/diff/260001/content/common/frame.m... 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.m... 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/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2805763004/diff/260001/content/renderer/rende... 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/keyb... File content/test/data/keyboard_lock/cancel_keylock.html (right): https://codereview.chromium.org/2805763004/diff/260001/content/test/data/keyb... 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/keyb... File content/test/data/keyboard_lock/cancel_keylock.js (right): https://codereview.chromium.org/2805763004/diff/260001/content/test/data/keyb... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/BUILD.gn (right): https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/Sou... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h (right): https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/Sou... 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/medias...) 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/pub... File third_party/WebKit/public/web/WebKeyLockRequestCompletion.h (right): https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/pub... 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/pub... 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/WebCa... > One example would be in the Presentation API implementation: > https://cs.chromium.org/webrtc/src/third_party/WebKit/Source/modules/presenta... Done. https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebRuntimeFeatures.h (right): https://codereview.chromium.org/2805763004/diff/260001/third_party/WebKit/pub... 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
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_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2805763004/diff/320001/content/browser/frame_... 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/keyboa... File content/browser/keyboard_lock/keyboard_lock_browsertest.cc (right): https://codereview.chromium.org/2805763004/diff/320001/content/browser/keyboa... 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/keyboa... 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.m... File content/common/frame.mojom (right): https://codereview.chromium.org/2805763004/diff/320001/content/common/frame.m... content/common/frame.mojom:7: import "mojo/common/string16.mojom"; nit: remove? https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/VirtualTestSuites (right): https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Lay... 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/Lay... File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html (right): https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Lay... 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/Lay... 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/rem... If you verified the test passes and fails correctly locally if you pretend to reject and resolve in your implementation, then it's okay to leave as is :) Also, if your implementation does return the rejection reason, you should probably verify the reason value (e.g. DOMException type and so on). https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Sou... 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/Sou... third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:89: if (allowed) { nit: ditto https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Sou... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h (right): https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Sou... 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/Sou... 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/Sou... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl (right): https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Sou... 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/pub... File third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom (right): https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/pub... 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?
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #6 (id:340001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2805763004/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/360001/third_party/WebKit/Sou... 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.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2805763004/diff/320001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2805763004/diff/320001/content/browser/frame_... 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/keyboa... File content/browser/keyboard_lock/keyboard_lock_browsertest.cc (right): https://codereview.chromium.org/2805763004/diff/320001/content/browser/keyboa... 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/keyboa... 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.m... File content/common/frame.mojom (right): https://codereview.chromium.org/2805763004/diff/320001/content/common/frame.m... 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/Lay... File third_party/WebKit/LayoutTests/VirtualTestSuites (right): https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Lay... 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/Lay... File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html (right): https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Lay... 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/Lay... 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/rem... > > If you verified the test passes and fails correctly locally if you pretend to > reject and resolve in your implementation, then it's okay to leave as is :) > > Also, if your implementation does return the rejection reason, you should > probably verify the reason value (e.g. DOMException type and so on). Yes, I was a little bit confusing about the async_test and promise_test, but the example of promise_test (https://cs.chromium.org/chromium/src/docs/testing/writing_layout_tests.md?typ...) shows it accepts a Promise object. And indeed a promise_test is an async_test (https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources...). As long as the test passes, I think current solution seems clean and readable. The spec has not actively talked about the reason we should return from the rejected Promise. I have updated the implementation in browser layer to return a true instead of false with "Unsupported" to avoid the misleading information from this change. P.S. I tried to use internals, but the logic in renderer process (NavigatorKeyboardLock) is too simple (just forwarding the requests), mocking its behavior does not really help to test its correctness. https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Sou... 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/Sou... 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/Sou... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h (right): https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Sou... 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/Sou... 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/Sou... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl (right): https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Sou... 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/pub... File third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom (right): https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/pub... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/360001/third_party/WebKit/Sou... 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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.
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.
avayvod@chromium.org changed reviewers: - garykac@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/keyboa... File content/browser/keyboard_lock/keyboard_lock_browsertest.cc (right): https://codereview.chromium.org/2805763004/diff/320001/content/browser/keyboa... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl (right): https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Sou... 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/Lay... File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html (right): https://codereview.chromium.org/2805763004/diff/380001/third_party/WebKit/Lay... 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/Lay... 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/Lay... 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/Lay... 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.');
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
zijiehe@chromium.org changed reviewers: + avi@chromium.org, foolip@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/keyboa... File content/browser/keyboard_lock/keyboard_lock_browsertest.cc (right): https://codereview.chromium.org/2805763004/diff/320001/content/browser/keyboa... 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: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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl (right): https://codereview.chromium.org/2805763004/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl:5: partial interface Navigator { 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/Lay... File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html (right): https://codereview.chromium.org/2805763004/diff/380001/third_party/WebKit/Lay... 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/Lay... 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/Lay... third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html:20: promise_rejects(t, null, p2, On 2017/04/13 02:34:36, whywhat wrote: > I tried your cl locally. If I change the NavigatorKeyboardLock.cpp like below, > the test still passes: > > index 4069544f2637..2c22ca763590 100644 > --- a/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp > +++ b/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp > @@ -41,9 +41,10 @@ ScriptPromise NavigatorKeyboardLock::requestKeyLock( > const Vector<String>& keycodes) { > DCHECK(state); > if (request_keylock_resolver_) { > - return ScriptPromise::Reject( > - state, V8String(state->GetIsolate(), > - "Last requestKeyLock() has not finished yet.")); > + request_keylock_resolver_->Resolve(); > + // return ScriptPromise::Reject( > + // state, V8String(state->GetIsolate(), > + // "Last requestKeyLock() has not finished yet.")); > } > > if (!service_) { > @@ -89,6 +90,7 @@ void NavigatorKeyboardLock::cancelKeyLock(Navigator& > navigator) { > > void NavigatorKeyboardLock::LockRequestFinished(bool allowed, > const String& reason) { > + if (!request_keylock_resolver_) return; > DCHECK(request_keylock_resolver_); > if (allowed) > request_keylock_resolver_->Resolve(); > > Note that for promise_test() you return one promise and the test will pass if > this promise fulfills and fail if it rejects. Once it's settled, no further > callback code will have effect (as test.done() is called) so calling > promise_rejects from there doesn't have any effect AFAIK. > > I think you could just return promise_rejects(): > > promise_test((t) => { > const p1 = navigator.requestKeyLock(['a', 'b']); > const p2 = navigator.requestKeyLock(['c', 'd']); > return promise_rejects(t, null, p2); > }, 'Test that calling requestKeyLock() rejects before the previous promise was > settled'); > > However, as is, the next test would fail because it may be run before p1 is > resolved so its promise will get rejected too. > Having each test in a separate file fixes that. This is out of my expectation: Promise.then() returns a new Promise, i.e. "return p1.then(() => ...)" returns a new Promise instance other than p1 which can only be resolved once the then() is executed. Maybe I should also "return promise_rejects(t, null, p2, ...);" from "p1.then(() => ...)" to ensure the "then" Promise won't be resolved until the "promise_rejects" Promise is resolved. And yes, you are correct, I wrote it like this to ensure both Promises p1 and p2 would be resolved before next test is being executed. But since your solution is pretty simple and clear, I am happy to use it. https://codereview.chromium.org/2805763004/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html:37: return navigator.requestKeyLock(['a', 'b']) On 2017/04/13 02:34:36, whywhat wrote: > This test still passes if I comment all the rest out and modify the > implementation like this: > > index 4069544f2637..fe74ebfba76c 100644 > --- a/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp > +++ b/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp > @@ -94,7 +94,7 @@ void NavigatorKeyboardLock::LockRequestFinished(bool allowed, > request_keylock_resolver_->Resolve(); > else > request_keylock_resolver_->Reject(reason); > - request_keylock_resolver_ = nullptr; > +// request_keylock_resolver_ = nullptr; > } > > // static > > For similar reasons - as long as the first promise resolves, promise_test() > marks the test as done and catch for the second promise doesn't fail the test. > > This should be an async_test that can be written like this (note t.step_func() > usage as it's not a promise_test()): > > async_test((t) => { > navigator.requestKeyLock([]).then(t.step_func(() => { > navigator.requestKeyLock([]) > .then(t.step_func_done()) > .catch(t.unreached_func()); > })) > .catch(t.unreached_func()); > }, 'Tests that second requestKeyLock succeeds once the promise returned by the > first one is fulfilled.'); After some investigations, the behavior of promise_test() is explainable. As long as we return a Promise in the then() function, it won't be ignored. But if nothing is returned from then(), current Promise is resolved by design. I have applied your change to comment out request_keylock_resolver_ = nullptr line, and change the sequential test into return navigator.requestKeyLock(['a', 'b']) .then(() => { - navigator.requestKeyLock(['c', 'd']) + return navigator.requestKeyLock(['c', 'd']) .then(() => {}) .catch((reason) => { the test fails as expected. This document https://developers.google.com/web/fundamentals/getting-started/primers/promises explains the behavior of chaining promises, which can apply here.
non-owners lgtm https://codereview.chromium.org/2805763004/diff/380001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html (right): https://codereview.chromium.org/2805763004/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html:20: promise_rejects(t, null, p2, On 2017/04/13 at 22:24:25, Hzj_jie wrote: > On 2017/04/13 02:34:36, whywhat wrote: > > I tried your cl locally. If I change the NavigatorKeyboardLock.cpp like below, > > the test still passes: > > > > index 4069544f2637..2c22ca763590 100644 > > --- a/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp > > +++ b/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp > > @@ -41,9 +41,10 @@ ScriptPromise NavigatorKeyboardLock::requestKeyLock( > > const Vector<String>& keycodes) { > > DCHECK(state); > > if (request_keylock_resolver_) { > > - return ScriptPromise::Reject( > > - state, V8String(state->GetIsolate(), > > - "Last requestKeyLock() has not finished yet.")); > > + request_keylock_resolver_->Resolve(); > > + // return ScriptPromise::Reject( > > + // state, V8String(state->GetIsolate(), > > + // "Last requestKeyLock() has not finished yet.")); > > } > > > > if (!service_) { > > @@ -89,6 +90,7 @@ void NavigatorKeyboardLock::cancelKeyLock(Navigator& > > navigator) { > > > > void NavigatorKeyboardLock::LockRequestFinished(bool allowed, > > const String& reason) { > > + if (!request_keylock_resolver_) return; > > DCHECK(request_keylock_resolver_); > > if (allowed) > > request_keylock_resolver_->Resolve(); > > > > Note that for promise_test() you return one promise and the test will pass if > > this promise fulfills and fail if it rejects. Once it's settled, no further > > callback code will have effect (as test.done() is called) so calling > > promise_rejects from there doesn't have any effect AFAIK. > > > > I think you could just return promise_rejects(): > > > > promise_test((t) => { > > const p1 = navigator.requestKeyLock(['a', 'b']); > > const p2 = navigator.requestKeyLock(['c', 'd']); > > return promise_rejects(t, null, p2); > > }, 'Test that calling requestKeyLock() rejects before the previous promise was > > settled'); > > > > However, as is, the next test would fail because it may be run before p1 is > > resolved so its promise will get rejected too. > > Having each test in a separate file fixes that. > > This is out of my expectation: Promise.then() returns a new Promise, i.e. "return p1.then(() => ...)" returns a new Promise instance other than p1 which can only be resolved once the then() is executed. Maybe I should also "return promise_rejects(t, null, p2, ...);" from "p1.then(() => ...)" to ensure the "then" Promise won't be resolved until the "promise_rejects" Promise is resolved. > > And yes, you are correct, I wrote it like this to ensure both Promises p1 and p2 would be resolved before next test is being executed. But since your solution is pretty simple and clear, I am happy to use it. Ack. Maybe returning promise_rejects() is all it takes to fix your test then. https://codereview.chromium.org/2805763004/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html:37: return navigator.requestKeyLock(['a', 'b']) On 2017/04/13 at 22:24:25, Hzj_jie wrote: > On 2017/04/13 02:34:36, whywhat wrote: > > This test still passes if I comment all the rest out and modify the > > implementation like this: > > > > index 4069544f2637..fe74ebfba76c 100644 > > --- a/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp > > +++ b/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp > > @@ -94,7 +94,7 @@ void NavigatorKeyboardLock::LockRequestFinished(bool allowed, > > request_keylock_resolver_->Resolve(); > > else > > request_keylock_resolver_->Reject(reason); > > - request_keylock_resolver_ = nullptr; > > +// request_keylock_resolver_ = nullptr; > > } > > > > // static > > > > For similar reasons - as long as the first promise resolves, promise_test() > > marks the test as done and catch for the second promise doesn't fail the test. > > > > This should be an async_test that can be written like this (note t.step_func() > > usage as it's not a promise_test()): > > > > async_test((t) => { > > navigator.requestKeyLock([]).then(t.step_func(() => { > > navigator.requestKeyLock([]) > > .then(t.step_func_done()) > > .catch(t.unreached_func()); > > })) > > .catch(t.unreached_func()); > > }, 'Tests that second requestKeyLock succeeds once the promise returned by the > > first one is fulfilled.'); > > After some investigations, the behavior of promise_test() is explainable. As long as we return a Promise in the then() function, it won't be ignored. But if nothing is returned from then(), current Promise is resolved by design. > > I have applied your change to comment out request_keylock_resolver_ = nullptr line, and change the sequential test into > > return navigator.requestKeyLock(['a', 'b']) > .then(() => { > - navigator.requestKeyLock(['c', 'd']) > + return navigator.requestKeyLock(['c', 'd']) > .then(() => {}) > .catch((reason) => { > > the test fails as expected. > > This document https://developers.google.com/web/fundamentals/getting-started/primers/promises explains the behavior of chaining promises, which can apply here. Yeah, I was wondering that chaining should work. Glad it does!
zijiehe@chromium.org changed reviewers: + dcheng@chromium.org, esprehn@chromium.org, haraken@chromium.org, lukasza@chromium.org
content lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Which files should I look at? Too many reviewers to guess :)
avayvod@chromium.org changed reviewers: - esprehn@chromium.org, haraken@chromium.org, lukasza@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.
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.
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_iZbVTzyqY7LioqERU8DK3u3X... 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/Lay... File third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Lay... 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/Lay... File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Lay... 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-... https://github.com/w3c/web-platform-tests/blob/master/gamepad/idlharness.html https://github.com/w3c/web-platform-tests/blob/master/mediasession/idlharness... 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.htt... Because of [SecureContext], you will need to name it *.https.html https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Lay... 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/Lay... File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockCancel.html (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Lay... 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/Lay... 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/Lay... 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/Lay... File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockRequest.html (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Lay... 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/Lay... 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/Lay... 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/Lay... File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockTwoParallelRequests.html (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Lay... 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/Lay... File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockTwoSequentialRequests.html (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Lay... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Sou... 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/Sou... 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/Sou... 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/Sou... 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/Sou... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Sou... 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/Sou... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Sou... 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/Sou... 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/Sou... 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/Sou... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/OWNERS (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/keyboard_lock/OWNERS:1: zijiehe@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/pub... File third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/pub... 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.
haraken@chromium.org changed reviewers: + haraken@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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/OWNERS (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/keyboard_lock/OWNERS:1: zijiehe@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/pub... File third_party/WebKit/public/platform/modules/keyboard_lock/OWNERS (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/keyboard_lock/OWNERS:1: zijiehe@chromium.org Ditto.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2805763004/diff/420001/content/browser/keyboa... File content/browser/keyboard_lock/keyboard_lock_service_impl.cc (right): https://codereview.chromium.org/2805763004/diff/420001/content/browser/keyboa... 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/keyboa... 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/keyboa... File content/browser/keyboard_lock/keyboard_lock_service_impl.h (right): https://codereview.chromium.org/2805763004/diff/420001/content/browser/keyboa... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/440001/third_party/WebKit/Sou... 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.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #12 (id:480001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2805763004/diff/420001/content/browser/keyboa... File content/browser/keyboard_lock/keyboard_lock_service_impl.cc (right): https://codereview.chromium.org/2805763004/diff/420001/content/browser/keyboa... 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/keyboa... 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/keyboa... File content/browser/keyboard_lock/keyboard_lock_service_impl.h (right): https://codereview.chromium.org/2805763004/diff/420001/content/browser/keyboa... 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/Lay... File third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Lay... 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/Lay... File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Lay... 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-... > https://github.com/w3c/web-platform-tests/blob/master/gamepad/idlharness.html > https://github.com/w3c/web-platform-tests/blob/master/mediasession/idlharness... > 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.htt... > > Because of [SecureContext], you will need to name it *.https.html Done. https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Lay... 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/Lay... File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockCancel.html (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Lay... 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/Lay... 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/Lay... 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/Lay... File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockRequest.html (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Lay... 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/Lay... 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/Lay... 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/Lay... File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockTwoParallelRequests.html (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Lay... 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/Lay... File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/testKeyboardLockTwoSequentialRequests.html (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Lay... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Sou... 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/Sou... 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/Sou... 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/Sou... 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/Sou... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Sou... 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/Sou... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Sou... 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/Sou... 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/Sou... 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/Sou... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/OWNERS (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/keyboard_lock/OWNERS:1: zijiehe@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/Sou... third_party/WebKit/Source/modules/keyboard_lock/OWNERS:1: zijiehe@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/pub... File third_party/WebKit/public/platform/modules/keyboard_lock/OWNERS (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/keyboard_lock/OWNERS:1: zijiehe@chromium.org On 2017/04/17 14:15:57, haraken wrote: > > Ditto. Done. https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/pub... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/440001/third_party/WebKit/Sou... 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/Lay... File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html (right): https://codereview.chromium.org/2805763004/diff/460001/third_party/WebKit/Lay... 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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/Lay... File third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Lay... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Sou... 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/Sou... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Sou... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Sou... 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/Sou... 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/pub... File third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/pub... 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/Lay... File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html (right): https://codereview.chromium.org/2805763004/diff/460001/third_party/WebKit/Lay... 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/Lay... 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/Lay... File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.https.html (right): https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Lay... 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/Lay... File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html (right): https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Lay... 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/Lay... File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-cancelKeyLock.https.html (right): https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Lay... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h (right): https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Sou... 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/Sou... 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/Sou... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/OWNERS (right): https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/keyboard_lock/OWNERS:1: zijiehe@chromium.org How about garykac@chromium.org and jamiewalch@chromium.org? As editors of the spec, they should be the best reviewers of this code after you.
https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/keyboard_lock/OWNERS (right): https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/keyboard_lock/OWNERS:1: zijiehe@chromium.org On 2017/04/18 05:10:57, foolip OOO on Wednesday wrote: > How about mailto:garykac@chromium.org and mailto:jamiewalch@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.)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #13 (id:520001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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/Lay... File third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Lay... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Sou... 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: > 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. 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/Sou... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Sou... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Sou... 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/Sou... 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/Lay... File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html (right): https://codereview.chromium.org/2805763004/diff/460001/third_party/WebKit/Lay... 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/Lay... 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/Lay... File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-cancelKeyLock.https.html (right): https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Lay... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h (right): https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Sou... 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/Sou... 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/Sou... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/OWNERS (right): https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/keyboard_lock/OWNERS:1: zijiehe@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:garykac@chromium.org and mailto:jamiewalch@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.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.https.html
(right):
https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Lay...
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/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html
(right):
https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Lay...
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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:60: state, V8String(state->GetIsolate(), "Browser is not connecting.")); 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: > > > > 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. > > 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. 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Sou... 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/Lay... File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html (right): https://codereview.chromium.org/2805763004/diff/460001/third_party/WebKit/Lay... 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.
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/2f4bc7af7fbe4b74aec09310c028de...
Can you try playing with navigator.requestKeyLock.call(null, []) and similar to
see what's going on? Probably it's a real bug.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:60: state, V8String(state->GetIsolate(), "Browser is not connecting.")); On 2017/04/21 06:08:32, foolip_UTC7 wrote: > 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: > > > > > 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. > > > > 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. > > 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. 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h:24: : public GarbageCollectedFinalized<NavigatorKeyboardLock>, On 2017/04/21 06:08:32, foolip_UTC7 wrote: > 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. Done. https://codereview.chromium.org/2805763004/diff/460001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html (right): https://codereview.chromium.org/2805763004/diff/460001/third_party/WebKit/Lay... 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
Hi, Philip, is the fix in idlharness.js reasonable?
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.
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.
On 2017/04/24 19:08:34, Hzj_jie wrote: > 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. 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_plat... documents how it's imported and exported. It's supposed to be seamless so you don't have to care about the boundary :)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/24 19:18:54, foolip_UTC7 wrote: > On 2017/04/24 19:08:34, Hzj_jie wrote: > > 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. > > 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_plat... > documents how it's imported and exported. It's supposed to be seamless so you > don't have to care about the boundary :) Code has been updated.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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/Lay... File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-requestKeyLock.https.html (right): https://codereview.chromium.org/2805763004/diff/640001/third_party/WebKit/Lay... 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?
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
zijiehe@chromium.org changed reviewers: + esprehn@chromium.org
Since Daniel is OOO, adding Elliott to this change. https://codereview.chromium.org/2805763004/diff/640001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-requestKeyLock.https.html (right): https://codereview.chromium.org/2805763004/diff/640001/third_party/WebKit/Lay... 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
From the implementation perspective, WebKit LGTM. https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Sou... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h (right): https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Sou... 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/diff/660001/content/browser/keyboa... File content/browser/keyboard_lock/keyboard_lock_service_impl.cc (right): https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboa... 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/keyboa... 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/keyboa... 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/keyboa... File content/browser/keyboard_lock/keyboard_lock_service_impl.h (right): https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboa... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Sou... 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/Sou... 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/pub... File third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom (right): https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/pub... 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/pub... 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?
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboa... File content/browser/keyboard_lock/keyboard_lock_service_impl.cc (right): https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboa... 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/keyboa... 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/keyboa... 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/keyboa... File content/browser/keyboard_lock/keyboard_lock_service_impl.h (right): https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboa... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Sou... 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/Sou... 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/Sou... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h (right): https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Sou... 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/pub... File third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom (right): https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/pub... 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/pub... 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.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboa... File content/browser/keyboard_lock/keyboard_lock_service_impl.cc (right): https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboa... 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/keyboa... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Sou... 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/Sou... 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/pub... File third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom (right): https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/pub... 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.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Though (some of ) the trybots are down, the latest change passes all the tests. https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboa... File content/browser/keyboard_lock/keyboard_lock_service_impl.cc (right): https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboa... 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/keyboa... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Sou... 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/Sou... 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/pub... File third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom (right): https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/pub... 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/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: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. This will be done in a new change immediately.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org, avi@chromium.org, foolip@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2805763004/#ps760001 (title: "Update manifest")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ipc lgtm with comments addressed https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:52: if (!EnsureServiceConnected(&error_message)) { On 2017/04/28 00:10:31, Hzj_jie wrote: > 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. 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/keyboa... File content/browser/keyboard_lock/keyboard_lock_service_impl.cc (right): https://codereview.chromium.org/2805763004/diff/760001/content/browser/keyboa... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/760001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:91: DCHECK(service_.get()); Nit: omit .get()
The CQ bit was unchecked by zijiehe@chromium.org
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:52: if (!EnsureServiceConnected(&error_message)) { On 2017/04/28 02:24:49, dcheng (OOO through May 2) wrote: > On 2017/04/28 00:10:31, Hzj_jie wrote: > > 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. > > I'm OK with deferring 1 and 3, but 2 is easy enough to do and contained to this > CL, so please address 2. Done. https://codereview.chromium.org/2805763004/diff/760001/content/browser/keyboa... File content/browser/keyboard_lock/keyboard_lock_service_impl.cc (right): https://codereview.chromium.org/2805763004/diff/760001/content/browser/keyboa... 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/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/760001/third_party/WebKit/Sou... 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2805763004/diff/780001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/780001/third_party/WebKit/Sou... 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;
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2805763004/diff/780001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/780001/third_party/WebKit/Sou... 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, dcheng@chromium.org, foolip@chromium.org, haraken@chromium.org, avayvod@chromium.org Link to the patchset: https://codereview.chromium.org/2805763004/#ps820001 (title: "Sync latest changes")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by zijiehe@chromium.org
The CQ bit was checked by zijiehe@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 820001, "attempt_start_ts": 1493417458264280,
"parent_rev": "56fc9d639f67506841a0cb90a421a869e5107eea", "commit_rev":
"6016778c99f93e8f91eec1d1f20fc44642d3bfde"}
Message was sent while issue was closed.
Description was changed from ========== [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/1T9gJHYdA1VGZ6QHQeOu0hOacWWWJ7yNEt1VDbLju4... 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/lf... BUG=680809 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== [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/1T9gJHYdA1VGZ6QHQeOu0hOacWWWJ7yNEt1VDbLju4... 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/lf... BUG=680809 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2805763004 Cr-Commit-Position: refs/heads/master@{#468158} Committed: https://chromium.googlesource.com/chromium/src/+/6016778c99f93e8f91eec1d1f20f... ==========
Message was sent while issue was closed.
Committed patchset #27 (id:820001) as https://chromium.googlesource.com/chromium/src/+/6016778c99f93e8f91eec1d1f20f...
Message was sent while issue was closed.
On 2017/04/28 19:16:02, Hzj_jie wrote: > https://codereview.chromium.org/2805763004/diff/780001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp > (right): > > https://codereview.chromium.org/2805763004/diff/780001/third_party/WebKit/Sou... > 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. 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.
Message was sent while issue was closed.
On 2017/04/29 01:58:30, dcheng (OOO through May 2) wrote: > On 2017/04/28 19:16:02, Hzj_jie wrote: > > > https://codereview.chromium.org/2805763004/diff/780001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp > > (right): > > > > > https://codereview.chromium.org/2805763004/diff/780001/third_party/WebKit/Sou... > > 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. > > 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. 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.) |
