Bug 31665 - Web Inspector: Implement the Audits panel
: Web Inspector: Implement the Audits panel
Status: RESOLVED FIXED
: WebKit
Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-11-19 05:03 PST by
Modified: 2009-12-27 20:02 PST (History)


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-
Review Patch | 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-
Review Patch | 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 Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-11-19 05:03:00 PST
The Audits panel should be implemented.
------- Comment #1 From 2009-11-20 17:01:06 PST -------
The Audits panel will be like Google PageSpeed and YSlow!.
------- Comment #2 From 2009-12-02 08:27:22 PST -------
Created an attachment (id=44151) [details]
[IMAGE] Audit runner view
------- Comment #3 From 2009-12-02 08:27:38 PST -------
Created an attachment (id=44152) [details]
[IMAGE
------- Comment #4 From 2009-12-02 08:28:25 PST -------
Timothy, this is to start the mock discussion.
------- Comment #5 From 2009-12-22 10:35:20 PST -------
Created an attachment (id=45390) [details]
[PATCH] Proposed AuditsPanel implementation

This patch corresponds to the results mock that follows.
------- Comment #6 From 2009-12-22 10:44:58 PST -------
Created an attachment (id=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 From 2009-12-22 11:05:28 PST -------
Created an attachment (id=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 From 2009-12-24 07:10:04 PST -------
(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'.

> +
> +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 From 2009-12-24 11:14:01 PST -------
(In reply to comment #8)
> (From update of attachment 45390 [details] [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 From 2009-12-24 11:15:22 PST -------
Created an attachment (id=45469) [details]
[PATCH] Review comments addressed
------- Comment #11 From 2009-12-24 13:28:00 PST -------
(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.

[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 From 2009-12-25 02:07:13 PST -------
(In reply to comment #11)
> (From update of attachment 45469 [details] [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 From 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 From 2009-12-25 03:11:13 PST -------
Created an attachment (id=45485) [details]
[PATCH] Patch with English.lproj changes
------- Comment #15 From 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 From 2009-12-27 20:02:41 PST -------
(From update of attachment 45485 [details])
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.