[webkit-reviews] review denied: [Bug 31665] Web Inspector: Implement the Audits panel : [Attachment 45390] [PATCH] Proposed AuditsPanel implementation

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 31665: Web Inspector: Implement the Audits panel
https://bugs.webkit.org/show_bug.cgi?id=31665

Attachment 45390: [PATCH] Proposed AuditsPanel implementation
https://bugs.webkit.org/attachment.cgi?id=45390&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
r- for a bunch of minor comments. Otherwise, looks good.

> +    getValue: function(key)
> +    {

No need for explicit getter - just make parameter public. Call it 'parameters'.


> +
> +WebInspector.AuditCategoryResult = function(categoryId, title)
> +{
> +    this.categoryId = categoryId;
> +    this.title = title;

Given that these id and title are taken from category, why not to simply pass
category here?

> +    appendRuleResult: function(entry)

No need for this given that entries is public.

> +
> +/**
> + * @param {string|Node|Array<Node|string>} value The entry message contents.

> + * @param {?number} type Optional entry type.
> + */
> +WebInspector.AuditRuleResult = function(value, type)
> +{

You don't seem to create it with given type - just get rid of this param?

> +    this.value = value;
> +    this.type = type || WebInspector.AuditRuleResult.TypeNA;
> +}
> +
> +WebInspector.AuditRuleResult.TypeNA = 0;
> +
> +WebInspector.AuditRuleResult.TypeHint = 1;
> +
> +WebInspector.AuditRuleResult.TypeViolation = 2;
> +

See WebInspector.ConsoleMessage.MessageType for how we define these.

> +WebInspector.AuditsPanel = function()
> +{

We typically define 'main' classes like this first in the file.

> +
> +    _executeAudit: function(categories, resultCallback)
> +    {
> +	   var resources = [];
> +	   for (var url in WebInspector.resourceURLMap)
> +	       resources.push(WebInspector.resourceURLMap[url]);
> +

You should iterate WebInpector.resources instead.

> +    reset: function()
> +    {
> +	   // No tree changes on reset.
> +	   delete this.currentQuery;
> +	   this.searchCanceled();

Copy / paste?

> +WebInspector.AuditsSidebarTreeElement = function()
> +{
> +    this.small = false;
> +
> +    WebInspector.SidebarTreeElement.call(this, "audits-sidebar-tree-item",
WebInspector.UIString("Audits"), "", null, false);

Strange space in between.

> + function entrySortFunction(a, b)
> + {
> +	return b.type - a.type;

This will make items with equal types equal. Is that what you want? (add
alphabetical sort as well?)