Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2815023002:
[System-Keyboard-Lock] Add KeyboardLockHost and KeyEventInterceptor (Closed)
Description[System-Keyboard-Lock] Add KeyboardLockHost and KeyEventFilter
This change starts KeyboardLock component, which is an object in browser process
(Browser object) to receive requestKeyboardLock() and cancelKeyboardLock()
requests from web pages, monitor web frame states, receive low level key events
and forward to corresponding web pages.
KeyEventFilter is an interface to receive low level key events, which is
used by low level key event callback on Windows and Mac OS, and
PlatformEventSource on Linux, to deliver the key events directly to the web
page.
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
A prototype can be found at,
https://codereview.chromium.org/2879033002
BUG=680809
Patch Set 1 #
Total comments: 10
Patch Set 2 : Resolve review comments #
Total comments: 4
Patch Set 3 : More comments #
Total comments: 16
Patch Set 4 : Resolve review comments #Patch Set 5 : Remove useless changes #Patch Set 6 : Use NativeKeycode instead of ui::KeyboardCode #
Messages
Total messages: 102 (79 generated)
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: This issue passed the CQ dry run.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) 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...
Description was changed from ========== Add KeyboardLockHost and KeyEventInterceptor BUG= ========== to ========== [System-Keyboard-Lock] Add KeyboardLockHost and KeyEventInterceptor This change starts KeyboardLock component, which is a singleton object in browser process to receive requestKeyLock() and cancelKeyLock() requests from web pages, monitor web frame states, receive low level key events and forward to corresponding web pages. KeyEventInterceptor is an interface to receive low level key events, which is used by low level key event callback on Windows and Mac OS, and PlatformEventSource on Linux, to deliver the key events directly to the web page. 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 ==========
zijiehe@chromium.org changed reviewers: + chongz@chromium.org, dtapuska@chromium.org
Since this change impacts ui/events, and it's also part of input pipeline, Dave and Chong, would you mind to have a look?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2017/04/12 22:23:12, Hzj_jie wrote: > Since this change impacts ui/events, and it's also part of input pipeline, Dave > and Chong, would you mind to have a look? Kindly ping ...
zijiehe@chromium.org changed reviewers: + sadrul@chromium.org
On 2017/04/13 20:38:50, Hzj_jie wrote: > On 2017/04/12 22:23:12, Hzj_jie wrote: > > Since this change impacts ui/events, and it's also part of input pipeline, > Dave > > and Chong, would you mind to have a look? > > Kindly ping ... Hi, Sadrul, Dave and Chong, Would you mind to have a quick look at the BUILD file changes under ui/event/? In System Keyboard Lock feature, we prefer to use ui::KeyboardCode to avoid platform dependency. But now it's placed in events_base target which is mostly not useful for the feature. Considering the ui::KeyboardCode is a set of constants, I have moved it to event_constants target, so targets under components/keyboard_lock can depend on a very minimum set of events logic. Thank you.
Patchset #1 (id:40001) has been deleted
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: This issue passed the CQ dry run.
zijiehe@chromium.org changed reviewers: + blundell@chromium.org, caitkp@chromium.org - chongz@chromium.org, dtapuska@chromium.org, sadrul@chromium.org
It looks like Dave, Chong and Sadrul are busy these days. Maybe we can start from the other half of the change. Colin and Cait, would you mind to have a look at the changes under components/ folder? Briefly, this change starts a new component called keyboard_lock, which is the main component to handle requests described in https://github.com/garykac/system-keyboard-lock.
On 2017/04/17 21:38:19, Hzj_jie wrote: > It looks like Dave, Chong and Sadrul are busy these days. Maybe we can start > from the other half of the change. > Colin and Cait, would you mind to have a look at the changes under components/ > folder? > > Briefly, this change starts a new component called keyboard_lock, which is the > main component to handle requests described in > https://github.com/garykac/system-keyboard-lock. Is there a reason that this needs to be in //components (as opposed to e.g. //chrome)?
dtapuska@chromium.org changed reviewers: + dtapuska@chromium.org
https://codereview.chromium.org/2815023002/diff/60001/components/keyboard_loc... File components/keyboard_lock/key_event_interceptor_installer.cc (right): https://codereview.chromium.org/2815023002/diff/60001/components/keyboard_loc... components/keyboard_lock/key_event_interceptor_installer.cc:17: host->SetKeyEventInterceptor(std::unique_ptr<KeyEventInterceptor>(this)); I'd think this is dangerous. What happens if the SetKeyEventInterceptor calls the virtual methods during the set? I personally prefer construction and registration to be separate. You could have a static method that Creates and Registers an instance but it isn't actually the ctor. https://codereview.chromium.org/2815023002/diff/60001/components/keyboard_loc... File components/keyboard_lock/key_event_interceptor_installer.h (right): https://codereview.chromium.org/2815023002/diff/60001/components/keyboard_loc... components/keyboard_lock/key_event_interceptor_installer.h:21: KeyEventInterceptorInstaller(KeyboardLockHost* host); Do you need a DISALLOW_COPY ? https://codereview.chromium.org/2815023002/diff/60001/components/keyboard_loc... File components/keyboard_lock/keyboard_lock_host.cc (right): https://codereview.chromium.org/2815023002/diff/60001/components/keyboard_loc... components/keyboard_lock/keyboard_lock_host.cc:41: base::AutoLock lock(lock_); Are there multiple threads running around that you'd need an interceptor for? If there are then the lifecycle of the returned pointer is bogus because it is stored via a unique_ptr. If there are multiple threads then the object should be refcounted. If there aren't then you don't need this lock. https://codereview.chromium.org/2815023002/diff/60001/components/keyboard_loc... File components/keyboard_lock/keyboard_lock_host.h (right): https://codereview.chromium.org/2815023002/diff/60001/components/keyboard_loc... components/keyboard_lock/keyboard_lock_host.h:35: KeyEventInterceptor* interceptor() const; Returning a non-const ptr from a const object is weird. I believe it is allowed but discouraged.
On 2017/04/18 13:31:38, blundell wrote: > On 2017/04/17 21:38:19, Hzj_jie wrote: > > It looks like Dave, Chong and Sadrul are busy these days. Maybe we can start > > from the other half of the change. > > Colin and Cait, would you mind to have a look at the changes under components/ > > folder? > > > > Briefly, this change starts a new component called keyboard_lock, which is the > > main component to handle requests described in > > https://github.com/garykac/system-keyboard-lock. > > Is there a reason that this needs to be in //components (as opposed to e.g. > //chrome)? I chose components because the /components/README states -code that is shared between multiple embedders of content (e.g., Android WebView and Chrome) This feature is targeting to support all platforms.
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-...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #2 (id:80001) 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_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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
Patchset #2 (id:100001) 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/2815023002/diff/60001/components/keyboard_loc... File components/keyboard_lock/key_event_interceptor_installer.cc (right): https://codereview.chromium.org/2815023002/diff/60001/components/keyboard_loc... components/keyboard_lock/key_event_interceptor_installer.cc:17: host->SetKeyEventInterceptor(std::unique_ptr<KeyEventInterceptor>(this)); On 2017/04/18 13:43:17, dtapuska wrote: > I'd think this is dangerous. What happens if the SetKeyEventInterceptor calls > the virtual methods during the set? I personally prefer construction and > registration to be separate. You could have a static method that Creates and > Registers an instance but it isn't actually the ctor. Done. Good point, I have not considered this possibility before: the SetKeyEventInterceptor() does not likely call any functions of KeyEventInterceptor. https://codereview.chromium.org/2815023002/diff/60001/components/keyboard_loc... File components/keyboard_lock/key_event_interceptor_installer.h (right): https://codereview.chromium.org/2815023002/diff/60001/components/keyboard_loc... components/keyboard_lock/key_event_interceptor_installer.h:21: KeyEventInterceptorInstaller(KeyboardLockHost* host); On 2017/04/18 13:43:17, dtapuska wrote: > Do you need a DISALLOW_COPY ? Done. https://codereview.chromium.org/2815023002/diff/60001/components/keyboard_loc... File components/keyboard_lock/keyboard_lock_host.cc (right): https://codereview.chromium.org/2815023002/diff/60001/components/keyboard_loc... components/keyboard_lock/keyboard_lock_host.cc:41: base::AutoLock lock(lock_); On 2017/04/18 13:43:17, dtapuska wrote: > Are there multiple threads running around that you'd need an interceptor for? If > there are then the lifecycle of the returned pointer is bogus because it is > stored via a unique_ptr. > > If there are multiple threads then the object should be refcounted. If there > aren't then you don't need this lock. Multiple threads are required: the interceptor will be accessed by native message loop by X11 or low level callbacks of key events on Windows / Mac OS, which are running on its own thread or unpredictable threads. I believe this behavior is safe, since the KeyboardLockHost as well as the interceptor are never destroyed because of the base::LeakySingletonTraits used above. https://codereview.chromium.org/2815023002/diff/60001/components/keyboard_loc... File components/keyboard_lock/keyboard_lock_host.h (right): https://codereview.chromium.org/2815023002/diff/60001/components/keyboard_loc... components/keyboard_lock/keyboard_lock_host.h:35: KeyEventInterceptor* interceptor() const; On 2017/04/18 13:43:17, dtapuska wrote: > Returning a non-const ptr from a const object is weird. I believe it is allowed > but discouraged. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
"All platforms" here means "including Android WebView"? If so, that is indeed a perfect reason for this code to live in //components. Could you find a suitable second owner for the new component and have them do a detailed review before I stamp the addition of the component?
On 2017/04/19 09:06:26, blundell wrote: > "All platforms" here means "including Android WebView"? If so, that is indeed a > perfect reason for this code to live in //components. > > Could you find a suitable second owner for the new component and have them do a > detailed review before I stamp the addition of the component? There was a discussion regarding to the platforms we would support for this feature. Though for the first version we are targeting Windows / Linux / Mac and Chrome OS to resolve the immediate requirement, we do not have any decision to not support Android, Android WebView or even iOS: the scenario is perfectly reasonable on these platforms, as both mobile platforms support physical keyboard and fullscreen web browsing. For the code review, I would invite JamieWalch@ and SergeyU@ from Chromoting team as well.
zijiehe@chromium.org changed reviewers: + jamiewalch@chromium.org, sergeyu@chromium.org
Dave, any other suggestion for this change?
https://codereview.chromium.org/2815023002/diff/60001/components/keyboard_loc... File components/keyboard_lock/keyboard_lock_host.cc (right): https://codereview.chromium.org/2815023002/diff/60001/components/keyboard_loc... components/keyboard_lock/keyboard_lock_host.cc:41: base::AutoLock lock(lock_); On 2017/04/19 00:46:26, Hzj_jie wrote: > On 2017/04/18 13:43:17, dtapuska wrote: > > Are there multiple threads running around that you'd need an interceptor for? > If > > there are then the lifecycle of the returned pointer is bogus because it is > > stored via a unique_ptr. > > > > If there are multiple threads then the object should be refcounted. If there > > aren't then you don't need this lock. > > Multiple threads are required: the interceptor will be accessed by native > message loop by X11 or low level callbacks of key events on Windows / Mac OS, > which are running on its own thread or unpredictable threads. > > I believe this behavior is safe, since the KeyboardLockHost as well as the > interceptor are never destroyed because of the base::LeakySingletonTraits used > above. So ultimately is this just not a construction problem. In that you want to initialize the keyboard lock host before any threads start up? https://codereview.chromium.org/2815023002/diff/120001/components/keyboard_lo... File components/keyboard_lock/key_event_interceptor_installer.h (right): https://codereview.chromium.org/2815023002/diff/120001/components/keyboard_lo... components/keyboard_lock/key_event_interceptor_installer.h:17: class KeyEventInterceptorInstaller : public KeyEventInterceptor { Can you detail how the implementation of this class is going to be thread safe? https://codereview.chromium.org/2815023002/diff/120001/components/keyboard_lo... components/keyboard_lock/key_event_interceptor_installer.h:24: void Install(); The lifecyle of the install is weird. In that you take a ptr and move yourself as a unique_ptr.
https://codereview.chromium.org/2815023002/diff/60001/components/keyboard_loc... File components/keyboard_lock/keyboard_lock_host.cc (right): https://codereview.chromium.org/2815023002/diff/60001/components/keyboard_loc... components/keyboard_lock/keyboard_lock_host.cc:41: base::AutoLock lock(lock_); On 2017/04/21 14:48:33, dtapuska wrote: > On 2017/04/19 00:46:26, Hzj_jie wrote: > > On 2017/04/18 13:43:17, dtapuska wrote: > > > Are there multiple threads running around that you'd need an interceptor > for? > > If > > > there are then the lifecycle of the returned pointer is bogus because it is > > > stored via a unique_ptr. > > > > > > If there are multiple threads then the object should be refcounted. If there > > > aren't then you don't need this lock. > > > > Multiple threads are required: the interceptor will be accessed by native > > message loop by X11 or low level callbacks of key events on Windows / Mac OS, > > which are running on its own thread or unpredictable threads. > > > > I believe this behavior is safe, since the KeyboardLockHost as well as the > > interceptor are never destroyed because of the base::LeakySingletonTraits used > > above. > > So ultimately is this just not a construction problem. In that you want to > initialize the keyboard lock host before any threads start up? Sorry, it looks like I do not follow. KeyboardLockHost should only be initialized during the first GetInstance() call, which is usually triggered by a navigator.requestKeyLock() call. https://codereview.chromium.org/2815023002/diff/120001/components/keyboard_lo... File components/keyboard_lock/key_event_interceptor_installer.h (right): https://codereview.chromium.org/2815023002/diff/120001/components/keyboard_lo... components/keyboard_lock/key_event_interceptor_installer.h:17: class KeyEventInterceptorInstaller : public KeyEventInterceptor { On 2017/04/21 14:48:33, dtapuska wrote: > Can you detail how the implementation of this class is going to be thread safe? A typical implementation does not need to take care of the multithreading, it would be guarded by a KeyEventInterceptorThreadWrapper. i.e. The OnKeyDown() and OnKeyUp() function calls will always ensure to be multithreading-protected. I will update the comments of KeyEventInterceptor. https://codereview.chromium.org/2815023002/diff/120001/components/keyboard_lo... components/keyboard_lock/key_event_interceptor_installer.h:24: void Install(); On 2017/04/21 14:48:33, dtapuska wrote: > The lifecyle of the install is weird. In that you take a ptr and move yourself > as a unique_ptr. The KeyLockHost takes owner-ship of the KeyEventInterceptorInstaller, so user cannot create an instance of KeyEventInterceptorInstaller(), but only use Create() function to create one and return a weak reference. But I do agree this looks a little bit weird, do you happen to have a better solution?
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: This issue passed the CQ dry run.
On 2017/04/21 21:13:09, Hzj_jie wrote: > https://codereview.chromium.org/2815023002/diff/60001/components/keyboard_loc... > File components/keyboard_lock/keyboard_lock_host.cc (right): > > https://codereview.chromium.org/2815023002/diff/60001/components/keyboard_loc... > components/keyboard_lock/keyboard_lock_host.cc:41: base::AutoLock lock(lock_); > On 2017/04/21 14:48:33, dtapuska wrote: > > On 2017/04/19 00:46:26, Hzj_jie wrote: > > > On 2017/04/18 13:43:17, dtapuska wrote: > > > > Are there multiple threads running around that you'd need an interceptor > > for? > > > If > > > > there are then the lifecycle of the returned pointer is bogus because it > is > > > > stored via a unique_ptr. > > > > > > > > If there are multiple threads then the object should be refcounted. If > there > > > > aren't then you don't need this lock. > > > > > > Multiple threads are required: the interceptor will be accessed by native > > > message loop by X11 or low level callbacks of key events on Windows / Mac > OS, > > > which are running on its own thread or unpredictable threads. > > > > > > I believe this behavior is safe, since the KeyboardLockHost as well as the > > > interceptor are never destroyed because of the base::LeakySingletonTraits > used > > > above. > > > > So ultimately is this just not a construction problem. In that you want to > > initialize the keyboard lock host before any threads start up? > > Sorry, it looks like I do not follow. KeyboardLockHost should only be > initialized during the first GetInstance() call, which is usually triggered by a > navigator.requestKeyLock() call. > > https://codereview.chromium.org/2815023002/diff/120001/components/keyboard_lo... > File components/keyboard_lock/key_event_interceptor_installer.h (right): > > https://codereview.chromium.org/2815023002/diff/120001/components/keyboard_lo... > components/keyboard_lock/key_event_interceptor_installer.h:17: class > KeyEventInterceptorInstaller : public KeyEventInterceptor { > On 2017/04/21 14:48:33, dtapuska wrote: > > Can you detail how the implementation of this class is going to be thread > safe? > > A typical implementation does not need to take care of the multithreading, it > would be guarded by a KeyEventInterceptorThreadWrapper. i.e. The OnKeyDown() and > OnKeyUp() function calls will always ensure to be multithreading-protected. > > I will update the comments of KeyEventInterceptor. > > https://codereview.chromium.org/2815023002/diff/120001/components/keyboard_lo... > components/keyboard_lock/key_event_interceptor_installer.h:24: void Install(); > On 2017/04/21 14:48:33, dtapuska wrote: > > The lifecyle of the install is weird. In that you take a ptr and move yourself > > as a unique_ptr. > > The KeyLockHost takes owner-ship of the KeyEventInterceptorInstaller, so user > cannot create an instance of KeyEventInterceptorInstaller(), but only use > Create() function to create one and return a weak reference. But I do agree this > looks a little bit weird, do you happen to have a better solution? Dave, I still cannot understand your concern regarding to the KeyboardLockHost::GetInstance(), would you mind to give me more hints?
More comments?
zijiehe@chromium.org changed reviewers: + wez@chromium.org
Talked offline with Dave and Wez to involve Wez in this change.
Some initial comments, but I need to read through the design properly before I continue reviewing! https://codereview.chromium.org/2815023002/diff/140001/components/keyboard_lo... File components/keyboard_lock/key_event_interceptor.h (right): https://codereview.chromium.org/2815023002/diff/140001/components/keyboard_lo... components/keyboard_lock/key_event_interceptor.h:15: class KeyEventInterceptor { "interceptor" sounds like this is the thing that intercepts, whereas it is actually the thing that receives the intercepted events, I think? Consider calling this KeyLockReceiver, or KeyLockClient (which would fit the KeyboardLockHost naming better). https://codereview.chromium.org/2815023002/diff/140001/components/keyboard_lo... components/keyboard_lock/key_event_interceptor.h:21: virtual bool OnKeyUp(ui::KeyboardCode code) = 0; ui::KeyboardCode is evil and deprecated. :P Is it possible to pass a ui::NativeEvent, or even a ui::KeyEvent here instead? https://codereview.chromium.org/2815023002/diff/140001/components/keyboard_lo... File components/keyboard_lock/key_event_interceptor_installer.h (right): https://codereview.chromium.org/2815023002/diff/140001/components/keyboard_lo... components/keyboard_lock/key_event_interceptor_installer.h:17: class KeyEventInterceptorInstaller : public KeyEventInterceptor { So this class installs itself as the current KeyEventInterceptor, and then removes itself in its destructor? https://codereview.chromium.org/2815023002/diff/140001/components/keyboard_lo... components/keyboard_lock/key_event_interceptor_installer.h:25: // calls Installer() function and returns a raw pointer from it. Why do we need this complexity? Couldn't the implementor of the KeyEventInterceptor just call to the Host directly to set/clear the current interceptor instance? https://codereview.chromium.org/2815023002/diff/140001/components/keyboard_lo... File components/keyboard_lock/keyboard_lock_host.h (right): https://codereview.chromium.org/2815023002/diff/140001/components/keyboard_lo... components/keyboard_lock/keyboard_lock_host.h:34: // is allowed during the lifetime of the process. How does a caller install a KeyEventInterceptor? There doesn't seem to be an API for that? What does it mean for only one to be allowed "for the lifetime of the process"? Do you mean "no more than one at a time", or do you really mean that we can't register and unregister over the lifetime of Chrome? https://codereview.chromium.org/2815023002/diff/140001/components/keyboard_lo... components/keyboard_lock/keyboard_lock_host.h:37: std::unique_ptr<KeyEventInterceptor::Retriever> GetInterceptorRetriever(); What does this method do? https://codereview.chromium.org/2815023002/diff/140001/components/keyboard_lo... components/keyboard_lock/keyboard_lock_host.h:43: // Allows Singleton to create a instance of KeyboardLockHost. Why do we want to use Singleton rather than creating a Leaky LazyInstance, or even just having a static global pointer to the instance? https://codereview.chromium.org/2815023002/diff/140001/components/keyboard_lo... components/keyboard_lock/keyboard_lock_host.h:59: mutable base::Lock lock_; You have a simple-getter for |interceptor_|, above, which returns a bare-pointer - if this member can be set/got from any thread, how do we manage the locking when something calls interceptor() to get it?
https://codereview.chromium.org/2815023002/diff/140001/components/keyboard_lo... File components/keyboard_lock/key_event_interceptor.h (right): https://codereview.chromium.org/2815023002/diff/140001/components/keyboard_lo... components/keyboard_lock/key_event_interceptor.h:15: class KeyEventInterceptor { On 2017/04/28 00:22:44, Wez wrote: > "interceptor" sounds like this is the thing that intercepts, whereas it is > actually the thing that receives the intercepted events, I think? > > Consider calling this KeyLockReceiver, or KeyLockClient (which would fit the > KeyboardLockHost naming better). Yes, it does intercept the events, it controls whether the event should be deprecated by returning true / false from its OnKeyDown() and OnKeyUp() functions. https://codereview.chromium.org/2815023002/diff/140001/components/keyboard_lo... components/keyboard_lock/key_event_interceptor.h:21: virtual bool OnKeyUp(ui::KeyboardCode code) = 0; On 2017/04/28 00:22:44, Wez wrote: > ui::KeyboardCode is evil and deprecated. :P > > Is it possible to pass a ui::NativeEvent, or even a ui::KeyEvent here instead? That's too bad. I suppose there is always a replacement, is there? ui::NativeEvent is not good for the scenario: most of the logic related to this interface are platform independent. ui::KeyEvent is workable, but it contains up / down information, which is conflict with OnKeyDown() and OnKeyUp() functions: AFAICT, both Windows and Mac OS provide key down / up events separately from low level callback. https://codereview.chromium.org/2815023002/diff/140001/components/keyboard_lo... File components/keyboard_lock/key_event_interceptor_installer.h (right): https://codereview.chromium.org/2815023002/diff/140001/components/keyboard_lo... components/keyboard_lock/key_event_interceptor_installer.h:17: class KeyEventInterceptorInstaller : public KeyEventInterceptor { On 2017/04/28 00:22:44, Wez wrote: > So this class installs itself as the current KeyEventInterceptor, and then > removes itself in its destructor? Yes, but technically speaking, it won't be destructed during the lifetime of the process. The destructor works only in test cases. https://codereview.chromium.org/2815023002/diff/140001/components/keyboard_lo... components/keyboard_lock/key_event_interceptor_installer.h:25: // calls Installer() function and returns a raw pointer from it. On 2017/04/28 00:22:44, Wez wrote: > Why do we need this complexity? Couldn't the implementor of the > KeyEventInterceptor just call to the Host directly to set/clear the current > interceptor instance? That's too dangerous, the setter is not accessible out of KeyEventInterceptorInstaller because we do not like to return a deleting object from KeyboardLockHost. KeyEventInterceptor works cross threads, and the consumers have only a weak reference of it from KeyboardLockHost. https://codereview.chromium.org/2815023002/diff/140001/components/keyboard_lo... File components/keyboard_lock/keyboard_lock_host.h (right): https://codereview.chromium.org/2815023002/diff/140001/components/keyboard_lo... components/keyboard_lock/keyboard_lock_host.h:34: // is allowed during the lifetime of the process. On 2017/04/28 00:22:45, Wez wrote: > How does a caller install a KeyEventInterceptor? There doesn't seem to be an API > for that? > > What does it mean for only one to be allowed "for the lifetime of the process"? > Do you mean "no more than one at a time", or do you really mean that we can't > register and unregister over the lifetime of Chrome? We cannot register and unregister over the lifetime of Chrome. https://codereview.chromium.org/2815023002/diff/140001/components/keyboard_lo... components/keyboard_lock/keyboard_lock_host.h:37: std::unique_ptr<KeyEventInterceptor::Retriever> GetInterceptorRetriever(); On 2017/04/28 00:22:45, Wez wrote: > What does this method do? To reduce the dependency of the singleton object of KeyboardLockHost. Considering the scenario, a consumer needs to access the KeyEventInterceptor, say the PlatformEventSource, to forward keys directly to the web page without executing all the logic in the browser, instead of doing, KeyboardLockHost::GetInstance()->interceptor()->OnKeyDown(...); It can do this, PlatformEventSource::PlatformEventSource(std::unique_ptr<KeyEventInterceptor::Retriever> retriever) { ... } if (retriever_->Get()) { retriever_->Get()->OnKeyDown(...); } So the PlatformEventSource is still testable. https://codereview.chromium.org/2815023002/diff/140001/components/keyboard_lo... components/keyboard_lock/keyboard_lock_host.h:43: // Allows Singleton to create a instance of KeyboardLockHost. On 2017/04/28 00:22:44, Wez wrote: > Why do we want to use Singleton rather than creating a Leaky LazyInstance, or > even just having a static global pointer to the instance? Is there a preference? As long as they are leaking, I do not see a significant difference. https://codereview.chromium.org/2815023002/diff/140001/components/keyboard_lo... components/keyboard_lock/keyboard_lock_host.h:59: mutable base::Lock lock_; On 2017/04/28 00:22:44, Wez wrote: > You have a simple-getter for |interceptor_|, above, which returns a bare-pointer > - if this member can be set/got from any thread, how do we manage the locking > when something calls interceptor() to get it? That's the reason setter can only be called once. There is only one change of state of interceptor(), nullptr or a live object. If one get interceptor() != nullptr, it can assume it won't become nullptr again, and the object won't be deleted.
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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #4 (id:160001) 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: This issue passed the CQ dry run.
zijiehe@chromium.org changed reviewers: + garykac@chromium.org - jamiewalch@chromium.org, sergeyu@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #5 (id:200001) has been deleted
Description was changed from ========== [System-Keyboard-Lock] Add KeyboardLockHost and KeyEventInterceptor This change starts KeyboardLock component, which is a singleton object in browser process to receive requestKeyLock() and cancelKeyLock() requests from web pages, monitor web frame states, receive low level key events and forward to corresponding web pages. KeyEventInterceptor is an interface to receive low level key events, which is used by low level key event callback on Windows and Mac OS, and PlatformEventSource on Linux, to deliver the key events directly to the web page. 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 ========== to ========== [System-Keyboard-Lock] Add KeyboardLockHost and KeyEventInterceptor This change starts KeyboardLock component, which is a singleton object in browser process to receive requestKeyLock() and cancelKeyLock() requests from web pages, monitor web frame states, receive low level key events and forward to corresponding web pages. KeyEventInterceptor is an interface to receive low level key events, which is used by low level key event callback on Windows and Mac OS, and PlatformEventSource on Linux, to deliver the key events directly to the web page. 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... A prototype can be found at, https://codereview.chromium.org/2879033002 BUG=680809 ==========
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 #5 (id:220001) 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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== [System-Keyboard-Lock] Add KeyboardLockHost and KeyEventInterceptor This change starts KeyboardLock component, which is a singleton object in browser process to receive requestKeyLock() and cancelKeyLock() requests from web pages, monitor web frame states, receive low level key events and forward to corresponding web pages. KeyEventInterceptor is an interface to receive low level key events, which is used by low level key event callback on Windows and Mac OS, and PlatformEventSource on Linux, to deliver the key events directly to the web page. 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... A prototype can be found at, https://codereview.chromium.org/2879033002 BUG=680809 ========== to ========== [System-Keyboard-Lock] Add KeyboardLockHost and KeyEventFilter This change starts KeyboardLock component, which is an object in browser process (Browser object) to receive requestKeyboardLock() and cancelKeyboardLock() requests from web pages, monitor web frame states, receive low level key events and forward to corresponding web pages. KeyEventFilter is an interface to receive low level key events, which is used by low level key event callback on Windows and Mac OS, and PlatformEventSource on Linux, to deliver the key events directly to the web page. 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... A prototype can be found at, https://codereview.chromium.org/2879033002 BUG=680809 ==========
zijiehe@chromium.org changed reviewers: - garykac@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) 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
Patchset #6 (id:260001) 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...
Hi, Wez, Thank you for your suggestion on the design doc. I have updated the code according to the latest design. Would you mind to have a look? Thank you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Will do, but I'm afraid not until later-on tomorrow. On 7 June 2017 at 17:42, <zijiehe@chromium.org> wrote: > Hi, Wez, > Thank you for your suggestion on the design doc. I have updated the code > according to the latest design. Would you mind to have a look? > > Thank you. > > https://codereview.chromium.org/2815023002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/06/08 08:11:20, Wez wrote: > Will do, but I'm afraid not until later-on tomorrow. > > On 7 June 2017 at 17:42, <mailto:zijiehe@chromium.org> wrote: > > > Hi, Wez, > > Thank you for your suggestion on the design doc. I have updated the code > > according to the latest design. Would you mind to have a look? > > > > Thank you. > > > > https://codereview.chromium.org/2815023002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Any comments?
wez@chromium.org changed reviewers: + garykac@chromium.org - wez@chromium.org
zijiehe@chromium.org changed reviewers: + jochen@chromium.org - blundell@chromium.org, caitkp@chromium.org, dtapuska@chromium.org
After a long design discussion, we finally can restart the code reviewing process of the feature. Gary, this change is not impacted by the issue we left about the IME. Please take a look. Jochen, this change requires owners of components/ folder to sign off. But the original reviewer Colin is OOO recently. Thank you.
Message was sent while issue was closed.
zijiehe@chromium.org changed reviewers: - garykac@chromium.org, jochen@chromium.org
Message was sent while issue was closed.
On 2017/08/11 18:39:26, Hzj_jie wrote: > After a long design discussion, we finally can restart the code reviewing > process of the feature. > > Gary, this change is not impacted by the issue we left about the IME. Please > take a look. > Jochen, this change requires owners of components/ folder to sign off. But the > original reviewer Colin is OOO recently. > > Thank you. This cursed change has been abandoned. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
