Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(39)

Issue 2311393004: Laser tool blocks events from propagating. (Closed)

Created:
4 years, 3 months ago by sammiequon
Modified:
4 years, 3 months ago
CC:
chromium-reviews, kalyank, sadrul, oshima+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

chromeos: Laser tool blocks events from propagating. After discussion concluded the best way was to delegate the laser pointer out of ash/common. Moved all the laser related files into ash/laser. LaserPointerController.* replaces LaserPointerMode.* and it inherits from EventHandler to grab the events and propagate them to LaserPointerView/LaserPointerPoints, which remain the same. LaserPointerMode just inherits common palette tool and calls the delegate. BUG=644804 TEST=ash_unittests --gtest_filter="LaserPointer*" https://screenshot.googleplex.com/BtwX1GTCpcW Committed: https://crrev.com/203ae0296e08db2452a4558d3e84dcf713f6badf Cr-Commit-Position: refs/heads/master@{#419400}

Patch Set 1 #

Total comments: 28

Patch Set 2 : Fixed patch set 1 errors. #

Total comments: 61

Patch Set 3 : Rebased. #

Patch Set 4 : Fixed patch set 2 errors. #

Total comments: 23

Patch Set 5 : Fixed patch set 4 errors. #

Total comments: 39

Patch Set 6 : Fixed patch set 5 errors. #

Total comments: 23

Patch Set 7 : Fixed patch set 6 errors. #

Patch Set 8 : Added a test. #

Total comments: 2

Patch Set 9 : Nit. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+501 lines, -916 lines) Patch
M ash/BUILD.gn View 1 2 3 4 8 chunks +23 lines, -14 lines 0 comments Download
M ash/aura/wm_shell_aura.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M ash/aura/wm_shell_aura.cc View 1 2 3 4 2 chunks +5 lines, -0 lines 0 comments Download
M ash/common/palette_delegate.h View 1 2 3 2 chunks +2 lines, -4 lines 0 comments Download
M ash/common/system/chromeos/palette/palette_tool.cc View 1 chunk +1 line, -1 line 0 comments Download
M ash/common/system/chromeos/palette/tools/laser_pointer_mode.h View 3 chunks +2 lines, -29 lines 0 comments Download
M ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc View 1 2 3 3 chunks +3 lines, -65 lines 0 comments Download
M ash/common/system/chromeos/palette/tools/laser_pointer_mode_test_api.h View 1 chunk +0 lines, -37 lines 0 comments Download
M ash/common/system/chromeos/palette/tools/laser_pointer_mode_test_api.cc View 1 chunk +0 lines, -35 lines 0 comments Download
D ash/common/system/chromeos/palette/tools/laser_pointer_points.h View 1 chunk +0 lines, -64 lines 0 comments Download
D ash/common/system/chromeos/palette/tools/laser_pointer_points.cc View 1 chunk +0 lines, -74 lines 0 comments Download
D ash/common/system/chromeos/palette/tools/laser_pointer_points_test_api.h View 1 chunk +0 lines, -36 lines 0 comments Download
D ash/common/system/chromeos/palette/tools/laser_pointer_points_test_api.cc View 1 chunk +0 lines, -31 lines 0 comments Download
M ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc View 1 chunk +0 lines, -192 lines 0 comments Download
M ash/common/system/chromeos/palette/tools/laser_pointer_view.h View 1 chunk +0 lines, -49 lines 0 comments Download
M ash/common/system/chromeos/palette/tools/laser_pointer_view.cc View 1 chunk +0 lines, -139 lines 0 comments Download
M ash/common/test/test_palette_delegate.h View 1 2 3 1 chunk +0 lines, -2 lines 0 comments Download
M ash/common/test/test_palette_delegate.cc View 1 2 3 1 chunk +0 lines, -4 lines 0 comments Download
M ash/common/wm_shell.h View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
A ash/laser/laser_pointer_controller.h View 1 2 3 4 5 6 1 chunk +76 lines, -0 lines 0 comments Download
A ash/laser/laser_pointer_controller.cc View 1 2 3 4 5 6 7 8 1 chunk +140 lines, -0 lines 0 comments Download
A ash/laser/laser_pointer_controller_test_api.h View 1 2 3 4 5 1 chunk +35 lines, -0 lines 0 comments Download
A ash/laser/laser_pointer_controller_test_api.cc View 1 2 3 4 1 chunk +35 lines, -0 lines 0 comments Download
A + ash/laser/laser_pointer_controller_unittest.cc View 1 2 3 4 5 6 7 4 chunks +79 lines, -65 lines 0 comments Download
A + ash/laser/laser_pointer_points.h View 2 chunks +3 lines, -3 lines 0 comments Download
A + ash/laser/laser_pointer_points.cc View 1 chunk +1 line, -1 line 0 comments Download
A + ash/laser/laser_pointer_points_test_api.h View 1 2 3 4 5 3 chunks +5 lines, -7 lines 0 comments Download
A + ash/laser/laser_pointer_points_test_api.cc View 1 2 3 4 1 chunk +4 lines, -4 lines 0 comments Download
A + ash/laser/laser_pointer_view.h View 1 2 3 4 5 6 3 chunks +14 lines, -6 lines 0 comments Download
A + ash/laser/laser_pointer_view.cc View 1 2 3 4 5 6 7 chunks +40 lines, -12 lines 0 comments Download
M ash/magnifier/partial_magnification_controller.cc View 1 2 3 4 5 chunks +5 lines, -16 lines 0 comments Download
M ash/mus/bridge/wm_shell_mus.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M ash/mus/bridge/wm_shell_mus.cc View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download
M ash/shell.h View 1 2 3 4 5 5 chunks +11 lines, -6 lines 0 comments Download
M ash/shell.cc View 1 2 3 4 5 6 4 chunks +8 lines, -3 lines 0 comments Download
M ash/shell/shell_delegate_impl.cc View 1 2 3 4 1 chunk +0 lines, -2 lines 0 comments Download
M chrome/browser/ui/ash/palette_delegate_chromeos.h View 1 2 3 1 chunk +0 lines, -3 lines 0 comments Download
M chrome/browser/ui/ash/palette_delegate_chromeos.cc View 1 2 3 4 5 6 1 chunk +0 lines, -12 lines 0 comments Download

Messages

Total messages: 87 (57 generated)
sammiequon
jdufault@ - Please take a look. Thanks!
4 years, 3 months ago (2016-09-07 18:52:13 UTC) #5
jdufault
https://codereview.chromium.org/2311393004/diff/40001/ash/common/system/chromeos/palette/tools/laser_pointer_view.cc File ash/common/system/chromeos/palette/tools/laser_pointer_view.cc (right): https://codereview.chromium.org/2311393004/diff/40001/ash/common/system/chromeos/palette/tools/laser_pointer_view.cc#newcode78 ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:78: WmShell::Get()->GetPrimaryRootWindow()->GetBounds(); Does this work properly on multiple displays? You ...
4 years, 3 months ago (2016-09-07 18:57:41 UTC) #6
sammiequon
Accidentally deleted patch set 1, sorry. That one was the one where the laser pointer ...
4 years, 3 months ago (2016-09-09 22:32:48 UTC) #10
jdufault
https://codereview.chromium.org/2311393004/diff/80001/ash/laser/laser_pointer_controller.cc File ash/laser/laser_pointer_controller.cc (right): https://codereview.chromium.org/2311393004/diff/80001/ash/laser/laser_pointer_controller.cc#newcode48 ash/laser/laser_pointer_controller.cc:48: } // namespace newline above https://codereview.chromium.org/2311393004/diff/80001/ash/laser/laser_pointer_controller.cc#newcode87 ash/laser/laser_pointer_controller.cc:87: void LaserPointerController::OnTouchEvent(ui::TouchEvent* ...
4 years, 3 months ago (2016-09-12 21:10:12 UTC) #11
sammiequon
https://codereview.chromium.org/2311393004/diff/80001/ash/laser/laser_pointer_controller.cc File ash/laser/laser_pointer_controller.cc (right): https://codereview.chromium.org/2311393004/diff/80001/ash/laser/laser_pointer_controller.cc#newcode48 ash/laser/laser_pointer_controller.cc:48: } // namespace On 2016/09/12 21:10:11, jdufault wrote: > ...
4 years, 3 months ago (2016-09-13 01:09:57 UTC) #13
jdufault
lgtm
4 years, 3 months ago (2016-09-13 18:24:22 UTC) #14
sammiequon
jamescook@ - Please take a look. Thanks!
4 years, 3 months ago (2016-09-13 18:45:05 UTC) #16
James Cook
Overall, I'm OK with this approach. But I have many comments. https://codereview.chromium.org/2311393004/diff/120001/ash/ash.gyp File ash/ash.gyp (right): ...
4 years, 3 months ago (2016-09-14 17:52:29 UTC) #18
sammiequon
https://codereview.chromium.org/2311393004/diff/120001/ash/ash.gyp File ash/ash.gyp (right): https://codereview.chromium.org/2311393004/diff/120001/ash/ash.gyp#newcode1013 ash/ash.gyp:1013: 'laser/laser_pointer_controller_test_api.cc', On 2016/09/14 17:52:25, James Cook wrote: > The ...
4 years, 3 months ago (2016-09-14 21:44:10 UTC) #19
James Cook
Almost there https://codereview.chromium.org/2311393004/diff/120001/ash/laser/laser_pointer_controller.cc File ash/laser/laser_pointer_controller.cc (right): https://codereview.chromium.org/2311393004/diff/120001/ash/laser/laser_pointer_controller.cc#newcode109 ash/laser/laser_pointer_controller.cc:109: current_mouse_location_ = event_location; On 2016/09/14 21:44:09, sammiequon ...
4 years, 3 months ago (2016-09-14 22:53:38 UTC) #21
sammiequon
Sorry this round took so long, I had trouble getting the trybots to pass as ...
4 years, 3 months ago (2016-09-16 17:35:27 UTC) #56
James Cook
jdufault, can you also take another look at this? It has significantly changed since you ...
4 years, 3 months ago (2016-09-16 18:13:32 UTC) #57
jdufault
https://codereview.chromium.org/2311393004/diff/300001/ash/common/system/chromeos/palette/tools/laser_pointer_mode.h File ash/common/system/chromeos/palette/tools/laser_pointer_mode.h (right): https://codereview.chromium.org/2311393004/diff/300001/ash/common/system/chromeos/palette/tools/laser_pointer_mode.h#newcode17 ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:17: // Controller for the laser pointer functionality. This comment ...
4 years, 3 months ago (2016-09-16 18:43:54 UTC) #58
sammiequon
https://codereview.chromium.org/2311393004/diff/300001/ash/common/system/chromeos/palette/tools/laser_pointer_mode.h File ash/common/system/chromeos/palette/tools/laser_pointer_mode.h (right): https://codereview.chromium.org/2311393004/diff/300001/ash/common/system/chromeos/palette/tools/laser_pointer_mode.h#newcode17 ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:17: // Controller for the laser pointer functionality. On 2016/09/16 ...
4 years, 3 months ago (2016-09-16 19:21:04 UTC) #59
jdufault
https://codereview.chromium.org/2311393004/diff/320001/ash/laser/laser_pointer_controller.cc File ash/laser/laser_pointer_controller.cc (right): https://codereview.chromium.org/2311393004/diff/320001/ash/laser/laser_pointer_controller.cc#newcode115 ash/laser/laser_pointer_controller.cc:115: void LaserPointerController::StopStationaryTimer() { Remove this function and just call ...
4 years, 3 months ago (2016-09-16 19:58:42 UTC) #60
sammiequon
https://codereview.chromium.org/2311393004/diff/320001/ash/laser/laser_pointer_controller.cc File ash/laser/laser_pointer_controller.cc (right): https://codereview.chromium.org/2311393004/diff/320001/ash/laser/laser_pointer_controller.cc#newcode115 ash/laser/laser_pointer_controller.cc:115: void LaserPointerController::StopStationaryTimer() { On 2016/09/16 19:58:41, jdufault wrote: > ...
4 years, 3 months ago (2016-09-16 20:37:40 UTC) #62
James Cook
ping me when you want me to take an OWNERS look
4 years, 3 months ago (2016-09-16 21:56:56 UTC) #63
jdufault
lgtm https://codereview.chromium.org/2311393004/diff/320001/ash/laser/laser_pointer_controller_unittest.cc File ash/laser/laser_pointer_controller_unittest.cc (right): https://codereview.chromium.org/2311393004/diff/320001/ash/laser/laser_pointer_controller_unittest.cc#newcode178 ash/laser/laser_pointer_controller_unittest.cc:178: controller_test_api_.SetEnabled(false); On 2016/09/16 20:37:40, sammiequon wrote: > On ...
4 years, 3 months ago (2016-09-16 21:59:26 UTC) #64
sammiequon
On 2016/09/16 21:59:26, jdufault wrote: > lgtm > > https://codereview.chromium.org/2311393004/diff/320001/ash/laser/laser_pointer_controller_unittest.cc > File ash/laser/laser_pointer_controller_unittest.cc (right): > ...
4 years, 3 months ago (2016-09-16 22:54:01 UTC) #65
sammiequon
jamescook@ - Please take a look. Thanks!
4 years, 3 months ago (2016-09-16 22:54:29 UTC) #66
sammiequon
jamescook@ - Please take a look. Thanks!
4 years, 3 months ago (2016-09-16 22:54:34 UTC) #67
James Cook
LGTM with nit https://codereview.chromium.org/2311393004/diff/380001/ash/laser/laser_pointer_controller.cc File ash/laser/laser_pointer_controller.cc (right): https://codereview.chromium.org/2311393004/diff/380001/ash/laser/laser_pointer_controller.cc#newcode134 ash/laser/laser_pointer_controller.cc:134: if (stationary_timer_repeat_count_++ * kAddStationaryPointsDelayMs >= nit: ...
4 years, 3 months ago (2016-09-16 23:47:01 UTC) #68
sammiequon
https://codereview.chromium.org/2311393004/diff/380001/ash/laser/laser_pointer_controller.cc File ash/laser/laser_pointer_controller.cc (right): https://codereview.chromium.org/2311393004/diff/380001/ash/laser/laser_pointer_controller.cc#newcode134 ash/laser/laser_pointer_controller.cc:134: if (stationary_timer_repeat_count_++ * kAddStationaryPointsDelayMs >= On 2016/09/16 23:47:00, James ...
4 years, 3 months ago (2016-09-17 00:04:24 UTC) #69
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2311393004/400001
4 years, 3 months ago (2016-09-17 00:05:04 UTC) #72
commit-bot: I haz the power
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_presubmit/builds/261600)
4 years, 3 months ago (2016-09-17 00:14:34 UTC) #74
sammiequon
derat@ - Please take a look at chrome/browser/ui/ash/*. Thanks!
4 years, 3 months ago (2016-09-17 00:16:01 UTC) #76
Daniel Erat
rubber-stamp lgtm for c/b/ui/ash
4 years, 3 months ago (2016-09-18 13:56:05 UTC) #81
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2311393004/400001
4 years, 3 months ago (2016-09-18 16:35:15 UTC) #83
commit-bot: I haz the power
Committed patchset #9 (id:400001)
4 years, 3 months ago (2016-09-18 17:24:05 UTC) #85
commit-bot: I haz the power
4 years, 3 months ago (2016-09-18 17:25:49 UTC) #87
Message was sent while issue was closed.
Patchset 9 (id:??) landed as
https://crrev.com/203ae0296e08db2452a4558d3e84dcf713f6badf
Cr-Commit-Position: refs/heads/master@{#419400}

Powered by Google App Engine
This is Rietveld 408576698