One AXObjectCache per frame

One AXObjectCache per frame

Gets rid of AXScrollView, AXScrollbar, and all of the code that
allows AXObjectCache to span multiple frames. Now there's
one AXObjectCache per document, except for page popups
which are part of the same tree.

Also makes a small change to send focus events even
when focus is cleared, which is needed for Chromium to
keep track of focus across frames.

BUG=532249, 484878

[dmazzoni, 2015-09-16 23:49:36]

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

[dmazzoni, 2015-09-16 23:49:37]

Ready for an initial look.

Look how much code we can delete!

Alice, could you review the accessibility code?

Daniel, could you review the core code and also review in the broader context of
out-of-process iframe work?

[dcheng, 2015-09-17 21:54:09]

https://codereview.chromium.org/1348503003/diff/40001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1348503003/diff/40001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2240: return *const_cast<Document*>(this);
If this method can return a mutable ref to this, it probably shouldn't be const?

https://codereview.chromium.org/1348503003/diff/40001/Source/web/ChromeClient...
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/1348503003/diff/40001/Source/web/ChromeClient...
Source/web/ChromeClientImpl.cpp:215: m_webView->client()->focusedFrameChanged();
How come we can't just piggyback off the WebFrameClient notification?

Also, is it possible for WebViewClient to ever be null here?

[dmazzoni, 2015-09-17 22:11:57]

https://codereview.chromium.org/1348503003/diff/40001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1348503003/diff/40001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2240: return *const_cast<Document*>(this);
On 2015/09/17 21:54:08, dcheng wrote:
> If this method can return a mutable ref to this, it probably shouldn't be
const?

Yeah, note that topDocument() is doing a const_cast, which is why this function
worked before. Does that smell bad to you too? Maybe I can try to fix the
const_cast in topDocument first.

https://codereview.chromium.org/1348503003/diff/40001/Source/web/ChromeClient...
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/1348503003/diff/40001/Source/web/ChromeClient...
Source/web/ChromeClientImpl.cpp:215: m_webView->client()->focusedFrameChanged();
On 2015/09/17 21:54:08, dcheng wrote:
> How come we can't just piggyback off the WebFrameClient notification?

That goes to only the frame that got focus.

To avoid ambiguity and race conditions, I've decided to let the main frame send
the accessible notification that the focused frame has changed.

When the frame that gets focus isn't local, |frame| will be null but this
function still gets called. In that case the WebFrameClient notification won't
get called because it's not local.

> Also, is it possible for WebViewClient to ever be null here?

Not that I'm aware of. Other uses of m_webView->client() don't do any null
checks.

[aboxhall, 2015-09-22 21:48:27]

Nothing major, but a couple of things I'm still trying to understand.

https://codereview.chromium.org/1348503003/diff/60001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1348503003/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:3530: if (!focusChangeBlocked) {
Why this change?

https://codereview.chromium.org/1348503003/diff/60001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/1348503003/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.h:1254: 
Blank line?

https://codereview.chromium.org/1348503003/diff/60001/Source/modules/accessib...
File Source/modules/accessibility/AXObjectCacheImpl.cpp (right):

https://codereview.chromium.org/1348503003/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXObjectCacheImpl.cpp:1090: newFocusedNode =
m_document.get();
This seems like a major change. Is this functionally equivalent to what we had
before or is this new?

https://codereview.chromium.org/1348503003/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXObjectCacheImpl.cpp:1094: if
(isHTMLAreaElement(newFocusedNode))
Can you explain why this is now needed?

[dmazzoni, 2015-09-22 22:08:45]

https://codereview.chromium.org/1348503003/diff/60001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1348503003/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:3530: if (!focusChangeBlocked) {
On 2015/09/22 21:48:27, aboxhall wrote:
> Why this change?

A consequence of this change is that when focus moves between frames, we were no
longer getting focus events fired in each frame, but we should have - because
previously when everything was in one big AXObjectCache we just fired an event
on the new node that had focus, no matter what it was.

Now when focus is lost from a frame, m_focusedElement might be null - but we do
want to fire an event, so that Chromium knows that this particular frame doesn't
have a focused element anymore.

https://codereview.chromium.org/1348503003/diff/60001/Source/modules/accessib...
File Source/modules/accessibility/AXObjectCacheImpl.cpp (right):

https://codereview.chromium.org/1348503003/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXObjectCacheImpl.cpp:1090: newFocusedNode =
m_document.get();
On 2015/09/22 21:48:27, aboxhall wrote:
> This seems like a major change. Is this functionally equivalent to what we had
> before or is this new?

This is more or less equivalent.

Internally, a document's active element can be null. From the perspective of
JavaScript, document.activeElement returns the body if nothing has focus. We're
just doing the same thing now, firing a focus event on the body if nothing
within a particular frame has focus.

This wasn't needed before because we could detect when nothing in the page had
focus at all from Chromium code. That trick won't work anymore because we need
to know what's focused in every single frame now.

https://codereview.chromium.org/1348503003/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXObjectCacheImpl.cpp:1094: if
(isHTMLAreaElement(newFocusedNode))
On 2015/09/22 21:48:27, aboxhall wrote:
> Can you explain why this is now needed?

This is just coming from AXObjectCacheImpl::focusedUIElementForPage. When an
AREA element has focus, we set accessibility focus to the element within the
image map that has focus. It's kind of like active descendant. This isn't new,
we just have to do that check in this place now.

[aboxhall, 2015-09-23 16:38:45]

LGTM

Thanks for the explanations!

[dmazzoni, 2015-09-30 22:53:18]

As discussed offline with dcheng@, I'll modify this to get the focused
frame from the frame tree in the browser process, rather than having
focusedFrameChanged() in WebViewClient.

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

[dmazzoni, 2016-03-03 08:43:01]

Superceded by: https://codereview.chromium.org/1761633002/