Bug 31665 – Web Inspector: Implement the Audits panel

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

[Alexander Pavlov (apavlov)]

The Audits panel should be implemented.

[Timothy Hatcher]

The Audits panel will be like Google PageSpeed and YSlow!.

[Pavel Feldman]

Created attachment 44151 [details]
[IMAGE] Audit runner view

[Pavel Feldman]

Created attachment 44152 [details]
[IMAGE] Audits result view

[Pavel Feldman]

Timothy, this is to start the mock discussion.

[Alexander Pavlov (apavlov)]

Created attachment 45390 [details]
[PATCH] Proposed AuditsPanel implementation

This patch corresponds to the results mock that follows.

[Alexander Pavlov (apavlov)]

Created attachment 45391 [details]
[IMAGE] Audit Result View rendered with the proposed patch (45390)

After some discussion, we agreed to have only two basic result categories (reflected with color bullets) and an "N/A" (== no bullet), which can be changed when we achieve some consensus. The rest of the result view was left intact in comparison to the original mock attached by pfeldman.

[Alexander Pavlov (apavlov)]

Created attachment 45393 [details]
[IMAGE] Audit Launcher View rendered with the proposed patch (45390)

- The first radio-button is disabled as the resource tracking is disabled. Once the resource tracking is enabled through the Resources panel or reloading the page and auditing it on load (clicking "Run" with the second radio button selected), the first radio button gets enabled and the parenthesized part is removed from its label.
- When there are no audits, a blank panel akin to "No Breakpoints" in the BreakpointsSidebarPane is shown.
- The "Run" button is disabled when there are no audits selected (this is the case in the screenshot).
- The audit checkboxes are rendered in a div with "overflow-y: auto" which takes up all vertical space down to the radio buttons, so that you can always see the radio-buttons and the "Run" button when there are lots of different audits in the list.

[Pavel Feldman]

Comment on attachment 45390 [details]
[PATCH] Proposed AuditsPanel implementation

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

[Alexander Pavlov (apavlov)]

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

[Alexander Pavlov (apavlov)]

Created attachment 45469 [details]
[PATCH] Review comments addressed

[Pavel Feldman]

Comment on attachment 45469 [details]
[PATCH] Review comments addressed

The change is quite big, so it is getting hard to re-review it. I am fine with landing it as is, given the following:

[BUG1]
- Sample audit category (either 'network utilization' or 'page performance') is getting landed shortly. We need to see how it looks all together.

[BUG2]
- Result view migrates from divs to the treeoutline structure.

[BUG3]
- Double borders are fixed
- All audits get enabled by default
- Results are expanded only one level

Please file corresponding bugs prior to closing this one. (I'll land this one tomorrow our timezone unless Timothy objects).

[Alexander Pavlov (apavlov)]

(In reply to comment #11)
> (From update of attachment 45469 [details])
> The change is quite big, so it is getting hard to re-review it. I am fine with
> landing it as is, given the following:
> 
> [BUG1]
> - Sample audit category (either 'network utilization' or 'page performance') is
> getting landed shortly. We need to see how it looks all together.
https://bugs.webkit.org/show_bug.cgi?id=32930 filed.

> [BUG2]
> - Result view migrates from divs to the treeoutline structure.
https://bugs.webkit.org/show_bug.cgi?id=32931 filed.

> [BUG3]
> - Double borders are fixed
> - All audits get enabled by default
> - Results are expanded only one level
https://bugs.webkit.org/show_bug.cgi?id=32932 filed.

[Alexander Pavlov (apavlov)]

Oh, the patch 45469 seems to be missing the "English.lproj" changes due to git diff skipping binary file changes...

[Alexander Pavlov (apavlov)]

Created attachment 45485 [details]
[PATCH] Patch with English.lproj changes

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/English.lproj/localizedStrings.js
	M	WebCore/WebCore.gypi
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	A	WebCore/inspector/front-end/AuditLauncherView.js
	A	WebCore/inspector/front-end/AuditResultView.js
	A	WebCore/inspector/front-end/AuditsPanel.js
	M	WebCore/inspector/front-end/WebKit.qrc
	A	WebCore/inspector/front-end/audits.css
	M	WebCore/inspector/front-end/inspector.html
	M	WebCore/inspector/front-end/inspector.js
Committed r52558

[Eric Seidel (no email)]

Comment on attachment 45485 [details]
[PATCH] Patch with English.lproj changes

This bug is closed.  Clearing the review flag on this patch as I assume it is not longer up for review.  If this patch does in fact need review, it would be best to attach it to a new bug.