Bug 153758

Summary: Web Inspector: Memory Timeline View should be responsive / resizable
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: 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 Flags
Patch
none
[IMAGE] Issue
none
Patch
none
Patch
none
[Image] After Patch is applied
none
Patch none

Description Joseph Pecoraro 2016-02-01 13:19:25 PST
* 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.
Comment 1 Radar WebKit Bug Importer 2016-02-01 13:19:58 PST
<rdar://problem/24444320>
Comment 2 Devin Rousso 2016-08-15 17:56:51 PDT
Created attachment 286123 [details]
Patch
Comment 3 Joseph Pecoraro 2016-08-15 19:39:37 PDT
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.
Comment 4 Joseph Pecoraro 2016-08-15 19:39:56 PDT
Created attachment 286137 [details]
[IMAGE] Issue
Comment 5 Devin Rousso 2019-01-30 19:08:10 PST
Created attachment 360672 [details]
Patch
Comment 6 Devin Rousso 2019-01-30 19:11:27 PST
Created attachment 360674 [details]
Patch
Comment 7 Devin Rousso 2019-01-30 19:14:46 PST
Created attachment 360675 [details]
[Image] After Patch is applied
Comment 8 Joseph Pecoraro 2019-01-30 19:43:42 PST
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 9 Devin Rousso 2019-01-30 22:14:53 PST
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.
Comment 10 Devin Rousso 2019-01-30 22:15:03 PST
Created attachment 360690 [details]
Patch
Comment 11 WebKit Commit Bot 2019-01-30 22:52:40 PST
Comment on attachment 360690 [details]
Patch

Clearing flags on attachment: 360690

Committed r240763: <https://trac.webkit.org/changeset/240763>
Comment 12 WebKit Commit Bot 2019-01-30 22:52:42 PST
All reviewed patches have been landed.  Closing bug.