Bug 148878 - Add support for canvas.toBlob
Summary: Add support for canvas.toBlob
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy Estes
URL:
Keywords: InRadar
: 71270 (view as bug list)
Depends on:
Blocks: 169240
  Show dependency treegraph
 
Reported: 2015-09-04 20:12 PDT by Ryosuke Niwa
Modified: 2024-03-27 09:24 PDT (History)
21 users (show)

See Also:


Attachments
Patch (33.01 KB, patch)
2017-03-02 18:24 PST, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (33.00 KB, patch)
2017-03-02 19:30 PST, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (33.67 KB, patch)
2017-03-03 16:54 PST, Andy Estes
no flags Details | Formatted Diff | Diff
Address review feedback (3.23 KB, patch)
2017-03-03 19:32 PST, Andy Estes
thorton: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-elcapitan (1.75 MB, application/zip)
2017-03-03 20:57 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2015-09-04 20:12:54 PDT
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
Comment 1 Radar WebKit Bug Importer 2015-09-04 20:13:37 PDT
<rdar://problem/22590406>
Comment 2 David Kilzer (:ddkilzer) 2016-01-25 11:38:56 PST
*** Bug 71270 has been marked as a duplicate of this bug. ***
Comment 3 Andy Estes 2017-03-02 18:24:20 PST
Created attachment 303283 [details]
Patch
Comment 4 Chris Dumez 2017-03-02 18:39:29 PST
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 5 Chris Dumez 2017-03-02 18:49:11 PST
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.
Comment 6 Andy Estes 2017-03-02 18:52:41 PST
(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.
Comment 7 Andy Estes 2017-03-02 19:30:14 PST
Created attachment 303293 [details]
Patch
Comment 8 WebKit Commit Bot 2017-03-02 20:11:10 PST
Comment on attachment 303293 [details]
Patch

Clearing flags on attachment: 303293

Committed r213344: <http://trac.webkit.org/changeset/213344>
Comment 9 WebKit Commit Bot 2017-03-02 20:11:17 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Ryan Haddad 2017-03-03 13:14:26 PST
Reverted r213344 for reason:

This changed caused LayoutTest crashes under GuardMalloc.

Committed r213378: <http://trac.webkit.org/changeset/213378>
Comment 11 Andy Estes 2017-03-03 16:54:00 PST
Created attachment 303358 [details]
Patch
Comment 12 noel gordon 2017-03-03 17:50:30 PST
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 13 Andy Estes 2017-03-03 18:01:55 PST
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 14 WebKit Commit Bot 2017-03-03 18:15:53 PST
Comment on attachment 303358 [details]
Patch

Clearing flags on attachment: 303358

Committed r213412: <http://trac.webkit.org/changeset/213412>
Comment 15 WebKit Commit Bot 2017-03-03 18:16:00 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Darin Adler 2017-03-03 18:38:39 PST
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 17 noel gordon 2017-03-03 18:45:11 PST
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
Comment 18 Andy Estes 2017-03-03 18:57:59 PST
Thanks for the feedback, Darin and Noel. I'll post a follow-up patch to address these items.
Comment 19 Andy Estes 2017-03-03 19:32:53 PST
Reopening to attach new patch.
Comment 20 Andy Estes 2017-03-03 19:32:58 PST
Created attachment 303377 [details]
Address review feedback
Comment 21 Build Bot 2017-03-03 20:57:16 PST
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
Comment 22 Build Bot 2017-03-03 20:57:26 PST
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 23 noel gordon 2017-03-03 21:41:11 PST
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?
Comment 24 Andy Estes 2017-03-03 23:22:57 PST
(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.
Comment 25 Andy Estes 2017-03-05 12:32:13 PST
Committed r213437: <https://trac.webkit.org/changeset/213437>
Comment 26 noel gordon 2017-03-08 18:00:28 PST
(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.