Reland of: Make Imagebitmap Transferable

Reland of: Make Imagebitmap Transferable

This CL including a layout test that does 2 things:
1. transfer an ImageBitmap to the worker and let worker transfer back to
   main, the main thread exam the transfered-back ImageBitmap to make
   sure the data is the same as the original ImageBitmap.
2. When worker receives the ImageBitmap from main, worker calls
   createImageBitmap() to create a new ImageBitmap and send it back to
   main

BUG=532142
Committed: https://crrev.com/33126b69207406348d90d088364d66023c1d9500
Cr-Commit-Position: refs/heads/master@{#361124}

[xidachen, 2015-11-06 18:10:23]

Description was changed from

==========
Reland of: Make Imagebitmap Transferable

This CL including a layout test that does 2 things:
1. transfer an ImageBitmap to the worker and let worker transfer back to
   main, the main thread exam the transfered-back ImageBitmap to make
   sure the data is the same as the original ImageBitmap.
2. When worker receives the ImageBitmap from main, worker calls
   createImageBimmap() to create a new ImageBitmap and send it back to
   main

BUG=532142
==========

to

==========
Reland of: Make Imagebitmap Transferable

This CL including a layout test that does 2 things:
1. transfer an ImageBitmap to the worker and let worker transfer back to
   main, the main thread exam the transfered-back ImageBitmap to make
   sure the data is the same as the original ImageBitmap.
2. When worker receives the ImageBitmap from main, worker calls
   createImageBimmap() to create a new ImageBitmap and send it back to
   main

BUG=532142
==========

[xidachen, 2015-11-06 18:10:23]

xidachen@chromium.org changed reviewers:
+ chrishtr@chromium.org, haraken@chromium.org, jochen@chromium.org,
junov@chromium.org, sigbjornf@opera.com, yutak@chromium.org

[xidachen, 2015-11-06 18:12:46]

Hi,

Please review this CL. The previous version of CL is:
https://codereview.chromium.org/1409243005/

Thank you.

[jbroman, 2015-11-06 18:26:31]

Description was changed from

==========
Reland of: Make Imagebitmap Transferable

This CL including a layout test that does 2 things:
1. transfer an ImageBitmap to the worker and let worker transfer back to
   main, the main thread exam the transfered-back ImageBitmap to make
   sure the data is the same as the original ImageBitmap.
2. When worker receives the ImageBitmap from main, worker calls
   createImageBimmap() to create a new ImageBitmap and send it back to
   main

BUG=532142
==========

to

==========
Reland of: Make Imagebitmap Transferable

This CL including a layout test that does 2 things:
1. transfer an ImageBitmap to the worker and let worker transfer back to
   main, the main thread exam the transfered-back ImageBitmap to make
   sure the data is the same as the original ImageBitmap.
2. When worker receives the ImageBitmap from main, worker calls
   createImageBitmap() to create a new ImageBitmap and send it back to
   main

BUG=532142
==========

[haraken, 2015-11-07 01:13:18]

bindings/ LGTM

[jochen (gone - plz use gerrit), 2015-11-07 06:29:05]

what changed?

[xidachen, 2015-11-07 17:30:59]

On 2015/11/07 06:29:05, jochen (slow - traveling) wrote:
> what changed?

In another CL here: https://codereview.chromium.org/1413583004/, we refractor
the ImageBitmap class. Right now the ImageBitmap class hold a RefPtr<SkImage>
where it was RefPtr<Image>. We believe this change is important because
"SkImage" is thread-safe ref counted while "Image" is not.

So at the moment, after the ownership of the SkImage is transferred, the
deserializer calls ImageBitmap::create() to create and ImageBitmap from the
SkImage that is passed over. Since the SkImage is thread-safe ref counted, this
should not cause problem to oilpan.

[Justin Novosad, 2015-11-08 22:23:47]

On 2015/11/07 17:30:59, xidachen wrote:
> On 2015/11/07 06:29:05, jochen (slow - traveling) wrote:
> > what changed?
> 
> In another CL here: https://codereview.chromium.org/1413583004/, we refractor
> the ImageBitmap class. Right now the ImageBitmap class hold a RefPtr<SkImage>
> where it was RefPtr<Image>. We believe this change is important because
> "SkImage" is thread-safe ref counted while "Image" is not.
> 
> So at the moment, after the ownership of the SkImage is transferred, the
> deserializer calls ImageBitmap::create() to create and ImageBitmap from the
> SkImage that is passed over. Since the SkImage is thread-safe ref counted,
this
> should not cause problem to oilpan.

Actually, the previous version of this patch was transferring an ImageBitmap
(GarbageCollected, v8 object) across thread and v8 scope boundaries.
This patch does like we do for other transferables: pass an internal ref counted
object, and wrap in a new v8 object in the destination v8 scope, destroy the old
wrapper in the source scope. The refactor makes it simpler to do this transfer
because ImageBitmap now has a single data member that holds the entire internal
representation.

[sof, 2015-11-09 12:25:54]

https://codereview.chromium.org/1423513005/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h (right):

https://codereview.chromium.org/1423513005/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h:12: #include
"core/frame/ImageBitmap.h"
This #include isn't needed any longer?

https://codereview.chromium.org/1423513005/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):

https://codereview.chromium.org/1423513005/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:122:
contents->at(i) = imageBitmaps[i]->transfer();
Could you explain why holes are kept in the ImageBitmapContentsArray result for
duplicates?

[Justin Novosad, 2015-11-11 05:29:08]

https://codereview.chromium.org/1423513005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/1423513005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:97: return
m_image.release();
From the postMessage spec: "If any object is listed in transfer more than once,
or any of the Transferable objects listed in transfer are marked as neutered,
then throw a DataCloneError exception and abort these steps."  
I suppose handling this situation is already taken care of some place else, but
here we should at least ASSERT(!isNeutered())

https://codereview.chromium.org/1423513005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/ImageBitmap.h (right):

https://codereview.chromium.org/1423513005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/ImageBitmap.h:36: static
PassRefPtrWillBeRawPtr<ImageBitmap> create(SkImage*);
SkImage* => PassRefPtr<SkImage>

https://codereview.chromium.org/1423513005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/ImageBitmap.h:43: bool isNeutered() const {
return !m_image; }
are we 100% sure that !m_image always means that the object was neutered?

[xidachen, 2015-11-16 18:00:51]

To junov&sof:
Thanks for the comments. I have fixed all the issues.

https://codereview.chromium.org/1423513005/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h (right):

https://codereview.chromium.org/1423513005/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h:12: #include
"core/frame/ImageBitmap.h"
On 2015/11/09 12:25:54, sof wrote:
> This #include isn't needed any longer?

Done.

https://codereview.chromium.org/1423513005/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):

https://codereview.chromium.org/1423513005/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:122:
contents->at(i) = imageBitmaps[i]->transfer();
On 2015/11/09 12:25:54, sof wrote:
> Could you explain why holes are kept in the ImageBitmapContentsArray result
for
> duplicates?

Thank you for pointing it out, this is an excellent point.
My current fix is to keep a "validIdx" in contents, and remove everything after
validIdx. Please let me know if there is a better implementation.

https://codereview.chromium.org/1423513005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/1423513005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:97: return
m_image.release();
On 2015/11/11 05:29:08, Justin Novosad wrote:
> From the postMessage spec: "If any object is listed in transfer more than
once,
> or any of the Transferable objects listed in transfer are marked as neutered,
> then throw a DataCloneError exception and abort these steps."  
> I suppose handling this situation is already taken care of some place else,
but
> here we should at least ASSERT(!isNeutered())

Done.

https://codereview.chromium.org/1423513005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/ImageBitmap.h (right):

https://codereview.chromium.org/1423513005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/ImageBitmap.h:36: static
PassRefPtrWillBeRawPtr<ImageBitmap> create(SkImage*);
On 2015/11/11 05:29:08, Justin Novosad wrote:
> SkImage* => PassRefPtr<SkImage>

Done.

https://codereview.chromium.org/1423513005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/ImageBitmap.h:43: bool isNeutered() const {
return !m_image; }
On 2015/11/11 05:29:08, Justin Novosad wrote:
> are we 100% sure that !m_image always means that the object was neutered?

This is a good point. I think we should have a private member to indicate
whether it is neutered or not.

[Justin Novosad, 2015-11-16 18:11:41]

lgtm for Source/core

[sof, 2015-11-16 19:22:34]

The simplifications already made to ImageBitmap (which passed me by at the time)
are really good, btw.

https://codereview.chromium.org/1423513005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):

https://codereview.chromium.org/1423513005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:119: for
(size_t i = 0; i < imageBitmaps.size(); i++) {
The code here works fine, but I wonder if we can shorten it up a bit:

  ...
  OwnPtr<ImageBitmapContentsArray> contents = adoptPtr(new
ImageBitmapContentsArray);
  for (ImageBitmap* imageBitmap : imageBitmaps) {
      if (visited.contains(imageBitmap))
          continue;
      visited.add(imageBitmap);
      contents->append(imageBitmap->transfer());
  }
  return contents.release();

i.e., I don't think it is worth being exactingly precise about pre-sizing
'contents', nor shrink its backing buffer to exactly fit afterwards.

[xidachen, 2015-11-16 20:15:51]

https://codereview.chromium.org/1423513005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):

https://codereview.chromium.org/1423513005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:119: for
(size_t i = 0; i < imageBitmaps.size(); i++) {
Thank you for the comments. I think you made a good point because the number of
elements in the vector is likely small in most cases, so it is not worth to
pre-allocate and then shrink.

[sof, 2015-11-16 20:56:17]

lgtm; haraken@ may want to have a final bindings/ look?

https://codereview.chromium.org/1423513005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/ImageBitmap.h (right):

https://codereview.chromium.org/1423513005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.h:72: bool m_isNeutered;
nit: if you provide the initial value here (bool m_isNeutered = false;), you
wouldn't have to initialize it in every constructor.

[haraken, 2015-11-16 23:40:44]

https://codereview.chromium.org/1423513005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h (right):

https://codereview.chromium.org/1423513005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h:12: #include
"third_party/skia/include/core/SkImage.h"

Do we want to allow this include? We've never included third_party stuff in
bindings/.

https://codereview.chromium.org/1423513005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h (right):

https://codereview.chromium.org/1423513005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:36: #include
"third_party/skia/include/core/SkImage.h"

Ditto.

[xidachen, 2015-11-17 15:24:27]

https://codereview.chromium.org/1423513005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h (right):

https://codereview.chromium.org/1423513005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h:12: #include
"third_party/skia/include/core/SkImage.h"
On 2015/11/16 23:40:44, haraken wrote:
> 
> Do we want to allow this include? We've never included third_party stuff in
> bindings/.

Done! It is now using forward declaration instead.

https://codereview.chromium.org/1423513005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h (right):

https://codereview.chromium.org/1423513005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:36: #include
"third_party/skia/include/core/SkImage.h"
On 2015/11/16 23:40:44, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/1423513005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/ImageBitmap.h (right):

https://codereview.chromium.org/1423513005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.h:72: bool m_isNeutered;
On 2015/11/16 20:56:17, sof wrote:
> nit: if you provide the initial value here (bool m_isNeutered = false;), you
> wouldn't have to initialize it in every constructor.

Done.

[Justin Novosad, 2015-11-17 15:31:09]

On 2015/11/17 15:24:27, xidachen wrote:
>
https://codereview.chromium.org/1423513005/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h
(right):
> 
>
https://codereview.chromium.org/1423513005/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h:12:
#include
> "third_party/skia/include/core/SkImage.h"
> On 2015/11/16 23:40:44, haraken wrote:
> > 
> > Do we want to allow this include? We've never included third_party stuff in
> > bindings/.
> 
> Done! It is now using forward declaration instead.

LOL. The fact that this even compiles means ScriptValueSerializer.cpp is already
(indirectly) including SkImage.h
@haraken: are you okay with using skia types directly here? If your not ready to
drink the skia koolaid there is a wrapper we can use:
platform/graphics/StaticBitmapImage.h

[haraken, 2015-11-17 23:27:51]

On 2015/11/17 15:31:09, Justin Novosad wrote:
> On 2015/11/17 15:24:27, xidachen wrote:
> >
>
https://codereview.chromium.org/1423513005/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h
> (right):
> > 
> >
>
https://codereview.chromium.org/1423513005/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h:12:
> #include
> > "third_party/skia/include/core/SkImage.h"
> > On 2015/11/16 23:40:44, haraken wrote:
> > > 
> > > Do we want to allow this include? We've never included third_party stuff
in
> > > bindings/.
> > 
> > Done! It is now using forward declaration instead.
> 
> LOL. The fact that this even compiles means ScriptValueSerializer.cpp is
already
> (indirectly) including SkImage.h
> @haraken: are you okay with using skia types directly here? If your not ready
to
> drink the skia koolaid there is a wrapper we can use:
> platform/graphics/StaticBitmapImage.h

Yeah, I prefer using platform/graphics/StaticBitmapImage.h.

It's unfortunate that bindings/ is already indirectly including SkImage.h, but I
don't want to increase the problem.

[xidachen, 2015-11-23 03:23:25]

haraken@: Could you take a look at this patch? I have changed code under
bindings/ so that StaticBitmapImage is used instead of SkImage.
Also, please take a look at Source/Web/ where only one file is changed.

junov@: Please take a look at Source/core/

Thank you so much.

[haraken, 2015-11-23 03:48:27]

bindings/ LGTM

[Justin Novosad, 2015-11-23 15:01:36]

On 2015/11/23 03:48:27, haraken wrote:
> bindings/ LGTM

lgtm for core/

[xidachen, 2015-11-23 15:04:13]

The CQ bit was checked by xidachen@chromium.org

[xidachen, 2015-11-23 15:04:14]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/1423513005/#ps120001
(title: "should-work code")

[commit-bot: I haz the power, 2015-11-23 15:04:22]

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

[commit-bot: I haz the power, 2015-11-23 16:41:05]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-23 16:41:48]

Patchset 7 (id:??) landed as
https://crrev.com/33126b69207406348d90d088364d66023c1d9500
Cr-Commit-Position: refs/heads/master@{#361124}