[DevTools] Accessibility: Show siblings and children of selected node

Show siblings and children of selected node

BUG=560525
Committed: https://crrev.com/3a46cc72d58945804dd4919a679a8ce26690bfc0
Cr-Commit-Position: refs/heads/master@{#431304}

[aboxhall, 2016-10-19 00:08:52]

Description was changed from

==========
Show siblings and children of selected node

BUG=
==========

to

==========
Show siblings and children of selected node

BUG=560525
==========

[aboxhall, 2016-10-19 00:08:52]

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

[aboxhall, 2016-10-19 00:09:26]

Ready for a first look. See bug for screenshots.

Suggestions welcome on how/if I might factor this into smaller changes!

[aboxhall, 2016-10-19 00:26:03]

On 2016/10/19 00:09:26, aboxhall wrote:
> Ready for a first look. See bug for screenshots.
> 
> Suggestions welcome on how/if I might factor this into smaller changes!

(I see those redundant trivial {} changes; can't upload right now so please
ignore them.)

[dmazzoni, 2016-10-19 16:33:49]

No big concerns, this looks pretty reasonable overall!

I only skimmed the JS, I focused on the C++ code,
I'm assuming that the trickier logic is in C++ and that someone
on the DevTools team will review the JS code more carefully

It looks like this code is already exercised by tests - it might be
nice to try to fully cover all code paths with tests, even if you
don't exhaustively assert the correctness of all of them - just
make sure that there's some automated test that runs that
tries all of of the obscure cases and ensures they don't crash
or leak.

There's no easy code coverage tool I'm aware of, I was
thinking maybe add a few log statements at each interesting
code path and then try to create a test that makes each one
log at least once.

I just don't want to accidentally break this especially as
we're planning to mess with the "ignored" code.

https://codereview.chromium.org/2390783006/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
(right):

https://codereview.chromium.org/2390783006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:156:
void fillWidgetProperties(AXObject& axObject,
You could land a quick change to replace pointers with references for all
existing APIs
first to cut down on the size of this review

https://codereview.chromium.org/2390783006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:172:
if (headingLevel > 0) {
Did this come from "git cl format" or some other automated pass?

You could land another change to add braces to existing code
first to cut down on the size of this change

https://codereview.chromium.org/2390783006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:434:
AXObject* parent = inspectedAXObject->parentObjectUnignored();
Will it be confusing if you can click on a node and see it's ignored, but when
you explore the partial tree you won't see ignored parents? Just making sure
that's what you intended.

https://codereview.chromium.org/2390783006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:441:
(*nodes)->addItem(std::move(parentNodeObject));
Are you using parentNodeObject? If not, maybe just do:

    (*nodes)->addItem(
        buildProtocolAXObject(*parent, inspectedAXObject, *nodes, *cache));

https://codereview.chromium.org/2390783006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:445:
buildProtocolAXObject(*ancestor, inspectedAXObject, *nodes, *cache));
I think you can eliminate a few lines of code here - you handle the first parent
and then the rest of the ancestors, but I think you can replace it with just one
loop that does all of the ancestors

https://codereview.chromium.org/2390783006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:464:
axID = axObject->axObjectID();
Duplicate, you just computed it above

https://codereview.chromium.org/2390783006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:494:
Node* parentNode = inspectedDOMNode.parentNode();
I'm not an expert at all, but I suspect you may want to use
FlatTreeTraversal::parent(inspectedDOMNode) here in order to properly walk up
through shadow roots. Or check with someone on the core Blink team to better
understand the differences here.

https://codereview.chromium.org/2390783006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:529:
childIds->addItem(String::number(0));
Can you document this magic number? Why is the inspected node id 0 and not its
actual id?

https://codereview.chromium.org/2390783006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:546:
// Otherwise, just add the un-ignored children.
Reading through all of the special cases for building a tree based on a DOM node
(because it has no AXObject) vs building a tree based on an AXObject directly
makes me worried that it will be confusing to see a subtly different tree
depending on what the current inspected DOM node is.

In particular, think about how an AXObject builds its list of children: when one
of its children is ignored, it calls addChildren recursively on that ignored
child, so they all look like direct children of the ancestor. Now here you've
got a DOM node whose AXObject is ignored (otherwise you wouldn't be calling
findDOMNodeChildren), but you're taking one of its children and adding its
unignored children...but these could all actually be children of some other
ancestor AXObject that's had them added as its own children.

Sorry if my explanation is confusing.

I guess what I'm leaning towards is maybe simplifying what you try to do when
exploring a DOM node that's ignored. Perhaps you should ONLY display the parent
chain in that case. That'd be more foolproof.

Another idea: what if we got rid of the tiny number of cases where
AXObjectCacheImpl::getOrCreate(Node*) returns null? Just have it return an
object that's ignored. It looks like that would simplify your code here a lot if
you only had to deal with ignored and unignored, rather than ignored, unignored,
and null.

[aboxhall, 2016-10-19 23:49:05]

https://codereview.chromium.org/2390783006/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
(right):

https://codereview.chromium.org/2390783006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:156:
void fillWidgetProperties(AXObject& axObject,
On 2016/10/19 16:33:49, dmazzoni wrote:
> You could land a quick change to replace pointers with references for all
> existing APIs
> first to cut down on the size of this review

Done.

https://codereview.chromium.org/2390783006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:172:
if (headingLevel > 0) {
On 2016/10/19 16:33:49, dmazzoni wrote:
> Did this come from "git cl format" or some other automated pass?
> 
> You could land another change to add braces to existing code
> first to cut down on the size of this change

Done.

https://codereview.chromium.org/2390783006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:434:
AXObject* parent = inspectedAXObject->parentObjectUnignored();
On 2016/10/19 16:33:48, dmazzoni wrote:
> Will it be confusing if you can click on a node and see it's ignored, but when
> you explore the partial tree you won't see ignored parents? Just making sure
> that's what you intended.

Yeah, I think it's either that (i.e. show ignored nodes iff they map to the
inspected DOM node) or just include *all* ignored nodes in the tree, which would
be really noisy. What do you think?

https://codereview.chromium.org/2390783006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:441:
(*nodes)->addItem(std::move(parentNodeObject));
On 2016/10/19 16:33:48, dmazzoni wrote:
> Are you using parentNodeObject? If not, maybe just do:
> 
>     (*nodes)->addItem(
>         buildProtocolAXObject(*parent, inspectedAXObject, *nodes, *cache));

Done.

https://codereview.chromium.org/2390783006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:445:
buildProtocolAXObject(*ancestor, inspectedAXObject, *nodes, *cache));
On 2016/10/19 16:33:48, dmazzoni wrote:
> I think you can eliminate a few lines of code here - you handle the first
parent
> and then the rest of the ancestors, but I think you can replace it with just
one
> loop that does all of the ancestors

Done (yeah, it was originally special-casing the parent which is why it was
pulled out into a variable as well).

https://codereview.chromium.org/2390783006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:464:
axID = axObject->axObjectID();
On 2016/10/19 16:33:49, dmazzoni wrote:
> Duplicate, you just computed it above

Done.

https://codereview.chromium.org/2390783006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:494:
Node* parentNode = inspectedDOMNode.parentNode();
On 2016/10/19 16:33:48, dmazzoni wrote:
> I'm not an expert at all, but I suspect you may want to use
> FlatTreeTraversal::parent(inspectedDOMNode) here in order to properly walk up
> through shadow roots. Or check with someone on the core Blink team to better
> understand the differences here.

That makes sense, thanks for catching.

https://codereview.chromium.org/2390783006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:529:
childIds->addItem(String::number(0));
On 2016/10/19 16:33:48, dmazzoni wrote:
> Can you document this magic number? Why is the inspected node id 0 and not its
> actual id?

It doesn't have an actual id - there is no AXNode for the inspected DOM node in
this case, so there's no ID to use.

https://codereview.chromium.org/2390783006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:546:
// Otherwise, just add the un-ignored children.
On 2016/10/19 16:33:48, dmazzoni wrote:
> Reading through all of the special cases for building a tree based on a DOM
node
> (because it has no AXObject) vs building a tree based on an AXObject directly
> makes me worried that it will be confusing to see a subtly different tree
> depending on what the current inspected DOM node is.
> 
> In particular, think about how an AXObject builds its list of children: when
one
> of its children is ignored, it calls addChildren recursively on that ignored
> child, so they all look like direct children of the ancestor. Now here you've
> got a DOM node whose AXObject is ignored (otherwise you wouldn't be calling
> findDOMNodeChildren), but you're taking one of its children and adding its
> unignored children...but these could all actually be children of some other
> ancestor AXObject that's had them added as its own children.

Not quite accurate - this is only called if there is *no* AXObject. However, the
same basic strategy is used if the AXObject is ignored. Effectively, it
temporarily "reinstates" the ignored node so that you can see the shape of the
AX tree around it.

> Sorry if my explanation is confusing.

Not at all, this code was extremely confusing to write :\

> I guess what I'm leaning towards is maybe simplifying what you try to do when
> exploring a DOM node that's ignored. Perhaps you should ONLY display the
parent
> chain in that case. That'd be more foolproof.

I think that would be confusing to use.

> Another idea: what if we got rid of the tiny number of cases where
> AXObjectCacheImpl::getOrCreate(Node*) returns null? Just have it return an
> object that's ignored. It looks like that would simplify your code here a lot
if
> you only had to deal with ignored and unignored, rather than ignored,
unignored,
> and null.

That would be much easier! I'll give this a go.

[aboxhall, 2016-10-21 17:28:23]

https://codereview.chromium.org/2390783006/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
(right):

https://codereview.chromium.org/2390783006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:546:
// Otherwise, just add the un-ignored children.
On 2016/10/19 23:49:05, aboxhall wrote:
> On 2016/10/19 16:33:48, dmazzoni wrote:
> > Reading through all of the special cases for building a tree based on a DOM
> node
> > (because it has no AXObject) vs building a tree based on an AXObject
directly
> > makes me worried that it will be confusing to see a subtly different tree
> > depending on what the current inspected DOM node is.
> > 
> > In particular, think about how an AXObject builds its list of children: when
> one
> > of its children is ignored, it calls addChildren recursively on that ignored
> > child, so they all look like direct children of the ancestor. Now here
you've
> > got a DOM node whose AXObject is ignored (otherwise you wouldn't be calling
> > findDOMNodeChildren), but you're taking one of its children and adding its
> > unignored children...but these could all actually be children of some other
> > ancestor AXObject that's had them added as its own children.
> 
> Not quite accurate - this is only called if there is *no* AXObject. However,
the
> same basic strategy is used if the AXObject is ignored. Effectively, it
> temporarily "reinstates" the ignored node so that you can see the shape of the
> AX tree around it.
> 
> > Sorry if my explanation is confusing.
> 
> Not at all, this code was extremely confusing to write :\
> 
> > I guess what I'm leaning towards is maybe simplifying what you try to do
when
> > exploring a DOM node that's ignored. Perhaps you should ONLY display the
> parent
> > chain in that case. That'd be more foolproof.
> 
> I think that would be confusing to use.
> 
> > Another idea: what if we got rid of the tiny number of cases where
> > AXObjectCacheImpl::getOrCreate(Node*) returns null? Just have it return an
> > object that's ignored. It looks like that would simplify your code here a
lot
> if
> > you only had to deal with ignored and unignored, rather than ignored,
> unignored,
> > and null.
> 
> That would be much easier! I'll give this a go.

Ok, I tried that out and it actually doesn't buy us much, because the AXTree is
built up based on the layout tree, so if something doesn't have a layout object
it still ends up "weird". So I'd prefer to leave this code in if possible.

If you have the time, it'd be great if you could patch this change in and have a
play to see how it all works together; meanwhile I'll add some more screenshots
to the bug to show the flow for ignored/unrendered nodes.

[dmazzoni, 2016-10-21 18:28:42]

On Wed, Oct 19, 2016 at 4:49 PM <aboxhall@chromium.org> wrote:

>
>
https://codereview.chromium.org/2390783006/diff/160001/third_party/WebKit/Sou...
>
>
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:434:
> AXObject* parent = inspectedAXObject->parentObjectUnignored();
> On 2016/10/19 16:33:48, dmazzoni wrote:
> > Will it be confusing if you can click on a node and see it's ignored,
> but when
> > you explore the partial tree you won't see ignored parents? Just
> making sure
> > that's what you intended.
>
> Yeah, I think it's either that (i.e. show ignored nodes iff they map to
> the inspected DOM node) or just include *all* ignored nodes in the tree,
> which would be really noisy. What do you think?
>

I think it'd be cleaner to do the former - only show ignored nodes if they
map to the inspected DOM node. It seems like that would eliminate a
lot of special cases in your code and prevent some confusion.


https://codereview.chromium.org/2390783006/diff/160001/third_party/WebKit/Sou...
>
>
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:546:
> // Otherwise, just add the un-ignored children.
> On 2016/10/19 16:33:48, dmazzoni wrote:
> > Reading through all of the special cases for building a tree based on
> a DOM node
> > (because it has no AXObject) vs building a tree based on an AXObject
> directly
> > makes me worried that it will be confusing to see a subtly different
> tree
> > depending on what the current inspected DOM node is.
> >
> > In particular, think about how an AXObject builds its list of
> children: when one
> > of its children is ignored, it calls addChildren recursively on that
> ignored
> > child, so they all look like direct children of the ancestor. Now here
> you've
> > got a DOM node whose AXObject is ignored (otherwise you wouldn't be
> calling
> > findDOMNodeChildren), but you're taking one of its children and adding
> its
> > unignored children...but these could all actually be children of some
> other
> > ancestor AXObject that's had them added as its own children.
>
> Not quite accurate - this is only called if there is *no* AXObject.
> However, the same basic strategy is used if the AXObject is ignored.
> Effectively, it temporarily "reinstates" the ignored node so that you
> can see the shape of the AX tree around it.
>

I just think it's misleading to show siblings for an ignored node,
because they're not really siblings.

How about just showing parents?

Alternatively, if the AXObject doesn't exist, maybe just skip
the accessibility tree entirely? The user can still see the
reasons it doesn't exist, and if they want to jump to the
DOM parent they can easily do that, but it just seems really
misleading to show the "parent" of a node that doesn't
actually have a parent!

> I guess what I'm leaning towards is maybe simplifying what you try to
> do when
> > exploring a DOM node that's ignored. Perhaps you should ONLY display
> the parent
> > chain in that case. That'd be more foolproof.
>
> I think that would be confusing to use.
>

What's the intended use case? If someone clicks on a DOM node and
they see it's not in the accessibility tree, what possible use is there
of exploring "the tree around the node" when it's not actually in the
tree? It makes more sense to explore the DOM around that node
and figure out what's going on there.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2016-10-21 18:29:16]

On Wed, Oct 19, 2016 at 4:49 PM <aboxhall@chromium.org> wrote:

>
>
https://codereview.chromium.org/2390783006/diff/160001/third_party/WebKit/Sou...
>
>
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:434:
> AXObject* parent = inspectedAXObject->parentObjectUnignored();
> On 2016/10/19 16:33:48, dmazzoni wrote:
> > Will it be confusing if you can click on a node and see it's ignored,
> but when
> > you explore the partial tree you won't see ignored parents? Just
> making sure
> > that's what you intended.
>
> Yeah, I think it's either that (i.e. show ignored nodes iff they map to
> the inspected DOM node) or just include *all* ignored nodes in the tree,
> which would be really noisy. What do you think?
>

I think it'd be cleaner to do the former - only show ignored nodes if they
map to the inspected DOM node. It seems like that would eliminate a
lot of special cases in your code and prevent some confusion.


https://codereview.chromium.org/2390783006/diff/160001/third_party/WebKit/Sou...
>
>
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:546:
> // Otherwise, just add the un-ignored children.
> On 2016/10/19 16:33:48, dmazzoni wrote:
> > Reading through all of the special cases for building a tree based on
> a DOM node
> > (because it has no AXObject) vs building a tree based on an AXObject
> directly
> > makes me worried that it will be confusing to see a subtly different
> tree
> > depending on what the current inspected DOM node is.
> >
> > In particular, think about how an AXObject builds its list of
> children: when one
> > of its children is ignored, it calls addChildren recursively on that
> ignored
> > child, so they all look like direct children of the ancestor. Now here
> you've
> > got a DOM node whose AXObject is ignored (otherwise you wouldn't be
> calling
> > findDOMNodeChildren), but you're taking one of its children and adding
> its
> > unignored children...but these could all actually be children of some
> other
> > ancestor AXObject that's had them added as its own children.
>
> Not quite accurate - this is only called if there is *no* AXObject.
> However, the same basic strategy is used if the AXObject is ignored.
> Effectively, it temporarily "reinstates" the ignored node so that you
> can see the shape of the AX tree around it.
>

I just think it's misleading to show siblings for an ignored node,
because they're not really siblings.

How about just showing parents?

Alternatively, if the AXObject doesn't exist, maybe just skip
the accessibility tree entirely? The user can still see the
reasons it doesn't exist, and if they want to jump to the
DOM parent they can easily do that, but it just seems really
misleading to show the "parent" of a node that doesn't
actually have a parent!

> I guess what I'm leaning towards is maybe simplifying what you try to
> do when
> > exploring a DOM node that's ignored. Perhaps you should ONLY display
> the parent
> > chain in that case. That'd be more foolproof.
>
> I think that would be confusing to use.
>

What's the intended use case? If someone clicks on a DOM node and
they see it's not in the accessibility tree, what possible use is there
of exploring "the tree around the node" when it's not actually in the
tree? It makes more sense to explore the DOM around that node
and figure out what's going on there.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[aboxhall, 2016-10-21 18:47:48]

On Fri, Oct 21, 2016 at 11:28 AM, Dominic Mazzoni <dmazzoni@chromium.org>
wrote:

> On Wed, Oct 19, 2016 at 4:49 PM <aboxhall@chromium.org> wrote:
>
>> https://codereview.chromium.org/2390783006/diff/160001/
>> third_party/WebKit/Source/modules/accessibility/
>> InspectorAccessibilityAgent.cpp#newcode434
>> third_party/WebKit/Source/modules/accessibility/
>> InspectorAccessibilityAgent.cpp:434:
>> AXObject* parent = inspectedAXObject->parentObjectUnignored();
>> On 2016/10/19 16:33:48, dmazzoni wrote:
>> > Will it be confusing if you can click on a node and see it's ignored,
>> but when
>> > you explore the partial tree you won't see ignored parents? Just
>> making sure
>> > that's what you intended.
>>
>> Yeah, I think it's either that (i.e. show ignored nodes iff they map to
>> the inspected DOM node) or just include *all* ignored nodes in the tree,
>> which would be really noisy. What do you think?
>>
>
> I think it'd be cleaner to do the former - only show ignored nodes if they
> map to the inspected DOM node. It seems like that would eliminate a
> lot of special cases in your code and prevent some confusion.
>

To clarify: that's what this code *does*. Please take a look at the
screenshots in the bug.


> https://codereview.chromium.org/2390783006/diff/160001/
>> third_party/WebKit/Source/modules/accessibility/
>> InspectorAccessibilityAgent.cpp#newcode546
>> third_party/WebKit/Source/modules/accessibility/
>> InspectorAccessibilityAgent.cpp:546:
>> // Otherwise, just add the un-ignored children.
>> On 2016/10/19 16:33:48, dmazzoni wrote:
>> > Reading through all of the special cases for building a tree based on
>> a DOM node
>> > (because it has no AXObject) vs building a tree based on an AXObject
>> directly
>> > makes me worried that it will be confusing to see a subtly different
>> tree
>> > depending on what the current inspected DOM node is.
>> >
>> > In particular, think about how an AXObject builds its list of
>> children: when one
>> > of its children is ignored, it calls addChildren recursively on that
>> ignored
>> > child, so they all look like direct children of the ancestor. Now here
>> you've
>> > got a DOM node whose AXObject is ignored (otherwise you wouldn't be
>> calling
>> > findDOMNodeChildren), but you're taking one of its children and adding
>> its
>> > unignored children...but these could all actually be children of some
>> other
>> > ancestor AXObject that's had them added as its own children.
>>
>> Not quite accurate - this is only called if there is *no* AXObject.
>> However, the same basic strategy is used if the AXObject is ignored.
>> Effectively, it temporarily "reinstates" the ignored node so that you
>> can see the shape of the AX tree around it.
>>
>
> I just think it's misleading to show siblings for an ignored node,
> because they're not really siblings.
>
> How about just showing parents?
>
> Alternatively, if the AXObject doesn't exist, maybe just skip
> the accessibility tree entirely? The user can still see the
> reasons it doesn't exist, and if they want to jump to the
> DOM parent they can easily do that, but it just seems really
> misleading to show the "parent" of a node that doesn't
> actually have a parent!
>

They're siblings in the sense that if this node weren't ignored, these
would be its siblings. I think it would be a disjointed experience not to
show the tree for ignored nodes.


> > I guess what I'm leaning towards is maybe simplifying what you try to
>> do when
>> > exploring a DOM node that's ignored. Perhaps you should ONLY display
>> the parent
>> > chain in that case. That'd be more foolproof.
>>
>> I think that would be confusing to use.
>>
>
> What's the intended use case? If someone clicks on a DOM node and
> they see it's not in the accessibility tree, what possible use is there
> of exploring "the tree around the node" when it's not actually in the
> tree? It makes more sense to explore the DOM around that node
> and figure out what's going on there.
>

Because this is the quickest way to get to non-ignored nodes - if we didn't
show the a11y tree for ignored nodes, users would need to randomly jump
around the DOM tree until they found a non-ignored node from which to
explore the a11y tree.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[aboxhall, 2016-10-21 18:47:50]

On Fri, Oct 21, 2016 at 11:28 AM, Dominic Mazzoni <dmazzoni@chromium.org>
wrote:

> On Wed, Oct 19, 2016 at 4:49 PM <aboxhall@chromium.org> wrote:
>
>> https://codereview.chromium.org/2390783006/diff/160001/
>> third_party/WebKit/Source/modules/accessibility/
>> InspectorAccessibilityAgent.cpp#newcode434
>> third_party/WebKit/Source/modules/accessibility/
>> InspectorAccessibilityAgent.cpp:434:
>> AXObject* parent = inspectedAXObject->parentObjectUnignored();
>> On 2016/10/19 16:33:48, dmazzoni wrote:
>> > Will it be confusing if you can click on a node and see it's ignored,
>> but when
>> > you explore the partial tree you won't see ignored parents? Just
>> making sure
>> > that's what you intended.
>>
>> Yeah, I think it's either that (i.e. show ignored nodes iff they map to
>> the inspected DOM node) or just include *all* ignored nodes in the tree,
>> which would be really noisy. What do you think?
>>
>
> I think it'd be cleaner to do the former - only show ignored nodes if they
> map to the inspected DOM node. It seems like that would eliminate a
> lot of special cases in your code and prevent some confusion.
>

To clarify: that's what this code *does*. Please take a look at the
screenshots in the bug.


> https://codereview.chromium.org/2390783006/diff/160001/
>> third_party/WebKit/Source/modules/accessibility/
>> InspectorAccessibilityAgent.cpp#newcode546
>> third_party/WebKit/Source/modules/accessibility/
>> InspectorAccessibilityAgent.cpp:546:
>> // Otherwise, just add the un-ignored children.
>> On 2016/10/19 16:33:48, dmazzoni wrote:
>> > Reading through all of the special cases for building a tree based on
>> a DOM node
>> > (because it has no AXObject) vs building a tree based on an AXObject
>> directly
>> > makes me worried that it will be confusing to see a subtly different
>> tree
>> > depending on what the current inspected DOM node is.
>> >
>> > In particular, think about how an AXObject builds its list of
>> children: when one
>> > of its children is ignored, it calls addChildren recursively on that
>> ignored
>> > child, so they all look like direct children of the ancestor. Now here
>> you've
>> > got a DOM node whose AXObject is ignored (otherwise you wouldn't be
>> calling
>> > findDOMNodeChildren), but you're taking one of its children and adding
>> its
>> > unignored children...but these could all actually be children of some
>> other
>> > ancestor AXObject that's had them added as its own children.
>>
>> Not quite accurate - this is only called if there is *no* AXObject.
>> However, the same basic strategy is used if the AXObject is ignored.
>> Effectively, it temporarily "reinstates" the ignored node so that you
>> can see the shape of the AX tree around it.
>>
>
> I just think it's misleading to show siblings for an ignored node,
> because they're not really siblings.
>
> How about just showing parents?
>
> Alternatively, if the AXObject doesn't exist, maybe just skip
> the accessibility tree entirely? The user can still see the
> reasons it doesn't exist, and if they want to jump to the
> DOM parent they can easily do that, but it just seems really
> misleading to show the "parent" of a node that doesn't
> actually have a parent!
>

They're siblings in the sense that if this node weren't ignored, these
would be its siblings. I think it would be a disjointed experience not to
show the tree for ignored nodes.


> > I guess what I'm leaning towards is maybe simplifying what you try to
>> do when
>> > exploring a DOM node that's ignored. Perhaps you should ONLY display
>> the parent
>> > chain in that case. That'd be more foolproof.
>>
>> I think that would be confusing to use.
>>
>
> What's the intended use case? If someone clicks on a DOM node and
> they see it's not in the accessibility tree, what possible use is there
> of exploring "the tree around the node" when it's not actually in the
> tree? It makes more sense to explore the DOM around that node
> and figure out what's going on there.
>

Because this is the quickest way to get to non-ignored nodes - if we didn't
show the a11y tree for ignored nodes, users would need to randomly jump
around the DOM tree until they found a non-ignored node from which to
explore the a11y tree.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2016-10-21 19:57:11]

On Fri, Oct 21, 2016 at 11:47 AM Alice Boxhall <aboxhall@chromium.org>
wrote:

> Yeah, I think it's either that (i.e. show ignored nodes iff they map to
> the inspected DOM node) or just include *all* ignored nodes in the tree,
> which would be really noisy. What do you think?
>
>
> I think it'd be cleaner to do the former - only show ignored nodes if they
> map to the inspected DOM node. It seems like that would eliminate a
> lot of special cases in your code and prevent some confusion.
>
>
> To clarify: that's what this code *does*. Please take a look at the
> screenshots in the bug.
>

I guess I meant it in a stronger sense - like if it's ignored, just
literally skip trying to explore the tree.

They're siblings in the sense that if this node weren't ignored, these
> would be its siblings. I think it would be a disjointed experience not to
> show the tree for ignored nodes.
>

I think there are cases where this isn't true, though. I think it's very
possible that you could show nodes that would *not* be its siblings if it
was not ignored, even though in the common case you'd be right.

For example, think about aria-owns:

<section id="section" role="presentation">
  <div role="list" id="list" aria-owns="item1 item2 item3">
    <button id="showmore">Show more items...</button>
  </div>
  <div role="listitem" id="item1"></div>
  <div role="listitem" id="item2"></div>
  <div role="listitem" id="item3" hidden></div>
</section>

If you click on #item3, does your code work correctly? I feel like there
are almost certainly corner cases where it shows parents or siblings that
are wrong or misleading.

What's the intended use case? If someone clicks on a DOM node and
> they see it's not in the accessibility tree, what possible use is there
> of exploring "the tree around the node" when it's not actually in the
> tree? It makes more sense to explore the DOM around that node
> and figure out what's going on there.
>
>
> Because this is the quickest way to get to non-ignored nodes - if we
> didn't show the a11y tree for ignored nodes, users would need to randomly
> jump around the DOM tree until they found a non-ignored node from which to
> explore the a11y tree.
>

I can see the argument for jumping to the nearest unignored ancestor, but
showing a partial accessibility tree for a node that's not actually part of
the tree just seems like it could be misleading.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2016-10-21 19:57:11]

On Fri, Oct 21, 2016 at 11:47 AM Alice Boxhall <aboxhall@chromium.org>
wrote:

> Yeah, I think it's either that (i.e. show ignored nodes iff they map to
> the inspected DOM node) or just include *all* ignored nodes in the tree,
> which would be really noisy. What do you think?
>
>
> I think it'd be cleaner to do the former - only show ignored nodes if they
> map to the inspected DOM node. It seems like that would eliminate a
> lot of special cases in your code and prevent some confusion.
>
>
> To clarify: that's what this code *does*. Please take a look at the
> screenshots in the bug.
>

I guess I meant it in a stronger sense - like if it's ignored, just
literally skip trying to explore the tree.

They're siblings in the sense that if this node weren't ignored, these
> would be its siblings. I think it would be a disjointed experience not to
> show the tree for ignored nodes.
>

I think there are cases where this isn't true, though. I think it's very
possible that you could show nodes that would *not* be its siblings if it
was not ignored, even though in the common case you'd be right.

For example, think about aria-owns:

<section id="section" role="presentation">
  <div role="list" id="list" aria-owns="item1 item2 item3">
    <button id="showmore">Show more items...</button>
  </div>
  <div role="listitem" id="item1"></div>
  <div role="listitem" id="item2"></div>
  <div role="listitem" id="item3" hidden></div>
</section>

If you click on #item3, does your code work correctly? I feel like there
are almost certainly corner cases where it shows parents or siblings that
are wrong or misleading.

What's the intended use case? If someone clicks on a DOM node and
> they see it's not in the accessibility tree, what possible use is there
> of exploring "the tree around the node" when it's not actually in the
> tree? It makes more sense to explore the DOM around that node
> and figure out what's going on there.
>
>
> Because this is the quickest way to get to non-ignored nodes - if we
> didn't show the a11y tree for ignored nodes, users would need to randomly
> jump around the DOM tree until they found a non-ignored node from which to
> explore the a11y tree.
>

I can see the argument for jumping to the nearest unignored ancestor, but
showing a partial accessibility tree for a node that's not actually part of
the tree just seems like it could be misleading.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dmazzoni, 2016-10-21 20:09:42]

lgtm

I took another pass through the code. I see one more
potential issue with addSiblingsOfIgnored, but it's
pretty similar to my other concerns. The code looks
clean otherwise - I only have concerns that the end
result will be misleading, not concerns about how
it's implemented. So if you'd still prefer to do
something along these lines, I'm okay with landing it
and we can get feedback from more people when it's
on canary. If the consensus is that the partial tree
is more useful than misleading, that's fine.

https://codereview.chromium.org/2390783006/diff/280001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
(right):

https://codereview.chromium.org/2390783006/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:691:
for (AXObject* childAXObject = parentAXObject.rawFirstChild(); childAXObject;
rawFirstChild skips over some important corner cases like
children of a <canvas> element or children with aria-hidden=false.

[aboxhall, 2016-10-21 20:24:39]

On Fri, Oct 21, 2016 at 12:56 PM, Dominic Mazzoni <dmazzoni@chromium.org>
wrote:

> On Fri, Oct 21, 2016 at 11:47 AM Alice Boxhall <aboxhall@chromium.org>
> wrote:
>
>> Yeah, I think it's either that (i.e. show ignored nodes iff they map to
>> the inspected DOM node) or just include *all* ignored nodes in the tree,
>> which would be really noisy. What do you think?
>>
>>
>> I think it'd be cleaner to do the former - only show ignored nodes if they
>> map to the inspected DOM node. It seems like that would eliminate a
>> lot of special cases in your code and prevent some confusion.
>>
>>
>> To clarify: that's what this code *does*. Please take a look at the
>> screenshots in the bug.
>>
>
> I guess I meant it in a stronger sense - like if it's ignored, just
> literally skip trying to explore the tree.
>
> They're siblings in the sense that if this node weren't ignored, these
>> would be its siblings. I think it would be a disjointed experience not to
>> show the tree for ignored nodes.
>>
>
> I think there are cases where this isn't true, though. I think it's very
> possible that you could show nodes that would *not* be its siblings if it
> was not ignored, even though in the common case you'd be right.
>
> For example, think about aria-owns:
>
> <section id="section" role="presentation">
>   <div role="list" id="list" aria-owns="item1 item2 item3">
>     <button id="showmore">Show more items...</button>
>   </div>
>   <div role="listitem" id="item1"></div>
>   <div role="listitem" id="item2"></div>
>   <div role="listitem" id="item3" hidden></div>
> </section>
>
> If you click on #item3, does your code work correctly? I feel like there
> are almost certainly corner cases where it shows parents or siblings that
> are wrong or misleading.
>

Right, this does come out weird:
https://bugs.chromium.org/p/chromium/issues/detail?id=560525#c26

>
> What's the intended use case? If someone clicks on a DOM node and
>> they see it's not in the accessibility tree, what possible use is there
>> of exploring "the tree around the node" when it's not actually in the
>> tree? It makes more sense to explore the DOM around that node
>> and figure out what's going on there.
>>
>>
>> Because this is the quickest way to get to non-ignored nodes - if we
>> didn't show the a11y tree for ignored nodes, users would need to randomly
>> jump around the DOM tree until they found a non-ignored node from which to
>> explore the a11y tree.
>>
>
> I can see the argument for jumping to the nearest unignored ancestor, but
> showing a partial accessibility tree for a node that's not actually part of
> the tree just seems like it could be misleading.
>

Yeah, I can definitely see both sides. My hunch is that not showing these
nodes will be more annoying than the occasional weirdness that showing them
causes, but I agree that we'll get the best signal from seeing how people
use and react to it.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[aboxhall, 2016-10-21 20:24:39]

On Fri, Oct 21, 2016 at 12:56 PM, Dominic Mazzoni <dmazzoni@chromium.org>
wrote:

> On Fri, Oct 21, 2016 at 11:47 AM Alice Boxhall <aboxhall@chromium.org>
> wrote:
>
>> Yeah, I think it's either that (i.e. show ignored nodes iff they map to
>> the inspected DOM node) or just include *all* ignored nodes in the tree,
>> which would be really noisy. What do you think?
>>
>>
>> I think it'd be cleaner to do the former - only show ignored nodes if they
>> map to the inspected DOM node. It seems like that would eliminate a
>> lot of special cases in your code and prevent some confusion.
>>
>>
>> To clarify: that's what this code *does*. Please take a look at the
>> screenshots in the bug.
>>
>
> I guess I meant it in a stronger sense - like if it's ignored, just
> literally skip trying to explore the tree.
>
> They're siblings in the sense that if this node weren't ignored, these
>> would be its siblings. I think it would be a disjointed experience not to
>> show the tree for ignored nodes.
>>
>
> I think there are cases where this isn't true, though. I think it's very
> possible that you could show nodes that would *not* be its siblings if it
> was not ignored, even though in the common case you'd be right.
>
> For example, think about aria-owns:
>
> <section id="section" role="presentation">
>   <div role="list" id="list" aria-owns="item1 item2 item3">
>     <button id="showmore">Show more items...</button>
>   </div>
>   <div role="listitem" id="item1"></div>
>   <div role="listitem" id="item2"></div>
>   <div role="listitem" id="item3" hidden></div>
> </section>
>
> If you click on #item3, does your code work correctly? I feel like there
> are almost certainly corner cases where it shows parents or siblings that
> are wrong or misleading.
>

Right, this does come out weird:
https://bugs.chromium.org/p/chromium/issues/detail?id=560525#c26

>
> What's the intended use case? If someone clicks on a DOM node and
>> they see it's not in the accessibility tree, what possible use is there
>> of exploring "the tree around the node" when it's not actually in the
>> tree? It makes more sense to explore the DOM around that node
>> and figure out what's going on there.
>>
>>
>> Because this is the quickest way to get to non-ignored nodes - if we
>> didn't show the a11y tree for ignored nodes, users would need to randomly
>> jump around the DOM tree until they found a non-ignored node from which to
>> explore the a11y tree.
>>
>
> I can see the argument for jumping to the nearest unignored ancestor, but
> showing a partial accessibility tree for a node that's not actually part of
> the tree just seems like it could be misleading.
>

Yeah, I can definitely see both sides. My hunch is that not showing these
nodes will be more annoying than the occasional weirdness that showing them
causes, but I agree that we'll get the best signal from seeing how people
use and react to it.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dmazzoni, 2016-10-24 14:08:56]

lgtm

[aboxhall, 2016-10-31 19:33:23]

aboxhall@chromium.org changed reviewers:
+ dgozman@chromium.org

[aboxhall, 2016-10-31 19:33:25]

+dgozman - this is now (finally) ready for a look on the devtools side.

https://codereview.chromium.org/2390783006/diff/280001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
(right):

https://codereview.chromium.org/2390783006/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:691:
for (AXObject* childAXObject = parentAXObject.rawFirstChild(); childAXObject;
On 2016/10/21 20:09:42, dmazzoni wrote:
> rawFirstChild skips over some important corner cases like
> children of a <canvas> element or children with aria-hidden=false.

I added a test for <canvas> fallback content.
I'm not sure about aria-hidden=false. I can't see any evidence in tests or code
for it working today for this case:
<div style="display: none">
  <span aria-hidden="false">
    The span is ignored but this text shouldn't be
  </span>
</span>

I added a test for the more straightforward case:
<div style="display: none">
  <button aria-hidden="false">Not hidden button</button>
</div>

Although I note that in the content tests, aria-hidden=false doesn't seem to
have any actual effect anyway - it doesn't seem to be exposed on any platform as
far as I can tell.

[aboxhall, 2016-10-31 20:43:09]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 20:43:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dgozman, 2016-10-31 21:36:31]

Made a first pass. A lot to review!
- Please mention DevTools and accessibility in description.
- I think securityIcons files are here by accident.
- Why do we need siblings? Where are they in UI? Mind writing down an
explanation of what we try to achieve here?

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4382: { "name":
"childIds", "type": "array", "items": { "$ref": "AXNodeId" }, "description":
"IDs for each of this node's child nodes.", "optional": true },
It's usually unnecessary and error-prone to have both parentId and childId,
since they must be symmetrical and bugs can cripple in. Do we need both?

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js
(right):

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js:309:
WebInspector.context.setFlavor(WebInspector.AccessibilityNode, this._axNode);
Where is this flavor being used?

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js:330:
_handleMouseDown: function(event) {
style: { on next line (here and below)

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilityModel.js
(right):

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilityModel.js:248:
setDOMNode: function(node)
setDOMNode is a strange api for a model. It should be managed outside (e.g. by
the pane).

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js
(right):

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:71:
.then(accessibilityModel.resolveAllDOMNodes.bind(accessibilityModel))
What's the point of resolving all the nodes right away? Should we push nodes
from backend before sending ids instead, so that there is no need to resolve?

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right):

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:890:
collapseOrAscend: function(altKey)
Let's land changes to tree outline separately, with a description of what's
happening and why.

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
(right):

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:431:
domNode, inspectedAXObject, justFetchRelatives, *nodes, *cache));
fetchRelatives.fromMaybe(true)
would be easier.

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:513:
while (!parentAXObject) {
while (parentNode && !parentAXObject)

[commit-bot: I haz the power, 2016-10-31 22:17:23]

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

[commit-bot: I haz the power, 2016-10-31 22:17:25]

Dry run: This issue passed the CQ dry run.

[aboxhall, 2016-10-31 22:45:16]

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4382: { "name":
"childIds", "type": "array", "items": { "$ref": "AXNodeId" }, "description":
"IDs for each of this node's child nodes.", "optional": true },
On 2016/10/31 21:36:30, dgozman wrote:
> It's usually unnecessary and error-prone to have both parentId and childId,
> since they must be symmetrical and bugs can cripple in. Do we need both?

Hm, yeah, I can probably do without parentId.

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js
(right):

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js:309:
WebInspector.context.setFlavor(WebInspector.AccessibilityNode, this._axNode);
On 2016/10/31 21:36:30, dgozman wrote:
> Where is this flavor being used?

Ah, thanks for catching - I was using it earlier but it's no longer needed.

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js:330:
_handleMouseDown: function(event) {
On 2016/10/31 21:36:30, dgozman wrote:
> style: { on next line (here and below)

Done.

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilityModel.js
(right):

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilityModel.js:248:
setDOMNode: function(node)
On 2016/10/31 21:36:30, dgozman wrote:
> setDOMNode is a strange api for a model. It should be managed outside (e.g. by
> the pane).

The reason is that the model is centered on the inspected DOM node and
invalidated when that changes.

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js
(right):

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:71:
.then(accessibilityModel.resolveAllDOMNodes.bind(accessibilityModel))
On 2016/10/31 21:36:30, dgozman wrote:
> What's the point of resolving all the nodes right away? Should we push nodes
> from backend before sending ids instead, so that there is no need to resolve?

You mean send a front-end ID? I think I tried that when I was originally
creating the protocol and got some push-back. In general I would vastly prefer
that if possible!

The reason we need to resolve them all is so we can match up the inspected node
with its corresponding AX node.

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right):

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:890:
collapseOrAscend: function(altKey)
On 2016/10/31 21:36:31, dgozman wrote:
> Let's land changes to tree outline separately, with a description of what's
> happening and why.

Sounds good, will do.

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
(right):

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:431:
domNode, inspectedAXObject, justFetchRelatives, *nodes, *cache));
On 2016/10/31 21:36:31, dgozman wrote:
> fetchRelatives.fromMaybe(true)
> would be easier.

Thanks, done!

https://codereview.chromium.org/2390783006/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:513:
while (!parentAXObject) {
On 2016/10/31 21:36:31, dgozman wrote:
> while (parentNode && !parentAXObject)

Done.

[aboxhall, 2016-10-31 22:45:49]

Oops, hit publish too soon, will ping again when new patch ready.

[aboxhall, 2016-10-31 23:14:01]

Patch uploaded, still need to create that other patch though.

[aboxhall, 2016-11-03 16:51:30]

Reformatted and made some UI tweaks based on advice from Chris.

[dgozman, 2016-11-03 22:07:29]

Some top-level comments below. I'll do thorough pass once we resolve them.

https://codereview.chromium.org/2390783006/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js
(right):

https://codereview.chromium.org/2390783006/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js:181:
this.listItemElement.classList.add('inspected');
this._inspected = inspected.
this.listItemElement.classList.toggle('inspected', inspected);

https://codereview.chromium.org/2390783006/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js:209:
WebInspector.Revealer.reveal(this._axNode.domNode());
Note that we can reveal DeferredDOMNode.
And for comparing nodes, we could just compare backendId of the DOMNode we
already have in elements tree with a backendId you have in ax tree. This way you
don't have to resolve all nodes at the start, which will save a lot. WDYT?

https://codereview.chromium.org/2390783006/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js:360:
this._expandSibingsButton = new WebInspector.ExpandSiblingsButton(this,
numSiblings);
typo: Sibings

https://codereview.chromium.org/2390783006/diff/400001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js
(right):

https://codereview.chromium.org/2390783006/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:64:
return accessibilityModel.setDOMNode(node)
Instead of this, I propose to:
- call accessibilityModel.clear() to clear caches;
- call accessibilityModel.partialAXTree() to get the data;
- store resulting this._axNode (you do that in accessibilityNodeCallback);
- do not resolve all dom nodes.

This moves the management of "current node" to sidebar view, while making model
maintain caches and handling protocol calls.

https://codereview.chromium.org/2390783006/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right):

https://codereview.chromium.org/2390783006/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:216: handled =
this.selectedTreeElement.collapseOrAscend(event.altKey);
Time to rebase atop the other change?

https://codereview.chromium.org/2390783006/diff/400001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.h
(right):

https://codereview.chromium.org/2390783006/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.h:46:
std::unique_ptr<protocol::Array<protocol::Accessibility::AXNode>>& nodes,
I'd suggest to do
using protocol::Accessibility::AXNode;
using protocol::Accessibility::AXNodeId;

[aboxhall, 2016-11-04 22:31:17]

https://codereview.chromium.org/2390783006/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js
(right):

https://codereview.chromium.org/2390783006/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js:181:
this.listItemElement.classList.add('inspected');
On 2016/11/03 22:07:29, dgozman wrote:
> this._inspected = inspected.
> this.listItemElement.classList.toggle('inspected', inspected);

Done.

https://codereview.chromium.org/2390783006/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js:209:
WebInspector.Revealer.reveal(this._axNode.domNode());
On 2016/11/03 22:07:29, dgozman wrote:
> Note that we can reveal DeferredDOMNode.
> And for comparing nodes, we could just compare backendId of the DOMNode we
> already have in elements tree with a backendId you have in ax tree. This way
you
> don't have to resolve all nodes at the start, which will save a lot. WDYT?

Done.

https://codereview.chromium.org/2390783006/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js:360:
this._expandSibingsButton = new WebInspector.ExpandSiblingsButton(this,
numSiblings);
On 2016/11/03 22:07:29, dgozman wrote:
> typo: Sibings

Done.

https://codereview.chromium.org/2390783006/diff/400001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js
(right):

https://codereview.chromium.org/2390783006/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:64:
return accessibilityModel.setDOMNode(node)
On 2016/11/03 22:07:29, dgozman wrote:
> Instead of this, I propose to:
> - call accessibilityModel.clear() to clear caches;
> - call accessibilityModel.partialAXTree() to get the data;
> - store resulting this._axNode (you do that in accessibilityNodeCallback);
> - do not resolve all dom nodes.
> 
> This moves the management of "current node" to sidebar view, while making
model
> maintain caches and handling protocol calls.

Done, I think. Was this what you had in mind?

https://codereview.chromium.org/2390783006/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right):

https://codereview.chromium.org/2390783006/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:216: handled =
this.selectedTreeElement.collapseOrAscend(event.altKey);
On 2016/11/03 22:07:29, dgozman wrote:
> Time to rebase atop the other change?

Done.

https://codereview.chromium.org/2390783006/diff/400001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.h
(right):

https://codereview.chromium.org/2390783006/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.h:46:
std::unique_ptr<protocol::Array<protocol::Accessibility::AXNode>>& nodes,
On 2016/11/03 22:07:29, dgozman wrote:
> I'd suggest to do
> using protocol::Accessibility::AXNode;
> using protocol::Accessibility::AXNodeId;

Isn't this somewhat frowned upon?
https://google.github.io/styleguide/cppguide.html#Aliases

[dgozman, 2016-11-04 23:22:46]

Frontend mostly looks good. I'll take another pass on backend on Monday.
Tests are just impossible to review :-)

https://codereview.chromium.org/2390783006/diff/400001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.h
(right):

https://codereview.chromium.org/2390783006/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.h:46:
std::unique_ptr<protocol::Array<protocol::Accessibility::AXNode>>& nodes,
On 2016/11/04 22:31:17, aboxhall wrote:
> On 2016/11/03 22:07:29, dgozman wrote:
> > I'd suggest to do
> > using protocol::Accessibility::AXNode;
> > using protocol::Accessibility::AXNodeId;
> 
> Isn't this somewhat frowned upon?
> https://google.github.io/styleguide/cppguide.html#Aliases

I think this qualifies for the last bullet there: "However, local convenience
aliases are fine in function definitions, private sections of classes,
explicitly marked internal namespaces, and in .cc files".

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js
(right):

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js:221: if
(this._axNode && this._axNode.name().value) {
this._axNode is always there.

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js:228:
this._hasOnlyUnloadedChildren = true;
Why not call this._axNode.hasOnlyUnloadedChildren() everywhere instead?

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js:301:
this._ensureSelection();
Isn't this a private method of superclass? Isn't it called by super.select()
anyway?

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js:328: '+
%d %s', numSiblings, numSiblings === 1 ? 'node' : 'nodes');
We should only substitute user data, not the parts of ui string. This code
should be:
UIString(numSiblings === 1 ? '+ %s node' : '+ %s nodes', numSiblings)

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js:391:
TreeElement.prototype.expand.call(this);
super.expand();

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilityModel.js
(right):

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilityModel.js:136:
* @return WebInspector.DeferredDOMNode
{?....}

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js
(right):

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:64:
return accessibilityModel.requestPartialAXTree(node)
accessibilityModel.clear()

[aboxhall, 2016-11-04 23:28:33]

On 2016/11/04 23:22:46, dgozman wrote:
> Frontend mostly looks good. I'll take another pass on backend on Monday.
> Tests are just impossible to review :-)

The tests are mostly boring, but have a look at
https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Lay...
- this shows the result of all that complicated finagling of ignored nodes.

[aboxhall, 2016-11-07 19:54:43]

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js
(right):

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js:221: if
(this._axNode && this._axNode.name().value) {
On 2016/11/04 23:22:45, dgozman wrote:
> this._axNode is always there.

Done.

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js:228:
this._hasOnlyUnloadedChildren = true;
On 2016/11/04 23:22:45, dgozman wrote:
> Why not call this._axNode.hasOnlyUnloadedChildren() everywhere instead?

Done.

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js:301:
this._ensureSelection();
On 2016/11/04 23:22:45, dgozman wrote:
> Isn't this a private method of superclass? Isn't it called by super.select()
> anyway?

Nope, not called by select():
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front...

Should I make it a public method? If I don't call it, the blue selection bar
doesn't appear.

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js:328: '+
%d %s', numSiblings, numSiblings === 1 ? 'node' : 'nodes');
On 2016/11/04 23:22:45, dgozman wrote:
> We should only substitute user data, not the parts of ui string. This code
> should be:
> UIString(numSiblings === 1 ? '+ %s node' : '+ %s nodes', numSiblings)

Done.

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js:391:
TreeElement.prototype.expand.call(this);
On 2016/11/04 23:22:45, dgozman wrote:
> super.expand();

Done.

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilityModel.js
(right):

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilityModel.js:136:
* @return WebInspector.DeferredDOMNode
On 2016/11/04 23:22:45, dgozman wrote:
> {?....}

Weird presubmit didn't catch that, done.

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js
(right):

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:64:
return accessibilityModel.requestPartialAXTree(node)
On 2016/11/04 23:22:45, dgozman wrote:
> accessibilityModel.clear() 

Done.

[dgozman, 2016-11-08 16:38:41]

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js
(right):

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js:301:
this._ensureSelection();
On 2016/11/07 19:54:43, aboxhall wrote:
> On 2016/11/04 23:22:45, dgozman wrote:
> > Isn't this a private method of superclass? Isn't it called by super.select()
> > anyway?
> 
> Nope, not called by select():
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front...
> 
> Should I make it a public method? If I don't call it, the blue selection bar
> doesn't appear.

Actually, it's called right after onattach, so it should be fine unless you
removeChildren after that.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front...

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js
(right):

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:64:
return accessibilityModel.requestPartialAXTree(node)
On 2016/11/07 19:54:43, aboxhall wrote:
> On 2016/11/04 23:22:45, dgozman wrote:
> > accessibilityModel.clear() 
> 
> Done.

Missed?

https://codereview.chromium.org/2390783006/diff/460001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
(right):

https://codereview.chromium.org/2390783006/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:514:
parentAXObject = parentAXObject->parentObjectUnignored();
Could this return nullptr? Let's null-check.

https://codereview.chromium.org/2390783006/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:749:
if (&childAXObject != inspectedAXObject &&
Let's break this into multiple if(...)continue.

if (&childAXObject == inspectedAXObject)
  continue;
...

https://codereview.chromium.org/2390783006/diff/460001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.h
(right):

https://codereview.chromium.org/2390783006/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.h:19:
using protocol::Accessibility::AXNode;
Move usings to private section (per style guide).

https://codereview.chromium.org/2390783006/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.h:40:
Member<Page> m_page;
Fields should be listed after methods.

[aboxhall, 2016-11-08 18:02:18]

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js
(right):

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js:301:
this._ensureSelection();
On 2016/11/08 16:38:41, dgozman wrote:
> On 2016/11/07 19:54:43, aboxhall wrote:
> > On 2016/11/04 23:22:45, dgozman wrote:
> > > Isn't this a private method of superclass? Isn't it called by
super.select()
> > > anyway?
> > 
> > Nope, not called by select():
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front...
> > 
> > Should I make it a public method? If I don't call it, the blue selection bar
> > doesn't appear.
> 
> Actually, it's called right after onattach, so it should be fine unless you
> removeChildren after that.
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front...

Ok, the issue is that it was being called earlier but early-outing. Fixed by
attaching parent element to tree sooner on line 54.

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js
(right):

https://codereview.chromium.org/2390783006/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:64:
return accessibilityModel.requestPartialAXTree(node)
On 2016/11/08 16:38:41, dgozman wrote:
> On 2016/11/07 19:54:43, aboxhall wrote:
> > On 2016/11/04 23:22:45, dgozman wrote:
> > > accessibilityModel.clear() 
> > 
> > Done.
> 
> Missed?

Oops, forgot to save!

https://codereview.chromium.org/2390783006/diff/460001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
(right):

https://codereview.chromium.org/2390783006/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:514:
parentAXObject = parentAXObject->parentObjectUnignored();
On 2016/11/08 16:38:41, dgozman wrote:
> Could this return nullptr? Let's null-check.

Done.

(This shouldn't happen in practice, since the root accessibility node shouldn't
be ignored, but just in case something weird happens.)

https://codereview.chromium.org/2390783006/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:749:
if (&childAXObject != inspectedAXObject &&
On 2016/11/08 16:38:41, dgozman wrote:
> Let's break this into multiple if(...)continue.
> 
> if (&childAXObject == inspectedAXObject)
>   continue;
> ...

Done.

https://codereview.chromium.org/2390783006/diff/460001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.h
(right):

https://codereview.chromium.org/2390783006/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.h:19:
using protocol::Accessibility::AXNode;
On 2016/11/08 16:38:41, dgozman wrote:
> Move usings to private section (per style guide).

I originally had that, but it doesn't compile:
error: using declaration in class refers into 'protocol::Accessibility::', which
is not a class
  using protocol::Accessibility::AXNode;

https://codereview.chromium.org/2390783006/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.h:40:
Member<Page> m_page;
On 2016/11/08 16:38:41, dgozman wrote:
> Fields should be listed after methods.

Done.

[dgozman, 2016-11-09 19:48:24]

lgtm

[aboxhall, 2016-11-09 19:51:35]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 19:52:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 21:46:58]

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

[commit-bot: I haz the power, 2016-11-09 21:46:59]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[aboxhall, 2016-11-10 17:23:24]

The CQ bit was checked by aboxhall@chromium.org

[aboxhall, 2016-11-10 17:23:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2390783006/#ps520001
(title: "Rebase tests again and be consistent about backendDOMNodeId")

[commit-bot: I haz the power, 2016-11-10 17:23:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 18:57:30]

Description was changed from

==========
Show siblings and children of selected node

BUG=560525
==========

to

==========
Show siblings and children of selected node

BUG=560525
==========

[commit-bot: I haz the power, 2016-11-10 18:57:32]

Committed patchset #27 (id:520001)

[commit-bot: I haz the power, 2016-11-10 19:09:13]

Description was changed from

==========
Show siblings and children of selected node

BUG=560525
==========

to

==========
Show siblings and children of selected node

BUG=560525

Committed: https://crrev.com/3a46cc72d58945804dd4919a679a8ce26690bfc0
Cr-Commit-Position: refs/heads/master@{#431304}
==========

[commit-bot: I haz the power, 2016-11-10 19:09:15]

Patchset 27 (id:??) landed as
https://crrev.com/3a46cc72d58945804dd4919a679a8ce26690bfc0
Cr-Commit-Position: refs/heads/master@{#431304}