Show reasons why nodes are ignored in accessibility sidebar

Show reasons why nodes are ignored in accessibility sidebar

BUG=475314
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194823

[aboxhall, 2015-04-09 00:54:29]

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

[aboxhall, 2015-04-09 00:54:29]

dmazzoni@: before I wire up all the UI, would you be able to take a look at the
protocol and back-end changes and see if all the reasons I'm detecting make
sense to you to expose?

[dmazzoni, 2015-04-09 05:46:11]

My worry here is that this seems to blur the line between correct specified
behavior and heuristics.

I'm not totally happy with computeAccessibilityIsIgnored now. If anything I'd
like to simplify it and have it ignore less. Maybe it'd make sense to separate
out the idea of things that are *hidden* from the accessibility tree
(aria-hidden, descendants of barren parents, etc.) from things like <span> and
LayoutBlocks that are pruned from the tree to keep the tree shallower - the
latter are just heuristics.

https://codereview.chromium.org/1076453004/diff/1/Source/devtools/protocol.json
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/1076453004/diff/1/Source/devtools/protocol.js...
Source/devtools/protocol.json:5054: "description": "Enum of possible reasons why
a node could be ignored for accessibility.o"
nit: "o" after period

https://codereview.chromium.org/1076453004/diff/1/Source/modules/accessibilit...
File Source/modules/accessibility/InspectorAccessibilityAgent.cpp (right):

https://codereview.chromium.org/1076453004/diff/1/Source/modules/accessibilit...
Source/modules/accessibility/InspectorAccessibilityAgent.cpp:412:
PassRefPtr<TypeBuilder::Array<AXProperty>> buildIgnoredReasons(Node* node,
AXObject* axObject, AXObjectCacheImpl* cacheImpl)
Should this be in AXLayoutObject? It feels like we're duplicating logic here.

What if computeAccessibilityIsIgnored returned an enum or a struct indicating
the reason?

https://codereview.chromium.org/1076453004/diff/1/Source/modules/accessibilit...
Source/modules/accessibility/InspectorAccessibilityAgent.cpp:514:
ignoredReasons->addItem(createProperty(AXIgnoredReasons::Focusable,
createBooleanValue(false)));
"not focusable" seems to be a strange reason to me.

[dmazzoni, 2015-04-09 05:53:16]

What would you think about:

* Reasons that a node is *definitely* ignored
* An "other* category indicating a node is heuristically ignored without
specifying a reason

https://codereview.chromium.org/1076453004/diff/1/Source/devtools/protocol.json
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/1076453004/diff/1/Source/devtools/protocol.js...
Source/devtools/protocol.json:5083: { "name": "hiddenTagName", "type": "string",
"description": "If this is a type of element (e.g. a span) which is always
hidden.", "optional": true },
This one is misleading, because <span> is not always hidden - for example if it
has an ARIA role it will be included.

Basically, this is a heuristic, we should figure out what to do with it.

https://codereview.chromium.org/1076453004/diff/1/Source/devtools/protocol.js...
Source/devtools/protocol.json:5086: { "name": "alt", "type": "boolean",
"description": "Whether this is an image with empty alt text.", "optional": true
},
Maybe call this emptyAlt?

https://codereview.chromium.org/1076453004/diff/1/Source/devtools/protocol.js...
Source/devtools/protocol.json:5087: { "name": "noTextAlternative", "type":
"boolean", "description": "Whether this element lacks a text alternative.",
"optional": true },
This also seems like a heuristic, because lots of nodes with no text alternative
might still be included in the AX tree

[aboxhall, 2015-04-09 20:09:06]

On 2015/04/09 05:46:11, dmazzoni wrote:
> My worry here is that this seems to blur the line between correct specified
> behavior and heuristics.
> 
> I'm not totally happy with computeAccessibilityIsIgnored now. If anything I'd
> like to simplify it and have it ignore less. Maybe it'd make sense to separate
> out the idea of things that are *hidden* from the accessibility tree
> (aria-hidden, descendants of barren parents, etc.) from things like <span> and
> LayoutBlocks that are pruned from the tree to keep the tree shallower - the
> latter are just heuristics.

Makes sense. I'll prioritise making UI for the former case while we figure out
what to do with the latter...

[aboxhall, 2015-04-09 20:15:37]

https://codereview.chromium.org/1076453004/diff/1/Source/devtools/protocol.json
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/1076453004/diff/1/Source/devtools/protocol.js...
Source/devtools/protocol.json:5083: { "name": "hiddenTagName", "type": "string",
"description": "If this is a type of element (e.g. a span) which is always
hidden.", "optional": true },
On 2015/04/09 05:53:16, dmazzoni wrote:
> This one is misleading, because <span> is not always hidden - for example if
it
> has an ARIA role it will be included.
> 
> Basically, this is a heuristic, we should figure out what to do with it.

Agreed; similarly for noTextAlternative.

https://codereview.chromium.org/1076453004/diff/1/Source/devtools/protocol.js...
Source/devtools/protocol.json:5086: { "name": "alt", "type": "boolean",
"description": "Whether this is an image with empty alt text.", "optional": true
},
On 2015/04/09 05:53:16, dmazzoni wrote:
> Maybe call this emptyAlt?

This object is actually going to disappear (I created it, then realised I was
better off using a simpler mechanism similar to the way I serialize properties,
but wanted to temporarily keep it around to refer to what types I had in mind
while I was coding the logic). The current logic returns an (empty) string for
this attribute, so this "documentation" is even less useful in this instance!

https://codereview.chromium.org/1076453004/diff/1/Source/modules/accessibilit...
File Source/modules/accessibility/InspectorAccessibilityAgent.cpp (right):

https://codereview.chromium.org/1076453004/diff/1/Source/modules/accessibilit...
Source/modules/accessibility/InspectorAccessibilityAgent.cpp:412:
PassRefPtr<TypeBuilder::Array<AXProperty>> buildIgnoredReasons(Node* node,
AXObject* axObject, AXObjectCacheImpl* cacheImpl)
On 2015/04/09 05:46:10, dmazzoni wrote:
> Should this be in AXLayoutObject? It feels like we're duplicating logic here.
> 
> What if computeAccessibilityIsIgnored returned an enum or a struct indicating
> the reason?

Yeah, I considered that. It would certainly be cleaner not to have to re-create
all this logic here. However, I feel like it would be potentially unnecessarily
expensive - this does things like fetching a reference to the node which caused
a particular state to exist (like the active modal dialog case) whereas the
actual logic only cares about whether or not it exists.

What do you think?

https://codereview.chromium.org/1076453004/diff/1/Source/modules/accessibilit...
Source/modules/accessibility/InspectorAccessibilityAgent.cpp:514:
ignoredReasons->addItem(createProperty(AXIgnoredReasons::Focusable,
createBooleanValue(false)));
On 2015/04/09 05:46:10, dmazzoni wrote:
> "not focusable" seems to be a strange reason to me.

As you say, it's more of a heuristic than a reason. I guess my reasoning is, in
the absence of a clear reason, I'd like to try and surface some factors that we
look at in computing our heuristics.

[dmazzoni, 2015-04-09 20:22:10]

>
> What if computeAccessibilityIsIgnored returned an enum or a struct
>>
> indicating
>
>> the reason?
>>
>
> Yeah, I considered that. It would certainly be cleaner not to have to
> re-create all this logic here. However, I feel like it would be
> potentially unnecessarily expensive - this does things like fetching a
> reference to the node which caused a particular state to exist (like the
> active modal dialog case) whereas the actual logic only cares about
> whether or not it exists.
>
> What do you think?


What if it took an optional argument containing a struct to dump the reason
and other data; if the struct is present it fills it in, but if not it
skips it. That way the non-Inspector case is just as fast but we keep the
code all in one place.

https://codereview.chromium.org/1076453004/diff/1/Source/
> modules/accessibility/InspectorAccessibilityAgent.cpp#newcode514
> Source/modules/accessibility/InspectorAccessibilityAgent.cpp:514:
> ignoredReasons->addItem(createProperty(AXIgnoredReasons::Focusable,
> createBooleanValue(false)));
> On 2015/04/09 05:46:10, dmazzoni wrote:
>
>> "not focusable" seems to be a strange reason to me.
>>
>
> As you say, it's more of a heuristic than a reason. I guess my reasoning
> is, in the absence of a clear reason, I'd like to try and surface some
> factors that we look at in computing our heuristics.
>

How about calling these "redundant"? If it doesn't fit any other reason,
then we're hiding a node from the AX tree because it seems to be redundant.
We could say that adding a role or any ARIA attribute automatically makes
it not redundant.

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

[aboxhall, 2015-04-09 20:36:34]

On 2015/04/09 20:22:10, dmazzoni wrote:
> >
> > What if computeAccessibilityIsIgnored returned an enum or a struct
> >>
> > indicating
> >
> >> the reason?
> >>
> >
> > Yeah, I considered that. It would certainly be cleaner not to have to
> > re-create all this logic here. However, I feel like it would be
> > potentially unnecessarily expensive - this does things like fetching a
> > reference to the node which caused a particular state to exist (like the
> > active modal dialog case) whereas the actual logic only cares about
> > whether or not it exists.
> >
> > What do you think?
> 
> 
> What if it took an optional argument containing a struct to dump the reason
> and other data; if the struct is present it fills it in, but if not it
> skips it. That way the non-Inspector case is just as fast but we keep the
> code all in one place.

I'll give it a go.

> https://codereview.chromium.org/1076453004/diff/1/Source/
> > modules/accessibility/InspectorAccessibilityAgent.cpp#newcode514
> > Source/modules/accessibility/InspectorAccessibilityAgent.cpp:514:
> > ignoredReasons->addItem(createProperty(AXIgnoredReasons::Focusable,
> > createBooleanValue(false)));
> > On 2015/04/09 05:46:10, dmazzoni wrote:
> >
> >> "not focusable" seems to be a strange reason to me.
> >>
> >
> > As you say, it's more of a heuristic than a reason. I guess my reasoning
> > is, in the absence of a clear reason, I'd like to try and surface some
> > factors that we look at in computing our heuristics.
> >
> 
> How about calling these "redundant"? If it doesn't fit any other reason,
> then we're hiding a node from the AX tree because it seems to be redundant.
> We could say that adding a role or any ARIA attribute automatically makes
> it not redundant.

Sure - do we want to actually surface information about those heuristics (e.g.
role, focusability, text alternatives...) here?

[dmazzoni, 2015-04-09 20:48:29]

On 2015/04/09 20:36:34, aboxhall wrote:
> On 2015/04/09 20:22:10, dmazzoni wrote:
> > >
> > > What if computeAccessibilityIsIgnored returned an enum or a struct
> > >>
> > > indicating
> > >
> > >> the reason?
> > >>
> > >
> > > Yeah, I considered that. It would certainly be cleaner not to have to
> > > re-create all this logic here. However, I feel like it would be
> > > potentially unnecessarily expensive - this does things like fetching a
> > > reference to the node which caused a particular state to exist (like the
> > > active modal dialog case) whereas the actual logic only cares about
> > > whether or not it exists.
> > >
> > > What do you think?
> > 
> > 
> > What if it took an optional argument containing a struct to dump the reason
> > and other data; if the struct is present it fills it in, but if not it
> > skips it. That way the non-Inspector case is just as fast but we keep the
> > code all in one place.
> 
> I'll give it a go.
> 
> > https://codereview.chromium.org/1076453004/diff/1/Source/
> > > modules/accessibility/InspectorAccessibilityAgent.cpp#newcode514
> > > Source/modules/accessibility/InspectorAccessibilityAgent.cpp:514:
> > > ignoredReasons->addItem(createProperty(AXIgnoredReasons::Focusable,
> > > createBooleanValue(false)));
> > > On 2015/04/09 05:46:10, dmazzoni wrote:
> > >
> > >> "not focusable" seems to be a strange reason to me.
> > >>
> > >
> > > As you say, it's more of a heuristic than a reason. I guess my reasoning
> > > is, in the absence of a clear reason, I'd like to try and surface some
> > > factors that we look at in computing our heuristics.
> > >
> > 
> > How about calling these "redundant"? If it doesn't fit any other reason,
> > then we're hiding a node from the AX tree because it seems to be redundant.
> > We could say that adding a role or any ARIA attribute automatically makes
> > it not redundant.
> 
> Sure - do we want to actually surface information about those heuristics (e.g.
> role, focusability, text alternatives...) here?

I guess my hope here is that we can avoid surfacing heuristics. If they're
complicated to explain, that's a good sign we just want to get rid of them.

I'd like to seriously consider just including those <span> elements in the AX
tree. If we decide that's a bad idea, we should be able to articulate why and
come up with a clear, concise rule rather than a heuristic.

[aboxhall, 2015-04-14 20:49:58]

Patchset #2 (id:20001) has been deleted

[aboxhall, 2015-04-14 20:52:39]

Not ready to submit, but ready for another look.

[dmazzoni, 2015-04-15 17:00:02]

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

https://codereview.chromium.org/1076453004/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXLayoutObject.cpp:515:
ignoredReasons->addItem(createProperty(AXIgnoredReasons::AncestorIsLeafNode,
createRelatedNodeValue(leafNodeAncestor())));
Perhaps you could add a helper function for this. It'd be nice if it could be as
short as this:

addIgnoredReason(AncestorIsLeafNode, leafNodeAncestor());

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

https://codereview.chromium.org/1076453004/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXObject.cpp:589: }
Handle the else case?

In the future there may be an "inert" attribute, so a node could be inert but
not due to a dialog.

https://codereview.chromium.org/1076453004/diff/60001/Source/modules/accessib...
File Source/modules/accessibility/AXObject.h (right):

https://codereview.chromium.org/1076453004/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXObject.h:447: virtual bool
computeAccessibilityIsIgnored(PassRefPtr<TypeBuilder::Array<TypeBuilder::Accessibility::AXProperty>>
= PassRefPtr<TypeBuilder::Array<TypeBuilder::Accessibility::AXProperty>>())
const { return true; }
How about a typedef for
PassRefPtr<TypeBuilder::Array<TypeBuilder::Accessibility::AXProperty>>

https://codereview.chromium.org/1076453004/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXObject.h:448: bool
accessibilityIsIgnoredByDefault(PassRefPtr<TypeBuilder::Array<TypeBuilder::Accessibility::AXProperty>>
= PassRefPtr<TypeBuilder::Array<TypeBuilder::Accessibility::AXProperty>>())
const;
accessibilityIsIgnoredByDefault hardly does anything - it's a left over from
WebKit when it did something different on each platform.

Maybe consider first doing a quick cleanup change to get rid of it and fold it
into computeAccessibilityIsIgnored?

https://codereview.chromium.org/1076453004/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXObject.h:454: bool isDescendantOfLeafNode()
const;
This sounds great, but let's split the renames into a separate (trivial) change

[aboxhall, 2015-04-15 17:55:46]

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

https://codereview.chromium.org/1076453004/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXLayoutObject.cpp:515:
ignoredReasons->addItem(createProperty(AXIgnoredReasons::AncestorIsLeafNode,
createRelatedNodeValue(leafNodeAncestor())));
On 2015/04/15 17:00:02, dmazzoni wrote:
> Perhaps you could add a helper function for this. It'd be nice if it could be
as
> short as this:
> 
> addIgnoredReason(AncestorIsLeafNode, leafNodeAncestor());

I feel silly for not thinking of this. Done.

https://codereview.chromium.org/1076453004/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXLayoutObject.cpp:515:
ignoredReasons->addItem(createProperty(AXIgnoredReasons::AncestorIsLeafNode,
createRelatedNodeValue(leafNodeAncestor())));
On 2015/04/15 17:00:02, dmazzoni wrote:
> Perhaps you could add a helper function for this. It'd be nice if it could be
as
> short as this:
> 
> addIgnoredReason(AncestorIsLeafNode, leafNodeAncestor());

Great idea, done.

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

https://codereview.chromium.org/1076453004/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXObject.cpp:589: }
On 2015/04/15 17:00:02, dmazzoni wrote:
> Handle the else case?
> 
> In the future there may be an "inert" attribute, so a node could be inert but
> not due to a dialog.

But it'll be due to an inert attribute somewhere, right? So we'll need to do
extra work at that point anyway, similar to what's done for aria-hidden here.

https://codereview.chromium.org/1076453004/diff/60001/Source/modules/accessib...
File Source/modules/accessibility/AXObject.h (right):

https://codereview.chromium.org/1076453004/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXObject.h:447: virtual bool
computeAccessibilityIsIgnored(PassRefPtr<TypeBuilder::Array<TypeBuilder::Accessibility::AXProperty>>
= PassRefPtr<TypeBuilder::Array<TypeBuilder::Accessibility::AXProperty>>())
const { return true; }
On 2015/04/15 17:00:02, dmazzoni wrote:
> How about a typedef for
> PassRefPtr<TypeBuilder::Array<TypeBuilder::Accessibility::AXProperty>>

Done.

https://codereview.chromium.org/1076453004/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXObject.h:448: bool
accessibilityIsIgnoredByDefault(PassRefPtr<TypeBuilder::Array<TypeBuilder::Accessibility::AXProperty>>
= PassRefPtr<TypeBuilder::Array<TypeBuilder::Accessibility::AXProperty>>())
const;
On 2015/04/15 17:00:02, dmazzoni wrote:
> accessibilityIsIgnoredByDefault hardly does anything - it's a left over from
> WebKit when it did something different on each platform.
> 
> Maybe consider first doing a quick cleanup change to get rid of it and fold it
> into computeAccessibilityIsIgnored?

Ok, working on that now.

https://codereview.chromium.org/1076453004/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXObject.h:454: bool isDescendantOfLeafNode()
const;
On 2015/04/15 17:00:02, dmazzoni wrote:
> This sounds great, but let's split the renames into a separate (trivial)
change

I can do that, but note that it's a type change for the related method as well
as a rename (and this change won't work without the latter).

[aboxhall, 2015-04-22 21:59:20]

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

[aboxhall, 2015-04-22 23:21:05]

Still adding tests, but PTAL in the meantime.

[dmazzoni, 2015-04-23 05:28:39]

https://codereview.chromium.org/1076453004/diff/120001/Source/modules/accessi...
File Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/1076453004/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXLayoutObject.cpp:512: if (roleValue() ==
IgnoredRole) {
Worth cleaning this up maybe. The only two places this seem to be used now are
for the <html> element and the LayoutTableSection LayoutObject. I wonder if it
wouldn't make more sense just to merge that with "presentational"?

https://codereview.chromium.org/1076453004/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXLayoutObject.cpp:537: // ignore popup menu items
because AppKit does
ooh, I think my recent change made this block unnecessary because it's now
covered by ancestorForWhichThisIsAPresentationalChild. It should be much better
to call that directly rather than special casing the menu list.

https://codereview.chromium.org/1076453004/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXLayoutObject.cpp:557:
ignoredReasons->append(IgnoredReason(AXLabelContainer, labelAXObject));
Null-check labelAXObject, even if there isn't any sensible way it should be null
- since getOrCreate() is allowed to return null, we should be safe

https://codereview.chromium.org/1076453004/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXLayoutObject.cpp:664: if
(isHTMLSpanElement(node)) {
fwiw, i'd like to get rid of this rule. there's nothing magic about <span> that
makes it less interesting than a <div>, I think we should have more general
rules

https://codereview.chromium.org/1076453004/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXLayoutObject.cpp:705:
ignoredReasons->append(IgnoredReason(AXProbablyPresentational));
How about an explicit reason for this one, like AXZeroSizeImage?

https://codereview.chromium.org/1076453004/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXLayoutObject.cpp:714:
ignoredReasons->append(IgnoredReason(AXProbablyPresentational));
Same

https://codereview.chromium.org/1076453004/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXLayoutObject.cpp:729:
ignoredReasons->append(IgnoredReason(AXProbablyPresentational));
Same

https://codereview.chromium.org/1076453004/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXLayoutObject.cpp:760:
ignoredReasons->append(IgnoredReason(AXUninteresting));
If we skip <span>, it should be handled here.

To clarify, I'm not asking you to make those changes in this changelist, but you
should keep them in mind when deciding what the right set of reasons are. A
reason whose only purpose is to support a special case that we don't actually
want to be special might be one to eliminate.

[aboxhall, 2015-04-28 02:08:04]

Patchset #7 (id:140001) has been deleted

[aboxhall, 2015-04-28 02:08:13]

Patchset #7 (id:160001) has been deleted

[aboxhall, 2015-04-28 02:23:42]

Patchset #7 (id:180001) has been deleted

[aboxhall, 2015-04-28 02:28:42]

Patchset #7 (id:200001) has been deleted

[aboxhall, 2015-04-28 02:31:45]

Patchset #7 (id:220001) has been deleted

[aboxhall, 2015-04-28 02:35:16]

Patchset #7 (id:240001) has been deleted

[aboxhall, 2015-04-28 02:36:12]

pfeldman: test ready now, PTAL.

https://codereview.chromium.org/1076453004/diff/120001/Source/modules/accessi...
File Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/1076453004/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXLayoutObject.cpp:512: if (roleValue() ==
IgnoredRole) {
On 2015/04/23 05:28:38, dmazzoni wrote:
> Worth cleaning this up maybe. The only two places this seem to be used now are
> for the <html> element and the LayoutTableSection LayoutObject. I wonder if it
> wouldn't make more sense just to merge that with "presentational"?

I'd prefer to try not to make any functional changes in this change if I can
avoid it, to avoid potentially breaking layout tests. I can follow up with this
if you like (since it doesn't involve changing the enum at all).

https://codereview.chromium.org/1076453004/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXLayoutObject.cpp:537: // ignore popup menu items
because AppKit does
On 2015/04/23 05:28:38, dmazzoni wrote:
> ooh, I think my recent change made this block unnecessary because it's now
> covered by ancestorForWhichThisIsAPresentationalChild. It should be much
better
> to call that directly rather than special casing the menu list.
> 

Done.

https://codereview.chromium.org/1076453004/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXLayoutObject.cpp:557:
ignoredReasons->append(IgnoredReason(AXLabelContainer, labelAXObject));
On 2015/04/23 05:28:38, dmazzoni wrote:
> Null-check labelAXObject, even if there isn't any sensible way it should be
null
> - since getOrCreate() is allowed to return null, we should be safe
> 

There's nothing I can really do about it if it's null, though. It won't break
anything, it'll just render a little oddly (and I can even fix that on the
presentation side if we really care).

https://codereview.chromium.org/1076453004/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXLayoutObject.cpp:664: if
(isHTMLSpanElement(node)) {
On 2015/04/23 05:28:38, dmazzoni wrote:
> fwiw, i'd like to get rid of this rule. there's nothing magic about <span>
that
> makes it less interesting than a <div>, I think we should have more general
> rules

Agreed. Let's chat about this for the follow up refactoring/simplifying CL.

https://codereview.chromium.org/1076453004/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXLayoutObject.cpp:705:
ignoredReasons->append(IgnoredReason(AXProbablyPresentational));
On 2015/04/23 05:28:38, dmazzoni wrote:
> How about an explicit reason for this one, like AXZeroSizeImage?

I figured this was more on the heuristic side.

https://codereview.chromium.org/1076453004/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXLayoutObject.cpp:714:
ignoredReasons->append(IgnoredReason(AXProbablyPresentational));
On 2015/04/23 05:28:38, dmazzoni wrote:
> Same

As above.

https://codereview.chromium.org/1076453004/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXLayoutObject.cpp:729:
ignoredReasons->append(IgnoredReason(AXProbablyPresentational));
On 2015/04/23 05:28:38, dmazzoni wrote:
> Same

As above.

https://codereview.chromium.org/1076453004/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXLayoutObject.cpp:760:
ignoredReasons->append(IgnoredReason(AXUninteresting));
On 2015/04/23 05:28:38, dmazzoni wrote:
> If we skip <span>, it should be handled here.
> 
> To clarify, I'm not asking you to make those changes in this changelist, but
you
> should keep them in mind when deciding what the right set of reasons are. A
> reason whose only purpose is to support a special case that we don't actually
> want to be special might be one to eliminate.

Acknowledged.

[aboxhall, 2015-04-28 14:53:34]

Ping?

[pfeldman, 2015-04-28 15:49:36]

https://codereview.chromium.org/1076453004/diff/260001/LayoutTests/http/tests...
File LayoutTests/http/tests/inspector-protocol/inspector-protocol-test.js
(right):

https://codereview.chromium.org/1076453004/diff/260001/LayoutTests/http/tests...
LayoutTests/http/tests/inspector-protocol/inspector-protocol-test.js:123: else
if (/^common_/.test(symbol))
Revert this, use initialize

https://codereview.chromium.org/1076453004/diff/260001/LayoutTests/inspector-...
File
LayoutTests/inspector-protocol/accessibility/accessibility-getRelationships-expected.txt
(right):

https://codereview.chromium.org/1076453004/diff/260001/LayoutTests/inspector-...
LayoutTests/inspector-protocol/accessibility/accessibility-getRelationships-expected.txt:1:
CONSOLE WARNING: 'window.webkitStorageInfo' is deprecated. Please use
'navigator.webkitTemporaryStorage' or 'navigator.webkitPersistentStorage'
instead.
This test will fail when these unrelated warnings are changed / removed.

https://codereview.chromium.org/1076453004/diff/260001/LayoutTests/inspector-...
File
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodes-common.js
(right):

https://codereview.chromium.org/1076453004/diff/260001/LayoutTests/inspector-...
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodes-common.js:1:
function common_onGotDocument(maxId, msg)
Can this be named in a way that it was clear what it does?

https://codereview.chromium.org/1076453004/diff/260001/LayoutTests/inspector-...
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodes-common.js:9:
function sendCommandPromise(command, params) {
Here and below, poor coding style.

https://codereview.chromium.org/1076453004/diff/260001/LayoutTests/inspector-...
File
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodes.html
(right):

https://codereview.chromium.org/1076453004/diff/260001/LayoutTests/inspector-...
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodes.html:8:
function test() {
\n before {

https://codereview.chromium.org/1076453004/diff/260001/Source/devtools/front_...
File Source/devtools/front_end/accessibility/AccessibilitySidebarView.js
(right):

https://codereview.chromium.org/1076453004/diff/260001/Source/devtools/front_...
Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:62:
this._noNodeInfo = createElementWithClass("div", "info");
Is this in the global CSS space? Please use full qualified names.

https://codereview.chromium.org/1076453004/diff/260001/Source/devtools/front_...
Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:141: var
target = this.parentView().parentView().node().target();
This is very fragile, please pass target in a reasonable manner.

https://codereview.chromium.org/1076453004/diff/260001/Source/devtools/front_...
Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:152:
console.log('ignored node:', axNode);
remove

https://codereview.chromium.org/1076453004/diff/260001/Source/devtools/front_...
Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:159:
function addIgnoredReason(property) {
code style, annotations.

https://codereview.chromium.org/1076453004/diff/260001/Source/devtools/front_...
Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:164:
console.log("ignoredReason: ", reason);
remove

https://codereview.chromium.org/1076453004/diff/260001/Source/devtools/front_...
Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:433:
this._update();
inline?

[aboxhall, 2015-04-28 16:00:03]

https://codereview.chromium.org/1076453004/diff/260001/LayoutTests/http/tests...
File LayoutTests/http/tests/inspector-protocol/inspector-protocol-test.js
(right):

https://codereview.chromium.org/1076453004/diff/260001/LayoutTests/http/tests...
LayoutTests/http/tests/inspector-protocol/inspector-protocol-test.js:123: else
if (/^common_/.test(symbol))
On 2015/04/28 15:49:36, pfeldman wrote:
> Revert this, use initialize

Note that this isn't using an IIFE, so initialize isn't appropriate.

[aboxhall, 2015-04-28 16:01:16]

https://codereview.chromium.org/1076453004/diff/260001/LayoutTests/inspector-...
File
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodes-common.js
(right):

https://codereview.chromium.org/1076453004/diff/260001/LayoutTests/inspector-...
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodes-common.js:9:
function sendCommandPromise(command, params) {
On 2015/04/28 15:49:36, pfeldman wrote:
> Here and below, poor coding style.

Could you give some more detail on this? What problem are you referring to?

[aboxhall, 2015-04-28 16:08:33]

https://codereview.chromium.org/1076453004/diff/260001/LayoutTests/inspector-...
File
LayoutTests/inspector-protocol/accessibility/accessibility-getRelationships-expected.txt
(right):

https://codereview.chromium.org/1076453004/diff/260001/LayoutTests/inspector-...
LayoutTests/inspector-protocol/accessibility/accessibility-getRelationships-expected.txt:1:
CONSOLE WARNING: 'window.webkitStorageInfo' is deprecated. Please use
'navigator.webkitTemporaryStorage' or 'navigator.webkitPersistentStorage'
instead.
On 2015/04/28 15:49:36, pfeldman wrote:
> This test will fail when these unrelated warnings are changed / removed.

Yes? And it will fail until then unless I add them in to the expectation, won't
it?

[pfeldman, 2015-04-28 16:13:15]

> Yes? And it will fail until then unless I add them in to the expectation,
won't
> it?

Is there a way to not provoke them? (Or catch / suppress them?)

[aboxhall, 2015-04-28 16:17:23]

On 2015/04/28 16:13:15, pfeldman wrote:
> > Yes? And it will fail until then unless I add them in to the expectation,
> won't
> > it?
> 
> Is there a way to not provoke them? (Or catch / suppress them?)

They seem to be spread throughout the layout tests - other test expectations
include them as well. They're not a result of anything I changed, nor anything I
could see how to fix.

[aboxhall, 2015-04-28 18:14:13]

Patchset #8 (id:280001) has been deleted

[aboxhall, 2015-04-28 18:14:20]

https://codereview.chromium.org/1076453004/diff/260001/LayoutTests/http/tests...
File LayoutTests/http/tests/inspector-protocol/inspector-protocol-test.js
(right):

https://codereview.chromium.org/1076453004/diff/260001/LayoutTests/http/tests...
LayoutTests/http/tests/inspector-protocol/inspector-protocol-test.js:123: else
if (/^common_/.test(symbol))
On 2015/04/28 16:00:03, aboxhall wrote:
> On 2015/04/28 15:49:36, pfeldman wrote:
> > Revert this, use initialize
> 
> Note that this isn't using an IIFE, so initialize isn't appropriate.

Done.

https://codereview.chromium.org/1076453004/diff/260001/LayoutTests/inspector-...
File
LayoutTests/inspector-protocol/accessibility/accessibility-getRelationships-expected.txt
(right):

https://codereview.chromium.org/1076453004/diff/260001/LayoutTests/inspector-...
LayoutTests/inspector-protocol/accessibility/accessibility-getRelationships-expected.txt:1:
CONSOLE WARNING: 'window.webkitStorageInfo' is deprecated. Please use
'navigator.webkitTemporaryStorage' or 'navigator.webkitPersistentStorage'
instead.
On 2015/04/28 16:08:33, aboxhall wrote:
> On 2015/04/28 15:49:36, pfeldman wrote:
> > This test will fail when these unrelated warnings are changed / removed.
> 
> Yes? And it will fail until then unless I add them in to the expectation,
won't
> it?

Done.

https://codereview.chromium.org/1076453004/diff/260001/LayoutTests/inspector-...
File
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodes-common.js
(right):

https://codereview.chromium.org/1076453004/diff/260001/LayoutTests/inspector-...
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodes-common.js:1:
function common_onGotDocument(maxId, msg)
On 2015/04/28 15:49:36, pfeldman wrote:
> Can this be named in a way that it was clear what it does?

Done.

https://codereview.chromium.org/1076453004/diff/260001/LayoutTests/inspector-...
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodes-common.js:9:
function sendCommandPromise(command, params) {
On 2015/04/28 16:01:16, aboxhall wrote:
> On 2015/04/28 15:49:36, pfeldman wrote:
> > Here and below, poor coding style.
> 
> Could you give some more detail on this? What problem are you referring to?

Done, I think.

https://codereview.chromium.org/1076453004/diff/260001/LayoutTests/inspector-...
File
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodes.html
(right):

https://codereview.chromium.org/1076453004/diff/260001/LayoutTests/inspector-...
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodes.html:8:
function test() {
On 2015/04/28 15:49:36, pfeldman wrote:
> \n before {

Done.

[pfeldman, 2015-04-29 13:59:22]

https://codereview.chromium.org/1076453004/diff/300001/LayoutTests/inspector-...
File
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodesTest.js
(right):

https://codereview.chromium.org/1076453004/diff/300001/LayoutTests/inspector-...
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodesTest.js:1:
function initialize_IgnoredNodesTests() {
insert copyright, unindent contents.

https://codereview.chromium.org/1076453004/diff/300001/LayoutTests/inspector-...
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodesTest.js:2:
InspectorTest.dumpAccessibilityNodesByIdref = function(ids, msg)
...AndCompleteTest

https://codereview.chromium.org/1076453004/diff/300001/LayoutTests/inspector-...
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodesTest.js:26:
function sendCommandPromise(command, params)
These should all be prefixed with InspectorTest.

private ones should start with _.

https://codereview.chromium.org/1076453004/diff/300001/Source/devtools/front_...
File Source/devtools/front_end/accessibility/AccessibilitySidebarView.js
(right):

https://codereview.chromium.org/1076453004/diff/300001/Source/devtools/front_...
Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:433:
this._update();
Inline this method?

https://codereview.chromium.org/1076453004/diff/300001/Source/devtools/front_...
Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:463:
reasonElement.innerHTML = "Element is hidden by active modal dialog: ";
Never ever ever ever use innerHTML =, should be textContent =.

[aboxhall, 2015-04-29 19:07:09]

Patchset #9 (id:320001) has been deleted

[aboxhall, 2015-04-29 19:08:12]

https://codereview.chromium.org/1076453004/diff/300001/LayoutTests/inspector-...
File
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodesTest.js
(right):

https://codereview.chromium.org/1076453004/diff/300001/LayoutTests/inspector-...
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodesTest.js:1:
function initialize_IgnoredNodesTests() {
On 2015/04/29 13:59:22, pfeldman wrote:
> insert copyright, unindent contents.

Done.

https://codereview.chromium.org/1076453004/diff/300001/LayoutTests/inspector-...
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodesTest.js:2:
InspectorTest.dumpAccessibilityNodesByIdref = function(ids, msg)
On 2015/04/29 13:59:22, pfeldman wrote:
> ...AndCompleteTest

Done.

https://codereview.chromium.org/1076453004/diff/300001/LayoutTests/inspector-...
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodesTest.js:26:
function sendCommandPromise(command, params)
On 2015/04/29 13:59:22, pfeldman wrote:
> These should all be prefixed with InspectorTest.
> 
> private ones should start with _.

Done, but:
What benefit does this provide? Won't these functions already be scoped to the
closure? Surely this just makes the code less readable and risks clobbering
things already on InspectorTest.

https://codereview.chromium.org/1076453004/diff/300001/Source/devtools/front_...
File Source/devtools/front_end/accessibility/AccessibilitySidebarView.js
(right):

https://codereview.chromium.org/1076453004/diff/300001/Source/devtools/front_...
Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:433:
this._update();
On 2015/04/29 13:59:22, pfeldman wrote:
> Inline this method?

Done.

https://codereview.chromium.org/1076453004/diff/300001/Source/devtools/front_...
Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:463:
reasonElement.innerHTML = "Element is hidden by active modal dialog: ";
On 2015/04/29 13:59:22, pfeldman wrote:
> Never ever ever ever use innerHTML =, should be textContent =.

Done.

[pfeldman, 2015-04-29 19:38:46]

devtools lgtm % nits

https://codereview.chromium.org/1076453004/diff/340001/LayoutTests/inspector-...
File
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodesTest.js
(right):

https://codereview.chromium.org/1076453004/diff/340001/LayoutTests/inspector-...
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodesTest.js:23:
.catch(function(msg) { InspectorTest.log("Error: " + JSON.stringify(msg)); })
Lets insert some new lines here.

https://codereview.chromium.org/1076453004/diff/340001/LayoutTests/inspector-...
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodesTest.js:30:
InspectorTest._sendCommandPromise = function(command, params)
These are not a part of closure. This whole closure is being evaluated in on the
global object, so all these functions are available to the caller within test.

https://codereview.chromium.org/1076453004/diff/340001/LayoutTests/inspector-...
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodesTest.js:34:
if (msg.error) { reject(msg.error); return; }
Lets insert some new lines here.

https://codereview.chromium.org/1076453004/diff/340001/LayoutTests/inspector-...
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodesTest.js:46:
InspectorTest._getAXNode = function(msg) {
{ -> next line

https://codereview.chromium.org/1076453004/diff/340001/Source/devtools/front_...
File Source/devtools/front_end/accessibility/AccessibilitySidebarView.js
(right):

https://codereview.chromium.org/1076453004/diff/340001/Source/devtools/front_...
Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:457:
reasonElement.createTextChild("Element is hidden by active modal dialog: ");
Wrap these with WebInspector.UIString()

[aboxhall, 2015-04-29 21:02:49]

Patchset #10 (id:360001) has been deleted

[aboxhall, 2015-04-29 21:03:32]

https://codereview.chromium.org/1076453004/diff/340001/LayoutTests/inspector-...
File
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodesTest.js
(right):

https://codereview.chromium.org/1076453004/diff/340001/LayoutTests/inspector-...
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodesTest.js:23:
.catch(function(msg) { InspectorTest.log("Error: " + JSON.stringify(msg)); })
On 2015/04/29 19:38:46, pfeldman wrote:
> Lets insert some new lines here.

Done.

https://codereview.chromium.org/1076453004/diff/340001/LayoutTests/inspector-...
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodesTest.js:30:
InspectorTest._sendCommandPromise = function(command, params)
On 2015/04/29 19:38:46, pfeldman wrote:
> These are not a part of closure. This whole closure is being evaluated in on
the
> global object, so all these functions are available to the caller within test.

Acknowledged.

https://codereview.chromium.org/1076453004/diff/340001/LayoutTests/inspector-...
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodesTest.js:34:
if (msg.error) { reject(msg.error); return; }
On 2015/04/29 19:38:46, pfeldman wrote:
> Lets insert some new lines here.

Done.

https://codereview.chromium.org/1076453004/diff/340001/LayoutTests/inspector-...
LayoutTests/inspector-protocol/accessibility/accessibility-ignoredNodesTest.js:46:
InspectorTest._getAXNode = function(msg) {
On 2015/04/29 19:38:46, pfeldman wrote:
> { -> next line

Done.

https://codereview.chromium.org/1076453004/diff/340001/Source/devtools/front_...
File Source/devtools/front_end/accessibility/AccessibilitySidebarView.js
(right):

https://codereview.chromium.org/1076453004/diff/340001/Source/devtools/front_...
Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:457:
reasonElement.createTextChild("Element is hidden by active modal dialog: ");
On 2015/04/29 19:38:46, pfeldman wrote:
> Wrap these with WebInspector.UIString()

Done.

[aboxhall, 2015-04-29 21:15:26]

dmazzoni: PTAL?

[dmazzoni, 2015-04-30 15:06:40]

lgtm

https://codereview.chromium.org/1076453004/diff/380001/Source/modules/accessi...
File Source/modules/accessibility/AXObject.h (right):

https://codereview.chromium.org/1076453004/diff/380001/Source/modules/accessi...
Source/modules/accessibility/AXObject.h:322: AXPresentationRole,
How about PresentationalRole since it could be either role=presentation or
role=none?

[aboxhall, 2015-04-30 20:22:22]

Patchset #11 (id:400001) has been deleted

[aboxhall, 2015-04-30 20:23:17]

https://codereview.chromium.org/1076453004/diff/260001/Source/devtools/front_...
File Source/devtools/front_end/accessibility/AccessibilitySidebarView.js
(right):

https://codereview.chromium.org/1076453004/diff/260001/Source/devtools/front_...
Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:62:
this._noNodeInfo = createElementWithClass("div", "info");
On 2015/04/28 15:49:36, pfeldman wrote:
> Is this in the global CSS space? Please use full qualified names.

This is deliberately reusing the global style.

https://codereview.chromium.org/1076453004/diff/260001/Source/devtools/front_...
Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:141: var
target = this.parentView().parentView().node().target();
On 2015/04/28 15:49:36, pfeldman wrote:
> This is very fragile, please pass target in a reasonable manner.

Done.

https://codereview.chromium.org/1076453004/diff/260001/Source/devtools/front_...
Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:152:
console.log('ignored node:', axNode);
On 2015/04/28 15:49:36, pfeldman wrote:
> remove

Done.

https://codereview.chromium.org/1076453004/diff/260001/Source/devtools/front_...
Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:159:
function addIgnoredReason(property) {
On 2015/04/28 15:49:36, pfeldman wrote:
> code style, annotations.

Done.

https://codereview.chromium.org/1076453004/diff/260001/Source/devtools/front_...
Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:164:
console.log("ignoredReason: ", reason);
On 2015/04/28 15:49:36, pfeldman wrote:
> remove

Done.

https://codereview.chromium.org/1076453004/diff/260001/Source/devtools/front_...
Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:433:
this._update();
On 2015/04/28 15:49:36, pfeldman wrote:
> inline?

Done.

[aboxhall, 2015-04-30 20:23:27]

The CQ bit was checked by aboxhall@chromium.org

[aboxhall, 2015-04-30 20:23:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org, dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1076453004/#ps420001
(title: "dmazzoni review comments plus a few stragglers from pfeldman")

[commit-bot: I haz the power, 2015-04-30 20:23:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1076453004/420001

[commit-bot: I haz the power, 2015-04-30 20:27:09]

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

[commit-bot: I haz the power, 2015-04-30 20:27:11]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/54053)

[aboxhall, 2015-04-30 21:15:30]

The CQ bit was checked by aboxhall@chromium.org

[aboxhall, 2015-04-30 21:15:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1076453004/#ps440001
(title: "Rebase")

[commit-bot: I haz the power, 2015-04-30 21:16:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1076453004/440001

[aboxhall, 2015-04-30 21:19:52]

The CQ bit was unchecked by aboxhall@chromium.org

[aboxhall, 2015-05-01 02:52:23]

The CQ bit was checked by aboxhall@chromium.org

[aboxhall, 2015-05-01 02:52:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1076453004/#ps460001
(title: "Rebaseline tests")

[commit-bot: I haz the power, 2015-05-01 02:52:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1076453004/460001

[commit-bot: I haz the power, 2015-05-01 03:13:48]

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

[commit-bot: I haz the power, 2015-05-01 03:13:49]

Try jobs failed on following builders:
  win_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[aboxhall, 2015-05-01 17:11:41]

The CQ bit was checked by aboxhall@chromium.org

[aboxhall, 2015-05-01 17:11:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1076453004/#ps480001
(title: "Add a catch-all return to ignoredReasonName")

[commit-bot: I haz the power, 2015-05-01 17:12:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1076453004/480001

[commit-bot: I haz the power, 2015-05-01 18:49:00]

Committed patchset #14 (id:480001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194823