Summary: | Web Inspector: [Audits] Implement "Stop" button and progress bar instead of spinner. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexander Pavlov (apavlov) <apavlov> | ||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Alexander Pavlov (apavlov) <apavlov> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | apavlov, bweinstein, dglazkov, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Alexander Pavlov (apavlov)
2011-12-01 22:57:44 PST
Created attachment 117620 [details]
Patch
Created attachment 117622 [details]
[PATCH] Added localized string "Stop"
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. } (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? > 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".
(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. Created attachment 117866 [details]
[PATCH] Comment addressed
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 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(). (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 Committed r102109: <http://trac.webkit.org/changeset/102109> |