Bug 96755 - Web Inspector: reuse WebInspector.ProgressIndicator in Audits panel
Summary: Web Inspector: reuse WebInspector.ProgressIndicator in Audits panel
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Andrey Kosyakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-14 04:46 PDT by Andrey Kosyakov
Modified: 2012-09-14 06:07 PDT (History)
10 users (show)

See Also:


Attachments
Patch (34.17 KB, patch)
2012-09-14 04:58 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
Patch (34.63 KB, patch)
2012-09-14 05:41 PDT, Andrey Kosyakov
apavlov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Kosyakov 2012-09-14 04:46:10 PDT
Audits panel currently has a custom implementation of progress indciator. Let's replaces it with generic one already used in other places of inspector.
Comment 1 Andrey Kosyakov 2012-09-14 04:58:17 PDT
Created attachment 164108 [details]
Patch
Comment 2 Alexander Pavlov (apavlov) 2012-09-14 05:19:33 PDT
Comment on attachment 164108 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164108&action=review

> Source/WebCore/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

I don't believe this line should be here.

> Source/WebCore/ChangeLog:10
> +        - remove AuditProgressMonitor, use WebInspector.ProgressIndicator instead

Please use item delimiters consistently (semicolons, commas, periods, whatever).

> Source/WebCore/ChangeLog:12
> +        - simplify control flow -- remove callbacks where possible;

and this one should end in a period, it seems

>              this._setAuditRunning(true);

tip: you could try extracting this along with its counterpart as this._setAuditRunning(!this._auditRunning);

> Source/WebCore/inspector/front-end/AuditRules.js:77
> +     * @param {WebInspector.NetworkRequest} requests

Can you explain this change? The new annotations should not compile. Same thing for the other rules' doRun().
Comment 3 Andrey Kosyakov 2012-09-14 05:41:02 PDT
(In reply to comment #2)

> > +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).
> 
> I don't believe this line should be here.

Oops, thanks, fixed!

> Please use item delimiters consistently (semicolons, commas, periods, whatever).

fixed.

> 
> > Source/WebCore/ChangeLog:12
> > +        - simplify control flow -- remove callbacks where possible;
> 
> and this one should end in a period, it seems

ditto.

> 
> >              this._setAuditRunning(true);
> 
> tip: you could try extracting this along with its counterpart as this._setAuditRunning(!this._auditRunning);

I've split it into _startAudit()/_stopAudit()

> > Source/WebCore/inspector/front-end/AuditRules.js:77
> > +     * @param {WebInspector.NetworkRequest} requests
> 
> Can you explain this change? The new annotations should not compile. Same thing for the other rules' doRun().

Fixed this one be an array. Unfortunately, this did compile :(
Comment 4 Andrey Kosyakov 2012-09-14 05:41:44 PDT
Created attachment 164119 [details]
Patch
Comment 5 Alexander Pavlov (apavlov) 2012-09-14 05:54:28 PDT
Comment on attachment 164119 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164119&action=review

> Source/WebCore/inspector/front-end/AuditsPanel.js:180
> +            function onReload()

You could move it a bit above and call from the "if (runImmediately)" branch (changing the name appropriately) to reduce code duplication.
Comment 6 Andrey Kosyakov 2012-09-14 06:07:22 PDT
Committed r128598: <http://trac.webkit.org/changeset/128598>