Bug 153758 – Web Inspector: Memory Timeline View should be responsive / resizable

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* SUMMARY
Memory Timeline View should be responsive / resizable.

Currently the MemoryCategoryView is fixed at 800/75. In general the Memory Timeline View expects to be 900px wide. It doesn't need to be so wide. Also it might be nice to layout the charts differently at narrow widths.

[Radar WebKit Bug Importer]

<rdar://problem/24444320>

[Devin Rousso]

Created attachment 286123 [details]
Patch

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 286137 [details]
[IMAGE] Issue

[Devin Rousso]

Created attachment 360672 [details]
Patch

[Devin Rousso]

Created attachment 360674 [details]
Patch

[Devin Rousso]

Created attachment 360675 [details]
[Image] After Patch is applied

[Joseph Pecoraro]

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).

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 360690 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 360690 [details]
Patch

Clearing flags on attachment: 360690

Committed r240763: <https://trac.webkit.org/changeset/240763>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.