[webkit-reviews] review denied: [Bug 174482] Web Inspector: Record actions performed on CanvasRenderingContext2D : [Attachment 316487] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jul 26 16:04:12 PDT 2017


Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 174482: Web Inspector: Record actions performed on CanvasRenderingContext2D
https://bugs.webkit.org/show_bug.cgi?id=174482

Attachment 316487: Patch

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




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

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

r- because compiles failed.

> LayoutTests/inspector/canvas/recording-2d-expected.txt:1617
> +    "data:,",
> +    "testB",

Is the empty data: value expected for the createPattern from a video? And
probably everything else trying tog et image data from a video.

Can you add a test that actually gets image data from a video? It doesn't need
to be part of this large thing, it can be a very targeted test given its a
specific scenario.

> LayoutTests/inspector/canvas/recording-2d.html:45
> +function e(f){
> +    try { f(); } catch(e) { }
> +}

This name is rather cryptic. `ignoreException` would be better, but I think you
wanted it short to avoid clutter down below?

> LayoutTests/inspector/canvas/recording-2d.html:370
> +	   () => {

I think there should be a test for a function call with NaN to see what that
produces:

    ctx.setLineWidth(NaN)

Since NaN is not JSON stringifyable. It seems right now we produce `null` but
we should have a test for its behavior which we can see changes when we fix
that issue (if we decide to address this edge case).

> LayoutTests/inspector/canvas/recording-2d.html:386
> +    function requestRecording(parameters, resolve, reject) {

Instead of an array of parameters you could do:

    {singleFrame, memoryLimit}

Having a value of `undefined` should be the same as the default.

> LayoutTests/inspector/canvas/recording-2d.html:404
> +		   // The bots will fail since the path locally is most likely
different than the bots.

What does this mean?

> LayoutTests/inspector/canvas/recording-2d.html:411
> +	   CanvasAgent.requestRecording(canvases[0].identifier, ...parameters,
(error) => {

You'd then just pass the params here and it would be clear:

    CanvasAgent.requestRecording(canvases[0].identifier, singleFrame,
memoryLimit, (error) => { ... });

> LayoutTests/inspector/canvas/recording-2d.html:420
> +	       InspectorTest.evaluateInPage(`performActions()`, (error) => {
> +		   if (error)
> +		       reject(error);
> +	       });

Lets just do: InspectorTest.evaluateInPage(`performActions()`); We don't expect
these to error for simple cases like this, and if they did the test page would
still report it (and I think try to stop).

> LayoutTests/inspector/canvas/recording-2d.html:466
> +    suite.addTestCase({
> +	   name: "Canvas.requestRecording.InvalidCanvasId",

These tests can go into a more basic test:

    + LayoutTests/inspector/canvas/requestRecording.html
    + LayoutTests/inspector/canvas/cancelRecording.html
    * LayoutTests/inspector/canvas/recording-2d.html

Since these are not specific to 2d at all. Its also weird to bury them in here.


More information about the webkit-reviews mailing list