Implement Asynchronous image encoding for Canvas.toBlob

Implement Asynchronous image encoding for Canvas.toBlob

Main thread creates a new thread to carry out the computation-intensive asynchronous image encoding
operation. Afterwards the encoded image is passed back to the main thread to create blob and invoke user
callback.

Empty blob is passed to user callback in the senarios when the canvas has zero pixels or when the image
encoding fails.

This code review issue continues from the previous issue at https://codereview.chromium.org/1331443009/
before the blink and chromium are merged.

BUG=523414
BUG=67587
Committed: https://crrev.com/9dcdf861954dd8294faf3eefde89ea228f99c322
Cr-Commit-Position: refs/heads/master@{#350910}

[xlai (Olivia), 2015-09-24 17:00:35]

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

[Justin Novosad, 2015-09-24 20:51:31]

https://codereview.chromium.org/1355333005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/1355333005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.h:83: static
PassOwnPtr<WebThread> getToBlobThreadInstance();
This is not a proper use of PassOwnPtr. I really think that wrapping the thread
ownership in a singleton would be the way to go here. I looked through blink
code, and I found one really clean example of a singleton that owns a thread,
with explicit initialization and teardown, which is nice because it means we are
not even leaking the singleton. Checkout CompositorWorkerManager. In your case,
you are not in a module, so your global init/uninit would go in WebKit.cpp

[xlai (Olivia), 2015-09-25 16:40:18]

This time, I use DEFINE_STATIC_LOCAL for the static variable s_toBlobThread.
There is no need to use an explicit delete on s_toBlobThread because it is
declared as an OwnPtr<WebThread> object, which will be memory-freed at the end
of program (that's the purpose of DEFINE_STATIC_LOCAL).

Other than that change, I've also changed the race layout test according to last
code review.

[Justin Novosad, 2015-09-25 16:47:34]

https://codereview.chromium.org/1355333005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1355333005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:142: return
s_toBlobThread;
Returning an "OwnPtr<WebThread>&" is not appropriate. The return type should be
WebThread* and it should return s_toBlobThread.get(). Otherwise, this looks good
to me.

[Justin Novosad, 2015-09-25 16:49:25]

Why is there a
third_party/WebKit/LayoutTests/platform/linux/fast/canvas/canvas-toBlob-toDataURL-race-imageEncoder-png-expected.png
?

[xlai (Olivia), 2015-09-25 18:46:33]

removed the expected.png file and changed the code accordingly.

[Justin Novosad, 2015-09-25 18:57:10]

lgtm with nit

https://codereview.chromium.org/1355333005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1355333005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:570:
Platform::current()->mainThread()->taskRunner()->postTask(FROM_HERE,
threadSafeBind(&HTMLCanvasElement::createBlobAndCall,
AllowCrossThreadAccess(this), encodedImage.release(), mimeType,
AllowCrossThreadAccess(callback)));
Passing this to the thread is still unsafe. If you want to fix it in a follow-up
patch, you should file a crbug for it and add a TODO comment here that
references the bug. Also, you should mark the bug as blocking the main feature.

[xlai (Olivia), 2015-09-25 19:15:37]

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

[commit-bot: I haz the power, 2015-09-25 19:16:40]

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

[commit-bot: I haz the power, 2015-09-25 20:54:01]

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

[commit-bot: I haz the power, 2015-09-25 20:54:02]

Dry run: This issue passed the CQ dry run.

[xlai (Olivia), 2015-09-25 20:54:29]

The CQ bit was checked by xlai@chromium.org

[xlai (Olivia), 2015-09-25 20:54:29]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org
Link to the patchset: https://codereview.chromium.org/1355333005/#ps80001
(title: "Change async func to static so no need worry if canvas instance does
not live long enough")

[commit-bot: I haz the power, 2015-09-25 20:55:33]

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

[commit-bot: I haz the power, 2015-09-25 21:05:34]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-09-25 21:06:37]

Patchset 5 (id:??) landed as
https://crrev.com/9dcdf861954dd8294faf3eefde89ea228f99c322
Cr-Commit-Position: refs/heads/master@{#350910}

[yosin_UTC9, 2015-12-03 02:50:41]

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

[yosin_UTC9, 2015-12-03 02:50:41]

https://codereview.chromium.org/1355333005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1355333005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:534: String
HTMLCanvasElement::toDataURLInternal(const String& mimeType, const double&
quality, SourceDrawingBuffer sourceBuffer) const
Just curiosity, why don't you use |double quality|?

[Justin Novosad, 2015-12-03 03:20:16]

On 2015/12/03 02:50:41, Yosi_back_on_Jan_5 wrote:
>
https://codereview.chromium.org/1355333005/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):
> 
>
https://codereview.chromium.org/1355333005/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:534: String
> HTMLCanvasElement::toDataURLInternal(const String& mimeType, const double&
> quality, SourceDrawingBuffer sourceBuffer) const
> Just curiosity, why don't you use |double quality|?

Ah, I missed it.