Refactor and send voice interaction structure

1. Refactor accessibility to allow code sharing from
   web_contents_android and ARC.
2. Implement sending voice interaction structure from
   cros to ARC.


BUG=b/35643583
BUG=704684
TEST=
  browser_tests --gtest_filter=\
     ArcVoiceInteractionArcHomeServiceTest.*
TEST=manually verified ax element position in clank
TEST=verified voice interaction structure sent to ARC

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

[Muyuan, 2017-04-12 00:24:07]

muyuanli@chromium.org changed reviewers:
+ dmazzoni@chromium.org, nasko@chromium.org, sgurun@chromium.org

[Muyuan, 2017-04-12 00:24:08]

nasko: Could you please review changes to content/*? This CL results from a
discussion in issue 2768093005

sgurun: Could you please take a look at web_contents_android.cc /
WebContentsImpl.java and see if
the approach that adds an intermediate structure (AXSnapshotAndroid) to share
code between ARC and Clank 
looks good to you.

dmazzoni: Could you please take a look at ui/accessibility/* to see if I'm
moving in the right
direction?

[dmazzoni, 2017-04-12 20:46:56]

I'll have to think about this some more but something
along these lines should work

https://codereview.chromium.org/2808383004/diff/20001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_android_snapshot.h (right):

https://codereview.chromium.org/2808383004/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_android_snapshot.h:51: AX_EXPORT std::string
GetClassName(const AXNode* node);
Why are the following functions all defined in the
header file? Can they just be private methods of
AXSnapshotNodeAndroid, or static helper functions in
the .cc file?

If they have to be public, maybe public static methods
of AXSnapshotNodeAndroid?

https://codereview.chromium.org/2808383004/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_android_snapshot.h:55: AX_EXPORT int
GetIntAttribute(const AXNode* node, ui::AXIntAttribute);
Unused?

[Muyuan, 2017-04-12 23:57:12]

https://codereview.chromium.org/2808383004/diff/20001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_android_snapshot.h (right):

https://codereview.chromium.org/2808383004/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_android_snapshot.h:51: AX_EXPORT std::string
GetClassName(const AXNode* node);
On 2017/04/12 20:46:56, dmazzoni wrote:
> Why are the following functions all defined in the
> header file? Can they just be private methods of
> AXSnapshotNodeAndroid, or static helper functions in
> the .cc file?
> 
> If they have to be public, maybe public static methods
> of AXSnapshotNodeAndroid?
> 

I'm a bit hesitated here.. I don't know if we really want to make them public.
They do have almost 1-1 correspondence in terms of functionality and
implementation with the functions in BrowserAccessibility but on the other hand
I doubt how useful it will be to make them public.

https://codereview.chromium.org/2808383004/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_android_snapshot.h:55: AX_EXPORT int
GetIntAttribute(const AXNode* node, ui::AXIntAttribute);
On 2017/04/12 20:46:56, dmazzoni wrote:
> Unused?

Acknowledged.

[Muyuan, 2017-04-14 00:58:13]

muyuanli@chromium.org changed reviewers:
+ avi@chromium.org

[Muyuan, 2017-04-14 00:58:14]

avi: could you please take a look at contents/*? This is CL's approach is based
on the discussion in issue 2768093005

[sgurun-gerrit only, 2017-04-14 14:35:46]

On 2017/04/14 00:58:14, Muyuan wrote:
> avi: could you please take a look at contents/*? This is CL's approach is
based
> on the discussion in issue 2768093005

I will take a look at this over the weekend or early next week.

[Avi (use Gerrit), 2017-04-14 14:45:05]

This exposes RequestAXTreeSnapshot to the public API, and that is an approach
that I am happy with and would approve. I will cede further review to Dominic,
and will stamp when he's happy.

https://codereview.chromium.org/2808383004/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/2808383004/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:240: void
RequestAXTreeSnapshot(AXTreeSnapshotCallback callback) override;
You'll need to move this to the block of overriding methods, to line 309. (Keep
the same order for the methods as in web_contents.h).

[Muyuan, 2017-04-18 18:24:51]

https://codereview.chromium.org/2808383004/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/2808383004/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:240: void
RequestAXTreeSnapshot(AXTreeSnapshotCallback callback) override;
On 2017/04/14 14:45:05, Avi (concussion NO REVIEWS) wrote:
> You'll need to move this to the block of overriding methods, to line 309.
(Keep
> the same order for the methods as in web_contents.h).

Done.

[Muyuan, 2017-04-20 00:06:22]

https://codereview.chromium.org/2808383004/diff/20001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_android_snapshot.h (right):

https://codereview.chromium.org/2808383004/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_android_snapshot.h:51: AX_EXPORT std::string
GetClassName(const AXNode* node);
On 2017/04/12 23:57:11, Muyuan wrote:
> On 2017/04/12 20:46:56, dmazzoni wrote:
> > Why are the following functions all defined in the
> > header file? Can they just be private methods of
> > AXSnapshotNodeAndroid, or static helper functions in
> > the .cc file?
> > 
> > If they have to be public, maybe public static methods
> > of AXSnapshotNodeAndroid?
> > 
> 
> I'm a bit hesitated here.. I don't know if we really want to make them public.
> They do have almost 1-1 correspondence in terms of functionality and
> implementation with the functions in BrowserAccessibility but on the other
hand
> I doubt how useful it will be to make them public. 
Moved details into private namespace.

https://codereview.chromium.org/2808383004/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_android_snapshot.h:55: AX_EXPORT int
GetIntAttribute(const AXNode* node, ui::AXIntAttribute);
On 2017/04/12 23:57:12, Muyuan wrote:
> On 2017/04/12 20:46:56, dmazzoni wrote:
> > Unused?
> 
> Acknowledged.

Done.

[dmazzoni, 2017-04-20 21:04:08]

OK, getting much better. Please address the feedback below,
then it'd be good to have a test.

It's fine if the test doesn't actually interact with ARC. I'm thinking
a browser test that does a snapshot from web contents and
then creates the VoiceInteractionStructurePtr and asserts that
the data structure is correct.

You can use the tests in this file as a guide:

content/browser/accessibility/snapshot_ax_tree_browsertest.cc

Something like SnapshotAccessibilityTreeFromWebContents
might be good, except that you'll do it using the public
interface on web_contents.h and you'll convert to the
mojo structure too. The web content you test should be
nice and small like that, just a tiny url with a single element.

Thanks!

https://codereview.chromium.org/2808383004/diff/40001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_android_snapshot.h (right):

https://codereview.chromium.org/2808383004/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_android_snapshot.h:24: struct AXSnapshotNodeAndroid
{
This struct name is fine, but make the file name match:
ax_snapshot_node_android.h

https://codereview.chromium.org/2808383004/diff/60001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_android_snapshot.cc (right):

https://codereview.chromium.org/2808383004/diff/60001/ui/accessibility/platfo...
ui/accessibility/platform/ax_android_snapshot.cc:23: std::string
GetClassName(const AXNode* node) {
This code is all copied from browser_accessibility_android.cc, you should share
it.

How about a static function in AXSnapshotNodeAndroid:

const char* AXSnapshotNodeAndroid::AXRoleToAndroidClassName(ui::AXRole role)

https://codereview.chromium.org/2808383004/diff/60001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_android_snapshot.h (right):

https://codereview.chromium.org/2808383004/diff/60001/ui/accessibility/platfo...
ui/accessibility/platform/ax_android_snapshot.h:12: #include
"ui/accessibility/ax_export.h"
Unused? I think you're only using it in the cc file

https://codereview.chromium.org/2808383004/diff/60001/ui/accessibility/platfo...
ui/accessibility/platform/ax_android_snapshot.h:13: #include
"ui/accessibility/ax_tree_update.h"
Forward-declare instead of including this

https://codereview.chromium.org/2808383004/diff/60001/ui/accessibility/platfo...
ui/accessibility/platform/ax_android_snapshot.h:22: static
std::unique_ptr<AXSnapshotNodeAndroid> Create(
Add a comment that this builds a whole tree of AXSnapshotNodeAndroid objects
given an AXTreeUpdate.

https://codereview.chromium.org/2808383004/diff/60001/ui/accessibility/platfo...
ui/accessibility/platform/ax_android_snapshot.h:24: AXSnapshotNodeAndroid();
Should the constructor be private so that the only way you can create
one is with AXSnapshotNodeAndroid::Create?

[sgurun-gerrit only, 2017-04-20 23:25:01]

https://codereview.chromium.org/2808383004/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_android.cc (left):

https://codereview.chromium.org/2808383004/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_android.cc:184:
manager->set_prune_tree_for_screen_reader(false);
where is this code now?

[Muyuan, 2017-04-21 18:45:12]

dmazzoni@:
I'm still working on the tests.. Since the CL involves calculating relative
position of the widgets, it may be a bit more involved than that in
snapshot_ax_tree_browsertest.cc.

https://codereview.chromium.org/2808383004/diff/40001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_android_snapshot.h (right):

https://codereview.chromium.org/2808383004/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_android_snapshot.h:24: struct AXSnapshotNodeAndroid
{
On 2017/04/20 21:04:07, dmazzoni wrote:
> This struct name is fine, but make the file name match:
> ax_snapshot_node_android.h

I can't give it a name that ends in _android.. otherwise it won't be included in
arc build targets.

https://codereview.chromium.org/2808383004/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_android.cc (left):

https://codereview.chromium.org/2808383004/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_android.cc:184:
manager->set_prune_tree_for_screen_reader(false);
On 2017/04/20 23:25:01, sgurun wrote:
> where is this code now?

The code logic is consistent with this path taken. You can compare IsLeaf at L98
and
https://cs.chromium.org/chromium/src/content/browser/accessibility/browser_ac...

https://codereview.chromium.org/2808383004/diff/60001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_android_snapshot.cc (right):

https://codereview.chromium.org/2808383004/diff/60001/ui/accessibility/platfo...
ui/accessibility/platform/ax_android_snapshot.cc:23: std::string
GetClassName(const AXNode* node) {
On 2017/04/20 21:04:07, dmazzoni wrote:
> This code is all copied from browser_accessibility_android.cc, you should
share
> it.
> 
> How about a static function in AXSnapshotNodeAndroid:
> 
> const char* AXSnapshotNodeAndroid::AXRoleToAndroidClassName(ui::AXRole role)

Done.

https://codereview.chromium.org/2808383004/diff/60001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_android_snapshot.h (right):

https://codereview.chromium.org/2808383004/diff/60001/ui/accessibility/platfo...
ui/accessibility/platform/ax_android_snapshot.h:12: #include
"ui/accessibility/ax_export.h"
On 2017/04/20 21:04:07, dmazzoni wrote:
> Unused? I think you're only using it in the cc file

Done.

https://codereview.chromium.org/2808383004/diff/60001/ui/accessibility/platfo...
ui/accessibility/platform/ax_android_snapshot.h:13: #include
"ui/accessibility/ax_tree_update.h"
On 2017/04/20 21:04:07, dmazzoni wrote:
> Forward-declare instead of including this
AXTreeUpdate seems to be a type alias instead of a class itself..

https://codereview.chromium.org/2808383004/diff/60001/ui/accessibility/platfo...
ui/accessibility/platform/ax_android_snapshot.h:22: static
std::unique_ptr<AXSnapshotNodeAndroid> Create(
On 2017/04/20 21:04:08, dmazzoni wrote:
> Add a comment that this builds a whole tree of AXSnapshotNodeAndroid objects
> given an AXTreeUpdate.

Done.

https://codereview.chromium.org/2808383004/diff/60001/ui/accessibility/platfo...
ui/accessibility/platform/ax_android_snapshot.h:24: AXSnapshotNodeAndroid();
On 2017/04/20 21:04:07, dmazzoni wrote:
> Should the constructor be private so that the only way you can create
> one is with AXSnapshotNodeAndroid::Create?

base::MakeUnique in WalkAXTreeDepthFirst will not work then. Should I make
WalkAXTreeDepthFirst a friend or private static?

[dmazzoni, 2017-04-21 21:28:55]

https://codereview.chromium.org/2808383004/diff/40001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_android_snapshot.h (right):

https://codereview.chromium.org/2808383004/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_android_snapshot.h:24: struct AXSnapshotNodeAndroid
{
On 2017/04/21 18:45:11, Muyuan wrote:
> On 2017/04/20 21:04:07, dmazzoni wrote:
> > This struct name is fine, but make the file name match:
> > ax_snapshot_node_android.h
> 
> I can't give it a name that ends in _android.. otherwise it won't be included
in
> arc build targets.

Can't you override that global build rule with a local exception?

I thought that the build rules that exclude "_android" were just a first pass,
but I could be wrong. Seems worth looking into because there may be other
Android files we want to share with Arc.

Or, how about calling it AndroidArc, or AndroidPlatform or something
like that?

[dmazzoni, 2017-04-21 21:31:47]

https://codereview.chromium.org/2808383004/diff/60001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_android_snapshot.h (right):

https://codereview.chromium.org/2808383004/diff/60001/ui/accessibility/platfo...
ui/accessibility/platform/ax_android_snapshot.h:24: AXSnapshotNodeAndroid();
On 2017/04/21 18:45:12, Muyuan wrote:
> On 2017/04/20 21:04:07, dmazzoni wrote:
> > Should the constructor be private so that the only way you can create
> > one is with AXSnapshotNodeAndroid::Create?
> 
> base::MakeUnique in WalkAXTreeDepthFirst will not work then. Should I make
> WalkAXTreeDepthFirst a friend or private static?

private static would make sense to me.

[sgurun-gerrit only, 2017-04-21 21:48:22]

https://codereview.chromium.org/2808383004/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_android.cc (left):

https://codereview.chromium.org/2808383004/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_android.cc:184:
manager->set_prune_tree_for_screen_reader(false);
On 2017/04/21 18:45:11, Muyuan wrote:
> On 2017/04/20 23:25:01, sgurun wrote:
> > where is this code now?
> 
> The code logic is consistent with this path taken. You can compare IsLeaf at
L98
> and
>
https://cs.chromium.org/chromium/src/content/browser/accessibility/browser_ac...

I don't know this part of the code well so leaving it to the judgement of
dmazzoni, others sgtm. I am guessing you tested it well.

[Muyuan, 2017-04-25 04:31:06]

dmazzoni@: A test case is added.

https://codereview.chromium.org/2808383004/diff/60001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_android_snapshot.h (right):

https://codereview.chromium.org/2808383004/diff/60001/ui/accessibility/platfo...
ui/accessibility/platform/ax_android_snapshot.h:24: AXSnapshotNodeAndroid();
On 2017/04/21 21:31:47, dmazzoni wrote:
> On 2017/04/21 18:45:12, Muyuan wrote:
> > On 2017/04/20 21:04:07, dmazzoni wrote:
> > > Should the constructor be private so that the only way you can create
> > > one is with AXSnapshotNodeAndroid::Create?
> > 
> > base::MakeUnique in WalkAXTreeDepthFirst will not work then. Should I make
> > WalkAXTreeDepthFirst a friend or private static?
> 
> private static would make sense to me.

Done.

[dmazzoni, 2017-04-25 17:06:58]

lgtm

Thanks for your hard work on this!

https://codereview.chromium.org/2808383004/diff/100001/ui/accessibility/platf...
File ui/accessibility/platform/ax_snapshot_node_android_platform.h (right):

https://codereview.chromium.org/2808383004/diff/100001/ui/accessibility/platf...
ui/accessibility/platform/ax_snapshot_node_android_platform.h:26: // an
AxTreeUpdate.
nit: AXTreeUpdate (capitalization)

[dmazzoni, 2017-04-25 17:07:29]

One last thing, please update the subject and change
description now that it's not WIP anymore.

[Avi (use Gerrit), 2017-04-25 17:13:31]

Content stamp lgtm

[Muyuan, 2017-04-25 17:42:35]

Description was changed from

==========
WIP: Refactor and send voice interaction structure

1. Refactor accessibility to allow code sharing from
   web_contents_android and ARC.
2. Implement sending voice interaction structure from
   cros to ARC.

NOTE:
This CL is currently not meant to check in. Its
primary purpose is to get input from owners involved.
When things are looking good, I could split the CL
into 2 if necessary.

BUG=b/35643583
BUG=704684
==========

to

==========
1. Refactor accessibility to allow code sharing from
   web_contents_android and ARC.
2. Implement sending voice interaction structure from
   cros to ARC.


BUG=b/35643583
BUG=704684
TEST=
  browser_tests --gtest_filter=\
     ArcVoiceInteractionArcHomeServiceTest.*
TEST=manually verified ax element position in clank
TEST=verified voice interaction structure sent to ARC
==========

[Muyuan, 2017-04-25 17:46:46]

muyuanli@chromium.org changed reviewers:
+ lhchavez@chromium.org, ochang@chromium.org

[Muyuan, 2017-04-25 17:46:47]

lhchavez: Could you please take a look at chrome/browser/* and components/arc/*?
This is mostly a rework of CL:
 https://codereview.chromium.org/2768093005/
but you are more than welcome to comment on other files as well :p

ochang: Could you please take a look at the .mojom files?

[Muyuan, 2017-04-25 17:51:03]

https://codereview.chromium.org/2808383004/diff/100001/ui/accessibility/platf...
File ui/accessibility/platform/ax_snapshot_node_android_platform.h (right):

https://codereview.chromium.org/2808383004/diff/100001/ui/accessibility/platf...
ui/accessibility/platform/ax_snapshot_node_android_platform.h:26: // an
AxTreeUpdate.
On 2017/04/25 17:06:58, dmazzoni wrote:
> nit: AXTreeUpdate (capitalization)

Done.

[Muyuan, 2017-04-25 17:52:29]

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

[commit-bot: I haz the power, 2017-04-25 17:52:52]

Dry run: CQ is trying da patch.

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

[Luis Héctor Chávez, 2017-04-25 18:22:37]

https://codereview.chromium.org/2808383004/diff/100001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc
(right):

https://codereview.chromium.org/2808383004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:69:
base::Bind(&ArcVoiceInteractionArcHomeService::
iwyu: #include "base/bind.h"

https://codereview.chromium.org/2808383004/diff/100001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.h
(right):

https://codereview.chromium.org/2808383004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.h:16:

nit: typically fwd-declarations of this style don't have empty lines around the
namespace:

namespace ui {
struct AXSnapshotNodeAndroid;
}  // namespace ui

example:
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/policy/arc_p...

https://codereview.chromium.org/2808383004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.h:43:
static mojom::VoiceInteractionStructurePtr CreateVoiceInteractionStructure(
it's better to move these private static methods to the anonymous namespace.

If you need to expose one in tests, instead of friending the test you can create
a thin wrapper:

public:
 static mojom::VoiceInteractionStructurePtr
CreateVoiceInteractionStructureForTesting(...);

this one can just call the anonymous namespaced
CreateVoiceInteractionStructure(). See
arc::GetUrlToNavigateOnDeactivateForTesting() for an example.

https://codereview.chromium.org/2808383004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.h:46:
const base::Callback<void(mojom::VoiceInteractionStructurePtr)>& callback,
iwyu: #include "base/callback_forward.h"

https://codereview.chromium.org/2808383004/diff/100001/content/public/browser...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/2808383004/diff/100001/content/public/browser...
content/public/browser/web_contents.h:285: virtual void
RequestAXTreeSnapshot(AXTreeSnapshotCallback callback) = 0;
does it make sense to have this be const AXTreeSnapshotCallback&? would doing
that prevent unnecessary copies?

https://codereview.chromium.org/2808383004/diff/100001/ui/accessibility/platf...
File ui/accessibility/platform/ax_snapshot_node_android_platform.cc (right):

https://codereview.chromium.org/2808383004/diff/100001/ui/accessibility/platf...
ui/accessibility/platform/ax_snapshot_node_android_platform.cc:7: #include
<memory>
nit: you don't need this since you're already including it in the .h

https://codereview.chromium.org/2808383004/diff/100001/ui/accessibility/platf...
ui/accessibility/platform/ax_snapshot_node_android_platform.cc:196:
std::unique_ptr<AXSnapshotNodeAndroid>(new AXSnapshotNodeAndroid);
nit: prefer base::make_unique<AXSnapshotNodeAndroid>();

or add a comment if this is an optimization:
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Prefer...

https://codereview.chromium.org/2808383004/diff/100001/ui/accessibility/platf...
File ui/accessibility/platform/ax_snapshot_node_android_platform.h (right):

https://codereview.chromium.org/2808383004/diff/100001/ui/accessibility/platf...
ui/accessibility/platform/ax_snapshot_node_android_platform.h:39: int32_t color;
iwyu: #include <cstdint>

https://codereview.chromium.org/2808383004/diff/100001/ui/accessibility/platf...
ui/accessibility/platform/ax_snapshot_node_android_platform.h:56: static
std::unique_ptr<AXSnapshotNodeAndroid> WalkAXTreeDepthFirst(
does it make sense to move this private static method to the anonymous namespace
in the .cc?

[commit-bot: I haz the power, 2017-04-25 18:43:24]

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

[commit-bot: I haz the power, 2017-04-25 18:43:25]

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

[Muyuan, 2017-04-25 19:38:14]

https://codereview.chromium.org/2808383004/diff/100001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc
(right):

https://codereview.chromium.org/2808383004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.cc:69:
base::Bind(&ArcVoiceInteractionArcHomeService::
On 2017/04/25 18:22:36, Luis Héctor Chávez wrote:
> iwyu: #include "base/bind.h"

Done.

https://codereview.chromium.org/2808383004/diff/100001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.h
(right):

https://codereview.chromium.org/2808383004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.h:16:

On 2017/04/25 18:22:36, Luis Héctor Chávez wrote:
> nit: typically fwd-declarations of this style don't have empty lines around
the
> namespace:
> 
> namespace ui {
> struct AXSnapshotNodeAndroid;
> }  // namespace ui
> 
> example:
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/policy/arc_p...

Done.

https://codereview.chromium.org/2808383004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.h:43:
static mojom::VoiceInteractionStructurePtr CreateVoiceInteractionStructure(
On 2017/04/25 18:22:36, Luis Héctor Chávez wrote:
> it's better to move these private static methods to the anonymous namespace.
> 
> If you need to expose one in tests, instead of friending the test you can
create
> a thin wrapper:
> 
> public:
>  static mojom::VoiceInteractionStructurePtr
> CreateVoiceInteractionStructureForTesting(...);
> 
> this one can just call the anonymous namespaced
> CreateVoiceInteractionStructure(). See
> arc::GetUrlToNavigateOnDeactivateForTesting() for an example.

Done.

https://codereview.chromium.org/2808383004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_arc_home_service.h:46:
const base::Callback<void(mojom::VoiceInteractionStructurePtr)>& callback,
On 2017/04/25 18:22:36, Luis Héctor Chávez wrote:
> iwyu: #include "base/callback_forward.h"

Done. Moved to anonymous namespace.

https://codereview.chromium.org/2808383004/diff/100001/content/public/browser...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/2808383004/diff/100001/content/public/browser...
content/public/browser/web_contents.h:285: virtual void
RequestAXTreeSnapshot(AXTreeSnapshotCallback callback) = 0;
On 2017/04/25 18:22:36, Luis Héctor Chávez wrote:
> does it make sense to have this be const AXTreeSnapshotCallback&? would doing
> that prevent unnecessary copies?

The API originally looks like that.. gonna check with dmazzoni to see if we
should modify the API signature.

https://codereview.chromium.org/2808383004/diff/100001/ui/accessibility/platf...
File ui/accessibility/platform/ax_snapshot_node_android_platform.cc (right):

https://codereview.chromium.org/2808383004/diff/100001/ui/accessibility/platf...
ui/accessibility/platform/ax_snapshot_node_android_platform.cc:7: #include
<memory>
On 2017/04/25 18:22:36, Luis Héctor Chávez wrote:
> nit: you don't need this since you're already including it in the .h

Done.

https://codereview.chromium.org/2808383004/diff/100001/ui/accessibility/platf...
ui/accessibility/platform/ax_snapshot_node_android_platform.cc:196:
std::unique_ptr<AXSnapshotNodeAndroid>(new AXSnapshotNodeAndroid);
On 2017/04/25 18:22:36, Luis Héctor Chávez wrote:
> nit: prefer base::make_unique<AXSnapshotNodeAndroid>();
> 
> or add a comment if this is an optimization:
>
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Prefer...

OK.. actually it was there to address the comment:
https://codereview.chromium.org/2808383004/#msg13

https://codereview.chromium.org/2808383004/diff/100001/ui/accessibility/platf...
File ui/accessibility/platform/ax_snapshot_node_android_platform.h (right):

https://codereview.chromium.org/2808383004/diff/100001/ui/accessibility/platf...
ui/accessibility/platform/ax_snapshot_node_android_platform.h:39: int32_t color;
On 2017/04/25 18:22:36, Luis Héctor Chávez wrote:
> iwyu: #include <cstdint>

Done.

https://codereview.chromium.org/2808383004/diff/100001/ui/accessibility/platf...
ui/accessibility/platform/ax_snapshot_node_android_platform.h:56: static
std::unique_ptr<AXSnapshotNodeAndroid> WalkAXTreeDepthFirst(
On 2017/04/25 18:22:37, Luis Héctor Chávez wrote:
> does it make sense to move this private static method to the anonymous
namespace
> in the .cc?

It was moved here to get rid of public constructor AXSnapshotNodeAndroid. See:
https://codereview.chromium.org/2808383004/#msg13

[Muyuan, 2017-04-25 19:46:36]

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

[commit-bot: I haz the power, 2017-04-25 19:47:10]

Dry run: CQ is trying da patch.

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

[Luis Héctor Chávez, 2017-04-25 20:05:32]

lgtm with a final nit.

https://codereview.chromium.org/2808383004/diff/100001/ui/accessibility/platf...
File ui/accessibility/platform/ax_snapshot_node_android_platform.cc (right):

https://codereview.chromium.org/2808383004/diff/100001/ui/accessibility/platf...
ui/accessibility/platform/ax_snapshot_node_android_platform.cc:196:
std::unique_ptr<AXSnapshotNodeAndroid>(new AXSnapshotNodeAndroid);
On 2017/04/25 19:38:13, Muyuan wrote:
> On 2017/04/25 18:22:36, Luis Héctor Chávez wrote:
> > nit: prefer base::make_unique<AXSnapshotNodeAndroid>();
> > 
> > or add a comment if this is an optimization:
> >
>
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Prefer...
> 
> OK.. actually it was there to address the comment:
> https://codereview.chromium.org/2808383004/#msg13
> 

As discussed offline, either parens or a comment about this being an
optimization should go here to aid in future refactorings.

https://codereview.chromium.org/2808383004/diff/100001/ui/accessibility/platf...
File ui/accessibility/platform/ax_snapshot_node_android_platform.h (right):

https://codereview.chromium.org/2808383004/diff/100001/ui/accessibility/platf...
ui/accessibility/platform/ax_snapshot_node_android_platform.h:56: static
std::unique_ptr<AXSnapshotNodeAndroid> WalkAXTreeDepthFirst(
On 2017/04/25 19:38:13, Muyuan wrote:
> On 2017/04/25 18:22:37, Luis Héctor Chávez wrote:
> > does it make sense to move this private static method to the anonymous
> namespace
> > in the .cc?
> 
> It was moved here to get rid of public constructor AXSnapshotNodeAndroid. See:
> https://codereview.chromium.org/2808383004/#msg13

Acknowledged.

[commit-bot: I haz the power, 2017-04-25 20:41:00]

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

[commit-bot: I haz the power, 2017-04-25 20:41:01]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Muyuan, 2017-04-25 20:49:42]

https://codereview.chromium.org/2808383004/diff/100001/content/public/browser...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/2808383004/diff/100001/content/public/browser...
content/public/browser/web_contents.h:285: virtual void
RequestAXTreeSnapshot(AXTreeSnapshotCallback callback) = 0;
On 2017/04/25 19:38:13, Muyuan wrote:
> On 2017/04/25 18:22:36, Luis Héctor Chávez wrote:
> > does it make sense to have this be const AXTreeSnapshotCallback&? would
doing
> > that prevent unnecessary copies?
> 
> The API originally looks like that.. gonna check with dmazzoni to see if we
> should modify the API signature. 

After discussing with dmazzoni, it should be OK to change it to const
AXTreeSnapshotCallback&

https://codereview.chromium.org/2808383004/diff/100001/ui/accessibility/platf...
File ui/accessibility/platform/ax_snapshot_node_android_platform.cc (right):

https://codereview.chromium.org/2808383004/diff/100001/ui/accessibility/platf...
ui/accessibility/platform/ax_snapshot_node_android_platform.cc:196:
std::unique_ptr<AXSnapshotNodeAndroid>(new AXSnapshotNodeAndroid);
On 2017/04/25 20:05:32, Luis Héctor Chávez wrote:
> On 2017/04/25 19:38:13, Muyuan wrote:
> > On 2017/04/25 18:22:36, Luis Héctor Chávez wrote:
> > > nit: prefer base::make_unique<AXSnapshotNodeAndroid>();
> > > 
> > > or add a comment if this is an optimization:
> > >
> >
>
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Prefer...
> > 
> > OK.. actually it was there to address the comment:
> > https://codereview.chromium.org/2808383004/#msg13
> > 
> 
> As discussed offline, either parens or a comment about this being an
> optimization should go here to aid in future refactorings.

Done.

[Muyuan, 2017-04-25 20:51:15]

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

[commit-bot: I haz the power, 2017-04-25 20:52:00]

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, 2017-04-25 21:51:30]

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

[commit-bot: I haz the power, 2017-04-25 21:51:32]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Muyuan, 2017-04-26 19:48:46]

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

[commit-bot: I haz the power, 2017-04-26 19:50:06]

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, 2017-04-26 20:27:36]

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

[commit-bot: I haz the power, 2017-04-26 20:27:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Muyuan, 2017-04-27 01:50:54]

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

[commit-bot: I haz the power, 2017-04-27 01:51:41]

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, 2017-04-27 02:28:18]

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

[commit-bot: I haz the power, 2017-04-27 02:28:19]

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

[Muyuan, 2017-04-27 02:42:02]

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

[commit-bot: I haz the power, 2017-04-27 02:42:46]

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, 2017-04-27 04:24:28]

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

[commit-bot: I haz the power, 2017-04-27 04:24:29]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2017-04-27 05:12:22]

still lgtm, thanks!

[Oliver Chang, 2017-04-27 18:26:46]

RS LGTM

[Muyuan, 2017-04-27 18:32:47]

The CQ bit was checked by muyuanli@chromium.org

[Muyuan, 2017-04-27 18:32:48]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
lhchavez@chromium.org
Link to the patchset: https://codereview.chromium.org/2808383004/#ps280001
(title: "fix window build")

[commit-bot: I haz the power, 2017-04-27 18:33:29]

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-04-27 19:12:45]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1493317967558930,
"parent_rev": "c4c12fd03602a3ab702b75cbe804df9c13e95188", "commit_rev":
"c693ba146941bf2baba7f2544f3ccb6349dd211f"}

[commit-bot: I haz the power, 2017-04-27 19:12:54]

Description was changed from

==========
1. Refactor accessibility to allow code sharing from
   web_contents_android and ARC.
2. Implement sending voice interaction structure from
   cros to ARC.


BUG=b/35643583
BUG=704684
TEST=
  browser_tests --gtest_filter=\
     ArcVoiceInteractionArcHomeServiceTest.*
TEST=manually verified ax element position in clank
TEST=verified voice interaction structure sent to ARC
==========

to

==========
1. Refactor accessibility to allow code sharing from
   web_contents_android and ARC.
2. Implement sending voice interaction structure from
   cros to ARC.


BUG=b/35643583
BUG=704684
TEST=
  browser_tests --gtest_filter=\
     ArcVoiceInteractionArcHomeServiceTest.*
TEST=manually verified ax element position in clank
TEST=verified voice interaction structure sent to ARC

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

[commit-bot: I haz the power, 2017-04-27 19:12:56]

Committed patchset #15 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/c693ba146941bf2baba7f2544f3c...