Fix computation of Automation API location offsets in WebViews.

Fix computation of Automation API location offsets in WebViews.

The user-visible error here was that when ChromeVox was enabled,
the orange highlight around an object was sometimes wrong when inside
a <webview>, like in the new OOBE.

Previously, to get the global bounds of an object, we'd figure out the
global bounds relative to the top frame, then use a location offset to
convert from frame-relative coordinates to global screen coordinates.

This wasn't correct for WebViews, because a WebView has its own frame tree
and its own location offset, so the location offset was essentially being
counted twice.

It turns out that now that http://crbug.com/618120 is fixed, we don't need
the location offset. Instead we should just walk up from any node all the
way to the desktop root node, applying offsets and transforms as we go.
That way there's a single consistent way to compute global bounding boxes
that's the same everywhere, with no special cases for webview. This required
a small fix to GetParent() to search the desktop tree if an explicit parent
tree ID wasn't found.

Includes a new location test with a WebView.

BUG=658947, 618120
Review-Url: https://codereview.chromium.org/2762373002
Cr-Commit-Position: refs/heads/master@{#460412}
Committed: https://chromium.googlesource.com/chromium/src/+/c7c9e55f9f13ae947e938b8a4c929fefdeffe397

[dmazzoni, 2017-03-21 22:40:51]

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

[dmazzoni, 2017-03-21 22:41:36]

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

[commit-bot: I haz the power, 2017-03-21 22:42:05]

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-03-21 23:05:43]

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

[commit-bot: I haz the power, 2017-03-21 23:05:44]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[dmazzoni, 2017-03-23 17:07:23]

Friendly ping

[dmazzoni, 2017-03-23 17:08:10]

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

[commit-bot: I haz the power, 2017-03-23 17:08:32]

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-03-23 17:39:20]

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

[commit-bot: I haz the power, 2017-03-23 17:39:21]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[David Tseng, 2017-03-23 17:58:29]

https://codereview.chromium.org/2762373002/diff/40001/chrome/renderer/extensi...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (left):

https://codereview.chromium.org/2762373002/diff/40001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:1016: int
parent_tree_id = (*in_out_cache)->tree.data().parent_tree_id;
Can you check the js bindings? This code doesn't seem relevant given that we
track the hosting node explicitly in the root or perhaps I'm missing something.
If GetParent is supposed to work as written, then we shouldn't have to track the
host node in js, but with the trade off of a non-constant time lookup.

[dmazzoni, 2017-03-23 22:02:33]

https://codereview.chromium.org/2762373002/diff/40001/chrome/renderer/extensi...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (left):

https://codereview.chromium.org/2762373002/diff/40001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:1016: int
parent_tree_id = (*in_out_cache)->tree.data().parent_tree_id;
On 2017/03/23 17:58:29, David Tseng wrote:
> Can you check the js bindings? This code doesn't seem relevant given that we
> track the hosting node explicitly in the root or perhaps I'm missing
something.
> If GetParent is supposed to work as written, then we shouldn't have to track
the
> host node in js, but with the trade off of a non-constant time lookup.

Good idea.  Honestly I'd far prefer if JS didn't have to keep track
of the host node. I'd like to keep the JS bindings much simpler. Note
that the lookup is supposed to be non-constant only once, then we cache
it. So the performance difference should be negligible.

I uploaded that follow-up here:
https://codereview.chromium.org/2770713008

OK to go ahead with this change as-is and then work on that follow-up?

[David Tseng, 2017-03-24 21:49:07]

lgtm

https://codereview.chromium.org/2762373002/diff/40001/chrome/renderer/extensi...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (left):

https://codereview.chromium.org/2762373002/diff/40001/chrome/renderer/extensi...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:1016: int
parent_tree_id = (*in_out_cache)->tree.data().parent_tree_id;
On 2017/03/23 22:02:33, dmazzoni wrote:
> On 2017/03/23 17:58:29, David Tseng wrote:
> > Can you check the js bindings? This code doesn't seem relevant given that we
> > track the hosting node explicitly in the root or perhaps I'm missing
> something.
> > If GetParent is supposed to work as written, then we shouldn't have to track
> the
> > host node in js, but with the trade off of a non-constant time lookup.
> 
> Good idea.  Honestly I'd far prefer if JS didn't have to keep track
> of the host node. I'd like to keep the JS bindings much simpler. Note
> that the lookup is supposed to be non-constant only once, then we cache
> it. So the performance difference should be negligible.
> 
> I uploaded that follow-up here:
> https://codereview.chromium.org/2770713008
> 
> OK to go ahead with this change as-is and then work on that follow-up?

Yes this change is fine, but I don't really feel comfortable with this code
since it clearly never got hit before. As far as performance goes, you can do
better if you just ported the same ideas over from js (see the above cl). Please
don't dismiss the work that went into designing the js side of things.

[dmazzoni, 2017-03-27 17:33:27]

On 2017/03/24 21:49:07, David Tseng wrote:
> lgtm
> 
>
https://codereview.chromium.org/2762373002/diff/40001/chrome/renderer/extensi...
> File chrome/renderer/extensions/automation_internal_custom_bindings.cc (left):
> 
>
https://codereview.chromium.org/2762373002/diff/40001/chrome/renderer/extensi...
> chrome/renderer/extensions/automation_internal_custom_bindings.cc:1016: int
> parent_tree_id = (*in_out_cache)->tree.data().parent_tree_id;
> On 2017/03/23 22:02:33, dmazzoni wrote:
> > On 2017/03/23 17:58:29, David Tseng wrote:
> > > Can you check the js bindings? This code doesn't seem relevant given that
we
> > > track the hosting node explicitly in the root or perhaps I'm missing
> > something.
> > > If GetParent is supposed to work as written, then we shouldn't have to
track
> > the
> > > host node in js, but with the trade off of a non-constant time lookup.
> > 
> > Good idea.  Honestly I'd far prefer if JS didn't have to keep track
> > of the host node. I'd like to keep the JS bindings much simpler. Note
> > that the lookup is supposed to be non-constant only once, then we cache
> > it. So the performance difference should be negligible.
> > 
> > I uploaded that follow-up here:
> > https://codereview.chromium.org/2770713008
> > 
> > OK to go ahead with this change as-is and then work on that follow-up?
> 
> Yes this change is fine, but I don't really feel comfortable with this code
> since it clearly never got hit before. As far as performance goes, you can do
> better if you just ported the same ideas over from js (see the above cl).
Please
> don't dismiss the work that went into designing the js side of things.

The code was definitely used, it's been used for bounds calculations.
But totally agreed about caching the parent node
just like JS did, I'll implement that in the follow-up change and
add some tests so we can be confident that this works well before we
remove the JS GetParent code.

[dmazzoni, 2017-03-27 17:33:39]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2017-03-27 17:33:39]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org
Link to the patchset: https://codereview.chromium.org/2762373002/#ps60001
(title: "More robust")

[commit-bot: I haz the power, 2017-03-27 17:34:40]

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-03-27 17:44:55]

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

[commit-bot: I haz the power, 2017-03-27 17:44:56]

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

[dmazzoni, 2017-03-28 17:33:46]

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

[dmazzoni, 2017-03-28 17:33:47]

+tsepez for chrome_extension_messages

[Tom Sepez, 2017-03-28 17:49:54]

lgtm

[dmazzoni, 2017-03-29 06:24:12]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2017-03-29 06:24:13]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
dtseng@chromium.org
Link to the patchset: https://codereview.chromium.org/2762373002/#ps80001
(title: "Only run new test on chromeos because of required perms")

[commit-bot: I haz the power, 2017-03-29 06:24:40]

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-03-29 07:04:40]

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

[commit-bot: I haz the power, 2017-03-29 07:04:41]

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

[dmazzoni, 2017-03-29 15:16:52]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2017-03-29 15:17:38]

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-03-29 16:21:57]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490800612273640,
"parent_rev": "b92ae006f9fcafbd31fea15459c9b704b506aee6", "commit_rev":
"c7c9e55f9f13ae947e938b8a4c929fefdeffe397"}

[commit-bot: I haz the power, 2017-03-29 16:22:41]

Description was changed from

==========
Fix computation of Automation API location offsets in WebViews.

The user-visible error here was that when ChromeVox was enabled,
the orange highlight around an object was sometimes wrong when inside
a <webview>, like in the new OOBE.

Previously, to get the global bounds of an object, we'd figure out the
global bounds relative to the top frame, then use a location offset to
convert from frame-relative coordinates to global screen coordinates.

This wasn't correct for WebViews, because a WebView has its own frame tree
and its own location offset, so the location offset was essentially being
counted twice.

It turns out that now that http://crbug.com/618120 is fixed, we don't need
the location offset. Instead we should just walk up from any node all the
way to the desktop root node, applying offsets and transforms as we go.
That way there's a single consistent way to compute global bounding boxes
that's the same everywhere, with no special cases for webview. This required
a small fix to GetParent() to search the desktop tree if an explicit parent
tree ID wasn't found.

Includes a new location test with a WebView.

BUG=658947,618120
==========

to

==========
Fix computation of Automation API location offsets in WebViews.

The user-visible error here was that when ChromeVox was enabled,
the orange highlight around an object was sometimes wrong when inside
a <webview>, like in the new OOBE.

Previously, to get the global bounds of an object, we'd figure out the
global bounds relative to the top frame, then use a location offset to
convert from frame-relative coordinates to global screen coordinates.

This wasn't correct for WebViews, because a WebView has its own frame tree
and its own location offset, so the location offset was essentially being
counted twice.

It turns out that now that http://crbug.com/618120 is fixed, we don't need
the location offset. Instead we should just walk up from any node all the
way to the desktop root node, applying offsets and transforms as we go.
That way there's a single consistent way to compute global bounding boxes
that's the same everywhere, with no special cases for webview. This required
a small fix to GetParent() to search the desktop tree if an explicit parent
tree ID wasn't found.

Includes a new location test with a WebView.

BUG=658947,618120

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

[commit-bot: I haz the power, 2017-03-29 16:22:42]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/c7c9e55f9f13ae947e938b8a4c92...