[webkit-reviews] review granted: [Bug 125709] Web Inspector: refactor TimelineDecorations into TimelineRuler : [Attachment 219196] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Timothy Hatcher
<timothy at apple.com>'s request for review:
Bug 125709: Web Inspector: refactor TimelineDecorations into TimelineRuler
https://bugs.webkit.org/show_bug.cgi?id=125709

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

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=219196&action=review


r=me

> Source/WebInspectorUI/UserInterface/TimelineRuler.js:46
> +    this._allowsClippedLabels = false;

I would include "this._endTimePinned = false" up here with the list of other
member variables that don't get `delete`d.

> Source/WebInspectorUI/UserInterface/TimelineRuler.js:165
> +	   return this._secondsPerPixel;

Should this _recalculate before returning a value? Someone could call
_needsLayout, but that only updates this member variable after a rAF. So if
someone set endTime then checked secondsPerPixel it could be wrong.

> Source/WebInspectorUI/UserInterface/TimelineRuler.js:248
> +	       var newLeftPosition = ((dividerTime - this._startTime) /
duration);

Style: unnecessary extra parens

> Source/WebInspectorUI/UserInterface/TimelineRuler.js:254
> +		       removeDividerAndSelectNext();
> +		       continue;

Is it necessary to remove divider and select next? Why not just continue, and
keep to reuse the current divider? Extra dividers are removed after this loop
anyways (lines 284-285).

> Source/WebInspectorUI/UserInterface/TimelineRuler.js:264
> +		       removeDividerAndSelectNext();
> +		       continue;

Ditto.