[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