Migrate accessibility from RenderView to RenderFrame.

Migrate accessibility from RenderView to RenderFrame and RVH to RFH.

In preparation for out-of-process iframes, move almost all accessibility
functionality from RenderView to RenderFrame, and from RVHI and RWHI
to RFHI.

In addition, the accessibility mode is moved to WebContents, and changing
the mode triggers changing it on all frames.

BUG=368298
R=jam@chromium.org, kenrb@chromium.org, nick@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285767

[dmazzoni, 2014-05-12 15:15:00]

I need to rebase and fix a couple of tests, but this is 90% done so I'd
appreciate a quick look soon in case you see any potential concerns.

[nasko, 2014-05-12 17:28:00]

I think it is mostly there. Few nits and comments.
One thing I don't see here is how InterstitialPageImpl will change as a result
of this CL. It might be useful to include those changes here.

https://codereview.chromium.org/273423004/diff/1/chrome/browser/chrome_browse...
File chrome/browser/chrome_browser_application_mac.mm (left):

https://codereview.chromium.org/273423004/diff/1/chrome/browser/chrome_browse...
chrome/browser/chrome_browser_application_mac.mm:541: for (TabContentsIterator
it; !it.done(); it.Next()) {
There doesn't seem to be updated version of this code. Is this intentional?

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

https://codereview.chromium.org/273423004/diff/1/content/browser/accessibilit...
content/browser/accessibility/accessibility_mode_helper.cc:5: #include
"base/logging.h"
Is this really needed?

https://codereview.chromium.org/273423004/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/273423004/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_delegate.h:106: // Get the
accessibility mode for all frames in this WebContents.
nit: This wording reads as though it will return the setting for each frame. The
accessibility setting is for the WebContents or more generally the Delegate and
as an implementation detail it happens to apply to all frames.

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

https://codereview.chromium.org/273423004/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:328: Send(new
AccessibilityMsg_SetFocus(GetRoutingID(), object_id));
nit: The pattern in this class is to use routing_id_ instead of calling the
public method.

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

https://codereview.chromium.org/273423004/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.h:338: scoped_ptr<ui::AXTree>
ax_tree_;
Let's label the variable to indicate it is to be used for testing only. The
comment is good, but I don't think it is sufficient.

https://codereview.chromium.org/273423004/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/273423004/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:451:
std::vector<WebContentsImpl*>* web_contents_vector) {
Why not return the vector as output element instead of taking it as a parameter?
Optimizations in the compiler will avoid copies if you declare the vector as a
stack variable and return it.

https://codereview.chromium.org/273423004/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:453:
RenderWidgetHostImpl::GetAllRenderWidgetHosts());
You probably don't want the *All* version of the method, just use
GetRenderWidgetHosts. It will return less elements to iterate on and will cover
the same set of WebContents.

https://codereview.chromium.org/273423004/diff/1/content/common/frame_messages.h
File content/common/frame_messages.h (right):

https://codereview.chromium.org/273423004/diff/1/content/common/frame_message...
content/common/frame_messages.h:29: AccessibilityModeComplete)
Nice! Thanks for making the IPC more secure!

https://codereview.chromium.org/273423004/diff/1/content/public/browser/web_c...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/273423004/diff/1/content/public/browser/web_c...
content/public/browser/web_contents.h:223: virtual bool
IsTreeOnlyAccessibilityModeForTesting() const = 0;
I'm not sure if we add testing methods to the public API. I'll leave it to jam@
and creis@ to comment, but it feels strange.

[dmazzoni, 2014-05-13 06:05:30]

On 2014/05/12 17:28:00, nasko wrote:
> I think it is mostly there. Few nits and comments.
> One thing I don't see here is how InterstitialPageImpl will change as a result
> of this CL. It might be useful to include those changes here.

Fixed now. I think all we need to do is support GetAccessibilityMode, by having
it forward the state from the WebContentsImpl, if any.

[dmazzoni, 2014-05-13 06:36:52]

https://codereview.chromium.org/273423004/diff/1/chrome/browser/chrome_browse...
File chrome/browser/chrome_browser_application_mac.mm (left):

https://codereview.chromium.org/273423004/diff/1/chrome/browser/chrome_browse...
chrome/browser/chrome_browser_application_mac.mm:541: for (TabContentsIterator
it; !it.done(); it.Next()) {
On 2014/05/12 17:28:01, nasko wrote:
> There doesn't seem to be updated version of this code. Is this intentional?

This is intentional. BrowserAccessibilityState does this automatically now, we
forgot to remove this redundant code earlier.

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

https://codereview.chromium.org/273423004/diff/1/content/browser/accessibilit...
content/browser/accessibility/accessibility_mode_helper.cc:5: #include
"base/logging.h"
On 2014/05/12 17:28:01, nasko wrote:
> Is this really needed?

It's needed for the DCHECK, below. This change won't compile without it - it's
probably because we changed what gets included from other header files included
by this one.

https://codereview.chromium.org/273423004/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/273423004/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_delegate.h:106: // Get the
accessibility mode for all frames in this WebContents.
On 2014/05/12 17:28:01, nasko wrote:
> nit: This wording reads as though it will return the setting for each frame.
The
> accessibility setting is for the WebContents or more generally the Delegate
and
> as an implementation detail it happens to apply to all frames.

Good point. I clarified the wording.

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

https://codereview.chromium.org/273423004/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:328: Send(new
AccessibilityMsg_SetFocus(GetRoutingID(), object_id));
On 2014/05/12 17:28:01, nasko wrote:
> nit: The pattern in this class is to use routing_id_ instead of calling the
> public method.

Done.

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

https://codereview.chromium.org/273423004/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.h:338: scoped_ptr<ui::AXTree>
ax_tree_;
On 2014/05/12 17:28:01, nasko wrote:
> Let's label the variable to indicate it is to be used for testing only. The
> comment is good, but I don't think it is sufficient.

Done.

https://codereview.chromium.org/273423004/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/273423004/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:451:
std::vector<WebContentsImpl*>* web_contents_vector) {
On 2014/05/12 17:28:01, nasko wrote:
> Why not return the vector as output element instead of taking it as a
parameter?
> Optimizations in the compiler will avoid copies if you declare the vector as a
> stack variable and return it.

Done.

https://codereview.chromium.org/273423004/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:453:
RenderWidgetHostImpl::GetAllRenderWidgetHosts());
On 2014/05/12 17:28:01, nasko wrote:
> You probably don't want the *All* version of the method, just use
> GetRenderWidgetHosts. It will return less elements to iterate on and will
cover
> the same set of WebContents.

OK. Just to check, though, we want to be sure to set the mode properly on
anything that's swapped out, because it might get swapped in later. Is it still
okay to only call GetRenderWidgetHosts?

https://codereview.chromium.org/273423004/diff/1/content/common/frame_messages.h
File content/common/frame_messages.h (right):

https://codereview.chromium.org/273423004/diff/1/content/common/frame_message...
content/common/frame_messages.h:29: AccessibilityModeComplete)
On 2014/05/12 17:28:01, nasko wrote:
> Nice! Thanks for making the IPC more secure!

:)

https://codereview.chromium.org/273423004/diff/1/content/public/browser/web_c...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/273423004/diff/1/content/public/browser/web_c...
content/public/browser/web_contents.h:223: virtual bool
IsTreeOnlyAccessibilityModeForTesting() const = 0;
On 2014/05/12 17:28:01, nasko wrote:
> I'm not sure if we add testing methods to the public API. I'll leave it to
jam@
> and creis@ to comment, but it feels strange.

The reason we need these is because we want to test them from Chrome code (an
extension api that toggles these). It's not clear how else we'd do that.

[ncarter (slow), 2014-05-14 23:56:22]

https://codereview.chromium.org/273423004/diff/120001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_application_mac.mm (right):

https://codereview.chromium.org/273423004/diff/120001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_application_mac.mm:540:
content::BrowserAccessibilityState::GetInstance()->OnScreenReaderDetected();
Some code was deleted from here -- if it was supposed to move somewhere else, I
didn't follow where in the patch that happened.

https://codereview.chromium.org/273423004/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/273423004/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1198:
GetBrowserAccessibilityManager()->set_delegate(delegate);
Is this delegate relationship correct?

Previously the accessibility delegate was the rwh, which always was reachable
from the rwhv. Same same, no lifetime issues. Now the delegate is a rfh, and
(for sanity's sake) I would hope that this means that it's the RFH of the main
frame. But I actually don't see any such guarantee in the code?

It is possible for two rfh's exist that are sharing a rvh; hence sharing a rwh
and hence sharing a rwhv.

The BrowserAccessibilityManager, which is owned by the rwhv, retains a pointer
to one of these rfh's (as a delegate). So at any given time, the
BrowserAccessibilityManager has a pointer to the most recent rfh that called
view->CreateBrowserAccessibilityManagerIfNeeded(this), which is done in response
to an IPC. But what happens if that rfh goes away (say, it's the RFH of a
same-site iframe, and it is deleted)? Is the RWHV or its accessibility manager
informed somehow that the accessibility delegate no longer exists? I don't see
where that happens, and so I'm a little worried that the pointer might persist
after the rfh object is deleted.

Should the delegate responsibilities go somewhere other than the RFH, or else
does each RFH need its accessibility manager?

Unfortunately I don't have a solution ready to suggest, as I don't fully grok
what powers the BrowserAccessibilityDelegate really needs. So let's figure it
out together. I'd definitely like nasko's thoughts on this too -- or it might
help to pick jam's brain.

https://codereview.chromium.org/273423004/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1222:
BrowserAccessibilityManagerWin::GetEmptyDocument(), delegate);
Since this delegate is sometimes null, I wonder if there's an advantage to not
passing it in the ctor, and requiring instead for it always to arrive via
set_delegate? Seems to me that this would force the null and non-null callers of
this function down a more similar code path.

https://codereview.chromium.org/273423004/diff/120001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/273423004/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:727:
ForEachFrame(base::Bind(&SetAccessibilityModeOnFrame, mode));
ForEachFrame does not run on all RenderFrameHosts affiliated with the
WebContents, just the current frames at each frame tree node. There can be a
pending RFH at each level too, as well as RFHs that exist as proxies (though
nasko is changing that).

A quick fix would be to add:

frame_tree_.ForEach(base::Bind(&ForEachPendingFrameInternal,

This still doesn't address the RenderFrameHosts that are owned by
RenderFrameProxyHosts -- but I'd need nasko to say whether or not that is
something worth worrying about.

I'd be supportive of adding a ForEachFrame variant to WebContentsImpl that let
you specify whether to visit pending frames too; this is a common enough need
and the current API is quite misleading.

https://codereview.chromium.org/273423004/diff/120001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/273423004/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:1124: // The accssibility mode
for all frames. This is queried when each frame
typo in second word.

https://codereview.chromium.org/273423004/diff/120001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/273423004/diff/120001/content/public/browser/...
content/public/browser/web_contents.h:231: 
These deserve comments; this is a widely read file.

https://codereview.chromium.org/273423004/diff/120001/content/renderer/access...
File content/renderer/accessibility/renderer_accessibility.h (right):

https://codereview.chromium.org/273423004/diff/120001/content/renderer/access...
content/renderer/accessibility/renderer_accessibility.h:39: // RenderViewImpl;
it lazily starts as Off or EditableTextOnly depending on
RenderView->RenderFrame in these comments.

https://codereview.chromium.org/273423004/diff/120001/content/renderer/access...
File content/renderer/accessibility/renderer_accessibility_focus_only.h (right):

https://codereview.chromium.org/273423004/diff/120001/content/renderer/access...
content/renderer/accessibility/renderer_accessibility_focus_only.h:12: // This
is an accsessibility implementation that only handles whatever
typo here.

[dmazzoni, 2014-05-19 07:25:00]

In reference to comments below - I went ahead and made RFHI the owner of the
BrowserAccessibilityManager.

https://codereview.chromium.org/273423004/diff/120001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_application_mac.mm (right):

https://codereview.chromium.org/273423004/diff/120001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_application_mac.mm:540:
content::BrowserAccessibilityState::GetInstance()->OnScreenReaderDetected();
On 2014/05/14 23:56:23, ncarter wrote:
> Some code was deleted from here -- if it was supposed to move somewhere else,
I
> didn't follow where in the patch that happened.

This was on purpose. That code isn't needed anymore because
BrowserAccessibilityState updates all of the WebContents automatically now.

https://codereview.chromium.org/273423004/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/273423004/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1198:
GetBrowserAccessibilityManager()->set_delegate(delegate);
On 2014/05/14 23:56:23, ncarter wrote:
> Is this delegate relationship correct?
> 
> Previously the accessibility delegate was the rwh, which always was reachable
> from the rwhv. Same same, no lifetime issues. Now the delegate is a rfh, and
> (for sanity's sake) I would hope that this means that it's the RFH of the main
> frame. But I actually don't see any such guarantee in the code?
> 
> It is possible for two rfh's exist that are sharing a rvh; hence sharing a rwh
> and hence sharing a rwhv.
> 
> The BrowserAccessibilityManager, which is owned by the rwhv, retains a pointer
> to one of these rfh's (as a delegate). So at any given time, the
> BrowserAccessibilityManager has a pointer to the most recent rfh that called
> view->CreateBrowserAccessibilityManagerIfNeeded(this), which is done in
response
> to an IPC. But what happens if that rfh goes away (say, it's the RFH of a
> same-site iframe, and it is deleted)? Is the RWHV or its accessibility manager
> informed somehow that the accessibility delegate no longer exists? I don't see
> where that happens, and so I'm a little worried that the pointer might persist
> after the rfh object is deleted.
> 
> Should the delegate responsibilities go somewhere other than the RFH, or else
> does each RFH need its accessibility manager?
> 
> Unfortunately I don't have a solution ready to suggest, as I don't fully grok
> what powers the BrowserAccessibilityDelegate really needs. So let's figure it
> out together. I'd definitely like nasko's thoughts on this too -- or it might
> help to pick jam's brain.

This is a great question. I don't think I fully understand the implications of
two
RFH's sharing a RVH, but let me explain what requirements we need on the
accessibility side.

Right now Blink's AX tree spans all of the frames reachable from the local page.
We need one BrowserAccessibilityManager (henceforth BAM) per Blink AX tree.
Prior to OOPIF, the only case where we need two BAMs is when we have a <webview>
tag.
Note that <webview> is not accessible now; this refactoring should help make
<webview>
accessibility possible.

My understanding is that eventually there will be a separate instance of Blink
for each
frame, even when the frames are from the same origin, right? So eventually we'll
have a
separate AX tree for every frame, and we'll definitely need a BAM for each
frame.

If there's ever a world when there's some mix of both - like some frames will be
handled by the same Blink page, and other frames will be in a separate process,
then
we could have a situation where only some frames need a BAM. However, one
simplification we could make would be to modify Blink accessibility so that
there's
just one AX tree per frame no matter what. I don't see any downsides to this,
once
OOPIF is possible there's no reason not to do this.

OK, so it seems to me that eventually we'll need one BAM per RFH, but that in
the
interim there may be some RFHs that don't need a BAM, because their parent's
RFH's
BAM already covers that frame. I don't think there's any problem with this - the
Blink
AX code will establish which BAM is a descendant of which node of another BAM,
so having an unused BAM in one RFH shouldn't break anything.

In addition, the *root* BAM needs to be connected to its RWHV, because the root
RWHV will query the BAM when implementing accessibility APIs on its view, and
the BAM needs a bit of information about its containing view in order to
implement
native accessibility APIs for web elements.

Child BAMs don't really care about their RWHV, which I'm assuming will be
something like
a RWHVChildFrame anyway. Child BAMs should know they're children, and they
should
only communicate everything up through the root BAM that's the one immediately
contained in some native View.

BrowserAccessibilityDelegate currently conflates two different needs: it needs
to pass
messages to its corresponding render-process frame, and it also needs to pass
information to its native view.

I'm starting to think that the best solution might be for RFH to own the BAM and
to
implement the delegate. Instead of having the RWHV create and own the BAM, the
RFH
should just query the RWHV as needed by the BAM.

How about this: if the BAM asks the Delegate for, e.g., its native view, only
the RFH for the
main frame would return it (by asking its RWHV). Any other RFH would return
NULL, forcing
the BAM to pass its message up through its parent BAM.

Am I correct in my understanding that one WebContents will only have one "real"
RWHV, and
any other subframe will only have a RWHVChildFrame or something like that?

https://codereview.chromium.org/273423004/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1222:
BrowserAccessibilityManagerWin::GetEmptyDocument(), delegate);
On 2014/05/14 23:56:23, ncarter wrote:
> Since this delegate is sometimes null, I wonder if there's an advantage to not
> passing it in the ctor, and requiring instead for it always to arrive via
> set_delegate? Seems to me that this would force the null and non-null callers
of
> this function down a more similar code path.

This would also be simplified if the RFHI owns the BAM.

https://codereview.chromium.org/273423004/diff/120001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/273423004/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:727:
ForEachFrame(base::Bind(&SetAccessibilityModeOnFrame, mode));
On 2014/05/14 23:56:23, ncarter wrote:
> ForEachFrame does not run on all RenderFrameHosts affiliated with the
> WebContents, just the current frames at each frame tree node. There can be a
> pending RFH at each level too, as well as RFHs that exist as proxies (though
> nasko is changing that).
> 
> A quick fix would be to add:
> 
> frame_tree_.ForEach(base::Bind(&ForEachPendingFrameInternal,
> 
> This still doesn't address the RenderFrameHosts that are owned by
> RenderFrameProxyHosts -- but I'd need nasko to say whether or not that is
> something worth worrying about.
> 
> I'd be supportive of adding a ForEachFrame variant to WebContentsImpl that let
> you specify whether to visit pending frames too; this is a common enough need
> and the current API is quite misleading.

I added a public ForEachFrameAndPendingFrame and used that instead. Any better
ideas for a name?

If you think other callers could be confused, how about we make that the default
- i.e. ForEachFrame and ForEachActiveFrame or something like that?

https://codereview.chromium.org/273423004/diff/120001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/273423004/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:1124: // The accssibility mode
for all frames. This is queried when each frame
On 2014/05/14 23:56:23, ncarter wrote:
> typo in second word.

Done.

https://codereview.chromium.org/273423004/diff/120001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/273423004/diff/120001/content/public/browser/...
content/public/browser/web_contents.h:231: 
On 2014/05/14 23:56:23, ncarter wrote:
> These deserve comments; this is a widely read file.

Done.

https://codereview.chromium.org/273423004/diff/120001/content/renderer/access...
File content/renderer/accessibility/renderer_accessibility.h (right):

https://codereview.chromium.org/273423004/diff/120001/content/renderer/access...
content/renderer/accessibility/renderer_accessibility.h:39: // RenderViewImpl;
it lazily starts as Off or EditableTextOnly depending on
On 2014/05/14 23:56:23, ncarter wrote:
> RenderView->RenderFrame in these comments.

Done.

https://codereview.chromium.org/273423004/diff/120001/content/renderer/access...
File content/renderer/accessibility/renderer_accessibility_focus_only.h (right):

https://codereview.chromium.org/273423004/diff/120001/content/renderer/access...
content/renderer/accessibility/renderer_accessibility_focus_only.h:12: // This
is an accsessibility implementation that only handles whatever
On 2014/05/14 23:56:23, ncarter wrote:
> typo here.

Done.

[dmazzoni, 2014-05-23 23:16:24]

I think this is getting closer, please take a fresh look when you have a chance.

[ncarter (slow), 2014-05-28 00:50:10]

I took another pass through, and don't really have any improvements to suggest
-- I think this is headed in the right direction.

https://codereview.chromium.org/273423004/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/273423004/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1198:
GetBrowserAccessibilityManager()->set_delegate(delegate);
On 2014/05/19 07:25:01, dmazzoni wrote:
> On 2014/05/14 23:56:23, ncarter wrote:
> > Is this delegate relationship correct?
> > 
> > Previously the accessibility delegate was the rwh, which always was
reachable
> > from the rwhv. Same same, no lifetime issues. Now the delegate is a rfh, and
> > (for sanity's sake) I would hope that this means that it's the RFH of the
main
> > frame. But I actually don't see any such guarantee in the code?
> > 
> > It is possible for two rfh's exist that are sharing a rvh; hence sharing a
rwh
> > and hence sharing a rwhv.
> > 
> > The BrowserAccessibilityManager, which is owned by the rwhv, retains a
pointer
> > to one of these rfh's (as a delegate). So at any given time, the
> > BrowserAccessibilityManager has a pointer to the most recent rfh that called
> > view->CreateBrowserAccessibilityManagerIfNeeded(this), which is done in
> response
> > to an IPC. But what happens if that rfh goes away (say, it's the RFH of a
> > same-site iframe, and it is deleted)? Is the RWHV or its accessibility
manager
> > informed somehow that the accessibility delegate no longer exists? I don't
see
> > where that happens, and so I'm a little worried that the pointer might
persist
> > after the rfh object is deleted.
> > 
> > Should the delegate responsibilities go somewhere other than the RFH, or
else
> > does each RFH need its accessibility manager?
> > 
> > Unfortunately I don't have a solution ready to suggest, as I don't fully
grok
> > what powers the BrowserAccessibilityDelegate really needs. So let's figure
it
> > out together. I'd definitely like nasko's thoughts on this too -- or it
might
> > help to pick jam's brain.
> 
> This is a great question. I don't think I fully understand the implications of
> two
> RFH's sharing a RVH, but let me explain what requirements we need on the
> accessibility side.
> 
> Right now Blink's AX tree spans all of the frames reachable from the local
page.
> We need one BrowserAccessibilityManager (henceforth BAM) per Blink AX tree.
> Prior to OOPIF, the only case where we need two BAMs is when we have a
<webview>
> tag.
> Note that <webview> is not accessible now; this refactoring should help make
> <webview>
> accessibility possible.
> 
> My understanding is that eventually there will be a separate instance of Blink
> for each
> frame, even when the frames are from the same origin, right? So eventually
we'll
> have a
> separate AX tree for every frame, and we'll definitely need a BAM for each
> frame.
> 
> If there's ever a world when there's some mix of both - like some frames will
be
> handled by the same Blink page, and other frames will be in a separate
process,
> then
> we could have a situation where only some frames need a BAM. However, one
> simplification we could make would be to modify Blink accessibility so that
> there's
> just one AX tree per frame no matter what. I don't see any downsides to this,
> once
> OOPIF is possible there's no reason not to do this.

I think you have it right. Where possible I think it is best (from a design
simplicity and maintenance perspective) to arrange things so there is, as you
say, "one AX tree per frame no matter what". If we do that then same-process and
cross-process frames work in essentially the same way as far as the goo in the
browser process is concerned.

> OK, so it seems to me that eventually we'll need one BAM per RFH, but that in
> the
> interim there may be some RFHs that don't need a BAM, because their parent's
> RFH's
> BAM already covers that frame. I don't think there's any problem with this -
the
> Blink
> AX code will establish which BAM is a descendant of which node of another BAM,
> so having an unused BAM in one RFH shouldn't break anything.
>
> In addition, the *root* BAM needs to be connected to its RWHV, because the
root
> RWHV will query the BAM when implementing accessibility APIs on its view, and
> the BAM needs a bit of information about its containing view in order to
> implement
> native accessibility APIs for web elements.

This makes sense. You're heading in a direction, I think, where the BAMs are
going to exist one per frame but necessarily somewhat tree-aware. An open
question in my mind is whether the top level BAM (which is the one interacting
with a "real" RWHV) will ultimately have the same runtime type as the lower
level BAMs, or whether it will need a specialized subclass or something. The act
of coding this up will probably make the design clear.

> Child BAMs don't really care about their RWHV, which I'm assuming will be
> something like
> a RWHVChildFrame anyway. Child BAMs should know they're children, and they
> should
> only communicate everything up through the root BAM that's the one immediately
> contained in some native View.
> 
> BrowserAccessibilityDelegate currently conflates two different needs: it needs
> to pass
> messages to its corresponding render-process frame, and it also needs to pass
> information to its native view.
> 
> I'm starting to think that the best solution might be for RFH to own the BAM
and
> to
> implement the delegate. Instead of having the RWHV create and own the BAM, the
> RFH
> should just query the RWHV as needed by the BAM.

Makes sense, and I see you've implemented this.

> How about this: if the BAM asks the Delegate for, e.g., its native view, only
> the RFH for the
> main frame would return it (by asking its RWHV). Any other RFH would return
> NULL, forcing
> the BAM to pass its message up through its parent BAM.
> 
> Am I correct in my understanding that one WebContents will only have one
"real"
> RWHV, and
> any other subframe will only have a RWHVChildFrame or something like that?

Yes, this understanding (only one real RWHV in the frame tree) is correct.

https://codereview.chromium.org/273423004/diff/250001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/273423004/diff/250001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1193:
RenderWidgetHostViewAura::CreateBrowserAccessibilityManager(
Indentation is off here.

https://codereview.chromium.org/273423004/diff/250001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/273423004/diff/250001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.cc:484:
RenderWidgetHostViewBase::CreateBrowserAccessibilityManager(
Indentation is off.

[dmazzoni, 2014-05-28 15:03:37]

Thanks!

@jam, time to take a look?

On Tue, May 27, 2014 at 5:50 PM, <nick@chromium.org> wrote:

> I took another pass through, and don't really have any improvements to
> suggest
> -- I think this is headed in the right direction.
>
>
>
> https://codereview.chromium.org/273423004/diff/120001/
> content/browser/renderer_host/render_widget_host_view_aura.cc
> File content/browser/renderer_host/render_widget_host_view_aura.cc
> (right):
>
> https://codereview.chromium.org/273423004/diff/120001/
> content/browser/renderer_host/render_widget_host_view_aura.cc#newcode1198
> content/browser/renderer_host/render_widget_host_view_aura.cc:1198:
> GetBrowserAccessibilityManager()->set_delegate(delegate);
> On 2014/05/19 07:25:01, dmazzoni wrote:
>
>> On 2014/05/14 23:56:23, ncarter wrote:
>> > Is this delegate relationship correct?
>> >
>> > Previously the accessibility delegate was the rwh, which always was
>>
> reachable
>
>> > from the rwhv. Same same, no lifetime issues. Now the delegate is a
>>
> rfh, and
>
>> > (for sanity's sake) I would hope that this means that it's the RFH
>>
> of the main
>
>> > frame. But I actually don't see any such guarantee in the code?
>> >
>> > It is possible for two rfh's exist that are sharing a rvh; hence
>>
> sharing a rwh
>
>> > and hence sharing a rwhv.
>> >
>> > The BrowserAccessibilityManager, which is owned by the rwhv, retains
>>
> a pointer
>
>> > to one of these rfh's (as a delegate). So at any given time, the
>> > BrowserAccessibilityManager has a pointer to the most recent rfh
>>
> that called
>
>> > view->CreateBrowserAccessibilityManagerIfNeeded(this), which is done
>>
> in
>
>> response
>> > to an IPC. But what happens if that rfh goes away (say, it's the RFH
>>
> of a
>
>> > same-site iframe, and it is deleted)? Is the RWHV or its
>>
> accessibility manager
>
>> > informed somehow that the accessibility delegate no longer exists? I
>>
> don't see
>
>> > where that happens, and so I'm a little worried that the pointer
>>
> might persist
>
>> > after the rfh object is deleted.
>> >
>> > Should the delegate responsibilities go somewhere other than the
>>
> RFH, or else
>
>> > does each RFH need its accessibility manager?
>> >
>> > Unfortunately I don't have a solution ready to suggest, as I don't
>>
> fully grok
>
>> > what powers the BrowserAccessibilityDelegate really needs. So let's
>>
> figure it
>
>> > out together. I'd definitely like nasko's thoughts on this too -- or
>>
> it might
>
>> > help to pick jam's brain.
>>
>
>  This is a great question. I don't think I fully understand the
>>
> implications of
>
>> two
>> RFH's sharing a RVH, but let me explain what requirements we need on
>>
> the
>
>> accessibility side.
>>
>
>  Right now Blink's AX tree spans all of the frames reachable from the
>>
> local page.
>
>> We need one BrowserAccessibilityManager (henceforth BAM) per Blink AX
>>
> tree.
>
>> Prior to OOPIF, the only case where we need two BAMs is when we have a
>>
> <webview>
>
>> tag.
>> Note that <webview> is not accessible now; this refactoring should
>>
> help make
>
>> <webview>
>> accessibility possible.
>>
>
>  My understanding is that eventually there will be a separate instance
>>
> of Blink
>
>> for each
>> frame, even when the frames are from the same origin, right? So
>>
> eventually we'll
>
>> have a
>> separate AX tree for every frame, and we'll definitely need a BAM for
>>
> each
>
>> frame.
>>
>
>  If there's ever a world when there's some mix of both - like some
>>
> frames will be
>
>> handled by the same Blink page, and other frames will be in a separate
>>
> process,
>
>> then
>> we could have a situation where only some frames need a BAM. However,
>>
> one
>
>> simplification we could make would be to modify Blink accessibility so
>>
> that
>
>> there's
>> just one AX tree per frame no matter what. I don't see any downsides
>>
> to this,
>
>> once
>> OOPIF is possible there's no reason not to do this.
>>
>
> I think you have it right. Where possible I think it is best (from a
> design simplicity and maintenance perspective) to arrange things so
> there is, as you say, "one AX tree per frame no matter what". If we do
> that then same-process and cross-process frames work in essentially the
> same way as far as the goo in the browser process is concerned.
>
>
>  OK, so it seems to me that eventually we'll need one BAM per RFH, but
>>
> that in
>
>> the
>> interim there may be some RFHs that don't need a BAM, because their
>>
> parent's
>
>> RFH's
>> BAM already covers that frame. I don't think there's any problem with
>>
> this - the
>
>> Blink
>> AX code will establish which BAM is a descendant of which node of
>>
> another BAM,
>
>> so having an unused BAM in one RFH shouldn't break anything.
>>
>
>  In addition, the *root* BAM needs to be connected to its RWHV, because
>>
> the root
>
>> RWHV will query the BAM when implementing accessibility APIs on its
>>
> view, and
>
>> the BAM needs a bit of information about its containing view in order
>>
> to
>
>> implement
>> native accessibility APIs for web elements.
>>
>
> This makes sense. You're heading in a direction, I think, where the BAMs
> are going to exist one per frame but necessarily somewhat tree-aware. An
> open question in my mind is whether the top level BAM (which is the one
> interacting with a "real" RWHV) will ultimately have the same runtime
> type as the lower level BAMs, or whether it will need a specialized
> subclass or something. The act of coding this up will probably make the
> design clear.
>
>
>  Child BAMs don't really care about their RWHV, which I'm assuming will
>>
> be
>
>> something like
>> a RWHVChildFrame anyway. Child BAMs should know they're children, and
>>
> they
>
>> should
>> only communicate everything up through the root BAM that's the one
>>
> immediately
>
>> contained in some native View.
>>
>
>  BrowserAccessibilityDelegate currently conflates two different needs:
>>
> it needs
>
>> to pass
>> messages to its corresponding render-process frame, and it also needs
>>
> to pass
>
>> information to its native view.
>>
>
>  I'm starting to think that the best solution might be for RFH to own
>>
> the BAM and
>
>> to
>> implement the delegate. Instead of having the RWHV create and own the
>>
> BAM, the
>
>> RFH
>> should just query the RWHV as needed by the BAM.
>>
>
> Makes sense, and I see you've implemented this.
>
>
>  How about this: if the BAM asks the Delegate for, e.g., its native
>>
> view, only
>
>> the RFH for the
>> main frame would return it (by asking its RWHV). Any other RFH would
>>
> return
>
>> NULL, forcing
>> the BAM to pass its message up through its parent BAM.
>>
>
>  Am I correct in my understanding that one WebContents will only have
>>
> one "real"
>
>> RWHV, and
>> any other subframe will only have a RWHVChildFrame or something like
>>
> that?
>
> Yes, this understanding (only one real RWHV in the frame tree) is
> correct.
>
> https://codereview.chromium.org/273423004/diff/250001/
> content/browser/renderer_host/render_widget_host_view_aura.cc
> File content/browser/renderer_host/render_widget_host_view_aura.cc
> (right):
>
> https://codereview.chromium.org/273423004/diff/250001/
> content/browser/renderer_host/render_widget_host_view_aura.cc#newcode1193
> content/browser/renderer_host/render_widget_host_view_aura.cc:1193:
> RenderWidgetHostViewAura::CreateBrowserAccessibilityManager(
> Indentation is off here.
>
> https://codereview.chromium.org/273423004/diff/250001/
> content/browser/renderer_host/render_widget_host_view_base.cc
> File content/browser/renderer_host/render_widget_host_view_base.cc
> (right):
>
> https://codereview.chromium.org/273423004/diff/250001/
> content/browser/renderer_host/render_widget_host_view_base.cc#newcode484
> content/browser/renderer_host/render_widget_host_view_base.cc:484:
> RenderWidgetHostViewBase::CreateBrowserAccessibilityManager(
> Indentation is off.
>
> https://codereview.chromium.org/273423004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2014-05-28 15:03:56]

+jam (please review)

[jam, 2014-05-28 18:49:20]

https://codereview.chromium.org/273423004/diff/250001/chrome/browser/extensio...
File
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc
(right):

https://codereview.chromium.org/273423004/diff/250001/chrome/browser/extensio...
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:99:
rfh->AccessibilityDoDefaultAction(params->args.automation_node_id);
can you explain to me how this id is generated? I tried looking and I got as far
as
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re...
but I didn't know what "this.wrapper.id" is.

https://codereview.chromium.org/273423004/diff/250001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/273423004/diff/250001/content/public/browser/...
content/public/browser/web_contents.h:192: const
base::Callback<void(RenderFrameHost*)>& on_frame) = 0;
2 nits:
-since this is only called in content, it shouldn't be in the public api
(http://www.chromium.org/developers/content-module/content-api)
-since this is only called by WebContentsImpl, also don't add it to
WebContentsImpl's public methods. pending-rvh is a concept that we have tried to
hide to as few classes as possible. so just in-line the two statements in this
method inside SetAccessibilityMode

[dmazzoni, 2014-05-28 18:59:32]

https://codereview.chromium.org/273423004/diff/250001/chrome/browser/extensio...
File
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc
(right):

https://codereview.chromium.org/273423004/diff/250001/chrome/browser/extensio...
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:99:
rfh->AccessibilityDoDefaultAction(params->args.automation_node_id);
On 2014/05/28 18:49:21, jam wrote:
> can you explain to me how this id is generated? I tried looking and I got as
far
> as
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re...
> but I didn't know what "this.wrapper.id" is.

Every accessibility node in Blink has an accessibility-specific node id. An
automation api client receives the accessibility tree and subsequent updates,
just like BrowserAccessibilityManager does. When the automation api client wants
to perform an action on a node (like moving focus), it sends the message back up
to Blink using that id. If the id doesn't exist anymore it's safely ignored.

It's exactly analogous to how BrowserAccessibilityManager calls
AccessibilitySetFocus now; the only difference is that this has to be a public
interface because it's coming from chrome instead of from within content.

[jam, 2014-05-28 21:10:30]

https://codereview.chromium.org/273423004/diff/250001/chrome/browser/extensio...
File
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc
(right):

https://codereview.chromium.org/273423004/diff/250001/chrome/browser/extensio...
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:99:
rfh->AccessibilityDoDefaultAction(params->args.automation_node_id);
On 2014/05/28 18:59:33, dmazzoni wrote:
> On 2014/05/28 18:49:21, jam wrote:
> > can you explain to me how this id is generated? I tried looking and I got as
> far
> > as
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re...
> > but I didn't know what "this.wrapper.id" is.
> 
> Every accessibility node in Blink has an accessibility-specific node id. An
> automation api client receives the accessibility tree and subsequent updates,
> just like BrowserAccessibilityManager does. When the automation api client
wants
> to perform an action on a node (like moving focus), it sends the message back
up
> to Blink using that id. If the id doesn't exist anymore it's safely ignored.
> 
> It's exactly analogous to how BrowserAccessibilityManager calls
> AccessibilitySetFocus now; the only difference is that this has to be a public
> interface because it's coming from chrome instead of from within content.

I see, thanks for the explanation I understand this better now. So because the
IPC is sent from chrome\renderer and it has a per-frame accessibility id,
content will need a unique way of getting to the frame, which is the
render_frame_id/render_process_id (i.e. method on RFH). ok, this part is fine
then; just the other nits and it's good to go.

[dmazzoni, 2014-05-28 22:01:16]

https://codereview.chromium.org/273423004/diff/250001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/273423004/diff/250001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1193:
RenderWidgetHostViewAura::CreateBrowserAccessibilityManager(
On 2014/05/28 00:50:11, ncarter wrote:
> Indentation is off here.

Done.

https://codereview.chromium.org/273423004/diff/250001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/273423004/diff/250001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.cc:484:
RenderWidgetHostViewBase::CreateBrowserAccessibilityManager(
On 2014/05/28 00:50:11, ncarter wrote:
> Indentation is off.

Done.

https://codereview.chromium.org/273423004/diff/250001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/273423004/diff/250001/content/public/browser/...
content/public/browser/web_contents.h:192: const
base::Callback<void(RenderFrameHost*)>& on_frame) = 0;
On 2014/05/28 18:49:21, jam wrote:
> 2 nits:
> -since this is only called in content, it shouldn't be in the public api
> (http://www.chromium.org/developers/content-module/content-api)
> -since this is only called by WebContentsImpl, also don't add it to
> WebContentsImpl's public methods. pending-rvh is a concept that we have tried
to
> hide to as few classes as possible. so just in-line the two statements in this
> method inside SetAccessibilityMode

Done.

[jam, 2014-05-28 23:17:40]

lgtm, i defer to others for the accessibility details

https://codereview.chromium.org/273423004/diff/290001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/273423004/diff/290001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:539: OVERRIDE;
nit: don't start a line with OVERRIDE (if clang format did this, then the fixed
version hasn't been updated yet)

https://codereview.chromium.org/273423004/diff/290001/content/renderer/access...
File content/renderer/accessibility/blink_ax_tree_source.cc (right):

https://codereview.chromium.org/273423004/diff/290001/content/renderer/access...
content/renderer/accessibility/blink_ax_tree_source.cc:556: WebFrame* main_frame
= view ? view->mainFrame() : NULL;
ditto

https://codereview.chromium.org/273423004/diff/290001/content/renderer/access...
File content/renderer/accessibility/renderer_accessibility.cc (right):

https://codereview.chromium.org/273423004/diff/290001/content/renderer/access...
content/renderer/accessibility/renderer_accessibility.cc:33: WebFrame*
main_frame = view ? view->mainFrame() : NULL;
does this make sense anymore now that this class is per-frame? should this just
be "return render_frame_->GetWebFrame()->document()"?

https://codereview.chromium.org/273423004/diff/290001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/273423004/diff/290001/content/renderer/render...
content/renderer/render_view_impl.h:608:
//FRIEND_TEST_ALL_PREFIXES(RenderViewImplTest, OnSetAccessibilityMode);
remove?

[jam, 2014-05-28 23:18:07]

also, thanks for converting this code to work per frame!

[ncarter (slow), 2014-06-25 18:02:59]

On this patch, is there anything I can help with?

[dmazzoni, 2014-06-25 18:35:35]

It's mostly done but a few tests failed and I need to merge in some changes
that happened since I started this. I dropped it to work on some other
priorities for Chrome 37. I will pick this up again after I/O and I'll let
you know. Thanks!



On Wed, Jun 25, 2014 at 11:02 AM, <nick@chromium.org> wrote:

> On this patch, is there anything I can help with?
>
> https://codereview.chromium.org/273423004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2014-07-08 22:02:38]

PTAL.

Try runs are successful! I believe all trybot redness is unrelated. I've also
patched this in and manually tested it on multiple platforms.

Please give this change a final and thorough review!

[David Tseng, 2014-07-14 18:18:58]

Still digging through most of this cl; a few starter comments.

https://codereview.chromium.org/273423004/diff/610001/content/browser/accessi...
File content/browser/accessibility/accessibility_mode_helper.cc (right):

https://codereview.chromium.org/273423004/diff/610001/content/browser/accessi...
content/browser/accessibility/accessibility_mode_helper.cc:5: #include
"base/logging.h"
nit: Unused?

https://codereview.chromium.org/273423004/diff/610001/content/browser/accessi...
File content/browser/accessibility/accessibility_tree_formatter.cc (left):

https://codereview.chromium.org/273423004/diff/610001/content/browser/accessi...
content/browser/accessibility/accessibility_tree_formatter.cc:15: #include
"content/public/browser/render_view_host.h"
nit: unused?

https://codereview.chromium.org/273423004/diff/610001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_manager.h (right):

https://codereview.chromium.org/273423004/diff/610001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_manager.h:50: virtual void
AccessibilityShowMenu(const gfx::Point& point) = 0;
Relative to which view's coords? Why is this changed in this cl?

https://codereview.chromium.org/273423004/diff/610001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_manager_win.cc (right):

https://codereview.chromium.org/273423004/diff/610001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_manager_win.cc:58:
accessible_hwnd_->OnManagerDeleted(this);
|this| is deleted? Does it make sense to reame this method? Otherwise, |this|
seems to be an av if accessed.

https://codereview.chromium.org/273423004/diff/610001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_manager_win.cc:79:
accessible_hwnd_->OnManagerDeleted(this);
This seems like an hidden side effect that might bite us in the future?

[David Tseng, 2014-07-14 19:08:02]

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_delegate.h:122: // Get the
accessibility mode for the WebContents that owns this frame.
this frame delegate's frame?

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_delegate.h:123: virtual
AccessibilityMode GetAccessibilityMode() const;
Does it make sense to have this on the delegate? Shouldn't the delegated object
ask the web contents for the acc mode?

Maybe it should be called when the accessibility mode changes (e.g.
AccessibilityModeChanged) much like the other methods on this delegate?

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_delegate.h:129: #if defined(OS_WIN)
Why only on Win? Do we need anything for Mac as well?

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

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:207: return
accessibility_mode_;
Does each frame get its own accessibility mode? If so (as it seems like it is
here), then the comment in the delegate seems incorrect.

Automation can be adapted to ensure the mode gets set on a per tab (all
subframes would have to be set when encountered).

Do we foresee some time in the future where this would be useful? If not, then I
think we should just keep one accessibility mode on the top level frame?

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:216: // retrieved using
GetAXTreeForTesting().
Why not just pass the ax tree as a param of the callback and get rid of the
other test method?

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:223: const ui::AXTree*
GetAXTreeForTesting();
Not really needed; see above.

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:226:
BrowserAccessibilityManager* browser_accessibility_manager() const {
How do you feel about naming this RenderFrameHostAccessibilityManager (or some
variation of that)?

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:376: // The most recently
received accessibility tree - for testing only.
Are we ok with test only members?

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:378:
scoped_ptr<BrowserAccessibilityManager> browser_accessibility_manager_;
Seems like test members should go last?

https://codereview.chromium.org/273423004/diff/610001/content/browser/rendere...
File content/browser/renderer_host/legacy_render_widget_host_win.cc (right):

https://codereview.chromium.org/273423004/diff/610001/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.cc:75: if (manager_
== manager)
Ok; I see why you needed manager to be passed in here.

I'm just not entirely clear on the ownership relationship here. It seems like
maybe BAM should own LRWHW.

[David Tseng, 2014-07-14 19:46:23]

https://codereview.chromium.org/273423004/diff/610001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/273423004/diff/610001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:380: virtual AccessibilityMode
GetAccessibilityMode() const OVERRIDE;
dups accessibility_mode() above?

[ncarter (slow), 2014-07-15 01:23:29]

lgtm with nits, and they're mostly optional

https://codereview.chromium.org/273423004/diff/610001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_cocoa.mm (right):

https://codereview.chromium.org/273423004/diff/610001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_cocoa.mm:1558:
origin.set_y(origin.y() + size.width / 2);
Should this be size.height instead of .width?

You could also write these 2 lines as:

  origin.Offset(size.width()/2, size.height()/2);

Or maybe even :

  origin = gfx::Rect(origin, gfx::Size(NSSizeToCGSize(size)).CenterPoint();

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

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:865:
std::vector<AXEventNotificationDetails> details;
(maybe) details.reserve(params.size());

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:866: for (unsigned int i =
0; i < params.size(); ++i) {
Use size_t here.

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:888: for (unsigned i = 0; i
< params.size(); i++) {
Use size_t here

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1126: #endif
#endif  // defined(OS_WIN)

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

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:90: int acc_obj_id,
gfx::Rect subfocus) OVERRIDE;
Probably worth passing the gfx::Rect as a const ref.

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:376: // The most recently
received accessibility tree - for testing only.
On 2014/07/14 19:08:02, David Tseng wrote:
> Are we ok with test only members?

I also would rather we not have test-only members, but in this patch the
test-only member is just moving from one spot to another, so I don't consider it
at issue.

https://codereview.chromium.org/273423004/diff/610001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/273423004/diff/610001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:380: virtual AccessibilityMode
GetAccessibilityMode() const OVERRIDE;
On 2014/07/14 19:46:23, David Tseng wrote:
> dups accessibility_mode() above?

It's a sort-of dup (one's virtual the other isn't); there's precedent for having
both and I've made peace with letting both exist.

https://codereview.chromium.org/273423004/diff/610001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/273423004/diff/610001/content/renderer/render...
content/renderer/render_view_impl.h:595:
//FRIEND_TEST_ALL_PREFIXES(RenderViewImplTest, OnSetAccessibilityMode);
Just delete this line?

[dmazzoni, 2014-07-15 07:55:04]

Thanks for the review! I'm going to try to land soon.

https://codereview.chromium.org/273423004/diff/610001/content/browser/accessi...
File content/browser/accessibility/accessibility_mode_helper.cc (right):

https://codereview.chromium.org/273423004/diff/610001/content/browser/accessi...
content/browser/accessibility/accessibility_mode_helper.cc:5: #include
"base/logging.h"
On 2014/07/14 18:18:57, David Tseng wrote:
> nit: Unused?

Actually needed for DCHECK. It was implicitly included before this refactoring.

https://codereview.chromium.org/273423004/diff/610001/content/browser/accessi...
File content/browser/accessibility/accessibility_tree_formatter.cc (left):

https://codereview.chromium.org/273423004/diff/610001/content/browser/accessi...
content/browser/accessibility/accessibility_tree_formatter.cc:15: #include
"content/public/browser/render_view_host.h"
On 2014/07/14 18:18:57, David Tseng wrote:
> nit: unused?

Done.

https://codereview.chromium.org/273423004/diff/610001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_cocoa.mm (right):

https://codereview.chromium.org/273423004/diff/610001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_cocoa.mm:1558:
origin.set_y(origin.y() + size.width / 2);
On 2014/07/15 01:23:28, ncarter wrote:
> Should this be size.height instead of .width?
> 
> You could also write these 2 lines as:
> 
>   origin.Offset(size.width()/2, size.height()/2);
> 
> Or maybe even :
> 
>   origin = gfx::Rect(origin, gfx::Size(NSSizeToCGSize(size)).CenterPoint();

Good catch, thanks!

https://codereview.chromium.org/273423004/diff/610001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_manager.h (right):

https://codereview.chromium.org/273423004/diff/610001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_manager.h:50: virtual void
AccessibilityShowMenu(const gfx::Point& point) = 0;
On 2014/07/14 18:18:57, David Tseng wrote:
> Relative to which view's coords? Why is this changed in this cl?

I changed it to say global_point to clarify.

I changed it in this CL because it became the only reason RenderWidgetHostView
needed to know about BrowserAccessibility. It's cleaner to just ask
RenderWidgetHostView to show the menu at a location rather than having it
indirectly look up the accessible object and show the menu there.

https://codereview.chromium.org/273423004/diff/610001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_manager_win.cc (right):

https://codereview.chromium.org/273423004/diff/610001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_manager_win.cc:58:
accessible_hwnd_->OnManagerDeleted(this);
On 2014/07/14 18:18:57, David Tseng wrote:
> |this| is deleted? Does it make sense to reame this method? Otherwise, |this|
> seems to be an av if accessed.

This is being called from within the destructor, why isn't that safe? It's
basically saying that the accessible hwnd should delete the manager if it's
equal to this, but not if it's already been passed a different manager. This is
necessary for tests to pass.

Here's a follow-up change with a better fix:
https://codereview.chromium.org/387353004/

https://codereview.chromium.org/273423004/diff/610001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_manager_win.cc:79:
accessible_hwnd_->OnManagerDeleted(this);
On 2014/07/14 18:18:57, David Tseng wrote:
> This seems like an hidden side effect that might bite us in the future?

Totally agreed - this is the minimal band-aid to make this work. Addressed by
this follow-up change: https://codereview.chromium.org/387353004/

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_delegate.h:122: // Get the
accessibility mode for the WebContents that owns this frame.
On 2014/07/14 19:08:01, David Tseng wrote:
> this frame delegate's frame?

I'm not sure what you mean? The WebContents is who owns the mode now.

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_delegate.h:123: virtual
AccessibilityMode GetAccessibilityMode() const;
On 2014/07/14 19:08:01, David Tseng wrote:
> Does it make sense to have this on the delegate? Shouldn't the delegated
object
> ask the web contents for the acc mode?
> 
> Maybe it should be called when the accessibility mode changes (e.g.
> AccessibilityModeChanged) much like the other methods on this delegate?

WebContentsImpl is the class that implements RenderFrameHostDelegate. Everything
here is stuff we want to call into WebContentsImpl from RenderFrameHostImpl.

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_delegate.h:129: #if defined(OS_WIN)
On 2014/07/14 19:08:01, David Tseng wrote:
> Why only on Win? Do we need anything for Mac as well?

It's complicated - basically the parent native view isn't always available when
a RWHVA is constructed, so we have to pass it in later. We don't have that issue
on Mac. This isn't new in this change, it's just moving to RFHD.

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

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:865:
std::vector<AXEventNotificationDetails> details;
On 2014/07/15 01:23:28, ncarter wrote:
> (maybe) details.reserve(params.size());

Done.

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:866: for (unsigned int i =
0; i < params.size(); ++i) {
On 2014/07/15 01:23:28, ncarter wrote:
> Use size_t here.

Done.

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:888: for (unsigned i = 0; i
< params.size(); i++) {
On 2014/07/15 01:23:28, ncarter wrote:
> Use size_t here

Done.

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1126: #endif
On 2014/07/15 01:23:28, ncarter wrote:
> #endif  // defined(OS_WIN)

Done.

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

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:90: int acc_obj_id,
gfx::Rect subfocus) OVERRIDE;
On 2014/07/15 01:23:28, ncarter wrote:
> Probably worth passing the gfx::Rect as a const ref.

Done.

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:207: return
accessibility_mode_;
On 2014/07/14 19:08:02, David Tseng wrote:
> Does each frame get its own accessibility mode? If so (as it seems like it is
> here), then the comment in the delegate seems incorrect.
> 
> Automation can be adapted to ensure the mode gets set on a per tab (all
> subframes would have to be set when encountered).
> 
> Do we foresee some time in the future where this would be useful? If not, then
I
> think we should just keep one accessibility mode on the top level frame?

This change already made the WebContents have the only authoritative mode. You
made me realize that there's no reason to cache it here. I replaced the few
places where we used accessibility_mode() or accessibility_mode_ with
delegate_->GetAccessibilityMode().

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:216: // retrieved using
GetAXTreeForTesting().
On 2014/07/14 19:08:02, David Tseng wrote:
> Why not just pass the ax tree as a param of the callback and get rid of the
> other test method?

Often the callback is used by the accessibility waiter, and the ax tree is
retrieved directly by the test.

None of this is changing in this changelist, it's just moving.

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:226:
BrowserAccessibilityManager* browser_accessibility_manager() const {
On 2014/07/14 19:08:02, David Tseng wrote:
> How do you feel about naming this RenderFrameHostAccessibilityManager (or some
> variation of that)?

I was thinking FrameAccessibility or FrameHostAccessibility.

Not in this change, but definitely up for that.

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:376: // The most recently
received accessibility tree - for testing only.
On 2014/07/15 01:23:28, ncarter wrote:
> On 2014/07/14 19:08:02, David Tseng wrote:
> > Are we ok with test only members?
> 
> I also would rather we not have test-only members, but in this patch the
> test-only member is just moving from one spot to another, so I don't consider
it
> at issue.

Yeah, I'm happy to work on ways to do this without test-only members other than
maybe the callback, in a future change.

https://codereview.chromium.org/273423004/diff/610001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:378:
scoped_ptr<BrowserAccessibilityManager> browser_accessibility_manager_;
On 2014/07/14 19:08:02, David Tseng wrote:
> Seems like test members should go last?

Done.

https://codereview.chromium.org/273423004/diff/610001/content/browser/rendere...
File content/browser/renderer_host/legacy_render_widget_host_win.cc (right):

https://codereview.chromium.org/273423004/diff/610001/content/browser/rendere...
content/browser/renderer_host/legacy_render_widget_host_win.cc:75: if (manager_
== manager)
On 2014/07/14 19:08:02, David Tseng wrote:
> Ok; I see why you needed manager to be passed in here.
> 
> I'm just not entirely clear on the ownership relationship here. It seems like
> maybe BAM should own LRWHW.

Yeah, it used to - and then LRWHW ended up being used by RWHVA for other
purposes and that messed up the lifecycle. The change I pointed to earlier will
clean this up.

https://codereview.chromium.org/273423004/diff/610001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/273423004/diff/610001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:380: virtual AccessibilityMode
GetAccessibilityMode() const OVERRIDE;
On 2014/07/15 01:23:28, ncarter wrote:
> On 2014/07/14 19:46:23, David Tseng wrote:
> > dups accessibility_mode() above?
> 
> It's a sort-of dup (one's virtual the other isn't); there's precedent for
having
> both and I've made peace with letting both exist.

We don't need both. I got rid of the non-virtual one, it was only called from a
couple of places.

https://codereview.chromium.org/273423004/diff/610001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/273423004/diff/610001/content/renderer/render...
content/renderer/render_view_impl.h:595:
//FRIEND_TEST_ALL_PREFIXES(RenderViewImplTest, OnSetAccessibilityMode);
On 2014/07/15 01:23:29, ncarter wrote:
> Just delete this line?

Thanks, done.

[dmazzoni, 2014-07-17 06:45:26]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-07-17 06:48:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/273423004/710001

[commit-bot: I haz the power, 2014-07-17 08:54:22]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-07-17 08:58:43]

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

[commit-bot: I haz the power, 2014-07-17 08:58:44]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[dmazzoni, 2014-07-17 15:23:31]

+inferno or jschuh for a quick review of content/common/*messages* - things are
just moving around, there are no new APIs added.

[kenrb, 2014-07-17 18:22:31]

ipc lgtm

[dmazzoni, 2014-07-17 18:31:38]

Thanks!


On Thu, Jul 17, 2014 at 11:22 AM, <kenrb@chromium.org> wrote:

> ipc lgtm
>
>
> https://codereview.chromium.org/273423004/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2014-07-17 21:34:43]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-07-17 21:36:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/273423004/770001

[dmazzoni, 2014-07-17 22:13:00]

The CQ bit was unchecked by dmazzoni@chromium.org

[dmazzoni, 2014-07-17 22:13:39]

CQ is taking a while. I'm going to dcommit later tonight when the tree is less
busy and I can watch it for any errors.

[dmazzoni, 2014-07-18 03:33:28]

Committed patchset #41 manually as r283994 (presubmit successful).

[Mike West, 2014-07-18 07:38:15]

A revert of this CL has been created in
https://codereview.chromium.org/407493002/ by mkwst@chromium.org.

The reason for reverting is: Speculative revert to fix XP bots.

XP bots started crashing
http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/builds...
and
http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%282%29/builds...

This CL is the most suspicious in the range. If it doesn't fix the bots, I'll
unrevert it..

[dmazzoni, 2014-07-23 15:40:57]

Planning to re-land this change shortly, and then land a fix
(https://codereview.chromium.org/415633002/) immediately afterwards to address
the DrMemory-discovered UAF.

[dmazzoni, 2014-07-26 06:06:28]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-07-26 06:07:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/273423004/850001

[commit-bot: I haz the power, 2014-07-26 12:10:24]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-07-26 18:14:43]

Change committed as 285767