Implement initial support for nesting one ax tree in another.

Implement initial support for nesting one ax tree in another.

This will be used to implement support for out-of-process iframes
and <webview> accessibility.

BUG=368298, 330307

[David Tseng, 2014-05-01 18:07:53]

First pass; I'll continue after lunch.

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

https://codereview.chromium.org/252253002/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.cc:31: child_frame_id_(0) {
Maybe the id should be a uint32?

https://codereview.chromium.org/252253002/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.cc:68: if (!child_frame_id_)
So, 0 means no iframe? Would it make sense to define that as a named constant?

https://codereview.chromium.org/252253002/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.cc:93: if (child_index == 0
&& child_frame_id_ != 0) {
A named constant would help the readability of this statement.

https://codereview.chromium.org/252253002/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.cc:93: if (child_index == 0
&& child_frame_id_ != 0) {
Crazy question...but can an iframe element have children besides another
document? (e.g. scroll bars, groups, etc).

https://codereview.chromium.org/252253002/diff/1/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility.h (left):

https://codereview.chromium.org/252253002/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.h:265: private:
nit: I think the above private: specifier suffices?

https://codereview.chromium.org/252253002/diff/1/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility.h (right):

https://codereview.chromium.org/252253002/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.h:248: // For an iframe
element
nit: period and perhaps "if this element is an iframe"?

https://codereview.chromium.org/252253002/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.h:249: void
SetChildFrameId(int child_frame_id);
nit: perhaps ID?

Also, on a higher level, child frame ID to me means one child frame's id. What
about other child iframes? The naming can be confusing especially as this is the
first file I'm reviewing.

https://codereview.chromium.org/252253002/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.h:268: int child_frame_id_;
nit: did you want these sorted?

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

https://codereview.chromium.org/252253002/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_manager.h:73: : public
ui::AXTreeDelegate {
nit: why was this changed? I think it still fits on the previous line.

https://codereview.chromium.org/252253002/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_manager.h:255: int32
parent_frame_frame_id_;
I would split this comment into separate ones for each field since they are ids
for different set of objects.

[dmazzoni, 2014-05-05 07:17:56]

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

https://codereview.chromium.org/252253002/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.cc:31: child_frame_id_(0) {
On 2014/05/01 18:07:54, David Tseng wrote:
> Maybe the id should be a uint32?

Done.

https://codereview.chromium.org/252253002/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.cc:68: if (!child_frame_id_)
On 2014/05/01 18:07:54, David Tseng wrote:
> So, 0 means no iframe? Would it make sense to define that as a named constant?

Done.

https://codereview.chromium.org/252253002/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.cc:93: if (child_index == 0
&& child_frame_id_ != 0) {
On 2014/05/01 18:07:54, David Tseng wrote:
> A named constant would help the readability of this statement.

Done.

https://codereview.chromium.org/252253002/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.cc:93: if (child_index == 0
&& child_frame_id_ != 0) {
On 2014/05/01 18:07:54, David Tseng wrote:
> Crazy question...but can an iframe element have children besides another
> document? (e.g. scroll bars, groups, etc).

No - but even if that were the case that shouldn't really matter, in Blink we
could just add a mock node in the middle. On the browser side we really don't
need to care about what these nodes are, we just replace any children with a
link to the root node of another tree.

Anything that says iframe here should be illustrative - the implementation here
should be more general than that and should apply to any way to embed one tree
in another, like <webview>.

https://codereview.chromium.org/252253002/diff/1/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility.h (left):

https://codereview.chromium.org/252253002/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.h:265: private:
On 2014/05/01 18:07:54, David Tseng wrote:
> nit: I think the above private: specifier suffices?

Done.

https://codereview.chromium.org/252253002/diff/1/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility.h (right):

https://codereview.chromium.org/252253002/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.h:248: // For an iframe
element
On 2014/05/01 18:07:54, David Tseng wrote:
> nit: period and perhaps "if this element is an iframe"?

Done.

https://codereview.chromium.org/252253002/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.h:249: void
SetChildFrameId(int child_frame_id);
On 2014/05/01 18:07:54, David Tseng wrote:
> nit: perhaps ID?

Id is consistent with the rest of this file; I have no strong preference if you
want to switch to ID throughout all accessibility files.

> Also, on a higher level, child frame ID to me means one child frame's id. What
> about other child iframes? The naming can be confusing especially as this is
the
> first file I'm reviewing.

I tried to clarify the comment a lot, see if it's more clear nw.

https://codereview.chromium.org/252253002/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.h:268: int child_frame_id_;
On 2014/05/01 18:07:54, David Tseng wrote:
> nit: did you want these sorted?

I don't want to sort unrelated fields.

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

https://codereview.chromium.org/252253002/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_manager.h:73: : public
ui::AXTreeDelegate {
On 2014/05/01 18:07:54, David Tseng wrote:
> nit: why was this changed? I think it still fits on the previous line.

Accident. Thanks.

https://codereview.chromium.org/252253002/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_manager.h:255: int32
parent_frame_frame_id_;
On 2014/05/01 18:07:54, David Tseng wrote:
> I would split this comment into separate ones for each field since they are
ids
> for different set of objects.

Done.

[aboxhall, 2014-05-06 15:41:27]

https://codereview.chromium.org/252253002/diff/20001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/252253002/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility.cc:68: if (child_frame_id_
!= kNoFrameId)
Should this be == ?

https://codereview.chromium.org/252253002/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility.cc:93: if (child_index == 0
&& child_frame_id_ != kNoFrameId) {
Perhaps the order of these two cases should be reversed to match
PlatformChildCount.

https://codereview.chromium.org/252253002/diff/20001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_manager.h (right):

https://codereview.chromium.org/252253002/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager.h:255: uint32
parent_frame_frame_id_;
I'm wondering whether this and the below should be a struct. Possibly not, just
an idea.

https://codereview.chromium.org/252253002/diff/20001/content/browser/accessib...
File content/browser/accessibility/frame_tree_accessibility.cc (right):

https://codereview.chromium.org/252253002/diff/20001/content/browser/accessib...
content/browser/accessibility/frame_tree_accessibility.cc:42: if
(frame_to_id_.find(frame) == frame_to_id_.end())
How could this happen? Is it an error?

https://codereview.chromium.org/252253002/diff/20001/content/browser/accessib...
File content/browser/accessibility/frame_tree_accessibility.h (right):

https://codereview.chromium.org/252253002/diff/20001/content/browser/accessib...
content/browser/accessibility/frame_tree_accessibility.h:25: }  // namespace
__gnu_cxx
BASE_HASH_NAMESPACE?