Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2792573003:
Update PointerEvent handled state correctly in OnWindowInputEvent. (Closed)
DescriptionUpdate PointerEvent handled state correctly in OnWindowInputEvent.
BUG=none, related to https://codereview.chromium.org/2779093004
TEST=aura_unittests
Review-Url: https://codereview.chromium.org/2792573003
Cr-Commit-Position: refs/heads/master@{#461276}
Committed: https://chromium.googlesource.com/chromium/src/+/bb62d38f7b9e0dfc9ca362084f4278427d459e93
Patch Set 1 #
Total comments: 2
Patch Set 2 : MapEvent #
Total comments: 4
Patch Set 3 : comments #
Total comments: 4
Patch Set 4 : comments #Messages
Total messages: 25 (15 generated)
The CQ bit was checked by riajiang@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...
riajiang@chromium.org changed reviewers: + sadrul@chromium.org
Hi sadrul@, could you take a look? Thanks!
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_...)
sky@chromium.org changed reviewers: + sky@chromium.org
https://codereview.chromium.org/2792573003/diff/1/ui/aura/mus/window_tree_cli... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2792573003/diff/1/ui/aura/mus/window_tree_cli... ui/aura/mus/window_tree_client.cc:1241: if (event->IsMousePointerEvent()) { In hopes of reducing copy/paste, WDYT of a MapEvent functions that takes a const ui::Event& and returns std::unique_ptr<ui::Event> ? MapEvent has the logic of creating and return the right Event subclass. That way the code here comes: std::unique_ptr<ui::Event> mapped_event = MapEvent(event.get()); DispatchEventToTarget(mapped_event.get(), window); ack_handler.set_handled(mapped_event->handled()); ?
The CQ bit was checked by riajiang@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...
https://codereview.chromium.org/2792573003/diff/1/ui/aura/mus/window_tree_cli... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2792573003/diff/1/ui/aura/mus/window_tree_cli... ui/aura/mus/window_tree_client.cc:1241: if (event->IsMousePointerEvent()) { On 2017/03/31 17:41:24, sky wrote: > In hopes of reducing copy/paste, WDYT of a MapEvent functions that takes a const > ui::Event& and returns std::unique_ptr<ui::Event> ? MapEvent has the logic of > creating and return the right Event subclass. That way the code here comes: > > std::unique_ptr<ui::Event> mapped_event = MapEvent(event.get()); > DispatchEventToTarget(mapped_event.get(), window); > ack_handler.set_handled(mapped_event->handled()); > ? Yea that's cleaner! Done.
lgtm https://codereview.chromium.org/2792573003/diff/20001/ui/aura/mus/window_tree... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2792573003/diff/20001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:176: } You don't need a lot of the else's. if (..) return ...; else return ...; Should become: if (...) return ...; return ...; https://codereview.chromium.org/2792573003/diff/20001/ui/aura/mus/window_tree... File ui/aura/mus/window_tree_client_unittest.cc (right): https://codereview.chromium.org/2792573003/diff/20001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client_unittest.cc:752: std::unique_ptr<ui::PointerEvent> pointer_event(new ui::PointerEvent( You shouldn't need a std::unique_ptr<> here.
https://codereview.chromium.org/2792573003/diff/20001/ui/aura/mus/window_tree... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2792573003/diff/20001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:176: } On 2017/03/31 19:02:08, sadrul wrote: > You don't need a lot of the else's. > if (..) > return ...; > else > return ...; > > Should become: > if (...) > return ...; > return ...; Done. https://codereview.chromium.org/2792573003/diff/20001/ui/aura/mus/window_tree... File ui/aura/mus/window_tree_client_unittest.cc (right): https://codereview.chromium.org/2792573003/diff/20001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client_unittest.cc:752: std::unique_ptr<ui::PointerEvent> pointer_event(new ui::PointerEvent( On 2017/03/31 19:02:08, sadrul wrote: > You shouldn't need a std::unique_ptr<> here. Done.
The CQ bit was checked by riajiang@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/2792573003/#ps40001 (title: "comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM https://codereview.chromium.org/2792573003/diff/40001/ui/aura/mus/window_tree... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2792573003/diff/40001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:165: std::unique_ptr<ui::Event> MapEvent(const ui::Event& event) { Please add a comment. https://codereview.chromium.org/2792573003/diff/40001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:170: } else if (event.IsTouchPointerEvent()) { no else after return (see chromium style guide).
The CQ bit was unchecked by riajiang@chromium.org
https://codereview.chromium.org/2792573003/diff/40001/ui/aura/mus/window_tree... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2792573003/diff/40001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:165: std::unique_ptr<ui::Event> MapEvent(const ui::Event& event) { On 2017/03/31 20:57:30, sky wrote: > Please add a comment. Done. https://codereview.chromium.org/2792573003/diff/40001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:170: } else if (event.IsTouchPointerEvent()) { On 2017/03/31 20:57:30, sky wrote: > no else after return (see chromium style guide). Done.
The CQ bit was checked by riajiang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2792573003/#ps60001 (title: "comments")
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": 60001, "attempt_start_ts": 1490995065978870,
"parent_rev": "c4244cbf21c2519f8dd003b96baa82c94bc5b26d", "commit_rev":
"bb62d38f7b9e0dfc9ca362084f4278427d459e93"}
Message was sent while issue was closed.
Description was changed from ========== Update PointerEvent handled state correctly in OnWindowInputEvent. BUG=none, related to https://codereview.chromium.org/2779093004 TEST=aura_unittests ========== to ========== Update PointerEvent handled state correctly in OnWindowInputEvent. BUG=none, related to https://codereview.chromium.org/2779093004 TEST=aura_unittests Review-Url: https://codereview.chromium.org/2792573003 Cr-Commit-Position: refs/heads/master@{#461276} Committed: https://chromium.googlesource.com/chromium/src/+/bb62d38f7b9e0dfc9ca362084f42... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/bb62d38f7b9e0dfc9ca362084f42... |
