[HTMLCanvasElement.toBlob] Default callback version without scheduler

The toBlob method in HTMLCanvasElement has been implemented.

Changes include:
(1) HTMLCanvasElement: 

The toBlob function is implemented in a similar fashion as the existing toDataURL function, taking in parameters like mimeType and qualityArgument. It has an additional argument FileCallback which accepts user-defined callback function.

(2) ImageDataBuffer:

encodeImage is made public.

(3) File:

A new create factory method is added to allow HTMLCanvasElement to create a new File object using the blob data. File object is a subtype of Blob object; it has two additional attributes -- name, last modification time -- as compared to Blob. We use empty string and current time to fill in these two attributes; FF creates the latter attribute in the same way.

(4) FileCallback: 

Moved from module/ to core/. 



BUG=67587
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201327

[xlai (Olivia), 2015-07-31 18:21:24]

xlai@chromium.org changed reviewers:
+ junov@chromium.org

[Justin Novosad, 2015-08-13 21:03:40]

https://codereview.chromium.org/1257253004/diff/40001/LayoutTests/fast/canvas...
File LayoutTests/fast/canvas/canvas-toBlob-defaultpng.html (right):

https://codereview.chromium.org/1257253004/diff/40001/LayoutTests/fast/canvas...
LayoutTests/fast/canvas/canvas-toBlob-defaultpng.html:7: canvas =
document.createElement("canvas");
var canvas

https://codereview.chromium.org/1257253004/diff/40001/LayoutTests/fast/canvas...
LayoutTests/fast/canvas/canvas-toBlob-defaultpng.html:13: var newImg =
document.createElement("img"),
, -> ;

https://codereview.chromium.org/1257253004/diff/40001/LayoutTests/fast/canvas...
LayoutTests/fast/canvas/canvas-toBlob-defaultpng.html:14: url =
URL.createObjectURL(blob);
indentation

https://codereview.chromium.org/1257253004/diff/40001/LayoutTests/fast/canvas...
LayoutTests/fast/canvas/canvas-toBlob-defaultpng.html:18:
URL.revokeObjectURL(url);
This will not be called because the test finishes too early.

https://codereview.chromium.org/1257253004/diff/40001/LayoutTests/fast/canvas...
LayoutTests/fast/canvas/canvas-toBlob-defaultpng.html:26: var tags =
document.getElementsByTagName('img');
you should just make newImg global and use it directly here. The checks below
would be unnecessary.

https://codereview.chromium.org/1257253004/diff/40001/LayoutTests/fast/canvas...
LayoutTests/fast/canvas/canvas-toBlob-defaultpng.html:35:
shouldBecomeEqual("tryGetImageTag()", "'blob:null'", function(){}, 1000);
Can you explain what exactly this tests, why is this asynchronous?
Also, this test doe not verify that toBlob worked as far as I can tell.

https://codereview.chromium.org/1257253004/diff/40001/LayoutTests/fast/canvas...
LayoutTests/fast/canvas/canvas-toBlob-defaultpng.html:38:
testRunner.notifyDone();
This belongs in the shouldBecomeEqual completion handler. IF it works like this,
you probably don't need the test to be async in the first place. Also, you are
missing the mating call to waitUntilDone()

https://codereview.chromium.org/1257253004/diff/40001/Source/core/fileapi/Fil...
File Source/core/fileapi/File.cpp (right):

https://codereview.chromium.org/1257253004/diff/40001/Source/core/fileapi/Fil...
Source/core/fileapi/File.cpp:121: blobData->setContentType(mimeType);
your test should be checking that setting the mimeType worked

https://codereview.chromium.org/1257253004/diff/40001/Source/core/fileapi/File.h
File Source/core/fileapi/File.h (right):

https://codereview.chromium.org/1257253004/diff/40001/Source/core/fileapi/Fil...
Source/core/fileapi/File.h:72: static File* create(Vector<char>* encodedImage,
const String& mimeType);
You don't need to add a dependency on Vector here. You could just take char* +
size_t since this function does not even use any functionality of Vector.

https://codereview.chromium.org/1257253004/diff/40001/Source/core/html/HTMLCa...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1257253004/diff/40001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.cpp:37: #include "core/fileapi/File.h"
File is in core.  I think FileCallback should be as well then. That would
resolve the layering violation.  Forget what I said about making a partial
interface. Just move FileCallback to core/fileapi.  I think moving it to a
module was a mistake.

https://codereview.chromium.org/1257253004/diff/40001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.cpp:540:
exceptionState.throwSecurityError("Tainted canvases may not be exported.");
Excellent. But this need to be covered by a test.

https://codereview.chromium.org/1257253004/diff/40001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.cpp:544: File* resultBlob;
Code is less fragile if you initialize to nullptr here.

https://codereview.chromium.org/1257253004/diff/40001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.cpp:569:
Platform::current()->mainThread()->postDelayedTask(FROM_HERE,
threadSafeBind(&invokeFileCallbackWithBlob, callback, resultBlob), 1);
Why delayed?

https://codereview.chromium.org/1257253004/diff/40001/Source/core/html/HTMLCa...
File Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/1257253004/diff/40001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.h:117: void toBlob(FileCallback*, const
String& mimeType, const ScriptValue& qualityArgument, ExceptionState&) const;
This new entry point is not tested

https://codereview.chromium.org/1257253004/diff/40001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.h:192: 
spurious new line

https://codereview.chromium.org/1257253004/diff/40001/Source/core/html/HTMLCa...
File Source/core/html/HTMLCanvasElement.idl (right):

https://codereview.chromium.org/1257253004/diff/40001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.idl:55: [RaisesException] void
toBlob(FileCallback? _callback, optional DOMString type = null, optional any
arguments);
This feature is not yet approved for shipping. You need to hide it with
[RuntimeEnabled=ExperimentalCanvasFeatures]

https://codereview.chromium.org/1257253004/diff/40001/Source/platform/graphic...
File Source/platform/graphics/ImageBuffer.cpp (right):

https://codereview.chromium.org/1257253004/diff/40001/Source/platform/graphic...
Source/platform/graphics/ImageBuffer.cpp:366: bool
ImageDataBuffer::toDataVector(const String& mimeType, const double* quality,
Vector<char>& encodedDataVector) const
This is just a dumb wrapper. You might as well just make encodeImage a public
member of ImageDataBuffer

https://codereview.chromium.org/1257253004/diff/40001/Source/platform/graphic...
File Source/platform/graphics/ImageBuffer.h (right):

https://codereview.chromium.org/1257253004/diff/40001/Source/platform/graphic...
Source/platform/graphics/ImageBuffer.h:151: bool PLATFORM_EXPORT
toDataVector(const String& mimeType, const double* quality, Vector<char>&
encodedDataVector) const;
encodedDataVector is an output param: should be passed by pointer instead of by
ref.

[Justin Novosad, 2015-08-13 21:04:11]

This is really on the right track. Good work.

[Justin Novosad, 2015-08-13 21:07:20]

Forgot to mention. Please respect the maximum line length of 72 chars in the
description. Otherwise the logs look very bad, especially when we have to merge
or revert.

[xlai (Olivia), 2015-08-17 20:48:16]

xlai@chromium.org changed reviewers:
+ kinuko@chromium.org

[xlai (Olivia), 2015-08-20 18:09:43]

Patchset #7 (id:120001) has been deleted

[xlai (Olivia), 2015-08-20 19:38:28]

Patchset #5 (id:80001) has been deleted

[xlai (Olivia), 2015-08-20 19:46:44]

This is an implementation of toBlob function at HTML canvas element. It is the
default version similar to that exists in FF. 

Please only review this final patch set of this issue, which runs successfully
on all trybots.

Future plans after this implementation would be implementing an additional
toBlob function that uses Promise instead of Callback, as well as utilizing a
more advanced scheduler.

To Justin:
I didn't upload the layout test that catches security error for tainted canvas
because the current setup in Layout tests does not allow me to add an image from
external source; yet that's required for creating an image with foreign origin.
But I tested it in content-shell locally and it worked correctly.

To Kinuko:
Just to let you know that the interface of FileCallback has been moved from
module/ directory to core/ directory. This interface is needed by
HTMLCanvasElement.toBlob as an argument according to the specification.

https://codereview.chromium.org/1257253004/diff/40001/LayoutTests/fast/canvas...
File LayoutTests/fast/canvas/canvas-toBlob-defaultpng.html (right):

https://codereview.chromium.org/1257253004/diff/40001/LayoutTests/fast/canvas...
LayoutTests/fast/canvas/canvas-toBlob-defaultpng.html:7: canvas =
document.createElement("canvas");
On 2015/08/13 21:03:40, Justin Novosad wrote:
> var canvas

Done for this and elsewhere.

https://codereview.chromium.org/1257253004/diff/40001/LayoutTests/fast/canvas...
LayoutTests/fast/canvas/canvas-toBlob-defaultpng.html:38:
testRunner.notifyDone();
On 2015/08/13 21:03:39, Justin Novosad wrote:
> This belongs in the shouldBecomeEqual completion handler. IF it works like
this,
> you probably don't need the test to be async in the first place. Also, you are
> missing the mating call to waitUntilDone()

Deleted this. Changed the shouldBecomeEqual to shouldBe function. Remove
unnecessary checks about img tag.

https://codereview.chromium.org/1257253004/diff/40001/Source/core/fileapi/Fil...
File Source/core/fileapi/File.cpp (right):

https://codereview.chromium.org/1257253004/diff/40001/Source/core/fileapi/Fil...
Source/core/fileapi/File.cpp:121: blobData->setContentType(mimeType);
On 2015/08/13 21:03:40, Justin Novosad wrote:
> your test should be checking that setting the mimeType worked

Acknowledged. Planning to write one more layout test to specifically test the
mime type.

https://codereview.chromium.org/1257253004/diff/40001/Source/core/fileapi/File.h
File Source/core/fileapi/File.h (right):

https://codereview.chromium.org/1257253004/diff/40001/Source/core/fileapi/Fil...
Source/core/fileapi/File.h:72: static File* create(Vector<char>* encodedImage,
const String& mimeType);
On 2015/08/13 21:03:40, Justin Novosad wrote:
> You don't need to add a dependency on Vector here. You could just take char* +
> size_t since this function does not even use any functionality of Vector.

Done.

https://codereview.chromium.org/1257253004/diff/40001/Source/core/html/HTMLCa...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1257253004/diff/40001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.cpp:37: #include "core/fileapi/File.h"
On 2015/08/13 21:03:40, Justin Novosad wrote:
> File is in core.  I think FileCallback should be as well then. That would
> resolve the layering violation.  Forget what I said about making a partial
> interface. Just move FileCallback to core/fileapi.  I think moving it to a
> module was a mistake.

Acknowledged. Great! This sounds a simpler solution. Based on what I observed in
the code search, FileCallback was not used by anyone else.

https://codereview.chromium.org/1257253004/diff/40001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.cpp:540:
exceptionState.throwSecurityError("Tainted canvases may not be exported.");
On 2015/08/13 21:03:40, Justin Novosad wrote:
> Excellent. But this need to be covered by a test.

Acknowledged. One more layout test in my to-do list.

https://codereview.chromium.org/1257253004/diff/40001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.cpp:569:
Platform::current()->mainThread()->postDelayedTask(FROM_HERE,
threadSafeBind(&invokeFileCallbackWithBlob, callback, resultBlob), 1);
On 2015/08/13 21:03:40, Justin Novosad wrote:
> Why delayed?

Done. Changed back to postTask. It was my layout test's problem.

https://codereview.chromium.org/1257253004/diff/40001/Source/core/html/HTMLCa...
File Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/1257253004/diff/40001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.h:117: void toBlob(FileCallback*, const
String& mimeType, const ScriptValue& qualityArgument, ExceptionState&) const;
On 2015/08/13 21:03:40, Justin Novosad wrote:
> This new entry point is not tested

Acknowledged. Planning to write another Layout test to specifically test for
quality arguments.

https://codereview.chromium.org/1257253004/diff/40001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.h:192: 
On 2015/08/13 21:03:40, Justin Novosad wrote:
> spurious new line

Done.

https://codereview.chromium.org/1257253004/diff/40001/Source/core/html/HTMLCa...
File Source/core/html/HTMLCanvasElement.idl (right):

https://codereview.chromium.org/1257253004/diff/40001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.idl:55: [RaisesException] void
toBlob(FileCallback? _callback, optional DOMString type = null, optional any
arguments);
On 2015/08/13 21:03:40, Justin Novosad wrote:
> This feature is not yet approved for shipping. You need to hide it with
> [RuntimeEnabled=ExperimentalCanvasFeatures]

Done.

https://codereview.chromium.org/1257253004/diff/40001/Source/platform/graphic...
File Source/platform/graphics/ImageBuffer.cpp (right):

https://codereview.chromium.org/1257253004/diff/40001/Source/platform/graphic...
Source/platform/graphics/ImageBuffer.cpp:366: bool
ImageDataBuffer::toDataVector(const String& mimeType, const double* quality,
Vector<char>& encodedDataVector) const
On 2015/08/13 21:03:40, Justin Novosad wrote:
> This is just a dumb wrapper. You might as well just make encodeImage a public
> member of ImageDataBuffer

Done.

[Justin Novosad, 2015-08-20 20:38:43]

The duplicate set of baselines in platform/linux should be removed. I think they
are stale.

https://codereview.chromium.org/1257253004/diff/140001/LayoutTests/TestExpect...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/1257253004/diff/140001/LayoutTests/TestExpect...
LayoutTests/TestExpectations:753: crbug.com/67587  [ Mac Win ]
fast/canvas/canvas-toBlob-jpeg-maximum-quality.html [ NeedsRebaseline ]
These are new tests, so you shouldn't request platform-specific re-baselines.
Remove the "[ Mac Win ]"

https://codereview.chromium.org/1257253004/diff/140001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-toBlob-case-insensitive-mimetype.html
(right):

https://codereview.chromium.org/1257253004/diff/140001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-toBlob-case-insensitive-mimetype.html:25: if
(counter == 8) {
To make the test easier to extend, you should count the number of times
tryMimeType was called, and use that instead of the hard-coded 8.

https://codereview.chromium.org/1257253004/diff/140001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-toBlob-case-insensitive-mimetype.html:32:
function startTest()
This function does not really serve a purpose. It's code might as well just be
at global scope.

https://codereview.chromium.org/1257253004/diff/140001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-toBlob-case-insensitive-mimetype.html:34: counter
= 0;
declaration and init can be on same line.

https://codereview.chromium.org/1257253004/diff/140001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-toBlob-case-insensitive-mimetype.html:40:
tryMimeType("image/PNG", "image/png");
Just one test is enough to verify case insensitivity. You should also add a test
case that verifies that unsupported mime types fall back to png.

https://codereview.chromium.org/1257253004/diff/140001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-toBlob-defaultpng.html (right):

https://codereview.chromium.org/1257253004/diff/140001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-toBlob-defaultpng.html:18:
canvas.toBlob(function(blob) {
This needs to be written as an asynchronous test. Otherwise it will be flaky.

https://codereview.chromium.org/1257253004/diff/140001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-toBlob-jpeg-maximum-quality.html (right):

https://codereview.chromium.org/1257253004/diff/140001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-toBlob-jpeg-maximum-quality.html:2: <img
id="result" onload="testDone()">
Test is more readable if you set the onload handler near where you set src.

https://codereview.chromium.org/1257253004/diff/140001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-toBlob-jpeg-maximum-quality.html:17: var canvas =
document.getElementById("mycanvas");
indentation

https://codereview.chromium.org/1257253004/diff/140001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-toBlob-jpeg-maximum-quality.html:25: result.src =
url;
how come this works? Aren't we missing result =
document.getElementById('result');

https://codereview.chromium.org/1257253004/diff/140001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-toBlob-webp-maximum-quality.html (right):

https://codereview.chromium.org/1257253004/diff/140001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-toBlob-webp-maximum-quality.html:26: },
"image/webp", 1.0);
No need to solve this now, but please log a bug for it: the visual result from
this test are disappointing. Maximum quality compression should be lossless or
near lossless. The current results are visibly blurry.

https://codereview.chromium.org/1257253004/diff/140001/Source/core/fileapi/Fi...
File Source/core/fileapi/File.cpp (right):

https://codereview.chromium.org/1257253004/diff/140001/Source/core/fileapi/Fi...
Source/core/fileapi/File.cpp:116: File* File::create(char* encodedImage, size_t
size, const String& mimeType)
encodedImage -> data
size -> bytes

https://codereview.chromium.org/1257253004/diff/140001/Source/core/fileapi/Fi...
File Source/core/fileapi/File.h (right):

https://codereview.chromium.org/1257253004/diff/140001/Source/core/fileapi/Fi...
Source/core/fileapi/File.h:71: static File* create(char* encodedImage, size_t,
const String& mimeType);
Keep this generic: encodedImage -> data
Though we do prefer to omit arg names when the name provide no information, in
this case, I think you should name the size_t arg to specify the units. You
could call it "bytes".

https://codereview.chromium.org/1257253004/diff/140001/Source/core/html/HTMLC...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1257253004/diff/140001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.cpp:540:
exceptionState.throwSecurityError("Tainted canvases may not be exported.");
I did not see a test for this.

https://codereview.chromium.org/1257253004/diff/140001/Source/core/html/HTMLC...
File Source/core/html/HTMLCanvasElement.idl (right):

https://codereview.chromium.org/1257253004/diff/140001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.idl:49: [CallWith = ScriptState] any
getContext(DOMString contextId, [PermissiveDictionaryConversion] optional
CanvasContextCreationAttributes attributes);
Revert whitespace change.

https://codereview.chromium.org/1257253004/diff/140001/Source/platform/graphi...
File Source/platform/graphics/ImageBuffer.cpp (right):

https://codereview.chromium.org/1257253004/diff/140001/Source/platform/graphi...
Source/platform/graphics/ImageBuffer.cpp:330: bool
ImageDataBuffer::encodeImage(const String& mimeType, const double* quality,
Vector<char>* output)
This should be const

https://codereview.chromium.org/1257253004/diff/140001/Source/platform/graphi...
Source/platform/graphics/ImageBuffer.cpp:355: String
ImageDataBuffer::toDataURL(const String& mimeType, const double* quality)
this should stay const

https://codereview.chromium.org/1257253004/diff/140001/Source/platform/graphi...
Source/platform/graphics/ImageBuffer.cpp:366: 
unnecessary whitespace

[xlai (Olivia), 2015-08-20 21:36:29]

Some of my responses to the review feedbacks.

https://codereview.chromium.org/1257253004/diff/140001/LayoutTests/TestExpect...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/1257253004/diff/140001/LayoutTests/TestExpect...
LayoutTests/TestExpectations:753: crbug.com/67587  [ Mac Win ]
fast/canvas/canvas-toBlob-jpeg-maximum-quality.html [ NeedsRebaseline ]
On 2015/08/20 20:38:42, Justin Novosad wrote:
> These are new tests, so you shouldn't request platform-specific re-baselines.
> Remove the "[ Mac Win ]"

If I don't request platform-specific re-baselines, the tree on Mac/Win servers
are going to turn red. Suppress failures?

https://codereview.chromium.org/1257253004/diff/140001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-toBlob-case-insensitive-mimetype.html
(right):

https://codereview.chromium.org/1257253004/diff/140001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-toBlob-case-insensitive-mimetype.html:25: if
(counter == 8) {
On 2015/08/20 20:38:42, Justin Novosad wrote:
> To make the test easier to extend, you should count the number of times
> tryMimeType was called, and use that instead of the hard-coded 8.

Doing that might result in a flaky test. If the blob callback function of the
previous test finishes earlier than the tryMimeType is invoked in the next test,
and I am comparing whether the numbers of calls on both functions "balance" in
order to call notifyDone(), this test set might end up calling notifyDone()
earlier than expected. Right now, although hard-coding is not good looking, the
test can end at exactly the time when it should end.

https://codereview.chromium.org/1257253004/diff/140001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-toBlob-case-insensitive-mimetype.html:40:
tryMimeType("image/PNG", "image/png");
On 2015/08/20 20:38:42, Justin Novosad wrote:
> Just one test is enough to verify case insensitivity. You should also add a
test
> case that verifies that unsupported mime types fall back to png.

Acknowledged and elsewhere.

https://codereview.chromium.org/1257253004/diff/140001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-toBlob-jpeg-maximum-quality.html (right):

https://codereview.chromium.org/1257253004/diff/140001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-toBlob-jpeg-maximum-quality.html:25: result.src =
url;
On 2015/08/20 20:38:42, Justin Novosad wrote:
> how come this works? Aren't we missing result =
> document.getElementById('result');

Acknowledged and elsewhere.
I copied this from toDataURL layout test and was also surprised that the webpage
shows correctly the result image. Will changed that to getElementByID as that
looks more secure.

https://codereview.chromium.org/1257253004/diff/140001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-toBlob-webp-maximum-quality.html (right):

https://codereview.chromium.org/1257253004/diff/140001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-toBlob-webp-maximum-quality.html:26: },
"image/webp", 1.0);
On 2015/08/20 20:38:43, Justin Novosad wrote:
> No need to solve this now, but please log a bug for it: the visual result from
> this test are disappointing. Maximum quality compression should be lossless or
> near lossless. The current results are visibly blurry.

Done. Issued Logged: https://code.google.com/p/chromium/issues/detail?id=523098

May I ask if this is related to Issue 170565 (the alpha channel webp one) as
well?

https://codereview.chromium.org/1257253004/diff/140001/Source/core/fileapi/Fi...
File Source/core/fileapi/File.cpp (right):

https://codereview.chromium.org/1257253004/diff/140001/Source/core/fileapi/Fi...
Source/core/fileapi/File.cpp:116: File* File::create(char* encodedImage, size_t
size, const String& mimeType)
On 2015/08/20 20:38:43, Justin Novosad wrote:
> encodedImage -> data
> size -> bytes

Acknowledged.

https://codereview.chromium.org/1257253004/diff/140001/Source/core/fileapi/Fi...
File Source/core/fileapi/File.h (right):

https://codereview.chromium.org/1257253004/diff/140001/Source/core/fileapi/Fi...
Source/core/fileapi/File.h:71: static File* create(char* encodedImage, size_t,
const String& mimeType);
On 2015/08/20 20:38:43, Justin Novosad wrote:
> Keep this generic: encodedImage -> data
> Though we do prefer to omit arg names when the name provide no information, in
> this case, I think you should name the size_t arg to specify the units. You
> could call it "bytes".

Hi Justin, if I give a name to the parameter with size_t, then PRE_SUBMMIT will
fail as it protests that this name gives no information.

https://codereview.chromium.org/1257253004/diff/140001/Source/core/html/HTMLC...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1257253004/diff/140001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.cpp:540:
exceptionState.throwSecurityError("Tainted canvases may not be exported.");
On 2015/08/20 20:38:43, Justin Novosad wrote:
> I did not see a test for this.

Hi Justin, I wrote in the message about the reason why I didn't upload the
layout test for this one. First of all, in order to create this security error I
must create a "tainted canvas"; a tainted canvas is resulted from drawing to
canvas from an image with a foreign origin. In this case, it is an image from
external source. However, the current setup in Layout tests does not allow me to
add an image from external source; the run_webkit_tests script will protest on
that external source. Images from local directory cannot cause an exception.

I only tested it in content-shell locally and the browser does throw a JS
exception of "toBlob cannot be called: tainted canvas cannot be exported"
message properly.

Alos, this part is exactly the same as that in toDataURL.

https://codereview.chromium.org/1257253004/diff/140001/Source/core/html/HTMLC...
File Source/core/html/HTMLCanvasElement.idl (right):

https://codereview.chromium.org/1257253004/diff/140001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.idl:49: [CallWith = ScriptState] any
getContext(DOMString contextId, [PermissiveDictionaryConversion] optional
CanvasContextCreationAttributes attributes);
On 2015/08/20 20:38:43, Justin Novosad wrote:
> Revert whitespace change.

Acknowledged.

https://codereview.chromium.org/1257253004/diff/140001/Source/platform/graphi...
File Source/platform/graphics/ImageBuffer.cpp (right):

https://codereview.chromium.org/1257253004/diff/140001/Source/platform/graphi...
Source/platform/graphics/ImageBuffer.cpp:330: bool
ImageDataBuffer::encodeImage(const String& mimeType, const double* quality,
Vector<char>* output)
On 2015/08/20 20:38:43, Justin Novosad wrote:
> This should be const

Acknowledged and elsewhere.

[Justin Novosad, 2015-08-21 15:31:21]

On 2015/08/20 21:36:29, Olivia wrote:
> On 2015/08/20 20:38:42, Justin Novosad wrote:
> > These are new tests, so you shouldn't request platform-specific
re-baselines.
> > Remove the "[ Mac Win ]"
> 
> If I don't request platform-specific re-baselines, the tree on Mac/Win servers
> are going to turn red. Suppress failures?

That is not how it works.  If you don't specify platforms, all platforms are
rebaselined by the auto rebaseline bot. It will automatically figure out which
platforms need specific baselines and which can rely on the fallback chain to
use the baseline from a parent platform.

> 

> On 2015/08/20 20:38:43, Justin Novosad wrote:
> > No need to solve this now, but please log a bug for it: the visual result
from
> > this test are disappointing. Maximum quality compression should be lossless
or
> > near lossless. The current results are visibly blurry.
> 
> Done. Issued Logged:
https://code.google.com/p/chromium/issues/detail?id=523098
> 
> May I ask if this is related to Issue 170565 (the alpha channel webp one) as
> well?
>

Related but not the same. IIRC webp is capable of lossless compression.  We
should probably
explicitly trigger that when requesting 100% quality.
 

> On 2015/08/20 20:38:43, Justin Novosad wrote:
> > Keep this generic: encodedImage -> data
> > Though we do prefer to omit arg names when the name provide no information,
in
> > this case, I think you should name the size_t arg to specify the units. You
> > could call it "bytes".
> 
> Hi Justin, if I give a name to the parameter with size_t, then PRE_SUBMMIT
will
> fail as it protests that this name gives no information.

That is probably because you used the name "size". I am suggesting the name
"bytes", which does provide information.
Just so you know, the presubmit check protests when the argument name is a
substring of the type name.

[xlai (Olivia), 2015-08-21 18:40:06]

Please review only the last patch set. Thanks!

https://codereview.chromium.org/1257253004/diff/180001/LayoutTests/TestExpect...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/1257253004/diff/180001/LayoutTests/TestExpect...
LayoutTests/TestExpectations:756: crbug.com/67587 
fast/canvas/canvas-toBlob-defaultpng.html [ NeedsRebaseline ]
Default PNG is now a pixel test; this is in line with the other supported image
formats (jpeg, webp) that are also tested in pixel tests.
The platform specific wording has been removed as requested.

https://codereview.chromium.org/1257253004/diff/180001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-toBlob-case-insensitive-mimetype.html
(right):

https://codereview.chromium.org/1257253004/diff/180001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-toBlob-case-insensitive-mimetype.html:42:
//Unsupported mime type falls back to png
Added a test for unsupported mime type as requested. Trimmed down repetitive
tests on others. The hard-coded counter is not deleted (reason explained in
previous code review comments about flakiness) but moved to a more obvious place
to avoid potential incorrect modification in the future.

https://codereview.chromium.org/1257253004/diff/180001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-toBlob-defaultpng.html (right):

https://codereview.chromium.org/1257253004/diff/180001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-toBlob-defaultpng.html:2: <script src =
"../../resources/js-test.js"></script>
Using asynchronous testing from js-test testing framework instead of testharness
framework. This is now a pixel test (directly comparing end result from toBlob).

https://codereview.chromium.org/1257253004/diff/180001/LayoutTests/http/tests...
File LayoutTests/http/tests/security/canvas-read-blocked-by-setting.html
(right):

https://codereview.chromium.org/1257253004/diff/180001/LayoutTests/http/tests...
LayoutTests/http/tests/security/canvas-read-blocked-by-setting.html:53:
test(function () {
security error test for toBlob function added; this is same as toDataURL.

https://codereview.chromium.org/1257253004/diff/180001/Source/core/fileapi/Fi...
File Source/core/fileapi/File.h (right):

https://codereview.chromium.org/1257253004/diff/180001/Source/core/fileapi/Fi...
Source/core/fileapi/File.h:71: static File* create(char* data, size_t bytes,
const String& mimeType);
Renamed the arguments as requested.

https://codereview.chromium.org/1257253004/diff/180001/Source/platform/graphi...
File Source/platform/graphics/ImageBuffer.h (right):

https://codereview.chromium.org/1257253004/diff/180001/Source/platform/graphi...
Source/platform/graphics/ImageBuffer.h:151: bool PLATFORM_EXPORT
encodeImage(const String& mimeType, const double* quality, Vector<char>* output)
const;
const keyword added as requested.

[Justin Novosad, 2015-08-21 18:53:44]

https://codereview.chromium.org/1257253004/diff/180001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-toBlob-defaultpng.html (right):

https://codereview.chromium.org/1257253004/diff/180001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-toBlob-defaultpng.html:5:
testRunner.dumpAsTextWithPixelResults();
This should not be a pixel test. We only use pixel tests as a last resort, in
case where text or ref baselines cannot get the job done.  This is because image
baselines are a burden to maintain and they take a lot of space in the repo. 
The other tests you create are appropriate as pixel tests because the baselines
will capture subtleties in compression quality/artifacts.

https://codereview.chromium.org/1257253004/diff/180001/Source/core/html/HTMLC...
File Source/core/html/HTMLCanvasElement.idl (right):

https://codereview.chromium.org/1257253004/diff/180001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.idl:56: [RaisesException,
RuntimeEnabled=ExperimentalCanvasFeatures] void toBlob(FileCallback? _callback,
optional DOMString type = null, optional any arguments);
Why the '_' before callback?

[acterhd, 2015-08-21 19:53:03]

acterhd@gmail.com changed reviewers:
+ acterhd@gmail.com

[acterhd, 2015-08-21 19:53:03]

I made compatible patch. 
https://codereview.chromium.org/1294973005/
- Added alpha channel support
- Used lossy compression

[xlai (Olivia), 2015-08-24 15:24:42]

xlai@chromium.org changed reviewers:
- acterhd@gmail.com

[xlai (Olivia), 2015-08-24 19:21:23]

Modified the default layout test (from pixel test back to text test again) based
on discussion with Justin. Callback version completed.

[kinuko, 2015-08-26 06:28:31]

Sorry for slow review. LGTM for the changes related to FileCallback.

Could you also add a link to the spec for this method and to the thread on
blink-dev for intent-to-implement?

https://codereview.chromium.org/1257253004/diff/220001/Source/core/html/HTMLC...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1257253004/diff/220001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.cpp:568:
Platform::current()->mainThread()->postTask(FROM_HERE,
threadSafeBind(&invokeFileCallbackWithBlob, callback, resultBlob));
nit: if you're posting on from the main thread just using bind() should be fine,
and I think you can also directly bind the callback like:
bink(&FileCallback::handleEvent, callback, resultBlob)

[xlai (Olivia), 2015-08-26 14:01:57]

xlai@chromium.org changed reviewers:
+ senorblanco@google.com

[xlai (Olivia), 2015-08-26 14:19:07]

The CQ bit was checked by xlai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-26 14:19:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257253004/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257253004/220001

[commit-bot: I haz the power, 2015-08-26 14:21:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-26 14:21:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[xlai (Olivia), 2015-08-26 17:00:32]

The CQ bit was checked by xlai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-26 17:00:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257253004/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257253004/240001

[commit-bot: I haz the power, 2015-08-26 17:22:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-26 17:22:49]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[xlai (Olivia), 2015-08-26 19:21:23]

The CQ bit was checked by xlai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-26 19:21:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257253004/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257253004/240001

[commit-bot: I haz the power, 2015-08-26 20:14:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-26 20:14:46]

Dry run: This issue passed the CQ dry run.

[xlai (Olivia), 2015-08-27 15:56:14]

The CQ bit was checked by xlai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-27 15:56:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257253004/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257253004/260001

[commit-bot: I haz the power, 2015-08-27 17:00:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-27 17:00:05]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[xlai (Olivia), 2015-08-27 17:19:04]

The CQ bit was checked by xlai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-27 17:19:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257253004/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257253004/260001

[commit-bot: I haz the power, 2015-08-27 18:17:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-27 18:17:09]

Dry run: This issue passed the CQ dry run.

[xlai (Olivia), 2015-08-27 18:41:50]

Stephen,

Right now the last patch set passed all dry run on 2015-08-27 18:17:09 UTC after
I did a merge with origin master and a small modification based on Kinuko's
feedback.

For this issue I've gone through three iterations of code reviews with Justin
Novosad as you could see from the comments. In the last round, there was only
one layout test (canvas-toBlob-defaultpng.html) that needs to be changed from
pixel test to text-only test. We had a discussion before his vacation and agreed
upon to change this test in such ways:

Justin Novosad:
You can draw the image into a second canvas and call getImageData on the second
canvas to inspect the result of the draw
I am suggesting is to use "shouldBe'
There are plenty of canvas tests that work this way
draw to a canvas, peek a pixel with getImageData, compare the value to what you
expect
That will verify that the image data survives the toBlob
in the image onload, you do the draw to the second canvas
secondCanvas.getContext('2d').drawImage(imageThatHasTheBlobLoadedInotIt, ...); 

Olivia Lai:
Ah....I see....I should draw it to a second canvas and do the comparison there
first.
So this test involves three objects: image, canvas1, canvas2

Justin Novosad:
yes
exactly

https://codereview.chromium.org/1257253004/diff/180001/Source/core/html/HTMLC...
File Source/core/html/HTMLCanvasElement.idl (right):

https://codereview.chromium.org/1257253004/diff/180001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.idl:56: [RaisesException,
RuntimeEnabled=ExperimentalCanvasFeatures] void toBlob(FileCallback? _callback,
optional DOMString type = null, optional any arguments);
On 2015/08/21 18:53:44, Justin Novosad OOO back Sept 4 wrote:
> Why the '_' before callback?

Based on discussion with Justin on Aug 21, "_" appears in the WebIDL
specification. So we should still keep it.

https://codereview.chromium.org/1257253004/diff/220001/Source/core/html/HTMLC...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1257253004/diff/220001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.cpp:568:
Platform::current()->mainThread()->postTask(FROM_HERE,
threadSafeBind(&invokeFileCallbackWithBlob, callback, resultBlob));
On 2015/08/26 06:28:30, kinuko wrote:
> nit: if you're posting on from the main thread just using bind() should be
fine,
> and I think you can also directly bind the callback like:
> bink(&FileCallback::handleEvent, callback, resultBlob)

Done.

[Stephen White, 2015-08-27 19:10:46]

senorblanco@chromium.org changed reviewers:
+ senorblanco@chromium.org

[Stephen White, 2015-08-27 19:10:47]

LGTM.

Nits can be fixed or ignored, at your discretion.

https://codereview.chromium.org/1257253004/diff/260001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-toBlob-case-insensitive-mimetype.html
(right):

https://codereview.chromium.org/1257253004/diff/260001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-toBlob-case-insensitive-mimetype.html:35: // at
random order but they will all print PASS when they pass.
I think the way to make this totally safe would be to only record the pass/fail
state for each test case while running, and then output the pass/fail state for
each test once the counter hits zero (just before notifyDone()).

If one test case does start failing, for whatever reason, the test as-written
will fail flakily (ie., in a way that can't be captured with deterministic
results). Not a big deal, though.

https://codereview.chromium.org/1257253004/diff/260001/Source/core/fileapi/Fi...
File Source/core/fileapi/File.cpp (left):

https://codereview.chromium.org/1257253004/diff/260001/Source/core/fileapi/Fi...
Source/core/fileapi/File.cpp:104: 
Nit: spurious whitespace change?

[xlai (Olivia), 2015-08-27 19:18:57]

The CQ bit was checked by xlai@chromium.org

[xlai (Olivia), 2015-08-27 19:18:57]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/1257253004/#ps260001
(title: "A small change from thread safe bind to common bind based on Kinuko's
review")

[commit-bot: I haz the power, 2015-08-27 19:19:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257253004/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257253004/260001

[commit-bot: I haz the power, 2015-08-27 19:24:37]

Committed patchset #12 (id:260001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201327

[Noel Gordon, 2015-08-28 08:53:46]

noel@chromium.org changed reviewers:
+ noel@chromium.org

[Noel Gordon, 2015-08-28 08:53:47]

I did not see at test for using toBlob from a webgl canvas.  Could you add one?

Other comments in-line ...

https://codereview.chromium.org/1257253004/diff/260001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-toBlob-defaultpng.html (right):

https://codereview.chromium.org/1257253004/diff/260001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-toBlob-defaultpng.html:20: //Randomly pick a
pixel to check whether they match
This doesn't appear to be random pixel pick (it always picks imageData.data[4]).

Since your image is so small (150x75), could you fill the source canvas with
random pixel values, and compare the canvii getImageData.data for all values,
not just .data[4]?

Also is the canvas hardware accelerated (it depends on the canvas size)?  Is
there a toBlob test for the accelerated 2d canvas case?

https://codereview.chromium.org/1257253004/diff/260001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-toBlob-webp-maximum-quality.html (right):

https://codereview.chromium.org/1257253004/diff/260001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-toBlob-webp-maximum-quality.html:26: },
"image/webp", 1.0);
image/webp does not have the special treatment that image/jpeg gets at quality
1.0.  Thanks for the bug report.

https://codereview.chromium.org/1257253004/diff/260001/Source/core/fileapi/Fi...
File Source/core/fileapi/File.cpp (right):

https://codereview.chromium.org/1257253004/diff/260001/Source/core/fileapi/Fi...
Source/core/fileapi/File.cpp:126: return File::create("", currentTimeMS(),
BlobDataHandle::create(blobData.release(), blobSize));
Mozilla returns a Blob, but Chrome returns a File by doing this way. That
difference might cause compat problems, right? 

Could we change this code to return a Blob.  The FileCallback will still work
since a File* and Blob* are acceptable in the callback argument, I believe, but
you'd need to test.

https://codereview.chromium.org/1257253004/diff/260001/Source/core/html/HTMLC...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1257253004/diff/260001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.cpp:554: return;
I don't think the spec allows us to just return here.  What does the spec say to
do in this case?

https://codereview.chromium.org/1257253004/diff/260001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.cpp:572: ImageDataBuffer(imageData->size(),
imageData->data()->data()).encodeImage(encodingMimeType, &quality,
&encodedImage);
encodeImage is bool returning (success/failure).  toDataURL must return "data:,"
on failure, and the <canvas> spec requires that  toBlob similarly handle the
failure case.  What does the spec say to do on failure?

Secondly, the image is encoded on the main thread here, which is not really in
the spirit of the toBlob spec (wants an aysnc encode). Is there a plan to ensure
that this image encode does not jank the main thread?  No point doing toBlob
otherwise.

https://codereview.chromium.org/1257253004/diff/260001/Source/core/html/HTMLC...
File Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/1257253004/diff/260001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.h:118: void toBlob(FileCallback* callback,
const String& mimeType, ExceptionState& exceptionState) { return
toBlob(callback, mimeType, ScriptValue(), exceptionState); }
nit: if you removed this line, does chrome compile?  Not sure there are any
callers of this second variant of toBlob (or the similar toDataURL variant
either). We should remove them if they are dead-code.

https://codereview.chromium.org/1257253004/diff/260001/Source/core/html/HTMLC...
File Source/core/html/HTMLCanvasElement.idl (right):

https://codereview.chromium.org/1257253004/diff/260001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.idl:56: [RaisesException,
RuntimeEnabled=ExperimentalCanvasFeatures] void toBlob(FileCallback? _callback,
optional DOMString type = null, optional any arguments);
_callback does look odd.  Is there a reason why it can't just be callback?

https://codereview.chromium.org/1257253004/diff/260001/Source/modules/filesys...
File Source/modules/filesystem/InspectorFileSystemAgent.cpp (left):

https://codereview.chromium.org/1257253004/diff/260001/Source/modules/filesys...
Source/modules/filesystem/InspectorFileSystemAgent.cpp:41: #include
"core/fileapi/FileError.h"
Agree, having FileCallback in module was just plain wrong.  File and Blob are
Core concepts and their callbacks are as well.  Thanks for moving it.

https://codereview.chromium.org/1257253004/diff/260001/Source/platform/graphi...
File Source/platform/graphics/ImageBuffer.cpp (right):

https://codereview.chromium.org/1257253004/diff/260001/Source/platform/graphi...
Source/platform/graphics/ImageBuffer.cpp:344: bool
ImageDataBuffer::encodeImage(const String& mimeType, const double* quality,
Vector<char>* output) const
This function was moved to ImageDataBuffer so toBlob can call it directly, but
the ASSERT that

  ASSERT(MIMETypeRegistry::isSupportedImageMIMETypeForEncoding(mimeType));

seems to be gone. The toDataURL code path has that ASSERT, but the toBlob code
path does not (unless I missed it).  How come?

[xlai (Olivia), 2015-08-28 19:54:19]

Hi Noel,

Right now, toBlob is still marked as an experimental feature; and I work on
improving it incrementally.

This issue is just the first step of our multi-step plan on rolling out the
functionality; at this time I only aim to land a basic functionality with some
basic smoke tests. 

More conformance tests and edge case tests are going to be written as next
steps. I'll take note of your suggestions on this issue.

Also, as what you've pointed out, the image encoding should be asynchronous. I'm
working on Issue 523414 which is about using post idle task for the image
encoding process to make the whole process async. If you're interested you can
follow that.

Other replies are inline.

https://codereview.chromium.org/1257253004/diff/260001/LayoutTests/fast/canva...
File LayoutTests/fast/canvas/canvas-toBlob-defaultpng.html (right):

https://codereview.chromium.org/1257253004/diff/260001/LayoutTests/fast/canva...
LayoutTests/fast/canvas/canvas-toBlob-defaultpng.html:20: //Randomly pick a
pixel to check whether they match
On 2015/08/28 08:53:47, noel gordon wrote:
> This doesn't appear to be random pixel pick (it always picks
imageData.data[4]).
> 
> Since your image is so small (150x75), could you fill the source canvas with
> random pixel values, and compare the canvii getImageData.data for all values,
> not just .data[4]?
> 
> Also is the canvas hardware accelerated (it depends on the canvas size)?  Is
> there a toBlob test for the accelerated 2d canvas case?

Acknowledged for randomized pixel picking.

https://codereview.chromium.org/1257253004/diff/260001/Source/core/fileapi/Fi...
File Source/core/fileapi/File.cpp (left):

https://codereview.chromium.org/1257253004/diff/260001/Source/core/fileapi/Fi...
Source/core/fileapi/File.cpp:104: 
On 2015/08/27 19:10:47, Stephen White wrote:
> Nit: spurious whitespace change?

Acknowledged.

https://codereview.chromium.org/1257253004/diff/260001/Source/core/fileapi/Fi...
File Source/core/fileapi/File.cpp (right):

https://codereview.chromium.org/1257253004/diff/260001/Source/core/fileapi/Fi...
Source/core/fileapi/File.cpp:126: return File::create("", currentTimeMS(),
BlobDataHandle::create(blobData.release(), blobSize));
On 2015/08/28 08:53:47, noel gordon wrote:
> Mozilla returns a Blob, but Chrome returns a File by doing this way. That
> difference might cause compat problems, right? 
> 
> Could we change this code to return a Blob.  The FileCallback will still work
> since a File* and Blob* are acceptable in the callback argument, I believe,
but
> you'd need to test.

I did make attempts to create Blob* instead of File* but it didn't compile on
FileCallback.handleEvent. Meanwhile, the spec at FileCallback only has functions
handling File*
(https://html.spec.whatwg.org/multipage/infrastructure.html#filecallback). 

I browsed through FF's source code on this and felt that they seemed to have
defined their own version of spec on File/Blob to circumvent this situation. We
need serious discussion with Justin if we want to do the same; changing the spec
isn't trivial.

https://codereview.chromium.org/1257253004/diff/260001/Source/core/html/HTMLC...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1257253004/diff/260001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.cpp:554: return;
On 2015/08/28 08:53:47, noel gordon wrote:
> I don't think the spec allows us to just return here.  What does the spec say
to
> do in this case?

Acknowledged. The spec says "If the canvas element's bitmap has no pixels (i.e.
either its horizontal dimension or its vertical dimension is zero) then let
result be null.". I think I need to fix this in the next round of code review
issue by passing the null resultBlob to the call back function.

https://codereview.chromium.org/1257253004/diff/260001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.cpp:572: ImageDataBuffer(imageData->size(),
imageData->data()->data()).encodeImage(encodingMimeType, &quality,
&encodedImage);
On 2015/08/28 08:53:47, noel gordon wrote:
> encodeImage is bool returning (success/failure).  toDataURL must return
"data:,"
> on failure, and the <canvas> spec requires that  toBlob similarly handle the
> failure case.  What does the spec say to do on failure?
> 
> Secondly, the image is encoded on the main thread here, which is not really in
> the spirit of the toBlob spec (wants an aysnc encode). Is there a plan to
ensure
> that this image encode does not jank the main thread?  No point doing toBlob
> otherwise.

Acknowledged. I will explain in the overall message I sent to you.

https://codereview.chromium.org/1257253004/diff/260001/Source/core/html/HTMLC...
File Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/1257253004/diff/260001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.h:118: void toBlob(FileCallback* callback,
const String& mimeType, ExceptionState& exceptionState) { return
toBlob(callback, mimeType, ScriptValue(), exceptionState); }
On 2015/08/28 08:53:47, noel gordon wrote:
> nit: if you removed this line, does chrome compile?  Not sure there are any
> callers of this second variant of toBlob (or the similar toDataURL variant
> either). We should remove them if they are dead-code.

Haven't tried removing it and compile yet; I'll give a try later. 

But I don't think they are dead code. What if users call toBlob or toDataURL
functions without supplying any quality arguments?

https://codereview.chromium.org/1257253004/diff/260001/Source/core/html/HTMLC...
File Source/core/html/HTMLCanvasElement.idl (right):

https://codereview.chromium.org/1257253004/diff/260001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.idl:56: [RaisesException,
RuntimeEnabled=ExperimentalCanvasFeatures] void toBlob(FileCallback? _callback,
optional DOMString type = null, optional any arguments);
On 2015/08/28 08:53:47, noel gordon wrote:
> _callback does look odd.  Is there a reason why it can't just be callback?

Justin raised this issue before and we had discussed abt it. I was following the
WebIDL at spec
(https://html.spec.whatwg.org/multipage/scripting.html#the-canvas-element), the
argument's name has an underscore there.

https://codereview.chromium.org/1257253004/diff/260001/Source/platform/graphi...
File Source/platform/graphics/ImageBuffer.cpp (right):

https://codereview.chromium.org/1257253004/diff/260001/Source/platform/graphi...
Source/platform/graphics/ImageBuffer.cpp:344: bool
ImageDataBuffer::encodeImage(const String& mimeType, const double* quality,
Vector<char>* output) const
On 2015/08/28 08:53:47, noel gordon wrote:
> This function was moved to ImageDataBuffer so toBlob can call it directly, but
> the ASSERT that
> 
>   ASSERT(MIMETypeRegistry::isSupportedImageMIMETypeForEncoding(mimeType));
> 
> seems to be gone. The toDataURL code path has that ASSERT, but the toBlob code
> path does not (unless I missed it).  How come?

I believe that this ASSERT in toDataURL is redundant. I can take away that in
future commit.

Both toDataURL and toBlob call an internal method in HTMLCanvasElement.cpp
called "HTMLCanvasElement::toEncodingMimeType(const String& mimeType)" and that
method already invokes "MIMETypeRegistry::isSupportedImageMIMETypeForEncoding".
So by the time when the execution reaches encodeImage, it is already guaranteed
that the mime type is a supported type. One of my layout test does prove
unsupported mime type does fall back to default png.

[Noel Gordon, 2015-09-01 07:47:19]

On 2015/08/28 19:54:19, Olivia wrote:
> Hi Noel,
> 
> Right now, toBlob is still marked as an experimental feature; and I work on
> improving it incrementally.
> 
> This issue is just the first step of our multi-step plan on rolling out the
> functionality; at this time I only aim to land a basic functionality with some
> basic smoke tests.

sgtm.

> More conformance tests and edge case tests are going to be written as next
> steps. I'll take note of your suggestions on this issue.

ok.

> Also, as what you've pointed out, the image encoding should be asynchronous.
I'm
> working on Issue 523414 which is about using post idle task for the image
> encoding process to make the whole process async. If you're interested you can
> follow that.

followed.
 
> Other replies are inline.
> 
>
https://codereview.chromium.org/1257253004/diff/260001/Source/core/fileapi/Fi...
> File Source/core/fileapi/File.cpp (right):
> 
>
https://codereview.chromium.org/1257253004/diff/260001/Source/core/fileapi/Fi...
> Source/core/fileapi/File.cpp:126: return File::create("", currentTimeMS(),
> BlobDataHandle::create(blobData.release(), blobSize));
> On 2015/08/28 08:53:47, noel gordon wrote:
> > Mozilla returns a Blob, but Chrome returns a File by doing this way. That
> > difference might cause compat problems, right? 
> > 
> > Could we change this code to return a Blob.  The FileCallback will still
work
> > since a File* and Blob* are acceptable in the callback argument, I believe,
> but
> > you'd need to test.
> 
> I did make attempts to create Blob* instead of File* but it didn't compile on
> FileCallback.handleEvent. Meanwhile, the spec at FileCallback only has
functions
> handling File*
> (https://html.spec.whatwg.org/multipage/infrastructure.html#filecallback). 

Maybe define the static helper to create the Blob in Blob.cpp (rather than
File.cpp) and make it return a Blob*, and when you invoke the callback, cast the
Blob* to a File*.  Something like this ...

https://gist.github.com/anonymous/a17729b9661dd10703bd

> I browsed through FF's source code on this and felt that they seemed to have
> defined their own version of spec on File/Blob to circumvent this situation.
We
> need serious discussion with Justin if we want to do the same; changing the
spec
> isn't trivial.

When we were writing the spec, I asked Ian about the FileCallback argument and
why don't we just define a BlobCallback instead with a Blob argument?  His
answer was that File and Blob are synonymous under the IDL, so passing a Blob to
FileCallback is correct.  /me answered: ok.

So I'm not sure the spec needs changing. The trivial answer is to cast the Blob*
to File* when invoking the callback I think.

>
https://codereview.chromium.org/1257253004/diff/260001/Source/core/html/HTMLC...
> File Source/core/html/HTMLCanvasElement.cpp (right):
> 
>
https://codereview.chromium.org/1257253004/diff/260001/Source/core/html/HTMLC...
> Source/core/html/HTMLCanvasElement.cpp:554: return;
> On 2015/08/28 08:53:47, noel gordon wrote:
> > I don't think the spec allows us to just return here.  What does the spec
say
> to
> > do in this case?
> 
> Acknowledged. The spec says "If the canvas element's bitmap has no pixels
(i.e.
> either its horizontal dimension or its vertical dimension is zero) then let
> result be null.". I think I need to fix this in the next round of code review
> issue by passing the null resultBlob to the call back function.

Ok, and pretty simple to do, see
https://gist.github.com/anonymous/a17729b9661dd10703bd

>
https://codereview.chromium.org/1257253004/diff/260001/Source/core/html/HTMLC...
> File Source/core/html/HTMLCanvasElement.h (right):
> 
>
https://codereview.chromium.org/1257253004/diff/260001/Source/core/html/HTMLC...
> Source/core/html/HTMLCanvasElement.h:118: void toBlob(FileCallback* callback,
> const String& mimeType, ExceptionState& exceptionState) { return
> toBlob(callback, mimeType, ScriptValue(), exceptionState); }
> On 2015/08/28 08:53:47, noel gordon wrote:
> > nit: if you removed this line, does chrome compile?  Not sure there are any
> > callers of this second variant of toBlob (or the similar toDataURL variant
> > either). We should remove them if they are dead-code.
> 
> Haven't tried removing it and compile yet; I'll give a try later. 

Failed for me, no compile.

> But I don't think they are dead code. What if users call toBlob or toDataURL
> functions without supplying any quality arguments?

Yeap, the way the V8 bindings work today has changed since I last looked at
them, but the bindings do need them these days.

>
https://codereview.chromium.org/1257253004/diff/260001/Source/platform/graphi...
> File Source/platform/graphics/ImageBuffer.cpp (right):
> 
>
https://codereview.chromium.org/1257253004/diff/260001/Source/platform/graphi...
> Source/platform/graphics/ImageBuffer.cpp:344: bool
> ImageDataBuffer::encodeImage(const String& mimeType, const double* quality,
> Vector<char>* output) const
> On 2015/08/28 08:53:47, noel gordon wrote:
> > This function was moved to ImageDataBuffer so toBlob can call it directly,
but
> > the ASSERT that
> > 
> >   ASSERT(MIMETypeRegistry::isSupportedImageMIMETypeForEncoding(mimeType));
> > 
> > seems to be gone. The toDataURL code path has that ASSERT, but the toBlob
code
> > path does not (unless I missed it).  How come?
> 
> I believe that this ASSERT in toDataURL is redundant. I can take away that in
> future commit.

It's there to check that callers do the right thing, so I don't think it's
redundant.  I'd remove it from ImageDataBuffer::toDataURL and add it to
ImageDataBuffer::encodeImage instead.

[Noel Gordon, 2015-09-01 07:50:15]

I'm fine with the rest, thanks for your explanations.