[webkit-reviews] review denied: [Bug 219987] Web Inspector: console.takeHeapSnapshot() appears to have no effect : [Attachment 416670] Patch v1.0

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has denied Patrick Angle <pangle at apple.com>'s
request for review:
Bug 219987: Web Inspector: console.takeHeapSnapshot() appears to have no effect
https://bugs.webkit.org/show_bug.cgi?id=219987

Attachment 416670: Patch v1.0

https://bugs.webkit.org/attachment.cgi?id=416670&action=review




--- Comment #9 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 416670
  --> https://bugs.webkit.org/attachment.cgi?id=416670
Patch v1.0

View in context: https://bugs.webkit.org/attachment.cgi?id=416670&action=review

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:289
> +	  
this.dispatchEventToListeners(WI.TimelineView.Event.NeedsEntireSelectedRange);

We probably don't want to do this.  We automatically take heap snapshots every
10s, so this would mean that any attempt to narrow down the range of a timeline
recording while it's still recording would be frustrating, as the selection
would reset every 10s.

I intentionally made it so that we only select the full range when importing or
clicking the button in the UI so that a malicious page (e.g. one that calls
`console.takeHeapSnapshot()` frequently) can't make the UX awful.

I think a better solution to this would be to show some sort of banner in the
JavaScript Allocations timeline view when a new heap snapshot is added that is
outside of the selected range that has a button to change the selected range to
either just extend to include the new snapshot or just select all of time.  I'm
imagining something along the lines of what happens in the Console if you've
selected the "Evaluations" filter and then evaluate `console.error(42);`.