Make Windows accessibility event firing aware of guest / child frames.

Make Windows accessibility event firing aware of guest / child frames.

Windows-specific fixes for accessibility of out-of-process iframes
and guest frames (e.g. <webview>):

1. When firing an event or doing a bounds calculation, we need to
call functions on the root BrowserAccessibilityManager's delegate
(not on the child frame's delegate).

2. When a MSAA client tries to look up an MSAA object by child ID,
we need to search child frames as well.

BUG=330307, 368298
Committed: https://crrev.com/28060a875c0a7fb9bbfe06a0057b4321518599a7
Cr-Commit-Position: refs/heads/master@{#312080}

[dmazzoni, 2015-01-15 18:33:50]

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

[nasko, 2015-01-15 21:44:38]

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/f...
File content/browser/frame_host/frame_accessibility.cc (right):

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_accessibility.cc:203: return NULL;
s/NULL/nullptr/

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_accessibility.cc:209: return NULL;
s/NULL/nullptr/

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_accessibility.cc:212: return
child_node->current_frame_host();
There is no verification that the frame found is a direct child of the passed in
RFH.

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/f...
File content/browser/frame_host/frame_accessibility.h (right):

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_accessibility.h:52: // all child frames or
guest frames that was previously associated with this
nit: s/was/were/

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_accessibility.h:74: struct ChildFrameMapping {
I know this isn't part of this CL, but this struct is missing a comment, so I
don't know what it is actually used for. Pairing RFH and FrameTreeNode ID
doesn't seem quite right.

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:1488: bool
RenderFrameHostImpl::IsOutOfProcessFrameInSameBrowserContext(
The name is a bit too long and confusing. What about IsSameSiteInstance() and
invert the meaning of the return value?

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:1489: RenderFrameHostImpl*
child_frame) {
This method doesn't really check if child_frame is indeed child of the current
RFH.

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:1490: if
(child_frame->GetProcess()->GetID() == GetProcess()->GetID())
This check doesn't seem semantically correct. We isolate iframes which are in
separate SiteInstance. The process isolation is an implementation detail of how
SiteInstances are isolated from each other, but they can end up in the same
process if we are above the process limit.

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:1494: if
(GetSiteInstance()->GetBrowserContext() !=
If this ever fails, we have really really bad problems. If you want to keep
that, convert it to a CHECK.

[David Tseng, 2015-01-15 22:33:27]

lgtm
for accessibility related code

https://codereview.chromium.org/799633007/diff/1/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_manager.cc (right):

https://codereview.chromium.org/799633007/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_manager.cc:439:
BrowserAccessibility* root = GetRoot();
Would maybe be cleaner to just hold a pointer to BrowserAccessibilityManager and
walk up that way.

https://codereview.chromium.org/799633007/diff/1/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_manager_win.cc (right):

https://codereview.chromium.org/799633007/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_manager_win.cc:132:
BrowserAccessibilityDelegate* delegate = GetDelegateFromRootManager();
nit: might be good to name these root_delegate.

https://codereview.chromium.org/799633007/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_manager_win.cc:300: if
(result)
nit: not needed.

[dmazzoni, 2015-01-15 23:09:23]

https://codereview.chromium.org/799633007/diff/1/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_manager.cc (right):

https://codereview.chromium.org/799633007/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_manager.cc:439:
BrowserAccessibility* root = GetRoot();
On 2015/01/15 22:33:27, David Tseng wrote:
> Would maybe be cleaner to just hold a pointer to BrowserAccessibilityManager
and
> walk up that way.

Done.

https://codereview.chromium.org/799633007/diff/1/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_manager_win.cc (right):

https://codereview.chromium.org/799633007/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_manager_win.cc:132:
BrowserAccessibilityDelegate* delegate = GetDelegateFromRootManager();
On 2015/01/15 22:33:27, David Tseng wrote:
> nit: might be good to name these root_delegate.

Done.

https://codereview.chromium.org/799633007/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_manager_win.cc:300: if
(result)
On 2015/01/15 22:33:27, David Tseng wrote:
> nit: not needed.

It's needed because it's in a loop

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/f...
File content/browser/frame_host/frame_accessibility.cc (right):

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_accessibility.cc:203: return NULL;
On 2015/01/15 21:44:38, nasko wrote:
> s/NULL/nullptr/

Done.

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_accessibility.cc:209: return NULL;
On 2015/01/15 21:44:38, nasko wrote:
> s/NULL/nullptr/

Done.

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_accessibility.cc:212: return
child_node->current_frame_host();
On 2015/01/15 21:44:38, nasko wrote:
> There is no verification that the frame found is a direct child of the passed
in
> RFH.

True. Note that this is not new logic, I just refactored it out of
FrameAccessibility::GetChild, above, since it's also needed by
FrameAccessibility::GetAllChildFrames.

I added a check, does that look right?

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/f...
File content/browser/frame_host/frame_accessibility.h (right):

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_accessibility.h:52: // all child frames or
guest frames that was previously associated with this
On 2015/01/15 21:44:38, nasko wrote:
> nit: s/was/were/

Done.

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_accessibility.h:74: struct ChildFrameMapping {
On 2015/01/15 21:44:38, nasko wrote:
> I know this isn't part of this CL, but this struct is missing a comment, so I
> don't know what it is actually used for. Pairing RFH and FrameTreeNode ID
> doesn't seem quite right.

It's a bit odd but I think it's right - I added some comments, see if it makes
more sense now.

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:1488: bool
RenderFrameHostImpl::IsOutOfProcessFrameInSameBrowserContext(
On 2015/01/15 21:44:38, nasko wrote:
> The name is a bit too long and confusing. What about IsSameSiteInstance() and
> invert the meaning of the return value?

Done.

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:1489: RenderFrameHostImpl*
child_frame) {
On 2015/01/15 21:44:38, nasko wrote:
> This method doesn't really check if child_frame is indeed child of the current
> RFH.

Is it better now that it's called IsSameSiteInstance, or do you think an
additional check is needed?

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:1490: if
(child_frame->GetProcess()->GetID() == GetProcess()->GetID())
On 2015/01/15 21:44:38, nasko wrote:
> This check doesn't seem semantically correct. We isolate iframes which are in
> separate SiteInstance. The process isolation is an implementation detail of
how
> SiteInstances are isolated from each other, but they can end up in the same
> process if we are above the process limit.

Got it - so all that matters is if it's the same SiteInstance or not.

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:1494: if
(GetSiteInstance()->GetBrowserContext() !=
On 2015/01/15 21:44:38, nasko wrote:
> If this ever fails, we have really really bad problems. If you want to keep
> that, convert it to a CHECK.

Done.

[nasko, 2015-01-16 00:20:33]

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/f...
File content/browser/frame_host/frame_accessibility.cc (right):

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_accessibility.cc:212: return
child_node->current_frame_host();
On 2015/01/15 23:09:22, dmazzoni wrote:
> On 2015/01/15 21:44:38, nasko wrote:
> > There is no verification that the frame found is a direct child of the
passed
> in
> > RFH.
> 
> True. Note that this is not new logic, I just refactored it out of
> FrameAccessibility::GetChild, above, since it's also needed by
> FrameAccessibility::GetAllChildFrames.

Yeah, I saw it is just moved, but it is useful to improve as we move, right?
 
> I added a check, does that look right?

I don't see a check here, I see a check in FrameAccessibility::GetParent.

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/f...
File content/browser/frame_host/frame_accessibility.h (right):

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_accessibility.h:74: struct ChildFrameMapping {
On 2015/01/15 23:09:22, dmazzoni wrote:
> On 2015/01/15 21:44:38, nasko wrote:
> > I know this isn't part of this CL, but this struct is missing a comment, so
I
> > don't know what it is actually used for. Pairing RFH and FrameTreeNode ID
> > doesn't seem quite right.
> 
> It's a bit odd but I think it's right - I added some comments, see if it makes
> more sense now.

Thanks! It is a lot better. I'm curious why this is needed for iframes, but I
can see it for browser plugin. It will be neat if we can remove that once
<webview> uses iframes instead of browser plugin.

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/799633007/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:1490: if
(child_frame->GetProcess()->GetID() == GetProcess()->GetID())
On 2015/01/15 23:09:22, dmazzoni wrote:
> On 2015/01/15 21:44:38, nasko wrote:
> > This check doesn't seem semantically correct. We isolate iframes which are
in
> > separate SiteInstance. The process isolation is an implementation detail of
> how
> > SiteInstances are isolated from each other, but they can end up in the same
> > process if we are above the process limit.
> 
> Got it - so all that matters is if it's the same SiteInstance or not.

Yes, indeed.

https://codereview.chromium.org/799633007/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/frame_accessibility.cc (right):

https://codereview.chromium.org/799633007/diff/20001/content/browser/frame_ho...
content/browser/frame_host/frame_accessibility.cc:164: // We should have gotten
a node in the same frame tree.
Isn't this test now redundant if the two nodes share the same parent, which was
a check added below on 171.

https://codereview.chromium.org/799633007/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/frame_accessibility.h (right):

https://codereview.chromium.org/799633007/diff/20001/content/browser/frame_ho...
content/browser/frame_host/frame_accessibility.h:90: // If the child frame is an
iframe, this is the iframe's frame tree node id,
nit: s/frame tree node/FrameTreeNode/

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

https://codereview.chromium.org/799633007/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:1484: bool
RenderFrameHostImpl::IsSameSiteInstance(RenderFrameHostImpl* child_frame) {
nit: s/child_frame/render_frame_host/

Nothing in the method requires that the argument is a direct child, so let's
avoid shoehorning the parameter name into it.

[dmazzoni, 2015-01-16 17:44:56]

https://codereview.chromium.org/799633007/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/frame_accessibility.cc (right):

https://codereview.chromium.org/799633007/diff/20001/content/browser/frame_ho...
content/browser/frame_host/frame_accessibility.cc:164: // We should have gotten
a node in the same frame tree.
On 2015/01/16 00:20:33, nasko wrote:
> Isn't this test now redundant if the two nodes share the same parent, which
was
> a check added below on 171.

Done.

https://codereview.chromium.org/799633007/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/frame_accessibility.h (right):

https://codereview.chromium.org/799633007/diff/20001/content/browser/frame_ho...
content/browser/frame_host/frame_accessibility.h:90: // If the child frame is an
iframe, this is the iframe's frame tree node id,
On 2015/01/16 00:20:33, nasko wrote:
> nit: s/frame tree node/FrameTreeNode/

Done.

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

https://codereview.chromium.org/799633007/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:1484: bool
RenderFrameHostImpl::IsSameSiteInstance(RenderFrameHostImpl* child_frame) {
On 2015/01/16 00:20:33, nasko wrote:
> nit: s/child_frame/render_frame_host/
> 
> Nothing in the method requires that the argument is a direct child, so let's
> avoid shoehorning the parameter name into it.

Done.

[dmazzoni, 2015-01-16 17:45:00]

On 2015/01/16 00:20:33, nasko wrote:
> > > There is no verification that the frame found is a direct child of the
>  
> > I added a check, does that look right?
> 
> I don't see a check here, I see a check in FrameAccessibility::GetParent.

Oops, you're right - I meant to add it to both places. Done.

> Thanks! It is a lot better. I'm curious why this is needed for iframes, but I
> can see it for browser plugin. It will be neat if we can remove that once
> <webview> uses iframes instead of browser plugin.

The missing piece that's needed for both iframes and browser plugins is the node
that's the host of the child frame.

The code in content/browser/frame_host lets me walk the frame tree, so I know
that the main frame has two child frames - great. But the accessibility tree for
the main frame has two <iframe> elements in it - which one corresponds to which
child frame?

[nasko, 2015-01-16 20:12:55]

LGTM

https://codereview.chromium.org/799633007/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/frame_accessibility.cc (right):

https://codereview.chromium.org/799633007/diff/40001/content/browser/frame_ho...
content/browser/frame_host/frame_accessibility.cc:207: return false;
Why return false? The return value type is pointer, so nullptr should be used.

https://codereview.chromium.org/799633007/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/799633007/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:498: // Asserts that the
given render frame host is part of the same browser
nit: s/render frame host/RenderFrameHost/

[nasko, 2015-01-16 20:13:57]

On 2015/01/16 17:45:00, dmazzoni wrote:
> > Thanks! It is a lot better. I'm curious why this is needed for iframes, but
I
> > can see it for browser plugin. It will be neat if we can remove that once
> > <webview> uses iframes instead of browser plugin.
> 
> The missing piece that's needed for both iframes and browser plugins is the
node
> that's the host of the child frame.
> 
> The code in content/browser/frame_host lets me walk the frame tree, so I know
> that the main frame has two child frames - great. But the accessibility tree
for
> the main frame has two <iframe> elements in it - which one corresponds to
which
> child frame?

Don't routing ids help distinguish? Either way, it is outside of the scope of
this CL, so we can discuss separately.

[dmazzoni, 2015-01-16 20:20:03]

https://codereview.chromium.org/799633007/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/frame_accessibility.cc (right):

https://codereview.chromium.org/799633007/diff/40001/content/browser/frame_ho...
content/browser/frame_host/frame_accessibility.cc:207: return false;
On 2015/01/16 20:12:55, nasko wrote:
> Why return false? The return value type is pointer, so nullptr should be used.

Done.

https://codereview.chromium.org/799633007/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/799633007/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:498: // Asserts that the
given render frame host is part of the same browser
On 2015/01/16 20:12:55, nasko wrote:
> nit: s/render frame host/RenderFrameHost/

Done.

[dmazzoni, 2015-01-19 06:41:46]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2015-01-19 06:42:29]

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

[commit-bot: I haz the power, 2015-01-19 08:27:10]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-01-19 08:28:21]

Patchset 9 (id:??) landed as
https://crrev.com/28060a875c0a7fb9bbfe06a0057b4321518599a7
Cr-Commit-Position: refs/heads/master@{#312080}