[webkit-reviews] review granted: [Bug 126035] Web Inspector: implement time range selection for TimelineOverview and TimelineRuler : [Attachment 219709] 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 126035: Web Inspector: implement time range selection for TimelineOverview
and TimelineRuler
https://bugs.webkit.org/show_bug.cgi?id=126035

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

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


Nice and clean! r=me

> Source/WebInspectorUI/UserInterface/TimelineRuler.js:71
> +WebInspector.TimelineRuler.MinimumSelectionTimeRange = 0.1;

I think this is a bit too wide a margin right now. Snapping to 100ms boundaries
would be rough if you only want to see what happened during a couple frames
(~30ms duration). We should relax this a bit.

> Source/WebInspectorUI/UserInterface/TimelineRuler.js:104
> +	   return this._allowsTimeRangeSelection

Style: Missing semicolon.

> Source/WebInspectorUI/UserInterface/TimelineRuler.js:139
> +	       this._needsLayout();

Why needsLayout and not needsSelectionLayout?

> Source/WebInspectorUI/UserInterface/TimelineRuler.js:547
> +	   if (!this._selectionDragElement.parentNode)
> +	       this._element.appendChild(this._selectionDragElement);
> +
> +	   if (!this._leftShadedAreaElement.parentNode)
> +	       this._element.appendChild(this._leftShadedAreaElement);
> +
> +	   if (!this._leftSelectionHandleElement.parentNode)
> +	       this._element.appendChild(this._leftSelectionHandleElement);
> +
> +	   if (!this._rightShadedAreaElement.parentNode)
> +	       this._element.appendChild(this._rightShadedAreaElement);
> +
> +	   if (!this._rightSelectionHandleElement.parentNode)
> +	       this._element.appendChild(this._rightSelectionHandleElement);

You only add/remove them as a group. I would just test for parentNode on one,
and append all at once.

> Source/WebInspectorUI/UserInterface/TimelineRuler.js:572
> +	   // Only handle left mouse clicks.
> +	   if (event.button !== 0)
> +	       return;

Should we also handle ⌃+Click on OS X translating as a right click. We have a
lot of existing code that does:

    if (event.button !== 0 || event.ctrlKey)
	return;

> Source/WebInspectorUI/UserInterface/TimelineRuler.js:657
> +	   // Only handle left mouse clicks.
> +	   if (event.button !== 0)
> +	       return;

Ditto