Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2731283004:
Do not use ash in chromeos::EventRewriter. (Closed)
DescriptionDo not use ash in chromeos::EventRewriter.
In CL https://codereview.chromium.org/2724913002/, we will move
chromeos::EventRewriter to //ui/chromeos/events. So it can not use
ash. This CL make StickyKeysController implement ui::EventRewriter,
and let chromeos::EventRewriter use the ui::EventRewriter interface
instead StickyKeysController.
BUG=693180
Review-Url: https://codereview.chromium.org/2731283004
Cr-Commit-Position: refs/heads/master@{#456067}
Committed: https://chromium.googlesource.com/chromium/src/+/64dcd3803c8e5370075b14c880d4c7f6f4a4d41f
Patch Set 1 #Patch Set 2 : Fix build issue #Patch Set 3 : Fix a crash #
Total comments: 2
Patch Set 4 : Address review issue #Patch Set 5 : Fix typos. #Patch Set 6 : Fix trybot issues #Patch Set 7 : Fix chromeos x11 build #
Total comments: 8
Patch Set 8 : Address review issues. #
Total comments: 4
Messages
Total messages: 50 (32 generated)
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
penghuang@chromium.org changed reviewers: + sadrul@chromium.org
Hi Sadrul, PTAL. Thanks.
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_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 penghuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by penghuang@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...
penghuang@chromium.org changed reviewers: + tengs@chromium.org
Hi Tim, PTAL. Thanks.
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_...)
https://codereview.chromium.org/2731283004/diff/40001/ash/sticky_keys/sticky_... File ash/sticky_keys/sticky_keys_controller.cc (left): https://codereview.chromium.org/2731283004/diff/40001/ash/sticky_keys/sticky_... ash/sticky_keys/sticky_keys_controller.cc:125: ui::EventRewriteStatus StickyKeysController::RewriteKeyEvent( Could you keep these functions (but make them private) and call them from RewriteEvent()? It's easier to read than a large if statement of complex logic.
https://codereview.chromium.org/2731283004/diff/40001/ash/sticky_keys/sticky_... File ash/sticky_keys/sticky_keys_controller.cc (left): https://codereview.chromium.org/2731283004/diff/40001/ash/sticky_keys/sticky_... ash/sticky_keys/sticky_keys_controller.cc:125: ui::EventRewriteStatus StickyKeysController::RewriteKeyEvent( On 2017/03/07 18:48:54, Tim Song wrote: > Could you keep these functions (but make them private) and call them from > RewriteEvent()? It's easier to read than a large if statement of complex logic. Done
The CQ bit was checked by penghuang@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_...)
Would you mind looking at the test failures? Those look relevant.
The CQ bit was checked by penghuang@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_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 penghuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/08 02:00:08, sadrul wrote: > Would you mind looking at the test failures? Those look relevant. Hi Sadrul, I just fixed the test. PTAL. Thanks.
On 2017/03/08 02:00:08, sadrul wrote: > Would you mind looking at the test failures? Those look relevant. Hi Sadrul, I just fixed the test. PTAL. Thanks.
I looked at the chrome/ changes (I will leave the ash changes to tengs@, unless there's something specific in there you want me to look at). Looks pretty good. Mostly some nits: https://codereview.chromium.org/2731283004/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/2731283004/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/events/event_rewriter.cc:522: std::unique_ptr<ui::Event> tmp_event = ui::Event::Clone(key_event); Can you simply make a copy, i.e: ui::KeyEvent tmp_event = key_event; https://codereview.chromium.org/2731283004/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/events/event_rewriter.cc:579: std::unique_ptr<ui::Event> tmp_event = ui::Event::Clone(mouse_event); Just make a copy, as above? https://codereview.chromium.org/2731283004/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/events/event_rewriter.cc:628: // whell event has been rewritten and stored in |rewritten_event|. indentation seems off here. https://codereview.chromium.org/2731283004/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/events/event_rewriter.cc:640: #endif Can you remove the USE_X11 changes? Because those are no longer used on any device. If it breaks some unit-tests, then it's probably a better idea to remove those tests too. Or may be do that in a separate CL ... I am OK with that too.
https://codereview.chromium.org/2731283004/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/2731283004/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/events/event_rewriter.cc:522: std::unique_ptr<ui::Event> tmp_event = ui::Event::Clone(key_event); On 2017/03/09 06:28:12, sadrul wrote: > Can you simply make a copy, i.e: > > ui::KeyEvent tmp_event = key_event; Done. https://codereview.chromium.org/2731283004/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/events/event_rewriter.cc:579: std::unique_ptr<ui::Event> tmp_event = ui::Event::Clone(mouse_event); On 2017/03/09 06:28:13, sadrul wrote: > Just make a copy, as above? Done. https://codereview.chromium.org/2731283004/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/events/event_rewriter.cc:628: // whell event has been rewritten and stored in |rewritten_event|. On 2017/03/09 06:28:12, sadrul wrote: > indentation seems off here. Done. https://codereview.chromium.org/2731283004/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/events/event_rewriter.cc:640: #endif On 2017/03/09 06:28:12, sadrul wrote: > Can you remove the USE_X11 changes? Because those are no longer used on any > device. If it breaks some unit-tests, then it's probably a better idea to remove > those tests too. Or may be do that in a separate CL ... I am OK with that too. I tried, but seems linux_chromium_chromeos_rel_ng are still using X11 for chromeos. some test cases failed without those code. I will remove them in separate CL later. I am thinking why we still need this bot if we don't use X11 for chromeos anymore.
Hi Tim, I updated the CL. Could you please take a look? Thanks.
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
penghuang@chromium.org changed reviewers: + xiyuan@chromium.org
+xiyuan for cchrome/browser/chromeos/chrome_browser_main_chromeos.cc Hi Xiyuan, PTAL. Thanks.
https://codereview.chromium.org/2731283004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/2731283004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:810: ash::Shell::GetInstance()->sticky_keys_controller()))); The CL changes StickyKeysController to be an ui::EventRewriter. chromeos::EventRewriter is also derived from ui::EventRewriter. Having an EventRewriter to run inside another EventRewriter seems a bit strange. Why do we need to make StickyKeysController an ui::EventRewriter? To pretend that chromeos::EventRewriter does not depend on ash?
https://codereview.chromium.org/2731283004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/2731283004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:810: ash::Shell::GetInstance()->sticky_keys_controller()))); On 2017/03/09 18:49:29, xiyuan wrote: > The CL changes StickyKeysController to be an ui::EventRewriter. > chromeos::EventRewriter is also derived from ui::EventRewriter. Having an > EventRewriter to run inside another EventRewriter seems a bit strange. > > Why do we need to make StickyKeysController an ui::EventRewriter? To pretend > that chromeos::EventRewriter does not depend on ash? https://codereview.chromium.org/2724913002/ - In this CL, we will move chromeos::EventRewriter to //ui/chromeos/events, so it can be used by //services/ui. To do it, chromeos::EventRewriter cannot depend on ash. That's reason why we need this CL.
https://codereview.chromium.org/2731283004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/2731283004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:810: ash::Shell::GetInstance()->sticky_keys_controller()))); On 2017/03/09 18:58:45, Peng wrote: > On 2017/03/09 18:49:29, xiyuan wrote: > > The CL changes StickyKeysController to be an ui::EventRewriter. > > chromeos::EventRewriter is also derived from ui::EventRewriter. Having an > > EventRewriter to run inside another EventRewriter seems a bit strange. > > > > Why do we need to make StickyKeysController an ui::EventRewriter? To pretend > > that chromeos::EventRewriter does not depend on ash? > > https://codereview.chromium.org/2724913002/ - In this CL, we will move > chromeos::EventRewriter to //ui/chromeos/events, so it can be used by > //services/ui. To do it, chromeos::EventRewriter cannot depend on ash. > That's reason why we need this CL. I understand the goal. My point is changing StickyKeysController to be an ui::EventRewriter feels a bit strange. We would then have an EventRewriter running inside another EventRewriter. It would be confusing to read. Reader like me would then ask why don't we chain them up in EventSource like all other EventRewriters. I would recommend to extract an interface, say StickyKeysControllerInterface to contain what chromeos::EventRewriter needs. The interface can sit in //chromeos. chromeos::EventRewriter takes this interface instead of concrete impl. And ash::StickyKeysController is an impl of this interface. This way, we breaks up the deps on ash and I think it would make more sense than using ui::EventWriter.
lgtm https://codereview.chromium.org/2731283004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/2731283004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:810: ash::Shell::GetInstance()->sticky_keys_controller()))); On 2017/03/09 19:26:50, xiyuan wrote: > On 2017/03/09 18:58:45, Peng wrote: > > On 2017/03/09 18:49:29, xiyuan wrote: > > > The CL changes StickyKeysController to be an ui::EventRewriter. > > > chromeos::EventRewriter is also derived from ui::EventRewriter. Having an > > > EventRewriter to run inside another EventRewriter seems a bit strange. > > > > > > Why do we need to make StickyKeysController an ui::EventRewriter? To pretend > > > that chromeos::EventRewriter does not depend on ash? > > > > https://codereview.chromium.org/2724913002/ - In this CL, we will move > > chromeos::EventRewriter to //ui/chromeos/events, so it can be used by > > //services/ui. To do it, chromeos::EventRewriter cannot depend on ash. > > That's reason why we need this CL. > > I understand the goal. My point is changing StickyKeysController to be an > ui::EventRewriter feels a bit strange. We would then have an EventRewriter > running inside another EventRewriter. It would be confusing to read. Reader like > me would then ask why don't we chain them up in EventSource like all other > EventRewriters. > > I would recommend to extract an interface, say StickyKeysControllerInterface to > contain what chromeos::EventRewriter needs. The interface can sit in //chromeos. > chromeos::EventRewriter takes this interface instead of concrete impl. And > ash::StickyKeysController is an impl of this interface. This way, we breaks up > the deps on ash and I think it would make more sense than using ui::EventWriter. Per offline discussion, using a StickyKeysController interface is considered and seems not very useful. For the record, see https://codereview.chromium.org/2724913002/diff/160001/ui/chromeos/events/eve...
lgtm
The CQ bit was checked by penghuang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489159908852370,
"parent_rev": "ed2a0e974f741050243b2d922653e4023c64fbc6", "commit_rev":
"64dcd3803c8e5370075b14c880d4c7f6f4a4d41f"}
Message was sent while issue was closed.
Description was changed from ========== Do not use ash in chromeos::EventRewriter. In CL https://codereview.chromium.org/2724913002/, we will move chromeos::EventRewriter to //ui/chromeos/events. So it can not use ash. This CL make StickyKeysController implement ui::EventRewriter, and let chromeos::EventRewriter use the ui::EventRewriter interface instead StickyKeysController. BUG=693180 ========== to ========== Do not use ash in chromeos::EventRewriter. In CL https://codereview.chromium.org/2724913002/, we will move chromeos::EventRewriter to //ui/chromeos/events. So it can not use ash. This CL make StickyKeysController implement ui::EventRewriter, and let chromeos::EventRewriter use the ui::EventRewriter interface instead StickyKeysController. BUG=693180 Review-Url: https://codereview.chromium.org/2731283004 Cr-Commit-Position: refs/heads/master@{#456067} Committed: https://chromium.googlesource.com/chromium/src/+/64dcd3803c8e5370075b14c880d4... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/64dcd3803c8e5370075b14c880d4... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
