Bug 31665 - Web Inspector: Implement the Audits panel
: Web Inspector: Implement the Audits panel
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-11-19 05:03 PST by Alexander Pavlov (apavlov)
Modified: 2009-12-27 20:02 PST (History)
7 users (show)

See Also:


Attachments
[IMAGE] Audit runner view (70.45 KB, image/png)
2009-12-02 08:27 PST, Pavel Feldman
no flags Details
[IMAGE] Audits result view (112.26 KB, image/png)
2009-12-02 08:27 PST, Pavel Feldman
no flags Details
[PATCH] Proposed AuditsPanel implementation (47.50 KB, patch)
2009-12-22 10:35 PST, Alexander Pavlov (apavlov)
pfeldman: review-
Details | Formatted Diff | Diff
[IMAGE] Audit Result View rendered with the proposed patch (45390) (72.30 KB, image/png)
2009-12-22 10:44 PST, Alexander Pavlov (apavlov)
no flags Details
[IMAGE] Audit Launcher View rendered with the proposed patch (45390) (41.58 KB, image/png)
2009-12-22 11:05 PST, Alexander Pavlov (apavlov)
no flags Details
[PATCH] Review comments addressed (47.54 KB, patch)
2009-12-24 11:15 PST, Alexander Pavlov (apavlov)
pfeldman: review+
pfeldman: commit‑queue-
Details | Formatted Diff | Diff
[PATCH] Patch with English.lproj changes (47.85 KB, patch)
2009-12-25 03:11 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2009-11-19 05:03:00 PST
The Audits panel should be implemented.
Comment 1 Timothy Hatcher 2009-11-20 17:01:06 PST
The Audits panel will be like Google PageSpeed and YSlow!.
Comment 2 Pavel Feldman 2009-12-02 08:27:22 PST
Created attachment 44151 [details]
[IMAGE] Audit runner view
Comment 3 Pavel Feldman 2009-12-02 08:27:38 PST
Created attachment 44152 [details]
[IMAGE] Audits result view
Comment 4 Pavel Feldman 2009-12-02 08:28:25 PST
Timothy, this is to start the mock discussion.
Comment 5 Alexander Pavlov (apavlov) 2009-12-22 10:35:20 PST
Created attachment 45390 [details]
[PATCH] Proposed AuditsPanel implementation

This patch corresponds to the results mock that follows.
Comment 6 Alexander Pavlov (apavlov) 2009-12-22 10:44:58 PST
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.
Comment 7 Alexander Pavlov (apavlov) 2009-12-22 11:05:28 PST
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.
Comment 8 Pavel Feldman 2009-12-24 07:10:04 PST
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?)
Comment 9 Alexander Pavlov (apavlov) 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.
Comment 10 Alexander Pavlov (apavlov) 2009-12-24 11:15:22 PST
Created attachment 45469 [details]
[PATCH] Review comments addressed
Comment 11 Pavel Feldman 2009-12-24 13:28:00 PST
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).
Comment 12 Alexander Pavlov (apavlov) 2009-12-25 02:07:13 PST
(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.
Comment 13 Alexander Pavlov (apavlov) 2009-12-25 02:58:19 PST
Oh, the patch 45469 seems to be missing the "English.lproj" changes due to git diff skipping binary file changes...
Comment 14 Alexander Pavlov (apavlov) 2009-12-25 03:11:13 PST
Created attachment 45485 [details]
[PATCH] Patch with English.lproj changes
Comment 15 Pavel Feldman 2009-12-25 06:59:16 PST
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
Comment 16 Eric Seidel 2009-12-27 20:02:41 PST
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.