Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2362063002:
cros: Laser pointer fades out on release, do not cover palette. (Closed)
|
Created:
4 years, 3 months ago by sammiequon Modified:
4 years, 2 months ago CC:
chromium-reviews, kalyank, sadrul Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncros: Laser pointer fades out on release, do not cover palette.
The laser pointer fades out quickly now on release instead of disappearing. It gets hidden before the palette now. I check if the event is over the palette first now. Also modified the laser points to be able to move forward in time (and thus delete points) without adding a new point.
BUG=647795, 649517
TEST=ash_unittests --gtest_filter="LaserPointer*"
Committed: https://crrev.com/4034b4e474bb881411690569ce59e298676ab20f
Cr-Commit-Position: refs/heads/master@{#425827}
Patch Set 1 #
Total comments: 26
Patch Set 2 : Fixed patch set 1 errors. #Patch Set 3 : Moved age logic to laser pointer points class. #
Total comments: 30
Patch Set 4 : Rebased. #Patch Set 5 : Fixed patch set 3 errors. #
Total comments: 19
Patch Set 6 : Fixed patch set 5 errors. #
Total comments: 20
Patch Set 7 : Fixed patch set 6 errors. #
Total comments: 10
Patch Set 8 : Rebased. #Patch Set 9 : Fixed patch set 7 errors. #
Total comments: 6
Patch Set 10 : Fixed patch set 9 errors. #
Total comments: 4
Patch Set 11 : Fixed patch set 10 errors. #
Total comments: 2
Patch Set 12 : Nit. #
Messages
Total messages: 49 (26 generated)
Description was changed from ========== cros: Laser pointer fades out on release, do not cover palette. The laser pointer fades out quickly now on release instead of disappearing. It gets hidden before the palette now. I check if the event is over the palette first now. Also modified the laser points to be able to move forward in time (and thus delete points) without adding a new point. BUG=647795,649517 TEST=ash_unittests --gtest_filter="LaserPointer*" ========== to ========== cros: Laser pointer fades out on release, do not cover palette. The laser pointer fades out quickly now on release instead of disappearing. It gets hidden before the palette now. I check if the event is over the palette first now. Also modified the laser points to be able to move forward in time (and thus delete points) without adding a new point. BUG=647795,649517 TEST=ash_unittests --gtest_filter="LaserPointer*" ==========
sammiequon@chromium.org changed reviewers: + jdufault@chromium.org
On 2016/09/22 23:08:48, sammiequon wrote: > mailto:sammiequon@chromium.org changed reviewers: > + mailto:jdufault@chromium.org jdufault@ - Please take a look. Thanks!
I added some comments, but I think that the fade away logic can be greatly simplified if the renderer notifies LaserPointerPoints of the current time right before a render event. This notification will then remove any points that are too old. This also means that LaserPointerPoints doesn't need to keep have a concept of the most recent time. https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_con... File ash/laser/laser_pointer_controller.cc (right): https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_con... ash/laser/laser_pointer_controller.cc:82: SwitchTargetRootWindowIfNeeded(current_window); What is the purpose of this? https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_con... ash/laser/laser_pointer_controller.cc:87: aura::Window::ConvertPointToTarget(event_root, current_window, Keep all of the point remapping logic together. https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_con... ash/laser/laser_pointer_controller.cc:92: DestroyLaserPointerView(); I think we should continuing showing the laser pointer if it is already active and the user drags the stylus over the palette (similar to the magnifier). https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_con... ash/laser/laser_pointer_controller.cc:93: return; We should not be creating the view in the first place if the cursor is over the palette and the view is not already created. Right now it seems like if the user clicks on the stylus we: 1. Create view 2. Destroy view https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_con... ash/laser/laser_pointer_controller.cc:114: // interal collections timestamps if released. interal -> internal https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_poi... File ash/laser/laser_pointer_points.cc (right): https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_poi... ash/laser/laser_pointer_points.cc:22: collection_latest_time_ = current_time; We should always be updating collection_latest_time_, otherwise the logic in MoveForwardInTime will use a stale value. https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_poi... ash/laser/laser_pointer_points.cc:31: collection_latest_time_ = base::Time::Now(); Call MoveFowardInTime(base::Time::Now()) https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_poi... ash/laser/laser_pointer_points.cc:35: void LaserPointerPoints::MoveForwardInTime(const base::Time& new_latest_time) { Rename one of the MoveForwardInTime methods so they are not overloaded. https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_poi... ash/laser/laser_pointer_points.cc:89: if (IsEmpty()) Why do we need this if? https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_poi... File ash/laser/laser_pointer_points.h (right): https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_poi... ash/laser/laser_pointer_points.h:37: // too Fix formatting https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_poi... File ash/laser/laser_pointer_points_test_api.cc (right): https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_poi... ash/laser/laser_pointer_points_test_api.cc:28: instance_->collection_latest_time_ = new_time; Should this be calling SetLatestTime(new_time)? https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_view.h File ash/laser/laser_pointer_view.h (right): https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_vie... ash/laser/laser_pointer_view.h:38: void SetIsFadingAway(bool is_fading); Rename to set_is_fading_away. Trival methods (like this one) should be inlined as well. https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_vie... ash/laser/laser_pointer_view.h:39: bool GetIsFadingAway() const; Rename to is_fading_away() and inline the definition.
I moved the fading logic to laser_pointer_points.h as requested. https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_con... File ash/laser/laser_pointer_controller.cc (right): https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_con... ash/laser/laser_pointer_controller.cc:82: SwitchTargetRootWindowIfNeeded(current_window); On 2016/09/23 23:59:19, jdufault wrote: > What is the purpose of this? Done. https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_con... ash/laser/laser_pointer_controller.cc:87: aura::Window::ConvertPointToTarget(event_root, current_window, On 2016/09/23 23:59:19, jdufault wrote: > Keep all of the point remapping logic together. Done. https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_con... ash/laser/laser_pointer_controller.cc:92: DestroyLaserPointerView(); On 2016/09/23 23:59:19, jdufault wrote: > I think we should continuing showing the laser pointer if it is already active > and the user drags the stylus over the palette (similar to the magnifier). Done. https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_con... ash/laser/laser_pointer_controller.cc:93: return; On 2016/09/23 23:59:19, jdufault wrote: > We should not be creating the view in the first place if the cursor is over the > palette and the view is not already created. Right now it seems like if the user > clicks on the stylus we: > > 1. Create view > 2. Destroy view Done. https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_con... ash/laser/laser_pointer_controller.cc:114: // interal collections timestamps if released. On 2016/09/23 23:59:19, jdufault wrote: > interal -> internal Done. https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_poi... File ash/laser/laser_pointer_points.cc (right): https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_poi... ash/laser/laser_pointer_points.cc:22: collection_latest_time_ = current_time; On 2016/09/23 23:59:19, jdufault wrote: > We should always be updating collection_latest_time_, otherwise the logic in > MoveForwardInTime will use a stale value. Done. https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_poi... ash/laser/laser_pointer_points.cc:31: collection_latest_time_ = base::Time::Now(); On 2016/09/23 23:59:19, jdufault wrote: > Call MoveFowardInTime(base::Time::Now()) Done. https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_poi... ash/laser/laser_pointer_points.cc:35: void LaserPointerPoints::MoveForwardInTime(const base::Time& new_latest_time) { On 2016/09/23 23:59:19, jdufault wrote: > Rename one of the MoveForwardInTime methods so they are not overloaded. Done. https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_poi... ash/laser/laser_pointer_points.cc:89: if (IsEmpty()) On 2016/09/23 23:59:19, jdufault wrote: > Why do we need this if? Done. https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_poi... File ash/laser/laser_pointer_points.h (right): https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_poi... ash/laser/laser_pointer_points.h:37: // too On 2016/09/23 23:59:19, jdufault wrote: > Fix formatting Done. https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_poi... File ash/laser/laser_pointer_points_test_api.cc (right): https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_poi... ash/laser/laser_pointer_points_test_api.cc:28: instance_->collection_latest_time_ = new_time; On 2016/09/23 23:59:20, jdufault wrote: > Should this be calling SetLatestTime(new_time)? Done. https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_view.h File ash/laser/laser_pointer_view.h (right): https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_vie... ash/laser/laser_pointer_view.h:38: void SetIsFadingAway(bool is_fading); On 2016/09/23 23:59:20, jdufault wrote: > Rename to set_is_fading_away. Trival methods (like this one) should be inlined > as well. > Done. https://codereview.chromium.org/2362063002/diff/1/ash/laser/laser_pointer_vie... ash/laser/laser_pointer_view.h:39: bool GetIsFadingAway() const; On 2016/09/23 23:59:20, jdufault wrote: > Rename to is_fading_away() and inline the definition. Done.
https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... File ash/laser/laser_pointer_controller.cc (right): https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_controller.cc:88: // Remap point from where it was captured to the display it is actually on. Above line 85, what about this comment instead: // Compute the event coordinate relative to the display it is currently on (and not the one the event was captured on). https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_controller.cc:92: // If the stylus is over the palette icon or widget do not display the laser. This comment seems misleading because the laser pointer can be visible here if it is fading away. https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_controller.cc:99: // create a new one and stop the timer. nit: replace `one` with `view` Why the comment about stopping the timer? https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_controller.cc:113: is_fading_away_ = event->type() == ui::ET_MOUSE_RELEASED; Shouldn't this check happen before we add the point? https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_controller.cc:114: // Start the timer to add stationary points if dragged, or too advance the too => to https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_controller.cc:141: // add points when |laser_pointer_view_| is nullptr. nit: nullptr => null https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_controller.cc:145: laser_pointer_view_->Stop(); If we are destroying the view, why do we need to call Stop? https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... File ash/laser/laser_pointer_controller.h (right): https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_controller.h:65: bool enabled_ = false; newline https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... File ash/laser/laser_pointer_points.cc (right): https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_points.cc:44: UpdateAges(); What about doing something like this? auto delta = new_latest_time - collection_latest_time_; float lifespan_change = delta / life_duration_; for (auto& point : points_) point.age -= lifespan_change; ClearOldPoints(); ClearOldPoints can also be simplified to check if the age <= 0. https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... File ash/laser/laser_pointer_points.h (right): https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_points.h:26: base::Time creation_time; Why does the point store its creation_time? age can be computed relative to collection_latest_time_. https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... File ash/laser/laser_pointer_points_test_api.cc (right): https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_points_test_api.cc:32: if (index >= GetNumberOfPoints()) DCHECK on this? https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... File ash/laser/laser_pointer_points_test_api.h (right): https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_points_test_api.h:24: double AgeAtIndex(int index); What about doing GetPointAtIndex(int index) and then the calling code can do EXPECT_EQ(1, GetPointAtIndex(0).age); https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... File ash/laser/laser_pointer_view.cc (right): https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_view.cc:126: gfx::Vector2d previous_point_vector = FYI, this variable is very confusing (it's not clear that this is used to share initialization state) - I thought the vector suffix referred to some direction/etc, not the type of the variable. I'd remove the helper variable entirely and initialize previous_point directly. https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_view.cc:130: gfx::Point current_point; Declare this on line 144 since it is not used outside of the for loop. https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... File ash/laser/laser_pointer_view.h (right): https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_view.h:9: #include "ash/laser/laser_pointer_points.h" nit: restore newline
https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... File ash/laser/laser_pointer_controller.cc (right): https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_controller.cc:88: // Remap point from where it was captured to the display it is actually on. On 2016/10/04 22:04:12, jdufault wrote: > Above line 85, what about this comment instead: > > // Compute the event coordinate relative to the display it is currently on (and > not the one the event was captured on). Done. https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_controller.cc:92: // If the stylus is over the palette icon or widget do not display the laser. On 2016/10/04 22:04:12, jdufault wrote: > This comment seems misleading because the laser pointer can be visible here if > it is fading away. Done. https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_controller.cc:99: // create a new one and stop the timer. On 2016/10/04 22:04:12, jdufault wrote: > nit: replace `one` with `view` > > Why the comment about stopping the timer? Done. https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_controller.cc:113: is_fading_away_ = event->type() == ui::ET_MOUSE_RELEASED; On 2016/10/04 22:04:12, jdufault wrote: > Shouldn't this check happen before we add the point? Done. https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_controller.cc:114: // Start the timer to add stationary points if dragged, or too advance the On 2016/10/04 22:04:12, jdufault wrote: > too => to Done. https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_controller.cc:141: // add points when |laser_pointer_view_| is nullptr. On 2016/10/04 22:04:12, jdufault wrote: > nit: nullptr => null Done. https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_controller.cc:145: laser_pointer_view_->Stop(); On 2016/10/04 22:04:12, jdufault wrote: > If we are destroying the view, why do we need to call Stop? Done. https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... File ash/laser/laser_pointer_controller.h (right): https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_controller.h:65: bool enabled_ = false; On 2016/10/04 22:04:13, jdufault wrote: > newline Done. https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... File ash/laser/laser_pointer_points.cc (right): https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_points.cc:44: UpdateAges(); On 2016/10/04 22:04:13, jdufault wrote: > What about doing something like this? > > auto delta = new_latest_time - collection_latest_time_; > float lifespan_change = delta / life_duration_; > for (auto& point : points_) > point.age -= lifespan_change; > > ClearOldPoints(); > > ClearOldPoints can also be simplified to check if the age <= 0. Done. https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... File ash/laser/laser_pointer_points.h (right): https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_points.h:26: base::Time creation_time; On 2016/10/04 22:04:13, jdufault wrote: > Why does the point store its creation_time? age can be computed relative to > collection_latest_time_. Done. https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... File ash/laser/laser_pointer_points_test_api.cc (right): https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_points_test_api.cc:32: if (index >= GetNumberOfPoints()) On 2016/10/04 22:04:13, jdufault wrote: > DCHECK on this? Done. https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... File ash/laser/laser_pointer_points_test_api.h (right): https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_points_test_api.h:24: double AgeAtIndex(int index); On 2016/10/04 22:04:13, jdufault wrote: > What about doing GetPointAtIndex(int index) and then the calling code can do > > EXPECT_EQ(1, GetPointAtIndex(0).age); Done. https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... File ash/laser/laser_pointer_view.cc (right): https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_view.cc:126: gfx::Vector2d previous_point_vector = On 2016/10/04 22:04:13, jdufault wrote: > FYI, this variable is very confusing (it's not clear that this is used to share > initialization state) - I thought the vector suffix referred to some > direction/etc, not the type of the variable. > > I'd remove the helper variable entirely and initialize previous_point directly. Done. https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_view.cc:130: gfx::Point current_point; On 2016/10/04 22:04:13, jdufault wrote: > Declare this on line 144 since it is not used outside of the for loop. It's used on line 165. https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... File ash/laser/laser_pointer_view.h (right): https://codereview.chromium.org/2362063002/diff/40001/ash/laser/laser_pointer... ash/laser/laser_pointer_view.h:9: #include "ash/laser/laser_pointer_points.h" On 2016/10/04 22:04:13, jdufault wrote: > nit: restore newline Done.
https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... File ash/laser/laser_pointer_controller.cc (right): https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... ash/laser/laser_pointer_controller.cc:93: // If the stylus is over the palette icon or widget do not display the laser, I would say something like // Do not start actively adding points to the laser if the stylus is over the palette icon or widget. Note that the laser may be visible at this point if it is fading away. https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... ash/laser/laser_pointer_controller.cc:95: if ((!laser_pointer_view_ || is_fading_away_) && This logic is only applicable for ui::ET_MOUSE_PRESSED events, right? What about moving this if condition inside of the if block on line 102? I think it will make the data flow a bit easier to understand since it makes that assumption explicit. Actually, if you moved this logic into that if, you could just do if (event->type() == ui::ET_MOUSE_PRESSED) { // Do not start a new laser session if the event is over the // palette. if (PaletteContainsPointInScreen(event_location)) return; DestroyLaserPointerView(); SwitchTargetRootWindowIfNeeded(current_window); } Note that the SwitchTargetRootWindowIfNeeded is moved inside of the if. I think this is fine because you can't move a stylus seamlessly across two displays - there is going to be an up/down event at some point. https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... File ash/laser/laser_pointer_controller_test_api.cc (right): https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... ash/laser/laser_pointer_controller_test_api.cc:27: bool LaserPointerControllerTestApi::IsFadingOut() { nit: rename to IsFadingAway() https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... File ash/laser/laser_pointer_points.cc (right): https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... ash/laser/laser_pointer_points.cc:18: base::Time current_time = base::Time::Now(); Remove this helper variable. https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... ash/laser/laser_pointer_points.cc:35: auto delta = new_latest_time - collection_latest_time_; Specify type https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... ash/laser/laser_pointer_points.cc:38: for (auto& point : points_) Specify type https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... File ash/laser/laser_pointer_points.h (right): https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... ash/laser/laser_pointer_points.h:28: double age; Going from 0 -> 1 seems a bit strange to me (I'd expect 1 => new, 0 => old), but with the name 'age' it works the 0 -> 1 works. https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... File ash/laser/laser_pointer_view.cc (right): https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... ash/laser/laser_pointer_view.cc:88: void LaserPointerView::AddNewPoint(const gfx::Point& new_point, Instead of calling AddNewPoint(dummy, true) what about adding a new method, LaserPointerView::UpdateTime() { laser_points_.MoveForwardToCurrentTime(); OnPointsUpdated(); } LaserPointerView::AddNewPoint(const gfx::Point& point) { laser_points_.AddPoint(point); OnPointsUpdated(); } // common code in OnPointsUpdated() https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... ash/laser/laser_pointer_view.cc:126: gfx::Point previous_point = gfx::Point previous_point(..., ...);
https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... File ash/laser/laser_pointer_controller.cc (right): https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... ash/laser/laser_pointer_controller.cc:93: // If the stylus is over the palette icon or widget do not display the laser, On 2016/10/05 18:08:51, jdufault wrote: > I would say something like > > // Do not start actively adding points to the laser if the stylus is over the > palette icon or widget. Note that the laser may be visible at this point if it > is fading away. Done. https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... ash/laser/laser_pointer_controller.cc:95: if ((!laser_pointer_view_ || is_fading_away_) && On 2016/10/05 18:08:50, jdufault wrote: > This logic is only applicable for ui::ET_MOUSE_PRESSED events, right? What about > moving this if condition inside of the if block on line 102? I think it will > make the data flow a bit easier to understand since it makes that assumption > explicit. > > Actually, if you moved this logic into that if, you could just do > > if (event->type() == ui::ET_MOUSE_PRESSED) { > // Do not start a new laser session if the event is over the > // palette. > if (PaletteContainsPointInScreen(event_location)) > return; > > DestroyLaserPointerView(); > SwitchTargetRootWindowIfNeeded(current_window); > } > > Note that the SwitchTargetRootWindowIfNeeded is moved inside of the if. I think > this is fine because you can't move a stylus seamlessly across two displays - > there is going to be an up/down event at some point. If SwitchTargetRootWindowIfNeeded is not outside the when you drag across two windows and hold the mouse button down the laser will disappear and you will be left trying to drag. https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... File ash/laser/laser_pointer_controller_test_api.cc (right): https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... ash/laser/laser_pointer_controller_test_api.cc:27: bool LaserPointerControllerTestApi::IsFadingOut() { On 2016/10/05 18:08:51, jdufault wrote: > nit: rename to IsFadingAway() Done. https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... File ash/laser/laser_pointer_points.cc (right): https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... ash/laser/laser_pointer_points.cc:18: base::Time current_time = base::Time::Now(); On 2016/10/05 18:08:51, jdufault wrote: > Remove this helper variable. Done. https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... ash/laser/laser_pointer_points.cc:35: auto delta = new_latest_time - collection_latest_time_; On 2016/10/05 18:08:51, jdufault wrote: > Specify type Done. https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... ash/laser/laser_pointer_points.cc:38: for (auto& point : points_) On 2016/10/05 18:08:51, jdufault wrote: > Specify type Done. https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... File ash/laser/laser_pointer_points.h (right): https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... ash/laser/laser_pointer_points.h:28: double age; On 2016/10/05 18:08:51, jdufault wrote: > Going from 0 -> 1 seems a bit strange to me (I'd expect 1 => new, 0 => old), but > with the name 'age' it works the 0 -> 1 works. Is it normal convention to go the other way? https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... File ash/laser/laser_pointer_view.cc (right): https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... ash/laser/laser_pointer_view.cc:88: void LaserPointerView::AddNewPoint(const gfx::Point& new_point, On 2016/10/05 18:08:51, jdufault wrote: > Instead of calling AddNewPoint(dummy, true) what about adding a new method, > > LaserPointerView::UpdateTime() { > laser_points_.MoveForwardToCurrentTime(); > OnPointsUpdated(); > } > > LaserPointerView::AddNewPoint(const gfx::Point& point) { > laser_points_.AddPoint(point); > OnPointsUpdated(); > } > > // common code in OnPointsUpdated() Done. https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... ash/laser/laser_pointer_view.cc:126: gfx::Point previous_point = On 2016/10/05 18:08:51, jdufault wrote: > gfx::Point previous_point(..., ...); Done.
https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... File ash/laser/laser_pointer_points.h (right): https://codereview.chromium.org/2362063002/diff/80001/ash/laser/laser_pointer... ash/laser/laser_pointer_points.h:28: double age; On 2016/10/05 21:15:22, sammiequon wrote: > On 2016/10/05 18:08:51, jdufault wrote: > > Going from 0 -> 1 seems a bit strange to me (I'd expect 1 => new, 0 => old), > but > > with the name 'age' it works the 0 -> 1 works. > > Is it normal convention to go the other way? I think so, but with 'age' it is confusing. ie, lifetime 1 => 0 https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... File ash/laser/laser_pointer_controller.cc (right): https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... ash/laser/laser_pointer_controller.cc:78: // Check if the current window is valid. Destroy the laser pointer view if This comment is repeating the code without giving any additional detail. Please either remove it or replace it with the additional detail. https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... ash/laser/laser_pointer_controller.cc:104: SwitchTargetRootWindowIfNeeded(current_window); Won't this start a laser session if I hold the laser pointer down and drag it around the palette widget? https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... File ash/laser/laser_pointer_points.cc (right): https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... ash/laser/laser_pointer_points.cc:21: MoveForwardToTime(base::Time::Now()); Call this at the start of the function. It's needlessly splitting up the initialization of new_point. https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... ash/laser/laser_pointer_points.cc:29: void LaserPointerPoints::MoveForwardToTime(const base::Time& new_latest_time) { Drop new_ on new_latest_time_, it is redundant. https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... ash/laser/laser_pointer_points.cc:31: collection_latest_time_ = new_latest_time; If this is true then points_ must be empty, right? I would add DCHECK_IMPLIES(collection_latest_time_.is_null(), !points_.length()); The function doesn't really make a ton of sense with this if as well, since it then doesn't do anything. I would instead do if (!collection_latest_time_.is_null()) { // update point ages } collection_latest_time_ = latest_time_; https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... ash/laser/laser_pointer_points.cc:39: ClearOldPoints(); Call ClearOldPoints after updating collection_latest_time_ to keep related logic together. https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... ash/laser/laser_pointer_points.cc:83: void LaserPointerPoints::ClearOldPoints() { It looks like the only caller of this method is MoveForwardToTime. I would inline the method. https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... File ash/laser/laser_pointer_points.h (right): https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... ash/laser/laser_pointer_points.h:40: void MoveForwardToCurrentTime(); It looks like this is only used once. I don't think it's worthwhile to have a helper method. https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... File ash/laser/laser_pointer_points_test_api.cc (right): https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... ash/laser/laser_pointer_points_test_api.cc:25: instance_->MoveForwardToTime(new_time); Keep new_point initialization together; move the MoveForwardToTime call above the new_point declaration. https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... ash/laser/laser_pointer_points_test_api.cc:31: DCHECK(index >= GetNumberOfPoints()); Is this DCHECK right? It looks like it is inverted. Make sure you build chrome with DCHECK enabled. DHCECK(index < GetNumberOfPoints())
The CQ bit was checked by sammiequon@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_...)
The CQ bit was checked by sammiequon@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 sammiequon@chromium.org
Patchset #7 (id:120001) has been deleted
The CQ bit was checked by sammiequon@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.
https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... File ash/laser/laser_pointer_controller.cc (right): https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... ash/laser/laser_pointer_controller.cc:78: // Check if the current window is valid. Destroy the laser pointer view if On 2016/10/05 21:36:05, jdufault wrote: > This comment is repeating the code without giving any additional detail. Please > either remove it or replace it with the additional detail. Done. https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... ash/laser/laser_pointer_controller.cc:104: SwitchTargetRootWindowIfNeeded(current_window); On 2016/10/05 21:36:05, jdufault wrote: > Won't this start a laser session if I hold the laser pointer down and drag it > around the palette widget? Yes this won't work properly. I went back to the original. https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... File ash/laser/laser_pointer_points.cc (right): https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... ash/laser/laser_pointer_points.cc:21: MoveForwardToTime(base::Time::Now()); On 2016/10/05 21:36:05, jdufault wrote: > Call this at the start of the function. It's needlessly splitting up the > initialization of new_point. Done. https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... ash/laser/laser_pointer_points.cc:29: void LaserPointerPoints::MoveForwardToTime(const base::Time& new_latest_time) { On 2016/10/05 21:36:05, jdufault wrote: > Drop new_ on new_latest_time_, it is redundant. Done. https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... ash/laser/laser_pointer_points.cc:31: collection_latest_time_ = new_latest_time; On 2016/10/05 21:36:05, jdufault wrote: > If this is true then points_ must be empty, right? > > I would add > > DCHECK_IMPLIES(collection_latest_time_.is_null(), !points_.length()); > > The function doesn't really make a ton of sense with this if as well, since it > then doesn't do anything. I would instead do > > if (!collection_latest_time_.is_null()) { > // update point ages > } > > collection_latest_time_ = latest_time_; It seems DCHECK_IMPLIES is only used in v8 stuff. I think the way it's being used null time implies empty collection but thats becaused the laser pointer view gets reset every time the collection becomes empty. https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... ash/laser/laser_pointer_points.cc:39: ClearOldPoints(); On 2016/10/05 21:36:05, jdufault wrote: > Call ClearOldPoints after updating collection_latest_time_ to keep related logic > together. Done. https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... ash/laser/laser_pointer_points.cc:83: void LaserPointerPoints::ClearOldPoints() { On 2016/10/05 21:36:05, jdufault wrote: > It looks like the only caller of this method is MoveForwardToTime. I would > inline the method. Done. https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... File ash/laser/laser_pointer_points.h (right): https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... ash/laser/laser_pointer_points.h:40: void MoveForwardToCurrentTime(); On 2016/10/05 21:36:05, jdufault wrote: > It looks like this is only used once. I don't think it's worthwhile to have a > helper method. Done. https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... File ash/laser/laser_pointer_points_test_api.cc (right): https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... ash/laser/laser_pointer_points_test_api.cc:25: instance_->MoveForwardToTime(new_time); On 2016/10/05 21:36:05, jdufault wrote: > Keep new_point initialization together; move the MoveForwardToTime call above > the new_point declaration. Done. https://codereview.chromium.org/2362063002/diff/100001/ash/laser/laser_pointe... ash/laser/laser_pointer_points_test_api.cc:31: DCHECK(index >= GetNumberOfPoints()); On 2016/10/05 21:36:05, jdufault wrote: > Is this DCHECK right? It looks like it is inverted. Make sure you build chrome > with DCHECK enabled. > > DHCECK(index < GetNumberOfPoints()) Done.
https://codereview.chromium.org/2362063002/diff/140001/ash/laser/laser_pointe... File ash/laser/laser_pointer_controller.cc (right): https://codereview.chromium.org/2362063002/diff/140001/ash/laser/laser_pointe... ash/laser/laser_pointer_controller.cc:94: (is_fading_away_ || PaletteContainsPointInScreen(event_location))) { We should allow a new session even if the laser is fading away. For example, if the user lifts the stylus up off the screen for a brief second and puts it down before the laser has finished fading away, then they will not have a laser pointer until lifting up and pressing down again. When is the expression (!laser_pointer_view_ && is_fading_away_) true? https://codereview.chromium.org/2362063002/diff/140001/ash/laser/laser_pointe... ash/laser/laser_pointer_controller.cc:103: SwitchTargetRootWindowIfNeeded(current_window); I think this code would be much easier if the implicit state machine was made more explicit. For example: if (!enabled_) return; if (!event_is_not_pen) return; if (!current_window) { DestroyLaserPointerView(); return; } if (event_type_is_down && !OverStylusTools()) { // The Laser could still be fading away from a previous event sequence. DestroyLaserPointerView(); ShowLaserPointer() event->StopPropagation() } if (event_type_is_move && laser_pointer_view_) { ChangeLaserPointerWidgetIfNeeded(); laser_pointer_view->AddNewPoint(current_mouse_location_); stationary_timer_repeat_count_ = 0; event->StopPropagation() } if (event_type_is_up) { laser_pointer_view->UpdateTime(); stationary_timer_repeat_count_ = 0; is_fading_away_ = true; event->StopPropagation() } The event->StopPropagation calls can be deduped by moving the three ifs into a helper function that returns true if the event was handled, ie, if (UpdateLaserView(event)) event->StopPropagation() I'm not convinced that is worthwhile since it is only one statement. https://codereview.chromium.org/2362063002/diff/140001/ash/laser/laser_pointe... File ash/laser/laser_pointer_points.cc (right): https://codereview.chromium.org/2362063002/diff/140001/ash/laser/laser_pointe... ash/laser/laser_pointer_points.cc:10: #include "base/macros.h" Include this in the header file since you're using DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/2362063002/diff/140001/ash/laser/laser_pointe... ash/laser/laser_pointer_points.cc:30: // Update the ages of the points based on the change in new latest time. DCHECK(!collection_latest_time_.is_null()); https://codereview.chromium.org/2362063002/diff/140001/ash/laser/laser_pointe... ash/laser/laser_pointer_points.cc:30: // Update the ages of the points based on the change in new latest time. What about // Increase the age of points based on how much time has elapsed.
https://codereview.chromium.org/2362063002/diff/140001/ash/laser/laser_pointe... File ash/laser/laser_pointer_controller.cc (right): https://codereview.chromium.org/2362063002/diff/140001/ash/laser/laser_pointe... ash/laser/laser_pointer_controller.cc:94: (is_fading_away_ || PaletteContainsPointInScreen(event_location))) { On 2016/10/13 00:12:30, jdufault wrote: > We should allow a new session even if the laser is fading away. > > For example, if the user lifts the stylus up off the screen for a brief second > and puts it down before the laser has finished fading away, then they will not > have a laser pointer until lifting up and pressing down again. > > When is the expression (!laser_pointer_view_ && is_fading_away_) true? Yeah that expression never happens, so it was working as expected previously. https://codereview.chromium.org/2362063002/diff/140001/ash/laser/laser_pointe... ash/laser/laser_pointer_controller.cc:103: SwitchTargetRootWindowIfNeeded(current_window); On 2016/10/13 00:12:30, jdufault wrote: > I think this code would be much easier if the implicit state machine was made > more explicit. For example: > > if (!enabled_) > return; > if (!event_is_not_pen) > return; > if (!current_window) { > DestroyLaserPointerView(); > return; > } > > if (event_type_is_down && !OverStylusTools()) { > // The Laser could still be fading away from a previous event sequence. > DestroyLaserPointerView(); > ShowLaserPointer() > event->StopPropagation() > } > > if (event_type_is_move && laser_pointer_view_) { > ChangeLaserPointerWidgetIfNeeded(); > laser_pointer_view->AddNewPoint(current_mouse_location_); > stationary_timer_repeat_count_ = 0; > event->StopPropagation() > } > > if (event_type_is_up) { > laser_pointer_view->UpdateTime(); > stationary_timer_repeat_count_ = 0; > is_fading_away_ = true; > event->StopPropagation() > } > > > The event->StopPropagation calls can be deduped by moving the three ifs into a > helper function that returns true if the event was handled, ie, > > if (UpdateLaserView(event)) > event->StopPropagation() > > I'm not convinced that is worthwhile since it is only one statement. Done. https://codereview.chromium.org/2362063002/diff/140001/ash/laser/laser_pointe... File ash/laser/laser_pointer_points.cc (right): https://codereview.chromium.org/2362063002/diff/140001/ash/laser/laser_pointe... ash/laser/laser_pointer_points.cc:10: #include "base/macros.h" On 2016/10/13 00:12:30, jdufault wrote: > Include this in the header file since you're using DISALLOW_COPY_AND_ASSIGN. Done. https://codereview.chromium.org/2362063002/diff/140001/ash/laser/laser_pointe... ash/laser/laser_pointer_points.cc:30: // Update the ages of the points based on the change in new latest time. On 2016/10/13 00:12:30, jdufault wrote: > What about > > // Increase the age of points based on how much time has elapsed. Done. https://codereview.chromium.org/2362063002/diff/140001/ash/laser/laser_pointe... ash/laser/laser_pointer_points.cc:30: // Update the ages of the points based on the change in new latest time. On 2016/10/13 00:12:30, jdufault wrote: > DCHECK(!collection_latest_time_.is_null()); Done.
https://codereview.chromium.org/2362063002/diff/180001/ash/laser/laser_pointe... File ash/laser/laser_pointer_controller.cc (right): https://codereview.chromium.org/2362063002/diff/180001/ash/laser/laser_pointe... ash/laser/laser_pointer_controller.cc:135: if (start_timer) { Instead of having a boolean why not just extract this into a new method, RestartTimer()? https://codereview.chromium.org/2362063002/diff/180001/ash/laser/laser_pointe... File ash/laser/laser_pointer_points.cc (right): https://codereview.chromium.org/2362063002/diff/180001/ash/laser/laser_pointe... ash/laser/laser_pointer_points.cc:29: // Increase the age of points based on how much time has elapsed. Newline between the DCHECK and the comment. https://codereview.chromium.org/2362063002/diff/180001/ash/laser/laser_pointe... ash/laser/laser_pointer_points.cc:39: [](LaserPoint& p) { return p.age < 1.0; }); const LaserPoint& p
https://codereview.chromium.org/2362063002/diff/180001/ash/laser/laser_pointe... File ash/laser/laser_pointer_controller.cc (right): https://codereview.chromium.org/2362063002/diff/180001/ash/laser/laser_pointe... ash/laser/laser_pointer_controller.cc:135: if (start_timer) { On 2016/10/14 19:13:09, jdufault wrote: > Instead of having a boolean why not just extract this into a new method, > RestartTimer()? Done. https://codereview.chromium.org/2362063002/diff/180001/ash/laser/laser_pointe... File ash/laser/laser_pointer_points.cc (right): https://codereview.chromium.org/2362063002/diff/180001/ash/laser/laser_pointe... ash/laser/laser_pointer_points.cc:29: // Increase the age of points based on how much time has elapsed. On 2016/10/14 19:13:09, jdufault wrote: > Newline between the DCHECK and the comment. Done. https://codereview.chromium.org/2362063002/diff/180001/ash/laser/laser_pointe... ash/laser/laser_pointer_points.cc:39: [](LaserPoint& p) { return p.age < 1.0; }); On 2016/10/14 19:13:09, jdufault wrote: > const LaserPoint& p Done.
lgtm https://codereview.chromium.org/2362063002/diff/200001/ash/laser/laser_pointe... File ash/laser/laser_pointer_controller.h (right): https://codereview.chromium.org/2362063002/diff/200001/ash/laser/laser_pointe... ash/laser/laser_pointer_controller.h:60: void UpdateLaserPointerView(aura::Window* current_window, Update comment (no more timer section). https://codereview.chromium.org/2362063002/diff/200001/ash/laser/laser_pointe... File ash/laser/laser_pointer_points.h (right): https://codereview.chromium.org/2362063002/diff/200001/ash/laser/laser_pointe... ash/laser/laser_pointer_points.h:29: double age; double age = 0; Then you can remove all of the explicit sets to 0.
https://codereview.chromium.org/2362063002/diff/200001/ash/laser/laser_pointe... File ash/laser/laser_pointer_controller.h (right): https://codereview.chromium.org/2362063002/diff/200001/ash/laser/laser_pointe... ash/laser/laser_pointer_controller.h:60: void UpdateLaserPointerView(aura::Window* current_window, On 2016/10/14 21:59:47, jdufault wrote: > Update comment (no more timer section). Done. https://codereview.chromium.org/2362063002/diff/200001/ash/laser/laser_pointe... File ash/laser/laser_pointer_points.h (right): https://codereview.chromium.org/2362063002/diff/200001/ash/laser/laser_pointe... ash/laser/laser_pointer_points.h:29: double age; On 2016/10/14 21:59:47, jdufault wrote: > double age = 0; > > Then you can remove all of the explicit sets to 0. Done.
sammiequon@chromium.org changed reviewers: + jamescook@chromium.org
On 2016/10/17 19:12:24, sammiequon wrote: > https://codereview.chromium.org/2362063002/diff/200001/ash/laser/laser_pointe... > File ash/laser/laser_pointer_controller.h (right): > > https://codereview.chromium.org/2362063002/diff/200001/ash/laser/laser_pointe... > ash/laser/laser_pointer_controller.h:60: void > UpdateLaserPointerView(aura::Window* current_window, > On 2016/10/14 21:59:47, jdufault wrote: > > Update comment (no more timer section). > > Done. > > https://codereview.chromium.org/2362063002/diff/200001/ash/laser/laser_pointe... > File ash/laser/laser_pointer_points.h (right): > > https://codereview.chromium.org/2362063002/diff/200001/ash/laser/laser_pointe... > ash/laser/laser_pointer_points.h:29: double age; > On 2016/10/14 21:59:47, jdufault wrote: > > double age = 0; > > > > Then you can remove all of the explicit sets to 0. > > Done. jamescook@ - Please take a look. Thanks!
https://codereview.chromium.org/2362063002/diff/220001/ash/laser/laser_pointe... File ash/laser/laser_pointer_points.cc (right): https://codereview.chromium.org/2362063002/diff/220001/ash/laser/laser_pointe... ash/laser/laser_pointer_points.cc:22: new_point.age = 0.0; Remove since the default age is 0.0
lgtm
The CQ bit was checked by sammiequon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdufault@chromium.org Link to the patchset: https://codereview.chromium.org/2362063002/#ps220001 (title: "Fixed patch set 10 errors.")
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 sammiequon@chromium.org
The CQ bit was checked by sammiequon@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.
https://codereview.chromium.org/2362063002/diff/220001/ash/laser/laser_pointe... File ash/laser/laser_pointer_points.cc (right): https://codereview.chromium.org/2362063002/diff/220001/ash/laser/laser_pointe... ash/laser/laser_pointer_points.cc:22: new_point.age = 0.0; On 2016/10/17 19:14:13, jdufault wrote: > Remove since the default age is 0.0 Done.
The CQ bit was checked by sammiequon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdufault@chromium.org, jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2362063002/#ps240001 (title: "Nit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== cros: Laser pointer fades out on release, do not cover palette. The laser pointer fades out quickly now on release instead of disappearing. It gets hidden before the palette now. I check if the event is over the palette first now. Also modified the laser points to be able to move forward in time (and thus delete points) without adding a new point. BUG=647795,649517 TEST=ash_unittests --gtest_filter="LaserPointer*" ========== to ========== cros: Laser pointer fades out on release, do not cover palette. The laser pointer fades out quickly now on release instead of disappearing. It gets hidden before the palette now. I check if the event is over the palette first now. Also modified the laser points to be able to move forward in time (and thus delete points) without adding a new point. BUG=647795,649517 TEST=ash_unittests --gtest_filter="LaserPointer*" ==========
Message was sent while issue was closed.
Committed patchset #12 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== cros: Laser pointer fades out on release, do not cover palette. The laser pointer fades out quickly now on release instead of disappearing. It gets hidden before the palette now. I check if the event is over the palette first now. Also modified the laser points to be able to move forward in time (and thus delete points) without adding a new point. BUG=647795,649517 TEST=ash_unittests --gtest_filter="LaserPointer*" ========== to ========== cros: Laser pointer fades out on release, do not cover palette. The laser pointer fades out quickly now on release instead of disappearing. It gets hidden before the palette now. I check if the event is over the palette first now. Also modified the laser points to be able to move forward in time (and thus delete points) without adding a new point. BUG=647795,649517 TEST=ash_unittests --gtest_filter="LaserPointer*" Committed: https://crrev.com/4034b4e474bb881411690569ce59e298676ab20f Cr-Commit-Position: refs/heads/master@{#425827} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/4034b4e474bb881411690569ce59e298676ab20f Cr-Commit-Position: refs/heads/master@{#425827} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
