[webkit-reviews] review granted: [Bug 174481] Web Inspector: create protocol for recording Canvas contexts : [Attachment 316438] 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 174481: Web Inspector: create protocol for recording Canvas contexts
https://bugs.webkit.org/show_bug.cgi?id=174481

Attachment 316438: Patch

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




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

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

r=me

I have minor concerns about `NaN` not being handled properly because it is not
JSON serializable. But that is an edge case for canvas right now because we
never expect users to use NaN in these cases. I'll expect a test for that with
the next patch (the frontend portion).

> Source/WebCore/inspector/InspectorCanvas.cpp:147
> +    m_bufferLimit = std::min(memoryLimit,
static_cast<long>(std::numeric_limits<int>::max()));

std::min<long>() I think might make the right side cast not be necessary.

> Source/WebCore/inspector/InspectorCanvasAgent.h:79
> +    void didFinishRecordingCanvasFrame(HTMLCanvasElement&, bool
forceDispatch = false);

We prefer enums for boolean parameters so its clear what they mean at the call
site. Here you could do something like: ForceDispatch::Yes, ForceDispatch::No.
Not sure how important it is for this (I can't think of a great name) so I
could go either way.

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:48
> +    static fromPayload(payload)
> +    {
> +	   if (!Array.isArray(payload))
> +	       payload = [];
> +
> +	   if (isNaN(payload[0])) // Name
> +	       payload[0] = -1;
> +
> +	   if (!Array.isArray(payload[1])) // Parameters
> +	       payload[1] = [];
> +
> +	   return new WebInspector.RecordingAction(...payload);
> +    }

Maybe just a comment above the fromPayload describing the format. It'll be much
easier to read than the code below.

    // Payload format: [name, parameters]
    static fromPayload(payload)

Alternatively I've just added a comment with the protocol type it conforms to:

   Models/PropertyPreview.js
   47-	  // Runtime.PropertyPreview.
   48:	  static fromPayload(payload)