Fix accessible touch exploration inside of iframes.

Fix accessible touch exploration inside of iframes.

Now that we've switched to one accessibiliy tree per iframe, the previous
hit testing code broke. Previously we send a message to the top frame with
the point and it fired a HOVER event on the target node. Now if the target
node has a child frame, it sends a response IPC to the browser so it can
redispatch the hit test on the child frame and keep drilling down until it
reaches a leaf node.

Note: all of the changes made in this patch are appropriate for OOPIF,
however it's unclear if the coordinate transformation will work correctly
in that case. Waiting on a resolution to crbug.com/459591 before fixing
that, but since there are no plans to use OOPIF on Android right now this
isn't as urgent.

BUG=596101
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/a04ec0570cf902b3714fee07ffe3361fd80dfa16
Cr-Commit-Position: refs/heads/master@{#384520}

[dmazzoni, 2016-03-28 21:57:13]

Description was changed from

==========
Fix accessible touch exploration inside of iframes.

Now that we've switched to one accessibiliy tree per iframe, the previous
hit testing code broke. Previously we send a message to the top frame with
the point and it fired a HOVER event on the target node. Now if the target
node has a child frame, it sends a response IPC to the browser so it can
redispatch the hit test on the child frame and keep drilling down until it
reaches a leaf node.

Note: all of the changes made in this patch are appropriate for OOPIF,
however it's unclear if the coordinate transformation will work correctly
in that case. Waiting on a resolution to crbug.com/459591 before fixing
that, but since there are no plans to use OOPIF on Android right now this
isn't as urgent.

BUG=596101
==========

to

==========
Fix accessible touch exploration inside of iframes.

Now that we've switched to one accessibiliy tree per iframe, the previous
hit testing code broke. Previously we send a message to the top frame with
the point and it fired a HOVER event on the target node. Now if the target
node has a child frame, it sends a response IPC to the browser so it can
redispatch the hit test on the child frame and keep drilling down until it
reaches a leaf node.

Note: all of the changes made in this patch are appropriate for OOPIF,
however it's unclear if the coordinate transformation will work correctly
in that case. Waiting on a resolution to crbug.com/459591 before fixing
that, but since there are no plans to use OOPIF on Android right now this
isn't as urgent.

BUG=596101
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[dmazzoni, 2016-03-28 21:58:55]

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

[nasko, 2016-03-30 21:01:40]

nasko@chromium.org changed reviewers:
+ kenrb@chromium.org

[nasko, 2016-03-30 21:01:41]

Looping in kenrb@, as he is the expert on hit testing. Overall I'm curious why
we can't rely on the browser-side hit testing to ensure we aren't doing multiple
process hops.

Bunch of style nits.

https://codereview.chromium.org/1836113002/diff/20001/content/browser/accessi...
File content/browser/accessibility/android_hit_testing_browsertest.cc (right):

https://codereview.chromium.org/1836113002/diff/20001/content/browser/accessi...
content/browser/accessibility/android_hit_testing_browsertest.cc:34: bool
AXTreeContainsNodeWithName(BrowserAccessibility* node,
style: First param on a new line.

https://codereview.chromium.org/1836113002/diff/20001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_manager.cc (right):

https://codereview.chromium.org/1836113002/diff/20001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_manager.cc:402: const
gfx::Point& point, int hit_obj_id) {
style: Each param on a new line.

https://codereview.chromium.org/1836113002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1836113002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1741: const gfx::Point&
point, int hit_obj_id) {
style: Each param on a new line.

https://codereview.chromium.org/1836113002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1836113002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:648: const gfx::Point&
point, int hit_obj_id);
style: params on new line.

https://codereview.chromium.org/1836113002/diff/20001/content/common/accessib...
File content/common/accessibility_messages.h (right):

https://codereview.chromium.org/1836113002/diff/20001/content/common/accessib...
content/common/accessibility_messages.h:164: // If the target is an object with
a child frame (like an iframe element),
Does this mean that a <div> that has an <iframe> inside can cause this IPC to be
emitted even if the hittest is somewhere in the <div> section that is not the
<iframe>?

https://codereview.chromium.org/1836113002/diff/20001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/1836113002/diff/20001/content/content_tests.g...
content/content_tests.gypi:194:
'browser/accessibility/android_hit_testing_browsertest.cc',
If this is now a generic browser test, why keep "android_" in the filename?

https://codereview.chromium.org/1836113002/diff/20001/content/test/accessibil...
File content/test/accessibility_browser_test_utils.cc (right):

https://codereview.chromium.org/1836113002/diff/20001/content/test/accessibil...
content/test/accessibility_browser_test_utils.cc:95: RenderFrameHostImpl* rfhi,
ui::AXEvent event_type, int event_target_id) {
style: Each parameter should be on a new line. git cl format can help quite a
bit.

[Charlie Reis, 2016-03-30 21:08:06]

> since there are no plans to use OOPIF on Android right now this
isn't as urgent

Just wanted to clarify that we are doing some experimenting with OOPIFs on
Android (e.g., https://codereview.chromium.org/1797363002/).  It won't launch as
soon, but we shouldn't ignore that platform entirely.

[kenrb, 2016-03-30 22:29:29]

On 2016/03/30 21:01:41, nasko (slow) wrote:
> Looping in kenrb@, as he is the expert on hit testing. Overall I'm curious why
> we can't rely on the browser-side hit testing to ensure we aren't doing
multiple
> process hops.
> 

I assume because it is one per frame, whereas browser process hit testing
operates on Surfaces, which are one per local frame root. In the browser process
we can't distinguish between different frames that come from a single
RenderWidgetHost. Also, browser process hit testing hasn't been implemented on
Android yet.

Is this hit testing only used on Android?

[dmazzoni, 2016-03-30 23:31:27]

On 2016/03/30 22:29:29, kenrb wrote:
> On 2016/03/30 21:01:41, nasko (slow) wrote:
> > Looping in kenrb@, as he is the expert on hit testing. Overall I'm curious
why
> > we can't rely on the browser-side hit testing to ensure we aren't doing
> multiple
> > process hops.
> 
> I assume because it is one per frame, whereas browser process hit testing
> operates on Surfaces, which are one per local frame root. In the browser
process
> we can't distinguish between different frames that come from a single
> RenderWidgetHost. Also, browser process hit testing hasn't been implemented on
> Android yet.

Yes, that's correct. The browser-side hit testing only gets us to the right
process,
not the right frame.

We could try to optimize it by staying within the same renderer when the frame
happens
to be in-process, but this solution is simpler and more foolproof and I'd rather
get it
right first.

> Is this hit testing only used on Android?

Currently yes, only Android. It's important to land this in M51 because it used
to
work and the changes we made in M51 to support OOPIF broke it. In the future
I anticipate using this on Chrome OS.

> Just wanted to clarify that we are doing some experimenting with OOPIFs on
> Android (e.g., https://codereview.chromium.org/1797363002/).  It won't launch
as
> soon, but we shouldn't ignore that platform entirely.

Good to know! Should just require decoupling the page scale transformation
from the frame transformations.

[kenrb, 2016-03-31 15:05:34]

lgtm

[dmazzoni, 2016-03-31 16:15:55]

I ran git cl format, should be good now.

https://codereview.chromium.org/1836113002/diff/20001/content/browser/accessi...
File content/browser/accessibility/android_hit_testing_browsertest.cc (right):

https://codereview.chromium.org/1836113002/diff/20001/content/browser/accessi...
content/browser/accessibility/android_hit_testing_browsertest.cc:34: bool
AXTreeContainsNodeWithName(BrowserAccessibility* node,
On 2016/03/30 21:01:41, nasko (slow) wrote:
> style: First param on a new line.

Done.

https://codereview.chromium.org/1836113002/diff/20001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_manager.cc (right):

https://codereview.chromium.org/1836113002/diff/20001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_manager.cc:402: const
gfx::Point& point, int hit_obj_id) {
On 2016/03/30 21:01:41, nasko (slow) wrote:
> style: Each param on a new line.

Done.

https://codereview.chromium.org/1836113002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1836113002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1741: const gfx::Point&
point, int hit_obj_id) {
On 2016/03/30 21:01:41, nasko (slow) wrote:
> style: Each param on a new line.

Done.

https://codereview.chromium.org/1836113002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1836113002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:648: const gfx::Point&
point, int hit_obj_id);
On 2016/03/30 21:01:41, nasko (slow) wrote:
> style: params on new line.

Done.

https://codereview.chromium.org/1836113002/diff/20001/content/common/accessib...
File content/common/accessibility_messages.h (right):

https://codereview.chromium.org/1836113002/diff/20001/content/common/accessib...
content/common/accessibility_messages.h:164: // If the target is an object with
a child frame (like an iframe element),
On 2016/03/30 21:01:41, nasko (slow) wrote:
> Does this mean that a <div> that has an <iframe> inside can cause this IPC to
be
> emitted even if the hittest is somewhere in the <div> section that is not the
> <iframe>?

Sorry if my wording is unclear. I meant that the <iframe> itself is the result
of the
hit test. Similarly if it's a browser plugin <object>. If the result of the hit
test is
a div, we're done.

https://codereview.chromium.org/1836113002/diff/20001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/1836113002/diff/20001/content/content_tests.g...
content/content_tests.gypi:194:
'browser/accessibility/android_hit_testing_browsertest.cc',
On 2016/03/30 21:01:41, nasko (slow) wrote:
> If this is now a generic browser test, why keep "android_" in the filename?

Done.

https://codereview.chromium.org/1836113002/diff/20001/content/test/accessibil...
File content/test/accessibility_browser_test_utils.cc (right):

https://codereview.chromium.org/1836113002/diff/20001/content/test/accessibil...
content/test/accessibility_browser_test_utils.cc:95: RenderFrameHostImpl* rfhi,
ui::AXEvent event_type, int event_target_id) {
On 2016/03/30 21:01:41, nasko (slow) wrote:
> style: Each parameter should be on a new line. git cl format can help quite a
> bit.

Ran git cl format.

[nasko, 2016-03-31 16:35:57]

LGTM

https://codereview.chromium.org/1836113002/diff/20001/content/common/accessib...
File content/common/accessibility_messages.h (right):

https://codereview.chromium.org/1836113002/diff/20001/content/common/accessib...
content/common/accessibility_messages.h:164: // If the target is an object with
a child frame (like an iframe element),
On 2016/03/31 16:15:55, dmazzoni wrote:
> On 2016/03/30 21:01:41, nasko (slow) wrote:
> > Does this mean that a <div> that has an <iframe> inside can cause this IPC
to
> be
> > emitted even if the hittest is somewhere in the <div> section that is not
the
> > <iframe>?
> 
> Sorry if my wording is unclear. I meant that the <iframe> itself is the result
> of the
> hit test. Similarly if it's a browser plugin <object>. If the result of the
hit
> test is
> a div, we're done.

Thanks! This definitely clarifies it.

[dmazzoni, 2016-03-31 16:36:30]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-03-31 16:36:31]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org
Link to the patchset: https://codereview.chromium.org/1836113002/#ps40001
(title: "Run git cl format, address other feedback")

[commit-bot: I haz the power, 2016-03-31 16:36:50]

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

[commit-bot: I haz the power, 2016-03-31 18:37:41]

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

[commit-bot: I haz the power, 2016-03-31 18:37:43]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dmazzoni, 2016-03-31 20:29:37]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-03-31 20:30:05]

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

[dmazzoni, 2016-03-31 21:47:15]

The CQ bit was unchecked by dmazzoni@chromium.org

[dmazzoni, 2016-04-01 07:01:11]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-04-01 07:01:12]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1836113002/#ps60001
(title: "Fix test startup on Android")

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

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

[commit-bot: I haz the power, 2016-04-01 08:41:24]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-01 08:42:56]

Description was changed from

==========
Fix accessible touch exploration inside of iframes.

Now that we've switched to one accessibiliy tree per iframe, the previous
hit testing code broke. Previously we send a message to the top frame with
the point and it fired a HOVER event on the target node. Now if the target
node has a child frame, it sends a response IPC to the browser so it can
redispatch the hit test on the child frame and keep drilling down until it
reaches a leaf node.

Note: all of the changes made in this patch are appropriate for OOPIF,
however it's unclear if the coordinate transformation will work correctly
in that case. Waiting on a resolution to crbug.com/459591 before fixing
that, but since there are no plans to use OOPIF on Android right now this
isn't as urgent.

BUG=596101
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix accessible touch exploration inside of iframes.

Now that we've switched to one accessibiliy tree per iframe, the previous
hit testing code broke. Previously we send a message to the top frame with
the point and it fired a HOVER event on the target node. Now if the target
node has a child frame, it sends a response IPC to the browser so it can
redispatch the hit test on the child frame and keep drilling down until it
reaches a leaf node.

Note: all of the changes made in this patch are appropriate for OOPIF,
however it's unclear if the coordinate transformation will work correctly
in that case. Waiting on a resolution to crbug.com/459591 before fixing
that, but since there are no plans to use OOPIF on Android right now this
isn't as urgent.

BUG=596101
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/a04ec0570cf902b3714fee07ffe3361fd80dfa16
Cr-Commit-Position: refs/heads/master@{#384520}
==========

[commit-bot: I haz the power, 2016-04-01 08:42:57]

Patchset 4 (id:??) landed as
https://crrev.com/a04ec0570cf902b3714fee07ffe3361fd80dfa16
Cr-Commit-Position: refs/heads/master@{#384520}