Fix a nasty scroll bug for Chrome Now-on-tap feature. Also combine the

Fix a nasty scroll bug for Chrome Now-on-tap feature. Also combine the
code paths for Android Webview and Chrome.

Until now webview and chrome were using separate code paths for copying the snapshot tree. We are now combining both to use the same code. Further moving the
tests from android webview to content layer to add coverage for both chrome
and webview.

BUG=603746
Committed: https://crrev.com/98495a3d4de6f2be14a71f675212552b63c71def
Cr-Commit-Position: refs/heads/master@{#389374}

[sgurun-gerrit only, 2016-04-20 22:52:40]

Description was changed from

==========
Fix a nasty scroll bug for Chrome Now-on-tap feature. Also combine the
code paths for Android Webview and Chrome.

Until now webview and chrome were using separate code paths for copying the
snapshot tree. We are now combining both to use the same code. Further moving
the
tests from android webview to content layer to add coverage for both chrome
and webview.

BUG=603746
==========

to

==========
Fix a nasty scroll bug for Chrome Now-on-tap feature. Also combine the
code paths for Android Webview and Chrome.

Until now webview and chrome were using separate code paths for copying the
snapshot tree. We are now combining both to use the same code. Further moving
the
tests from android webview to content layer to add coverage for both chrome
and webview.

BUG=603746
==========

[sgurun-gerrit only, 2016-04-20 22:52:40]

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

[sgurun-gerrit only, 2016-04-20 22:53:24]

On 2016/04/20 22:52:40, sgurun wrote:
> mailto:sgurun@chromium.org changed reviewers:
> + mailto:dmazzoni@chromium.org

Dmazzoni, PTAL. (this is fixing a releaseblock bug for 51)

I will ask owner's review after addressing your comments.

[dmazzoni, 2016-04-21 16:23:21]

Refactoring so they use the same logic sounds great!

Are there any specific changes to the logic other than moving the code around or
is this a pure refactoring?

[sgurun-gerrit only, 2016-04-21 17:08:46]

On 2016/04/21 16:23:21, dmazzoni wrote:
> Refactoring so they use the same logic sounds great!
> 
> Are there any specific changes to the logic other than moving the code around
or
> is this a pure refactoring?

Not a pure refactoring. Specifically, I dropped the logic in
web_contents_android.cc where we get the scale_factor from contentviewcore and
calculate boundaries. Instead, I use the same logic that exists in
BrowserAccessibilityManager.setAccessibilityNodeInfoLocation. Note that since
Assist requires relative positions, I only need to calculate that.

I will do one more line of cleanup and upload a patch right away. then I
appreciate if you especially look at webcontentsandroid and contentviewcore
changes. The rest, I think owners can take a look.

thanks!

[dmazzoni, 2016-04-21 17:35:32]

lgtm

Thanks for clarifying! Just a couple of suggestions.

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

https://codereview.chromium.org/1907703002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_android.cc:113: gfx::Rect parent_rect
= node->GetParent()->GetLocalBoundsRect();
Since you're walking the tree depth-first, it'd be more efficient to pass the
parent's rect (or offset from origin) in as a parameter.

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

https://codereview.chromium.org/1907703002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3121:
viewNode.asyncNewChild(i), node.children.get(i), ignoreScrollOffset);
It's slightly confusing to me to pass ignoreScrollOffset, since it only applies
to the root node. Maybe pass true when calling recursively?

[sgurun-gerrit only, 2016-04-21 21:02:58]

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

https://codereview.chromium.org/1907703002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_android.cc:113: gfx::Rect parent_rect
= node->GetParent()->GetLocalBoundsRect();
On 2016/04/21 17:35:31, dmazzoni wrote:
> Since you're walking the tree depth-first, it'd be more efficient to pass the
> parent's rect (or offset from origin) in as a parameter.

Done.

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

https://codereview.chromium.org/1907703002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3121:
viewNode.asyncNewChild(i), node.children.get(i), ignoreScrollOffset);
On 2016/04/21 17:35:32, dmazzoni wrote:
> It's slightly confusing to me to pass ignoreScrollOffset, since it only
applies
> to the root node. Maybe pass true when calling recursively?

Done.

[sgurun-gerrit only, 2016-04-21 21:04:45]

sgurun@chromium.org changed reviewers:
+ michaelbai@google.com

[sgurun-gerrit only, 2016-04-21 21:05:02]

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

[sgurun-gerrit only, 2016-04-22 19:02:31]

On 2016/04/21 21:02:58, sgurun wrote:
>
https://codereview.chromium.org/1907703002/diff/40001/content/browser/web_con...
> File content/browser/web_contents/web_contents_android.cc (right):
> 
>
https://codereview.chromium.org/1907703002/diff/40001/content/browser/web_con...
> content/browser/web_contents/web_contents_android.cc:113: gfx::Rect
parent_rect
> = node->GetParent()->GetLocalBoundsRect();
> On 2016/04/21 17:35:31, dmazzoni wrote:
> > Since you're walking the tree depth-first, it'd be more efficient to pass
the
> > parent's rect (or offset from origin) in as a parameter.
> 
> Done.
> 
>
https://codereview.chromium.org/1907703002/diff/40001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> (right):
> 
>
https://codereview.chromium.org/1907703002/diff/40001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3121:
> viewNode.asyncNewChild(i), node.children.get(i), ignoreScrollOffset);
> On 2016/04/21 17:35:32, dmazzoni wrote:
> > It's slightly confusing to me to pass ignoreScrollOffset, since it only
> applies
> > to the root node. Maybe pass true when calling recursively?
> 
> Done.

PTAL. 
tao for android_webview/ 
ted for content/ and chrome/

[michaelbai, 2016-04-22 20:53:50]

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

[michaelbai, 2016-04-22 20:53:51]

android_webview LGTM

https://codereview.chromium.org/1907703002/diff/80001/content/public/android/...
File content/public/android/BUILD.gn (right):

https://codereview.chromium.org/1907703002/diff/80001/content/public/android/...
content/public/android/BUILD.gn:394:
"javatests/src/org/chromium/content/browser/webcontents/AccessibilitySnapshotTest.java",
Wrong order, did you get warning?

[sgurun-gerrit only, 2016-04-22 21:11:13]

https://codereview.chromium.org/1907703002/diff/80001/content/public/android/...
File content/public/android/BUILD.gn (right):

https://codereview.chromium.org/1907703002/diff/80001/content/public/android/...
content/public/android/BUILD.gn:394:
"javatests/src/org/chromium/content/browser/webcontents/AccessibilitySnapshotTest.java",
On 2016/04/22 20:53:51, michaelbai wrote:
> Wrong order, did you get warning?

good catch! I was actually relying on presubmit tests to catch all these. No I
did not get a warning.

[sgurun-gerrit only, 2016-04-22 21:11:45]

On 2016/04/22 21:11:13, sgurun wrote:
>
https://codereview.chromium.org/1907703002/diff/80001/content/public/android/...
> File content/public/android/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1907703002/diff/80001/content/public/android/...
> content/public/android/BUILD.gn:394:
>
"javatests/src/org/chromium/content/browser/webcontents/AccessibilitySnapshotTest.java",
> On 2016/04/22 20:53:51, michaelbai wrote:
> > Wrong order, did you get warning?
> 
> good catch! I was actually relying on presubmit tests to catch all these. No I
> did not get a warning.

result of presubmit tests:
sgurun@sgurun:$ git cl presubmit
Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/android/java/src/org/chromium/chrome/browser/ChromeVersionInfo.java
    chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
    content/browser/android/content_view_core_impl.cc
    content/browser/android/content_view_core_impl.h
    content/browser/web_contents/web_contents_android.cc
    content/browser/web_contents/web_contents_android.h
    content/public/android/BUILD.gn
   
content/public/android/java/src/org/chromium/content/browser/ContentView.java
   
content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java
   
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
   
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java
   
content/public/android/java/src/org/chromium/content_public/browser/AccessibilitySnapshotNode.java
   
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
   
content/public/android/javatests/src/org/chromium/content/browser/webcontents/AccessibilitySnapshotTest.java

Presubmit checks took 1.3s to calculate.

[sgurun-gerrit only, 2016-04-22 21:15:32]

sgurun@chromium.org changed reviewers:
- michaelbai@google.com

[Ted C, 2016-04-22 23:07:14]

lgtm

https://codereview.chromium.org/1907703002/diff/100001/content/browser/web_co...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/1907703002/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_android.cc:71: struct
AccessibilitySnapshotParams {
we should really pull this out of web_contents_android.  This is a big enough
feature to live in it's own class

https://codereview.chromium.org/1907703002/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_android.cc:113: bool is_root =
node->GetParent() == NULL;
think this is supposed to be null_ptr

[sgurun-gerrit only, 2016-04-23 00:41:15]

https://codereview.chromium.org/1907703002/diff/100001/content/browser/web_co...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/1907703002/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_android.cc:71: struct
AccessibilitySnapshotParams {
On 2016/04/22 23:07:13, Ted C wrote:
> we should really pull this out of web_contents_android.  This is a big enough
> feature to live in it's own class

next step, I have a crbug assigned to me for it. The first step was combining
the code, which is now done.

https://codereview.chromium.org/1907703002/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_android.cc:113: bool is_root =
node->GetParent() == NULL;
On 2016/04/22 23:07:13, Ted C wrote:
> think this is supposed to be null_ptr

good catch.

[sgurun-gerrit only, 2016-04-23 01:05:23]

The CQ bit was checked by sgurun@chromium.org

[sgurun-gerrit only, 2016-04-23 01:05:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, michaelbai@chromium.org, tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/1907703002/#ps120001
(title: "address ted's review")

[commit-bot: I haz the power, 2016-04-23 01:05:43]

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

[commit-bot: I haz the power, 2016-04-23 02:02:12]

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

[commit-bot: I haz the power, 2016-04-23 02:02:12]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[sgurun-gerrit only, 2016-04-23 06:45:35]

The CQ bit was checked by sgurun@chromium.org

[sgurun-gerrit only, 2016-04-23 06:45:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, michaelbai@chromium.org, dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1907703002/#ps140001
(title: "fix findbug issue (remove unused variable)")

[commit-bot: I haz the power, 2016-04-23 06:45:44]

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

[commit-bot: I haz the power, 2016-04-23 07:46:34]

Description was changed from

==========
Fix a nasty scroll bug for Chrome Now-on-tap feature. Also combine the
code paths for Android Webview and Chrome.

Until now webview and chrome were using separate code paths for copying the
snapshot tree. We are now combining both to use the same code. Further moving
the
tests from android webview to content layer to add coverage for both chrome
and webview.

BUG=603746
==========

to

==========
Fix a nasty scroll bug for Chrome Now-on-tap feature. Also combine the
code paths for Android Webview and Chrome.

Until now webview and chrome were using separate code paths for copying the
snapshot tree. We are now combining both to use the same code. Further moving
the
tests from android webview to content layer to add coverage for both chrome
and webview.

BUG=603746
==========

[commit-bot: I haz the power, 2016-04-23 07:46:36]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-04-23 07:48:12]

Description was changed from

==========
Fix a nasty scroll bug for Chrome Now-on-tap feature. Also combine the
code paths for Android Webview and Chrome.

Until now webview and chrome were using separate code paths for copying the
snapshot tree. We are now combining both to use the same code. Further moving
the
tests from android webview to content layer to add coverage for both chrome
and webview.

BUG=603746
==========

to

==========
Fix a nasty scroll bug for Chrome Now-on-tap feature. Also combine the
code paths for Android Webview and Chrome.

Until now webview and chrome were using separate code paths for copying the
snapshot tree. We are now combining both to use the same code. Further moving
the
tests from android webview to content layer to add coverage for both chrome
and webview.

BUG=603746

Committed: https://crrev.com/98495a3d4de6f2be14a71f675212552b63c71def
Cr-Commit-Position: refs/heads/master@{#389374}
==========

[commit-bot: I haz the power, 2016-04-23 07:48:13]

Patchset 8 (id:??) landed as
https://crrev.com/98495a3d4de6f2be14a71f675212552b63c71def
Cr-Commit-Position: refs/heads/master@{#389374}