Bug 44518 – Web Inspector: add audits support to extension API

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Andrey Kosyakov]

Add support for adding audit categories from WebInspector extensions.

[Andrey Kosyakov]

Created attachment 65263 [details]
patch

[Pavel Feldman]

Comment on attachment 65263 [details]
patch

Overall looks good, few nits below. r- for naming + formatter.

LayoutTests/inspector/extensions-audits-expected.txt:16
 +      cancel : <function>
cancel -> done

LayoutTests/inspector/extensions-audits.html:11
 +      function onStartAuditCategory(auditRun)
auditRun -> testResult

WebCore/inspector/front-end/AuditLauncherView.js:138
 +              insertBefore < this._sortedCategories.length && this._compareCategories(category, this._sortedCategories[insertBefore]) >= 0;
Please fix indentation.

WebCore/inspector/front-end/AuditLauncherView.js:142
 +              this._categoriesElement.insertBefore(categoryElement, this._categoriesElement.children[insertBefore]);
insertBefore undefined will do appendChild, no need to fork the code.

WebCore/inspector/front-end/AuditLauncherView.js:130
 +          categoryElement.firstChild.addEventListener("click", this._boundCategoryClickListener, false);
you could encapsulate this listener addition into the _createCategoryElement in order not to be involved with the internal structure here.

WebCore/inspector/front-end/AuditLauncherView.js:137
 +          for (var insertBefore = 0;
I just found indexOfObjectInListSortedByFunction in the utilities.js

WebCore/inspector/front-end/ExtensionAPI.js:50
 +          if (typeof callback != "function")
!==

WebCore/inspector/front-end/ExtensionAuditCategory.js:55
 +      runRules: function(resources, callback)
runRules -> run ?

WebCore/inspector/front-end/ExtensionAuditCategory.js:83
 +      addRuleResult: function(ruleDisplayName, severity, violationCount, details)
addResult ?

LayoutTests/inspector/extensions-audits.html:24
 +              type: webInspector.audits.nodeType.text,
I'd suggest that we use the following form text parameters:
["Foo bar %h", "http://link"].
"children" should still be there in order to organize a very common tree structure.
We could come up with more formatters as we go:
%s for string
%t for tree
%T for table and such...

[Andrey Kosyakov]

(In reply to comment #2)

> LayoutTests/inspector/extensions-audits.html:24
>  +              type: webInspector.audits.nodeType.text,
> I'd suggest that we use the following form text parameters:
> ["Foo bar %h", "http://link"].
> "children" should still be there in order to organize a very common tree structure.
> We could come up with more formatters as we go:
> %s for string
> %t for tree
> %T for table and such...

I realize this may be handy, but still don't like this: it's not readable, is not localizable (unless we add parameter numbering there), not quite extensible and does not quite suit to a case when rendering one entity requires multiple values (e.g. URL could have two: a href value and a text within the tag).

I suggest we either define named formatter classes, e.g.

webInspector.audit.formatters.url(href, text)
webInspector.audit.formatters.snippet(text)
webInspector.audit.formatters.subtree(expanded, root, children) etc.

or go for imperative interface, e.g.

results.addChild(webInspector.audit.resultNodes.url(href, text))

The latter is pretty close to the way we have it in Audits now, but keeps type away from method names (i.e. we don't have AddFoo for every type Foo -- rather have Foo constructors and generic AddChild method).

What do you think?

[Pavel Feldman]

> webInspector.audit.formatters.url(href, text)
> webInspector.audit.formatters.snippet(text)
> webInspector.audit.formatters.subtree(expanded, root, children) etc.
> 

I don't see how this addresses disadvantages pointed out + it does not handle mixed content such as text with links well. I don't see a point in additional 'formatters' object, i'd rather add methods on results:

var result = auditResults.addResult();
result.title = "Leverage browser caching";
result.details = ["See more info %h", ["http://foo", "here"]];
var childResult = result.addChild();
childResult.title = ["Number foo %d", 1];

addResult and addChild can have optional title and details parameters for shorter notation.

[Andrey Kosyakov]

Created attachment 65557 [details]
patch

- Renamed AuditRun to AuditResult
- Replaced node types with formatters in AuditResult
- Added support for concatenating different formatters and simplified passing text-only values.

[Early Warning System Bot]

Attachment 65557 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3793419

[Pavel Feldman]

Comment on attachment 65557 [details]
patch

LayoutTests/inspector/extensions-audits.html:15
 +          results.addResult("Passed rule", "this rule always passes ok", webInspector.audits.severity.info);
webInspector.audits.severity.info - too many letters! result.Severity.Info ?

WebCore/inspector/front-end/AuditsPanel.js:59
 +          this._launcherView.addCategory(this.categoriesById[id]);
indent.

WebCore/inspector/front-end/AuditsPanel.js:163
 +              category.run(resources, ruleResultReadyCallback.bind(null, result));
bind(null) looks strange.

LayoutTests/inspector/extensions-audits-expected.txt:13
 +  category.onAuditStarted fired, results dump follows:
It'd be great to dump APIs to a separate expectations file for better readability.

[Andrey Kosyakov]

Created attachment 65573 [details]
patch

- Added missing file
- Moved webInspector.audits.severity -> AuditResult.Severity
- Split audit tests into API dump and tests per se

[WebKit Review Bot]

Attachment 65573 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/inspector/front-end/AuditFormatters.js:1:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 86 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andrey Kosyakov]

Created attachment 65575 [details]
patch

- Capitalized severity constants
- Fixed EOLs

[Pavel Feldman]

Comment on attachment 65575 [details]
patch

Lets land manually to be on the safe side.

[WebKit Review Bot]

http://trac.webkit.org/changeset/66218 might have broken Qt Linux Release minimal, Qt Linux ARMv5 Release, Qt Linux ARMv7 Release, and Chromium Linux Release

[Andrey Kosyakov]

Manually committed r66218: http://trac.webkit.org/changeset/66218
Build fix committed as r66220: http://trac.webkit.org/changeset/66220

[Andrey Kosyakov]

Reverted as r66239 due to GTK & Snow Leopard test failures:
http://trac.webkit.org/changeset/66239

[Andrey Kosyakov]

Created attachment 66022 [details]
patch

- Migrated tests to use inspector-test2.js
- Fixed dispatching messages from front-end to extension in test
- Added InspectorTest.enableResourceTracking() & InspectorTest.disableResourceTracking()
- Log exceptions in test initialization functions

[Yury Semikhatsky]

Comment on attachment 66022 [details]
patch

> LayoutTests/inspector/extensions-audits-tests.js:39
> +test = function()
var test =

> LayoutTests/inspector/extensions-audits.html:14
> +        log("category.onAuditStarted fired");
Why is it not just output?

> LayoutTests/inspector/extensions-test.js:11
> +        if (/^extension_/.exec(symbol) && typeof window[symbol] === "function")
Should be test() since you don't use result array.

[WebKit Review Bot]

http://trac.webkit.org/changeset/66477 might have broken Qt Linux Release

[Andrey Kosyakov]

Manually committed r66477: http://trac.webkit.org/changeset/66477
Qt test fix committed as r66480: http://trac.webkit.org/changeset/66480