Summary: | Web Inspector: implement time range selection for TimelineOverview and TimelineRuler | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Timothy Hatcher <timothy> | ||||
Component: | Web Inspector | Assignee: | Timothy Hatcher <timothy> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | graouts, joepeck, timothy, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Timothy Hatcher
2013-12-19 17:15:39 PST
Created attachment 219709 [details]
Patch
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 |