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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Jun 24 00:56:15 PDT 2017


Joseph Pecoraro <joepeck at webkit.org> has denied 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 313488: Patch

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




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

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

r- to see another patch after dropping the "max" thing. The rest looks good.

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

This is great!

> Source/JavaScriptCore/inspector/protocol/Canvas.json:80
> +	   {
> +	       "name": "maxPixelMemory",
> +	       "description": "Gets the maximum pixel memory available for all
canvases.",
> +	       "returns": [
> +		   { "name": "maxPixelMemory", "type": "number", "description":
"Maximum pixel memory available for all canvases." }
> +	       ]

Lets drop this. I don't think this max is useful. If/when the page crosses this
max pixel memory a console message will be logged that lists what the maximum
size is.

> Source/JavaScriptCore/inspector/protocol/Canvas.json:99
> +		   { "name": "canvasId", "$ref": "CanvasId", "description":
"Identifier of canvas who's memory cost changed." },

Simplify the description.

> Source/WebCore/inspector/InspectorInstrumentation.h:1200
> +    if (InstrumentingAgents* instrumentingAgents =
instrumentingAgentsForDocument(&canvasElement.document()))

This should `FAST_RETURN_IF_NO_FRONTENDS(void());`


More information about the webkit-reviews mailing list