HTMLCanvasElement.prototype should have toBlog. See https://html.spec.whatwg.org/multipage/scripting.html#dom-canvas-toblob Found by the following newly added tests: LayoutTests/http/tests/w3c/html/semantics/embedded-content/the-canvas-element/toBlob.jpeg.html LayoutTests/http/tests/w3c/html/semantics/embedded-content/the-canvas-element/toBlob.png.html
<rdar://problem/22590406>
*** Bug 71270 has been marked as a duplicate of this bug. ***
Created attachment 303283 [details] Patch
Comment on attachment 303283 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303283&action=review > Source/WebCore/html/HTMLCanvasElement.cpp:503 > + quality = qualityValue.toNumber(context.execState()); Spec says: "If Type(quality) is not Number, or if quality is outside that range, the user agent must use its default quality value, as if the quality argument had not been given." We are not checking is value is a Number so are we really doing the right thing here?
Comment on attachment 303283 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303283&action=review >> Source/WebCore/html/HTMLCanvasElement.cpp:503 >> + quality = qualityValue.toNumber(context.execState()); > > Spec says: > "If Type(quality) is not Number, or if quality is outside that range, the user agent must use its default quality value, as if the quality argument had not been given." > > We are not checking is value is a Number so are we really doing the right thing here? For example, with the current code I believe the following cases would be wrong: - If the JS passes null, toNumber() returns 0 and we use 0 as quality instead of the default quality. - If the JS passes a Boolean, we are doing to use 0 or 1 as quality instead of the default one. - If the JS passes a Symbol, toNumber will throw an exception instead of using the default quality.
(In reply to comment #4) > Comment on attachment 303283 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=303283&action=review > > > Source/WebCore/html/HTMLCanvasElement.cpp:503 > > + quality = qualityValue.toNumber(context.execState()); > > Spec says: > "If Type(quality) is not Number, or if quality is outside that range, the > user agent must use its default quality value, as if the quality argument > had not been given." > > We are not checking is value is a Number so are we really doing the right > thing here? Good point. I'll check that qualityValue is a number instead of checking that it's not undefined.
Created attachment 303293 [details] Patch
Comment on attachment 303293 [details] Patch Clearing flags on attachment: 303293 Committed r213344: <http://trac.webkit.org/changeset/213344>
All reviewed patches have been landed. Closing bug.
Reverted r213344 for reason: This changed caused LayoutTest crashes under GuardMalloc. Committed r213378: <http://trac.webkit.org/changeset/213378>
Created attachment 303358 [details] Patch
Comment on attachment 303358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303358&action=review > Source/WebCore/html/HTMLCanvasElement.cpp:515 > + Vector<uint8_t> blobData = buffer()->toData(encodingMIMEType, quality); Andy, a question: if buffer()->toData fails, do we return an empty blob or a JS null blob to the JS caller here? (Spec says to return a null by the looks).
Comment on attachment 303358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303358&action=review >> Source/WebCore/html/HTMLCanvasElement.cpp:515 >> + Vector<uint8_t> blobData = buffer()->toData(encodingMIMEType, quality); > > Andy, a question: if buffer()->toData fails, do we return an empty blob or a JS null blob to the JS caller here? (Spec says to return a null by the looks). The spec says to return null if the canvas bitmap has no pixels, which we handle above on line 495. ImageBuffer::toData() can fail for various other reasons (primarily when the bitmap's area overflows an unsigned int), in which case we return an empty Blob. We could return null in that case too, but the standard doesn't specify a behavior for this case.
Comment on attachment 303358 [details] Patch Clearing flags on attachment: 303358 Committed r213412: <http://trac.webkit.org/changeset/213412>
Comment on attachment 303358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303358&action=review >>> Source/WebCore/html/HTMLCanvasElement.cpp:515 >>> + Vector<uint8_t> blobData = buffer()->toData(encodingMIMEType, quality); >> >> Andy, a question: if buffer()->toData fails, do we return an empty blob or a JS null blob to the JS caller here? (Spec says to return a null by the looks). > > The spec says to return null if the canvas bitmap has no pixels, which we handle above on line 495. ImageBuffer::toData() can fail for various other reasons (primarily when the bitmap's area overflows an unsigned int), in which case we return an empty Blob. We could return null in that case too, but the standard doesn't specify a behavior for this case. Seems to me that null blob is better. > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:614 > + auto image = adoptCF(CGImageCreate(source.width(), source.height(), 8, 32, 4 * source.width(), sRGBColorSpaceRef(), kCGBitmapByteOrderDefault | dataAlphaInfo, dataProvider.get(), 0, false, kCGRenderingIntentDefault)); > + return image; No reason for the local variable here. This should just be a return statement.
Comment on attachment 303358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303358&action=review >>> Source/WebCore/html/HTMLCanvasElement.cpp:515 >>> + Vector<uint8_t> blobData = buffer()->toData(encodingMIMEType, quality); >> >> Andy, a question: if buffer()->toData fails, do we return an empty blob or a JS null blob to the JS caller here? (Spec says to return a null by the looks). > > The spec says to return null if the canvas bitmap has no pixels, which we handle above on line 495. ImageBuffer::toData() can fail for various other reasons (primarily when the bitmap's area overflows an unsigned int), in which case we return an empty Blob. We could return null in that case too, but the standard doesn't specify a behavior for this case. If toDataURL fails for any reason, it returns "data:," and the code in this CL does that on quick scan, so that looks good to me (the canvas has no pixels, ImageBuffer::toData() fails, etc). JS developers can check for "data:," to detect failure. If toBlob fails for any reason, JS developers need a consistent return value on failure too. The original spec prose said to return a null Blob. As noted in [1], somehow the spec prose about that got lost. The spec was recently updated to recover the lost prose: see https://github.com/whatwg/html/pull/2343 [1] https://bugs.chromium.org/p/chromium/issues/detail?id=677691#c11
Thanks for the feedback, Darin and Noel. I'll post a follow-up patch to address these items.
Reopening to attach new patch.
Created attachment 303377 [details] Address review feedback
Comment on attachment 303377 [details] Address review feedback Attachment 303377 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3234821 New failing tests: http/tests/media/media-document.html
Created attachment 303382 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 303377 [details] Address review feedback View in context: https://bugs.webkit.org/attachment.cgi?id=303377&action=review > Source/WebCore/html/HTMLCanvasElement.cpp:509 > + if (!blobData.isEmpty()) Looks good. > Source/WebCore/html/HTMLCanvasElement.cpp:511 > + callback->scheduleCallback(context, WTFMove(blob)); If the first argument to toBlob is not a Function, for example if the JS user did this: document.createElement("canvas").toBlob(42); would callback be a C++ nullptr here?
(In reply to comment #23) > Comment on attachment 303377 [details] > Address review feedback > > View in context: > https://bugs.webkit.org/attachment.cgi?id=303377&action=review > > > Source/WebCore/html/HTMLCanvasElement.cpp:511 > > + callback->scheduleCallback(context, WTFMove(blob)); > > If the first argument to toBlob is not a Function, for example if the JS > user did this: > > document.createElement("canvas").toBlob(42); > > would callback be a C++ nullptr here? No, it would result in a TypeError being thrown in JavaScript. This is handled automatically by the generated bindings. The callback parameter of toBlob() is a Ref<>, which means it cannot be null. I'm going to write some new tests to check the various edge cases that have been brought up here.
Committed r213437: <https://trac.webkit.org/changeset/213437>
(In reply to comment #24) > (In reply to comment #23) > > Comment on attachment 303377 [details] > > Address review feedback > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=303377&action=review > > > > > Source/WebCore/html/HTMLCanvasElement.cpp:511 > > > + callback->scheduleCallback(context, WTFMove(blob)); > > > > If the first argument to toBlob is not a Function, for example if the JS > > user did this: > > > > document.createElement("canvas").toBlob(42); > > > > would callback be a C++ nullptr here? > > No, it would result in a TypeError being thrown in JavaScript. This is > handled automatically by the generated bindings. The callback parameter of > toBlob() is a Ref<>, which means it cannot be null. Ah good, thanks for explaining. > I'm going to write some new tests to check the various edge cases that have > been brought up here. Sounds good to me.