Pointer watcher modifications to support laser pointer.

Pointer watcher modifications to support laser pointer. 

Modifications made to pointer watcher and related classes so that it is compatible with laser pointer. (issue 2239743004)

BUG=616143

[jdufault, 2016-08-11 01:09:56]

jdufault@chromium.org changed reviewers:
+ jdufault@chromium.org

[jdufault, 2016-08-11 01:09:57]

https://codereview.chromium.org/2231533004/diff/1/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2231533004/diff/1/ash/shell.cc#newcode834
ash/shell.cc:834: cursor_manager_.get()));
This is causing chrome to consistently crash on shut-down.

[sammiequon, 2016-08-11 23:11:18]

Description was changed from

==========
Palette tool laser prototype.

BUG=616143
==========

to

==========
Palette tool laser prototype. 

Also a bunch of modifications to various wm_shell related files to allow
PointerWatcher to be used in Laser Pointer Mode.

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
==========

[sammiequon, 2016-08-11 23:11:59]

sammiequon@chromium.org changed reviewers:
+ jamescook@chromium.org

[sammiequon, 2016-08-11 23:19:36]

jamescook@ - Please take a look at ash/aura/*,
ash/common/pointer_watcher_delegate.h,
ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc,
ash/common/*, ash/mus/*, ash/*, ash/shell*, ash/sysui/*. Most of it is just
propagating wants_moves up from wm_shell.h. Thanks!

jdufault@ - Please take a look at ash/common/system/chromeos/palette/tools/*,
chrome/browser/ui/ash*. Thanks!

[jdufault, 2016-08-11 23:23:03]

> Also a bunch of modifications to various wm_shell related files to allow
PointerWatcher to be used in Laser Pointer Mode.

Please split these changes into a separate CL.

[jdufault, 2016-08-11 23:41:55]

I started going through some of the CL; I'll go through the rest after the CLs
are split up.

https://codereview.chromium.org/2231533004/diff/20001/ash/aura/wm_shell_aura.h
File ash/aura/wm_shell_aura.h (right):

https://codereview.chromium.org/2231533004/diff/20001/ash/aura/wm_shell_aura....
ash/aura/wm_shell_aura.h:72: void AddPointerWatcher(views::PointerWatcher*
watcher,
Default arguments are banned on virtual functions.

https://codereview.chromium.org/2231533004/diff/20001/ash/common/palette_dele...
File ash/common/palette_delegate.h (right):

https://codereview.chromium.org/2231533004/diff/20001/ash/common/palette_dele...
ash/common/palette_delegate.h:19: // TODO(jdufault): This delegate will be
implemented in later CLs.
Remove TODO

https://codereview.chromium.org/2231533004/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc (right):

https://codereview.chromium.org/2231533004/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:20:
timer_repeat_count_(0),
Initialize in header.

https://codereview.chromium.org/2231533004/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:22:
base::TimeDelta::FromMilliseconds(int{kPointLifeDurationMs})) {
Remove the int{} cast.

https://codereview.chromium.org/2231533004/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:23:
laser_pointer_view_ = new LaserPointerView();
Use std::unique_ptr

https://codereview.chromium.org/2231533004/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:68:
timer_.reset(new base::Timer(
Is this the output from git cl format?

https://codereview.chromium.org/2231533004/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:105: if
(event.type() == ui::ET_POINTER_MOVED) {
We want to only run this logic for pen pointer events as well.

event.pointer_details().pointer_type == ui::EventPointerType::POINTER_TYPE_PEN)

https://codereview.chromium.org/2231533004/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc:107: if
(enabled()) {
Do we want to always capture the mouse location? Can we move this if check to
the start of the function?

https://codereview.chromium.org/2231533004/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/palette/tools/laser_pointer_mode.h (right):

https://codereview.chromium.org/2231533004/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:21: protected:
Make this all private.

If you need access inside of a test, use a friend class. See ShelfTestApi as an
example.

https://codereview.chromium.org/2231533004/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:33: // PaletteTool
overrides.
// PaletteTool:

https://codereview.chromium.org/2231533004/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/palette/tools/laser_pointer_mode.h:54: static const
int kAddStationaryPointsDelayMs;
Move to cc file in an anonymous namespace.

https://codereview.chromium.org/2231533004/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/palette/tools/laser_pointer_points.h (right):

https://codereview.chromium.org/2231533004/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/palette/tools/laser_pointer_points.h:17: //
LaserPointerPoints is a helper class used for displaying the palette tool
Newline above the comment.

https://codereview.chromium.org/2231533004/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/palette/tools/laser_pointer_points.h:19: // pointer
and its trailing tail.
drop trailing

https://codereview.chromium.org/2231533004/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/palette/tools/laser_pointer_points.h:23: struct
LaserPointerPoint {
Just Point

https://codereview.chromium.org/2231533004/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/palette/tools/laser_pointer_points.h:24: gfx::Point
location_;
Public variables should not have a trailing _

https://codereview.chromium.org/2231533004/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/palette/tools/laser_pointer_points.h:30:
LaserPointerPoints(base::TimeDelta life_duration);
explicit

https://codereview.chromium.org/2231533004/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/palette/tools/laser_pointer_points.h:31:
LaserPointerPoints(const LaserPointerPoints& old);
We need to copy this object?

https://codereview.chromium.org/2231533004/diff/20001/chrome/browser/ui/ash/p...
File chrome/browser/ui/ash/palette_delegate_chromeos.cc (right):

https://codereview.chromium.org/2231533004/diff/20001/chrome/browser/ui/ash/p...
chrome/browser/ui/ash/palette_delegate_chromeos.cc:11:
PaletteDelegateChromeOS::PaletteDelegateChromeOS() {
Revert

[James Cook, 2016-08-11 23:51:11]

I'll also look in more detail after the CLs are split.

Please CC sky on the one for PointerWatchers (just say "CC sky as FYI") since
he's also working near that code.

https://codereview.chromium.org/2231533004/diff/20001/ash/pointer_watcher_del...
File ash/pointer_watcher_delegate_aura.cc (right):

https://codereview.chromium.org/2231533004/diff/20001/ash/pointer_watcher_del...
ash/pointer_watcher_delegate_aura.cc:30: if (wants_moves) {
Since want_moves is always true in the map, you could use a set.

https://codereview.chromium.org/2231533004/diff/20001/ash/pointer_watcher_del...
ash/pointer_watcher_delegate_aura.cc:46: if (event->type() !=
ui::ET_MOUSE_DRAGGED &&
Good catch on drags.

[sammiequon, 2016-08-12 00:11:10]

Description was changed from

==========
Palette tool laser prototype. 

Also a bunch of modifications to various wm_shell related files to allow
PointerWatcher to be used in Laser Pointer Mode.

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

==========
Pointer watcher modifications to support laser pointer. 

Modifications made to pointer watcher and related classes so that it is
compatible with laser pointer. (issue 2239743004)

BUG=616143
==========

[sammiequon, 2016-08-12 18:09:12]

On 2016/08/11 23:51:11, James Cook wrote:
> I'll also look in more detail after the CLs are split.
> 
> Please CC sky on the one for PointerWatchers (just say "CC sky as FYI") since
> he's also working near that code.
> 
>
https://codereview.chromium.org/2231533004/diff/20001/ash/pointer_watcher_del...
> File ash/pointer_watcher_delegate_aura.cc (right):
> 
>
https://codereview.chromium.org/2231533004/diff/20001/ash/pointer_watcher_del...
> ash/pointer_watcher_delegate_aura.cc:30: if (wants_moves) {
> Since want_moves is always true in the map, you could use a set.
> 
>
https://codereview.chromium.org/2231533004/diff/20001/ash/pointer_watcher_del...
> ash/pointer_watcher_delegate_aura.cc:46: if (event->type() !=
> ui::ET_MOUSE_DRAGGED &&
> Good catch on drags.

I think its best to wait until https://codereview.chromium.org/2235363003/
lands. Otherwise the comments regarding the laser pointer have been addressed in
https://codereview.chromium.org/2239743004/.

[James Cook, 2016-08-12 18:27:30]

On 2016/08/12 18:09:12, sammiequon wrote:
> On 2016/08/11 23:51:11, James Cook wrote:
> > I'll also look in more detail after the CLs are split.
> > 
> > Please CC sky on the one for PointerWatchers (just say "CC sky as FYI")
since
> > he's also working near that code.
> > 
> >
>
https://codereview.chromium.org/2231533004/diff/20001/ash/pointer_watcher_del...
> > File ash/pointer_watcher_delegate_aura.cc (right):
> > 
> >
>
https://codereview.chromium.org/2231533004/diff/20001/ash/pointer_watcher_del...
> > ash/pointer_watcher_delegate_aura.cc:30: if (wants_moves) {
> > Since want_moves is always true in the map, you could use a set.
> > 
> >
>
https://codereview.chromium.org/2231533004/diff/20001/ash/pointer_watcher_del...
> > ash/pointer_watcher_delegate_aura.cc:46: if (event->type() !=
> > ui::ET_MOUSE_DRAGGED &&
> > Good catch on drags.
> 
> I think its best to wait until https://codereview.chromium.org/2235363003/
> lands. Otherwise the comments regarding the laser pointer have been addressed
in
> https://codereview.chromium.org/2239743004/.

OK. https://codereview.chromium.org/2235363003/ has LGTM and should land
shortly. Ping me after you've rebased onto it with the list of things you'd like
me to review. Thanks.

[James Cook, 2016-08-12 18:27:56]

On 2016/08/12 18:27:30, James Cook wrote:
> On 2016/08/12 18:09:12, sammiequon wrote:
> > On 2016/08/11 23:51:11, James Cook wrote:
> > > I'll also look in more detail after the CLs are split.
> > > 
> > > Please CC sky on the one for PointerWatchers (just say "CC sky as FYI")
> since
> > > he's also working near that code.
> > > 
> > >
> >
>
https://codereview.chromium.org/2231533004/diff/20001/ash/pointer_watcher_del...
> > > File ash/pointer_watcher_delegate_aura.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2231533004/diff/20001/ash/pointer_watcher_del...
> > > ash/pointer_watcher_delegate_aura.cc:30: if (wants_moves) {
> > > Since want_moves is always true in the map, you could use a set.
> > > 
> > >
> >
>
https://codereview.chromium.org/2231533004/diff/20001/ash/pointer_watcher_del...
> > > ash/pointer_watcher_delegate_aura.cc:46: if (event->type() !=
> > > ui::ET_MOUSE_DRAGGED &&
> > > Good catch on drags.
> > 
> > I think its best to wait until https://codereview.chromium.org/2235363003/
> > lands. Otherwise the comments regarding the laser pointer have been
addressed
> in
> > https://codereview.chromium.org/2239743004/.
> 
> OK. https://codereview.chromium.org/2235363003/ has LGTM and should land
> shortly. Ping me after you've rebased onto it with the list of things you'd
like
> me to review. Thanks.

er, not LGTM yet for this CL, I'll review after rebase

[James Cook, 2016-09-13 04:36:41]

On 2016/08/12 18:27:56, James Cook wrote:
> On 2016/08/12 18:27:30, James Cook wrote:
> > On 2016/08/12 18:09:12, sammiequon wrote:
> > > On 2016/08/11 23:51:11, James Cook wrote:
> > > > I'll also look in more detail after the CLs are split.
> > > > 
> > > > Please CC sky on the one for PointerWatchers (just say "CC sky as FYI")
> > since
> > > > he's also working near that code.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2231533004/diff/20001/ash/pointer_watcher_del...
> > > > File ash/pointer_watcher_delegate_aura.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2231533004/diff/20001/ash/pointer_watcher_del...
> > > > ash/pointer_watcher_delegate_aura.cc:30: if (wants_moves) {
> > > > Since want_moves is always true in the map, you could use a set.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2231533004/diff/20001/ash/pointer_watcher_del...
> > > > ash/pointer_watcher_delegate_aura.cc:46: if (event->type() !=
> > > > ui::ET_MOUSE_DRAGGED &&
> > > > Good catch on drags.
> > > 
> > > I think its best to wait until https://codereview.chromium.org/2235363003/
> > > lands. Otherwise the comments regarding the laser pointer have been
> addressed
> > in
> > > https://codereview.chromium.org/2239743004/.
> > 
> > OK. https://codereview.chromium.org/2235363003/ has LGTM and should land
> > shortly. Ping me after you've rebased onto it with the list of things you'd
> like
> > me to review. Thanks.
> 
> er, not LGTM yet for this CL, I'll review after rebase

Can this CL be closed?