Bug 126035 – Web Inspector: implement time range selection for TimelineOverview and TimelineRuler

• Component: Web Inspector
• Status: RESOLVED FIXED

[Timothy Hatcher]

We need to add time range selection for the ruler and overview.

[Radar WebKit Bug Importer]

<rdar://problem/15704981>

[Timothy Hatcher]

Created attachment 219709 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 219709 [details]
Patch

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

[Timothy Hatcher]

https://trac.webkit.org/changeset/162410