[webkit-reviews] review denied: [Bug 79123] Web Inspector: [experimental] add a mode to display timeline events aligned by the start time : [Attachment 127990] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Andrey Kosyakov
<caseq at chromium.org>'s request for review:
Bug 79123: Web Inspector: [experimental] add a mode to display timeline events
aligned by the start time
https://bugs.webkit.org/show_bug.cgi?id=79123

Attachment 127990: Patch
https://bugs.webkit.org/attachment.cgi?id=127990&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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.