[Webkit-unassigned] [Bug 31665] Web Inspector: Implement the Audits panel

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=31665





--- Comment #9 from Alexander Pavlov (apavlov) <apavlov at chromium.org>  2009-12-24 11:14:01 PST ---
(In reply to comment #8)
> (From update of attachment 45390 [details])
> 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'.

I wanted to let audit rule developers easily find the cause of their problems.
It is so easy to cast "undefined" to NaN in an expression. Seems a reasonable
approach...

> > +
> > +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?

Done.

> > +    appendRuleResult: function(entry)
> 
> No need for this given that entries is public.

Done.

> > +
> > +/**
> > + * @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?

Done.

> > +    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.

Done.

> > +WebInspector.AuditsPanel = function()
> > +{
> 
> We typically define 'main' classes like this first in the file.

Done.

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

Done.

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

No. This is there to cancel the search if we have one. Can remove that for now.

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

We typically make the superconstructor invocation stand out in the code block
(see ProfileDataGridTree.js)

> > + 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?)

Could be. Done.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.