Bug 148878 – Add support for canvas.toBlob

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Ryosuke Niwa]

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

[Radar WebKit Bug Importer]

<rdar://problem/22590406>

[David Kilzer (:ddkilzer)]

*** Bug 71270 has been marked as a duplicate of this bug. ***

[Andy Estes]

Created attachment 303283 [details]
Patch

[Chris Dumez]

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?

[Chris Dumez]

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.

[Andy Estes]

(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.

[Andy Estes]

Created attachment 303293 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 303293 [details]
Patch

Clearing flags on attachment: 303293

Committed r213344: <http://trac.webkit.org/changeset/213344>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

Reverted r213344 for reason:

This changed caused LayoutTest crashes under GuardMalloc.

Committed r213378: <http://trac.webkit.org/changeset/213378>

[Andy Estes]

Created attachment 303358 [details]
Patch

[noel gordon]

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).

[Andy Estes]

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.

[WebKit Commit Bot]

Comment on attachment 303358 [details]
Patch

Clearing flags on attachment: 303358

Committed r213412: <http://trac.webkit.org/changeset/213412>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[noel gordon]

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

[Andy Estes]

Thanks for the feedback, Darin and Noel. I'll post a follow-up patch to address these items.

[Andy Estes]

Reopening to attach new patch.

[Andy Estes]

Created attachment 303377 [details]
Address review feedback

[Build Bot]

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

[Build Bot]

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

[noel gordon]

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?

[Andy Estes]

(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.

[Andy Estes]

Committed r213437: <https://trac.webkit.org/changeset/213437>

[noel gordon]

(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.