[webkit-reviews] review granted: [Bug 173087] Web Inspector: Instrument active pixel memory used by canvases : [Attachment 313996] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 173087: Web Inspector: Instrument active pixel memory used by canvases
https://bugs.webkit.org/show_bug.cgi?id=173087

Attachment 313996: Patch

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




--- Comment #17 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 313996
  --> https://bugs.webkit.org/attachment.cgi?id=313996
Patch

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

r=me, nice tests

> LayoutTests/inspector/canvas/memory.html:55
> +	       InspectorTest.log(`Change size of canvas to
${width}x${height}.`);

We typically use this "\xd7", not that its important here though.

> Source/JavaScriptCore/inspector/protocol/Canvas.json:42
> +		   { "name": "memoryCost", "type": "number", "optional": true,
"description": "Memory usage of the canvas." }

Lets specify "in bytes" somewhere.

> Source/WebInspectorUI/.eslintrc:37
> +	   "CanvasAgent": true,

Pro.

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:110
> +    CanvasMemoryChanged: "canvas-manager-canvas-memory-changed",

Its weird to me that this event is on the CanvasManager and not the Canvas
instance that is changing.

If it was on the Canvas object then listeners could either listen on a
particular instance:

    instance.addEventListener(WebInspector.Canvas.Event.MemoryChanged, () =>
{});

Or for all canvases:

   
WebInspector.Canvas.addEventListener(WebInspector.Canvas.Event.MemoryChanged,
() => {});

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:96
> +    set memoryCost(memoryCost) { this._memoryCost = memoryCost; }

This is where I would have expected the event to be dispatched.