[webkit-reviews] review denied: [Bug 44518] Web Inspector: add audits support to extension API : [Attachment 65263] patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Andrey Kosyakov
<caseq at chromium.org>'s request for review:
Bug 44518: Web Inspector: add audits support to extension API
https://bugs.webkit.org/show_bug.cgi?id=44518

Attachment 65263: patch
https://bugs.webkit.org/attachment.cgi?id=65263&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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...