One accessibility tree per frame.

One accessibility tree per frame.

Historically WebKit had a single accessibility tree that spanned all
frames. That simplified a lot of things like focus tracking.

The introduction of local and remote frames to support out-of-process
iframes and <webview> have added back a lot of that complexity. Having
accessibility trees span some frames, but not necessarily all of them,
means there were a lot of special cases to test.

To simplify things, this patch makes it so that we now have exactly one
accessibility tree per frame.

On the Blink side that means one AXObjectCache per document, rather
than trying to attach it to the topDocument. The only exception is a
page popup, we'll continue to use the accessibility tree of the
pagePopupOwner since it will always be local.

On the Chromium side we'll have one BrowserAccessibilityManager per
frame. The focused frame is now retrieved directly from the FrameTree,
which solves several tricky issues that were previously race conditions
when moving focus between a frame and a cross-process frame.

Depends on:
https://codereview.chromium.org/1762143002/

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

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

[dmazzoni, 2016-03-02 23:04:11]

Description was changed from

==========
One accessibility tree per frame.

Historically WebKit had a single accessibility tree that spanned all
frames. That simplified a lot of things like focus tracking.

The introduction of local and remote frames to support out-of-process
iframes and <webview> have added back a lot of that complexity. Having
accessibility trees span some frames, but not necessarily all of them,
means there were a lot of special cases to test.

To simplify things, this patch makes it so that we now have exactly one
accessibility tree per frame.

On the Blink side that means one AXObjectCache per document, rather
than trying to attach it to the topDocument. The only exception is a
page popup, we'll continue to use the accessibility tree of the
pagePopupOwner since it will always be local.

On the Chromium side we'll have one BrowserAccessibilityManager per
frame. The focused frame is now retrieved directly from the FrameTree,
which should solve several tricky OOPIF corner cases.

BUG=532249
==========

to

==========
One accessibility tree per frame.

Historically WebKit had a single accessibility tree that spanned all
frames. That simplified a lot of things like focus tracking.

The introduction of local and remote frames to support out-of-process
iframes and <webview> have added back a lot of that complexity. Having
accessibility trees span some frames, but not necessarily all of them,
means there were a lot of special cases to test.

To simplify things, this patch makes it so that we now have exactly one
accessibility tree per frame.

On the Blink side that means one AXObjectCache per document, rather
than trying to attach it to the topDocument. The only exception is a
page popup, we'll continue to use the accessibility tree of the
pagePopupOwner since it will always be local.

On the Chromium side we'll have one BrowserAccessibilityManager per
frame. The focused frame is now retrieved directly from the FrameTree,
which should solve several tricky OOPIF corner cases.

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

[dmazzoni, 2016-03-04 00:46:36]

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

[dmazzoni, 2016-03-04 07:05:49]

Description was changed from

==========
One accessibility tree per frame.

Historically WebKit had a single accessibility tree that spanned all
frames. That simplified a lot of things like focus tracking.

The introduction of local and remote frames to support out-of-process
iframes and <webview> have added back a lot of that complexity. Having
accessibility trees span some frames, but not necessarily all of them,
means there were a lot of special cases to test.

To simplify things, this patch makes it so that we now have exactly one
accessibility tree per frame.

On the Blink side that means one AXObjectCache per document, rather
than trying to attach it to the topDocument. The only exception is a
page popup, we'll continue to use the accessibility tree of the
pagePopupOwner since it will always be local.

On the Chromium side we'll have one BrowserAccessibilityManager per
frame. The focused frame is now retrieved directly from the FrameTree,
which should solve several tricky OOPIF corner cases.

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

to

==========
One accessibility tree per frame.

Historically WebKit had a single accessibility tree that spanned all
frames. That simplified a lot of things like focus tracking.

The introduction of local and remote frames to support out-of-process
iframes and <webview> have added back a lot of that complexity. Having
accessibility trees span some frames, but not necessarily all of them,
means there were a lot of special cases to test.

To simplify things, this patch makes it so that we now have exactly one
accessibility tree per frame.

On the Blink side that means one AXObjectCache per document, rather
than trying to attach it to the topDocument. The only exception is a
page popup, we'll continue to use the accessibility tree of the
pagePopupOwner since it will always be local.

On the Chromium side we'll have one BrowserAccessibilityManager per
frame. The focused frame is now retrieved directly from the FrameTree,
which solves several tricky issues that were previously race conditions
when moving focus between a frame and a cross-process frame.

https://codereview.chromium.org/1762143002/

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

[dmazzoni, 2016-03-04 07:06:09]

Description was changed from

==========
One accessibility tree per frame.

Historically WebKit had a single accessibility tree that spanned all
frames. That simplified a lot of things like focus tracking.

The introduction of local and remote frames to support out-of-process
iframes and <webview> have added back a lot of that complexity. Having
accessibility trees span some frames, but not necessarily all of them,
means there were a lot of special cases to test.

To simplify things, this patch makes it so that we now have exactly one
accessibility tree per frame.

On the Blink side that means one AXObjectCache per document, rather
than trying to attach it to the topDocument. The only exception is a
page popup, we'll continue to use the accessibility tree of the
pagePopupOwner since it will always be local.

On the Chromium side we'll have one BrowserAccessibilityManager per
frame. The focused frame is now retrieved directly from the FrameTree,
which solves several tricky issues that were previously race conditions
when moving focus between a frame and a cross-process frame.

https://codereview.chromium.org/1762143002/

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

to

==========
One accessibility tree per frame.

Historically WebKit had a single accessibility tree that spanned all
frames. That simplified a lot of things like focus tracking.

The introduction of local and remote frames to support out-of-process
iframes and <webview> have added back a lot of that complexity. Having
accessibility trees span some frames, but not necessarily all of them,
means there were a lot of special cases to test.

To simplify things, this patch makes it so that we now have exactly one
accessibility tree per frame.

On the Blink side that means one AXObjectCache per document, rather
than trying to attach it to the topDocument. The only exception is a
page popup, we'll continue to use the accessibility tree of the
pagePopupOwner since it will always be local.

On the Chromium side we'll have one BrowserAccessibilityManager per
frame. The focused frame is now retrieved directly from the FrameTree,
which solves several tricky issues that were previously race conditions
when moving focus between a frame and a cross-process frame.

Depends on:
https://codereview.chromium.org/1762143002/

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

[David Tseng, 2016-03-04 16:50:21]

I reviewed mostly the chrome browser changes.

https://codereview.chromium.org/1761633002/diff/180001/chrome/renderer/extens...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/1761633002/diff/180001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:145:
ui::AXNode* FindNodeWithChildTreeId(ui::AXNode* node, int child_tree_id) {
Would it make sense to have the AXTree maintain a map from child tree id to a
node id?

https://codereview.chromium.org/1761633002/diff/180001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:907:
TreeCache** cache) {
out_cache

https://codereview.chromium.org/1761633002/diff/180001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:912: if
(parent_tree_id <= 0)
Doesn't the desktop tree have id 0?

https://codereview.chromium.org/1761633002/diff/180001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:916: if
(!parent_cache)
Should |cache| be set to nullptr?

https://codereview.chromium.org/1761633002/diff/180001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:937: if
(parent) {
Is it an error if |parent| is null?

https://codereview.chromium.org/1761633002/diff/180001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/automation/sites/iframe_outer.html
(right):

https://codereview.chromium.org/1761633002/diff/180001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/automation/sites/iframe_outer.html:8:
iframe.src = childUrl;
Is this necessary? (relative paths so you could just in-line the src=
attribute).

https://codereview.chromium.org/1761633002/diff/180001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/automation/tests/desktop/focus_iframe.js
(right):

https://codereview.chromium.org/1761633002/diff/180001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/automation/tests/desktop/focus_iframe.js:35:
];
Could you also add a test for tree walking (out and into a frame)?

https://codereview.chromium.org/1761633002/diff/180001/content/browser/access...
File content/browser/accessibility/accessibility_tree_formatter_blink.cc
(right):

https://codereview.chromium.org/1761633002/diff/180001/content/browser/access...
content/browser/accessibility/accessibility_tree_formatter_blink.cc:115:
dict.GetString("internalRole", &role_value);
Why this change?

https://codereview.chromium.org/1761633002/diff/180001/content/browser/access...
File content/browser/accessibility/browser_accessibility_android.cc (left):

https://codereview.chromium.org/1761633002/diff/180001/content/browser/access...
content/browser/accessibility/browser_accessibility_android.cc:305: case
ui::AX_ROLE_ROOT_WEB_AREA:
So, AX_ROLE_WEB_AREA is no longer used?

https://codereview.chromium.org/1761633002/diff/180001/content/browser/access...
File content/browser/accessibility/browser_accessibility_android.cc (right):

https://codereview.chromium.org/1761633002/diff/180001/content/browser/access...
content/browser/accessibility/browser_accessibility_android.cc:947: } else if
(GetRole() == ui::AX_ROLE_ROOT_WEB_AREA) {
&& GetParent()

https://codereview.chromium.org/1761633002/diff/180001/content/browser/access...
File content/browser/accessibility/browser_accessibility_manager.cc (right):

https://codereview.chromium.org/1761633002/diff/180001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager.cc:193:
!focus->manager()->GetTreeData().loaded) {
Why do we want to exclude focus for this case?

https://codereview.chromium.org/1761633002/diff/180001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager.cc:200:
last_focused_node_ = focus;
This would set nullptr for the conditions above. Is that right?

https://codereview.chromium.org/1761633002/diff/180001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager.cc:201:
last_focused_manager_ = focus ? focus->manager() : nullptr;
Why not remove the FocusManager member pointer and just get it from the last
focused node when we need it?

https://codereview.chromium.org/1761633002/diff/180001/content/browser/access...
File content/browser/accessibility/browser_accessibility_manager_win.cc (left):

https://codereview.chromium.org/1761633002/diff/180001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager_win.cc:43:
inside_on_window_focused_(false) {
Looks like some amount of event bookkeeping went on; should Nektarios try this
patch out?

https://codereview.chromium.org/1761633002/diff/180001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager_win.cc:228:
!node->manager()->GetTreeData().loaded) {
I see; you bubble this up? Is this needed only for NVDA?

https://codereview.chromium.org/1761633002/diff/180001/content/renderer/acces...
File content/renderer/accessibility/blink_ax_tree_source.cc (right):

https://codereview.chromium.org/1761633002/diff/180001/content/renderer/acces...
content/renderer/accessibility/blink_ax_tree_source.cc:165: // Get the tree ID
for this frame the parent frame.
nit remove: the parent frame

[dmazzoni, 2016-03-07 21:35:32]

I owe you a test of walking in and out of an iframe, but otherwise all feedback
is addressed.

https://codereview.chromium.org/1761633002/diff/180001/chrome/renderer/extens...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/1761633002/diff/180001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:145:
ui::AXNode* FindNodeWithChildTreeId(ui::AXNode* node, int child_tree_id) {
On 2016/03/04 16:50:20, David Tseng wrote:
> Would it make sense to have the AXTree maintain a map from child tree id to a
> node id?

I debated that but I'm not convinced it would actually be more efficient. It'd
require checking for a CHILD_TREE_ID attribute every time a node is updated,
which is pretty frequent. The solution I have now is that the child tree
searches for it once and then caches it, potentially for a long time.

https://codereview.chromium.org/1761633002/diff/180001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:907:
TreeCache** cache) {
On 2016/03/04 16:50:20, David Tseng wrote:
> out_cache

Made it in_out_cache

https://codereview.chromium.org/1761633002/diff/180001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:912: if
(parent_tree_id <= 0)
On 2016/03/04 16:50:20, David Tseng wrote:
> Doesn't the desktop tree have id 0?

Done.

https://codereview.chromium.org/1761633002/diff/180001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:916: if
(!parent_cache)
On 2016/03/04 16:50:20, David Tseng wrote:
> Should |cache| be set to nullptr?

No, because it's an input and output parameter

https://codereview.chromium.org/1761633002/diff/180001/chrome/renderer/extens...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:937: if
(parent) {
On 2016/03/04 16:50:20, David Tseng wrote:
> Is it an error if |parent| is null? 

No, it's possible the child frame is loaded before the parent frame, or the
parent frame was destroyed and the child frame hasn't been destroyed yet. Those
aren't fatal errors.

https://codereview.chromium.org/1761633002/diff/180001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/automation/sites/iframe_outer.html
(right):

https://codereview.chromium.org/1761633002/diff/180001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/automation/sites/iframe_outer.html:8:
iframe.src = childUrl;
On 2016/03/04 16:50:20, David Tseng wrote:
> Is this necessary? (relative paths so you could just in-line the src=
> attribute).

Ah yes, much simpler.

https://codereview.chromium.org/1761633002/diff/180001/content/browser/access...
File content/browser/accessibility/accessibility_tree_formatter_blink.cc
(right):

https://codereview.chromium.org/1761633002/diff/180001/content/browser/access...
content/browser/accessibility/accessibility_tree_formatter_blink.cc:115:
dict.GetString("internalRole", &role_value);
On 2016/03/04 16:50:20, David Tseng wrote:
> Why this change?

It conflicted with the string attribute "role" (which stores the ARIA role as
specified by the user). This was the first case where those didn't happen to
result in printing the same string in a test.

https://codereview.chromium.org/1761633002/diff/180001/content/browser/access...
File content/browser/accessibility/browser_accessibility_android.cc (left):

https://codereview.chromium.org/1761633002/diff/180001/content/browser/access...
content/browser/accessibility/browser_accessibility_android.cc:305: case
ui::AX_ROLE_ROOT_WEB_AREA:
On 2016/03/04 16:50:20, David Tseng wrote:
> So, AX_ROLE_WEB_AREA is no longer used?

Yes. I was planning to delete it in a follow-up change, but that would touch a
lot of files so I didn't want to do it here.

https://codereview.chromium.org/1761633002/diff/180001/content/browser/access...
File content/browser/accessibility/browser_accessibility_android.cc (right):

https://codereview.chromium.org/1761633002/diff/180001/content/browser/access...
content/browser/accessibility/browser_accessibility_android.cc:947: } else if
(GetRole() == ui::AX_ROLE_ROOT_WEB_AREA) {
On 2016/03/04 16:50:20, David Tseng wrote:
> && GetParent()

Done.

https://codereview.chromium.org/1761633002/diff/180001/content/browser/access...
File content/browser/accessibility/browser_accessibility_manager.cc (right):

https://codereview.chromium.org/1761633002/diff/180001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager.cc:193:
!focus->manager()->GetTreeData().loaded) {
On 2016/03/04 16:50:20, David Tseng wrote:
> Why do we want to exclude focus for this case?

I moved this from Windows-specific code to cross-platform code, it's otherwise
unchanged. It confused NVDA in particular. It seems like not a bad idea in
general to wait until the page loads at least a tiny bit of content before
focusing it.

https://codereview.chromium.org/1761633002/diff/180001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager.cc:200:
last_focused_node_ = focus;
On 2016/03/04 16:50:20, David Tseng wrote:
> This would set nullptr for the conditions above. Is that right?

Yes, definitely intentional. Every single event we receive or every time the
window focus state changes, we call FireFocusEventsIfNeeded and that means we'll
know we haven't yet fired a focus event within this window yet and one is
needed.

https://codereview.chromium.org/1761633002/diff/180001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager.cc:201:
last_focused_manager_ = focus ? focus->manager() : nullptr;
On 2016/03/04 16:50:20, David Tseng wrote:
> Why not remove the FocusManager member pointer and just get it from the last
> focused node when we need it?

Because we don't have an easy way of clearing last_focused_node_ when a node is
destroyed, so last_focused_node_ is not dereferencable. The comment in the
header notes that it shouldn't be dereferenced.

I guess an alternative would be a WeakPtr, would you prefer that or is this
okay?

https://codereview.chromium.org/1761633002/diff/180001/content/browser/access...
File content/browser/accessibility/browser_accessibility_manager_win.cc (left):

https://codereview.chromium.org/1761633002/diff/180001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager_win.cc:43:
inside_on_window_focused_(false) {
On 2016/03/04 16:50:20, David Tseng wrote:
> Looks like some amount of event bookkeeping went on; should Nektarios try this
> patch out?

Yes, I'll ask him to try it. I did a lot of testing with NVDA and I'm pretty
sure this is an improvement. The previous bookkeeping was overly complicated and
probably wrong for some corner cases; the new approach seems simpler and more
foolproof to me, and achieves the same effect.

https://codereview.chromium.org/1761633002/diff/180001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager_win.cc:228:
!node->manager()->GetTreeData().loaded) {
On 2016/03/04 16:50:20, David Tseng wrote:
> I see; you bubble this up? Is this needed only for NVDA?

Added for NVDA, but seems useful in general as it slightly delays the first
focus event until after at least a tiny bit of content loads. The loaded check
ensures that it still works fine for an empty page, it will still fire the focus
event then.

https://codereview.chromium.org/1761633002/diff/180001/content/renderer/acces...
File content/renderer/accessibility/blink_ax_tree_source.cc (right):

https://codereview.chromium.org/1761633002/diff/180001/content/renderer/acces...
content/renderer/accessibility/blink_ax_tree_source.cc:165: // Get the tree ID
for this frame the parent frame.
On 2016/03/04 16:50:21, David Tseng wrote:
> nit remove: the parent frame

Changed to "and the parent frame", which is what I meant.

[dmazzoni, 2016-03-07 23:12:39]

https://codereview.chromium.org/1761633002/diff/180001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/automation/tests/desktop/focus_iframe.js
(right):

https://codereview.chromium.org/1761633002/diff/180001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/automation/tests/desktop/focus_iframe.js:35:
];
On 2016/03/04 16:50:20, David Tseng wrote:
> Could you also add a test for tree walking (out and into a frame)?

Done now. I had to make a change to AutomationPredicate.container, let me know
if that doesn't look right.

[David Tseng, 2016-03-08 00:20:06]

lgtm

[dcheng, 2016-03-08 01:14:27]

1) Does this mean we can get rid of things like
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
Or do we still need to pre-emptively clear it when the frame is going away?

2) While I was looking at some other uses of topDocument() and AX stuff, I came
across this:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....
This was added in https://codereview.chromium.org/286143008. Maybe we can just
check document->view() now? (this can be in a separate patch since it's not
really related at all to this patch)

https://codereview.chromium.org/1761633002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1761633002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:2323: Document* top =
const_cast<Document*>(this);
Document* doc?

https://codereview.chromium.org/1761633002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:2324: if (top->frame() &&
top->frame()->pagePopupOwner()) {
Part of me wonders if this warrants a comment of some sort. Page popups are kind
of weird and poorly documented.

[dmazzoni, 2016-03-08 18:13:19]

On 2016/03/08 01:14:27, dcheng wrote:
> 1) Does this mean we can get rid of things like
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> Or do we still need to pre-emptively clear it when the frame is going away?

Good call, I don't think we need that at all now. Removed.

I also removed topDocument from HTMLDialogElement, and got rid of another
place we cleared the axObjectCache from FrameView.

> 2) While I was looking at some other uses of topDocument() and AX stuff, I
came
> across this:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....
> This was added in https://codereview.chromium.org/286143008. Maybe we can just
> check document->view() now? (this can be in a separate patch since it's not
> really related at all to this patch)

Good idea, done.

[dmazzoni, 2016-03-08 18:51:04]

https://codereview.chromium.org/1761633002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1761633002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:2323: Document* top =
const_cast<Document*>(this);
On 2016/03/08 01:14:27, dcheng wrote:
> Document* doc?

Done.

https://codereview.chromium.org/1761633002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:2324: if (top->frame() &&
top->frame()->pagePopupOwner()) {
On 2016/03/08 01:14:27, dcheng wrote:
> Part of me wonders if this warrants a comment of some sort. Page popups are
kind
> of weird and poorly documented.

Done.

[dcheng, 2016-03-08 20:08:29]

LGTM!

\(^o^)/

[dmazzoni, 2016-03-08 20:30:59]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-03-08 20:31:00]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1761633002/#ps260001
(title: "Fix one Mac test expectation")

[commit-bot: I haz the power, 2016-03-08 20:31:32]

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

[commit-bot: I haz the power, 2016-03-08 20:43:42]

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

[commit-bot: I haz the power, 2016-03-08 20:43:43]

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

[dmazzoni, 2016-03-08 20:47:40]

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

[dmazzoni, 2016-03-08 20:47:41]

+nasko for owners review of:
content/browser/frame_host
content/browser/renderer_host

[nasko, 2016-03-09 22:44:37]

content/browser/*_host LGTM with a nit.

https://codereview.chromium.org/1761633002/diff/260001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1761633002/diff/260001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2671: if (!focused_frame)
Is this even possible? I think we can't have a frame, which can be focused,
which doesn't have any document associated with it.

[dmazzoni, 2016-03-09 22:55:29]

https://codereview.chromium.org/1761633002/diff/260001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1761633002/diff/260001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2671: if (!focused_frame)
On 2016/03/09 22:44:37, nasko (slow) wrote:
> Is this even possible? I think we can't have a frame, which can be focused,
> which doesn't have any document associated with it.

Good call. I'll change it to DCHECK.

[dmazzoni, 2016-03-10 04:52:08]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-03-10 04:52:09]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org,
dcheng@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1761633002/#ps300001
(title: "Fix is-richly-editable test")

[commit-bot: I haz the power, 2016-03-10 04:52:34]

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

[commit-bot: I haz the power, 2016-03-10 05:45:22]

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

[commit-bot: I haz the power, 2016-03-10 05:45:24]

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

[dmazzoni, 2016-03-10 06:36:46]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-03-10 06:36:58]

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

[commit-bot: I haz the power, 2016-03-10 09:06:44]

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

[commit-bot: I haz the power, 2016-03-10 09:06:46]

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

[dcheng, 2016-03-10 09:26:39]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2016-03-10 09:26:50]

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

[commit-bot: I haz the power, 2016-03-10 11:29:00]

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

[commit-bot: I haz the power, 2016-03-10 11:29:02]

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

[dmazzoni, 2016-03-10 14:44:01]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-03-10 14:44:28]

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

[commit-bot: I haz the power, 2016-03-10 15:52:08]

Description was changed from

==========
One accessibility tree per frame.

Historically WebKit had a single accessibility tree that spanned all
frames. That simplified a lot of things like focus tracking.

The introduction of local and remote frames to support out-of-process
iframes and <webview> have added back a lot of that complexity. Having
accessibility trees span some frames, but not necessarily all of them,
means there were a lot of special cases to test.

To simplify things, this patch makes it so that we now have exactly one
accessibility tree per frame.

On the Blink side that means one AXObjectCache per document, rather
than trying to attach it to the topDocument. The only exception is a
page popup, we'll continue to use the accessibility tree of the
pagePopupOwner since it will always be local.

On the Chromium side we'll have one BrowserAccessibilityManager per
frame. The focused frame is now retrieved directly from the FrameTree,
which solves several tricky issues that were previously race conditions
when moving focus between a frame and a cross-process frame.

Depends on:
https://codereview.chromium.org/1762143002/

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

to

==========
One accessibility tree per frame.

Historically WebKit had a single accessibility tree that spanned all
frames. That simplified a lot of things like focus tracking.

The introduction of local and remote frames to support out-of-process
iframes and <webview> have added back a lot of that complexity. Having
accessibility trees span some frames, but not necessarily all of them,
means there were a lot of special cases to test.

To simplify things, this patch makes it so that we now have exactly one
accessibility tree per frame.

On the Blink side that means one AXObjectCache per document, rather
than trying to attach it to the topDocument. The only exception is a
page popup, we'll continue to use the accessibility tree of the
pagePopupOwner since it will always be local.

On the Chromium side we'll have one BrowserAccessibilityManager per
frame. The focused frame is now retrieved directly from the FrameTree,
which solves several tricky issues that were previously race conditions
when moving focus between a frame and a cross-process frame.

Depends on:
https://codereview.chromium.org/1762143002/

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

[commit-bot: I haz the power, 2016-03-10 15:52:11]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-03-10 15:53:51]

Description was changed from

==========
One accessibility tree per frame.

Historically WebKit had a single accessibility tree that spanned all
frames. That simplified a lot of things like focus tracking.

The introduction of local and remote frames to support out-of-process
iframes and <webview> have added back a lot of that complexity. Having
accessibility trees span some frames, but not necessarily all of them,
means there were a lot of special cases to test.

To simplify things, this patch makes it so that we now have exactly one
accessibility tree per frame.

On the Blink side that means one AXObjectCache per document, rather
than trying to attach it to the topDocument. The only exception is a
page popup, we'll continue to use the accessibility tree of the
pagePopupOwner since it will always be local.

On the Chromium side we'll have one BrowserAccessibilityManager per
frame. The focused frame is now retrieved directly from the FrameTree,
which solves several tricky issues that were previously race conditions
when moving focus between a frame and a cross-process frame.

Depends on:
https://codereview.chromium.org/1762143002/

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

to

==========
One accessibility tree per frame.

Historically WebKit had a single accessibility tree that spanned all
frames. That simplified a lot of things like focus tracking.

The introduction of local and remote frames to support out-of-process
iframes and <webview> have added back a lot of that complexity. Having
accessibility trees span some frames, but not necessarily all of them,
means there were a lot of special cases to test.

To simplify things, this patch makes it so that we now have exactly one
accessibility tree per frame.

On the Blink side that means one AXObjectCache per document, rather
than trying to attach it to the topDocument. The only exception is a
page popup, we'll continue to use the accessibility tree of the
pagePopupOwner since it will always be local.

On the Chromium side we'll have one BrowserAccessibilityManager per
frame. The focused frame is now retrieved directly from the FrameTree,
which solves several tricky issues that were previously race conditions
when moving focus between a frame and a cross-process frame.

Depends on:
https://codereview.chromium.org/1762143002/

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

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

[commit-bot: I haz the power, 2016-03-10 15:53:52]

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