mash: Add autoclick app.

mash: Add autoclick app.

TODO:
1. crbug.com/628665: mus: Allow accessibility clients to generate
some events. So right now this app can't actually click things
after the autoclick delay time is up.
2. crbug.com/608547: Deprecate ui::Event::root_location and replace
it with ui::Event::screen_location. The autoclick ring widget
position is a bit off if the screen location of the event is
different from its location inside window.

BUG=629095
TEST=manual

Committed: https://crrev.com/4b626b97740da69f18ba83cd194189fbbe02b608
Cr-Commit-Position: refs/heads/master@{#418264}

[riajiang, 2016-09-09 17:33:34]

Patchset #1 (id:1) has been deleted

[riajiang, 2016-09-09 17:59:02]

Description was changed from

==========
Add autoclick app.

BUG=
==========

to

==========
mash: Add autoclick app.

BUG=629095
==========

[riajiang, 2016-09-09 18:06:12]

Patchset #2 (id:40001) has been deleted

[riajiang, 2016-09-09 18:06:31]

Patchset #1 (id:20001) has been deleted

[riajiang, 2016-09-09 18:17:46]

Description was changed from

==========
mash: Add autoclick app.

BUG=629095
==========

to

==========
mash: Add autoclick app.

TODO:
1. crbug.com/628665: mus: Allow accessibility clients to generate some events.
So right now this app can't actually click things after the autoclick delay time
is up.
2. crbug.com/608547: Deprecate ui::Event::root_location and replace it with
ui::Event::screen_location. The autoclick ring widget position is a bit off if
the screen location of the event is different from its location inside window.

BUG=629095
TEST=manual
==========

[riajiang, 2016-09-09 18:19:37]

Description was changed from

==========
mash: Add autoclick app.

TODO:
1. crbug.com/628665: mus: Allow accessibility clients to generate some events.
So right now this app can't actually click things after the autoclick delay time
is up.
2. crbug.com/608547: Deprecate ui::Event::root_location and replace it with
ui::Event::screen_location. The autoclick ring widget position is a bit off if
the screen location of the event is different from its location inside window.

BUG=629095
TEST=manual
==========

to

==========
mash: Add autoclick app.

TODO:
1. crbug.com/628665: mus: Allow accessibility clients to generate
some events. So right now this app can't actually click things 
after the autoclick delay time is up.
2. crbug.com/608547: Deprecate ui::Event::root_location and replace 
it with ui::Event::screen_location. The autoclick ring widget 
position is a bit off if the screen location of the event is 
different from its location inside window.

BUG=629095
TEST=manual
==========

[riajiang, 2016-09-09 18:21:31]

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

[riajiang, 2016-09-09 18:21:32]

PTAL. Thanks!

[sky, 2016-09-09 21:42:30]

You should also update the manifest for browser_tests. I recommend adding a
comment to MashPackagedService that indicates the things you need to do when
adding a new service (as it's easy to miss one).

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

https://codereview.chromium.org/2316553003/diff/60001/ash/autoclick/autoclick...
ash/autoclick/autoclick_controller.cc:80: widget_(nullptr),
Remove as this is unnecessary.

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

https://codereview.chromium.org/2316553003/diff/60001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common_delegate.h:25: // Creates a
ring widget at |event_location|. AutoclickControllerCommon
Update comment? Also, it's not clear why you want to change the ownership
around. Can you explain why ownership should move to the delegate and not the
caller?

https://codereview.chromium.org/2316553003/diff/60001/ash/autoclick/mus/BUILD.gn
File ash/autoclick/mus/BUILD.gn (right):

https://codereview.chromium.org/2316553003/diff/60001/ash/autoclick/mus/BUILD...
ash/autoclick/mus/BUILD.gn:37: service("accessibility_autoclick") {
Is there a reason why you want to prefix with accessibility_? Why not
'autoclick'?

https://codereview.chromium.org/2316553003/diff/60001/ash/autoclick/mus/DEPS
File ash/autoclick/mus/DEPS (right):

https://codereview.chromium.org/2316553003/diff/60001/ash/autoclick/mus/DEPS#...
ash/autoclick/mus/DEPS:3: "+ash/autoclick",
Shouldn't this be ash/autoclick/common?

https://codereview.chromium.org/2316553003/diff/60001/ash/autoclick/mus/autoc...
File ash/autoclick/mus/autoclick_application.cc (right):

https://codereview.chromium.org/2316553003/diff/60001/ash/autoclick/mus/autoc...
ash/autoclick/mus/autoclick_application.cc:78:
static_cast<int64_t>(kDefaultAutoclickDelayMs)),
Are you sure you need the cast?

https://codereview.chromium.org/2316553003/diff/60001/ash/autoclick/mus/autoc...
ash/autoclick/mus/autoclick_application.cc:113: widget_->SetOpacity(1.f);
This is the default.

https://codereview.chromium.org/2316553003/diff/60001/ash/autoclick/mus/autoc...
File ash/autoclick/mus/autoclick_application.h (right):

https://codereview.chromium.org/2316553003/diff/60001/ash/autoclick/mus/autoc...
ash/autoclick/mus/autoclick_application.h:69: static const int
kDefaultAutoclickDelayMs;
Unless you need this from a test I would move entirely to the .cc file.

If you do need from a test then prefix the .cc side with:
// static
See style guide for details on this.
And this should should move to the top of the section (constants are before
functions). Again, see style guide for details on ordering of declarations.

https://codereview.chromium.org/2316553003/diff/60001/ash/autoclick/mus/publi...
File ash/autoclick/mus/public/interfaces/autoclick.mojom (right):

https://codereview.chromium.org/2316553003/diff/60001/ash/autoclick/mus/publi...
ash/autoclick/mus/public/interfaces/autoclick.mojom:10: SetAutoclickDelay(int64
delay_in_milliseconds);
int64 here seems like overkill. Do we really want to support a delay > 32 bits?

https://codereview.chromium.org/2316553003/diff/60001/chrome/app/mojo/chrome_...
File chrome/app/mojo/chrome_manifest.json (left):

https://codereview.chromium.org/2316553003/diff/60001/chrome/app/mojo/chrome_...
chrome/app/mojo/chrome_manifest.json:14: "mojo:touch_hud": {
How come you're removing touch_hud? Isn't that still used?

https://codereview.chromium.org/2316553003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/2316553003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:878: if
(ash::Shell::HasInstance()) {
no {}

https://codereview.chromium.org/2316553003/diff/60001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2316553003/diff/60001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:1055: event.type() ==
ui::ET_POINTER_CAPTURE_CHANGED;
I don't think capture_changed is ever sent through here (we have a specific
function for it). Did you encounter it in your testing?

[riajiang, 2016-09-12 16:15:47]

On 2016/09/09 21:42:30, sky wrote:
> You should also update the manifest for browser_tests. I recommend adding a
> comment to MashPackagedService that indicates the things you need to do when
> adding a new service (as it's easy to miss one).

Updated chrome/test/BUILD.gn. 
Added comment.

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

https://codereview.chromium.org/2316553003/diff/60001/ash/autoclick/autoclick...
ash/autoclick/autoclick_controller.cc:80: widget_(nullptr),
On 2016/09/09 21:42:29, sky wrote:
> Remove as this is unnecessary.

Done.

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

https://codereview.chromium.org/2316553003/diff/60001/ash/autoclick/common/au...
ash/autoclick/common/autoclick_controller_common_delegate.h:25: // Creates a
ring widget at |event_location|. AutoclickControllerCommon
On 2016/09/09 21:42:29, sky wrote:
> Update comment? Also, it's not clear why you want to change the ownership
> around. Can you explain why ownership should move to the delegate and not the
> caller?

Done. I think it makes more sense since the app (the delegate in mash mode) is
the one that is responsible for creating and closing the widget and it owns
AutoclickControllerCommon as well?

https://codereview.chromium.org/2316553003/diff/60001/ash/autoclick/mus/BUILD.gn
File ash/autoclick/mus/BUILD.gn (right):

https://codereview.chromium.org/2316553003/diff/60001/ash/autoclick/mus/BUILD...
ash/autoclick/mus/BUILD.gn:37: service("accessibility_autoclick") {
On 2016/09/09 21:42:29, sky wrote:
> Is there a reason why you want to prefix with accessibility_? Why not
> 'autoclick'?

I was thinking to make it clear that this service is part of the accessibility
services (forgot to update touch_hud). What do you think?

https://codereview.chromium.org/2316553003/diff/60001/ash/autoclick/mus/DEPS
File ash/autoclick/mus/DEPS (right):

https://codereview.chromium.org/2316553003/diff/60001/ash/autoclick/mus/DEPS#...
ash/autoclick/mus/DEPS:3: "+ash/autoclick",
On 2016/09/09 21:42:29, sky wrote:
> Shouldn't this be ash/autoclick/common?

Done.

https://codereview.chromium.org/2316553003/diff/60001/ash/autoclick/mus/autoc...
File ash/autoclick/mus/autoclick_application.cc (right):

https://codereview.chromium.org/2316553003/diff/60001/ash/autoclick/mus/autoc...
ash/autoclick/mus/autoclick_application.cc:78:
static_cast<int64_t>(kDefaultAutoclickDelayMs)),
On 2016/09/09 21:42:29, sky wrote:
> Are you sure you need the cast?

Removed.

https://codereview.chromium.org/2316553003/diff/60001/ash/autoclick/mus/autoc...
ash/autoclick/mus/autoclick_application.cc:113: widget_->SetOpacity(1.f);
On 2016/09/09 21:42:29, sky wrote:
> This is the default.

Removed.

https://codereview.chromium.org/2316553003/diff/60001/ash/autoclick/mus/autoc...
File ash/autoclick/mus/autoclick_application.h (right):

https://codereview.chromium.org/2316553003/diff/60001/ash/autoclick/mus/autoc...
ash/autoclick/mus/autoclick_application.h:69: static const int
kDefaultAutoclickDelayMs;
On 2016/09/09 21:42:30, sky wrote:
> Unless you need this from a test I would move entirely to the .cc file.
> 
> If you do need from a test then prefix the .cc side with:
> // static
> See style guide for details on this.
> And this should should move to the top of the section (constants are before
> functions). Again, see style guide for details on ordering of declarations.

Removed from header file.

https://codereview.chromium.org/2316553003/diff/60001/ash/autoclick/mus/publi...
File ash/autoclick/mus/public/interfaces/autoclick.mojom (right):

https://codereview.chromium.org/2316553003/diff/60001/ash/autoclick/mus/publi...
ash/autoclick/mus/public/interfaces/autoclick.mojom:10: SetAutoclickDelay(int64
delay_in_milliseconds);
On 2016/09/09 21:42:30, sky wrote:
> int64 here seems like overkill. Do we really want to support a delay > 32
bits?

int32?

https://codereview.chromium.org/2316553003/diff/60001/chrome/app/mojo/chrome_...
File chrome/app/mojo/chrome_manifest.json (left):

https://codereview.chromium.org/2316553003/diff/60001/chrome/app/mojo/chrome_...
chrome/app/mojo/chrome_manifest.json:14: "mojo:touch_hud": {
On 2016/09/09 21:42:30, sky wrote:
> How come you're removing touch_hud? Isn't that still used?

No I moved that to line 7.

https://codereview.chromium.org/2316553003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/2316553003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:878: if
(ash::Shell::HasInstance()) {
On 2016/09/09 21:42:30, sky wrote:
> no {}

Done.

https://codereview.chromium.org/2316553003/diff/60001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2316553003/diff/60001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:1055: event.type() ==
ui::ET_POINTER_CAPTURE_CHANGED;
On 2016/09/09 21:42:30, sky wrote:
> I don't think capture_changed is ever sent through here (we have a specific
> function for it). Did you encounter it in your testing?

No I don't think I've encountered any capture change events. I added it because
I thought it's similar to mouse wheel events and would go through this function.
Removed.

[sky, 2016-09-12 17:28:09]

LGTM

https://codereview.chromium.org/2316553003/diff/80001/mash/package/mash_packa...
File mash/package/mash_packaged_service.cc (right):

https://codereview.chromium.org/2316553003/diff/80001/mash/package/mash_packa...
mash/package/mash_packaged_service.cc:58: if (name == "mojo:ash")
Please add a comment to see header for details on adding new services.

[riajiang, 2016-09-12 17:36:45]

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

[riajiang, 2016-09-12 17:36:45]

+tsepez@, could you take a look at the mojom? Thanks!

https://codereview.chromium.org/2316553003/diff/80001/mash/package/mash_packa...
File mash/package/mash_packaged_service.cc (right):

https://codereview.chromium.org/2316553003/diff/80001/mash/package/mash_packa...
mash/package/mash_packaged_service.cc:58: if (name == "mojo:ash")
On 2016/09/12 17:28:09, sky wrote:
> Please add a comment to see header for details on adding new services.

Done.

[Tom Sepez, 2016-09-12 17:41:37]

Mojom LGTM.

https://codereview.chromium.org/2316553003/diff/100001/ash/autoclick/mus/publ...
File ash/autoclick/mus/public/interfaces/autoclick.mojom (right):

https://codereview.chromium.org/2316553003/diff/100001/ash/autoclick/mus/publ...
ash/autoclick/mus/public/interfaces/autoclick.mojom:10: SetAutoclickDelay(int32
delay_in_milliseconds);
nit: can this be unsigned, or does something like -1 have some special meaning?

[riajiang, 2016-09-12 18:23:49]

https://codereview.chromium.org/2316553003/diff/100001/ash/autoclick/mus/publ...
File ash/autoclick/mus/public/interfaces/autoclick.mojom (right):

https://codereview.chromium.org/2316553003/diff/100001/ash/autoclick/mus/publ...
ash/autoclick/mus/public/interfaces/autoclick.mojom:10: SetAutoclickDelay(int32
delay_in_milliseconds);
On 2016/09/12 17:41:37, Tom Sepez wrote:
> nit: can this be unsigned, or does something like -1 have some special
meaning? 
> 

Yes it should be uint32.

[riajiang, 2016-09-12 18:23:57]

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

[commit-bot: I haz the power, 2016-09-12 18:24:34]

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-09-12 19:15:22]

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

[commit-bot: I haz the power, 2016-09-12 19:15:23]

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_...)

[sadrul, 2016-09-13 14:55:13]

Some nits. And a request to move the ws change into a separate CL with a
unit-test. With that, lgtm

https://codereview.chromium.org/2316553003/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/2316553003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:878: if
(ash::Shell::HasInstance())
Do you need this null check anymore?

https://codereview.chromium.org/2316553003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:918: if
(ash::Shell::HasInstance()) {
ditto

https://codereview.chromium.org/2316553003/diff/120001/services/ui/ws/window_...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2316553003/diff/120001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:1056: 
Can you move this part out in a separate CL with a unit-test?

[riajiang, 2016-09-13 15:05:49]

Also changed back to MouseEvent to preserve the event target.

https://codereview.chromium.org/2316553003/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/2316553003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:878: if
(ash::Shell::HasInstance())
On 2016/09/13 14:55:13, sadrul wrote:
> Do you need this null check anymore?

Removed.

https://codereview.chromium.org/2316553003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:918: if
(ash::Shell::HasInstance()) {
On 2016/09/13 14:55:13, sadrul wrote:
> ditto

Removed.

https://codereview.chromium.org/2316553003/diff/120001/services/ui/ws/window_...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2316553003/diff/120001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:1056: 
On 2016/09/13 14:55:13, sadrul wrote:
> Can you move this part out in a separate CL with a unit-test?

Sure.

[riajiang, 2016-09-13 15:06:18]

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

[commit-bot: I haz the power, 2016-09-13 15:06:33]

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-09-13 16:05:49]

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

[commit-bot: I haz the power, 2016-09-13 16:05:50]

Dry run: This issue passed the CQ dry run.

[riajiang, 2016-09-13 16:07:39]

The CQ bit was checked by riajiang@chromium.org

[riajiang, 2016-09-13 16:07:40]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
tsepez@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2316553003/#ps140001
(title: "MouseEvent; if etc.")

[commit-bot: I haz the power, 2016-09-13 16:07:53]

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-09-13 16:13:06]

Description was changed from

==========
mash: Add autoclick app.

TODO:
1. crbug.com/628665: mus: Allow accessibility clients to generate
some events. So right now this app can't actually click things 
after the autoclick delay time is up.
2. crbug.com/608547: Deprecate ui::Event::root_location and replace 
it with ui::Event::screen_location. The autoclick ring widget 
position is a bit off if the screen location of the event is 
different from its location inside window.

BUG=629095
TEST=manual
==========

to

==========
mash: Add autoclick app.

TODO:
1. crbug.com/628665: mus: Allow accessibility clients to generate
some events. So right now this app can't actually click things 
after the autoclick delay time is up.
2. crbug.com/608547: Deprecate ui::Event::root_location and replace 
it with ui::Event::screen_location. The autoclick ring widget 
position is a bit off if the screen location of the event is 
different from its location inside window.

BUG=629095
TEST=manual
==========

[commit-bot: I haz the power, 2016-09-13 16:13:08]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2016-09-13 16:15:04]

Description was changed from

==========
mash: Add autoclick app.

TODO:
1. crbug.com/628665: mus: Allow accessibility clients to generate
some events. So right now this app can't actually click things 
after the autoclick delay time is up.
2. crbug.com/608547: Deprecate ui::Event::root_location and replace 
it with ui::Event::screen_location. The autoclick ring widget 
position is a bit off if the screen location of the event is 
different from its location inside window.

BUG=629095
TEST=manual
==========

to

==========
mash: Add autoclick app.

TODO:
1. crbug.com/628665: mus: Allow accessibility clients to generate
some events. So right now this app can't actually click things
after the autoclick delay time is up.
2. crbug.com/608547: Deprecate ui::Event::root_location and replace
it with ui::Event::screen_location. The autoclick ring widget
position is a bit off if the screen location of the event is
different from its location inside window.

BUG=629095
TEST=manual

Committed: https://crrev.com/4b626b97740da69f18ba83cd194189fbbe02b608
Cr-Commit-Position: refs/heads/master@{#418264}
==========

[commit-bot: I haz the power, 2016-09-13 16:15:05]

Patchset 5 (id:??) landed as
https://crrev.com/4b626b97740da69f18ba83cd194189fbbe02b608
Cr-Commit-Position: refs/heads/master@{#418264}