Summary: | Web Inspector: Memory Timeline View should be responsive / resizable | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||||||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | commit-queue, hi, inspector-bugzilla-changes, joepeck, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Attachments: |
|
Description
Joseph Pecoraro
2016-02-01 13:19:25 PST
Created attachment 286123 [details]
Patch
Comment on attachment 286123 [details]
Patch
Wow this is awesome!
It is close to working but doesn't quite work at the moment though, so r- until fixed.
Issue:
When I resize Web Inspector > 800px the line chart only draws 800px wide but the timeline overview draws as wide as the window 1600px+.
Steps to Reproduce:
1. Inspect any page
2. Start Timeline recording with Memory Timeline
3. Record for about 10 seconds then stop
4. Resize Web Inspector > 1000px wide
5. Show Memory Timeline view
6. Select 0-10s in the timeline overview
=> Line Chart only draws 800px, doesn't match timeline ruler times
Also I would expect there to be some padding around the line charts, so that they don't go right up to the edge of the inspector window.
Created attachment 286137 [details]
[IMAGE] Issue
Created attachment 360672 [details]
Patch
Created attachment 360674 [details]
Patch
Created attachment 360675 [details]
[Image] After Patch is applied
Comment on attachment 360674 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360674&action=review r=me > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:165 > + function xScale(value) { Why rename this from `time` to `value`? > Source/WebInspectorUI/UserInterface/Views/LineChart.js:89 > addPoint(x, y) > { > this._points.push({x, y}); > + > + this.needsLayout(); > } > > clear() > { > this._points = []; > + > + this.needsLayout(); > } I don't like these changes. This is going to mean lots and lots of unnecessary needsLayout calls happening when adding say 1000 points to a chart (which will be the common case). Comment on attachment 360674 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360674&action=review >> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:165 >> + function xScale(value) { > > Why rename this from `time` to `value`? No good reason really, more-so to just match the changes to `yScale`. I'll revert. >> Source/WebInspectorUI/UserInterface/Views/LineChart.js:89 >> } > > I don't like these changes. This is going to mean lots and lots of unnecessary needsLayout calls happening when adding say 1000 points to a chart (which will be the common case). Successive calls to `needsLayout` are very early returns, so I don't think this is the worst thing, but you make a good point, so I'll remove these changes. Created attachment 360690 [details]
Patch
Comment on attachment 360690 [details] Patch Clearing flags on attachment: 360690 Committed r240763: <https://trac.webkit.org/changeset/240763> All reviewed patches have been landed. Closing bug. |