Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2925353002:
Implement sync of keyboard layout between Ozone and Wayland clients (Closed)
|
Created:
3 years, 6 months ago by Jason D. Clinton Modified:
3 years, 6 months ago Reviewers:
jclinton, reveman, sky, jam, Seigo Nonaka, Nico, denniskempin (chromium), sadrul, Shu Chen CC:
achuith+watch_chromium.org, alemate+watch_chromium.org, chromium-reviews, kalyank, nona+watch_chromium.org, oshima+watch_chromium.org, shuchen+watch_chromium.org, sjg, James Su, yusukes+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement sync of keyboard layout between Ozone and Wayland clients
BUG=chromium:707429, b:27877642
TEST=install Crouton environment, run gtk3 apps with GDK Wayland
backend, confirmed layout is correct at first launch and after toggling
layout, confirmed no new crashes introduced (tried US, US Dvorak and
Latin American layouts). toggling layout after closing Wayland app
doesn't crash Chrome (observer released).
Review-Url: https://codereview.chromium.org/2925353002
Cr-Commit-Position: refs/heads/master@{#479825}
Committed: https://chromium.googlesource.com/chromium/src/+/2870fad48915fa547f6653e7cdd7afd04cada8fa
Patch Set 1 #
Total comments: 50
Patch Set 2 : Address reveman comments #
Total comments: 20
Patch Set 3 : Address reveman comments round 2 #
Total comments: 16
Patch Set 4 : Address reveman comments round 3 #
Total comments: 4
Patch Set 5 : Address final comments #
Total comments: 8
Patch Set 6 : Rebase on master #Patch Set 7 : Address final comments round 2 #Patch Set 8 : Add unittest for observer functionality #Patch Set 9 : Fix -Wall build failure #
Total comments: 16
Patch Set 10 : Remove comments #Patch Set 11 : Fix potential nullptr deref #Patch Set 12 : Rebase on master #
Messages
Total messages: 85 (46 generated)
jclinton@google.com changed reviewers: + denniskempin@chromium.org, jclinton@google.com, reveman@chromium.org
Description was changed from ========== Implement sync of keyboard layout between Ozone and Wayland clients The impl first initializes to the default keyboard layout (current behavior) and then attempts to synchronize the keyboard layout through invoking the callback. BUG=chromium:707429,b:27877642 TEST=install Crouton environment, run gtk3 apps with GDK Wayland backend, confirmed layout is correct at first launch and after toggling layout, confirmed no new crashes introduced (tried US, US Dvorak and Latin American layouts). toggling layout after closing Wayland app doesn't crash Chrome (observer released). ========== to ========== Implement sync of keyboard layout between Ozone and Wayland clients The impl first initializes to the default keyboard layout (current behavior) and then attempts to synchronize the keyboard layout through invoking the callback. BUG=chromium:707429,b:27877642 TEST=install Crouton environment, run gtk3 apps with GDK Wayland backend, confirmed layout is correct at first launch and after toggling layout, confirmed no new crashes introduced (tried US, US Dvorak and Latin American layouts). toggling layout after closing Wayland app doesn't crash Chrome (observer released). ==========
https://codereview.chromium.org/2925353002/diff/1/ash/system/tray_caps_lock.h File ash/system/tray_caps_lock.h (right): https://codereview.chromium.org/2925353002/diff/1/ash/system/tray_caps_lock.h... ash/system/tray_caps_lock.h:30: // input_method::ImeKeyboard::Observer: we are not interested in layout: Simply "// Overridden from input_method::ImeKeyboard::Observer:" as I don't think the comment adds anything. If needed then put on the next line. https://codereview.chromium.org/2925353002/diff/1/ash/system/tray_caps_lock.h... ash/system/tray_caps_lock.h:31: void OnLayoutChanged(const std::string& layout_name) override{}; nit: s/override{};/override {}/ https://codereview.chromium.org/2925353002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right): https://codereview.chromium.org/2925353002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:462: // input_method::ImeKeyboard::Observer: we are not interested in layout: Simply "// input_method::ImeKeyboard::Observer implementation:" as I don't think the comment adds anything. If needed then put on the next line. https://codereview.chromium.org/2925353002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:463: void OnLayoutChanged(const std::string& layout_name) override{}; nit: s/override{};/override {}/ https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:72: #include "ui/base/ime/chromeos/ime_keyboard.h" we don't assume that this code is only built on chromeos. please move to chromeos ifdef below https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:80: #include "ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h" move this to #if defined(USE_OZONE) below https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2745: public chromeos::input_method::ImeKeyboard::Observer { I think this needs to be protected by a chromeos ifdef https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2765: chromeos::input_method::ImeKeyboard* keyboard = needs a chromeos ifdef https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2767: nit: no need for this blank line https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2768: if (keyboard) { when is this null? can we DCHECK instead? https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2770: keyboard->ReapplyCurrentKeyboardLayout(); Does this result in another wl_keyboard_send_keymap call? In that case please avoid the call above. https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2773: nit: no need for this blank line https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2774: ~WaylandKeyboardDelegate() { ~WaylandKeyboardDelegate() override ? as this inherits https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2775: chromeos::input_method::ImeKeyboard* keyboard = this needs ifdefs https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2777: nit: not need for blank line https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2778: if (keyboard) { DCHECK instead if possible https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2780: } nit need for {} https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2838: // input_method::ImeKeyboard::Observer: we are not interested in caps lock: "// Overridden from input_method::ImeKeyboard::Observer:" https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2840: remove this blank line. all input_method::ImeKeyboard::Observer overrides should be in one group https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2841: // input_method::ImeKeyboard::Observer implementation: remove comment as we should have the "Overridden from.." above https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2866: }; s/};/}/ and all this needs to be behind and ifdef https://codereview.chromium.org/2925353002/diff/1/ui/base/ime/chromeos/ime_ke... File ui/base/ime/chromeos/ime_keyboard.cc (right): https://codereview.chromium.org/2925353002/diff/1/ui/base/ime/chromeos/ime_ke... ui/base/ime/chromeos/ime_keyboard.cc:80: for (ImeKeyboard::Observer& observer : observers_) { nit: no need for {} https://codereview.chromium.org/2925353002/diff/1/ui/base/ime/chromeos/ime_ke... File ui/base/ime/chromeos/ime_keyboard.h (right): https://codereview.chromium.org/2925353002/diff/1/ui/base/ime/chromeos/ime_ke... ui/base/ime/chromeos/ime_keyboard.h:41: virtual void OnCapsLockChanged(bool enabled) = 0; blank line after this https://codereview.chromium.org/2925353002/diff/1/ui/base/ime/chromeos/ime_ke... ui/base/ime/chromeos/ime_keyboard.h:42: virtual void OnLayoutChanged(const std::string& layout_name) = 0; Add a short comment above this. "// Called when the layout state has changed."
Addressed all comments and manually re-ran test cases. https://codereview.chromium.org/2925353002/diff/1/ash/system/tray_caps_lock.h File ash/system/tray_caps_lock.h (right): https://codereview.chromium.org/2925353002/diff/1/ash/system/tray_caps_lock.h... ash/system/tray_caps_lock.h:30: // input_method::ImeKeyboard::Observer: we are not interested in layout: On 2017/06/09 14:53:17, reveman wrote: > Simply "// Overridden from input_method::ImeKeyboard::Observer:" as I don't > think the comment adds anything. If needed then put on the next line. Done. https://codereview.chromium.org/2925353002/diff/1/ash/system/tray_caps_lock.h... ash/system/tray_caps_lock.h:31: void OnLayoutChanged(const std::string& layout_name) override{}; On 2017/06/09 14:53:17, reveman wrote: > nit: s/override{};/override {}/ Done. https://codereview.chromium.org/2925353002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right): https://codereview.chromium.org/2925353002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:462: // input_method::ImeKeyboard::Observer: we are not interested in layout: On 2017/06/09 14:53:17, reveman wrote: > Simply "// input_method::ImeKeyboard::Observer implementation:" as I don't think > the comment adds anything. If needed then put on the next line. Done. https://codereview.chromium.org/2925353002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:463: void OnLayoutChanged(const std::string& layout_name) override{}; On 2017/06/09 14:53:17, reveman wrote: > nit: s/override{};/override {}/ Done. https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:72: #include "ui/base/ime/chromeos/ime_keyboard.h" On 2017/06/09 14:53:18, reveman wrote: > we don't assume that this code is only built on chromeos. please move to > chromeos ifdef below Done. https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:80: #include "ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.h" On 2017/06/09 14:53:18, reveman wrote: > move this to #if defined(USE_OZONE) below Done. https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2745: public chromeos::input_method::ImeKeyboard::Observer { On 2017/06/09 14:53:19, reveman wrote: > I think this needs to be protected by a chromeos ifdef Done. https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2765: chromeos::input_method::ImeKeyboard* keyboard = On 2017/06/09 14:53:18, reveman wrote: > needs a chromeos ifdef Done. https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2767: On 2017/06/09 14:53:18, reveman wrote: > nit: no need for this blank line Done. https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2768: if (keyboard) { On 2017/06/09 14:53:19, reveman wrote: > when is this null? can we DCHECK instead? It returns null here: https://cs.chromium.org/chromium/src/ui/base/ime/chromeos/mock_input_method_m... https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2770: keyboard->ReapplyCurrentKeyboardLayout(); On 2017/06/09 14:53:18, reveman wrote: > Does this result in another wl_keyboard_send_keymap call? In that case please > avoid the call above. Yes, it does but we can no longer avoid it after the introduction of the #ifdef requested above. Before, this was here to defensively attempt to preserve some kind of consistent state in cases where they keyboard layout cannot be determined. After the introduction of the #ifdef, that's still the case but now it's mandatory. https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2773: On 2017/06/09 14:53:18, reveman wrote: > nit: no need for this blank line Done. https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2774: ~WaylandKeyboardDelegate() { On 2017/06/09 14:53:18, reveman wrote: > ~WaylandKeyboardDelegate() override ? as this inherits Done. https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2775: chromeos::input_method::ImeKeyboard* keyboard = On 2017/06/09 14:53:18, reveman wrote: > this needs ifdefs Done. https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2777: On 2017/06/09 14:53:18, reveman wrote: > nit: not need for blank line Done. https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2778: if (keyboard) { On 2017/06/09 14:53:18, reveman wrote: > DCHECK instead if possible Acknowledged. https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2780: } On 2017/06/09 14:53:18, reveman wrote: > nit need for {} Done. https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2838: // input_method::ImeKeyboard::Observer: we are not interested in caps lock: On 2017/06/09 14:53:18, reveman wrote: > "// Overridden from input_method::ImeKeyboard::Observer:" Done. https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2840: On 2017/06/09 14:53:18, reveman wrote: > remove this blank line. all input_method::ImeKeyboard::Observer overrides should > be in one group Done. https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2841: // input_method::ImeKeyboard::Observer implementation: On 2017/06/09 14:53:18, reveman wrote: > remove comment as we should have the "Overridden from.." above Done. https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2866: }; On 2017/06/09 14:53:18, reveman wrote: > s/};/}/ and all this needs to be behind and ifdef Done. https://codereview.chromium.org/2925353002/diff/1/ui/base/ime/chromeos/ime_ke... File ui/base/ime/chromeos/ime_keyboard.cc (right): https://codereview.chromium.org/2925353002/diff/1/ui/base/ime/chromeos/ime_ke... ui/base/ime/chromeos/ime_keyboard.cc:80: for (ImeKeyboard::Observer& observer : observers_) { On 2017/06/09 14:53:19, reveman wrote: > nit: no need for {} Done. https://codereview.chromium.org/2925353002/diff/1/ui/base/ime/chromeos/ime_ke... File ui/base/ime/chromeos/ime_keyboard.h (right): https://codereview.chromium.org/2925353002/diff/1/ui/base/ime/chromeos/ime_ke... ui/base/ime/chromeos/ime_keyboard.h:41: virtual void OnCapsLockChanged(bool enabled) = 0; On 2017/06/09 14:53:19, reveman wrote: > blank line after this Done. https://codereview.chromium.org/2925353002/diff/1/ui/base/ime/chromeos/ime_ke... ui/base/ime/chromeos/ime_keyboard.h:42: virtual void OnLayoutChanged(const std::string& layout_name) = 0; On 2017/06/09 14:53:19, reveman wrote: > Add a short comment above this. "// Called when the layout state has changed." Done.
The CQ bit was checked by jclinton@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2768: if (keyboard) { On 2017/06/09 at 16:36:03, jclinton wrote: > On 2017/06/09 14:53:19, reveman wrote: > > when is this null? can we DCHECK instead? > > It returns null here: > https://cs.chromium.org/chromium/src/ui/base/ime/chromeos/mock_input_method_m... Ack https://codereview.chromium.org/2925353002/diff/1/components/exo/wayland/serv... components/exo/wayland/server.cc:2770: keyboard->ReapplyCurrentKeyboardLayout(); On 2017/06/09 at 16:36:03, jclinton wrote: > On 2017/06/09 14:53:18, reveman wrote: > > Does this result in another wl_keyboard_send_keymap call? In that case please > > avoid the call above. > > Yes, it does but we can no longer avoid it after the introduction of the #ifdef requested above. Before, this was here to defensively attempt to preserve some kind of consistent state in cases where they keyboard layout cannot be determined. After the introduction of the #ifdef, that's still the case but now it's mandatory. Not sure I follow. Looks like we just need to refactor this a bit. See my comments on latest patch. https://codereview.chromium.org/2925353002/diff/20001/ash/system/tray_caps_lo... File ash/system/tray_caps_lock.h (right): https://codereview.chromium.org/2925353002/diff/20001/ash/system/tray_caps_lo... ash/system/tray_caps_lock.h:30: // Overriden from input_method::ImeKeyboard::Observer s/Overriden/Overridden/ and s/::Observer/::Observer:/ https://codereview.chromium.org/2925353002/diff/20001/ash/system/tray_caps_lo... ash/system/tray_caps_lock.h:31: void OnLayoutChanged(const std::string& layout_name) override {}; remove ";" https://codereview.chromium.org/2925353002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right): https://codereview.chromium.org/2925353002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:462: // Overridden from input_method::ImeKeyboard::Observer "input_method::ImeKeyboard::Observer implementation:" to be consistent with l.459 https://codereview.chromium.org/2925353002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:463: void OnLayoutChanged(const std::string& layout_name) override {}; remove ";' https://codereview.chromium.org/2925353002/diff/20001/components/exo/wayland/... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2925353002/diff/20001/components/exo/wayland/... components/exo/wayland/server.cc:88: #include "ui/base/ime/chromeos/ime_keyboard.h" think we need another ifdef for this file. ozone can be used without chromeos. https://codereview.chromium.org/2925353002/diff/20001/components/exo/wayland/... components/exo/wayland/server.cc:89: #include "ui/base/ime/chromeos/input_method_manager.h" ditto https://codereview.chromium.org/2925353002/diff/20001/components/exo/wayland/... components/exo/wayland/server.cc:2746: , public chromeos::input_method::ImeKeyboard::Observer ozone can be used without chromeos https://codereview.chromium.org/2925353002/diff/20001/components/exo/wayland/... components/exo/wayland/server.cc:2773: keyboard->ReapplyCurrentKeyboardLayout(); See my SendLayout() comment below. Can we call that after the ifdef instead of calling ReapplyCurrentKeyboardLayout here? https://codereview.chromium.org/2925353002/diff/20001/components/exo/wayland/... components/exo/wayland/server.cc:2842: void OnLayoutChanged(const std::string& layout_name) override { This duplicates a lot of code in the ctor. Please move the common code into a SendLayout() function that can be called from the ctor. https://codereview.chromium.org/2925353002/diff/20001/components/exo/wayland/... components/exo/wayland/server.cc:2866: }; remove ";"
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
Addressed all comments and re-ran manual tests. https://codereview.chromium.org/2925353002/diff/20001/ash/system/tray_caps_lo... File ash/system/tray_caps_lock.h (right): https://codereview.chromium.org/2925353002/diff/20001/ash/system/tray_caps_lo... ash/system/tray_caps_lock.h:30: // Overriden from input_method::ImeKeyboard::Observer On 2017/06/09 16:50:12, reveman wrote: > s/Overriden/Overridden/ and s/::Observer/::Observer:/ Done. https://codereview.chromium.org/2925353002/diff/20001/ash/system/tray_caps_lo... ash/system/tray_caps_lock.h:31: void OnLayoutChanged(const std::string& layout_name) override {}; On 2017/06/09 16:50:12, reveman wrote: > remove ";" Done. https://codereview.chromium.org/2925353002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right): https://codereview.chromium.org/2925353002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:462: // Overridden from input_method::ImeKeyboard::Observer On 2017/06/09 16:50:12, reveman wrote: > "input_method::ImeKeyboard::Observer implementation:" to be consistent with > l.459 Done. https://codereview.chromium.org/2925353002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:463: void OnLayoutChanged(const std::string& layout_name) override {}; On 2017/06/09 16:50:12, reveman wrote: > remove ";' Done. https://codereview.chromium.org/2925353002/diff/20001/components/exo/wayland/... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2925353002/diff/20001/components/exo/wayland/... components/exo/wayland/server.cc:88: #include "ui/base/ime/chromeos/ime_keyboard.h" On 2017/06/09 16:50:13, reveman wrote: > think we need another ifdef for this file. ozone can be used without chromeos. Done. https://codereview.chromium.org/2925353002/diff/20001/components/exo/wayland/... components/exo/wayland/server.cc:89: #include "ui/base/ime/chromeos/input_method_manager.h" On 2017/06/09 16:50:13, reveman wrote: > ditto Done. https://codereview.chromium.org/2925353002/diff/20001/components/exo/wayland/... components/exo/wayland/server.cc:2746: , public chromeos::input_method::ImeKeyboard::Observer On 2017/06/09 16:50:12, reveman wrote: > ozone can be used without chromeos Done. https://codereview.chromium.org/2925353002/diff/20001/components/exo/wayland/... components/exo/wayland/server.cc:2773: keyboard->ReapplyCurrentKeyboardLayout(); On 2017/06/09 16:50:13, reveman wrote: > See my SendLayout() comment below. Can we call that after the ifdef instead of > calling ReapplyCurrentKeyboardLayout here? Done. https://codereview.chromium.org/2925353002/diff/20001/components/exo/wayland/... components/exo/wayland/server.cc:2842: void OnLayoutChanged(const std::string& layout_name) override { On 2017/06/09 16:50:13, reveman wrote: > This duplicates a lot of code in the ctor. Please move the common code into a > SendLayout() function that can be called from the ctor. Done. https://codereview.chromium.org/2925353002/diff/20001/components/exo/wayland/... components/exo/wayland/server.cc:2866: }; On 2017/06/09 16:50:12, reveman wrote: > remove ";" Done.
The CQ bit was checked by jclinton@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2925353002/diff/40001/components/exo/wayland/... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2925353002/diff/40001/components/exo/wayland/... components/exo/wayland/server.cc:2747: #if defined(OS_CHROMEOS) #if defined(USE_OZONE) && defined(OS_CHROMEOS) as chromeos can still be build without ozone afaik https://codereview.chromium.org/2925353002/diff/40001/components/exo/wayland/... components/exo/wayland/server.cc:2755: #if defined(OS_CHROMEOS) #if defined(USE_OZONE) && defined(OS_CHROMEOS) https://codereview.chromium.org/2925353002/diff/40001/components/exo/wayland/... components/exo/wayland/server.cc:2768: .options = ""}; Please avoid duplication of above code. Maybe add a SendCurrentLayout function that calls SendLayout. https://codereview.chromium.org/2925353002/diff/40001/components/exo/wayland/... components/exo/wayland/server.cc:2776: #else ifdef that spans multiple functions like this is confusing. please have one ifdef in the ctor and one in the dtor instead https://codereview.chromium.org/2925353002/diff/40001/components/exo/wayland/... components/exo/wayland/server.cc:2836: #if defined(OS_CHROMEOS) #if defined(USE_OZONE) && defined(OS_CHROMEOS) https://codereview.chromium.org/2925353002/diff/40001/components/exo/wayland/... components/exo/wayland/server.cc:2889: // Send the current keyboard layout to the Wayland client End sentence with "." and remove Wayland, "to the client." is enough https://codereview.chromium.org/2925353002/diff/40001/components/exo/wayland/... components/exo/wayland/server.cc:2890: void SendLayout(const struct xkb_rule_names& names) { "struct" is not needed in c++ code. and "const xkb_rule_names* names" as you pass nullptr in some cases https://codereview.chromium.org/2925353002/diff/40001/ui/base/ime/chromeos/im... File ui/base/ime/chromeos/ime_keyboard.h (right): https://codereview.chromium.org/2925353002/diff/40001/ui/base/ime/chromeos/im... ui/base/ime/chromeos/ime_keyboard.h:44: virtual void OnLayoutChanged(const std::string& layout_name) = 0; OnLayoutChanging as GetCurrentKeyboardLayoutName() still returns the old layout. Alternatively keep the current name and make sure GetCurrentKeyboardLayoutName() returns the new layout when this is called.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Addressed all comments and re-ran manual tests. https://codereview.chromium.org/2925353002/diff/40001/components/exo/wayland/... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2925353002/diff/40001/components/exo/wayland/... components/exo/wayland/server.cc:2747: #if defined(OS_CHROMEOS) On 2017/06/09 21:15:18, reveman wrote: > #if defined(USE_OZONE) && defined(OS_CHROMEOS) > > as chromeos can still be build without ozone afaik Done. https://codereview.chromium.org/2925353002/diff/40001/components/exo/wayland/... components/exo/wayland/server.cc:2755: #if defined(OS_CHROMEOS) On 2017/06/09 21:15:18, reveman wrote: > #if defined(USE_OZONE) && defined(OS_CHROMEOS) Done. https://codereview.chromium.org/2925353002/diff/40001/components/exo/wayland/... components/exo/wayland/server.cc:2768: .options = ""}; On 2017/06/09 21:15:18, reveman wrote: > Please avoid duplication of above code. Maybe add a SendCurrentLayout function > that calls SendLayout. Done. https://codereview.chromium.org/2925353002/diff/40001/components/exo/wayland/... components/exo/wayland/server.cc:2776: #else On 2017/06/09 21:15:18, reveman wrote: > ifdef that spans multiple functions like this is confusing. please have one > ifdef in the ctor and one in the dtor instead Done. https://codereview.chromium.org/2925353002/diff/40001/components/exo/wayland/... components/exo/wayland/server.cc:2836: #if defined(OS_CHROMEOS) On 2017/06/09 21:15:18, reveman wrote: > #if defined(USE_OZONE) && defined(OS_CHROMEOS) Done. https://codereview.chromium.org/2925353002/diff/40001/components/exo/wayland/... components/exo/wayland/server.cc:2889: // Send the current keyboard layout to the Wayland client On 2017/06/09 21:15:18, reveman wrote: > End sentence with "." and remove Wayland, "to the client." is enough Done. https://codereview.chromium.org/2925353002/diff/40001/components/exo/wayland/... components/exo/wayland/server.cc:2890: void SendLayout(const struct xkb_rule_names& names) { On 2017/06/09 21:15:18, reveman wrote: > "struct" is not needed in c++ code. and "const xkb_rule_names* names" as you > pass nullptr in some cases Done. https://codereview.chromium.org/2925353002/diff/40001/ui/base/ime/chromeos/im... File ui/base/ime/chromeos/ime_keyboard.h (right): https://codereview.chromium.org/2925353002/diff/40001/ui/base/ime/chromeos/im... ui/base/ime/chromeos/ime_keyboard.h:44: virtual void OnLayoutChanged(const std::string& layout_name) = 0; On 2017/06/09 21:15:18, reveman wrote: > OnLayoutChanging as GetCurrentKeyboardLayoutName() still returns the old layout. > Alternatively keep the current name and make sure GetCurrentKeyboardLayoutName() > returns the new layout when this is called. Done.
The CQ bit was checked by jclinton@google.com 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...
lgtm % nits https://codereview.chromium.org/2925353002/diff/60001/components/exo/wayland/... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2925353002/diff/60001/components/exo/wayland/... components/exo/wayland/server.cc:2877: // Send the keyboard layout named to the client. nit: Send the named keyboard layout to the.. https://codereview.chromium.org/2925353002/diff/60001/components/exo/wayland/... components/exo/wayland/server.cc:2878: void SendLayoutName(const std::string& layout_name) { nit: SendNamedLayout or SendLayoutWithName as we're not sending the name but the named layout
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2925353002/diff/60001/components/exo/wayland/... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2925353002/diff/60001/components/exo/wayland/... components/exo/wayland/server.cc:2877: // Send the keyboard layout named to the client. On 2017/06/09 22:59:11, reveman wrote: > nit: Send the named keyboard layout to the.. Done. https://codereview.chromium.org/2925353002/diff/60001/components/exo/wayland/... components/exo/wayland/server.cc:2878: void SendLayoutName(const std::string& layout_name) { On 2017/06/09 22:59:11, reveman wrote: > nit: SendNamedLayout or SendLayoutWithName as we're not sending the name but the > named layout Done.
The CQ bit was checked by jclinton@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2925353002/#ps80001 (title: "Address final comments")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Implement sync of keyboard layout between Ozone and Wayland clients The impl first initializes to the default keyboard layout (current behavior) and then attempts to synchronize the keyboard layout through invoking the callback. BUG=chromium:707429,b:27877642 TEST=install Crouton environment, run gtk3 apps with GDK Wayland backend, confirmed layout is correct at first launch and after toggling layout, confirmed no new crashes introduced (tried US, US Dvorak and Latin American layouts). toggling layout after closing Wayland app doesn't crash Chrome (observer released). ========== to ========== Implement sync of keyboard layout between Ozone and Wayland clients BUG=chromium:707429,b:27877642 TEST=install Crouton environment, run gtk3 apps with GDK Wayland backend, confirmed layout is correct at first launch and after toggling layout, confirmed no new crashes introduced (tried US, US Dvorak and Latin American layouts). toggling layout after closing Wayland app doesn't crash Chrome (observer released). ==========
jclinton@google.com changed reviewers: + jam@chromium.org, nona@chromium.org, sky@chromium.org
shuchen@chromium.org changed reviewers: + shuchen@chromium.org
Forgot to change ime_keyboard_x11 & fake_ime_keyboard? With the update of fake_ime_keyboard, please add test to cover the observer logic.
a few more nits, sorry I missed these the first time https://codereview.chromium.org/2925353002/diff/80001/components/exo/wayland/... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2925353002/diff/80001/components/exo/wayland/... components/exo/wayland/server.cc:2760: std::string layout_name = keyboard->GetCurrentKeyboardLayoutName(); nit: remove this temporary variable after making GetCurrentKeyboardLayoutName return a reference https://codereview.chromium.org/2925353002/diff/80001/components/exo/wayland/... components/exo/wayland/server.cc:2882: xkb_rule_names names = {.rules = NULL, nit: s/NULL/nullptr/ https://codereview.chromium.org/2925353002/diff/80001/components/exo/wayland/... components/exo/wayland/server.cc:2924: std::unique_ptr<xkb_keymap, ui::XkbKeymapDeleter> xkb_keymap_; Btw, can you make xkb_keymap_ and xkb_state_ local variables in SendLayout as part of this patch? Looks like they never had to be members. https://codereview.chromium.org/2925353002/diff/80001/ui/base/ime/chromeos/im... File ui/base/ime/chromeos/ime_keyboard.h (right): https://codereview.chromium.org/2925353002/diff/80001/ui/base/ime/chromeos/im... ui/base/ime/chromeos/ime_keyboard.h:58: const std::string GetCurrentKeyboardLayoutName() { return last_layout_; } "const std::string& GetCurrentKeyboardLayoutName() const" to avoid copies
On 2017/06/10 04:58:55, Shu Chen wrote: > Forgot to change ime_keyboard_x11 No, ime_keyboard_x11 is dead code. I don't know why it hasn't been deleted yet but that's outside of the scope of this code review. > & fake_ime_keyboard? I didn't forget: implementing that will break the expectations associated with the rest of that file: the rest of the file doesn't implement any logic at all. But fine, done. > With the update of fake_ime_keyboard, please add test to cover the observer > logic. There are no tests for any ime_keyboard* code. A test of the observer logic would require adding a test framework for this directory and would provide no value: the code changed here is trivial.
https://codereview.chromium.org/2925353002/diff/80001/components/exo/wayland/... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2925353002/diff/80001/components/exo/wayland/... components/exo/wayland/server.cc:2760: std::string layout_name = keyboard->GetCurrentKeyboardLayoutName(); On 2017/06/10 12:33:55, reveman wrote: > nit: remove this temporary variable after making GetCurrentKeyboardLayoutName > return a reference Done. https://codereview.chromium.org/2925353002/diff/80001/components/exo/wayland/... components/exo/wayland/server.cc:2882: xkb_rule_names names = {.rules = NULL, On 2017/06/10 12:33:55, reveman wrote: > nit: s/NULL/nullptr/ Done. https://codereview.chromium.org/2925353002/diff/80001/components/exo/wayland/... components/exo/wayland/server.cc:2924: std::unique_ptr<xkb_keymap, ui::XkbKeymapDeleter> xkb_keymap_; On 2017/06/10 12:33:55, reveman wrote: > Btw, can you make xkb_keymap_ and xkb_state_ local variables in SendLayout as > part of this patch? Looks like they never had to be members. xkb_keymap_ is used in ModifierFlagsToXkbModifiers also. xkb_state_ is used in OnKeyboardModifiers also. https://codereview.chromium.org/2925353002/diff/80001/ui/base/ime/chromeos/im... File ui/base/ime/chromeos/ime_keyboard.h (right): https://codereview.chromium.org/2925353002/diff/80001/ui/base/ime/chromeos/im... ui/base/ime/chromeos/ime_keyboard.h:58: const std::string GetCurrentKeyboardLayoutName() { return last_layout_; } On 2017/06/10 12:33:55, reveman wrote: > "const std::string& GetCurrentKeyboardLayoutName() const" to avoid copies Done.
The CQ bit was checked by jclinton@google.com 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 jclinton@google.com 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 jclinton@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/12 03:32:57, jclinton wrote: > On 2017/06/10 04:58:55, Shu Chen wrote: > > With the update of fake_ime_keyboard, please add test to cover the observer > > logic. > > There are no tests for any ime_keyboard* code. A test of the observer logic > would require adding a test framework for this directory and would provide no > value: the code changed here is trivial. I added a trivial test anyway.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Friendly ping. Submitting this CL is blocked on OWNERS for minor changes to directories outside of the main CL content.
On 2017/06/13 at 16:14:36, jclinton wrote: > Friendly ping. Submitting this CL is blocked on OWNERS for minor changes to directories outside of the main CL content. If you haven't already, can you please specify what reviewer should be reviewing what code to make it easier?
On 2017/06/13 16:22:11, reveman wrote: > On 2017/06/13 at 16:14:36, jclinton wrote: > > Friendly ping. Submitting this CL is blocked on OWNERS for minor changes to > directories outside of the main CL content. > > If you haven't already, can you please specify what reviewer should be reviewing > what code to make it easier? ...? nona: us/base/ime/chromeos/* shuchen: us/base/ime/chromeos/* jam: us/base/ime/chromeos/*, ash/system/tray_caps_lock.h, chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h sky: us/base/ime/chromeos/*, ash/system/tray_caps_lock.h, chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h
jclinton@google.com changed reviewers: + sadrul@chromium.org, thakis@chromium.org
On 2017/06/13 16:30:02, jclinton wrote: > On 2017/06/13 16:22:11, reveman wrote: > > On 2017/06/13 at 16:14:36, jclinton wrote: > > > Friendly ping. Submitting this CL is blocked on OWNERS for minor changes to > > directories outside of the main CL content. > > > > If you haven't already, can you please specify what reviewer should be > reviewing > > what code to make it easier? > > ...? > > nona: us/base/ime/chromeos/* > shuchen: us/base/ime/chromeos/* > jam: us/base/ime/chromeos/*, ash/system/tray_caps_lock.h, > chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h > sky: us/base/ime/chromeos/*, ash/system/tray_caps_lock.h, > chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h Added more specific subdirectory reviewers because it's been 3 days. Submitting this is blocked on trivial reviews outside of wayland/ directories.
https://codereview.chromium.org/2925353002/diff/160001/ash/system/tray_caps_l... File ash/system/tray_caps_lock.h (right): https://codereview.chromium.org/2925353002/diff/160001/ash/system/tray_caps_l... ash/system/tray_caps_lock.h:30: // Overridden from chromeos::input_method::ImeKeyboard::Observer: You don't need this comment for each function. Just move it up to the previous block. https://codereview.chromium.org/2925353002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right): https://codereview.chromium.org/2925353002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:462: // input_method::ImeKeyboard::Observer implementation: ditto https://codereview.chromium.org/2925353002/diff/160001/components/exo/wayland... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2925353002/diff/160001/components/exo/wayland... components/exo/wayland/server.cc:88: #if defined(OS_CHROMEOS) Do you need this? Are there other platforms where exo is built? https://codereview.chromium.org/2925353002/diff/160001/components/exo/wayland... components/exo/wayland/server.cc:2747: #if defined(USE_OZONE) && defined(OS_CHROMEOS) Do you need either of the define checks? https://codereview.chromium.org/2925353002/diff/160001/components/exo/wayland... components/exo/wayland/server.cc:2760: SendNamedLayout(keyboard->GetCurrentKeyboardLayoutName()); Looks like you don't actually need the null-check in line 2758?
What do you want the people you added to look at? On Tue, Jun 13, 2017 at 8:22 PM, <jclinton@google.com> wrote: > On 2017/06/13 16:30:02, jclinton wrote: > > On 2017/06/13 16:22:11, reveman wrote: > > > On 2017/06/13 at 16:14:36, jclinton wrote: > > > > Friendly ping. Submitting this CL is blocked on OWNERS for minor > changes > to > > > directories outside of the main CL content. > > > > > > If you haven't already, can you please specify what reviewer should be > > reviewing > > > what code to make it easier? > > > > ...? > > > > nona: us/base/ime/chromeos/* > > shuchen: us/base/ime/chromeos/* > > jam: us/base/ime/chromeos/*, ash/system/tray_caps_lock.h, > > chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h > > sky: us/base/ime/chromeos/*, ash/system/tray_caps_lock.h, > > chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h > > Added more specific subdirectory reviewers because it's been 3 days. > Submitting > this is blocked on trivial reviews outside of wayland/ directories. > > > https://codereview.chromium.org/2925353002/ > -- 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/14 14:25:01, Nico wrote: > What do you want the people you added to look at? I want your LGTM for your directory so I can submit this.
Cool; which one is my directory? On Wed, Jun 14, 2017 at 11:35 AM, <jclinton@google.com> wrote: > On 2017/06/14 14:25:01, Nico wrote: > > What do you want the people you added to look at? > > I want your LGTM for your directory so I can submit this. > > https://codereview.chromium.org/2925353002/ > -- 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/14 15:35:42, Nico wrote: > Cool; which one is my directory? chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h lgtm (note that i'm an ui/owner but didn't look at the ui/ stuff, so if you need a review there, wait for your reviewer there) I'd also tell other people you added (if more than me) what you want them to look at.
https://codereview.chromium.org/2925353002/diff/160001/ash/system/tray_caps_l... File ash/system/tray_caps_lock.h (right): https://codereview.chromium.org/2925353002/diff/160001/ash/system/tray_caps_l... ash/system/tray_caps_lock.h:30: // Overridden from chromeos::input_method::ImeKeyboard::Observer: On 2017/06/14 03:16:53, sadrul wrote: > You don't need this comment for each function. Just move it up to the previous > block. Done. https://codereview.chromium.org/2925353002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h (right): https://codereview.chromium.org/2925353002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h:462: // input_method::ImeKeyboard::Observer implementation: On 2017/06/14 03:16:53, sadrul wrote: > ditto Done. https://codereview.chromium.org/2925353002/diff/160001/components/exo/wayland... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2925353002/diff/160001/components/exo/wayland... components/exo/wayland/server.cc:88: #if defined(OS_CHROMEOS) On 2017/06/14 03:16:53, sadrul wrote: > Do you need this? Are there other platforms where exo is built? This was already reviewed and it is necessary. See review thread. https://codereview.chromium.org/2925353002/diff/160001/components/exo/wayland... components/exo/wayland/server.cc:2747: #if defined(USE_OZONE) && defined(OS_CHROMEOS) On 2017/06/14 03:16:53, sadrul wrote: > Do you need either of the define checks? This was already reviewed and it is necessary. See review thread. https://codereview.chromium.org/2925353002/diff/160001/components/exo/wayland... components/exo/wayland/server.cc:2760: SendNamedLayout(keyboard->GetCurrentKeyboardLayoutName()); On 2017/06/14 03:16:53, sadrul wrote: > Looks like you don't actually need the null-check in line 2758? This was already reviewed and it is necessary; null is returned from the fake. See review thread.
The CQ bit was checked by jclinton@google.com 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/06/14 00:22:32, jclinton wrote: > Added more specific subdirectory reviewers because it's been 3 days. Submitting > this is blocked on trivial reviews outside of wayland/ directories. It's now been 4 days. The last file without an LGTM is https://codereview.chromium.org/2925353002/diff/180001/ash/system/tray_caps_l... which is a single line change. sadrul@: sky@ and jam@ went on vacation so now it's just you.
Please prefix what you're saying with the username of the person you're talking to, else things are confusing, at least for people who get many reviews. On Jun 14, 2017 6:11 PM, <jclinton@google.com> wrote: > On 2017/06/14 00:22:32, jclinton wrote: > > Added more specific subdirectory reviewers because it's been 3 days. > Submitting > > this is blocked on trivial reviews outside of wayland/ directories. > > It's now been 4 days. The last file without an LGTM is > https://codereview.chromium.org/2925353002/diff/180001/ > ash/system/tray_caps_lock.h > which is a single line change. sadrul@: sky@ and jam@ went on vacation so > now > it's just you. > > > https://codereview.chromium.org/2925353002/ > -- 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.
https://codereview.chromium.org/2925353002/diff/160001/components/exo/wayland... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2925353002/diff/160001/components/exo/wayland... components/exo/wayland/server.cc:88: #if defined(OS_CHROMEOS) On 2017/06/14 15:49:04, jclinton wrote: > On 2017/06/14 03:16:53, sadrul wrote: > > Do you need this? Are there other platforms where exo is built? > > This was already reviewed and it is necessary. See review thread. Link please?
https://codereview.chromium.org/2925353002/diff/160001/components/exo/wayland... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2925353002/diff/160001/components/exo/wayland... components/exo/wayland/server.cc:2747: #if defined(USE_OZONE) && defined(OS_CHROMEOS) On 2017/06/14 15:49:04, jclinton wrote: > On 2017/06/14 03:16:53, sadrul wrote: > > Do you need either of the define checks? > > This was already reviewed and it is necessary. See review thread. I am requesting more information here to understand why we need these checks, because if we do need these, then it would be better to organize this code differently. For example, have a separate a separate independent ImeKeyboard::Observer object, owned by WaylandKeyboardDelegate, instead of having to litter the ifdefs everywhere in WaylandKeyboardDelegate, which makes this code difficult to read. Or an ozone+cros specific subclass of WaylandKeyboardDelegate. Also, USE_OZONE is turned on by default for chromeos [1], so you shouldn't need both checks. And exo/wayland has a dependency on ash [2, 3], and ash is supported only on chromeos [4]. That's why I want to understand better when we will be building exo on other platforms. [1] https://cs.chromium.org/chromium/src/build/config/ui.gni?l=30 [2] https://cs.chromium.org/chromium/src/components/exo/BUILD.gn?sq=package:chrom... [3] https://cs.chromium.org/chromium/src/components/exo/wayland/BUILD.gn?sq=packa... [4] https://cs.chromium.org/chromium/src/ash/BUILD.gn?sq=package:chromium&dr&l=12
https://codereview.chromium.org/2925353002/diff/160001/components/exo/wayland... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2925353002/diff/160001/components/exo/wayland... components/exo/wayland/server.cc:2747: #if defined(USE_OZONE) && defined(OS_CHROMEOS) On 2017/06/15 at 06:57:27, sadrul wrote: > On 2017/06/14 15:49:04, jclinton wrote: > > On 2017/06/14 03:16:53, sadrul wrote: > > > Do you need either of the define checks? > > > > This was already reviewed and it is necessary. See review thread. > > I am requesting more information here to understand why we need these checks, because if we do need these, then it would be better to organize this code differently. For example, have a separate a separate independent ImeKeyboard::Observer object, owned by WaylandKeyboardDelegate, instead of having to litter the ifdefs everywhere in WaylandKeyboardDelegate, which makes this code difficult to read. Or an ozone+cros specific subclass of WaylandKeyboardDelegate. We're not making the assumption that exo will only be built on chromeos and with ozone today. When exo was created we decided to not limit it to chromeos builds but also support ash on linux but now that this is no longer a supported build config we might want to change that design. Ozone was not used by default for workstation cros builds when we created exo so ifdefs for that was necessary. Organizing the code differently is fine with me if it improves readability but I'm also OK with current version (prefer it if we intend to remove some of these ifdefs in the near future). > > Also, USE_OZONE is turned on by default for chromeos [1], so you shouldn't need both checks. And exo/wayland has a dependency on ash [2, 3], and ash is supported only on chromeos [4]. That's why I want to understand better when we will be building exo on other platforms. Are non-ozone chromeos builds not supported at all anymore? I don't mind changing these build assumptions but if we do that then we should do it throughout all of exo and not just for this code. The current patch is consistent with rest of exo today. > > [1] https://cs.chromium.org/chromium/src/build/config/ui.gni?l=30 > [2] https://cs.chromium.org/chromium/src/components/exo/BUILD.gn?sq=package:chrom... > [3] https://cs.chromium.org/chromium/src/components/exo/wayland/BUILD.gn?sq=packa... > [4] https://cs.chromium.org/chromium/src/ash/BUILD.gn?sq=package:chromium&dr&l=12
https://codereview.chromium.org/2925353002/diff/160001/components/exo/wayland... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2925353002/diff/160001/components/exo/wayland... components/exo/wayland/server.cc:2747: #if defined(USE_OZONE) && defined(OS_CHROMEOS) On 2017/06/15 11:44:15, reveman wrote: > On 2017/06/15 at 06:57:27, sadrul wrote: > > On 2017/06/14 15:49:04, jclinton wrote: > > > On 2017/06/14 03:16:53, sadrul wrote: > > > > Do you need either of the define checks? > > > > > > This was already reviewed and it is necessary. See review thread. > > > > I am requesting more information here to understand why we need these checks, > because if we do need these, then it would be better to organize this code > differently. For example, have a separate a separate independent > ImeKeyboard::Observer object, owned by WaylandKeyboardDelegate, instead of > having to litter the ifdefs everywhere in WaylandKeyboardDelegate, which makes > this code difficult to read. Or an ozone+cros specific subclass of > WaylandKeyboardDelegate. > > We're not making the assumption that exo will only be built on chromeos and with > ozone today. When exo was created we decided to not limit it to chromeos builds > but also support ash on linux but now that this is no longer a supported build > config we might want to change that design. Ozone was not used by default for > workstation cros builds when we created exo so ifdefs for that was necessary. > Organizing the code differently is fine with me if it improves readability but > I'm also OK with current version (prefer it if we intend to remove some of these > ifdefs in the near future). > > > > > Also, USE_OZONE is turned on by default for chromeos [1], so you shouldn't > need both checks. And exo/wayland has a dependency on ash [2, 3], and ash is > supported only on chromeos [4]. That's why I want to understand better when we > will be building exo on other platforms. > > Are non-ozone chromeos builds not supported at all anymore? The linux_chromium_chromeos_rel_ng still runs non-ozone config. But the support is in the process of being removed (https://bugs.chromium.org/p/chromium/issues/detail?id=671355). > > I don't mind changing these build assumptions but if we do that then we should > do it throughout all of exo and not just for this code. The current patch is > consistent with rest of exo today. Sounds reasonable. Thanks. > > > > > [1] https://cs.chromium.org/chromium/src/build/config/ui.gni?l=30 > > [2] > https://cs.chromium.org/chromium/src/components/exo/BUILD.gn?sq=package:chrom... > > [3] > https://cs.chromium.org/chromium/src/components/exo/wayland/BUILD.gn?sq=packa... > > [4] > https://cs.chromium.org/chromium/src/ash/BUILD.gn?sq=package:chromium&dr&l=12 > https://codereview.chromium.org/2925353002/diff/160001/components/exo/wayland... components/exo/wayland/server.cc:2760: SendNamedLayout(keyboard->GetCurrentKeyboardLayoutName()); On 2017/06/14 15:49:04, jclinton wrote: > On 2017/06/14 03:16:53, sadrul wrote: > > Looks like you don't actually need the null-check in line 2758? > > This was already reviewed and it is necessary; null is returned from the fake. > See review thread. If |keyboard| can be null, won't this line cause a crash?
> https://codereview.chromium.org/2925353002/diff/160001/components/exo/wayland... > components/exo/wayland/server.cc:2760: > SendNamedLayout(keyboard->GetCurrentKeyboardLayoutName()); > On 2017/06/14 15:49:04, jclinton wrote: > > On 2017/06/14 03:16:53, sadrul wrote: > > > Looks like you don't actually need the null-check in line 2758? > > > > This was already reviewed and it is necessary; null is returned from the fake. > > See review thread. > > If |keyboard| can be null, won't this line cause a crash? ash/ and ui/base/ lgtm. I would still like the above resolved.
https://codereview.chromium.org/2925353002/diff/160001/components/exo/wayland... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2925353002/diff/160001/components/exo/wayland... components/exo/wayland/server.cc:2760: SendNamedLayout(keyboard->GetCurrentKeyboardLayoutName()); On 2017/06/15 13:18:11, sadrul wrote: > On 2017/06/14 15:49:04, jclinton wrote: > > On 2017/06/14 03:16:53, sadrul wrote: > > > Looks like you don't actually need the null-check in line 2758? > > > > This was already reviewed and it is necessary; null is returned from the fake. > > See review thread. > > If |keyboard| can be null, won't this line cause a crash? Done.
The CQ bit was checked by jclinton@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jclinton@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, thakis@chromium.org, jclinton@google.com, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2925353002/#ps200001 (title: "Fix potential nullptr deref")
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
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jclinton@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jclinton@google.com, reveman@chromium.org, thakis@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2925353002/#ps220001 (title: "Rebase on master")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1497554824745900,
"parent_rev": "79d7ee3d20d751cc7d3fc57db0763f70525d2db3", "commit_rev":
"2870fad48915fa547f6653e7cdd7afd04cada8fa"}
Message was sent while issue was closed.
Description was changed from ========== Implement sync of keyboard layout between Ozone and Wayland clients BUG=chromium:707429,b:27877642 TEST=install Crouton environment, run gtk3 apps with GDK Wayland backend, confirmed layout is correct at first launch and after toggling layout, confirmed no new crashes introduced (tried US, US Dvorak and Latin American layouts). toggling layout after closing Wayland app doesn't crash Chrome (observer released). ========== to ========== Implement sync of keyboard layout between Ozone and Wayland clients BUG=chromium:707429,b:27877642 TEST=install Crouton environment, run gtk3 apps with GDK Wayland backend, confirmed layout is correct at first launch and after toggling layout, confirmed no new crashes introduced (tried US, US Dvorak and Latin American layouts). toggling layout after closing Wayland app doesn't crash Chrome (observer released). Review-Url: https://codereview.chromium.org/2925353002 Cr-Commit-Position: refs/heads/master@{#479825} Committed: https://chromium.googlesource.com/chromium/src/+/2870fad48915fa547f6653e7cdd7... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/2870fad48915fa547f6653e7cdd7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
