Bug 92762 – Web Inspector: display progress bar while loading timeline data

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

[Andrey Kosyakov]

This implements generic support for progress bar and uses it to display progress of loading timeline data.
We may want to replace other progress indicators as well (e.g. in AdvancedSearchController, heap profiler, audits etc).

[Andrey Kosyakov]

Created attachment 155553 [details]
Patch

[WebKit Review Bot]

Comment on attachment 155553 [details]
Patch

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

[Andrey Kosyakov]

Created attachment 155558 [details]
Patch

[Andrey Kosyakov]

Created attachment 155560 [details]
screenshot

[Andrey Kosyakov]

Created attachment 155571 [details]
Patch

[WebKit Review Bot]

Comment on attachment 155571 [details]
Patch

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

New failing tests:
inspector/timeline/timeline-load.html
inspector/timeline/timeline-load-incompatible.html

[WebKit Review Bot]

Created attachment 155586 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Vsevolod Vlasov]

Comment on attachment 155571 [details]
Patch

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

Overall looks good to me

> Source/WebCore/inspector/front-end/Panel.js:234
> +        // If we have an active progress bar, ignore all panel-specific shortcuts, interrupt current operation on Esc.

nit: I don't think this comment is valuable here.

[Pavel Feldman]

Comment on attachment 155571 [details]
Patch

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

> Source/WebCore/WebCore.gypi:6343
> +            'inspector/front-end/ProgressBar.js',

Please mind alphabetic order

> Source/WebCore/inspector/front-end/Panel.js:265
> +        this._glassPane = new WebInspector.GlassPane(this.element);

I don't think we need to block the UI.

> Source/WebCore/inspector/front-end/ProgressBar.js:61
> +    hide: function()

I don't think that the task should be able to stop hide the progress indicator. It also prevents you from implementing composite progress properly. The idustry standard is to have "Progress" and "ProgressIndicator" entities. First is an interface, second is a UI component implementing this interface.

Progress {
    setTotalWork(number),
    worked(opt_number, opt_title),
    setTitle(title),
    done(),
    boolean isCanceled()
}

CompositeProgress (implements the Progress as well) (parent, 100)
var p1 = subProgress(20);
var p2 = subProgress(30);

> Source/WebCore/inspector/front-end/ProgressBar.js:74
> +    stopRequested: function()

isCanceled ?

> Source/WebCore/inspector/front-end/ProgressBar.js:79
> +    requestStop: function()

There should be no programmatic way to stop progress (make it private?)

[Andrey Kosyakov]

Created attachment 156425 [details]
Patch

[Pavel Feldman]

Comment on attachment 156425 [details]
Patch

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

Could you split this change into 2: progress indicator + timeline usage?

> Source/WebCore/ChangeLog:12
> +        - consume contextmenu events in the glass pane;

Seems to be unrelated.

> Source/WebCore/WebCore.gypi:6350
> +            'inspector/front-end/ProgressBar.js',

Please mind alphabetic order

> Source/WebCore/inspector/front-end/Panel.js:260
> +        this._progressIndicator = progressIndicator;

I think that progress should not correlate with the glass panel. Timeline should block its capabilities upon loading on its own.

> Source/WebCore/inspector/front-end/Panel.js:261
> +        this._progressIndicator.addEventListener(WebInspector.ProgressIndicator.Events.Done, this._onProgressIndicatorDone, this);

From the UI standpoint, it should be great if we could use
- per-inspector progress UI (located in the bottom part of the screen) for background operations (could be several of these in extreme cases)
- local convenience embeddable progress control bound to the progress interface.

> Source/WebCore/inspector/front-end/ProgressBar.js:50
> +     * @param {number} worked

number=

> Source/WebCore/inspector/front-end/ProgressBar.js:138
> +        this._progressElement.value = worked;

worked is a delta, additive value, not absolute (setWorked?).

> Source/WebCore/inspector/front-end/ProgressBar.js:156
> +    this._parent.worked(0);

worked(0) should be a noop.

> Source/WebCore/inspector/front-end/ProgressBar.js:170
> +    createSubProgress: function(weight, totalWork)

Remove totalWork.

> Source/WebCore/inspector/front-end/ProgressBar.js:200
> +    this._parent = parent;

this._composite ?

[Andrey Kosyakov]

Created attachment 157252 [details]
Patch

[Andrey Kosyakov]

Committed r125168: <http://trac.webkit.org/changeset/125168>