Provide an accessibility tree snapshot

Provide an accessibility tree snapshot

BUG=497258
Provide an accessibility tree snapshot when requested. This CL
contains a frameworks class that is taken from preview release
and will be removed when the public SDK is available.

Depends on https://codereview.chromium.org/1246163003/

Committed: https://crrev.com/650a6fa6cabd2c5a825af031e5f471cc4c7db60c
Cr-Commit-Position: refs/heads/master@{#339968}

[sgurun-gerrit only, 2015-07-21 00:13:48]

sgurun@chromium.org changed reviewers:
+ tedchoc@chromium.org

[sgurun-gerrit only, 2015-07-21 00:15:05]

On 2015/07/21 00:13:48, sgurun wrote:
> mailto:sgurun@chromium.org changed reviewers:
> + mailto:tedchoc@chromium.org

ptal, thanks!

[Ted C, 2015-07-21 00:35:48]

https://codereview.chromium.org/1246823003/diff/1/content/content.gyp
File content/content.gyp (right):

https://codereview.chromium.org/1246823003/diff/1/content/content.gyp#newcode471
content/content.gyp:471: ['android_sdk_version != "M"', {
although temporary, it really shouldn't be in content.

This should be in third_party/android_...something.  And we should depend on it
if not M.

Annoying indirection, but we shouldn't pollute content with this.

https://codereview.chromium.org/1246823003/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ContentView.java
(right):

https://codereview.chromium.org/1246823003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentView.java:88:
public void onProvideVirtualStructure(final ViewStructure structure) {
I would add something like just so we know why it's there.
// @Override[ANDROID-M]

Also, you should add @VisibleForTesting to ensure it does not get trimmed out.

That might make your proguard change unnecessary.

https://codereview.chromium.org/1246823003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentView.java:124:
while (children.hasNext()) {
why not:

for (int i = 0; i < node.children.size(); i++) {
    createVirtualStructure(viewNode.asyncNewChild(i++), node.children.get(i),
node.x, node.y);
}

The iterator and i counter seem unnecessary to me.

https://codereview.chromium.org/1246823003/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java
(right):

https://codereview.chromium.org/1246823003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:329:
RenderCoordinates r =
ContentViewCore.fromWebContents(this).getRenderCoordinates();
ContentViewCore is a layer above this, so we should be passing in the offset and
scroll instead of pulling from CVC.

[sgurun-gerrit only, 2015-07-21 20:22:15]

thanks, done

https://codereview.chromium.org/1246823003/diff/1/content/content.gyp
File content/content.gyp (right):

https://codereview.chromium.org/1246823003/diff/1/content/content.gyp#newcode471
content/content.gyp:471: ['android_sdk_version != "M"', {
On 2015/07/21 00:35:48, Ted C wrote:
> although temporary, it really shouldn't be in content.
> 
> This should be in third_party/android_...something.  And we should depend on
it
> if not M.
> 
> Annoying indirection, but we shouldn't pollute content with this.

Done.

https://codereview.chromium.org/1246823003/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/ContentView.java
(right):

https://codereview.chromium.org/1246823003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentView.java:88:
public void onProvideVirtualStructure(final ViewStructure structure) {
On 2015/07/21 00:35:48, Ted C wrote:
> I would add something like just so we know why it's there.
> // @Override[ANDROID-M]
> 
> Also, you should add @VisibleForTesting to ensure it does not get trimmed out.
> 
> That might make your proguard change unnecessary.

Done.

https://codereview.chromium.org/1246823003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/ContentView.java:124:
while (children.hasNext()) {
On 2015/07/21 00:35:48, Ted C wrote:
> why not:
> 
> for (int i = 0; i < node.children.size(); i++) {
>     createVirtualStructure(viewNode.asyncNewChild(i++), node.children.get(i),
> node.x, node.y);
> }
> 
> The iterator and i counter seem unnecessary to me.

Done.

https://codereview.chromium.org/1246823003/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java
(right):

https://codereview.chromium.org/1246823003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:329:
RenderCoordinates r =
ContentViewCore.fromWebContents(this).getRenderCoordinates();
On 2015/07/21 00:35:48, Ted C wrote:
> ContentViewCore is a layer above this, so we should be passing in the offset
and
> scroll instead of pulling from CVC.

Done.

[Ted C, 2015-07-21 21:21:03]

https://codereview.chromium.org/1246823003/diff/20001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1246823003/diff/20001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2121:
mWebContents.requestAccessibilitySnapshot(callback, 0, 0);
are these values correct?

https://codereview.chromium.org/1246823003/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/1246823003/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_android.cc:480:
contentViewCore->GetScaleFactor(), y_offset, x_scroll);
Hmm...didn't know we used ContentViewCore in this file.

Can these values be accessed out of the native side without passing them in?  I
doubt it, but it is worth investigating.

Also as a follow up, I don't think this method belongs here.  We should pull
this out as a separate file or move it to CVC if this needs to access it.  But
that can be a follow up.

https://codereview.chromium.org/1246823003/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1246823003/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2947:
public void requestAccessibilitySnapshot(final ViewStructure structure) {
Add javadoc for this.  Also, why not call it the same as the View method?  We do
that pretty normally through out this file and we add @see View#<...> as the
comment for the method.

I would add:

// @TargetApi(Build.VERSION_CODES.M)

Since we'll need to add that once we can use the M sdk

[sgurun-gerrit only, 2015-07-21 21:33:49]

https://codereview.chromium.org/1246823003/diff/20001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1246823003/diff/20001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2121:
mWebContents.requestAccessibilitySnapshot(callback, 0, 0);
On 2015/07/21 21:21:03, Ted C wrote:
> are these values correct?

yes, seem so.

https://codereview.chromium.org/1246823003/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/1246823003/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_android.cc:480:
contentViewCore->GetScaleFactor(), y_offset, x_scroll);
On 2015/07/21 21:21:03, Ted C wrote:
> Hmm...didn't know we used ContentViewCore in this file.
> 
> Can these values be accessed out of the native side without passing them in? 
I
> doubt it, but it is worth investigating.
> 
> Also as a follow up, I don't think this method belongs here.  We should pull
> this out as a separate file or move it to CVC if this needs to access it.  But
> that can be a follow up.

Alternatively -which I think is a better alternative-, we can move all the
geometry computation to Java and then this method will only be used to create
the java data structure. It is in my plans to explore it as a code clean up.
adding to the cleanup bug that I have.

https://codereview.chromium.org/1246823003/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1246823003/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2947:
public void requestAccessibilitySnapshot(final ViewStructure structure) {

On 2015/07/21 21:21:03, Ted C wrote:
> Add javadoc for this.  Also, why not call it the same as the View method?  We
do
> that pretty normally through out this file and we add @see View#<...> as the
> comment for the method.

sure we can do this. the method name was not public at the time I added.
> 
> I would add:
> 
> // @TargetApi(Build.VERSION_CODES.M)
> 
> Since we'll need to add that once we can use the M sdk
sure

[sgurun-gerrit only, 2015-07-21 21:55:15]

sgurun@chromium.org changed reviewers:
+ sievers@chromium.org

[sgurun-gerrit only, 2015-07-21 21:55:15]

sievers: please review content/content.gyp

[Ted C, 2015-07-21 21:57:32]

lgtm

[no sievers, 2015-07-21 22:14:06]

gyp changes lgtm stamp

[sgurun-gerrit only, 2015-07-22 15:57:56]

The CQ bit was checked by sgurun@chromium.org

[commit-bot: I haz the power, 2015-07-22 15:58:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1246823003/40001

[commit-bot: I haz the power, 2015-07-22 16:06:42]

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

[commit-bot: I haz the power, 2015-07-22 16:06:42]

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

[sgurun-gerrit only, 2015-07-22 20:05:22]

sgurun@chromium.org changed reviewers:
+ cjhopman@chromium.org

[sgurun-gerrit only, 2015-07-22 20:05:56]

On 2015/07/22 16:06:42, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

cjhopman: please review content/public/android/BUILD.gn and
build/secondary/third_party/android_tools/BUILD.gn
thanks

[cjhopman, 2015-07-22 20:28:16]

lgtm

[sgurun-gerrit only, 2015-07-22 20:28:27]

The CQ bit was checked by sgurun@chromium.org

[sgurun-gerrit only, 2015-07-22 20:28:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1246823003/#ps80001
(title: "fix gn")

[commit-bot: I haz the power, 2015-07-22 20:28:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1246823003/80001

[commit-bot: I haz the power, 2015-07-22 20:59:15]

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

[commit-bot: I haz the power, 2015-07-22 20:59:16]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sgurun-gerrit only, 2015-07-22 21:20:21]

The CQ bit was checked by sgurun@chromium.org

[commit-bot: I haz the power, 2015-07-22 21:21:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1246823003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1246823003/80001

[commit-bot: I haz the power, 2015-07-22 23:00:26]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-07-22 23:00:57]

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