Bug 79123 - Web Inspector: [experimental] add a mode to display timeline events aligned by the start time
Summary: Web Inspector: [experimental] add a mode to display timeline events aligned b...
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-02-21 09:54 PST by Andrey Kosyakov
Modified: 2012-02-22 09:59 PST (History)
12 users (show)

See Also:


Attachments
Patch (50.64 KB, patch)
2012-02-21 09:58 PST, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
Aligned mode, few events (columns in the overview are wide and rare) (91.21 KB, image/png)
2012-02-22 04:08 PST, Andrey Kosyakov
no flags Details
Aligned mode, many events (overview columns are dense; selection window visible) (77.30 KB, image/png)
2012-02-22 04:09 PST, Andrey Kosyakov
no flags Details
Patch (49.03 KB, patch)
2012-02-22 05:59 PST, Andrey Kosyakov
pfeldman: 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-02-21 09:54:43 PST
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.
Comment 1 Andrey Kosyakov 2012-02-21 09:58:21 PST
Created attachment 127990 [details]
Patch
Comment 2 Timothy Hatcher 2012-02-21 10:25:45 PST
Screenshot?
Comment 3 WebKit Review Bot 2012-02-21 11:16:12 PST
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
Comment 4 Andrey Kosyakov 2012-02-22 04:08:14 PST
Created attachment 128174 [details]
Aligned mode, few events (columns in the overview are wide and rare)
Comment 5 Andrey Kosyakov 2012-02-22 04:09:13 PST
Created attachment 128175 [details]
Aligned mode, many events (overview columns are dense; selection window visible)
Comment 6 Andrey Kosyakov 2012-02-22 04:16:35 PST
(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.
Comment 7 Pavel Feldman 2012-02-22 04:21:38 PST
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.
Comment 8 Andrey Kosyakov 2012-02-22 05:59:25 PST
Created attachment 128191 [details]
Patch
Comment 9 Andrey Kosyakov 2012-02-22 06:49:59 PST
(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.
Comment 10 Andrey Kosyakov 2012-02-22 07:16:44 PST
Committed r108502: <http://trac.webkit.org/changeset/108502>
Comment 11 Timothy Hatcher 2012-02-22 09:57:30 PST
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.
Comment 12 Pavel Feldman 2012-02-22 09:59:13 PST
(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.