[webkit-reviews] review granted: [Bug 201080] Web Inspector: AXI: Audit: obtuse error strings : [Attachment 408044] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted gr3g at apple.com's request for
review:
Bug 201080: Web Inspector: AXI: Audit: obtuse error strings
https://bugs.webkit.org/show_bug.cgi?id=201080

Attachment 408044: Patch

https://bugs.webkit.org/attachment.cgi?id=408044&action=review




--- Comment #4 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 408044
  --> https://bugs.webkit.org/attachment.cgi?id=408044
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408044&action=review

r=me, with some tweaks :)

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:1016
>		   ], {description: WI.UIString("These are all of the different
types of data that can be returned with the test result.")}),

let's be consistent with all of the group descriptions

    "These are example tests that demonstrate ..."

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:1019
> +			   new WI.AuditTestCase("getElementsByComputedRole",
removeWhitespace(getElementsByComputedRole), {description: WI.UIString("This is
an example test that uses the getElementsByComputedRole method to find elements
with a computed role of \u0022link\u0022."), supports: 1}),

I think we should be explicit about what we mean by `getElementsByComputedRole`
in that it's really
`WebInspectorAudit.Accessibility.getElementsByComputedRole`.  This way,
developers who look at this will know exactly what it's doing and how to
replicate it at a glance.

    WI.UIString("This is an example test that uses
WebInspectorAudit.Accessibility.getElementsByComputedRole to find elements with
a computed role of \u0022%s\u0022.").format(WI.unlocalizedString("link")

ditto for the other functions below

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:1029
>		       ], {description: WI.UIString("These tests demonstrate
how to use %s to get information about the accessibility
tree.").format(WI.unlocalizedString("WebInspectorAudit.Accessibility")),
supports: 1}),

ditto (:1016)

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:1033
>		       ], {description: WI.UIString("These tests demonstrate
how to use %s to get information about DOM
nodes.").format(WI.unlocalizedString("WebInspectorAudit.DOM")), supports: 1}),

ditto (:1016)

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:1039
> +		   new WI.AuditTestCase("unsupported",
removeWhitespace(unsupported), {description: WI.UIString("This is an example
test that shows the %s behavior when using an unsupported method or value. This
test will not run because it is
unsupported.").format(WI.unlocalizedString("WebInspectorAudit")), supports:
Infinity}),

This description isn't accurate, as it has nothing to do with
`WebInspectorAudit`.  It'd be more accurate to say:

    "This is an example of a test that will not run because it is unsupported."