ash: Refactor autoclick common code to ash/autoclick/common.

ash: Refactor autoclick common code to ash/autoclick/common.

BUG=629095
TEST=ash_unittests

Committed: https://crrev.com/fb5d899d60dab2d059772a08640424d25e229358
Cr-Commit-Position: refs/heads/master@{#412668}

[riajiang, 2016-07-28 22:36:28]

Description was changed from

==========
ash: Refactor autoclick common code to be a separate component in
ash/autoclick/common.

BUG=629095
TEST=ash_unittests
==========

to

==========
ash: Refactor autoclick common code to be a separate component in
ash/autoclick/common.

BUG=629095
TEST=ash_unittests
==========

[riajiang, 2016-07-28 22:36:28]

riajiang@chromium.org changed reviewers:
+ sadrul@chromium.org

[sadrul, 2016-07-29 20:13:35]

https://codereview.chromium.org/2193563002/diff/1/ash/autoclick/autoclick_con...
File ash/autoclick/autoclick_controller.cc (right):

https://codereview.chromium.org/2193563002/diff/1/ash/autoclick/autoclick_con...
ash/autoclick/autoclick_controller.cc:121: widget->SetOpacity(1.f);
Why do we need to create a new Widget for each mouse event?

https://codereview.chromium.org/2193563002/diff/1/ash/autoclick/common/BUILD.gn
File ash/autoclick/common/BUILD.gn (right):

https://codereview.chromium.org/2193563002/diff/1/ash/autoclick/common/BUILD....
ash/autoclick/common/BUILD.gn:7: component("autoclick") {
Go with source_set instead of component

https://codereview.chromium.org/2193563002/diff/1/ash/autoclick/common/BUILD....
ash/autoclick/common/BUILD.gn:9: "ash_autoclick_export.h",
You don't need the export if you have it a source_set

https://codereview.chromium.org/2193563002/diff/1/ash/autoclick/common/BUILD....
ash/autoclick/common/BUILD.gn:16: defines = [ "ASH_AUTOCLICK_IMPLEMENTATION" ]
You won't need this either

https://codereview.chromium.org/2193563002/diff/1/ash/autoclick/common/BUILD....
ash/autoclick/common/BUILD.gn:28: ]
Add a DEPS file similar to the one in touch_hud to exclude all of ash.

[riajiang, 2016-08-04 16:51:20]

https://codereview.chromium.org/2193563002/diff/1/ash/autoclick/autoclick_con...
File ash/autoclick/autoclick_controller.cc (right):

https://codereview.chromium.org/2193563002/diff/1/ash/autoclick/autoclick_con...
ash/autoclick/autoclick_controller.cc:121: widget->SetOpacity(1.f);
On 2016/07/29 20:13:35, sadrul wrote:
> Why do we need to create a new Widget for each mouse event?

They were doing this every time StartAnimation is called (which is when a new
mouse event happens and then a new AutoclickRingView is created). I think the
only difference for these widgets is that they have different context and parent
(based on different root_window). But I was not very sure whether all mouse
click locations have the same root window so I kept it like this. 

Update: Changed it to create a new widget only for the first time and reparent
if root_window is different.

https://codereview.chromium.org/2193563002/diff/1/ash/autoclick/common/BUILD.gn
File ash/autoclick/common/BUILD.gn (right):

https://codereview.chromium.org/2193563002/diff/1/ash/autoclick/common/BUILD....
ash/autoclick/common/BUILD.gn:7: component("autoclick") {
On 2016/07/29 20:13:35, sadrul wrote:
> Go with source_set instead of component

Done.

https://codereview.chromium.org/2193563002/diff/1/ash/autoclick/common/BUILD....
ash/autoclick/common/BUILD.gn:9: "ash_autoclick_export.h",
On 2016/07/29 20:13:35, sadrul wrote:
> You don't need the export if you have it a source_set

Done.

https://codereview.chromium.org/2193563002/diff/1/ash/autoclick/common/BUILD....
ash/autoclick/common/BUILD.gn:16: defines = [ "ASH_AUTOCLICK_IMPLEMENTATION" ]
On 2016/07/29 20:13:35, sadrul wrote:
> You won't need this either

Done.

https://codereview.chromium.org/2193563002/diff/1/ash/autoclick/common/BUILD....
ash/autoclick/common/BUILD.gn:28: ]
On 2016/07/29 20:13:35, sadrul wrote:
> Add a DEPS file similar to the one in touch_hud to exclude all of ash.

Done.

[sadrul, 2016-08-09 15:53:16]

https://codereview.chromium.org/2193563002/diff/20001/ash/autoclick/common/au...
File ash/autoclick/common/autoclick_controller_common.h (right):

https://codereview.chromium.org/2193563002/diff/20001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.h:46: void
ResetAutoclickTimer(base::Timer* new_timer);
The common code here should create+own+manage the timer.

https://codereview.chromium.org/2193563002/diff/20001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.h:58: 
This common code should also own the views::Widget. It should decide when to
create the widget, and ask the delegate (ash::AutoclickControllerImpl, in this
case) to create it. So the delegate interface would look something like:

  class Delegate {
    virtual views::Widget* CreateCursorRing(location) = 0;
    virtual void MoveCursorRingWidget(widget, location) = 0;
    virtual void DoAutoclick(location) = 0;
  };

[riajiang, 2016-08-10 17:50:07]

Description was changed from

==========
ash: Refactor autoclick common code to be a separate component in
ash/autoclick/common.

BUG=629095
TEST=ash_unittests
==========

to

==========
ash: Refactor autoclick common code to ash/autoclick/common.

BUG=629095
TEST=ash_unittests
==========

[riajiang, 2016-08-10 20:18:59]

https://codereview.chromium.org/2193563002/diff/20001/ash/autoclick/common/au...
File ash/autoclick/common/autoclick_controller_common.h (right):

https://codereview.chromium.org/2193563002/diff/20001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.h:46: void
ResetAutoclickTimer(base::Timer* new_timer);
On 2016/08/09 15:53:16, sadrul wrote:
> The common code here should create+own+manage the timer.

Done.

https://codereview.chromium.org/2193563002/diff/20001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.h:58: 
On 2016/08/09 15:53:16, sadrul wrote:
> This common code should also own the views::Widget. It should decide when to
> create the widget, and ask the delegate (ash::AutoclickControllerImpl, in this
> case) to create it. So the delegate interface would look something like:
> 
>   class Delegate {
>     virtual views::Widget* CreateCursorRing(location) = 0;
>     virtual void MoveCursorRingWidget(widget, location) = 0;
>     virtual void DoAutoclick(location) = 0;
>   };

Done.

[sadrul, 2016-08-12 15:00:36]

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
File ash/autoclick/common/autoclick_controller_common.cc (right):

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.cc:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
2016

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.cc:63: StopGesture();
Looks like both StopAutoclickTimer() and StopGesture() are always called
together. Merge these into a single Stop()/Cancel()/Reset() function instead?

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.cc:68:
autoclick_ring_handler_->StartGesture(delay_, anchor_location_, widget_);
Since |autoclick_timer_| is running at this point, do you actually want to
StartGesture(), or should you call SetGestureCenter() instead? (because it looks
like the timer running implies the gesture has started)?

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.cc:115: bool
AutoclickControllerCommon::IsModifierKey(const ui::KeyboardCode key_code) {
Move this into a separate function in the anon namespace in this .cc file

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.cc:130: gfx::Point
screen_location = aura::Env::GetInstance()->last_mouse_location();
Use Screen::GetCursorScreenPoint() instead.

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
File ash/autoclick/common/autoclick_controller_common.h (right):

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
2016

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.h:37: // location.
* Creates a ring widget at |event_location|.

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.h:39: const gfx::Point&
event_location) = 0;
Document the ownership of the returned Widget (e.g. does the delegate retain
ownership, or does this AutoclickControllerCommon takes ownership of the created
widget?

From reading the code, it looks like AutoclickControllerCommon is the one that
takes ownership. In that case, return a std::unique_ptr<Widget> from here to
make that obvious.

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.h:42: // the |widget| if
necessary.
* Moves |widget| to |event_location|

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.h:47: virtual void
DoAutoclick(const gfx::Point& event_location,
Generates a click with |mouse_event_flags| at |event_location|.

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.h:73: void
GetAutoclickRingWidget(const gfx::Point& mouse_location);
Call this UpdateRingWidget(). (Get* implies the function is going to return
something)

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.h:79: views::Widget* widget_;
Make this a std::unique_ptr<> if this owns the widget.

[riajiang, 2016-08-12 16:31:39]

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
File ash/autoclick/common/autoclick_controller_common.cc (right):

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.cc:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2016/08/12 15:00:35, sadrul wrote:
> 2016

Done.

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.cc:63: StopGesture();
On 2016/08/12 15:00:35, sadrul wrote:
> Looks like both StopAutoclickTimer() and StopGesture() are always called
> together. Merge these into a single Stop()/Cancel()/Reset() function instead?

Only StopAutoclickTimer() is called in AutoclickControllerImpl::SetEnabled() and
only StopGesture() is called in AutoclickControllerImpl::OnWindowDestroying().

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.cc:68:
autoclick_ring_handler_->StartGesture(delay_, anchor_location_, widget_);
On 2016/08/12 15:00:35, sadrul wrote:
> Since |autoclick_timer_| is running at this point, do you actually want to
> StartGesture(), or should you call SetGestureCenter() instead? (because it
looks
> like the timer running implies the gesture has started)?

StartGesture() will stop the previous gesture/ ring widget animation first and
then start animation at the new location. SetGestureCenter() only updates the
location and won't start animation.

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.cc:115: bool
AutoclickControllerCommon::IsModifierKey(const ui::KeyboardCode key_code) {
On 2016/08/12 15:00:35, sadrul wrote:
> Move this into a separate function in the anon namespace in this .cc file

But it's also being used in AutoclickControllerImpl?

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.cc:130: gfx::Point
screen_location = aura::Env::GetInstance()->last_mouse_location();
On 2016/08/12 15:00:35, sadrul wrote:
> Use Screen::GetCursorScreenPoint() instead.

Done.

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
File ash/autoclick/common/autoclick_controller_common.h (right):

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2016/08/12 15:00:35, sadrul wrote:
> 2016

Done.

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.h:37: // location.
On 2016/08/12 15:00:35, sadrul wrote:
> * Creates a ring widget at |event_location|.

Done.

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.h:39: const gfx::Point&
event_location) = 0;
On 2016/08/12 15:00:36, sadrul wrote:
> Document the ownership of the returned Widget (e.g. does the delegate retain
> ownership, or does this AutoclickControllerCommon takes ownership of the
created
> widget?
> 
> From reading the code, it looks like AutoclickControllerCommon is the one that
> takes ownership. In that case, return a std::unique_ptr<Widget> from here to
> make that obvious.

Done.

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.h:42: // the |widget| if
necessary.
On 2016/08/12 15:00:35, sadrul wrote:
> * Moves |widget| to |event_location|

Done.

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.h:47: virtual void
DoAutoclick(const gfx::Point& event_location,
On 2016/08/12 15:00:36, sadrul wrote:
> Generates a click with |mouse_event_flags| at |event_location|.

Done.

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.h:73: void
GetAutoclickRingWidget(const gfx::Point& mouse_location);
On 2016/08/12 15:00:35, sadrul wrote:
> Call this UpdateRingWidget(). (Get* implies the function is going to return
> something)

Done.

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.h:79: views::Widget* widget_;
On 2016/08/12 15:00:35, sadrul wrote:
> Make this a std::unique_ptr<> if this owns the widget.

Done.

[sadrul, 2016-08-15 15:25:10]

Very close. I think we can make the interface a little bit nicer. Other than
that, things look good!

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
File ash/autoclick/common/autoclick_controller_common.cc (right):

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.cc:63: StopGesture();
On 2016/08/12 16:31:38, riajiang wrote:
> On 2016/08/12 15:00:35, sadrul wrote:
> > Looks like both StopAutoclickTimer() and StopGesture() are always called
> > together. Merge these into a single Stop()/Cancel()/Reset() function
instead?
> 
> Only StopAutoclickTimer() is called in AutoclickControllerImpl::SetEnabled()
and
> only StopGesture() is called in AutoclickControllerImpl::OnWindowDestroying().

I think if you can call both StopTimer and StopGesture from both places.

https://codereview.chromium.org/2193563002/diff/60001/ash/autoclick/common/au...
File ash/autoclick/common/autoclick_controller_common.h (right):

https://codereview.chromium.org/2193563002/diff/60001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.h:49: 
Let's add OnAutoClickCanceled() here too, which will be called form the
Cancel()/Reset() function. AutoclickControllerImpl can SetTapDownTarget(nullptr)
from the override. This way, all of the logic of when the autoclick is canceled
remains in AutoclickControllerCommon. You can then remove all the explicit calls
to SetTapDownTarget(nullptr) from the On*Event overrides (and thus move
IsModifierKey() into the .cc file)

[riajiang, 2016-08-15 17:16:23]

riajiang@chromium.org changed reviewers:
+ sky@chromium.org

[riajiang, 2016-08-15 17:16:24]

+sky@, could you take a look at ash/ and chrome/browser? Thanks!

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
File ash/autoclick/common/autoclick_controller_common.cc (right):

https://codereview.chromium.org/2193563002/diff/40001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.cc:63: StopGesture();
On 2016/08/15 15:25:10, sadrul wrote:
> On 2016/08/12 16:31:38, riajiang wrote:
> > On 2016/08/12 15:00:35, sadrul wrote:
> > > Looks like both StopAutoclickTimer() and StopGesture() are always called
> > > together. Merge these into a single Stop()/Cancel()/Reset() function
> instead?
> > 
> > Only StopAutoclickTimer() is called in AutoclickControllerImpl::SetEnabled()
> and
> > only StopGesture() is called in
AutoclickControllerImpl::OnWindowDestroying().
> 
> I think if you can call both StopTimer and StopGesture from both places.

Done.

https://codereview.chromium.org/2193563002/diff/60001/ash/autoclick/common/au...
File ash/autoclick/common/autoclick_controller_common.h (right):

https://codereview.chromium.org/2193563002/diff/60001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.h:49: 
On 2016/08/15 15:25:10, sadrul wrote:
> Let's add OnAutoClickCanceled() here too, which will be called form the
> Cancel()/Reset() function. AutoclickControllerImpl can
SetTapDownTarget(nullptr)
> from the override. This way, all of the logic of when the autoclick is
canceled
> remains in AutoclickControllerCommon. You can then remove all the explicit
calls
> to SetTapDownTarget(nullptr) from the On*Event overrides (and thus move
> IsModifierKey() into the .cc file)

Makes sense! Done.

[sadrul, 2016-08-15 17:23:04]

lgtm

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/autoclick...
File ash/autoclick/autoclick_controller.cc (right):

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/autoclick...
ash/autoclick/autoclick_controller.cc:64: void SetTapDownTarget(aura::Window*
target);
Non-override before overrides.

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/common/au...
File ash/autoclick/common/autoclick_controller_common.cc (right):

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.cc:136:
delegate_->UpdateAutoclickRingWidget(widget_.get(), mouse_location);
Use {}

[sky, 2016-08-15 19:27:00]

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/common/au...
File ash/autoclick/common/autoclick_controller_common.cc (right):

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.cc:46: if (event.type() ==
ui::ET_MOUSE_MOVED &&
It seems like this code wants to observe drag events. If that's the case the
pointerwatcher code currently doesn't and will need to be updated.

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.cc:50:
::wm::ConvertPointToScreen(static_cast<aura::Window*>(event.target()),
Have you thought of how you will wire this up for mus? Will the target be
non-null and an aura window?

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/common/au...
File ash/autoclick/common/autoclick_controller_common.h (right):

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.h:28: // Autoclick controller
common code for ash and mus.
Please update this code to better document what autoclick is, and what this code
does.

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.h:31: class Delegate {
It's preferable to put delegates/observers into their own header files to
minimize import. See chromium style guide for more details.

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.h:61: void
HandleTouchEvent(const ui::TouchEvent& event);
Can touch/gesture/scroll be removed and instead call cancel?

[riajiang, 2016-08-16 00:44:24]

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/autoclick...
File ash/autoclick/autoclick_controller.cc (right):

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/autoclick...
ash/autoclick/autoclick_controller.cc:64: void SetTapDownTarget(aura::Window*
target);
On 2016/08/15 17:23:04, sadrul wrote:
> Non-override before overrides.

Done.

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/common/au...
File ash/autoclick/common/autoclick_controller_common.cc (right):

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.cc:46: if (event.type() ==
ui::ET_MOUSE_MOVED &&
On 2016/08/15 19:27:00, sky wrote:
> It seems like this code wants to observe drag events. If that's the case the
> pointerwatcher code currently doesn't and will need to be updated.

Why is it also observing drag events? I thought it's just mouse move here?
But mousewheel events can't convert to a PointerEvent, maybe something like this
https://cs.chromium.org/chromium/src/ui/views/pointer_watcher.h?dr=CSs&q=poin...
in mus?

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.cc:50:
::wm::ConvertPointToScreen(static_cast<aura::Window*>(event.target()),
On 2016/08/15 19:27:00, sky wrote:
> Have you thought of how you will wire this up for mus? Will the target be
> non-null and an aura window?

True... just found a comment that says event.target() is always null in mus,
didn't thought of this. Can we use the target in OnPointerEventObserved(event,
location_in_screen, target)? (it did mention if events are in windows not owned
by the current process, target will be null tho)

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.cc:136:
delegate_->UpdateAutoclickRingWidget(widget_.get(), mouse_location);
On 2016/08/15 17:23:04, sadrul wrote:
> Use {}

Done.

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/common/au...
File ash/autoclick/common/autoclick_controller_common.h (right):

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.h:28: // Autoclick controller
common code for ash and mus.
On 2016/08/15 19:27:00, sky wrote:
> Please update this code to better document what autoclick is, and what this
code
> does.

Done.

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.h:31: class Delegate {
On 2016/08/15 19:27:00, sky wrote:
> It's preferable to put delegates/observers into their own header files to
> minimize import. See chromium style guide for more details.

Done.

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.h:61: void
HandleTouchEvent(const ui::TouchEvent& event);
On 2016/08/15 19:27:00, sky wrote:
> Can touch/gesture/scroll be removed and instead call cancel?

Done.

[sadrul, 2016-08-16 00:47:35]

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/common/au...
File ash/autoclick/common/autoclick_controller_common.cc (right):

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.cc:50:
::wm::ConvertPointToScreen(static_cast<aura::Window*>(event.target()),
On 2016/08/16 00:44:24, riajiang wrote:
> On 2016/08/15 19:27:00, sky wrote:
> > Have you thought of how you will wire this up for mus? Will the target be
> > non-null and an aura window?
> 
> True... just found a comment that says event.target() is always null in mus,
> didn't thought of this. Can we use the target in OnPointerEventObserved(event,
> location_in_screen, target)? (it did mention if events are in windows not
owned
> by the current process, target will be null tho)

Ideally, we would have crbug.com/608547 fixed, and can just use
|LocatedEvent::screen_location()|. (maybe leave a TODO and a reference to the
bug?)

[sky, 2016-08-16 02:40:01]

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/common/au...
File ash/autoclick/common/autoclick_controller_common.cc (right):

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.cc:46: if (event.type() ==
ui::ET_MOUSE_MOVED &&
On 2016/08/16 00:44:24, riajiang wrote:
> On 2016/08/15 19:27:00, sky wrote:
> > It seems like this code wants to observe drag events. If that's the case the
> > pointerwatcher code currently doesn't and will need to be updated.
> 
> Why is it also observing drag events? I thought it's just mouse move here?
> But mousewheel events can't convert to a PointerEvent, maybe something like
this
>
https://cs.chromium.org/chromium/src/ui/views/pointer_watcher.h?dr=CSs&q=poin...
> in mus?

You are right, I missed that. Sorry.

[riajiang, 2016-08-17 15:23:48]

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/common/au...
File ash/autoclick/common/autoclick_controller_common.cc (right):

https://codereview.chromium.org/2193563002/diff/80001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common.cc:50:
::wm::ConvertPointToScreen(static_cast<aura::Window*>(event.target()),
On 2016/08/16 00:47:35, sadrul wrote:
> On 2016/08/16 00:44:24, riajiang wrote:
> > On 2016/08/15 19:27:00, sky wrote:
> > > Have you thought of how you will wire this up for mus? Will the target be
> > > non-null and an aura window?
> > 
> > True... just found a comment that says event.target() is always null in mus,
> > didn't thought of this. Can we use the target in
OnPointerEventObserved(event,
> > location_in_screen, target)? (it did mention if events are in windows not
> owned
> > by the current process, target will be null tho)
> 
> Ideally, we would have crbug.com/608547 fixed, and can just use
> |LocatedEvent::screen_location()|. (maybe leave a TODO and a reference to the
> bug?)

Done.

[sky, 2016-08-17 15:38:18]

LGTM

[riajiang, 2016-08-17 15:43:51]

The CQ bit was checked by riajiang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 15:44:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 16:25:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-17 16:25:48]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[riajiang, 2016-08-17 19:59:11]

The CQ bit was checked by riajiang@chromium.org

[riajiang, 2016-08-17 19:59:12]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2193563002/#ps160001
(title: "int")

[commit-bot: I haz the power, 2016-08-17 19:59:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 20:07:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-17 20:07:52]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[riajiang, 2016-08-17 20:37:10]

The CQ bit was checked by riajiang@chromium.org

[riajiang, 2016-08-17 20:37:10]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2193563002/#ps180001
(title: "deps")

[commit-bot: I haz the power, 2016-08-17 20:37:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 22:20:01]

Description was changed from

==========
ash: Refactor autoclick common code to ash/autoclick/common.

BUG=629095
TEST=ash_unittests
==========

to

==========
ash: Refactor autoclick common code to ash/autoclick/common.

BUG=629095
TEST=ash_unittests
==========

[commit-bot: I haz the power, 2016-08-17 22:20:02]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-08-17 22:24:32]

Description was changed from

==========
ash: Refactor autoclick common code to ash/autoclick/common.

BUG=629095
TEST=ash_unittests
==========

to

==========
ash: Refactor autoclick common code to ash/autoclick/common.

BUG=629095
TEST=ash_unittests

Committed: https://crrev.com/fb5d899d60dab2d059772a08640424d25e229358
Cr-Commit-Position: refs/heads/master@{#412668}
==========

[commit-bot: I haz the power, 2016-08-17 22:24:33]

Patchset 10 (id:??) landed as
https://crrev.com/fb5d899d60dab2d059772a08640424d25e229358
Cr-Commit-Position: refs/heads/master@{#412668}