Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1979573002:
Move mus::InputEventHandler implementation and tests from PlatformWindowMus to NativeWidgetMus (Closed)
|
Created:
4 years, 7 months ago by Mark Dittmer Modified:
4 years, 7 months ago Reviewers:
sadrul CC:
chromium-reviews, tfarina, Fady Samuel Base URL:
https://chromium.googlesource.com/chromium/src.git@native_widget_mus5 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove mus::InputEventHandler implementation from PlatformWindowMus to NativeWidgetMus
R=sadrul@chromium.org
BUG=609555
Committed: https://crrev.com/30bd151c77a3362c6d0cc2f8cd90969fbc97aa24
Cr-Commit-Position: refs/heads/master@{#394564}
Patch Set 1 #Patch Set 2 : Remove mus::InputEventHandler from platform_window_mus_unittests; add TODO for porting tests in sep… #Patch Set 3 : Delete no longer necessary views in platform_widget_mus_unittests #
Total comments: 3
Patch Set 4 : rebase #
Total comments: 4
Patch Set 5 : Merge in https://codereview.chromium.org/1981803003/ #Patch Set 6 : Remove dead code: PlatformWindowMus's EventAckHandler #Patch Set 7 : rebase #Patch Set 8 : Drop ref to platform_test_helper_mus.cc #Patch Set 9 : rebase from different branch #Patch Set 10 : Drop second copy of tests from merge mistake #Patch Set 11 : rebase #
Total comments: 4
Patch Set 12 : Address final comments from sadrul #Patch Set 13 : rebase #Patch Set 14 : Drop multiple copies of unittests (AGAIN) #
Dependent Patchsets: Messages
Total messages: 60 (22 generated)
Remove mus::InputEventHandler from platform_window_mus_unittests; add TODO for porting tests in separate CL
Delete no longer necessary views in platform_widget_mus_unittests
https://codereview.chromium.org/1979573002/diff/40001/ui/views/mus/platform_w... File ui/views/mus/platform_window_mus.cc (right): https://codereview.chromium.org/1979573002/diff/40001/ui/views/mus/platform_w... ui/views/mus/platform_window_mus.cc:28: class EventAckHandler : public base::MessageLoop::NestingObserver { This should be removed? https://codereview.chromium.org/1979573002/diff/40001/ui/views/mus/platform_w... File ui/views/mus/platform_window_mus_unittest.cc (right): https://codereview.chromium.org/1979573002/diff/40001/ui/views/mus/platform_w... ui/views/mus/platform_window_mus_unittest.cc:73: This should be fixed?
rebase
https://codereview.chromium.org/1979573002/diff/40001/ui/views/mus/platform_w... File ui/views/mus/platform_window_mus_unittest.cc (right): https://codereview.chromium.org/1979573002/diff/40001/ui/views/mus/platform_w... ui/views/mus/platform_window_mus_unittest.cc:73: On 2016/05/13 17:02:44, sadrul wrote: > This should be fixed? I was going to move tests in another CL. Actually, this whole file can go when I move the tests. Sound good?
Let's merge https://codereview.chromium.org/1981803003/ into this CL https://codereview.chromium.org/1979573002/diff/60001/ui/views/mus/native_wid... File ui/views/mus/native_widget_mus.cc (right): https://codereview.chromium.org/1979573002/diff/60001/ui/views/mus/native_wid... ui/views/mus/native_widget_mus.cc:19: #include "components/mus/public/interfaces/window_tree.mojom.h" Is this needed? https://codereview.chromium.org/1979573002/diff/60001/ui/views/mus/platform_w... File ui/views/mus/platform_window_mus.cc (right): https://codereview.chromium.org/1979573002/diff/60001/ui/views/mus/platform_w... ui/views/mus/platform_window_mus.cc:28: class EventAckHandler : public base::MessageLoop::NestingObserver { Remove.
Remove dead code: PlatformWindowMus's EventAckHandler
Addressed all comments. sadrul@chromium.org please take another look. https://codereview.chromium.org/1979573002/diff/60001/ui/views/mus/native_wid... File ui/views/mus/native_widget_mus.cc (right): https://codereview.chromium.org/1979573002/diff/60001/ui/views/mus/native_wid... ui/views/mus/native_widget_mus.cc:19: #include "components/mus/public/interfaces/window_tree.mojom.h" On 2016/05/17 00:18:48, sadrul wrote: > Is this needed? Yes. It provides mus::mojom::EventResult. https://codereview.chromium.org/1979573002/diff/60001/ui/views/mus/platform_w... File ui/views/mus/platform_window_mus.cc (right): https://codereview.chromium.org/1979573002/diff/60001/ui/views/mus/platform_w... ui/views/mus/platform_window_mus.cc:28: class EventAckHandler : public base::MessageLoop::NestingObserver { On 2016/05/17 00:18:48, sadrul wrote: > Remove. Done.
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1979573002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1979573002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
rebase
The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1979573002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1979573002/120001
This test migration is now blocked on https://bugs.chromium.org/p/chromium/issues/detail?id=612447. Candidate fix at https://codereview.chromium.org/1982333002/.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Drop ref to platform_test_helper_mus.cc
The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1979573002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1979573002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run
rebase from different branch
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1979573002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1979573002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run
Drop second copy of tests from merge mistake
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1979573002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1979573002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rebase
Description was changed from ========== Move mus::InputEventHandler implementation from PlatformWindowMus to NativeWidgetMus R=sadrul@chromium.org BUG=609555 ========== to ========== Move mus::InputEventHandler implementation from PlatformWindowMus to NativeWidgetMus R=sadrul@chromium.org BUG=609555 ==========
lgtm https://codereview.chromium.org/1979573002/diff/200001/ui/views/mus/native_wi... File ui/views/mus/native_widget_mus.cc (right): https://codereview.chromium.org/1979573002/diff/200001/ui/views/mus/native_wi... ui/views/mus/native_widget_mus.cc:1267: platform_window_delegate()->DispatchEvent(event.get()); Hm, this should actually just do this->OnEvent(event.get()), but that can't happen yet because IME is in WindowTreeHostMus right now. Can you add a TODO here to me for this? https://codereview.chromium.org/1979573002/diff/200001/ui/views/mus/platform_... File ui/views/mus/platform_window_mus.cc (right): https://codereview.chromium.org/1979573002/diff/200001/ui/views/mus/platform_... ui/views/mus/platform_window_mus.cc:7: #include "base/message_loop/message_loop.h" this can probably be removed now
Address final comments from sadrul
https://codereview.chromium.org/1979573002/diff/200001/ui/views/mus/native_wi... File ui/views/mus/native_widget_mus.cc (right): https://codereview.chromium.org/1979573002/diff/200001/ui/views/mus/native_wi... ui/views/mus/native_widget_mus.cc:1267: platform_window_delegate()->DispatchEvent(event.get()); On 2016/05/18 19:44:42, sadrul wrote: > Hm, this should actually just do this->OnEvent(event.get()), but that can't > happen yet because IME is in WindowTreeHostMus right now. Can you add a TODO > here to me for this? Done. https://codereview.chromium.org/1979573002/diff/200001/ui/views/mus/platform_... File ui/views/mus/platform_window_mus.cc (right): https://codereview.chromium.org/1979573002/diff/200001/ui/views/mus/platform_... ui/views/mus/platform_window_mus.cc:7: #include "base/message_loop/message_loop.h" On 2016/05/18 19:44:43, sadrul wrote: > this can probably be removed now Done.
The CQ bit was checked by markdittmer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/1979573002/#ps220001 (title: "Address final comments from sadrul")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1979573002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1979573002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
rebase
The CQ bit was checked by markdittmer@chromium.org
The CQ bit was unchecked by markdittmer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/1979573002/#ps240001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1979573002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1979573002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_clang on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Drop multiple copies of unittests (AGAIN)
The CQ bit was checked by markdittmer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/1979573002/#ps260001 (title: "Drop multiple copies of unittests (AGAIN)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1979573002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1979573002/260001
Message was sent while issue was closed.
Description was changed from ========== Move mus::InputEventHandler implementation from PlatformWindowMus to NativeWidgetMus R=sadrul@chromium.org BUG=609555 ========== to ========== Move mus::InputEventHandler implementation from PlatformWindowMus to NativeWidgetMus R=sadrul@chromium.org BUG=609555 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Move mus::InputEventHandler implementation from PlatformWindowMus to NativeWidgetMus R=sadrul@chromium.org BUG=609555 ========== to ========== Move mus::InputEventHandler implementation from PlatformWindowMus to NativeWidgetMus R=sadrul@chromium.org BUG=609555 Committed: https://crrev.com/30bd151c77a3362c6d0cc2f8cd90969fbc97aa24 Cr-Commit-Position: refs/heads/master@{#394564} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/30bd151c77a3362c6d0cc2f8cd90969fbc97aa24 Cr-Commit-Position: refs/heads/master@{#394564} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
