Bug 79123 – Web Inspector: [experimental] add a mode to display timeline events aligned by the start time

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

[Andrey Kosyakov]

This adds an "aligned mode" to the timeline panel, which is differs in the following aspects:
- asynchronous events are not connected as children to those events that caused them
- all events are aligned by top-level event start time (synchronous children are offset from the parent by their relative start time)
- the overview pane is replaced by vertical bars for each top-level event (or for the larges of sequential group of such events, when there's not enough space)

The intended use case for this mode is to troubleshoot occasional hiccups/delays in a page that generate large amount of similar, short-termed events, e.g. to perform an animation.

[Andrey Kosyakov]

Created attachment 127990 [details]
Patch

[Timothy Hatcher]

Screenshot?

[WebKit Review Bot]

Comment on attachment 127990 [details]
Patch

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

New failing tests:
inspector/network-status-non-http.html
http/tests/inspector/network/network-sidebar-width.html
http/tests/inspector/network/network-initiator-from-console.html

[Andrey Kosyakov]

Created attachment 128174 [details]
Aligned mode, few events (columns in the overview are wide and rare)

[Andrey Kosyakov]

Created attachment 128175 [details]
Aligned mode, many events (overview columns are dense; selection window visible)

[Andrey Kosyakov]

(In reply to comment #2)
> Screenshot?

Added. We did not decide on the best way to toggle it yet, and this is the primary reason it's behind the experimental switch. Currently, the mode is toggled by a button in the status bar, and when it's on, the Timeline/Memory toggle in the overview sidebar is hidden. We're considering to revamp the overview sidebar, so that the "normal" timeline mode, overview mode and performance graphs are controlled from there -- however, there are no particular ideas on how the control should look yet.

[Pavel Feldman]

Comment on attachment 127990 [details]
Patch

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

I don't think you should remove the Timeline/Memory buttons when your mode toggles.

> Source/WebCore/inspector/front-end/TimelineGrid.js:118
> +                dividerLabelBar._labelElement.textContent = calculator.formatTime(slice * i);

why did this change? Calculator could be used for arranging rows by size or latency, etc.

> Source/WebCore/inspector/front-end/TimelineOverviewPane.js:40
> +    this._alignedMode = false;

this._startAtZero

> Source/WebCore/inspector/front-end/TimelineOverviewPane.js:76
> +    this._alignedOverview = new WebInspector.TimelineAlignedModeOverview(this._presentationModel);

lets create it lazily

> Source/WebCore/inspector/front-end/TimelineOverviewPane.js:138
> +    setAlignedMode: function(enabled)

setStartAtZero

> Source/WebCore/inspector/front-end/TimelineOverviewPane.js:144
> +            this.element.addStyleClass("timeline-aligned-mode");

timeline-start-at-zero.

> Source/WebCore/inspector/front-end/TimelineOverviewPane.js:757
> +        var calculator = new WebInspector.AlignedTimelineCalculator();

do you need to create calculator on each update?

> Source/WebCore/inspector/front-end/TimelinePanel.js:687
> +        if (!this._boundariesAreValid)

why did this change?

> Source/WebCore/inspector/front-end/TimelinePanel.js:713
> +        var filter = this._alignMode ? new WebInspector.TimelineAlignedRecordFilter(this._presentationModel, this._rootRecord, this._showShortEvents)

You don't really need to re-create filter each time?

> Source/WebCore/inspector/front-end/TimelinePanel.js:-717
> -        if (updateBoundaries)

why did this change?

> Source/WebCore/inspector/front-end/TimelinePanel.js:1718
> +WebInspector.TimelineRecordFilter = function(calculator, showShortEvents)

Ok, we will hit 2K lines here soon. Time to extract some classes.

[Andrey Kosyakov]

Created attachment 128191 [details]
Patch

[Andrey Kosyakov]

(In reply to comment #7)
> (From update of attachment 127990 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127990&action=review
> 
> I don't think you should remove the Timeline/Memory buttons when your mode toggles.

I think the primary use case for memory mode is to correlate changes in heap size against timeline events -- so it does not look like it has much meaning when all events are aligned; on the other hand, we gain some real estate for the overview bars this way. I guess we can change this once we manage to fit all mode controls to the overview sidebar.

> 
> > Source/WebCore/inspector/front-end/TimelineGrid.js:118
> > +                dividerLabelBar._labelElement.textContent = calculator.formatTime(slice * i);
> 
> why did this change? Calculator could be used for arranging rows by size or latency, etc.

It's just that we only used time in timeline. Ok, I'm bringing this back to formatValue().

> > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:40
> > +    this._alignedMode = false;
> 
> this._startAtZero

Done.

> > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:76
> > +    this._alignedOverview = new WebInspector.TimelineAlignedModeOverview(this._presentationModel);
> 
> lets create it lazily

Done.

> > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:138
> > +    setAlignedMode: function(enabled)
> 
> setStartAtZero

Done.

> > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:144
> > +            this.element.addStyleClass("timeline-aligned-mode");
> 
> timeline-start-at-zero.

Done.

> > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:757
> > +        var calculator = new WebInspector.AlignedTimelineCalculator();
> 
> do you need to create calculator on each update?

Fixed.

> > Source/WebCore/inspector/front-end/TimelinePanel.js:687
> > +        if (!this._boundariesAreValid)
> 
> why did this change?

It's a drive-by change, I'd like to take boundaries update logic out of refreshRecords(). It's a trivial change, refreshRecords() is only called from here.

> > Source/WebCore/inspector/front-end/TimelinePanel.js:713
> > +        var filter = this._alignMode ? new WebInspector.TimelineAlignedRecordFilter(this._presentationModel, this._rootRecord, this._showShortEvents)
> 
> You don't really need to re-create filter each time?

I do, since it maintains some state that needs to be reset when we start enumerating records from 0 (one filter counts records, another holds a calculator).

> > Source/WebCore/inspector/front-end/TimelinePanel.js:-717
> > -        if (updateBoundaries)
> 
> why did this change?

See above.

> > Source/WebCore/inspector/front-end/TimelinePanel.js:1718
> > +WebInspector.TimelineRecordFilter = function(calculator, showShortEvents)
> 
> Ok, we will hit 2K lines here soon. Time to extract some classes.

I'm doing this gradually by extracting some stuff into presentation model, filters etc. We can move them to a separate file later.

[Andrey Kosyakov]

Committed r108502: <http://trac.webkit.org/changeset/108502>

[Timothy Hatcher]

WebKit policy is to always have classes in seperate files. We should do the same in the Inspector. It would encourage less spaghetti code and private function/property access.

[Pavel Feldman]

(In reply to comment #11)
> WebKit policy is to always have classes in seperate files. We should do the same in the Inspector. It would encourage less spaghetti code and private function/property access.

+1, I'm going to push for < 1KLOC classes. Private classes are still fine to belong to the same unit as long as it does not grow > 1KLOC.