Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2239743004:
Palette tool laser prototype. (Closed)
|
Created:
4 years, 4 months ago by sammiequon Modified:
4 years, 3 months ago CC:
chromium-reviews, kalyank, sadrul, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@patch Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPalette tool laser prototype.
Classes:
LaserPointerMode - Serves as controller as receiver of events.
LaserPointerPoints - Helper class that contains collections of the points.
LaserPointerView - Responsible for rendering the point(s).
Split of from 2231533004.
TEST=./out/Release/ash_unittests --gtest_filter=LaserPointer*
BUG=616143
Committed: https://crrev.com/01725f5d867a32b82bdd3adf5088cfbab0d54f29
Cr-Commit-Position: refs/heads/master@{#415496}
Patch Set 1 #Patch Set 2 : Addressed comments from issue 2231533004. #
Total comments: 112
Patch Set 3 : Fixed patch set 2 errors and removed patch dependency. #
Total comments: 103
Patch Set 4 : Fixed patch set 3 errors. #
Total comments: 60
Patch Set 5 : Fixed patch set 4 errors. #
Total comments: 58
Patch Set 6 : Fixed patch set 5 errors. #
Total comments: 42
Patch Set 7 : Fixed patch set 6 errors. #
Total comments: 26
Patch Set 8 : Fixed patch set 7 errors. #
Total comments: 39
Patch Set 9 : Fixed patch set 8 errors. #
Total comments: 24
Patch Set 10 : Rebased. #Patch Set 11 : Fixed patch set 9 errors. #
Total comments: 8
Patch Set 12 : Fixed patch set 11 errors. #Patch Set 13 : Refactored drawing code. #
Total comments: 39
Patch Set 14 : Fixed patch set 13 errors. #Patch Set 15 : Rebased. #Patch Set 16 : Added missing overrides. #Patch Set 17 : Added missing? include. #
Total comments: 1
Patch Set 18 : Rebased. #
Total comments: 2
Patch Set 19 : Fixed compiling issue. #Patch Set 20 : Modified test. #Patch Set 21 : Rebased. #Patch Set 22 : Rebased. #Messages
Total messages: 87 (49 generated)
Description was changed from ========== Palette tool laser prototype. Classes: LaserPointerMode - Serves as controller as receiver of events. LaserPointerPoints - Helper class that contains collections of the points. LaserPointerView - Responsible for rendering the point(s). BUG=616143 ========== to ========== Palette tool laser prototype. Classes: LaserPointerMode - Serves as controller as receiver of events. LaserPointerPoints - Helper class that contains collections of the points. LaserPointerView - Responsible for rendering the point(s). Split of from 2231533004. BUG=616143 ==========
sammiequon@chromium.org changed reviewers: + jdufault@chromium.org
jdufault@ - Please take a look. Laser only! Thanks.
On 2016/08/12 00:12:04, sammiequon wrote: > jdufault@ - Please take a look. Laser only! Thanks. jdufault@ - Addressed comments from https://codereview.chromium.org/2231533004.
On 2016/08/12 01:07:29, sammiequon wrote: > On 2016/08/12 00:12:04, sammiequon wrote: > > jdufault@ - Please take a look. Laser only! Thanks. > > jdufault@ - Addressed comments from https://codereview.chromium.org/2231533004. Okay - please close out the other issue.
https://codereview.chromium.org/2239743004/diff/20001/ash/common/palette_dele... File ash/common/palette_delegate.h (right): https://codereview.chromium.org/2239743004/diff/20001/ash/common/palette_dele... ash/common/palette_delegate.h:19: virtual void OnLaserModeEnabled() = 0; OnLaserPointerEnabled() OnLaserPointerDisabled() https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc (right): https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:22: base::TimeDelta::FromMilliseconds(int64_t{kPointLifeDurationMs})) { Drop int64_t cast. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:24: InitializeTimer(); Inline this function. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:42: if (WmShell::Get()->palette_delegate()) newline above if https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:50: if (WmShell::Get()->palette_delegate()) newline above if https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:74: void LaserPointerMode::StartTimer() { I'd consider inlining this method since it's only called once and it's one line. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:86: points_lock_.Acquire(); Instead of having a lock can we just make it so that this function is always called on the UI thread? You can verify this by adding the following statement: DCHECK_CURRENTLY_ON(BrowserThread::UI) https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:94: timer_repeat_count_++; This should only be modified inside of the lock. I think the variable decl also needs to be mutable. I'd strongly suggest staying away from making this code multithreaded - it's very easy to introduce bugs. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:109: if (enabled()) { Can this enabled() check be moved to the top of the function? https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:112: laser_points_.AddPoint(current_mouse_location_); What happens if only the timer callback adds points? Is behavior jerky/bad? https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.h (right): https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:16: // pointer as well as receives points and passes them off to be rendered. This needs to use actual strings/labels. See https://codereview.chromium.org/2235063002/ for an example, but you need to override CreateView. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:27: base::Lock points_lock_; Move all data members to the end of the class declaration, immediately before DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:41: // CommonPaletteTool overrides. CommonPaletteTool: https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:43: // views::PointerWatcher: Newline above https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:51: // Timer callback which adds a point where the mouse was last seen. This takes What about for the second sentence, This allows the trail to fade away when the mouse is stationary. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:51: // Timer callback which adds a point where the mouse was last seen. This takes Newline above comment. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_points.cc (right): https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:84: gfx::Rect oldBounds; This just expands the rect so it contains the point, right? We should see if it can be included in gfx::Rect directly. If it cannot be included in gfx::Rect, move this to anonymous namespace and have it return the expanded rect. I think this function can be removed entirely though - see below. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:88: bool xChanged = false; Can we find the min/max points and use gfx::BoundingRect? https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:112: for (auto it = points_.begin(); it != points_.end(); it++) { It looks like this is effectively computing the bottom-left / top-right points, so just rebuild the bounding box using gfx::BoundingRect. You can then remove the CalculateBounds function and eliminate a lot of the extra logic. That should also remove an additional source of errors (stale bounding box). https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:143: const LaserPoint& newPoint, new_point https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:144: std::vector<LaserPoint>& generatedPoints) { generated_points https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:148: LaserPoint lastPoint = points_.back(); laser_point https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:155: base::Time generatedTime = generated_time https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:158: normalized * float{(newPoint.creationTime - lastPoint.creationTime) Store normalized as a double and all of the casts can be eliminated. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:160: LaserPoint generatedPoint; generated_point https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:179: CalculateBounds(it->location); Find the min/max points and build bounding box from gfx::BoundingRect. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_points.h (right): https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.h:26: base::Time creationTime; creation_time https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.h:32: LaserPointerPoints(const LaserPointerPoints& old); Do we need copy ctor? Can we add DISALLOW_COPY_AND_ASSIGN to this class? https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.h:56: float DistanceMousePoints(const LaserPoint& point1, const LaserPoint& point2); DistanceBetween? Move this function to anonymous namespace in cc file. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.h:58: void GenerateCurvePoints(const LaserPoint& newPoint, new_point https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.h:59: std::vector<LaserPoint>& generatedPoints); generated_points https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.h:60: // Computes the new cummulative bounding box of the points. Spelling; cummulative => cumulative What does |point| do here? https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc (right): https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:26: void SetCurrentLocation(const gfx::Point& new_point) { set_current_location https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:26: void SetCurrentLocation(const gfx::Point& new_point) { Rename new_point to current_location https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:30: gfx::Point current_location_; private:? https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:31: // views::PointerWatcher: Overrides should go above member vars. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:44: TestLaserPointerMode(Delegate* delegate) : LaserPointerMode(delegate) {} Add dtor https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:46: // Expose the internal points structure for testing. private: https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:57: : laser_pointer_points_(base::TimeDelta::FromSecondsD(5.0)) {} Pull into constant https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:63: laser_pointer_mode_ = new TestLaserPointerMode(NULL); nullptr everywhere https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:73: protected: private https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:111: EXPECT_EQ(point4, watcher_.current_location_); Use a getter or make the var public and remove the trialing _ https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_view.cc (right): https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:45: widget->SetOpacity(1.f); Is this needed? https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:49: float DistanceBetweenPoints(const gfx::Point& point1, I feel like this is defined somewhere in the code base. If not, please only define it once in the CL. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:65: if (!points.IsEmpty()) { Just do an early return, so at the start of the fn add if (points.IsEmpty()) return; https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:79: normalized = float{(it->creationTime - oldest).InMillisecondsF()} / Store normalized as a double to eliminate all of the casts. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:92: paintColor = SkColorSetARGB(opacity, 255, 0, 0); Move constants to anonymous namespace. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:98: radius * 4 * radius) { What's radius * 4 * radius? https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:105: paintColor = SkColorSetARGB(kMouselaserUiStartOpacityValue, 255, 0, 0); Move constant to anonymous namespace https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:119: newBoundingBox.SetRect(0, 0, 1388, 768); ? https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:137: gfx::Rect oldBoundingBox, newBoundingBox; old_bounding_box, new_bounding_box https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_view.h (right): https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.h:13: #include "ui/gfx/animation/linear_animation.h" Cleanup includes https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.h:22: // help users with tracking the laser. What about "... to help users track the laser."? https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.h:32: // view::View overrides; // view::View: https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.h:35: LaserPointerPoints laser_points_; Instead of constantly copying points, what about using either a shared_ptr or having LaserPointerView own the points using a unique_ptr? The points would be exposed as a pointer that LaserPointerMode accesses. https://codereview.chromium.org/2239743004/diff/20001/chrome/browser/ui/ash/p... File chrome/browser/ui/ash/palette_delegate_chromeos.cc (right): https://codereview.chromium.org/2239743004/diff/20001/chrome/browser/ui/ash/p... chrome/browser/ui/ash/palette_delegate_chromeos.cc:17: // compound_event_filter.cc to be shown on every mouse movement. Can you clarify "to be shown on every mouse movement"?
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2239743004/diff/20001/ash/common/palette_dele... File ash/common/palette_delegate.h (right): https://codereview.chromium.org/2239743004/diff/20001/ash/common/palette_dele... ash/common/palette_delegate.h:19: virtual void OnLaserModeEnabled() = 0; On 2016/08/12 19:57:57, jdufault wrote: > OnLaserPointerEnabled() > OnLaserPointerDisabled() Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc (right): https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:22: base::TimeDelta::FromMilliseconds(int64_t{kPointLifeDurationMs})) { On 2016/08/12 19:57:58, jdufault wrote: > Drop int64_t cast. Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:24: InitializeTimer(); On 2016/08/12 19:57:58, jdufault wrote: > Inline this function. Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:42: if (WmShell::Get()->palette_delegate()) On 2016/08/12 19:57:58, jdufault wrote: > newline above if Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:50: if (WmShell::Get()->palette_delegate()) On 2016/08/12 19:57:58, jdufault wrote: > newline above if Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:74: void LaserPointerMode::StartTimer() { On 2016/08/12 19:57:57, jdufault wrote: > I'd consider inlining this method since it's only called once and it's one line. Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:86: points_lock_.Acquire(); On 2016/08/12 19:57:57, jdufault wrote: > Instead of having a lock can we just make it so that this function is always > called on the UI thread? You can verify this by adding the following statement: > > DCHECK_CURRENTLY_ON(BrowserThread::UI) Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:94: timer_repeat_count_++; On 2016/08/12 19:57:58, jdufault wrote: > This should only be modified inside of the lock. I think the variable decl also > needs to be mutable. > > I'd strongly suggest staying away from making this code multithreaded - it's > very easy to introduce bugs. Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:109: if (enabled()) { On 2016/08/12 19:57:58, jdufault wrote: > Can this enabled() check be moved to the top of the function? We need to update current_mouse_location_ when it is disabled so when it is enabled the dot shows up at the right place. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:112: laser_points_.AddPoint(current_mouse_location_); On 2016/08/12 19:57:58, jdufault wrote: > What happens if only the timer callback adds points? Is behavior jerky/bad? Yeah it lags a lot. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.h (right): https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:16: // pointer as well as receives points and passes them off to be rendered. On 2016/08/12 19:57:58, jdufault wrote: > This needs to use actual strings/labels. See > https://codereview.chromium.org/2235063002/ for an example, but you need to > override CreateView. Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:27: base::Lock points_lock_; On 2016/08/12 19:57:58, jdufault wrote: > Move all data members to the end of the class declaration, immediately before > DISALLOW_COPY_AND_ASSIGN. Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:41: // CommonPaletteTool overrides. On 2016/08/12 19:57:58, jdufault wrote: > CommonPaletteTool: Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:43: // views::PointerWatcher: On 2016/08/12 19:57:58, jdufault wrote: > Newline above Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:51: // Timer callback which adds a point where the mouse was last seen. This takes On 2016/08/12 19:57:58, jdufault wrote: > Newline above comment. Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:51: // Timer callback which adds a point where the mouse was last seen. This takes On 2016/08/12 19:57:58, jdufault wrote: > What about for the second sentence, > > This allows the trail to fade away when the mouse is stationary. Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_points.cc (right): https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:84: gfx::Rect oldBounds; On 2016/08/12 19:57:58, jdufault wrote: > This just expands the rect so it contains the point, right? We should see if it > can be included in gfx::Rect directly. > > If it cannot be included in gfx::Rect, move this to anonymous namespace and have > it return the expanded rect. > > I think this function can be removed entirely though - see below. I removed Recalculate bounds instead of CaculateBounds. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:88: bool xChanged = false; On 2016/08/12 19:57:58, jdufault wrote: > Can we find the min/max points and use gfx::BoundingRect? Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:112: for (auto it = points_.begin(); it != points_.end(); it++) { On 2016/08/12 19:57:58, jdufault wrote: > It looks like this is effectively computing the bottom-left / top-right points, > so just rebuild the bounding box using gfx::BoundingRect. You can then remove > the CalculateBounds function and eliminate a lot of the extra logic. That should > also remove an additional source of errors (stale bounding box). I removed Recalculate bounds instead of CaculateBounds. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:143: const LaserPoint& newPoint, On 2016/08/12 19:57:58, jdufault wrote: > new_point Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:144: std::vector<LaserPoint>& generatedPoints) { On 2016/08/12 19:57:58, jdufault wrote: > generated_points Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:148: LaserPoint lastPoint = points_.back(); On 2016/08/12 19:57:59, jdufault wrote: > laser_point Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:155: base::Time generatedTime = On 2016/08/12 19:57:58, jdufault wrote: > generated_time Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:158: normalized * float{(newPoint.creationTime - lastPoint.creationTime) On 2016/08/12 19:57:58, jdufault wrote: > Store normalized as a double and all of the casts can be eliminated. Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:160: LaserPoint generatedPoint; On 2016/08/12 19:57:58, jdufault wrote: > generated_point Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:179: CalculateBounds(it->location); On 2016/08/12 19:57:58, jdufault wrote: > Find the min/max points and build bounding box from gfx::BoundingRect. Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_points.h (right): https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.h:26: base::Time creationTime; On 2016/08/12 19:57:59, jdufault wrote: > creation_time Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.h:32: LaserPointerPoints(const LaserPointerPoints& old); On 2016/08/12 19:57:59, jdufault wrote: > Do we need copy ctor? > > Can we add DISALLOW_COPY_AND_ASSIGN to this class? Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.h:56: float DistanceMousePoints(const LaserPoint& point1, const LaserPoint& point2); On 2016/08/12 19:57:59, jdufault wrote: > DistanceBetween? > > Move this function to anonymous namespace in cc file. Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.h:58: void GenerateCurvePoints(const LaserPoint& newPoint, On 2016/08/12 19:57:59, jdufault wrote: > new_point Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.h:59: std::vector<LaserPoint>& generatedPoints); On 2016/08/12 19:57:59, jdufault wrote: > generated_points Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.h:60: // Computes the new cummulative bounding box of the points. On 2016/08/12 19:57:59, jdufault wrote: > Spelling; cummulative => cumulative > > What does |point| do here? Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc (right): https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:26: void SetCurrentLocation(const gfx::Point& new_point) { On 2016/08/12 19:57:59, jdufault wrote: > set_current_location Removed this class/test since it is in another CL. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:26: void SetCurrentLocation(const gfx::Point& new_point) { On 2016/08/12 19:57:59, jdufault wrote: > set_current_location Removed this class/test since it is in another CL. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:30: gfx::Point current_location_; On 2016/08/12 19:57:59, jdufault wrote: > private:? Removed this class/test since it is in another CL. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:31: // views::PointerWatcher: On 2016/08/12 19:57:59, jdufault wrote: > Overrides should go above member vars. > Removed this class/test since it is in another CL. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:44: TestLaserPointerMode(Delegate* delegate) : LaserPointerMode(delegate) {} On 2016/08/12 19:57:59, jdufault wrote: > Add dtor Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:46: // Expose the internal points structure for testing. On 2016/08/12 19:57:59, jdufault wrote: > private: Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:57: : laser_pointer_points_(base::TimeDelta::FromSecondsD(5.0)) {} On 2016/08/12 19:57:59, jdufault wrote: > Pull into constant Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:63: laser_pointer_mode_ = new TestLaserPointerMode(NULL); On 2016/08/12 19:57:59, jdufault wrote: > nullptr everywhere Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:73: protected: On 2016/08/12 19:57:59, jdufault wrote: > private Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:111: EXPECT_EQ(point4, watcher_.current_location_); On 2016/08/12 19:57:59, jdufault wrote: > Use a getter or make the var public and remove the trialing _ Removed this class/test since it is in another CL. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_view.cc (right): https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:45: widget->SetOpacity(1.f); On 2016/08/12 19:58:00, jdufault wrote: > Is this needed? Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:49: float DistanceBetweenPoints(const gfx::Point& point1, On 2016/08/12 19:57:59, jdufault wrote: > I feel like this is defined somewhere in the code base. If not, please only > define it once in the CL. I couldn't find it but I changed it to use base functions. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:65: if (!points.IsEmpty()) { On 2016/08/12 19:58:00, jdufault wrote: > Just do an early return, so at the start of the fn add > > if (points.IsEmpty()) > return; Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:79: normalized = float{(it->creationTime - oldest).InMillisecondsF()} / On 2016/08/12 19:58:00, jdufault wrote: > Store normalized as a double to eliminate all of the casts. Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:92: paintColor = SkColorSetARGB(opacity, 255, 0, 0); On 2016/08/12 19:58:00, jdufault wrote: > Move constants to anonymous namespace. Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:98: radius * 4 * radius) { On 2016/08/12 19:57:59, jdufault wrote: > What's radius * 4 * radius? Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:105: paintColor = SkColorSetARGB(kMouselaserUiStartOpacityValue, 255, 0, 0); On 2016/08/12 19:57:59, jdufault wrote: > Move constant to anonymous namespace Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:119: newBoundingBox.SetRect(0, 0, 1388, 768); On 2016/08/12 19:58:00, jdufault wrote: > ? Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:137: gfx::Rect oldBoundingBox, newBoundingBox; On 2016/08/12 19:57:59, jdufault wrote: > old_bounding_box, new_bounding_box Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_view.h (right): https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.h:13: #include "ui/gfx/animation/linear_animation.h" On 2016/08/12 19:58:00, jdufault wrote: > Cleanup includes Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.h:22: // help users with tracking the laser. On 2016/08/12 19:58:00, jdufault wrote: > What about "... to help users track the laser."? Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.h:32: // view::View overrides; On 2016/08/12 19:58:00, jdufault wrote: > // view::View: Done. https://codereview.chromium.org/2239743004/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.h:35: LaserPointerPoints laser_points_; On 2016/08/12 19:58:00, jdufault wrote: > Instead of constantly copying points, what about using either a shared_ptr or > having LaserPointerView own the points using a unique_ptr? The points would be > exposed as a pointer that LaserPointerMode accesses. Done. https://codereview.chromium.org/2239743004/diff/20001/chrome/browser/ui/ash/p... File chrome/browser/ui/ash/palette_delegate_chromeos.cc (right): https://codereview.chromium.org/2239743004/diff/20001/chrome/browser/ui/ash/p... chrome/browser/ui/ash/palette_delegate_chromeos.cc:17: // compound_event_filter.cc to be shown on every mouse movement. On 2016/08/12 19:58:00, jdufault wrote: > Can you clarify "to be shown on every mouse movement"? Done.
https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tool.cc (right): https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.cc:15: tool_manager->AddTool(base::WrapUnique(new LaserPointerMode(tool_manager))); base::MakeUnique<LaserPointerMode>(tool_manager) https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc (right): https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:28: false)); Can we make the timer repeating (change the false on line 28 to true) and then just use Stop/Reset to start/stop it? Then inside of AddStationaryPoint you won't have to constantly reset the timer. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.h (right): https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:15: class LaserPointerModeTestApi; Newline below this. Please also verify this is needed. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:20: friend class LaserPointerModeTestApi; Move friend class decl to right below private: https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:52: base::ThreadChecker thread_checker_; I'd move the thread_checker_ decl somewhere else so it doesn't separate timer_ and timer_repeat_count_ https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:53: mutable int timer_repeat_count_ = 0; Remove mutable since this should only be accessed on one thread https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_points.cc (right): https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:14: float DistanceBetweenMousePoints(const LaserPointerPoints::LaserPoint& point1, Make this take gfx::Point instances https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:22: life_duration_ = life_duration; Initialize in ctor list, ie, : life_duration_(life_duration) https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:39: // GenerateCurvePoints(newPoint, generatedPoints); Please remove GenerateCurvePoints as well as this comment if it isn't being used. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:91: minPoint = gfx::Point(std::min(minPoint.x(), point.x()), min_point https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:93: maxPoint = gfx::Point(std::max(maxPoint.x(), point.x()), max_point https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:101: gfx::Point minPoint = bounds_.origin(); min_point https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:102: gfx::Point maxPoint = bounds_.bottom_right(); max_point https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:105: minPoint.set_x(std::min(minPoint.x(), it->location.x())); Should this be std::max? https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:107: maxPoint.set_x(std::max(maxPoint.x(), it->location.x())); Should this be std::min? https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:109: points_.erase(it); erase invalidates the it iterator, which means that the erase needs to happen outside of the for loop. One solution is to use a helper vector and append the items to erase to it. Another is to use std::remove_if and filter the points that way, and then actually delete them using another <algorithm> function. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:116: bounds_ = gfx::BoundingRect(minPoint, maxPoint); How large is the points_ array? If it's not massive, I think it probably makes sense to recompute the bounding box entirely whenever we add a new point so that the code is simpler and easier to maintain. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_points.h (right): https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.h:29: // Constructor with a parameter to choose the fade out time off the points in off => of https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc (right): https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:20: ~LaserPointerModeTestApi() { delete instance_; } If LaserPointerModeTestApi takes ownership of LaserPointerMode it should be in the form of a std::unique_ptr. The general rule is to avoid manual memory management. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:22: void enable() { instance_->OnEnable(); } OnEnable https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:23: void disable() { instance_->OnDisable(); } OnDisable https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:38: class LaserPointerModeTest : public test::AshTestBase, Move the LaserPointerModeTest class into anonymous namespace. LaserPointerModeTestApi and the actual test functions should live in the ash namespace, though. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:39: public PaletteToolManager::Delegate { Does LaserPointerModeTest need to extend PaletteToolManager::Delegate? https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:54: AshTestBase::TearDown(); Make TearDown to the first line of the function https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:73: TEST_F(LaserPointerModeTest, LaserPointerPoints) { Please use a more descriptive test name - LaserPointerPoints doesn't describe the test. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:73: TEST_F(LaserPointerModeTest, LaserPointerPoints) { Add a comment describing what the test does. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:99: TEST_F(LaserPointerModeTest, LaserPointerView) { Add a comment describing the test. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:99: TEST_F(LaserPointerModeTest, LaserPointerView) { Please rename test to be more descriptive. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_view.cc (right): https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:8: #include "ash/common/shell_window_ids.h" newline between <memory> and "ash/..." https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:23: const double kMouselaserUiStartRadius = 4; Drop Mouse* prefix on all of these. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:23: const double kMouselaserUiStartRadius = 4; What about kPointInitialRadius = 4 kPointFinalRadius = 0.25 https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:25: const int kMouselaserUiStartOpacityValue = 200; kPointInitialOpacity kPointFinalOpacity https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:27: const int kMouselaserBlueValue = 0; kPointColor https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:29: const int kMouselaserRedValue = 255; Declare the RGB values together in an SkColor instance, ie, https://cs.chromium.org/chromium/src/ash/common/system/tray/tray_constants.cc... https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:52: gfx::Vector2d vector = point1 - point2; return (point1 - point2).Length() https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:75: double normalized; double normalized = 1.0; if (oldest != most_recent) { // ... } https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:75: double normalized; Add a comment along the lines of // Normalized is a value between [0,1] where 0 means the point is about to be removed and 1 means that the point was just added. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:75: double normalized; What do you think of the name 'progress' or 'relative_age' instead of 'normalized'? https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:88: kMouselaserUiStartRadius - Add a lerp function and replace all of the instances of this with it. double radius = LinearIntepolate(initial, final, progress) https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:95: paintColor = SkColorSetARGB(opacity, kMouselaserRedValue, paint_color https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:102: float distance_threshold = float{radius * 2}; Can we use a double to avoid the float cast? https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:120: } // namespace newline above https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:145: new_bounding_box.SetRect( Can you use only one bounding_box variable, all of the parameters will be evaluated before the function call so it should be safe. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:148: old_bounding_box.width() + 2 * kMouselaserUiStartRadius, old_bounding_box.width() + (kMouselaserUiStartRadius * 2) https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:156: PaintLaser(canvas, laser_points_, widget_->GetWindowBoundsInScreen()); Inline this method. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_view.h (right): https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.h:19: class LaserPointerModeTestApi; Please verify that this is needed https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.h:21: // as a point which replaces the mouse cursor, as well as a trail of lines to I'm a bit confused by "It draws the laser as a point" https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.h:24: friend class LaserPointerModeTestApi; Move friend class decl to right below private: https://codereview.chromium.org/2239743004/diff/60001/third_party/swiftshader... File third_party/swiftshader/LICENSE (left): https://codereview.chromium.org/2239743004/diff/60001/third_party/swiftshader... third_party/swiftshader/LICENSE:2: Copyright(c)2003-2011 TransGaming Inc Revert https://codereview.chromium.org/2239743004/diff/60001/third_party/swiftshader... File third_party/swiftshader/README.chromium (left): https://codereview.chromium.org/2239743004/diff/60001/third_party/swiftshader... third_party/swiftshader/README.chromium:3: URL: http://transgaming.com/business/swiftshader Revert https://codereview.chromium.org/2239743004/diff/60001/ui/gfx/vector_icons/pal... File ui/gfx/vector_icons/palette_mode_laser_pointer.1x.icon (right): https://codereview.chromium.org/2239743004/diff/60001/ui/gfx/vector_icons/pal... ui/gfx/vector_icons/palette_mode_laser_pointer.1x.icon:8: R_LINE_TO, 4.56f, -4.8f, Did you fix the multi-color issue? Please file a bug if not and add a TODO to the tool class.
https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tool.cc (right): https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tool.cc:15: tool_manager->AddTool(base::WrapUnique(new LaserPointerMode(tool_manager))); On 2016/08/16 19:33:55, jdufault wrote: > base::MakeUnique<LaserPointerMode>(tool_manager) Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc (right): https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:28: false)); On 2016/08/16 19:33:55, jdufault wrote: > Can we make the timer repeating (change the false on line 28 to true) and then > just use Stop/Reset to start/stop it? Then inside of AddStationaryPoint you > won't have to constantly reset the timer. Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.h (right): https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:15: class LaserPointerModeTestApi; On 2016/08/16 19:33:55, jdufault wrote: > Newline below this. Please also verify this is needed. Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:20: friend class LaserPointerModeTestApi; On 2016/08/16 19:33:55, jdufault wrote: > Move friend class decl to right below private: Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:52: base::ThreadChecker thread_checker_; On 2016/08/16 19:33:55, jdufault wrote: > I'd move the thread_checker_ decl somewhere else so it doesn't separate timer_ > and timer_repeat_count_ Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:53: mutable int timer_repeat_count_ = 0; On 2016/08/16 19:33:55, jdufault wrote: > Remove mutable since this should only be accessed on one thread Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_points.cc (right): https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:14: float DistanceBetweenMousePoints(const LaserPointerPoints::LaserPoint& point1, On 2016/08/16 19:33:56, jdufault wrote: > Make this take gfx::Point instances Removed. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:22: life_duration_ = life_duration; On 2016/08/16 19:33:56, jdufault wrote: > Initialize in ctor list, ie, : life_duration_(life_duration) Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:39: // GenerateCurvePoints(newPoint, generatedPoints); On 2016/08/16 19:33:56, jdufault wrote: > Please remove GenerateCurvePoints as well as this comment if it isn't being > used. Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:91: minPoint = gfx::Point(std::min(minPoint.x(), point.x()), On 2016/08/16 19:33:56, jdufault wrote: > min_point Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:93: maxPoint = gfx::Point(std::max(maxPoint.x(), point.x()), On 2016/08/16 19:33:56, jdufault wrote: > max_point Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:101: gfx::Point minPoint = bounds_.origin(); On 2016/08/16 19:33:56, jdufault wrote: > min_point Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:102: gfx::Point maxPoint = bounds_.bottom_right(); On 2016/08/16 19:33:56, jdufault wrote: > max_point Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:105: minPoint.set_x(std::min(minPoint.x(), it->location.x())); On 2016/08/16 19:33:56, jdufault wrote: > Should this be std::max? Removed. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:107: maxPoint.set_x(std::max(maxPoint.x(), it->location.x())); On 2016/08/16 19:33:55, jdufault wrote: > Should this be std::min? Removed. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:109: points_.erase(it); On 2016/08/16 19:33:56, jdufault wrote: > erase invalidates the it iterator, which means that the erase needs to happen > outside of the for loop. > > One solution is to use a helper vector and append the items to erase to it. > Another is to use std::remove_if and filter the points that way, and then > actually delete them using another <algorithm> function. Would this way work? I chose this way so we can still early exit and not use another buffer. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:116: bounds_ = gfx::BoundingRect(minPoint, maxPoint); On 2016/08/16 19:33:56, jdufault wrote: > How large is the points_ array? If it's not massive, I think it probably makes > sense to recompute the bounding box entirely whenever we add a new point so that > the code is simpler and easier to maintain. Around 40. Re-added Recalculate bounds. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_points.h (right): https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.h:29: // Constructor with a parameter to choose the fade out time off the points in On 2016/08/16 19:33:56, jdufault wrote: > off => of Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc (right): https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:20: ~LaserPointerModeTestApi() { delete instance_; } On 2016/08/16 19:33:57, jdufault wrote: > If LaserPointerModeTestApi takes ownership of LaserPointerMode it should be in > the form of a std::unique_ptr. The general rule is to avoid manual memory > management. Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:22: void enable() { instance_->OnEnable(); } On 2016/08/16 19:33:57, jdufault wrote: > OnEnable Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:23: void disable() { instance_->OnDisable(); } On 2016/08/16 19:33:57, jdufault wrote: > OnDisable Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:38: class LaserPointerModeTest : public test::AshTestBase, On 2016/08/16 19:33:57, jdufault wrote: > Move the LaserPointerModeTest class into anonymous namespace. > > LaserPointerModeTestApi and the actual test functions should live in the ash > namespace, though. Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:39: public PaletteToolManager::Delegate { On 2016/08/16 19:33:57, jdufault wrote: > Does LaserPointerModeTest need to extend PaletteToolManager::Delegate? Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:54: AshTestBase::TearDown(); On 2016/08/16 19:33:57, jdufault wrote: > Make TearDown to the first line of the function This will cause the test to crash. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:73: TEST_F(LaserPointerModeTest, LaserPointerPoints) { On 2016/08/16 19:33:56, jdufault wrote: > Please use a more descriptive test name - LaserPointerPoints doesn't describe > the test. Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:73: TEST_F(LaserPointerModeTest, LaserPointerPoints) { On 2016/08/16 19:33:57, jdufault wrote: > Add a comment describing what the test does. Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:99: TEST_F(LaserPointerModeTest, LaserPointerView) { On 2016/08/16 19:33:56, jdufault wrote: > Add a comment describing the test. Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:99: TEST_F(LaserPointerModeTest, LaserPointerView) { On 2016/08/16 19:33:57, jdufault wrote: > Please rename test to be more descriptive. Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_view.cc (right): https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:8: #include "ash/common/shell_window_ids.h" On 2016/08/16 19:33:58, jdufault wrote: > newline between <memory> and "ash/..." Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:23: const double kMouselaserUiStartRadius = 4; On 2016/08/16 19:33:57, jdufault wrote: > What about > > kPointInitialRadius = 4 > kPointFinalRadius = 0.25 Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:23: const double kMouselaserUiStartRadius = 4; On 2016/08/16 19:33:58, jdufault wrote: > Drop Mouse* prefix on all of these. Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:25: const int kMouselaserUiStartOpacityValue = 200; On 2016/08/16 19:33:58, jdufault wrote: > kPointInitialOpacity > kPointFinalOpacity Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:27: const int kMouselaserBlueValue = 0; On 2016/08/16 19:33:58, jdufault wrote: > kPointColor Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:29: const int kMouselaserRedValue = 255; On 2016/08/16 19:33:57, jdufault wrote: > Declare the RGB values together in an SkColor instance, ie, > > https://cs.chromium.org/chromium/src/ash/common/system/tray/tray_constants.cc... Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:52: gfx::Vector2d vector = point1 - point2; On 2016/08/16 19:33:57, jdufault wrote: > return (point1 - point2).Length() Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:75: double normalized; On 2016/08/16 19:33:58, jdufault wrote: > Add a comment along the lines of > > // Normalized is a value between [0,1] where 0 means the point is about to be > removed and 1 means that the point was just added. Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:75: double normalized; On 2016/08/16 19:33:57, jdufault wrote: > double normalized = 1.0; > if (oldest != most_recent) { > // ... > } Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:75: double normalized; On 2016/08/16 19:33:57, jdufault wrote: > double normalized = 1.0; > if (oldest != most_recent) { > // ... > } Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:88: kMouselaserUiStartRadius - On 2016/08/16 19:33:57, jdufault wrote: > Add a lerp function and replace all of the instances of this with it. > > double radius = LinearIntepolate(initial, final, progress) Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:95: paintColor = SkColorSetARGB(opacity, kMouselaserRedValue, On 2016/08/16 19:33:58, jdufault wrote: > paint_color Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:102: float distance_threshold = float{radius * 2}; On 2016/08/16 19:33:57, jdufault wrote: > Can we use a double to avoid the float cast? I think since the points are floats and the time delta is in double a cast will be required eventually. I think here would minimize the amount of casts. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:120: } // namespace On 2016/08/16 19:33:58, jdufault wrote: > newline above Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:145: new_bounding_box.SetRect( On 2016/08/16 19:33:58, jdufault wrote: > Can you use only one bounding_box variable, all of the parameters will be > evaluated before the function call so it should be safe. Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:148: old_bounding_box.width() + 2 * kMouselaserUiStartRadius, On 2016/08/16 19:33:57, jdufault wrote: > old_bounding_box.width() + (kMouselaserUiStartRadius * 2) Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:156: PaintLaser(canvas, laser_points_, widget_->GetWindowBoundsInScreen()); On 2016/08/16 19:33:58, jdufault wrote: > Inline this method. Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_view.h (right): https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.h:19: class LaserPointerModeTestApi; On 2016/08/16 19:33:58, jdufault wrote: > Please verify that this is needed Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.h:21: // as a point which replaces the mouse cursor, as well as a trail of lines to On 2016/08/16 19:33:58, jdufault wrote: > I'm a bit confused by "It draws the laser as a point" Done. https://codereview.chromium.org/2239743004/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.h:24: friend class LaserPointerModeTestApi; On 2016/08/16 19:33:58, jdufault wrote: > Move friend class decl to right below private: Done. https://codereview.chromium.org/2239743004/diff/60001/third_party/swiftshader... File third_party/swiftshader/LICENSE (left): https://codereview.chromium.org/2239743004/diff/60001/third_party/swiftshader... third_party/swiftshader/LICENSE:2: Copyright(c)2003-2011 TransGaming Inc On 2016/08/16 19:33:55, jdufault wrote: > Revert Done. https://codereview.chromium.org/2239743004/diff/60001/third_party/swiftshader... File third_party/swiftshader/README.chromium (left): https://codereview.chromium.org/2239743004/diff/60001/third_party/swiftshader... third_party/swiftshader/README.chromium:3: URL: http://transgaming.com/business/swiftshader On 2016/08/16 19:33:55, jdufault wrote: > Revert Done. https://codereview.chromium.org/2239743004/diff/60001/ui/gfx/vector_icons/pal... File ui/gfx/vector_icons/palette_mode_laser_pointer.1x.icon (right): https://codereview.chromium.org/2239743004/diff/60001/ui/gfx/vector_icons/pal... ui/gfx/vector_icons/palette_mode_laser_pointer.1x.icon:8: R_LINE_TO, 4.56f, -4.8f, On 2016/08/16 19:33:58, jdufault wrote: > Did you fix the multi-color issue? Please file a bug if not and add a TODO to > the tool class. I checked the skiafy output gives this. There doesn't seem to be any color information here, who should this bug and TODO be assigned to.
https://codereview.chromium.org/2239743004/diff/60001/ui/gfx/vector_icons/pal... File ui/gfx/vector_icons/palette_mode_laser_pointer.1x.icon (right): https://codereview.chromium.org/2239743004/diff/60001/ui/gfx/vector_icons/pal... ui/gfx/vector_icons/palette_mode_laser_pointer.1x.icon:8: R_LINE_TO, 4.56f, -4.8f, On 2016/08/16 23:18:56, sammiequon wrote: > On 2016/08/16 19:33:58, jdufault wrote: > > Did you fix the multi-color issue? Please file a bug if not and add a TODO to > > the tool class. > > I checked the skiafy output gives this. There doesn't seem to be any color > information here, who should this bug and TODO be assigned to. Right, we're probably going to need multiple icons. For the time being you can assign the bug to me, I'll redirect/follow-up as needed. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc (right): https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:56: if (WmShell::Get()->palette_delegate()) https://codereview.chromium.org/2235063002/ adds a TestPaletteDelegate class, I'd use that in the test class so we can eliminate these if checks. I'd add a TODO since it will be challenging to have this PS depend on that PS. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:83: int count = timer_repeat_count_; Let's always add to timer_repeat_count_ so the if conditional can be simplified. This should also eliminate the count variable. timer_repeat_count_ += 1; if (timer_repeat_count_ * foo >= foo) StopTimer(); https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.h (right): https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:9: #include "ash/common/system/chromeos/palette/tools/laser_pointer_view.h" Forward declare this include https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:52: base::ThreadChecker thread_checker_; So it looks like the timer always calls the function on the right thread? Since this is a private API it seems safe to remove the thread_checker_ logic. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_points.cc (right): https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:7: #include <limits.h> <limits> https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:17: // Creates and adds the new point. Updates the collection and bounding box Drop comment, it is already on the declaration. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:19: base::Time pointTime = base::Time::Now(); point_time https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:19: base::Time pointTime = base::Time::Now(); inline this variable since it is only used once. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:20: LaserPoint newPoint; new_point https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:42: if (!points_.empty()) Invert all of these conditions since the pattern is usually if (failure) handle ie, if (points_.empty()) return LaserPoint(); But thinking about it more, instead of having GetOldest return a default value, I think it's better to just DCHECK on IsEmpty since the default point doesn't have much meaning. GetOldest() { DCHECK(!IsEmpty()); return points_.front(); } https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:47: LaserPointerPoints::LaserPoint LaserPointerPoints::GetMostRecent() const { See GetOldest comment. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:97: points_.erase(points_.begin(), it); Nice, this works well. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:105: int j = 0; Because the point length is only 40 or so, I don't think having this fancy recalculate bounds logic is worth the effort to understand it. If we ever need a new bounds just recompute it entirely. I'd actually remove the cached bounds_ variable for the time being and if it's obviously slow, we can add it back. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_points.h (right): https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.h:10: #include <vector> Do you need the vector include? https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.h:65: // The collection of points. I'd remove this comment. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc (right): https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:20: instance_ = std::move(instance); Use constructor list whenever possible. : instance_(std::move(instance)) {} https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:37: DISALLOW_COPY_AND_ASSIGN(LaserPointerModeTestApi); newline above this https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:40: namespace { Merge the different anonymous namespaces. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:45: base::TimeDelta::FromSecondsD(kTestPointsLifetime)) {} Make kTestPointsLifetime an integral type and use FromSeconds(...) https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:51: base::WrapUnique<LaserPointerMode>(new LaserPointerMode(nullptr)))); base::MakeUnique https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:69: // Test to test the laser pointers internal collection is handling receiving Comment goes above TEST_F function // Test description TEST_F(Foo, Bar) { } https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:69: // Test to test the laser pointers internal collection is handling receiving Replace "Test to test" with "Tests that the ...". Also eliminate "is handling" and just say "handles" https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:82: EXPECT_EQ(4, laser_pointer_points_.GetNumberOfPoints()); If we're testing the functionality specifically for LaserPointerPoints, we should probably pull it out into a new test and just construct the LaserPointerPoints instance directly. Making sure the points get forwarded properly is a good test as well. I think that is covered by the other test though where you use the event generator. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:95: You should add a test that verifies points will disappear over time. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_view.cc (right): https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:120: // Normalize the distance. Remove this comment https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:123: relative_time = 1.0 - relative_time; Inline this with the computation above. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:135: paint_color = SkColorSetA(kPointColor, opacity); merge the opacity computation block and this block together so they are not split up by the center and current_point computation https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:149: canvas->DrawLine(last_point, current_point, paint); Do we need this special case? https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:152: paint_color = SkColorSetA(kPointColor, int{kPointInitialOpacity}); Store kPointInitialOpacity as an int to remove the cast.
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc (right): https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:56: if (WmShell::Get()->palette_delegate()) On 2016/08/17 21:41:07, jdufault wrote: > https://codereview.chromium.org/2235063002/ adds a TestPaletteDelegate class, > I'd use that in the test class so we can eliminate these if checks. > > I'd add a TODO since it will be challenging to have this PS depend on that PS. Done. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:83: int count = timer_repeat_count_; On 2016/08/17 21:41:07, jdufault wrote: > Let's always add to timer_repeat_count_ so the if conditional can be simplified. > This should also eliminate the count variable. > > timer_repeat_count_ += 1; > if (timer_repeat_count_ * foo >= foo) > StopTimer(); Done. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.h (right): https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:9: #include "ash/common/system/chromeos/palette/tools/laser_pointer_view.h" On 2016/08/17 21:41:07, jdufault wrote: > Forward declare this include Done. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:52: base::ThreadChecker thread_checker_; On 2016/08/17 21:41:07, jdufault wrote: > So it looks like the timer always calls the function on the right thread? Since > this is a private API it seems safe to remove the thread_checker_ logic. Done. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_points.cc (right): https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:7: #include <limits.h> On 2016/08/17 21:41:07, jdufault wrote: > <limits> Done. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:17: // Creates and adds the new point. Updates the collection and bounding box On 2016/08/17 21:41:08, jdufault wrote: > Drop comment, it is already on the declaration. Done. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:19: base::Time pointTime = base::Time::Now(); On 2016/08/17 21:41:08, jdufault wrote: > point_time Done. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:19: base::Time pointTime = base::Time::Now(); On 2016/08/17 21:41:08, jdufault wrote: > inline this variable since it is only used once. Done. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:20: LaserPoint newPoint; On 2016/08/17 21:41:07, jdufault wrote: > new_point Done. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:42: if (!points_.empty()) On 2016/08/17 21:41:08, jdufault wrote: > Invert all of these conditions since the pattern is usually > > if (failure) > handle > > ie, > > if (points_.empty()) > return LaserPoint(); > > But thinking about it more, instead of having GetOldest return a default value, > I think it's better to just DCHECK on IsEmpty since the default point doesn't > have much meaning. > > GetOldest() { > DCHECK(!IsEmpty()); > return points_.front(); > } Done. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:47: LaserPointerPoints::LaserPoint LaserPointerPoints::GetMostRecent() const { On 2016/08/17 21:41:08, jdufault wrote: > See GetOldest comment. Done. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:97: points_.erase(points_.begin(), it); On 2016/08/17 21:41:08, jdufault wrote: > Nice, this works well. :). https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:105: int j = 0; On 2016/08/17 21:41:08, jdufault wrote: > Because the point length is only 40 or so, I don't think having this fancy > recalculate bounds logic is worth the effort to understand it. > > If we ever need a new bounds just recompute it entirely. I'd actually remove the > cached bounds_ variable for the time being and if it's obviously slow, we can > add it back. Removed. Doesn't seem to cause too much lag. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_points.h (right): https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.h:10: #include <vector> On 2016/08/17 21:41:08, jdufault wrote: > Do you need the vector include? Done. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_points.h:65: // The collection of points. On 2016/08/17 21:41:08, jdufault wrote: > I'd remove this comment. Done. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc (right): https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:20: instance_ = std::move(instance); On 2016/08/17 21:41:08, jdufault wrote: > Use constructor list whenever possible. > > : instance_(std::move(instance)) {} Done. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:37: DISALLOW_COPY_AND_ASSIGN(LaserPointerModeTestApi); On 2016/08/17 21:41:08, jdufault wrote: > newline above this Done. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:40: namespace { On 2016/08/17 21:41:08, jdufault wrote: > Merge the different anonymous namespaces. Done. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:45: base::TimeDelta::FromSecondsD(kTestPointsLifetime)) {} On 2016/08/17 21:41:08, jdufault wrote: > Make kTestPointsLifetime an integral type and use FromSeconds(...) Done. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:51: base::WrapUnique<LaserPointerMode>(new LaserPointerMode(nullptr)))); On 2016/08/17 21:41:08, jdufault wrote: > base::MakeUnique Done. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:69: // Test to test the laser pointers internal collection is handling receiving On 2016/08/17 21:41:08, jdufault wrote: > Replace "Test to test" with "Tests that the ...". Also eliminate "is handling" > and just say "handles" Done. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:69: // Test to test the laser pointers internal collection is handling receiving On 2016/08/17 21:41:08, jdufault wrote: > Comment goes above TEST_F function > > // Test description > TEST_F(Foo, Bar) { > } Done. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:82: EXPECT_EQ(4, laser_pointer_points_.GetNumberOfPoints()); On 2016/08/17 21:41:08, jdufault wrote: > If we're testing the functionality specifically for LaserPointerPoints, we > should probably pull it out into a new test and just construct the > LaserPointerPoints instance directly. > > Making sure the points get forwarded properly is a good test as well. I think > that is covered by the other test though where you use the event generator. Like write a LaserPointerPoints test in another files? Can you still add the TEST_F(LaserPointerModeTest, foo) so they get run in the same batch? https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:95: On 2016/08/17 21:41:08, jdufault wrote: > You should add a test that verifies points will disappear over time. Done. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_view.cc (right): https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:120: // Normalize the distance. On 2016/08/17 21:41:09, jdufault wrote: > Remove this comment Done. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:123: relative_time = 1.0 - relative_time; On 2016/08/17 21:41:09, jdufault wrote: > Inline this with the computation above. Done. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:135: paint_color = SkColorSetA(kPointColor, opacity); On 2016/08/17 21:41:09, jdufault wrote: > merge the opacity computation block and this block together so they are not > split up by the center and current_point computation Done. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:149: canvas->DrawLine(last_point, current_point, paint); On 2016/08/17 21:41:09, jdufault wrote: > Do we need this special case? Yeah otherwise there will be a tiny but still visible gap between the last two points. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:152: paint_color = SkColorSetA(kPointColor, int{kPointInitialOpacity}); On 2016/08/17 21:41:08, jdufault wrote: > Store kPointInitialOpacity as an int to remove the cast. I'd have to cast twice during the opacity lerp though. Is that preferred?
https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc (right): https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:82: EXPECT_EQ(4, laser_pointer_points_.GetNumberOfPoints()); On 2016/08/18 00:52:08, sammiequon wrote: > On 2016/08/17 21:41:08, jdufault wrote: > > If we're testing the functionality specifically for LaserPointerPoints, we > > should probably pull it out into a new test and just construct the > > LaserPointerPoints instance directly. > > > > Making sure the points get forwarded properly is a good test as well. I think > > that is covered by the other test though where you use the event generator. > > Like write a LaserPointerPoints test in another files? Can you still add the > TEST_F(LaserPointerModeTest, foo) so they get run in the same batch? You have the LaserPointerRenderer test right now, I think it could be something like that in this same file. https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/palette/tools/laser_pointer_view.cc (right): https://codereview.chromium.org/2239743004/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:149: canvas->DrawLine(last_point, current_point, paint); On 2016/08/18 00:52:08, sammiequon wrote: > On 2016/08/17 21:41:09, jdufault wrote: > > Do we need this special case? > > Yeah otherwise there will be a tiny but still visible gap between the last two > points. Does the for loop iterate one more time after this is called? Is last_point used again? https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... File ash/common/system/chromeos/palette/palette_tool.cc (right): https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/palette_tool.cc:31: // TODO(jdufault): Icons do not seem to support multiple colors. Remove TODO once you upload the icon fix. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc (right): https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:18: const int kAddStationaryPointsDelayMs = 5; Should this be 10? https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:87: kPointLifeDurationMs) Wrap with {} since if condition is >1 line https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.h (right): https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:9: #include "ash/common/system/chromeos/palette/tools/laser_pointer_view.h" Forward declare LaserPointerView https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_points.cc (right): https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:29: gfx::Point min_point(INT_MAX, INT_MAX); std::numeric_limits<int64_t>::max() (or whatever the right generic type is) https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:31: for (auto it = points_.begin(); it != points_.end(); it++) { for (const LaserPoint& point : points_) https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:68: void LaserPointerPoints::ClearOldPoints() { This only gets called after a point has been added so instead of the if (!points_.empty()) add a DCHECK(!points_.empty()) https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:71: auto it = points_.begin(); What about auto it = std::find_if(points_.begin(), points_.end(), [](p) { return newest.creation_time - p.creation_time < life_duration; }); points_.erase(points_.begin(), it); If you don't like that approach, then please cleanup the for loop so that there is no empty if block - or maybe convert it to a while (condition) { ++it } block? https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc (right): https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:20: void AddTimedPoint(const base::Time& time, const gfx::Point& location) { AddPointAtTime(...) https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:55: namespace { Move anonymous namespace contents to the top of the file. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:80: LaserPointerPoints laser_pointer_points_; What about just points_? https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:81: LaserPointerPointsTestApi laser_pointer_points_delete_; points_test_api_? https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:81: LaserPointerPointsTestApi laser_pointer_points_delete_; If this test api instance is separate from laser_pointer_mode_ you should have separate fixtures, one for LaserPointerMode tests and another for LaserPointerPoints tests. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:104: EXPECT_EQ(gfx::Rect(1, 0, 29, 9), laser_pointer_points_.GetBoundingBox()); I think having renaming some of the points to top_left and bottom_right (or something similar), and then adding some unnamed points in between them (with a comment saying that the added points are between the top left and bottom right) will make this a bit more readable. You can then also check the bbox using named constants, ie, EXPECT_EQ(gfx::Rect(top_left.x, top_left.y, bottom_right.x - top_left.x, bottom_right.y - top_left.y), ...); I think that's much easier to read and understand instead of just the magic numbers. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:108: laser_pointer_points_.AddPoint(point5); Declare point here, ie, gfx::Point expanded_bottom_left (or the right name) = ... And do the EXPECT_EQ check using that named constant. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:120: base::Time three_second_time = base::Time::FromJsTime(3000); Use FromSeconds https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:122: base::Time six_half_second_time = base::Time::FromJsTime(6500); I don't think these constants aid in readability. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:128: // // ago should be removed. What about something like this for the comment? When a point older than kTestPointsLifetime is added, it should get removed. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:142: EXPECT_EQ(laser_pointer_points_delete_.GetNumberOfPoints(), 1); This seems strange to me. Why does adding a 20 second old point clear all of the other points? Is the test adding points to the front of the queue so it's actually in reverse order? I think a better approach here is instead of adding a point at a specific time, have the test API iterate over all of the existing points and move their time backwards. This mirrors the real environment a bit more closely. So the test would look like this AddPoint() EXPECT_EQ(1, PointCount()) MoveForwardInTime(1s) EXPECT_EQ(1, PointCount()) MoveForwardInTime(20s) EXPECT_EQ(0, PointCount()) I think this easier to read, too. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:168: EXPECT_EQ(gfx::Point(19, 71), GetEventGenerator().current_location() https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_view.cc (right): https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:27: const double kPointFinalOpacity = 0.0; Conceptually I think these are integral values, so having two casts later is okay. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:54: double LinearInterpolate(double initialValue, initial, final, progress https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:70: : views::View(), laser_points_(life_duration) { The default ctor call shouldn't be needed. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:101: if (!laser_points_.IsEmpty()) { Can we move the if (laser_points_.IsEmpty()) call to the start of the function and do an early return? https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:106: SkColor paint_color; I would inline paint_color computations and remove the variable https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:109: base::Time most_recent = laser_points_.GetMostRecent().creation_time; newest? https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_view.h (right): https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.h:12: #include "ui/events/event.h" needed? https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.h:13: #include "ui/gfx/geometry/point.h" forward declare https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.h:15: #include "ui/views/widget/widget.h" forward declare
https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... File ash/common/system/chromeos/palette/palette_tool.cc (right): https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/palette_tool.cc:31: // TODO(jdufault): Icons do not seem to support multiple colors. On 2016/08/19 01:02:08, jdufault wrote: > Remove TODO once you upload the icon fix. Ok. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc (right): https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:18: const int kAddStationaryPointsDelayMs = 5; On 2016/08/19 01:02:08, jdufault wrote: > Should this be 10? I played around with the constants I think this looks slightly better imo. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:87: kPointLifeDurationMs) On 2016/08/19 01:02:08, jdufault wrote: > Wrap with {} since if condition is >1 line Done. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.h (right): https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:9: #include "ash/common/system/chromeos/palette/tools/laser_pointer_view.h" On 2016/08/19 01:02:08, jdufault wrote: > Forward declare LaserPointerView Done. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_points.cc (right): https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:29: gfx::Point min_point(INT_MAX, INT_MAX); On 2016/08/19 01:02:08, jdufault wrote: > std::numeric_limits<int64_t>::max() (or whatever the right generic type is) Done. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:31: for (auto it = points_.begin(); it != points_.end(); it++) { On 2016/08/19 01:02:08, jdufault wrote: > for (const LaserPoint& point : points_) Done. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:68: void LaserPointerPoints::ClearOldPoints() { On 2016/08/19 01:02:08, jdufault wrote: > This only gets called after a point has been added so instead of the if > (!points_.empty()) add a DCHECK(!points_.empty()) Done. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:71: auto it = points_.begin(); On 2016/08/19 01:02:08, jdufault wrote: > What about > > auto it = std::find_if(points_.begin(), points_.end(), > [](p) { > return newest.creation_time - p.creation_time < > life_duration; > }); > points_.erase(points_.begin(), it); > > If you don't like that approach, then please cleanup the for loop so that there > is no empty if block - or maybe convert it to a while (condition) { ++it } > block? Done. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc (right): https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:20: void AddTimedPoint(const base::Time& time, const gfx::Point& location) { On 2016/08/19 01:02:09, jdufault wrote: > AddPointAtTime(...) Done. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:55: namespace { On 2016/08/19 01:02:09, jdufault wrote: > Move anonymous namespace contents to the top of the file. Done. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:80: LaserPointerPoints laser_pointer_points_; On 2016/08/19 01:02:09, jdufault wrote: > What about just points_? Done. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:81: LaserPointerPointsTestApi laser_pointer_points_delete_; On 2016/08/19 01:02:08, jdufault wrote: > points_test_api_? Done. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:81: LaserPointerPointsTestApi laser_pointer_points_delete_; On 2016/08/19 01:02:09, jdufault wrote: > If this test api instance is separate from laser_pointer_mode_ you should have > separate fixtures, one for LaserPointerMode tests and another for > LaserPointerPoints tests. Done. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:104: EXPECT_EQ(gfx::Rect(1, 0, 29, 9), laser_pointer_points_.GetBoundingBox()); On 2016/08/19 01:02:09, jdufault wrote: > I think having renaming some of the points to top_left and bottom_right (or > something similar), and then adding some unnamed points in between them (with a > comment saying that the added points are between the top left and bottom right) > will make this a bit more readable. You can then also check the bbox using named > constants, ie, > > EXPECT_EQ(gfx::Rect(top_left.x, top_left.y, bottom_right.x - top_left.x, > bottom_right.y - top_left.y), ...); > > I think that's much easier to read and understand instead of just the magic > numbers. Done. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:108: laser_pointer_points_.AddPoint(point5); On 2016/08/19 01:02:09, jdufault wrote: > Declare point here, ie, > > gfx::Point expanded_bottom_left (or the right name) = ... > > And do the EXPECT_EQ check using that named constant. Done. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:120: base::Time three_second_time = base::Time::FromJsTime(3000); On 2016/08/19 01:02:08, jdufault wrote: > Use FromSeconds As per offline conv this is fine. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:122: base::Time six_half_second_time = base::Time::FromJsTime(6500); On 2016/08/19 01:02:09, jdufault wrote: > I don't think these constants aid in readability. Done. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:128: // // ago should be removed. On 2016/08/19 01:02:08, jdufault wrote: > What about something like this for the comment? > > When a point older than kTestPointsLifetime is added, it should > get removed. Done. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:142: EXPECT_EQ(laser_pointer_points_delete_.GetNumberOfPoints(), 1); On 2016/08/19 01:02:08, jdufault wrote: > This seems strange to me. Why does adding a 20 second old point clear all of the > other points? Is the test adding points to the front of the queue so it's > actually in reverse order? > > I think a better approach here is instead of adding a point at a specific time, > have the test API iterate over all of the existing points and move their time > backwards. This mirrors the real environment a bit more closely. > > So the test would look like this > > AddPoint() > EXPECT_EQ(1, PointCount()) > MoveForwardInTime(1s) > EXPECT_EQ(1, PointCount()) > MoveForwardInTime(20s) > EXPECT_EQ(0, PointCount()) > > I think this easier to read, too. Done. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:168: EXPECT_EQ(gfx::Point(19, 71), On 2016/08/19 01:02:09, jdufault wrote: > GetEventGenerator().current_location() Done. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_view.cc (right): https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:27: const double kPointFinalOpacity = 0.0; On 2016/08/19 01:02:09, jdufault wrote: > Conceptually I think these are integral values, so having two casts later is > okay. Done. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:54: double LinearInterpolate(double initialValue, On 2016/08/19 01:02:09, jdufault wrote: > initial, final, progress Final is a keyword and i thought it will be weird if only one of them had _value. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:70: : views::View(), laser_points_(life_duration) { On 2016/08/19 01:02:09, jdufault wrote: > The default ctor call shouldn't be needed. Done. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:101: if (!laser_points_.IsEmpty()) { On 2016/08/19 01:02:09, jdufault wrote: > Can we move the if (laser_points_.IsEmpty()) call to the start of the function > and do an early return? Done. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:106: SkColor paint_color; On 2016/08/19 01:02:09, jdufault wrote: > I would inline paint_color computations and remove the variable Done. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:109: base::Time most_recent = laser_points_.GetMostRecent().creation_time; On 2016/08/19 01:02:09, jdufault wrote: > newest? Done. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_view.h (right): https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.h:12: #include "ui/events/event.h" On 2016/08/19 01:02:09, jdufault wrote: > needed? Done. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.h:13: #include "ui/gfx/geometry/point.h" On 2016/08/19 01:02:09, jdufault wrote: > forward declare Done. https://codereview.chromium.org/2239743004/diff/120001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.h:15: #include "ui/views/widget/widget.h" On 2016/08/19 01:02:09, jdufault wrote: > forward declare Done.
https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc (right): https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:50: // once test deleagte CL lands. nit: delegate https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:102: // Add the new point and notify the queue. I'd remove this comment, but it is up to you. I think "notify the queue" is a bit confusing, and "Add the new point" is evident because the next function call is "AddNewPoint" https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.h (right): https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:49: // This will remove points past a certain threshold. Instead of "threshold", what about "too old"? https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_points.cc (right): https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:75: auto it = std::find_if( What about 'first_alive_point' instead of 'it'? https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc (right): https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. No (c) https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:17: const double kTestTimeMs = 100000.0; Add comment for how long this is in a more readable unit, ie, const double kTestTimeMs = 100000.0; // 100 seconds https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:33: points_.reset(); reset() should be called for you by the dtor https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:35: AshTestBase::TearDown(); TearDown should be the first fn call. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:62: points_.reset(); I don't think you need to reset these instances; the entire class should get destroyed so the dtor will do it for you. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:65: AshTestBase::TearDown(); AshTestBase::TearDown should be the first function call https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:69: std::unique_ptr<LaserPointerModeTestApi> laser_pointer_mode_; mode_test_api_? https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:91: void LaserPointerPointsTestApi::MoveForwardInTime(int seconds) { TimeDelta delta https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:91: void LaserPointerPointsTestApi::MoveForwardInTime(int seconds) { What about eliminating AddPointAtTime and making this function assume its functionality, so have it also take a gfx::Point location? So the test API function will just move the existing points back in time by some given amount (|seconds|), and then will call the normal add point API that will add a new point at the given location (with the time set to base::Now()). https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:97: LaserPointerModeTestApi::LaserPointerModeTestApi( Where is the declaration for this class? If it is a separate h file, then this should also be in a separate cc file. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:126: points_->AddPoint(point2); Inline point2 since we don't need to name it. Add a comment saying it does not expand the bounding box. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:151: // Test the laser pointer points collection removes old points as expected. What about a comment like: // Verify that old points are removed. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_view.cc (right): https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:71: // Clear the points so that there is nothing to be rendered. I'd remove this comment. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:78: // Set the bounding box of the points to be able to enclose a point of max What about this? // Expand the bounding box so it includes the radius of the // points on the edges. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:92: return; newline after return https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:104: int num_laser_points_ = laser_points_.GetNumberOfPoints(); num_points
https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc (right): https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:50: // once test deleagte CL lands. On 2016/08/19 21:05:56, jdufault wrote: > nit: delegate Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:102: // Add the new point and notify the queue. On 2016/08/19 21:05:56, jdufault wrote: > I'd remove this comment, but it is up to you. I think "notify the queue" is a > bit confusing, and "Add the new point" is evident because the next function call > is "AddNewPoint" Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.h (right): https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:49: // This will remove points past a certain threshold. On 2016/08/19 21:05:56, jdufault wrote: > Instead of "threshold", what about "too old"? Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_points.cc (right): https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:75: auto it = std::find_if( On 2016/08/19 21:05:56, jdufault wrote: > What about 'first_alive_point' instead of 'it'? Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc (right): https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/08/19 21:05:56, jdufault wrote: > No (c) Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:17: const double kTestTimeMs = 100000.0; On 2016/08/19 21:05:56, jdufault wrote: > Add comment for how long this is in a more readable unit, ie, > > const double kTestTimeMs = 100000.0; // 100 seconds Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:33: points_.reset(); On 2016/08/19 21:05:56, jdufault wrote: > reset() should be called for you by the dtor Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:35: AshTestBase::TearDown(); On 2016/08/19 21:05:56, jdufault wrote: > TearDown should be the first fn call. Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:62: points_.reset(); On 2016/08/19 21:05:56, jdufault wrote: > I don't think you need to reset these instances; the entire class should get > destroyed so the dtor will do it for you. Laser pointer mode dtor removes the pointer watcher and this has to happen before TearDown otherwise tear down will complain there is still an pointer watcher observer when it expects none. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:65: AshTestBase::TearDown(); On 2016/08/19 21:05:56, jdufault wrote: > AshTestBase::TearDown should be the first function call See above. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:69: std::unique_ptr<LaserPointerModeTestApi> laser_pointer_mode_; On 2016/08/19 21:05:56, jdufault wrote: > mode_test_api_? Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:91: void LaserPointerPointsTestApi::MoveForwardInTime(int seconds) { On 2016/08/19 21:05:56, jdufault wrote: > What about eliminating AddPointAtTime and making this function assume its > functionality, so have it also take a gfx::Point location? > > So the test API function will just move the existing points back in time by some > given amount (|seconds|), and then will call the normal add point API that will > add a new point at the given location (with the time set to base::Now()). Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:91: void LaserPointerPointsTestApi::MoveForwardInTime(int seconds) { On 2016/08/19 21:05:56, jdufault wrote: > TimeDelta delta Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:97: LaserPointerModeTestApi::LaserPointerModeTestApi( On 2016/08/19 21:05:56, jdufault wrote: > Where is the declaration for this class? > > If it is a separate h file, then this should also be in a separate cc file. Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:126: points_->AddPoint(point2); On 2016/08/19 21:05:56, jdufault wrote: > Inline point2 since we don't need to name it. Add a comment saying it does not > expand the bounding box. Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:151: // Test the laser pointer points collection removes old points as expected. On 2016/08/19 21:05:56, jdufault wrote: > What about a comment like: > > // Verify that old points are removed. Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_view.cc (right): https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:71: // Clear the points so that there is nothing to be rendered. On 2016/08/19 21:05:57, jdufault wrote: > I'd remove this comment. Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:78: // Set the bounding box of the points to be able to enclose a point of max On 2016/08/19 21:05:57, jdufault wrote: > What about this? > > // Expand the bounding box so it includes the radius of the > // points on the edges. Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:92: return; On 2016/08/19 21:05:57, jdufault wrote: > newline after return Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:104: int num_laser_points_ = laser_points_.GetNumberOfPoints(); On 2016/08/19 21:05:57, jdufault wrote: > num_points Done.
https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc (right): https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:50: // once test deleagte CL lands. On 2016/08/19 21:05:56, jdufault wrote: > nit: delegate Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:102: // Add the new point and notify the queue. On 2016/08/19 21:05:56, jdufault wrote: > I'd remove this comment, but it is up to you. I think "notify the queue" is a > bit confusing, and "Add the new point" is evident because the next function call > is "AddNewPoint" Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.h (right): https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:49: // This will remove points past a certain threshold. On 2016/08/19 21:05:56, jdufault wrote: > Instead of "threshold", what about "too old"? Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_points.cc (right): https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:75: auto it = std::find_if( On 2016/08/19 21:05:56, jdufault wrote: > What about 'first_alive_point' instead of 'it'? Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc (right): https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/08/19 21:05:56, jdufault wrote: > No (c) Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:17: const double kTestTimeMs = 100000.0; On 2016/08/19 21:05:56, jdufault wrote: > Add comment for how long this is in a more readable unit, ie, > > const double kTestTimeMs = 100000.0; // 100 seconds Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:33: points_.reset(); On 2016/08/19 21:05:56, jdufault wrote: > reset() should be called for you by the dtor Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:35: AshTestBase::TearDown(); On 2016/08/19 21:05:56, jdufault wrote: > TearDown should be the first fn call. Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:62: points_.reset(); On 2016/08/19 21:05:56, jdufault wrote: > I don't think you need to reset these instances; the entire class should get > destroyed so the dtor will do it for you. Laser pointer mode dtor removes the pointer watcher and this has to happen before TearDown otherwise tear down will complain there is still an pointer watcher observer when it expects none. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:65: AshTestBase::TearDown(); On 2016/08/19 21:05:56, jdufault wrote: > AshTestBase::TearDown should be the first function call See above. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:69: std::unique_ptr<LaserPointerModeTestApi> laser_pointer_mode_; On 2016/08/19 21:05:56, jdufault wrote: > mode_test_api_? Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:91: void LaserPointerPointsTestApi::MoveForwardInTime(int seconds) { On 2016/08/19 21:05:56, jdufault wrote: > What about eliminating AddPointAtTime and making this function assume its > functionality, so have it also take a gfx::Point location? > > So the test API function will just move the existing points back in time by some > given amount (|seconds|), and then will call the normal add point API that will > add a new point at the given location (with the time set to base::Now()). Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:91: void LaserPointerPointsTestApi::MoveForwardInTime(int seconds) { On 2016/08/19 21:05:56, jdufault wrote: > TimeDelta delta Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:97: LaserPointerModeTestApi::LaserPointerModeTestApi( On 2016/08/19 21:05:56, jdufault wrote: > Where is the declaration for this class? > > If it is a separate h file, then this should also be in a separate cc file. Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:126: points_->AddPoint(point2); On 2016/08/19 21:05:56, jdufault wrote: > Inline point2 since we don't need to name it. Add a comment saying it does not > expand the bounding box. Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:151: // Test the laser pointer points collection removes old points as expected. On 2016/08/19 21:05:56, jdufault wrote: > What about a comment like: > > // Verify that old points are removed. Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_view.cc (right): https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:71: // Clear the points so that there is nothing to be rendered. On 2016/08/19 21:05:57, jdufault wrote: > I'd remove this comment. Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:78: // Set the bounding box of the points to be able to enclose a point of max On 2016/08/19 21:05:57, jdufault wrote: > What about this? > > // Expand the bounding box so it includes the radius of the > // points on the edges. Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:92: return; On 2016/08/19 21:05:57, jdufault wrote: > newline after return Done. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:104: int num_laser_points_ = laser_points_.GetNumberOfPoints(); On 2016/08/19 21:05:57, jdufault wrote: > num_points Done.
Are the icons in this patch multi-colored? https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc (right): https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:62: points_.reset(); On 2016/08/22 18:07:20, sammiequon wrote: > On 2016/08/19 21:05:56, jdufault wrote: > > I don't think you need to reset these instances; the entire class should get > > destroyed so the dtor will do it for you. > > Laser pointer mode dtor removes the pointer watcher and this has to happen > before TearDown otherwise tear down will complain there is still an pointer > watcher observer when it expects none. Please add a comment describing this requirement. https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_points.cc (right): https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:76: points_.begin(), points_.end(), [&newest, &life_duration](LaserPoint& p) { Can you capture [this] and use points_.back() and life_duration_ directly? https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc (right): https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:13: class LaserPointerPointsTestApi : public LaserPointerPoints { Can you make this take the LaserPointerPoints instance as parameter, similar to how LaserPointerModeTestApi works? Also, instead of using friend class for inheritance prefer to use protected. It communicates intent more clearly. https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:24: LaserPointerPoints::LaserPoint new_point; Instead of building a new_point instance just call AddPoint(location). This means you can also remove the |time| parameter. This will also take care of calling ClearOldPoints(). https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:60: namespace { The anonymous namespace should be at the top. Pull the *TestApi classes into separate files if you need to. https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:62: const int kTestPointsLifetime = 5; Can you specify the unit here? https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:120: points_->AddPoint(top_right); I think you should also verify the bounding box when the points_ object has 0 and 1 points. https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:122: // Test laser pointer points collection and helper functions. Replace this comment with a series of comments that give a very high-level overview of what each section of code does. I've written an example one below (line 131) https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:128: EXPECT_EQ(left, points_->GetOldest().location); Instead of testing GetOldest/GetNewest here, I'd write a separate test that verifies the oldest point changes as points are removed. https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:131: gfx::Point new_left_bottom(0, 40); // Add a new point which will expand the bounding box. https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:150: // removed. Can you add comments at the interesting points in this test? Right now it is hard to follow. ie, for a block of code, you're trying to simulate some behavior. What is the behavior? https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:191: EXPECT_EQ(0, mode_test_api_->laser_points().GetNumberOfPoints()); I'd add another EXPECT_EQ(0, ...) right after the OnDisable call as well. // Verify disabling the mode will clear any active points. mode_test_api_->OnDisable() EXPECT_EQ(0, count()) // Verify that the laser pointer ignores any input while disabled. GetEventGenerator()... GetEventGenerator()... EXPECT_EQ(0, count()) // Verify that enabling the laser pointer immediately adds a point at the current mouse location. ... https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_view.cc (right): https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:107: // Normalized is a value between [0,1] where 0 means the point is about to Update comment to reflect the current variable name. https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:137: } else if (point_count == num_points_ - 1) { Can this else if be combined with the previous one? if (point_count == 0) { ... } else if (DistanceBetween(...) && (point_count != num_points - 1)) { ... }
Patchset #8 (id:180001) has been deleted
The colors are in a different patch set not uploaded yet. https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc (right): https://codereview.chromium.org/2239743004/diff/140001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:62: points_.reset(); On 2016/08/22 18:37:36, jdufault wrote: > On 2016/08/22 18:07:20, sammiequon wrote: > > On 2016/08/19 21:05:56, jdufault wrote: > > > I don't think you need to reset these instances; the entire class should get > > > destroyed so the dtor will do it for you. > > > > Laser pointer mode dtor removes the pointer watcher and this has to happen > > before TearDown otherwise tear down will complain there is still an pointer > > watcher observer when it expects none. > > Please add a comment describing this requirement. Done. https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_points.cc (right): https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:76: points_.begin(), points_.end(), [&newest, &life_duration](LaserPoint& p) { On 2016/08/22 18:37:36, jdufault wrote: > Can you capture [this] and use points_.back() and life_duration_ directly? Done. https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc (right): https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:13: class LaserPointerPointsTestApi : public LaserPointerPoints { On 2016/08/22 18:37:36, jdufault wrote: > Can you make this take the LaserPointerPoints instance as parameter, similar to > how LaserPointerModeTestApi works? > > Also, instead of using friend class for inheritance prefer to use protected. It > communicates intent more clearly. Done. https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:24: LaserPointerPoints::LaserPoint new_point; On 2016/08/22 18:37:36, jdufault wrote: > Instead of building a new_point instance just call AddPoint(location). This > means you can also remove the |time| parameter. > > This will also take care of calling ClearOldPoints(). I did it this way since AddPoint adds it at Time::Now(), so if the computer is running the test is super slow it may mess it up. https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:60: namespace { On 2016/08/22 18:37:36, jdufault wrote: > The anonymous namespace should be at the top. Pull the *TestApi classes into > separate files if you need to. Done. https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:62: const int kTestPointsLifetime = 5; On 2016/08/22 18:37:36, jdufault wrote: > Can you specify the unit here? Done. https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:120: points_->AddPoint(top_right); On 2016/08/22 18:37:36, jdufault wrote: > I think you should also verify the bounding box when the points_ object has 0 > and 1 points. Done. https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:122: // Test laser pointer points collection and helper functions. On 2016/08/22 18:37:36, jdufault wrote: > Replace this comment with a series of comments that give a very high-level > overview of what each section of code does. I've written an example one below > (line 131) Done. https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:128: EXPECT_EQ(left, points_->GetOldest().location); On 2016/08/22 18:37:36, jdufault wrote: > Instead of testing GetOldest/GetNewest here, I'd write a separate test that > verifies the oldest point changes as points are removed. The time stuff is in the next test. https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:131: gfx::Point new_left_bottom(0, 40); On 2016/08/22 18:37:36, jdufault wrote: > // Add a new point which will expand the bounding box. Done. https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:150: // removed. On 2016/08/22 18:37:36, jdufault wrote: > Can you add comments at the interesting points in this test? Right now it is > hard to follow. > > ie, for a block of code, you're trying to simulate some behavior. What is the > behavior? Done. https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:191: EXPECT_EQ(0, mode_test_api_->laser_points().GetNumberOfPoints()); On 2016/08/22 18:37:36, jdufault wrote: > I'd add another EXPECT_EQ(0, ...) right after the OnDisable call as well. > > > // Verify disabling the mode will clear any active points. > mode_test_api_->OnDisable() > EXPECT_EQ(0, count()) > > // Verify that the laser pointer ignores any input while disabled. > GetEventGenerator()... > GetEventGenerator()... > EXPECT_EQ(0, count()) > > // Verify that enabling the laser pointer immediately adds a point at the > current mouse location. > ... Done. https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_view.cc (right): https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:107: // Normalized is a value between [0,1] where 0 means the point is about to On 2016/08/22 18:37:36, jdufault wrote: > Update comment to reflect the current variable name. Done. https://codereview.chromium.org/2239743004/diff/160001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:137: } else if (point_count == num_points_ - 1) { On 2016/08/22 18:37:37, jdufault wrote: > Can this else if be combined with the previous one? > > if (point_count == 0) { > ... > } else if (DistanceBetween(...) && (point_count != num_points - 1)) { > ... > } Done.
https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc (right): https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:104: timer_->Reset(); Should the timer be reset only if it is not running? https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_points.cc (right): https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:78: return this->points_.back().creation_time - p.creation_time < Can you remove the explicit this? https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_testapis.cc (right): https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_testapis.cc:18: int LaserPointerPointsTestApi::get_number_of_points() const { Keep method names the same: GetNumberOfPoints() https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_testapis.h (right): https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_testapis.h:8: #include "ash/common/system/chromeos/palette/tools/laser_pointer_points.h" Do you need this include? https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_testapis.h:15: class LaserPointerPointsTestApi { Move these into separate files. https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc (right): https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:78: // Should not expand bounding box. These assertions should be verified as you go, so I'd recommend adding more EXPECT_* statements. https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:115: start_time, test_point); Every call uses the same two constant parameters; you should remove the parameters. https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:119: EXPECT_EQ(points_test_api_->get_number_of_points(), 2); I don't think we specify it strictly, but typically Chrome does: EXPECT_*(expected, actual) For example: EXPECT_EQ(points_test_api_->GetNumberOfPoints(), 4) vs EXPECT_EQ(4, points_test_api_->GetNumberOfPoints()) https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:131: // Verify adding 3 points one second apart each will add 3 points to the This comment should go on line 125, before you add the points. https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:134: // Verify adding 1 point three seconds later will remove 2 points which are newline before each test "section" (ie, above each comment block where you test a specific function) https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:149: // Verify enabling the mode will start with a single point at the current Move comment up a line. https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:161: // Verify disabling the mode will clear any active points. Move comment up a line https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:165: // Verify that the laser pointer does not add points while disabled. Move comment up two lines, add a newline before it. https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_view.cc (right): https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:55: } // namespace newline above this line https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:62: widget_.reset(CreateLaserPointerWidget()); I would inline the CreateLaserPointerWidget function because it does not fully construct the widget. https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:101: gfx::Point last_point, current_point; What do you think of previous_point compared to last_point? last_point could refer to the last point in the list, or the last point you accessed, so it is a bit confusing. https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:131: if (point_count == 0) { It might be interesting to see what the code looks like if you initialize last_point before entering the for loop. Basically, inline the first iteration. The last iteration is already inlined, so it'd be a nice parallel as well. If inlining the first iteration ends up looking pretty clean, then it makes this loop easier to understand because there is no longer any need to reason about the state of last_point.
Icons multicolor as well. https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc (right): https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:104: timer_->Reset(); On 2016/08/23 02:11:25, jdufault wrote: > Should the timer be reset only if it is not running? I don't think so, otherwise the timer would never start. https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_points.cc (right): https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:78: return this->points_.back().creation_time - p.creation_time < On 2016/08/23 02:11:25, jdufault wrote: > Can you remove the explicit this? Done. https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_testapis.cc (right): https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_testapis.cc:18: int LaserPointerPointsTestApi::get_number_of_points() const { On 2016/08/23 02:11:25, jdufault wrote: > Keep method names the same: GetNumberOfPoints() Done. https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_testapis.h (right): https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_testapis.h:8: #include "ash/common/system/chromeos/palette/tools/laser_pointer_points.h" On 2016/08/23 02:11:25, jdufault wrote: > Do you need this include? Done. https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_testapis.h:15: class LaserPointerPointsTestApi { On 2016/08/23 02:11:25, jdufault wrote: > Move these into separate files. Done. https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc (right): https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:78: // Should not expand bounding box. On 2016/08/23 02:11:26, jdufault wrote: > These assertions should be verified as you go, so I'd recommend adding more > EXPECT_* statements. Done. https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:115: start_time, test_point); On 2016/08/23 02:11:26, jdufault wrote: > Every call uses the same two constant parameters; you should remove the > parameters. Done. https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:119: EXPECT_EQ(points_test_api_->get_number_of_points(), 2); On 2016/08/23 02:11:26, jdufault wrote: > I don't think we specify it strictly, but typically Chrome does: > > EXPECT_*(expected, actual) > > > For example: > > EXPECT_EQ(points_test_api_->GetNumberOfPoints(), 4) > > vs > > EXPECT_EQ(4, points_test_api_->GetNumberOfPoints()) Done. https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:131: // Verify adding 3 points one second apart each will add 3 points to the On 2016/08/23 02:11:26, jdufault wrote: > This comment should go on line 125, before you add the points. Done. https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:134: // Verify adding 1 point three seconds later will remove 2 points which are On 2016/08/23 02:11:26, jdufault wrote: > newline before each test "section" (ie, above each comment block where you test > a specific function) Done. https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:149: // Verify enabling the mode will start with a single point at the current On 2016/08/23 02:11:26, jdufault wrote: > Move comment up a line. Done. https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:161: // Verify disabling the mode will clear any active points. On 2016/08/23 02:11:26, jdufault wrote: > Move comment up a line Done. https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:165: // Verify that the laser pointer does not add points while disabled. On 2016/08/23 02:11:26, jdufault wrote: > Move comment up two lines, add a newline before it. Done. https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_view.cc (right): https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:55: } // namespace On 2016/08/23 02:11:26, jdufault wrote: > newline above this line Done. https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:62: widget_.reset(CreateLaserPointerWidget()); On 2016/08/23 02:11:26, jdufault wrote: > I would inline the CreateLaserPointerWidget function because it does not fully > construct the widget. Done. https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:101: gfx::Point last_point, current_point; On 2016/08/23 02:11:26, jdufault wrote: > What do you think of previous_point compared to last_point? > > last_point could refer to the last point in the list, or the last point you > accessed, so it is a bit confusing. Done. https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:131: if (point_count == 0) { On 2016/08/23 02:11:26, jdufault wrote: > It might be interesting to see what the code looks like if you initialize > last_point before entering the for loop. Basically, inline the first iteration. > The last iteration is already inlined, so it'd be a nice parallel as well. > > If inlining the first iteration ends up looking pretty clean, then it makes this > loop easier to understand because there is no longer any need to reason about > the state of last_point. We could set last_point to GetOldest() and remove this part of the if else, but I don't think that makes it easier to understand. What do you think?
Description was changed from ========== Palette tool laser prototype. Classes: LaserPointerMode - Serves as controller as receiver of events. LaserPointerPoints - Helper class that contains collections of the points. LaserPointerView - Responsible for rendering the point(s). Split of from 2231533004. BUG=616143 ========== to ========== Palette tool laser prototype. Classes: LaserPointerMode - Serves as controller as receiver of events. LaserPointerPoints - Helper class that contains collections of the points. LaserPointerView - Responsible for rendering the point(s). Split of from 2231533004. TEST=./out/Release/ash_unittests --gtest_filter=LaserPointer* BUG=616143 ==========
https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc (right): https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:104: timer_->Reset(); On 2016/08/23 19:30:02, sammiequon wrote: > On 2016/08/23 02:11:25, jdufault wrote: > > Should the timer be reset only if it is not running? > > I don't think so, otherwise the timer would never start. I mean, if (!timer_->IsRunning()) timer_->Reset() This changes how idle points are added. Right now if you move the mouse cursor the idle timer gets reset. With this the points would be added at a constant rate. https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_view.cc (right): https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:131: if (point_count == 0) { On 2016/08/23 19:30:02, sammiequon wrote: > On 2016/08/23 02:11:26, jdufault wrote: > > It might be interesting to see what the code looks like if you initialize > > last_point before entering the for loop. Basically, inline the first > iteration. > > The last iteration is already inlined, so it'd be a nice parallel as well. > > > > If inlining the first iteration ends up looking pretty clean, then it makes > this > > loop easier to understand because there is no longer any need to reason about > > the state of last_point. > > We could set last_point to GetOldest() and remove this part of the if else, but > I don't think that makes it easier to understand. What do you think? If we inline the first iteration we can remove the if/else entirely. The if (Distance(...)) statement could then be moved to the beginning of the for loop as well. For GetOldest, were you thinking of something like this? last_point = GetOldest(); // Skip the first iteration for (auto it = laser_points.PointsStart()++, ...) { // ... } https://codereview.chromium.org/2239743004/diff/220001/ash/ash.gyp File ash/ash.gyp (right): https://codereview.chromium.org/2239743004/diff/220001/ash/ash.gyp#newcode964 ash/ash.gyp:964: 'common/system/chromeos/palette/tools/laser_pointer_mode_testapi.cc', files should be named *_test_api.h, since it is TestApi and not Testapi https://codereview.chromium.org/2239743004/diff/220001/ash/ash.gyp#newcode966 ash/ash.gyp:966: 'common/system/chromeos/palette/tools/laser_pointer_points_testapi.cc', cc files go above h files. foo.cc, foo.h, https://codereview.chromium.org/2239743004/diff/220001/ash/common/palette_del... File ash/common/palette_delegate.h (right): https://codereview.chromium.org/2239743004/diff/220001/ash/common/palette_del... ash/common/palette_delegate.h:19: virtual void OnLaserPointerEnabled() = 0; Add a comment, something like // Called when the laser pointer has been enabled or disabled. https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... File ash/common/system/chromeos/palette/palette_tool.cc (right): https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... ash/common/system/chromeos/palette/palette_tool.cc:31: // TODO(sammiequon): Icons do not seem to support multiple colors. Remove TODO https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc (right): https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:51: if (WmShell::Get()->palette_delegate()) TestPaletteDelegate should be available now. https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_points.cc (right): https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:17: void LaserPointerPoints::AddPoint(const gfx::Point& point) { Instead of using base::Time::Now(), it might look better if we use the actual event time. If we have a queue of events to process, then they will get added at the correct times. https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:78: return points_.back().creation_time - p.creation_time < life_duration_; Use GetNewest() https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_points_testapi.cc (right): https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points_testapi.cc:13: : time_(base::Time::Now()), new_point_time_? Add a comment to the member saying it is the time that new points are added. https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points_testapi.cc:14: location_(gfx::Point()), Remove location_ and use gfx::Point() directly https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc (right): https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:67: } // namespace newline between end of class and end of namespace https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:139: // Verify adding 1 point three seconds later will remove 2 points which are Awesome! These tests read much better.
https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_view.cc (right): https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:131: if (point_count == 0) { On 2016/08/23 21:52:04, jdufault wrote: > On 2016/08/23 19:30:02, sammiequon wrote: > > On 2016/08/23 02:11:26, jdufault wrote: > > > It might be interesting to see what the code looks like if you initialize > > > last_point before entering the for loop. Basically, inline the first > > iteration. > > > The last iteration is already inlined, so it'd be a nice parallel as well. > > > > > > If inlining the first iteration ends up looking pretty clean, then it makes > > this > > > loop easier to understand because there is no longer any need to reason > about > > > the state of last_point. > > > > We could set last_point to GetOldest() and remove this part of the if else, > but > > I don't think that makes it easier to understand. What do you think? > > If we inline the first iteration we can remove the if/else entirely. The if > (Distance(...)) statement could then be moved to the beginning of the for loop > as well. > > For GetOldest, were you thinking of something like this? > > last_point = GetOldest(); > // Skip the first iteration > for (auto it = laser_points.PointsStart()++, ...) { > // ... > } Yes but we cannot move the distance if to the beginning of the loop since my crude algorithm depends on all the calculations beforehand. https://codereview.chromium.org/2239743004/diff/220001/ash/ash.gyp File ash/ash.gyp (right): https://codereview.chromium.org/2239743004/diff/220001/ash/ash.gyp#newcode964 ash/ash.gyp:964: 'common/system/chromeos/palette/tools/laser_pointer_mode_testapi.cc', On 2016/08/23 21:52:05, jdufault wrote: > files should be named *_test_api.h, since it is TestApi and not Testapi Done. https://codereview.chromium.org/2239743004/diff/220001/ash/ash.gyp#newcode966 ash/ash.gyp:966: 'common/system/chromeos/palette/tools/laser_pointer_points_testapi.cc', On 2016/08/23 21:52:05, jdufault wrote: > cc files go above h files. > > foo.cc, > foo.h, > > Done. https://codereview.chromium.org/2239743004/diff/220001/ash/common/palette_del... File ash/common/palette_delegate.h (right): https://codereview.chromium.org/2239743004/diff/220001/ash/common/palette_del... ash/common/palette_delegate.h:19: virtual void OnLaserPointerEnabled() = 0; On 2016/08/23 21:52:05, jdufault wrote: > Add a comment, something like > > // Called when the laser pointer has been enabled or disabled. Done. https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... File ash/common/system/chromeos/palette/palette_tool.cc (right): https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... ash/common/system/chromeos/palette/palette_tool.cc:31: // TODO(sammiequon): Icons do not seem to support multiple colors. On 2016/08/23 21:52:05, jdufault wrote: > Remove TODO Done. https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc (right): https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:51: if (WmShell::Get()->palette_delegate()) On 2016/08/23 21:52:05, jdufault wrote: > TestPaletteDelegate should be available now. Done. https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_points.cc (right): https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:17: void LaserPointerPoints::AddPoint(const gfx::Point& point) { On 2016/08/23 21:52:05, jdufault wrote: > Instead of using base::Time::Now(), it might look better if we use the actual > event time. If we have a queue of events to process, then they will get added at > the correct times. I was wondering if the event times might be a little earlier than current time by the time they reach here. Which may conflict with the times the timer creates. https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:78: return points_.back().creation_time - p.creation_time < life_duration_; On 2016/08/23 21:52:05, jdufault wrote: > Use GetNewest() Done. https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_points_testapi.cc (right): https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points_testapi.cc:13: : time_(base::Time::Now()), On 2016/08/23 21:52:05, jdufault wrote: > new_point_time_? > > Add a comment to the member saying it is the time that new points are added. Done. https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points_testapi.cc:14: location_(gfx::Point()), On 2016/08/23 21:52:05, jdufault wrote: > Remove location_ and use gfx::Point() directly Done. https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc (right): https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:67: } // namespace On 2016/08/23 21:52:05, jdufault wrote: > newline between end of class and end of namespace Done. https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:139: // Verify adding 1 point three seconds later will remove 2 points which are On 2016/08/23 21:52:05, jdufault wrote: > Awesome! These tests read much better. =D
lgtm after comments https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc (right): https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:104: timer_->Reset(); On 2016/08/23 21:52:04, jdufault wrote: > On 2016/08/23 19:30:02, sammiequon wrote: > > On 2016/08/23 02:11:25, jdufault wrote: > > > Should the timer be reset only if it is not running? > > > > I don't think so, otherwise the timer would never start. > > I mean, > > if (!timer_->IsRunning()) > timer_->Reset() > > This changes how idle points are added. Right now if you move the mouse cursor > the idle timer gets reset. With this the points would be added at a constant > rate. ? https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_points.cc (right): https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:17: void LaserPointerPoints::AddPoint(const gfx::Point& point) { On 2016/08/24 00:04:05, sammiequon wrote: > On 2016/08/23 21:52:05, jdufault wrote: > > Instead of using base::Time::Now(), it might look better if we use the actual > > event time. If we have a queue of events to process, then they will get added > at > > the correct times. > > I was wondering if the event times might be a little earlier than current time > by the time they reach here. Which may conflict with the times the timer > creates. What do you mean by conflict with the times that the timer creates? https://codereview.chromium.org/2239743004/diff/260001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_mode_test_api.cc (right): https://codereview.chromium.org/2239743004/diff/260001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode_test_api.cc:34: } // namespace ash newline above this https://codereview.chromium.org/2239743004/diff/260001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_points.cc (right): https://codereview.chromium.org/2239743004/diff/260001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:82: } // namespace ash newline above https://codereview.chromium.org/2239743004/diff/260001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_points_test_api.h (right): https://codereview.chromium.org/2239743004/diff/260001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points_test_api.h:23: void MoveForwardInTime(const base::TimeDelta& delta); Add a comment to this function saying it moves existing points back in time by |delta| and adds a new point at (0, 0). https://codereview.chromium.org/2239743004/diff/260001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_view.cc (right): https://codereview.chromium.org/2239743004/diff/260001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:127: if (DistanceBetweenPoints(previous_point, current_point) > Move this below the radius computation double radius = LinearInterpolate(...); // Do not draw points that are within a stroke width of each other; // doing so results in a very jagged line. float distance_threshold = float{radius * 2}; if (DistanceBetweenPoints(previous, current) < distance_thresold && point_count != num_points_ - 1) { continue; } All of the drawing code will then be close by as well.
https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc (right): https://codereview.chromium.org/2239743004/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:104: timer_->Reset(); On 2016/08/24 00:20:57, jdufault wrote: > On 2016/08/23 21:52:04, jdufault wrote: > > On 2016/08/23 19:30:02, sammiequon wrote: > > > On 2016/08/23 02:11:25, jdufault wrote: > > > > Should the timer be reset only if it is not running? > > > > > > I don't think so, otherwise the timer would never start. > > > > I mean, > > > > if (!timer_->IsRunning()) > > timer_->Reset() > > > > This changes how idle points are added. Right now if you move the mouse cursor > > the idle timer gets reset. With this the points would be added at a constant > > rate. > > ? Done. https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_points.cc (right): https://codereview.chromium.org/2239743004/diff/220001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:17: void LaserPointerPoints::AddPoint(const gfx::Point& point) { On 2016/08/24 00:20:57, jdufault wrote: > On 2016/08/24 00:04:05, sammiequon wrote: > > On 2016/08/23 21:52:05, jdufault wrote: > > > Instead of using base::Time::Now(), it might look better if we use the > actual > > > event time. If we have a queue of events to process, then they will get > added > > at > > > the correct times. > > > > I was wondering if the event times might be a little earlier than current time > > by the time they reach here. Which may conflict with the times the timer > > creates. > > What do you mean by conflict with the times that the timer creates? As per offline chat, this will stay the same. https://codereview.chromium.org/2239743004/diff/260001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_mode_test_api.cc (right): https://codereview.chromium.org/2239743004/diff/260001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode_test_api.cc:34: } // namespace ash On 2016/08/24 00:20:57, jdufault wrote: > newline above this Done. https://codereview.chromium.org/2239743004/diff/260001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_points.cc (right): https://codereview.chromium.org/2239743004/diff/260001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:82: } // namespace ash On 2016/08/24 00:20:57, jdufault wrote: > newline above Done. https://codereview.chromium.org/2239743004/diff/260001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_points_test_api.h (right): https://codereview.chromium.org/2239743004/diff/260001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points_test_api.h:23: void MoveForwardInTime(const base::TimeDelta& delta); On 2016/08/24 00:20:57, jdufault wrote: > Add a comment to this function saying it moves existing points back in time by > |delta| and adds a new point at (0, 0). Done. https://codereview.chromium.org/2239743004/diff/260001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_view.cc (right): https://codereview.chromium.org/2239743004/diff/260001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:127: if (DistanceBetweenPoints(previous_point, current_point) > On 2016/08/24 00:20:57, jdufault wrote: > Move this below the radius computation > > > double radius = LinearInterpolate(...); > > // Do not draw points that are within a stroke width of each other; > // doing so results in a very jagged line. > float distance_threshold = float{radius * 2}; > if (DistanceBetweenPoints(previous, current) < distance_thresold && > point_count != num_points_ - 1) { > continue; > } > > > All of the drawing code will then be close by as well. The paint/opacity code still has to be in between since the drawing uses the paint. I did move translating the point above all this though.
sammiequon@chromium.org changed reviewers: + estade@chromium.org
estade@ - Please take a look - ui/gfx/vector_icons. Thanks!
https://codereview.chromium.org/2239743004/diff/300001/ash/ash_strings.grd File ash/ash_strings.grd (right): https://codereview.chromium.org/2239743004/diff/300001/ash/ash_strings.grd#ne... ash/ash_strings.grd:137: <message name="IDS_ASH_PALETTE_LASER_POINTER_MODE" desc="Title of the laser pointer in the palette (a pop-up panel next to the status tray). Clicking this turns the mouse into a laser pointer. Additionally, the palette tray is closed."> Move this immediately above IDS_ASH_PALETTE_TITLE String should be "Laser pointer mode"
sammiequon@chromium.org changed reviewers: + oshima@chromium.org
oshima@ - Please take a look. Thanks!
https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc (right): https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:19: const int kAddStationaryPointsDelayMs = 5; please document these variables, and please keep at one new line befor and after each definition. Something like namespace { // Comment const int kFooMs = 200; // Comment const int kBarMs = 5; } // namespace https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:35: WmShell::Get()->RemovePointerWatcher(this); q: No need to disable? https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:49: // TODO(sammiequon): Remove check and set palette delegate to test delegate which check? https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:99: current_mouse_location_ = location_in_screen; am i correct that it always start recording when the later pointer event is detected? https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_mode_test_api.cc (right): https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode_test_api.cc:31: gfx::Point LaserPointerModeTestApi::current_mouse_location() { const gfx::Point& https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_points.cc (right): https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:36: std::numeric_limits<int>::min()); optional: since you're checking if it's empty, you cam just start with the initial point. https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:41: std::max(point.location.y(), max_point.y())); gfx::Point::SetToMin, SetToMax? https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_points.h (right): https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.h:50: std::deque<LaserPoint>::const_iterator PointsEnd() const; Optional: You may want to just expose const std::deque<LaserPoint>& laser_points() because you can use range based for loop, and stl_util.h has functions that take a collection. https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_points_test_api.cc (right): https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points_test_api.cc:28: new_point.location = gfx::Point(); you don't need this. https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc (right): https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:33: void TearDown() override { AshTestBase::TearDown(); } you don't need this. https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:37: std::unique_ptr<LaserPointerPointsTestApi> points_test_api_; optional: do these have to be allocated in heap? https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:65: std::unique_ptr<LaserPointerModeTestApi> mode_test_api_; ditto https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:81: gfx::Point last(2, 2); const ? https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_view.cc (right): https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:79: bounding_box.height() + (kPointInitialRadius * 2)); Offset(-kPointInInitialRadius, -kPointInInitialRadius); https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:88: canvas->Save(); Do you need this? (it doesn't looks like but) if you need it, then you need to call Restore at the end. https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:99: int point_count = 0; fyi: you can get the count by (itr - begin()) + 1. https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:129: double{kPointFinalOpacity}, relative_time)}; do you have a pointer to the doc that says this (using uniform initializer as a way to cast) is allowed? https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_view.h (right): https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.h:29: LaserPointerView(base::TimeDelta life_duration); explicit
Patchset #14 (id:320001) has been deleted
https://codereview.chromium.org/2239743004/diff/300001/ash/ash_strings.grd File ash/ash_strings.grd (right): https://codereview.chromium.org/2239743004/diff/300001/ash/ash_strings.grd#ne... ash/ash_strings.grd:137: <message name="IDS_ASH_PALETTE_LASER_POINTER_MODE" desc="Title of the laser pointer in the palette (a pop-up panel next to the status tray). Clicking this turns the mouse into a laser pointer. Additionally, the palette tray is closed."> On 2016/08/24 21:27:41, jdufault wrote: > Move this immediately above IDS_ASH_PALETTE_TITLE > > String should be "Laser pointer mode" Done. https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc (right): https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:19: const int kAddStationaryPointsDelayMs = 5; On 2016/08/24 23:01:03, oshima wrote: > please document these variables, and please keep at one new line befor and after > each definition. Something like > > namespace { > > // Comment > const int kFooMs = 200; > > // Comment > const int kBarMs = 5; > > } // namespace Done. https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:35: WmShell::Get()->RemovePointerWatcher(this); On 2016/08/24 23:01:03, oshima wrote: > q: No need to disable? Done. https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:49: // TODO(sammiequon): Remove check and set palette delegate to test delegate On 2016/08/24 23:01:03, oshima wrote: > which check? Done. https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:99: current_mouse_location_ = location_in_screen; On 2016/08/24 23:01:03, oshima wrote: > am i correct that it always start recording when the later pointer event is > detected? Yes it constantly updates even while the laser is not active so we can draw the laser at the correct starting point. Is there a way to get the current cursor location on enable? That way we can remove the pointer watcher when the laser is disabled. https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_mode_test_api.cc (right): https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_mode_test_api.cc:31: gfx::Point LaserPointerModeTestApi::current_mouse_location() { On 2016/08/24 23:01:03, oshima wrote: > const gfx::Point& Done. https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_points.cc (right): https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:36: std::numeric_limits<int>::min()); On 2016/08/24 23:01:03, oshima wrote: > optional: since you're checking if it's empty, you cam just start with the > initial point. Done. https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.cc:41: std::max(point.location.y(), max_point.y())); On 2016/08/24 23:01:03, oshima wrote: > gfx::Point::SetToMin, SetToMax? Done. https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_points.h (right): https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points.h:50: std::deque<LaserPoint>::const_iterator PointsEnd() const; On 2016/08/24 23:01:03, oshima wrote: > Optional: You may want to just expose const std::deque<LaserPoint>& > laser_points() > because you can use range based for loop, and stl_util.h has functions that take > a collection. Done. https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_points_test_api.cc (right): https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_points_test_api.cc:28: new_point.location = gfx::Point(); On 2016/08/24 23:01:03, oshima wrote: > you don't need this. Done. https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc (right): https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:33: void TearDown() override { AshTestBase::TearDown(); } On 2016/08/24 23:01:03, oshima wrote: > you don't need this. Done. https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:37: std::unique_ptr<LaserPointerPointsTestApi> points_test_api_; On 2016/08/24 23:01:03, oshima wrote: > optional: do these have to be allocated in heap? Nope. Done. https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:65: std::unique_ptr<LaserPointerModeTestApi> mode_test_api_; On 2016/08/24 23:01:03, oshima wrote: > ditto This one needs to because otherwise there will be error; see TearDown(). https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc:81: gfx::Point last(2, 2); On 2016/08/24 23:01:04, oshima wrote: > const ? Done. https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_view.cc (right): https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:79: bounding_box.height() + (kPointInitialRadius * 2)); On 2016/08/24 23:01:04, oshima wrote: > Offset(-kPointInInitialRadius, -kPointInInitialRadius); Done. https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:88: canvas->Save(); On 2016/08/24 23:01:04, oshima wrote: > Do you need this? (it doesn't looks like but) if you need it, then you need to > call Restore at the end. Done. https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:99: int point_count = 0; On 2016/08/24 23:01:04, oshima wrote: > fyi: you can get the count by (itr - begin()) + 1. I don't think this works with std::deque because the elements are not contiguous in memory. https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:129: double{kPointFinalOpacity}, relative_time)}; On 2016/08/24 23:01:04, oshima wrote: > do you have a pointer to the doc that says this (using uniform initializer as a > way to cast) is allowed? https://google.github.io/styleguide/cppguide.html#Casting - first line https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_view.h (right): https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.h:29: LaserPointerView(base::TimeDelta life_duration); On 2016/08/24 23:01:04, oshima wrote: > explicit Done.
lgtm https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... File ash/common/system/chromeos/palette/tools/laser_pointer_view.cc (right): https://codereview.chromium.org/2239743004/diff/300001/ash/common/system/chro... ash/common/system/chromeos/palette/tools/laser_pointer_view.cc:129: double{kPointFinalOpacity}, relative_time)}; On 2016/08/25 17:59:05, sammiequon wrote: > On 2016/08/24 23:01:04, oshima wrote: > > do you have a pointer to the doc that says this (using uniform initializer as > a > > way to cast) is allowed? > > https://google.github.io/styleguide/cppguide.html#Casting - first line ok thanks.
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2239743004/diff/400001/ash/ash_strings.grd File ash/ash_strings.grd (right): https://codereview.chromium.org/2239743004/diff/400001/ash/ash_strings.grd#ne... ash/ash_strings.grd:384: Laser Pointer Mode FYI this string has been submitted in a separate patch to get it in before branch.
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_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2239743004/diff/420001/ash/ash_strings.grd File ash/ash_strings.grd (right): https://codereview.chromium.org/2239743004/diff/420001/ash/ash_strings.grd#ne... ash/ash_strings.grd:393: <message name="IDS_ASH_PALETTE_TITLE" desc="The title of the palette in the ash shelf."> Revert these changes.
https://codereview.chromium.org/2239743004/diff/420001/ash/ash_strings.grd File ash/ash_strings.grd (right): https://codereview.chromium.org/2239743004/diff/420001/ash/ash_strings.grd#ne... ash/ash_strings.grd:393: <message name="IDS_ASH_PALETTE_TITLE" desc="The title of the palette in the ash shelf."> On 2016/08/26 22:01:06, jdufault wrote: > Revert these changes. Done.
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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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.
Patchset #22 (id:500001) has been deleted
estade@ - Please take a look - ui/gfx/vector_icons. Thanks!
vector icons 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, oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2239743004/#ps480001 (title: "Rebased.")
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
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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, oshima@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2239743004/#ps520001 (title: "Rebased.")
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 ========== Palette tool laser prototype. Classes: LaserPointerMode - Serves as controller as receiver of events. LaserPointerPoints - Helper class that contains collections of the points. LaserPointerView - Responsible for rendering the point(s). Split of from 2231533004. TEST=./out/Release/ash_unittests --gtest_filter=LaserPointer* BUG=616143 ========== to ========== Palette tool laser prototype. Classes: LaserPointerMode - Serves as controller as receiver of events. LaserPointerPoints - Helper class that contains collections of the points. LaserPointerView - Responsible for rendering the point(s). Split of from 2231533004. TEST=./out/Release/ash_unittests --gtest_filter=LaserPointer* BUG=616143 ==========
Message was sent while issue was closed.
Committed patchset #22 (id:520001)
Message was sent while issue was closed.
Description was changed from ========== Palette tool laser prototype. Classes: LaserPointerMode - Serves as controller as receiver of events. LaserPointerPoints - Helper class that contains collections of the points. LaserPointerView - Responsible for rendering the point(s). Split of from 2231533004. TEST=./out/Release/ash_unittests --gtest_filter=LaserPointer* BUG=616143 ========== to ========== Palette tool laser prototype. Classes: LaserPointerMode - Serves as controller as receiver of events. LaserPointerPoints - Helper class that contains collections of the points. LaserPointerView - Responsible for rendering the point(s). Split of from 2231533004. TEST=./out/Release/ash_unittests --gtest_filter=LaserPointer* BUG=616143 Committed: https://crrev.com/01725f5d867a32b82bdd3adf5088cfbab0d54f29 Cr-Commit-Position: refs/heads/master@{#415496} ==========
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/01725f5d867a32b82bdd3adf5088cfbab0d54f29 Cr-Commit-Position: refs/heads/master@{#415496} |
