Add custom action support

Add custom action support

- This CL extends chrome.automation API to support custom actions.
- Proposal to extend chrome.automation API:
  https://docs.google.com/document/d/1Gw-2kyCF5AoPotX9V3A0sWtrrSlatVxEdtFOEnSGM_U/edit
- This CL implements custom action support in Android windows. Note that
  we need to land Android side CL as well.

BUG=702659
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2873373005
Cr-Commit-Position: refs/heads/master@{#482187}
Committed: https://chromium.googlesource.com/chromium/src/+/b87aed4000a7e2824a8146891cc733bba653879c

[yawano, 2017-05-11 10:39:40]

yawano@chromium.org changed reviewers:
+ dmazzoni@chromium.org, dtseng@chromium.org

[yawano, 2017-05-11 10:39:41]

This is a WIP CL to add support for custom actions. I'm uploading WIP CL not to
surprise you after implementing everything.

[David Tseng, 2017-05-15 15:20:00]

Here's a quick first pass. I'll rebase on top of everything once I get the
lingering pending changes in. Thanks for working on this!

https://codereview.chromium.org/2873373005/diff/20001/ui/accessibility/ax_enu...
File ui/accessibility/ax_enums.idl (right):

https://codereview.chromium.org/2873373005/diff/20001/ui/accessibility/ax_enu...
ui/accessibility/ax_enums.idl:537: custom_action_ids
nit: .. // arc

https://codereview.chromium.org/2873373005/diff/20001/ui/accessibility/ax_enu...
ui/accessibility/ax_enums.idl:541: custom_action_descriptions
You can avoid doing this if you delimit via commas and add this as a string
attribute. You could then avoid adding string attribute support throughout

 Optional though, as I think this might be useful for other attributes in the
future.

[yawano, 2017-05-15 23:18:16]

Thank you!

https://codereview.chromium.org/2873373005/diff/20001/ui/accessibility/ax_enu...
File ui/accessibility/ax_enums.idl (right):

https://codereview.chromium.org/2873373005/diff/20001/ui/accessibility/ax_enu...
ui/accessibility/ax_enums.idl:541: custom_action_descriptions
I think adding string list attribute is more straightforward. An application can
use arbitrary characters for its description including commas. If we use comma
separated list, we need to workaround for those descriptions.

[David Tseng, 2017-05-16 22:18:55]

https://codereview.chromium.org/2873373005/diff/20001/chrome/common/extension...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/2873373005/diff/20001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:455: // An array of custom actions.
We'll also need a method to perform the custom action.

https://codereview.chromium.org/2873373005/diff/20001/ui/accessibility/ax_enu...
File ui/accessibility/ax_enums.idl (right):

https://codereview.chromium.org/2873373005/diff/20001/ui/accessibility/ax_enu...
ui/accessibility/ax_enums.idl:541: custom_action_descriptions
On 2017/05/15 23:18:16, yawano wrote:
> I think adding string list attribute is more straightforward. An application
can
> use arbitrary characters for its description including commas. If we use comma
> separated list, we need to workaround for those descriptions.

Fair enough. You'll have to add string list attribute support through.

[David Tseng, 2017-05-16 22:33:23]

https://codereview.chromium.org/2873373005/diff/20001/chrome/renderer/extensi...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/2873373005/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:757:
result.SetUndefined();
I would actually NOTREACHED here.

[yawano, 2017-05-31 02:04:48]

Description was changed from

==========
WIP: Add custom action support

BUG=702659
==========

to

==========
WIP: Add custom action support

BUG=702659
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[yawano, 2017-06-02 08:59:37]

Description was changed from

==========
WIP: Add custom action support

BUG=702659
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add custom action support

BUG=702659
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[yawano, 2017-06-02 09:02:39]

This CL is now ready for review. PTAL. Thank you!

FYI: API review for this is running at
https://groups.google.com/a/chromium.org/d/msg/apps-dev/PwxzUPNAmsU/oit2c16wBAAJ

https://codereview.chromium.org/2873373005/diff/20001/chrome/common/extension...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/2873373005/diff/20001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:455: // An array of custom actions.
On 2017/05/16 22:18:55, David Tseng wrote:
> We'll also need a method to perform the custom action.

Done.

https://codereview.chromium.org/2873373005/diff/20001/chrome/renderer/extensi...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/2873373005/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:757:
result.SetUndefined();
On 2017/05/16 22:33:23, David Tseng wrote:
> I would actually NOTREACHED here.

Done.

[dmazzoni, 2017-06-06 03:06:00]

https://codereview.chromium.org/2873373005/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd
(right):

https://codereview.chromium.org/2873373005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd:2519:
Custom actions
This is fine for this change, but I wonder if it'd be better for users to just
have a single Actions menu and then we could automatically populate it
with whatever actions are available on a given object, including things like
Activate, Focus, etc.

https://codereview.chromium.org/2873373005/diff/100001/chrome/common/extensio...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/2873373005/diff/100001/chrome/common/extensio...
chrome/common/extensions/api/automation.idl:720: static void
performCustomAction(long customActionId);
Would it be cleaner if this took a CustomAction as its argument, then
we extract the action id inside the internal bindings?

https://codereview.chromium.org/2873373005/diff/100001/chrome/common/extensio...
File chrome/common/extensions/api/automation_internal.idl (right):

https://codereview.chromium.org/2873373005/diff/100001/chrome/common/extensio...
chrome/common/extensions/api/automation_internal.idl:45: customAction
Sort this, the rest are all alphabetical

https://codereview.chromium.org/2873373005/diff/100001/ui/accessibility/ax_en...
File ui/accessibility/ax_enums.idl (right):

https://codereview.chromium.org/2873373005/diff/100001/ui/accessibility/ax_en...
ui/accessibility/ax_enums.idl:538: custom_action_ids
Add a comment for this. You could mention that they're currently only used
on Android and give a good example of a custom action there.

[dmazzoni, 2017-06-06 03:17:01]

Note: you may need to rebase automation_internal_custom_bindings when this
change lands:

https://chromium-review.googlesource.com/c/524243

[David Tseng, 2017-06-06 18:12:39]

https://codereview.chromium.org/2873373005/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js
(right):

https://codereview.chromium.org/2873373005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:456: }
else {
I would prefer not adding the menu at all.

https://codereview.chromium.org/2873373005/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd
(right):

https://codereview.chromium.org/2873373005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd:2519:
Custom actions
On 2017/06/06 03:06:00, dmazzoni wrote:
> This is fine for this change, but I wonder if it'd be better for users to just
> have a single Actions menu and then we could automatically populate it
> with whatever actions are available on a given object, including things like
> Activate, Focus, etc.
> 

Perhaps for this change, remove the "custom" part. No need for users to care
that the actions are custom.

[yawano, 2017-06-07 09:30:05]

https://codereview.chromium.org/2873373005/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd
(right):

https://codereview.chromium.org/2873373005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd:2519:
Custom actions
On 2017/06/06 18:12:39, David Tseng wrote:
> On 2017/06/06 03:06:00, dmazzoni wrote:
> > This is fine for this change, but I wonder if it'd be better for users to
just
> > have a single Actions menu and then we could automatically populate it
> > with whatever actions are available on a given object, including things like
> > Activate, Focus, etc.
> > 
> 
> Perhaps for this change, remove the "custom" part. No need for users to care
> that the actions are custom.

Okay, let's rename this to "actions". For this CL, only custom actions will be
populated there as we are not sending standard actions at this time. Let's
revisit the UI later how we will show actions in the ChromeVox.

[yawano, 2017-06-08 09:25:29]

https://codereview.chromium.org/2873373005/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js
(right):

https://codereview.chromium.org/2873373005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:456: }
else {
On 2017/06/06 18:12:39, David Tseng wrote:
> I would prefer not adding the menu at all.

Done. Changed not to show the menu if there is no custom action.

https://codereview.chromium.org/2873373005/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd
(right):

https://codereview.chromium.org/2873373005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd:2519:
Custom actions
On 2017/06/07 09:30:05, yawano wrote:
> On 2017/06/06 18:12:39, David Tseng wrote:
> > On 2017/06/06 03:06:00, dmazzoni wrote:
> > > This is fine for this change, but I wonder if it'd be better for users to
> just
> > > have a single Actions menu and then we could automatically populate it
> > > with whatever actions are available on a given object, including things
like
> > > Activate, Focus, etc.
> > > 
> > 
> > Perhaps for this change, remove the "custom" part. No need for users to care
> > that the actions are custom.
> 
> Okay, let's rename this to "actions". For this CL, only custom actions will be
> populated there as we are not sending standard actions at this time. Let's
> revisit the UI later how we will show actions in the ChromeVox.

Done.

https://codereview.chromium.org/2873373005/diff/100001/chrome/common/extensio...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/2873373005/diff/100001/chrome/common/extensio...
chrome/common/extensions/api/automation.idl:720: static void
performCustomAction(long customActionId);
On 2017/06/06 03:06:00, dmazzoni wrote:
> Would it be cleaner if this took a CustomAction as its argument, then
> we extract the action id inside the internal bindings?

Done. Yes, also it should be more extensible.

https://codereview.chromium.org/2873373005/diff/100001/chrome/common/extensio...
File chrome/common/extensions/api/automation_internal.idl (right):

https://codereview.chromium.org/2873373005/diff/100001/chrome/common/extensio...
chrome/common/extensions/api/automation_internal.idl:45: customAction
On 2017/06/06 03:06:00, dmazzoni wrote:
> Sort this, the rest are all alphabetical

Done.

https://codereview.chromium.org/2873373005/diff/100001/ui/accessibility/ax_en...
File ui/accessibility/ax_enums.idl (right):

https://codereview.chromium.org/2873373005/diff/100001/ui/accessibility/ax_en...
ui/accessibility/ax_enums.idl:538: custom_action_ids
On 2017/06/06 03:06:00, dmazzoni wrote:
> Add a comment for this. You could mention that they're currently only used
> on Android and give a good example of a custom action there.

Done.

[David Tseng, 2017-06-08 17:21:59]

https://codereview.chromium.org/2873373005/diff/200001/chrome/common/extensio...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/2873373005/diff/200001/chrome/common/extensio...
chrome/common/extensions/api/automation.idl:722: static void
performCustomAction(CustomAction customAction);
Wait, this is the _public_ api. It doesn't make sense for a caller to provide
both id and description, right?

https://codereview.chromium.org/2873373005/diff/200001/chrome/renderer/resour...
File chrome/renderer/resources/extensions/automation/automation_node.js (right):

https://codereview.chromium.org/2873373005/diff/200001/chrome/renderer/resour...
chrome/renderer/resources/extensions/automation/automation_node.js:420:
this.performAction_('customAction', { customActionID: customAction.id });
customAction.description unused here.

https://codereview.chromium.org/2873373005/diff/200001/components/arc/common/...
File components/arc/common/accessibility_helper.mojom (right):

https://codereview.chromium.org/2873373005/diff/200001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:141: ACTIONS,
This doesn't appear to be used. It might be safe to just rename it to
custom_action_ids.

https://codereview.chromium.org/2873373005/diff/200001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:211:
[MinVersion=3]PerformCustomAction@3(int32 id, int32 custom_action_id);
Considering we might want just one action list in the future, wouldn't it make
sense to try merging this with performAction (i.e. make performAction take an
ActionData)? On the Android end, therer isn't a distinction between a standard
action vs a custom action.

[yawano, 2017-06-09 01:05:14]

https://codereview.chromium.org/2873373005/diff/200001/chrome/common/extensio...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/2873373005/diff/200001/chrome/common/extensio...
chrome/common/extensions/api/automation.idl:722: static void
performCustomAction(CustomAction customAction);
Yes, this is public API. The intention is that it might be cleaner if user only
need to pass CustomAction object which is got by getCustomActions rather than
extracting an id from there. However, if I look other methods in automation.idl,
few method takes object as an argument. Let's make this back to pass custom
action id for now.

It might make sense to pass an object as an argument if we become to support
standard actions in the same way, i.e. if we implement performAction which is
capable of performing custom actions as well.

[yawano, 2017-06-09 08:03:02]

https://codereview.chromium.org/2873373005/diff/200001/components/arc/common/...
File components/arc/common/accessibility_helper.mojom (right):

https://codereview.chromium.org/2873373005/diff/200001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:211:
[MinVersion=3]PerformCustomAction@3(int32 id, int32 custom_action_id);
I would prefer to keep standard action and custom action separated in our mojo
interface. In Android, they are sharing ids with standard actions and custom
actions. But I'm afraid to share them in our mojo interface as well. It would be
difficult to keep maintaining our standard action ids in mojom file are synced
with those in Android framework.

If we need to manage those ids separately, we need to send them in the separated
int list from Android side to Chrome side. We will need to send descriptions of
standard actions as well in the future. Android supports to override standard
actions.

If we manage standard actions and custom actions separately for sending from
Android to Chrome, I think it makes sense to keep them separated in the other
way as well.

WDYT?

[David Tseng, 2017-06-09 18:01:32]

https://codereview.chromium.org/2873373005/diff/200001/components/arc/common/...
File components/arc/common/accessibility_helper.mojom (right):

https://codereview.chromium.org/2873373005/diff/200001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:211:
[MinVersion=3]PerformCustomAction@3(int32 id, int32 custom_action_id);
On 2017/06/09 08:03:01, yawano wrote:
> I would prefer to keep standard action and custom action separated in our mojo
> interface. In Android, they are sharing ids with standard actions and custom
> actions. But I'm afraid to share them in our mojo interface as well. It would
be
> difficult to keep maintaining our standard action ids in mojom file are synced
> with those in Android framework.
> 
> If we need to manage those ids separately, we need to send them in the
separated
> int list from Android side to Chrome side. We will need to send descriptions
of
> standard actions as well in the future. Android supports to override standard
> actions.
> 
> If we manage standard actions and custom actions separately for sending from
> Android to Chrome, I think it makes sense to keep them separated in the other
> way as well.
> 
> WDYT?

I should be more clear. I was thinking:
performAction(ActionData data);

// Passed to |performAction|. A caller must specify either standard_action_id or
custom_action_id.
struct ActionData {
  AccessibilityActionType standard_action_id;

  // Optional.
   int32 custom_id;

  ... (we can add additional args here).
};

[David Tseng, 2017-06-09 18:09:54]

https://codereview.chromium.org/2873373005/diff/200001/chrome/common/extensio...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/2873373005/diff/200001/chrome/common/extensio...
chrome/common/extensions/api/automation.idl:722: static void
performCustomAction(CustomAction customAction);
On 2017/06/09 01:05:14, yawano wrote:
> Yes, this is public API. The intention is that it might be cleaner if user
only
> need to pass CustomAction object which is got by getCustomActions rather than
> extracting an id from there. However, if I look other methods in
automation.idl,
> few method takes object as an argument. Let's make this back to pass custom
> action id for now.
> 
> It might make sense to pass an object as an argument if we become to support
> standard actions in the same way, i.e. if we implement performAction which is
> capable of performing custom actions as well.

If we chose to, I think we should separate the concept of an ActionDescription
vs an ActionResolver.

The former would in my mind describe an action e.g. id, description, is_custom,
etc.

The latter would be used to find an action e.g. by id, by description, by is
custom, etc. Since we're not doing this currently, and unsure if we need to do
this, I think doing it by id is clearer.

Also, note that publically, each standard action gets its own binding (e.g.
node.focus()).

[yawano, 2017-06-12 01:46:52]

https://codereview.chromium.org/2873373005/diff/200001/components/arc/common/...
File components/arc/common/accessibility_helper.mojom (right):

https://codereview.chromium.org/2873373005/diff/200001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:211:
[MinVersion=3]PerformCustomAction@3(int32 id, int32 custom_action_id);
My point was that it would make sense to keep standard actions and custom
actions separated if we treat them separated when sending them from Android to
Chrome side. It would be symmetric.

The pro of adding ActionData struct is that it makes easy to extend the API
later. Adding a field to the struct is much easier than changing the interface
of the method.

After a thought, I will change this to use ActionData. Some standard actions
take arguments, e.g. set text, text selection. We will need to make this
extensible in the future. This should be a good time to address the issue in
advance.

[David Tseng, 2017-06-12 16:55:01]

https://codereview.chromium.org/2873373005/diff/200001/components/arc/common/...
File components/arc/common/accessibility_helper.mojom (right):

https://codereview.chromium.org/2873373005/diff/200001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:211:
[MinVersion=3]PerformCustomAction@3(int32 id, int32 custom_action_id);
On 2017/06/12 01:46:52, yawano wrote:
> My point was that it would make sense to keep standard actions and custom
> actions separated if we treat them separated when sending them from Android to
> Chrome side. It would be symmetric.

Totally understood. However, the automation api packs up actions into one data
structure and has one internal binding for it
(AutomationInternal::performAction). 
> 
> The pro of adding ActionData struct is that it makes easy to extend the API
> later. Adding a field to the struct is much easier than changing the interface
> of the method.
> 
> After a thought, I will change this to use ActionData. Some standard actions
> take arguments, e.g. set text, text selection. We will need to make this
> extensible in the future. This should be a good time to address the issue in
> advance.


Yup; I was thinking of custom actions as a "standard" action with extra
arguments of action id. That could be one way to unify standard and custom --
add a custom action type to the AccessibilityActionType in the mojom as well as
in ax_enums.idl.

[yawano, 2017-06-13 06:58:47]

Changed automation.idl interface to take customActionId. Also changed
PerformAction in mojom interface to take AccessibilityActionData.

PTAL. Thank you!

https://codereview.chromium.org/2873373005/diff/200001/chrome/common/extensio...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/2873373005/diff/200001/chrome/common/extensio...
chrome/common/extensions/api/automation.idl:722: static void
performCustomAction(CustomAction customAction);
On 2017/06/09 18:09:53, David Tseng wrote:
> On 2017/06/09 01:05:14, yawano wrote:
> > Yes, this is public API. The intention is that it might be cleaner if user
> only
> > need to pass CustomAction object which is got by getCustomActions rather
than
> > extracting an id from there. However, if I look other methods in
> automation.idl,
> > few method takes object as an argument. Let's make this back to pass custom
> > action id for now.
> > 
> > It might make sense to pass an object as an argument if we become to support
> > standard actions in the same way, i.e. if we implement performAction which
is
> > capable of performing custom actions as well.
> 
> If we chose to, I think we should separate the concept of an ActionDescription
> vs an ActionResolver.
> 
> The former would in my mind describe an action e.g. id, description,
is_custom,
> etc.
> 
> The latter would be used to find an action e.g. by id, by description, by is
> custom, etc. Since we're not doing this currently, and unsure if we need to do
> this, I think doing it by id is clearer.
> 
> Also, note that publically, each standard action gets its own binding (e.g.
> node.focus()).

Changed performCustomAction to take customActionId.

https://codereview.chromium.org/2873373005/diff/200001/components/arc/common/...
File components/arc/common/accessibility_helper.mojom (right):

https://codereview.chromium.org/2873373005/diff/200001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:141: ACTIONS,
On 2017/06/08 17:21:59, David Tseng wrote:
> This doesn't appear to be used. It might be safe to just rename it to
> custom_action_ids.

Done.

https://codereview.chromium.org/2873373005/diff/200001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:211:
[MinVersion=3]PerformCustomAction@3(int32 id, int32 custom_action_id);
On 2017/06/12 16:55:01, David Tseng wrote:
> On 2017/06/12 01:46:52, yawano wrote:
> > My point was that it would make sense to keep standard actions and custom
> > actions separated if we treat them separated when sending them from Android
to
> > Chrome side. It would be symmetric.
> 
> Totally understood. However, the automation api packs up actions into one data
> structure and has one internal binding for it
> (AutomationInternal::performAction). 
> > 
> > The pro of adding ActionData struct is that it makes easy to extend the API
> > later. Adding a field to the struct is much easier than changing the
interface
> > of the method.
> > 
> > After a thought, I will change this to use ActionData. Some standard actions
> > take arguments, e.g. set text, text selection. We will need to make this
> > extensible in the future. This should be a good time to address the issue in
> > advance.
> 
> 
> Yup; I was thinking of custom actions as a "standard" action with extra
> arguments of action id. That could be one way to unify standard and custom --
> add a custom action type to the AccessibilityActionType in the mojom as well
as
> in ax_enums.idl.

Changed PerformAction to take AccessibilityActionData.

[Luis Héctor Chávez, 2017-06-13 15:00:21]

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

[Luis Héctor Chávez, 2017-06-13 15:00:22]

drive-by

https://codereview.chromium.org/2873373005/diff/260001/components/arc/common/...
File components/arc/common/accessibility_helper.mojom (right):

https://codereview.chromium.org/2873373005/diff/260001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:161:
[MinVersion=3]map<AccessibilityStringListProperty, array<string>>?
string_list_properties;
nit: 80col

https://codereview.chromium.org/2873373005/diff/260001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:220:
[MinVersion=3]PerformAction@3(int32 node_id, AccessibilityActionData
action_data);
nit: 80col

[David Tseng, 2017-06-13 17:02:05]

https://codereview.chromium.org/2873373005/diff/260001/components/arc/common/...
File components/arc/common/accessibility_helper.mojom (right):

https://codereview.chromium.org/2873373005/diff/260001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:220:
[MinVersion=3]PerformAction@3(int32 node_id, AccessibilityActionData
action_data);
nit: Could just make node_id part of the AccessibilityActionData

[yawano, 2017-06-14 01:13:07]

https://codereview.chromium.org/2873373005/diff/260001/components/arc/common/...
File components/arc/common/accessibility_helper.mojom (right):

https://codereview.chromium.org/2873373005/diff/260001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:161:
[MinVersion=3]map<AccessibilityStringListProperty, array<string>>?
string_list_properties;
On 2017/06/13 15:00:22, Luis Héctor Chávez wrote:
> nit: 80col

Done.

https://codereview.chromium.org/2873373005/diff/260001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:220:
[MinVersion=3]PerformAction@3(int32 node_id, AccessibilityActionData
action_data);
On 2017/06/13 17:02:05, David Tseng wrote:
> nit: Could just make node_id part of the AccessibilityActionData

Done.

[yawano, 2017-06-14 01:59:46]

yawano@chromium.org changed reviewers:
+ meacer@chromium.org, rdevlin.cronin@chromium.org

[yawano, 2017-06-14 01:59:48]

PTAL.

meacer@:
- chrome/common/extensions/chrome_extension_messages.h
- components/arc/common/accessibility_helper.mojom
- content/public/common/common_param_traits_macros.h
rdevlin.cronin@:
- chrome/common/extensions/api/automation.idl
- chrome/common/extensions/api/automation_internal.idl
- third_party/closure_compiler/externs/automation.js
dtseng@:
The rest of files.

Thank you!

[yawano, 2017-06-14 04:01:25]

Uploaded a new patch set to remove MinVersion= from struct in
accessibility_helper.mojom.

[Devlin, 2017-06-14 18:23:11]

https://codereview.chromium.org/2873373005/diff/300001/chrome/common/extensio...
File chrome/common/extensions/api/automation_internal.idl (right):

https://codereview.chromium.org/2873373005/diff/300001/chrome/common/extensio...
chrome/common/extensions/api/automation_internal.idl:55: dictionary
PerformCustomActionParams {
Is this used?

https://codereview.chromium.org/2873373005/diff/300001/chrome/renderer/resour...
File chrome/renderer/resources/extensions/automation/automation_node.js (right):

https://codereview.chromium.org/2873373005/diff/300001/chrome/renderer/resour...
chrome/renderer/resources/extensions/automation/automation_node.js:241: var
GetCustomActions = requireNative('automationInternal').GetCustomActions;
add closure annotations?

https://codereview.chromium.org/2873373005/diff/300001/third_party/closure_co...
File third_party/closure_compiler/externs/automation.js (right):

https://codereview.chromium.org/2873373005/diff/300001/third_party/closure_co...
third_party/closure_compiler/externs/automation.js:370: * @typedef {{
were these just generated through the script?

[yawano, 2017-06-15 10:43:21]

https://codereview.chromium.org/2873373005/diff/300001/chrome/common/extensio...
File chrome/common/extensions/api/automation_internal.idl (right):

https://codereview.chromium.org/2873373005/diff/300001/chrome/common/extensio...
chrome/common/extensions/api/automation_internal.idl:55: dictionary
PerformCustomActionParams {
On 2017/06/14 18:23:10, Devlin wrote:
> Is this used?

Yes, this is used. This is used in automation_internal_api.cc.

https://codereview.chromium.org/2873373005/diff/300001/chrome/renderer/resour...
File chrome/renderer/resources/extensions/automation/automation_node.js (right):

https://codereview.chromium.org/2873373005/diff/300001/chrome/renderer/resour...
chrome/renderer/resources/extensions/automation/automation_node.js:241: var
GetCustomActions = requireNative('automationInternal').GetCustomActions;
On 2017/06/14 18:23:10, Devlin wrote:
> add closure annotations?

Done.

https://codereview.chromium.org/2873373005/diff/300001/third_party/closure_co...
File third_party/closure_compiler/externs/automation.js (right):

https://codereview.chromium.org/2873373005/diff/300001/third_party/closure_co...
third_party/closure_compiler/externs/automation.js:370: * @typedef {{
On 2017/06/14 18:23:10, Devlin wrote:
> were these just generated through the script?

Yes, this is generated by tools/json_schema_compiler/compiler.py script.

[David Tseng, 2017-06-15 16:02:24]

lgtm

Just a few few small comments; otherwise, good to go.

https://codereview.chromium.org/2873373005/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc (right):

https://codereview.chromium.org/2873373005/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:397: using
AXIntListProperty = arc::mojom::AccessibilityIntListProperty;
nit: sort alphabetically

https://codereview.chromium.org/2873373005/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:441:
custom_action_ids.size() == custom_action_descriptions.size()) {
Should we CHECK here for all of the three conditions below?
- if AXIntListProperty::CUSTOM_ACTION_IDS exists, then
AXIntListProperty::CUSTOM_ACTION_DESCRIPTIONS should also exist
- the ids should never be empty if set
- the ids and descriptions are the same length

[Devlin, 2017-06-15 22:39:21]

automation.idl, automation_internal.idl,
third_party/closure_compiler/externs/automation.js lgtm

[yawano, 2017-06-16 02:24:02]

https://codereview.chromium.org/2873373005/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc (right):

https://codereview.chromium.org/2873373005/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:397: using
AXIntListProperty = arc::mojom::AccessibilityIntListProperty;
On 2017/06/15 16:02:24, David Tseng wrote:
> nit: sort alphabetically

Done.

https://codereview.chromium.org/2873373005/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:441:
custom_action_ids.size() == custom_action_descriptions.size()) {
On 2017/06/15 16:02:24, David Tseng wrote:
> Should we CHECK here for all of the three conditions below?
> - if AXIntListProperty::CUSTOM_ACTION_IDS exists, then
> AXIntListProperty::CUSTOM_ACTION_DESCRIPTIONS should also exist
> - the ids should never be empty if set
> - the ids and descriptions are the same length

Yes, we should never get inputs which fail those conditions. Moved them to CHECK
or CHECK_EQ.

[yawano, 2017-06-16 02:24:28]

meacer@: ping.

[meacer, 2017-06-16 22:36:56]

Sorry for the delay. Can you please expand the CL description and mention that
this is an extension API change, and perhaps link to the proposal?

https://codereview.chromium.org/2873373005/diff/360001/components/arc/common/...
File components/arc/common/accessibility_helper.mojom (right):

https://codereview.chromium.org/2873373005/diff/360001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:73: CONTEXT_CLICK,
Sorry, not sure where these are coming from. Were they previously missed? They
aren't referred elsewhere in this CL.

[yawano, 2017-06-20 05:26:47]

Description was changed from

==========
Add custom action support

BUG=702659
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add custom action support

- This CL extends chrome.automation API to support custom actions.
- Proposal to extend chrome.automation API:
https://docs.google.com/document/d/1Gw-2kyCF5AoPotX9V3A0sWtrrSlatVxEdtFOEnSGM...
- This CL implements custom action support in Android windows. Note that we need
to land Android side CL as well.

BUG=702659
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[yawano, 2017-06-20 05:28:17]

Description was changed from

==========
Add custom action support

- This CL extends chrome.automation API to support custom actions.
- Proposal to extend chrome.automation API:
https://docs.google.com/document/d/1Gw-2kyCF5AoPotX9V3A0sWtrrSlatVxEdtFOEnSGM...
- This CL implements custom action support in Android windows. Note that we need
to land Android side CL as well.

BUG=702659
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add custom action support

- This CL extends chrome.automation API to support custom actions.
- Proposal to extend chrome.automation API:
 
https://docs.google.com/document/d/1Gw-2kyCF5AoPotX9V3A0sWtrrSlatVxEdtFOEnSGM...
- This CL implements custom action support in Android windows. Note that
  we need to land Android side CL as well.

BUG=702659
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[yawano, 2017-06-20 05:30:49]

https://codereview.chromium.org/2873373005/diff/360001/components/arc/common/...
File components/arc/common/accessibility_helper.mojom (right):

https://codereview.chromium.org/2873373005/diff/360001/components/arc/common/...
components/arc/common/accessibility_helper.mojom:73: CONTEXT_CLICK,
On 2017/06/16 22:36:55, meacer wrote:
> Sorry, not sure where these are coming from. Were they previously missed? They
> aren't referred elsewhere in this CL.

I think they are just missed in the previous CLs. They are coming from standard
actions of AccessibilityActions in Android framework.

https://developer.android.com/reference/android/view/accessibility/Accessibil...

We are only using CLICK and CUSTOM_ACTION in
ArcAccessibilityHelperBridge::OnAction at this time.

[yawano, 2017-06-22 01:09:17]

On 2017/06/20 05:30:49, yawano wrote:
>
https://codereview.chromium.org/2873373005/diff/360001/components/arc/common/...
> File components/arc/common/accessibility_helper.mojom (right):
> 
>
https://codereview.chromium.org/2873373005/diff/360001/components/arc/common/...
> components/arc/common/accessibility_helper.mojom:73: CONTEXT_CLICK,
> On 2017/06/16 22:36:55, meacer wrote:
> > Sorry, not sure where these are coming from. Were they previously missed?
They
> > aren't referred elsewhere in this CL.
> 
> I think they are just missed in the previous CLs. They are coming from
standard
> actions of AccessibilityActions in Android framework.
> 
>
https://developer.android.com/reference/android/view/accessibility/Accessibil...
> 
> We are only using CLICK and CUSTOM_ACTION in
> ArcAccessibilityHelperBridge::OnAction at this time.

meacer@: ping

[meacer, 2017-06-22 01:10:58]

On 2017/06/22 01:09:17, yawano wrote:
> On 2017/06/20 05:30:49, yawano wrote:
> >
>
https://codereview.chromium.org/2873373005/diff/360001/components/arc/common/...
> > File components/arc/common/accessibility_helper.mojom (right):
> > 
> >
>
https://codereview.chromium.org/2873373005/diff/360001/components/arc/common/...
> > components/arc/common/accessibility_helper.mojom:73: CONTEXT_CLICK,
> > On 2017/06/16 22:36:55, meacer wrote:
> > > Sorry, not sure where these are coming from. Were they previously missed?
> They
> > > aren't referred elsewhere in this CL.
> > 
> > I think they are just missed in the previous CLs. They are coming from
> standard
> > actions of AccessibilityActions in Android framework.
> > 
> >
>
https://developer.android.com/reference/android/view/accessibility/Accessibil...
> > 
> > We are only using CLICK and CUSTOM_ACTION in
> > ArcAccessibilityHelperBridge::OnAction at this time.
> 
> meacer@: ping

Lgtm, thanks for the clarification.

[yawano, 2017-06-22 01:20:35]

On 2017/06/22 01:10:58, meacer wrote:
> On 2017/06/22 01:09:17, yawano wrote:
> > On 2017/06/20 05:30:49, yawano wrote:
> > >
> >
>
https://codereview.chromium.org/2873373005/diff/360001/components/arc/common/...
> > > File components/arc/common/accessibility_helper.mojom (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2873373005/diff/360001/components/arc/common/...
> > > components/arc/common/accessibility_helper.mojom:73: CONTEXT_CLICK,
> > > On 2017/06/16 22:36:55, meacer wrote:
> > > > Sorry, not sure where these are coming from. Were they previously
missed?
> > They
> > > > aren't referred elsewhere in this CL.
> > > 
> > > I think they are just missed in the previous CLs. They are coming from
> > standard
> > > actions of AccessibilityActions in Android framework.
> > > 
> > >
> >
>
https://developer.android.com/reference/android/view/accessibility/Accessibil...
> > > 
> > > We are only using CLICK and CUSTOM_ACTION in
> > > ArcAccessibilityHelperBridge::OnAction at this time.
> > 
> > meacer@: ping
> 
> Lgtm, thanks for the clarification.

Thank you!

[yawano, 2017-06-22 01:53:53]

The CQ bit was checked by yawano@chromium.org

[yawano, 2017-06-22 01:53:54]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org,
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2873373005/#ps360001
(title: "Address comments.")

[commit-bot: I haz the power, 2017-06-22 01:54:12]

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, 2017-06-22 01:56:46]

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

[commit-bot: I haz the power, 2017-06-22 01:56:48]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yawano, 2017-06-22 04:32:02]

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

[commit-bot: I haz the power, 2017-06-22 04:32:16]

Dry run: CQ is trying da patch.

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

[yawano, 2017-06-22 04:33:55]

dtseng@: Rebased the CL and had to migrate to DataObjectBuilder. PTAL
automation_internal_custom_bindings.cc. Thank you!

[commit-bot: I haz the power, 2017-06-22 05:47:54]

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

[commit-bot: I haz the power, 2017-06-22 05:47:55]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2017-06-23 18:22:13]

lgtm for changes to automation_internal_custom_bindings.cc
(and lgtm in general)

[David Tseng, 2017-06-23 18:28:52]

lgtm

lgtm in for latest changes

[yawano, 2017-06-26 00:50:21]

On 2017/06/23 18:28:52, David Tseng wrote:
> lgtm
> 
> lgtm in for latest changes

Thank you!

[yawano, 2017-06-26 00:50:29]

The CQ bit was checked by yawano@chromium.org

[yawano, 2017-06-26 00:50:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/2873373005/#ps400001
(title: "Migrate to DataObjectBuilder.")

[commit-bot: I haz the power, 2017-06-26 00:50:47]

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, 2017-06-26 02:55:31]

CQ is committing da patch.

Bot data: {"patchset_id": 400001, "attempt_start_ts": 1498438229078750,
"parent_rev": "78634df3327f642c0334519ff4f1bf290712146b", "commit_rev":
"b87aed4000a7e2824a8146891cc733bba653879c"}

[commit-bot: I haz the power, 2017-06-26 02:55:45]

Description was changed from

==========
Add custom action support

- This CL extends chrome.automation API to support custom actions.
- Proposal to extend chrome.automation API:
 
https://docs.google.com/document/d/1Gw-2kyCF5AoPotX9V3A0sWtrrSlatVxEdtFOEnSGM...
- This CL implements custom action support in Android windows. Note that
  we need to land Android side CL as well.

BUG=702659
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add custom action support

- This CL extends chrome.automation API to support custom actions.
- Proposal to extend chrome.automation API:
 
https://docs.google.com/document/d/1Gw-2kyCF5AoPotX9V3A0sWtrrSlatVxEdtFOEnSGM...
- This CL implements custom action support in Android windows. Note that
  we need to land Android side CL as well.

BUG=702659
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2873373005
Cr-Commit-Position: refs/heads/master@{#482187}
Committed:
https://chromium.googlesource.com/chromium/src/+/b87aed4000a7e2824a8146891cc7...
==========

[commit-bot: I haz the power, 2017-06-26 02:55:47]

Committed patchset #21 (id:400001) as
https://chromium.googlesource.com/chromium/src/+/b87aed4000a7e2824a8146891cc7...