Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2346643003:
[Remoting Host] Handle text event characters that are not presented on the keyboard (Closed)
Description[Remoting Host] Handle text event characters that are not presented on the keyboard
This CL allows the x11 injector to remap the last keycode to the character if
the character is not on the current keyboard layout.
BUG=645659
Committed: https://crrev.com/ab975ef6a5e726359982765e311712f3a0974831
Cr-Commit-Position: refs/heads/master@{#422585}
Patch Set 1 #Patch Set 2 : Use all available keycodes to map all characters in the text #Patch Set 3 : Use ostringstream #
Total comments: 12
Patch Set 4 : Reviewer's Feedback #
Total comments: 8
Patch Set 5 : Queue up characters (No test) #Patch Set 6 : Create KeyboardInterface and XServerKeyboardInterface #
Total comments: 41
Patch Set 7 : Reviewer's Feedback + Mapper Unittest #Patch Set 8 : Rename Files #Patch Set 9 : Some Lint #Patch Set 10 : Merge ToT #
Total comments: 12
Patch Set 11 : Reviewer's Feedback #Patch Set 12 : Remove mock keyboard reference #
Total comments: 2
Patch Set 13 : Fix build #Patch Set 14 : Fix build #Patch Set 15 : Fix flakiness & Try to fix build #Patch Set 16 : Fix Flakiness #
Total comments: 32
Patch Set 17 : Reviewer's Feedback #
Total comments: 4
Patch Set 18 : Reviewer's Feedback #Messages
Total messages: 72 (50 generated)
Description was changed from ========== [Remoting Host] Handle text event characters that are not presented on the keyboard ========== to ========== [Remoting Host] Handle text event characters that are not presented on the keyboard This CL allows the x11 injector to remap the last keycode to the character if the character is not on the current keyboard layout. BUG=645659 ==========
yuweih@chromium.org changed reviewers: + sergeyu@chromium.org
PTAL
jamiewalch@chromium.org changed reviewers: + jamiewalch@chromium.org
https://codereview.chromium.org/2346643003/diff/40001/remoting/host/input_inj... File remoting/host/input_injector_x11.cc (right): https://codereview.chromium.org/2346643003/diff/40001/remoting/host/input_inj... remoting/host/input_injector_x11.cc:123: } I think you can just use StringPrintf with %x instead. https://codereview.chromium.org/2346643003/diff/40001/remoting/host/input_inj... remoting/host/input_injector_x11.cc:392: InjectUnmappedText(text.substr(old_index)); Rather than injecting the whole of the rest of the string using newly-minted keys, it would be better to do so only for characters that can't be represented already. I think FindKeycodeForUnicode will take into consideration the new keys you've added, so all you need to do is allocate a new keycode for each unicode character you can't already represent. I don't think the key mapping can ever be reset because you don't know how long applications might take to respond to the keyboard event and try to look up the character. It might make sense to reset it on disconnect; particularly if this logic is not restricted to virtual desktops. Of course, not resetting the key mapping has the downside that you may eventually run out of keycodes, since it looks like there may typically be <100 unused. I'm not sure if there's a good solution to that problem. https://codereview.chromium.org/2346643003/diff/40001/remoting/host/input_inj... remoting/host/input_injector_x11.cc:419: KeySym* old_mapping = XGetKeyboardMapping(display_, min_keycode, Can you use XScopedPtr for to avoid needing to call XFree explicitly? https://codereview.chromium.org/2346643003/diff/40001/remoting/host/input_inj... remoting/host/input_injector_x11.cc:475: // Restores the old mapping. s/Restores/Restore/ https://codereview.chromium.org/2346643003/diff/40001/remoting/host/input_inj... remoting/host/input_injector_x11.cc:478: * old_sym_per_key, text_length); The multiplication should be on the previous line. https://codereview.chromium.org/2346643003/diff/40001/remoting/host/input_inj... remoting/host/input_injector_x11.cc:483: // press happens after the keyboard mapping is restored. I think this comment needs more explanation. It's not clear to me why this is necessary.
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
PTAL https://codereview.chromium.org/2346643003/diff/40001/remoting/host/input_inj... File remoting/host/input_injector_x11.cc (right): https://codereview.chromium.org/2346643003/diff/40001/remoting/host/input_inj... remoting/host/input_injector_x11.cc:123: } On 2016/09/17 00:06:22, Jamie wrote: > I think you can just use StringPrintf with %x instead. Done. https://codereview.chromium.org/2346643003/diff/40001/remoting/host/input_inj... remoting/host/input_injector_x11.cc:392: InjectUnmappedText(text.substr(old_index)); On 2016/09/17 00:06:23, Jamie wrote: > Rather than injecting the whole of the rest of the string using newly-minted > keys, it would be better to do so only for characters that can't be represented > already. I think FindKeycodeForUnicode will take into consideration the new keys > you've added, so all you need to do is allocate a new keycode for each unicode > character you can't already represent. Done. Basically reimplemented this CL. > I don't think the key mapping can ever be > reset because you don't know how long applications might take to respond to the > keyboard event and try to look up the character. It might make sense to reset it > on disconnect; particularly if this logic is not restricted to virtual desktops. Ack. We have to restore the state otherwise no available keycode will be found when we restart the host. The only concern is that things may go wrong if another app changes the keyboard layout during a session... > Of course, not resetting the key mapping has the downside that you may > eventually run out of keycodes, since it looks like there may typically be <100 > unused. `xmodmap -pke | egrep '=\s*$' | wc -l` shows that only 29 keycodes are available on my machine... > I'm not sure if there's a good solution to that problem. Not sure either. X11 doesn't provide more useful API's :( https://codereview.chromium.org/2346643003/diff/40001/remoting/host/input_inj... remoting/host/input_injector_x11.cc:419: KeySym* old_mapping = XGetKeyboardMapping(display_, min_keycode, On 2016/09/17 00:06:23, Jamie wrote: > Can you use XScopedPtr for to avoid needing to call XFree explicitly? Done. https://codereview.chromium.org/2346643003/diff/40001/remoting/host/input_inj... remoting/host/input_injector_x11.cc:475: // Restores the old mapping. On 2016/09/17 00:06:23, Jamie wrote: > s/Restores/Restore/ Obsolete. https://codereview.chromium.org/2346643003/diff/40001/remoting/host/input_inj... remoting/host/input_injector_x11.cc:478: * old_sym_per_key, text_length); On 2016/09/17 00:06:22, Jamie wrote: > The multiplication should be on the previous line. Obsolete. https://codereview.chromium.org/2346643003/diff/40001/remoting/host/input_inj... remoting/host/input_injector_x11.cc:483: // press happens after the keyboard mapping is restored. On 2016/09/17 00:06:22, Jamie wrote: > I think this comment needs more explanation. It's not clear to me why this is > necessary. Obsolete.
https://codereview.chromium.org/2346643003/diff/120001/remoting/host/input_in... File remoting/host/input_injector_x11.cc (right): https://codereview.chromium.org/2346643003/diff/120001/remoting/host/input_in... remoting/host/input_injector_x11.cc:374: int result = key_mapper_->AddNewCharacter(code_point); Does this work well when you inject a string of characters not present on the keyboard and the number of characters is higher than the number of keys we can modify. The receiving application may process the events later, after the layout is changed again, and as result incorrect character gets injected. To workaround this we need to ensure that each key character doesn't get changed for certain time after the key is injected (e.g. 100ms). And then here we would queue key events to be injected in the future when keys are available. https://codereview.chromium.org/2346643003/diff/120001/remoting/host/linux/x_... File remoting/host/linux/x_server_key_mapper.cc (right): https://codereview.chromium.org/2346643003/diff/120001/remoting/host/linux/x_... remoting/host/linux/x_server_key_mapper.cc:15: int min_keycode, max_keycode, sym_per_key; nit: please move sym_per_key just before XGetKeyboardMapping(). Style guide says "We encourage you to declare them in as local a scope as possible, and as close to the first use as possible. " Also separate min an max variables: int min_keycode; int max_keycode; This make code more readable. https://codereview.chromium.org/2346643003/diff/120001/remoting/host/linux/x_... File remoting/host/linux/x_server_key_mapper.h (right): https://codereview.chromium.org/2346643003/diff/120001/remoting/host/linux/x_... remoting/host/linux/x_server_key_mapper.h:40: // TODO(yuweih): Consider better data structure, e.g. LRU. To implement the current solution you can just have a vector and an index that gets incremented when a keycode is used. https://codereview.chromium.org/2346643003/diff/120001/remoting/host/linux/x_... remoting/host/linux/x_server_key_mapper.h:43: std::unordered_set<uint32_t> used_keycodes_; keycodes are always in range [8..255] so this can be a boolean array.
Patchset #6 (id:160001) has been deleted
Patchset #6 (id:180001) has been deleted
It's crazy that such a simple feature requires hundreds of lines of code... Would you go over this before I upload the unit test code (which would be another more hundreds of lines of code)? https://codereview.chromium.org/2346643003/diff/120001/remoting/host/input_in... File remoting/host/input_injector_x11.cc (right): https://codereview.chromium.org/2346643003/diff/120001/remoting/host/input_in... remoting/host/input_injector_x11.cc:374: int result = key_mapper_->AddNewCharacter(code_point); On 2016/09/19 21:39:00, Sergey Ulanov wrote: > Does this work well when you inject a string of characters not present on the > keyboard and the number of characters is higher than the number of keys we can > modify. The receiving application may process the events later, after the layout > is changed again, and as result incorrect character gets injected. To workaround > this we need to ensure that each key character doesn't get changed for certain > time after the key is injected (e.g. 100ms). And then here we would queue key > events to be injected in the future when keys are available. Done. https://codereview.chromium.org/2346643003/diff/120001/remoting/host/linux/x_... File remoting/host/linux/x_server_key_mapper.cc (right): https://codereview.chromium.org/2346643003/diff/120001/remoting/host/linux/x_... remoting/host/linux/x_server_key_mapper.cc:15: int min_keycode, max_keycode, sym_per_key; On 2016/09/19 21:39:00, Sergey Ulanov wrote: > nit: please move sym_per_key just before XGetKeyboardMapping(). Style guide says > "We encourage you to declare them in as local a scope as possible, and as close > to the first use as possible. " > Also separate min an max variables: > int min_keycode; > int max_keycode; > > This make code more readable. Done. https://codereview.chromium.org/2346643003/diff/120001/remoting/host/linux/x_... File remoting/host/linux/x_server_key_mapper.h (right): https://codereview.chromium.org/2346643003/diff/120001/remoting/host/linux/x_... remoting/host/linux/x_server_key_mapper.h:40: // TODO(yuweih): Consider better data structure, e.g. LRU. On 2016/09/19 21:39:00, Sergey Ulanov wrote: > To implement the current solution you can just have a vector and an index that > gets incremented when a keycode is used. For LRU I think we should count use of KeySym rather than the KeyCode, which is not feasible for this class since AddNewCharacter() won't be called if the character is already mapped. https://codereview.chromium.org/2346643003/diff/120001/remoting/host/linux/x_... remoting/host/linux/x_server_key_mapper.h:43: std::unordered_set<uint32_t> used_keycodes_; On 2016/09/19 21:39:00, Sergey Ulanov wrote: > keycodes are always in range [8..255] so this can be a boolean array. Obsolete. The |last_used| field of KeyInfo is enough to tell whether a keycode has been used. https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/ke... File remoting/host/linux/keyboard_interface.h (right): https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/ke... remoting/host/linux/keyboard_interface.h:19: class KeyboardInterface { This interface is basically for unit test. There will probably be another interface for XServerKeyMapper.
I don't think it's necessary to separate KeyMapper from CharacterInjector. Maybe move all logic from KeyMapper to CharacterInjector? https://codereview.chromium.org/2346643003/diff/200001/remoting/host/input_in... File remoting/host/input_injector_x11.cc (right): https://codereview.chromium.org/2346643003/diff/200001/remoting/host/input_in... remoting/host/input_injector_x11.cc:154: std::unique_ptr<XServerKeyboardInterface> keyboard_; Do you need this here? Can XServerCharacterInjector own this object? https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/ke... File remoting/host/linux/keyboard_interface.h (right): https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/ke... remoting/host/linux/keyboard_interface.h:19: class KeyboardInterface { We don't use Interface suffix for interfaces. Also this class is specific to X11. Call it X11Keyboard? https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... File remoting/host/linux/x_server_character_injector.cc (right): https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_character_injector.cc:17: task_runner_(base::ThreadTaskRunnerHandle::Get()), I don't think you need this. Just use base::ThreadTaskRunnerHandle::Get() in Schedule() and maybe add a thread checker. https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_character_injector.cc:31: if (consume_scheduled_) { DCHECK(!consume_scheduled_) https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_character_injector.cc:43: consume_scheduled_ = false; Addd DCHECK(consume_scheduled_); https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_character_injector.cc:54: } else { don't need else after continue https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... File remoting/host/linux/x_server_key_mapper.cc (right): https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_key_mapper.cc:57: info.last_used_time = base::TimeTicks::Now(); I think we also want to update last_used_time later, when the key gets reused. AddNewCharacter() currently is not invoked in that case. It's best to to put all keys in a heap to make this efficient. https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... File remoting/host/linux/x_server_key_mapper.h (right): https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_key_mapper.h:20: /** Use C++ style comments please: // https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_key_mapper.h:28: class XServerKeyMapper { X11KeyMapper? https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_key_mapper.h:33: // |keycode| will only be valid if success == true. Maybe remove success and use keycode=0 to indicate failure? https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... File remoting/host/linux/x_server_keyboard_interface.cc (right): https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_keyboard_interface.cc:105: for (std::vector<uint32_t>::iterator it = keysyms.begin(); Use range loop please: for (auto& keysym : keysyms) You could also change GetKeySymsForUnicode() to return std::vector<> instead of taking it in the arguments. With copy elision the compiler may generate more efficient code and this line would be simpler: for (auto& keysym : GetKeySymsForUnicode(code_point)) https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_keyboard_interface.cc:115: uint32_t keycode, int instead of uint32_t. https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_keyboard_interface.cc:117: uint32_t upper_case_code_point) { Maybe use KeySym for these two parameters? https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_keyboard_interface.cc:117: uint32_t upper_case_code_point) { Do we need it here? It doesn't appear to be useful XServerKeyMapper passes same value for these two parameters. https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_keyboard_interface.cc:127: KeySym syms[2]; = {lower_case_keysym, upper_case_keysum}; https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... File remoting/host/linux/x_server_keyboard_interface.h (right): https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_keyboard_interface.h:15: class XServerKeyboardInterface : public KeyboardInterface { This is not interface, so it shouldn't have Interface suffix. X11KeyboardImpl?
> I don't think it's necessary to separate KeyMapper from CharacterInjector. > Maybe move all logic from KeyMapper to CharacterInjector? I'm not sure how to unittest KeyMapper logic in that case. Testing private function doesn't look very good to me... https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/ke... File remoting/host/linux/keyboard_interface.h (right): https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/ke... remoting/host/linux/keyboard_interface.h:19: class KeyboardInterface { On 2016/09/21 19:59:23, Sergey Ulanov wrote: > We don't use Interface suffix for interfaces. "Interface" here actually means it is an interface to talk to the keyboard >Also this class is specific to X11. Call it X11Keyboard? If the interface takes "X11" as part of the name then how should we call the actual X11 implementation? Maybe just remove this interface and mark all functions of X11Keyboard virtual. https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... File remoting/host/linux/x_server_key_mapper.cc (right): https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_key_mapper.cc:57: info.last_used_time = base::TimeTicks::Now(); On 2016/09/21 19:59:23, Sergey Ulanov wrote: > I think we also want to update last_used_time later, when the key gets reused. > AddNewCharacter() currently is not invoked in that case. Since the time here is just to prevent remapping before last_used_time + kMinReuseDuration, I don't think changing it when no remapping takes place is necessary. Maybe just rename it to last_mapped_time? > It's best to to put all keys in a heap to make this efficient. SGTM. I'll add a field "use_count" in the struct.
https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/ke... File remoting/host/linux/keyboard_interface.h (right): https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/ke... remoting/host/linux/keyboard_interface.h:19: class KeyboardInterface { On 2016/09/21 20:35:32, Yuwei wrote: > On 2016/09/21 19:59:23, Sergey Ulanov wrote: > > We don't use Interface suffix for interfaces. > > "Interface" here actually means it is an interface to talk to the keyboard > > >Also this class is specific to X11. Call it X11Keyboard? > > If the interface takes "X11" as part of the name then how should we call the > actual X11 implementation? X11KeyboardImpl > > Maybe just remove this interface and mark all functions of X11Keyboard virtual. This would work as well. Potentially this code can be tested without mocking X11. https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... File remoting/host/linux/x_server_key_mapper.cc (right): https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_key_mapper.cc:57: info.last_used_time = base::TimeTicks::Now(); On 2016/09/21 20:35:32, Yuwei wrote: > On 2016/09/21 19:59:23, Sergey Ulanov wrote: > > I think we also want to update last_used_time later, when the key gets reused. > > AddNewCharacter() currently is not invoked in that case. > > Since the time here is just to prevent remapping before last_used_time + > kMinReuseDuration, I don't think changing it when no remapping takes place is > necessary. Maybe just rename it to last_mapped_time? I think we want to track the time when the mapping is used, not just when it's first mapped. If you inject a key event and immediately change mapping for that key then the receiving application may process the event with the updated mapping, i.e. it will get incorrect character. Apps get just keycode as part of XKeyEvent and they need to map each event to KeySym. e.g. suppose we have 3 keys for remapping and we receive ABC and then AD after 300ms delay. In this case this code will inject the first key mapped to A and then immediately remap it to D. As result the receiving application is likely to get ABCDD instead of ABCAD > > > It's best to to put all keys in a heap to make this efficient. > > SGTM. I'll add a field "use_count" in the struct. How would that be used? In the heap I think we want to order the keys by the time they were used last time.
https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... File remoting/host/linux/x_server_key_mapper.cc (right): https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_key_mapper.cc:57: info.last_used_time = base::TimeTicks::Now(); On 2016/09/21 20:55:23, Sergey Ulanov wrote: > On 2016/09/21 20:35:32, Yuwei wrote: > > On 2016/09/21 19:59:23, Sergey Ulanov wrote: > > > I think we also want to update last_used_time later, when the key gets > reused. > > > AddNewCharacter() currently is not invoked in that case. > > > > Since the time here is just to prevent remapping before last_used_time + > > kMinReuseDuration, I don't think changing it when no remapping takes place is > > necessary. Maybe just rename it to last_mapped_time? > > I think we want to track the time when the mapping is used, not just when it's > first mapped. If you inject a key event and immediately change mapping for that > key then the receiving application may process the event with the updated > mapping, i.e. it will get incorrect character. Apps get just keycode as part of > XKeyEvent and they need to map each event to KeySym. > > e.g. suppose we have 3 keys for remapping and we receive ABC and then AD after > 300ms delay. In this case this code will inject the first key mapped to A and > then immediately remap it to D. As result the receiving application is likely to > get ABCDD instead of ABCAD Got it. > > > It's best to to put all keys in a heap to make this efficient. > > > > SGTM. I'll add a field "use_count" in the struct. > > How would that be used? In the heap I think we want to order the keys by the > time they were used last time. Sorry. I was thinking about reusing the least used keycode. That doesn't quite work though since that ignores the minimum duration.
https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... File remoting/host/linux/x_server_key_mapper.cc (right): https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_key_mapper.cc:57: info.last_used_time = base::TimeTicks::Now(); On 2016/09/21 19:59:23, Sergey Ulanov wrote: > It's best to to put all keys in a heap to make this efficient. Looks like STL doesn't provide useful heap utility for updating the priority. The only function that fixes the heap is std::make_heap, which is O(n). Moving down the updated node can be O(log(n)) but whether the heap is binary is implementation specific... Also you will need an extra map to locate the KeyInfo from the keycode. I'm just thinking whether we can use a tree set to minimize code complexity. Even having an optimized heap implementation can't get any faster than O(log(n)) for adding (finding) character...
https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... File remoting/host/linux/x_server_key_mapper.cc (right): https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_key_mapper.cc:57: info.last_used_time = base::TimeTicks::Now(); On 2016/09/22 00:32:58, Yuwei wrote: > On 2016/09/21 19:59:23, Sergey Ulanov wrote: > > It's best to to put all keys in a heap to make this efficient. > > Looks like STL doesn't provide useful heap utility for updating the priority. > The only function that fixes the heap is std::make_heap, which is O(n). Moving > down the updated node can be O(log(n)) but whether the heap is binary is > implementation specific... Also you will need an extra map to locate the KeyInfo > from the keycode. > > I'm just thinking whether we can use a tree set to minimize code complexity. > Even having an optimized heap implementation can't get any faster than O(log(n)) > for adding (finding) character... I think just putting everything in a sorted vector will work perfectly fine. We will never have more than 255 elements here, so technically it's always O(1). With that few elements the vector will fit in L1 cache anyway.
PTAL Jamie suggested that we may probably need to block any inputs when the injector is waiting for available keycode just in case the cursor focus is changed to another textbox during that time. It looks like we have a dedicated thread for handling inputs so maybe sleep until the keycode is available could be the simpliest solution rather than post tasks and proactively block other inputs? https://codereview.chromium.org/2346643003/diff/200001/remoting/host/input_in... File remoting/host/input_injector_x11.cc (right): https://codereview.chromium.org/2346643003/diff/200001/remoting/host/input_in... remoting/host/input_injector_x11.cc:154: std::unique_ptr<XServerKeyboardInterface> keyboard_; On 2016/09/21 19:59:23, Sergey Ulanov wrote: > Do you need this here? Can XServerCharacterInjector own this object? Done. https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/ke... File remoting/host/linux/keyboard_interface.h (right): https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/ke... remoting/host/linux/keyboard_interface.h:19: class KeyboardInterface { On 2016/09/21 20:55:23, Sergey Ulanov wrote: > On 2016/09/21 20:35:32, Yuwei wrote: > > On 2016/09/21 19:59:23, Sergey Ulanov wrote: > > > We don't use Interface suffix for interfaces. > > > > "Interface" here actually means it is an interface to talk to the keyboard > > > > >Also this class is specific to X11. Call it X11Keyboard? > > > > If the interface takes "X11" as part of the name then how should we call the > > actual X11 implementation? > > X11KeyboardImpl > > > > > Maybe just remove this interface and mark all functions of X11Keyboard > virtual. > > This would work as well. > Potentially this code can be tested without mocking X11. Done. https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/ke... remoting/host/linux/keyboard_interface.h:19: class KeyboardInterface { On 2016/09/21 19:59:23, Sergey Ulanov wrote: > We don't use Interface suffix for interfaces. Also this class is specific to > X11. Call it X11Keyboard? Done. https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... File remoting/host/linux/x_server_character_injector.cc (right): https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_character_injector.cc:17: task_runner_(base::ThreadTaskRunnerHandle::Get()), On 2016/09/21 19:59:23, Sergey Ulanov wrote: > I don't think you need this. Just use base::ThreadTaskRunnerHandle::Get() in > Schedule() and maybe add a thread checker. Done. https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_character_injector.cc:31: if (consume_scheduled_) { On 2016/09/21 19:59:23, Sergey Ulanov wrote: > DCHECK(!consume_scheduled_) Obsolete. Changed the definition of Schedule() a little bit so that a failure in Consume() can reset scheduled execution time. https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_character_injector.cc:43: consume_scheduled_ = false; On 2016/09/21 19:59:23, Sergey Ulanov wrote: > Addd DCHECK(consume_scheduled_); Obsolete. Add check for `scheduled_consume_time_ && base::TimeTicks::Now() >= scheduled_consume_time_` instead. https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_character_injector.cc:54: } else { On 2016/09/21 19:59:23, Sergey Ulanov wrote: > don't need else after continue Done. https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... File remoting/host/linux/x_server_key_mapper.cc (right): https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_key_mapper.cc:57: info.last_used_time = base::TimeTicks::Now(); On 2016/09/22 18:32:46, Sergey Ulanov wrote: > On 2016/09/22 00:32:58, Yuwei wrote: > > On 2016/09/21 19:59:23, Sergey Ulanov wrote: > > > It's best to to put all keys in a heap to make this efficient. > > > > Looks like STL doesn't provide useful heap utility for updating the priority. > > The only function that fixes the heap is std::make_heap, which is O(n). Moving > > down the updated node can be O(log(n)) but whether the heap is binary is > > implementation specific... Also you will need an extra map to locate the > KeyInfo > > from the keycode. > > > > I'm just thinking whether we can use a tree set to minimize code complexity. > > Even having an optimized heap implementation can't get any faster than > O(log(n)) > > for adding (finding) character... > > I think just putting everything in a sorted vector will work perfectly fine. We > will never have more than 255 elements here, so technically it's always O(1). > With that few elements the vector will fit in L1 cache anyway. Done. https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... File remoting/host/linux/x_server_key_mapper.h (right): https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_key_mapper.h:20: /** On 2016/09/21 19:59:23, Sergey Ulanov wrote: > Use C++ style comments please: // Done. https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_key_mapper.h:28: class XServerKeyMapper { On 2016/09/21 19:59:23, Sergey Ulanov wrote: > X11KeyMapper? Done. https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_key_mapper.h:33: // |keycode| will only be valid if success == true. On 2016/09/21 19:59:23, Sergey Ulanov wrote: > Maybe remove success and use keycode=0 to indicate failure? Just think could keycode 0 be used for mapping... Since now we also have |modifiers| I think having an explicit success field may be clearer... Just removed this comment. https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... File remoting/host/linux/x_server_keyboard_interface.cc (right): https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_keyboard_interface.cc:105: for (std::vector<uint32_t>::iterator it = keysyms.begin(); On 2016/09/21 19:59:23, Sergey Ulanov wrote: > Use range loop please: > for (auto& keysym : keysyms) > > You could also change GetKeySymsForUnicode() to return std::vector<> instead of > taking it in the arguments. With copy elision the compiler may generate more > efficient code and this line would be simpler: > for (auto& keysym : GetKeySymsForUnicode(code_point)) Done. https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_keyboard_interface.cc:115: uint32_t keycode, On 2016/09/21 19:59:23, Sergey Ulanov wrote: > int instead of uint32_t. Is there any reason we prefer int? Looks like keycode is defined as unsigned... https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_keyboard_interface.cc:117: uint32_t upper_case_code_point) { On 2016/09/21 19:59:23, Sergey Ulanov wrote: > Maybe use KeySym for these two parameters? KeySym is tyepdef'ed as unsigned int. I tried to avoid using KeySym in function signature so that the header doesn't need to include Xlib https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_keyboard_interface.cc:117: uint32_t upper_case_code_point) { On 2016/09/21 19:59:24, Sergey Ulanov wrote: > Do we need it here? It doesn't appear to be useful XServerKeyMapper passes same > value for these two parameters. Removed. I just thought it could be useful if we allow setting lower and upper case separately. It's currently not used. https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_keyboard_interface.cc:127: KeySym syms[2]; On 2016/09/21 19:59:24, Sergey Ulanov wrote: > = {lower_case_keysym, upper_case_keysum}; Done. https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... File remoting/host/linux/x_server_keyboard_interface.h (right): https://codereview.chromium.org/2346643003/diff/200001/remoting/host/linux/x_... remoting/host/linux/x_server_keyboard_interface.h:15: class XServerKeyboardInterface : public KeyboardInterface { On 2016/09/21 19:59:24, Sergey Ulanov wrote: > This is not interface, so it shouldn't have Interface suffix. X11KeyboardImpl? Done.
The CQ bit was checked by yuweih@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...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) 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-...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) 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_compile_dbg_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_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_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_...) 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 yuweih@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_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
I still think it's best to merge X11CharacterInjector and X11KeyMapper. See my suggestions on how you can test them together. https://codereview.chromium.org/2346643003/diff/280001/remoting/host/linux/mo... File remoting/host/linux/mock_x11_keyboard.h (right): https://codereview.chromium.org/2346643003/diff/280001/remoting/host/linux/mo... remoting/host/linux/mock_x11_keyboard.h:15: class MockX11Keyboard : public X11Keyboard { Put this class in x11_key_mapper_unittest.cc . It's not likely to be useful anywhere else. https://codereview.chromium.org/2346643003/diff/280001/remoting/host/linux/mo... remoting/host/linux/mock_x11_keyboard.h:15: class MockX11Keyboard : public X11Keyboard { In this case I think the tests would be significantly simpler and easier to read with fake class instead of a mock. Here is how it could be implemented: - FakeX11Keyboard would implement the same logic that's used in X11 to map characters (simple keycode to codepoint map). Each character should be mapped with some small delay to emulate normal behavior of X11 apps. It would then store the sequence of characters that were injected, with their timestamps. - The test would try to inject a sequence of characters and then verify that correct sequence is injected. The test could also verify that proper delays were inserted between injected keys. I think this approach would allow to merge X11CharacterInjector with X11KeyMapper and test them together. Currently X11CharacterInjector doesn't have any tests, which is not ideal. Also the tests would be much simpler. In general GMock-based tests are not very readable and big PITA to work with (e.g. when refactoring). Here is an old but still relevant thread about gmock: https://groups.google.com/a/chromium.org/d/topic/chromium-dev/-KH_IP0rIWQ/dis... https://codereview.chromium.org/2346643003/diff/280001/remoting/host/linux/mo... remoting/host/linux/mock_x11_keyboard.h:21: MOCK_METHOD2(PressKey, void(uint32_t, uint32_t)); add parameter names for all parameters https://codereview.chromium.org/2346643003/diff/280001/remoting/host/linux/mo... remoting/host/linux/mock_x11_keyboard.h:25: void Flush() override; If this is a Mock class, then it's best to mock all methods. Then add EXPECT_CALL() if you don't want to verify that they are called properly. https://codereview.chromium.org/2346643003/diff/280001/remoting/host/linux/x1... File remoting/host/linux/x11_character_injector.cc (right): https://codereview.chromium.org/2346643003/diff/280001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector.cc:33: consume_timer_.Stop(); Don't need to call Stop(). Start() call below will cancel previuos timer before scheduling the new one. https://codereview.chromium.org/2346643003/diff/280001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector.cc:35: scheduled_consume_time_ = base::TimeTicks::Now() + delay; base::TimeTicks::Now() is called twice in this function. It's better to call it only once in the beginning of this function and use the result in both places.
Patchset #11 (id:300001) has been deleted
The CQ bit was checked by yuweih@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_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by yuweih@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...
PTAL https://codereview.chromium.org/2346643003/diff/280001/remoting/host/linux/mo... File remoting/host/linux/mock_x11_keyboard.h (right): https://codereview.chromium.org/2346643003/diff/280001/remoting/host/linux/mo... remoting/host/linux/mock_x11_keyboard.h:15: class MockX11Keyboard : public X11Keyboard { On 2016/09/23 18:51:26, Sergey Ulanov wrote: > In this case I think the tests would be significantly simpler and easier to read > with fake class instead of a mock. > Here is how it could be implemented: > - FakeX11Keyboard would implement the same logic that's used in X11 to map > characters (simple keycode to codepoint map). Each character should be mapped > with some small delay to emulate normal behavior of X11 apps. It would then > store the sequence of characters that were injected, with their timestamps. > - The test would try to inject a sequence of characters and then verify that > correct sequence is injected. The test could also verify that proper delays were > inserted between injected keys. > > I think this approach would allow to merge X11CharacterInjector with > X11KeyMapper and test them together. Currently X11CharacterInjector doesn't have > any tests, which is not ideal. > Also the tests would be much simpler. In general GMock-based tests are not very > readable and big PITA to work with (e.g. when refactoring). Here is an old but > still relevant thread about gmock: > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/-KH_IP0rIWQ/dis... Done. https://codereview.chromium.org/2346643003/diff/280001/remoting/host/linux/mo... remoting/host/linux/mock_x11_keyboard.h:15: class MockX11Keyboard : public X11Keyboard { On 2016/09/23 18:51:26, Sergey Ulanov wrote: > Put this class in x11_key_mapper_unittest.cc . It's not likely to be useful > anywhere else. Done. https://codereview.chromium.org/2346643003/diff/280001/remoting/host/linux/mo... remoting/host/linux/mock_x11_keyboard.h:21: MOCK_METHOD2(PressKey, void(uint32_t, uint32_t)); On 2016/09/23 18:51:27, Sergey Ulanov wrote: > add parameter names for all parameters Obsolete. https://codereview.chromium.org/2346643003/diff/280001/remoting/host/linux/mo... remoting/host/linux/mock_x11_keyboard.h:25: void Flush() override; On 2016/09/23 18:51:26, Sergey Ulanov wrote: > If this is a Mock class, then it's best to mock all methods. Then add > EXPECT_CALL() if you don't want to verify that they are called properly. Obsolete. https://codereview.chromium.org/2346643003/diff/280001/remoting/host/linux/x1... File remoting/host/linux/x11_character_injector.cc (right): https://codereview.chromium.org/2346643003/diff/280001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector.cc:33: consume_timer_.Stop(); On 2016/09/23 18:51:27, Sergey Ulanov wrote: > Don't need to call Stop(). Start() call below will cancel previuos timer before > scheduling the new one. Done. https://codereview.chromium.org/2346643003/diff/280001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector.cc:35: scheduled_consume_time_ = base::TimeTicks::Now() + delay; On 2016/09/23 18:51:27, Sergey Ulanov wrote: > base::TimeTicks::Now() is called twice in this function. It's better to call it > only once in the beginning of this function and use the result in both places. Done. https://codereview.chromium.org/2346643003/diff/340001/remoting/host/input_in... File remoting/host/input_injector_x11.cc (right): https://codereview.chromium.org/2346643003/diff/340001/remoting/host/input_in... remoting/host/input_injector_x11.cc:581: new X11CharacterInjector(base::MakeUnique<X11KeyboardImpl>(display_))); I think passing a unique_ptr<X11Keyboard> can avoid macro pollution by including X11 stuff in CharacterInjector's header file.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by yuweih@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by yuweih@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 yuweih@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by yuweih@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2346643003/diff/340001/remoting/host/input_in... File remoting/host/input_injector_x11.cc (right): https://codereview.chromium.org/2346643003/diff/340001/remoting/host/input_in... remoting/host/input_injector_x11.cc:581: new X11CharacterInjector(base::MakeUnique<X11KeyboardImpl>(display_))); On 2016/09/26 17:57:19, Yuwei wrote: > I think passing a unique_ptr<X11Keyboard> can avoid macro pollution by including > X11 stuff in CharacterInjector's header file. I agree. https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... File remoting/host/linux/x11_character_injector.cc (right): https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector.cc:74: scheduled_consume_time_ > expected_consume_time) { I don't think you really need this condition. Timer is always scheduled for the earliest moment when we will have a key is expected to be available. So this function could just check that the consume_timer_ is scheduled or not, if it is then it can just return. No need to compare scheduled compute time. I don't think there is a case when the timer needs to be moved forward. https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector.cc:78: scheduled_consume_time_ = expected_consume_time; OneShotTimer::desired_run_time() can be used instead of scheduled_consumer_time_, but you don't really need either of them - see my comment above. https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector.cc:81: void X11CharacterInjector::Consume() { Maybe call this function DoInject()? https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector.cc:91: continue; This doesn't look right. If MapCharacter() fails to map a character this function would loop indefinitely. Move character_queue_.pop() above the MapCharacter() call? https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector.cc:147: void X11CharacterInjector::ResetKeyInfoExpirationTime( Also pass base::TimeTicks::Now() value to avoid calling Now() multiple times when MapCharacter() is executed. https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector.cc:150: while (position + 1 < available_keycodes_.end() && Shouldn't this always move the key to the end of the queue (kMappingExpireDuration is the same for all keys and Now() increasing)? i.e. you just need to erase() it from the current position and then push_back() to the end? https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... File remoting/host/linux/x11_character_injector.h (right): https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector.h:64: base::WeakPtrFactory<X11CharacterInjector> weak_factory_; weak_factory_ is not used anywhere. Remove it? https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... File remoting/host/linux/x11_character_injector_unittest.cc (right): https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector_unittest.cc:35: class FakeX11Keyboard : public X11Keyboard { Add a short comment here to explain how this class works. https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector_unittest.cc:50: std::vector<uint32_t> GetUnusedKeycodes() override; // X11Keyboard interface. https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector_unittest.cc:72: std::map<uint32_t, uint32_t> keycode_mapping_; unordered_map (not that performance matters here, but map<> is should be avoided almost everywhere) https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector_unittest.cc:155: usleep(kSyncDelayMs * 1000); Why do we need to sleep here? https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector_unittest.cc:160: for (const auto& expectation : sequence) { keypress_expectations_.insert( keypress_expectations_.begin(), sequence.begin(), sequence.end()); https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector_unittest.cc:167: for (const auto& expectation : sequence) { use std::vector::insert() https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector_unittest.cc:285: auto inject_and_wait = [this](uint32_t keycode, Make this a class method instead of a closure? https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector_unittest.cc:297: inject_and_wait(54, 2); These tests are too rigorous in the way they set expectations which makes them brittle. E.g. they verify X11CharacterInjector uses available keycodes in a specific order. If X11CharacterInjector is ever changed to use a different container to store available keys then it may use them in different order. The code would still be correct, but this test will break. All that matters for X11CharacterInjector is that correct characters get injected. Which keycodes are used for them is irrelevant.
https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... File remoting/host/linux/x11_character_injector.cc (right): https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector.cc:74: scheduled_consume_time_ > expected_consume_time) { On 2016/09/27 18:40:48, Sergey Ulanov wrote: > I don't think you really need this condition. Timer is always scheduled for the > earliest moment when we will have a key is expected to be available. So this > function could just check that the consume_timer_ is scheduled or not, if it is > then it can just return. No need to compare scheduled compute time. I don't > think there is a case when the timer needs to be moved forward. My concern was if there is an upcoming Consume() scheduled by Inject() then it should be delayed. I think you are right. This won't happen since only one Consume() can be scheduled at a time. https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector.cc:91: continue; On 2016/09/27 18:40:48, Sergey Ulanov wrote: > This doesn't look right. If MapCharacter() fails to map a character this > function would loop indefinitely. Move character_queue_.pop() above the > MapCharacter() call? Yep. You just caught a bug :P
Patchset #17 (id:440001) has been deleted
The CQ bit was checked by yuweih@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...
PTAL Loosen the test a bit to only check the sequence of injected characters rather than sequence of changing layout and pressing keycode. https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... File remoting/host/linux/x11_character_injector.cc (right): https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector.cc:74: scheduled_consume_time_ > expected_consume_time) { On 2016/09/27 18:40:48, Sergey Ulanov wrote: > I don't think you really need this condition. Timer is always scheduled for the > earliest moment when we will have a key is expected to be available. So this > function could just check that the consume_timer_ is scheduled or not, if it is > then it can just return. No need to compare scheduled compute time. I don't > think there is a case when the timer needs to be moved forward. Removed. https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector.cc:78: scheduled_consume_time_ = expected_consume_time; On 2016/09/27 18:40:48, Sergey Ulanov wrote: > OneShotTimer::desired_run_time() can be used instead of > scheduled_consumer_time_, but you don't really need either of them - see my > comment above. Acknowledged. https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector.cc:81: void X11CharacterInjector::Consume() { On 2016/09/27 18:40:48, Sergey Ulanov wrote: > Maybe call this function DoInject()? Done. https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector.cc:91: continue; On 2016/09/27 18:40:48, Sergey Ulanov wrote: > This doesn't look right. If MapCharacter() fails to map a character this > function would loop indefinitely. Move character_queue_.pop() above the > MapCharacter() call? Fixed this by adding a pop before continue. Moving pop above MapCharacter doesn't quite work since the schedule-and-break route should not pop the character... https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector.cc:147: void X11CharacterInjector::ResetKeyInfoExpirationTime( On 2016/09/27 18:40:48, Sergey Ulanov wrote: > Also pass base::TimeTicks::Now() value to avoid calling Now() multiple times > when MapCharacter() is executed. Done. https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector.cc:150: while (position + 1 < available_keycodes_.end() && On 2016/09/27 18:40:48, Sergey Ulanov wrote: > Shouldn't this always move the key to the end of the queue > (kMappingExpireDuration is the same for all keys and Now() increasing)? Mostly yes. The only case it doesn't work is when multiple KeyInfo's have the same expiry time, then the one with lower keycode should be the last. I think the preference of higher keycode is barely useful (as long as GetUnusedKeycode returns the list in descending order) so I'll just remove that field from the sorting criteria. > i.e. you > just need to erase() it from the current position and then push_back() to the > end? Yep. Done. https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... File remoting/host/linux/x11_character_injector.h (right): https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector.h:64: base::WeakPtrFactory<X11CharacterInjector> weak_factory_; On 2016/09/27 18:40:48, Sergey Ulanov wrote: > weak_factory_ is not used anywhere. Remove it? Done. https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... File remoting/host/linux/x11_character_injector_unittest.cc (right): https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector_unittest.cc:35: class FakeX11Keyboard : public X11Keyboard { On 2016/09/27 18:40:49, Sergey Ulanov wrote: > Add a short comment here to explain how this class works. Done. https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector_unittest.cc:50: std::vector<uint32_t> GetUnusedKeycodes() override; On 2016/09/27 18:40:49, Sergey Ulanov wrote: > // X11Keyboard interface. Done. https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector_unittest.cc:72: std::map<uint32_t, uint32_t> keycode_mapping_; On 2016/09/27 18:40:49, Sergey Ulanov wrote: > unordered_map Done. > (not that performance matters here, but map<> is should be avoided almost > everywhere) Okay... These names are confusing and sounds like unordered_map is a subclass of map... https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector_unittest.cc:155: usleep(kSyncDelayMs * 1000); On 2016/09/27 18:40:49, Sergey Ulanov wrote: > Why do we need to sleep here? This was a simulation of sync delay and can help for counting how many Sync is called for entering each character. Just removed this delay since now we don't care about details like calling order or counts. https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector_unittest.cc:160: for (const auto& expectation : sequence) { On 2016/09/27 18:40:49, Sergey Ulanov wrote: > keypress_expectations_.insert( > keypress_expectations_.begin(), sequence.begin(), sequence.end()); Done in ExpectEnterCodePoints(). Changed queue to deque to get the iterators. https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector_unittest.cc:167: for (const auto& expectation : sequence) { On 2016/09/27 18:40:49, Sergey Ulanov wrote: > use std::vector::insert() Obsolete. https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector_unittest.cc:285: auto inject_and_wait = [this](uint32_t keycode, On 2016/09/27 18:40:49, Sergey Ulanov wrote: > Make this a class method instead of a closure? Made InjectAndRun function https://codereview.chromium.org/2346643003/diff/420001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector_unittest.cc:297: inject_and_wait(54, 2); On 2016/09/27 18:40:49, Sergey Ulanov wrote: > These tests are too rigorous in the way they set expectations which makes them > brittle. E.g. they verify X11CharacterInjector uses available keycodes in a > specific order. If X11CharacterInjector is ever changed to use a different > container to store available keys then it may use them in different order. I would say different ordering rules? Changing data structure may not necessarily change the order. > The > code would still be correct, but this test will break. All that matters for > X11CharacterInjector is that correct characters get injected. Which keycodes are > used for them is irrelevant. Agreed. I have loosened the test a bit to make it check the sequence of characters being injected instead.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for cleaning up the test! LGTM https://codereview.chromium.org/2346643003/diff/460001/remoting/host/linux/x1... File remoting/host/linux/x11_character_injector_unittest.cc (right): https://codereview.chromium.org/2346643003/diff/460001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector_unittest.cc:17: const base::TimeDelta kKeycodeReuseDuration = Please use constexpr for TimeDelta consts. indentation. https://codereview.chromium.org/2346643003/diff/460001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector_unittest.cc:71: DCHECK(expected_code_point_sequence_.empty()); EXPECT_TRUE()
Thanks! https://codereview.chromium.org/2346643003/diff/460001/remoting/host/linux/x1... File remoting/host/linux/x11_character_injector_unittest.cc (right): https://codereview.chromium.org/2346643003/diff/460001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector_unittest.cc:17: const base::TimeDelta kKeycodeReuseDuration = On 2016/10/03 17:41:56, Sergey Ulanov wrote: > Please use constexpr for TimeDelta consts. > indentation. Done. https://codereview.chromium.org/2346643003/diff/460001/remoting/host/linux/x1... remoting/host/linux/x11_character_injector_unittest.cc:71: DCHECK(expected_code_point_sequence_.empty()); On 2016/10/03 17:41:56, Sergey Ulanov wrote: > EXPECT_TRUE() Done.
The CQ bit was checked by yuweih@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2346643003/#ps480001 (title: "Reviewer's Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Remoting Host] Handle text event characters that are not presented on the keyboard This CL allows the x11 injector to remap the last keycode to the character if the character is not on the current keyboard layout. BUG=645659 ========== to ========== [Remoting Host] Handle text event characters that are not presented on the keyboard This CL allows the x11 injector to remap the last keycode to the character if the character is not on the current keyboard layout. BUG=645659 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:480001)
Message was sent while issue was closed.
Description was changed from ========== [Remoting Host] Handle text event characters that are not presented on the keyboard This CL allows the x11 injector to remap the last keycode to the character if the character is not on the current keyboard layout. BUG=645659 ========== to ========== [Remoting Host] Handle text event characters that are not presented on the keyboard This CL allows the x11 injector to remap the last keycode to the character if the character is not on the current keyboard layout. BUG=645659 Committed: https://crrev.com/ab975ef6a5e726359982765e311712f3a0974831 Cr-Commit-Position: refs/heads/master@{#422585} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/ab975ef6a5e726359982765e311712f3a0974831 Cr-Commit-Position: refs/heads/master@{#422585} |
