Bug 73626 – Web Inspector: [Audits] Implement "Stop" button and progress bar instead of spinner.

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

[Alexander Pavlov (apavlov)]

Patch to follow

[Alexander Pavlov (apavlov)]

Created attachment 117620 [details]
Patch

[Alexander Pavlov (apavlov)]

Created attachment 117622 [details]
[PATCH] Added localized string "Stop"

[Pavel Feldman]

Comment on attachment 117622 [details]
[PATCH] Added localized string "Stop"

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

> Source/WebCore/inspector/front-end/AuditLauncherView.js:137
> +            this._stopRequested = true;

I don't see the audit rule code that is querying for stopRequested prior to continuing execution. "Cancel" is usually implemented as a part of the progress indicator API:

// some rule body example:

progress.setTotalWork(Object.keys(nodes).length);

for (node in nodes) {
    if (progress.isCanceled())
        return;
    progress.worked(1);
    // process node.
}

[Alexander Pavlov (apavlov)]

(In reply to comment #3)
> (From update of attachment 117622 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117622&action=review
> 
> > Source/WebCore/inspector/front-end/AuditLauncherView.js:137
> > +            this._stopRequested = true;
> 
> I don't see the audit rule code that is querying for stopRequested prior to continuing execution. "Cancel" is usually implemented as a part of the progress indicator API:
> 
> // some rule body example:
> 
> progress.setTotalWork(Object.keys(nodes).length);
> 
> for (node in nodes) {
>     if (progress.isCanceled())
>         return;
>     progress.worked(1);
>     // process node.
> }

Do you hope an HTML UI element has a full-fledged progress monitor API? No, it only has the "max" and "value" attributes. Without them, it becomes an indeterminate progress bar (which is what this CL uses). We also use it to track the progress of the entire audit run, including resource loading (for which it is impossible to measure the max value initially.) Do you want me to implement a progress monitor using this control somehow?

[Pavel Feldman]

> Do you hope an HTML UI element has a full-fledged progress monitor API? No, it only has the "max" and "value" attributes. Without them, it becomes an indeterminate progress bar (which is what this CL uses). We also use it to track the progress of the entire audit run, including resource loading (for which it is impossible to measure the max value initially.) Do you want me to implement a progress monitor using this control somehow?

Not at all. I was talking about the control flow, not the UI. The point was that progress indicator API is ideal both for progress reporting as well as termination. You should pass progress into all rules, implement isCanceled() and do early returns in case it returns "true".

[Alexander Pavlov (apavlov)]

(In reply to comment #5)
> > Do you hope an HTML UI element has a full-fledged progress monitor API? No, it only has the "max" and "value" attributes. Without them, it becomes an indeterminate progress bar (which is what this CL uses). We also use it to track the progress of the entire audit run, including resource loading (for which it is impossible to measure the max value initially.) Do you want me to implement a progress monitor using this control somehow?
> 
> Not at all. I was talking about the control flow, not the UI. The point was that progress indicator API is ideal both for progress reporting as well as termination. You should pass progress into all rules, implement isCanceled() and do early returns in case it returns "true".

Oh, that makes perfect sense, can even try implementing a determinate progress at the audit execution stage.

[Alexander Pavlov (apavlov)]

Created attachment 117866 [details]
[PATCH] Comment addressed

[WebKit Review Bot]

Comment on attachment 117866 [details]
[PATCH] Comment addressed

Attachment 117866 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10689665

New failing tests:
svg/custom/linking-uri-01-b.svg

[Yury Semikhatsky]

Comment on attachment 117866 [details]
[PATCH] Comment addressed

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

> Source/WebCore/inspector/front-end/AuditLauncherView.js:128
> +            var childNodes = this._categoriesElement.childNodes;

Remove this line, childNodes is unused.

> Source/WebCore/inspector/front-end/AuditsPanel.js:506
> +        if (this.canceled || this.indeterminate || this.isDone())

Be consistent in naming either isCanceled or done().

[Alexander Pavlov (apavlov)]

(In reply to comment #9)
> (From update of attachment 117866 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117866&action=review
> 
> > Source/WebCore/inspector/front-end/AuditLauncherView.js:128
> > +            var childNodes = this._categoriesElement.childNodes;
> 
> Remove this line, childNodes is unused.

Done

> > Source/WebCore/inspector/front-end/AuditsPanel.js:506
> > +        if (this.canceled || this.indeterminate || this.isDone())
> 
> Be consistent in naming either isCanceled or done().

Done

[Alexander Pavlov (apavlov)]

Committed r102109: <http://trac.webkit.org/changeset/102109>