Change code path for structured cloning ImageBitmap (issue 2039983002 by xidachen@chromium.org)

[xida...@chromium.org]

Reviewers: haraken, jbroman
CL: https://codereview.chromium.org/2039983002/

Message:
PTAL

Description:
Change code path for structured cloning ImageBitmap

This is how it works currently:
When an ImageBitmap is un-premultiplied, we read bitmap's pixel data in
premultiplied format. When de-serialize happens, we call
ImageBitmap::create(ImageData*) and in that create() function, we create
an SkImage that is in premultiplied format because the input data is
already premultiplied. This is causing the problem because the SkImage
should be in un-premultiplied format.

We change the implementation like this:
When we serialize the ImageBitmap into uint8_t*, we always read the pixel
data in un-premultiplied format, also we store the information whether
the bitmap is premultiplied or not. When de-serialize happens, we call
the ImageBitmap::create(ImageData*) function. If the bitmap is
premultiplied/un-premultiplied, we create a new ImageBitmap that is
premultiplied/un-premultiplied.

Notice that after this change, the layout tests
fast/canvas/webgl/texImage-imageBitmap-structured-clone.html
passes.

BUG=615172

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+32, -27 lines):
M third_party/WebKit/LayoutTests/TestExpectations
M third_party/WebKit/LayoutTests/fast/canvas/canvas-ImageBitmap-structured-clone.html
M third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h
M third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
M third_party/WebKit/Source/core/frame/ImageBitmap.h
M third_party/WebKit/Source/core/frame/ImageBitmap.cpp


Index: third_party/WebKit/LayoutTests/TestExpectations
diff --git a/third_party/WebKit/LayoutTests/TestExpectations b/third_party/WebKit/LayoutTests/TestExpectations
index 50d3866ae78a783055bdad6325058cfdbee2f6fa..5cfae12e01b88fc17a8fc687dfa66cc0e5564af1 100644
--- a/third_party/WebKit/LayoutTests/TestExpectations
+++ b/third_party/WebKit/LayoutTests/TestExpectations
@@ -59,10 +59,6 @@ crbug.com/593514 virtual/gpu/fast/canvas/OffscreenCanvas-2d-gradients-in-worker.
crbug.com/593514 virtual/gpu/fast/canvas/OffscreenCanvas-2d-drawImage-in-worker.html [ Failure ]
crbug.com/593514 virtual/gpu/fast/canvas/OffscreenCanvas-2d-drawImage.html [ Failure ]

-crbug.com/615172 fast/canvas/webgl/texImage-imageBitmap-structured-clone.html [ Failure ]
-crbug.com/615172 virtual/display_list_2d_canvas/fast/canvas/webgl/texImage-imageBitmap-structured-clone.html [ Failure ]
-crbug.com/615172 virtual/gpu/fast/canvas/webgl/texImage-imageBitmap-structured-clone.html [ Failure ]
-
crbug.com/417782 [ Linux Win ] virtual/rootlayerscrolls/fast/scrolling/fractional-scroll-offset-fixed-position-non-composited.html [ Failure ]
crbug.com/492664 [ Linux ] imported/csswg-test/css-writing-modes-3/box-offsets-rel-pos-vlr-005.xht [ Failure ]
crbug.com/492664 [ Linux ] imported/csswg-test/css-writing-modes-3/box-offsets-rel-pos-vrl-004.xht [ Failure ]
Index: third_party/WebKit/LayoutTests/fast/canvas/canvas-ImageBitmap-structured-clone.html
diff --git a/third_party/WebKit/LayoutTests/fast/canvas/canvas-ImageBitmap-structured-clone.html b/third_party/WebKit/LayoutTests/fast/canvas/canvas-ImageBitmap-structured-clone.html
index 38c798b0cc81bed1e2bb836199412ae9b18788be..3b77b9da84ab66f631a1fef3225a7b3f08509f83 100644
--- a/third_party/WebKit/LayoutTests/fast/canvas/canvas-ImageBitmap-structured-clone.html
+++ b/third_party/WebKit/LayoutTests/fast/canvas/canvas-ImageBitmap-structured-clone.html
@@ -38,14 +38,14 @@ function checkImageBitmap1() {
// multiplying and dividing by alpha during the round trip, but
// they should be close.
// The alpha channel should be exactly the same.
- compareImageData(newImage.data, image1.data);
+ compareImageData(newImage.data, imageData1);
}

function checkImageBitmap2() {
shouldBe("bitmapWidth", "imageWidth2");
shouldBe("bitmapHeight", "imageHeight2");

- compareImageData(newImage.data, image2.data);
+ compareImageData(newImage.data, imageData2);
}

function postToWorker(message, transferable) {
@@ -59,7 +59,7 @@ function postToWorker(message, transferable) {
});
}

-function getNewImageDataFromWorkerReply(newImageBitmap)
+function getNewImageDataFromImageBitmap(newImageBitmap)
{
bitmapWidth = newImageBitmap.width;
bitmapHeight = newImageBitmap.height;
@@ -77,6 +77,8 @@ var bitmapHeight;
var imageBitmap1;
var imageBitmap2;
var newImage;
+var imageData1;
+var imageData2;
var image1 = new ImageData(new Uint8ClampedArray(
[255, 0, 0, 255,
0, 255, 0, 255,
@@ -91,6 +93,11 @@ var context = document.getElementById("canvas").getContext("2d");
var p1 = createImageBitmap(image1).then(function(image) {imageBitmap1 = image});
var p2 = createImageBitmap(image2, {imageOrientation: "none", premultiplyAlpha: "none"}).then(function(image) {imageBitmap2 = image});
Promise.all([p1, p2]).then(function() {
+ getNewImageDataFromImageBitmap(imageBitmap1);
+ imageData1 = newImage.data;
+ getNewImageDataFromImageBitmap(imageBitmap2);
+ imageData2 = newImage.data;
+
bitmapWidth = imageBitmap1.width;
bitmapHeight = imageBitmap1.height;
shouldBe("bitmapWidth", "imageWidth1");
@@ -105,7 +112,7 @@ Promise.all([p1, p2]).then(function() {
shouldBe("bitmapHeight", "imageHeight1");

return replyPromise.then(reply => {
- getNewImageDataFromWorkerReply(reply);
+ getNewImageDataFromImageBitmap(reply);
checkImageBitmap1();
});
}).then(function() {
@@ -118,7 +125,7 @@ Promise.all([p1, p2]).then(function() {
shouldBe("bitmapHeight", "0");

return replyPromise.then(reply => {
- getNewImageDataFromWorkerReply(reply);
+ getNewImageDataFromImageBitmap(reply);
checkImageBitmap1();
});
}).then(function() {
@@ -131,7 +138,7 @@ Promise.all([p1, p2]).then(function() {
shouldBe("bitmapHeight", "imageHeight2");

return replyPromise.then(reply => {
- getNewImageDataFromWorkerReply(reply);
+ getNewImageDataFromImageBitmap(reply);
checkImageBitmap2();
});
}).then(finishJSTest, shouldNotBeCalled);
Index: third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
diff --git a/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp b/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
index f143d25720079fb6e9fbe46b92f409591d3cfd6e..0a76804c5c16a8c9852d0125fe012c9b7ab0f983 100644
--- a/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
@@ -312,10 +312,11 @@ void SerializedScriptValueWriter::writeImageData(uint32_t width, uint32_t height
doWriteImageData(width, height, pixelData, pixelDataLength);
}

-void SerializedScriptValueWriter::writeImageBitmap(uint32_t width, uint32_t height, uint32_t isOriginClean, const uint8_t* pixelData, uint32_t pixelDataLength)
+void SerializedScriptValueWriter::writeImageBitmap(uint32_t width, uint32_t height, uint32_t isOriginClean, uint32_t isPremultiplied, const uint8_t* pixelData, uint32_t pixelDataLength)
{
append(ImageBitmapTag);
append(isOriginClean);
+ append(isPremultiplied);
doWriteImageData(width, height, pixelData, pixelDataLength);
}

@@ -1145,8 +1146,8 @@ ScriptValueSerializer::StateBase* ScriptValueSerializer::writeAndGreyImageBitmap
m_writer.writeTransferredImageBitmap(index);
} else {
greyObject(object);
- std::unique_ptr<uint8_t[]> pixelData = imageBitmap->copyBitmapData(PremultiplyAlpha);
- m_writer.writeImageBitmap(imageBitmap->width(), imageBitmap->height(), static_cast<uint32_t>(imageBitmap->originClean()), pixelData.get(), imageBitmap->width() * imageBitmap->height() * 4);
+ std::unique_ptr<uint8_t[]> pixelData = imageBitmap->copyBitmapData(DontPremultiplyAlpha);
+ m_writer.writeImageBitmap(imageBitmap->width(), imageBitmap->height(), static_cast<uint32_t>(imageBitmap->originClean()), static_cast<uint32_t>(imageBitmap->isPremultiplied()), pixelData.get(), imageBitmap->width() * imageBitmap->height() * 4);
}
return nullptr;
}
@@ -1785,12 +1786,16 @@ bool SerializedScriptValueReader::readImageBitmap(v8::Local<v8::Value>* value)
uint32_t isOriginClean;
if (!doReadUint32(&isOriginClean))
return false;
+ uint32_t isPremultiplied;
+ if (!doReadUint32(&isPremultiplied))
+ return false;
ImageData* imageData = doReadImageData();
if (!imageData)
return false;
ImageBitmapOptions options;
- options.setPremultiplyAlpha("none");
- ImageBitmap* imageBitmap = ImageBitmap::create(imageData, IntRect(0, 0, imageData->width(), imageData->height()), options, true, isOriginClean);
+ if (!isPremultiplied)
+ options.setPremultiplyAlpha("none");
+ ImageBitmap* imageBitmap = ImageBitmap::create(imageData, IntRect(0, 0, imageData->width(), imageData->height()), options, isOriginClean);
if (!imageBitmap)
return false;
*value = toV8(imageBitmap, m_scriptState->context()->Global(), isolate());
Index: third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h
diff --git a/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h b/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h
index 15b23b9623ba313238597fb1125296fbac38168d..5efba487dbbd08aae70f5d783e6e43ee6e7036df 100644
--- a/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h
+++ b/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h
@@ -143,7 +143,7 @@ protected:
void writeArrayBufferView(const DOMArrayBufferView&);
void doWriteImageData(uint32_t width, uint32_t height, const uint8_t* pixelData, uint32_t pixelDataLength);
void writeImageData(uint32_t width, uint32_t height, const uint8_t* pixelData, uint32_t pixelDataLength);
- void writeImageBitmap(uint32_t width, uint32_t height, uint32_t isOriginClean, const uint8_t* pixelData, uint32_t pixelDataLength);
+ void writeImageBitmap(uint32_t width, uint32_t height, uint32_t isOriginClean, uint32_t isPremultiplied, const uint8_t* pixelData, uint32_t pixelDataLength);
void writeRegExp(v8::Local<v8::String> pattern, v8::RegExp::Flags);
void writeTransferredMessagePort(uint32_t index);
void writeTransferredArrayBuffer(uint32_t index);
Index: third_party/WebKit/Source/core/frame/ImageBitmap.cpp
diff --git a/third_party/WebKit/Source/core/frame/ImageBitmap.cpp b/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
index 7c0de1e9e10b593f496e058d70579b981bd201f8..30949c9a0f1006ae8070ca25a8b0c95b284e1563 100644
--- a/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
+++ b/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
@@ -258,8 +258,8 @@ ImageBitmap::ImageBitmap(HTMLCanvasElement* canvas, const IntRect& cropRect, con
m_image->setPremultiplied(premultiplyAlpha);
}

-// The last two parameters are used for structure-cloning.
-ImageBitmap::ImageBitmap(ImageData* data, const IntRect& cropRect, const ImageBitmapOptions& options, const bool& isImageDataPremultiplied, const bool& isImageDataOriginClean)
+// The last parameter are used for structure-cloning.
+ImageBitmap::ImageBitmap(ImageData* data, const IntRect& cropRect, const ImageBitmapOptions& options, const bool& isImageDataOriginClean)
{
bool flipY;
bool premultiplyAlpha;
@@ -273,11 +273,7 @@ ImageBitmap::ImageBitmap(ImageData* data, const IntRect& cropRect, const ImageBi
int dstHeight = cropRect.height();
// TODO (xidachen): skia doesn't support SkImage::NewRasterCopy from a kRGBA color type.
// For now, we swap R and B channel and uses kBGRA color type.
- SkImageInfo info;
- if (!isImageDataPremultiplied)
- info = SkImageInfo::Make(cropRect.width(), dstHeight, kBGRA_8888_SkColorType, kUnpremul_SkAlphaType);
- else
- info = SkImageInfo::Make(cropRect.width(), dstHeight, kBGRA_8888_SkColorType, kPremul_SkAlphaType);
+ SkImageInfo info = SkImageInfo::Make(cropRect.width(), dstHeight, kBGRA_8888_SkColorType, kUnpremul_SkAlphaType);
int srcPixelBytesPerRow = info.bytesPerPixel() * data->size().width();
int dstPixelBytesPerRow = info.bytesPerPixel() * cropRect.width();
if (cropRect == IntRect(IntPoint(), data->size())) {
@@ -340,6 +336,7 @@ ImageBitmap::ImageBitmap(ImageData* data, const IntRect& cropRect, const ImageBi
m_image = StaticBitmapImage::create(flipSkImageVertically(buffer->newSkImageSnapshot(PreferNoAcceleration, SnapshotReasonUnknown).get(), PremultiplyAlpha));
else
m_image = StaticBitmapImage::create(buffer->newSkImageSnapshot(PreferNoAcceleration, SnapshotReasonUnknown));
+ m_image->setOriginClean(isImageDataOriginClean);
}

ImageBitmap::ImageBitmap(ImageBitmap* bitmap, const IntRect& cropRect, const ImageBitmapOptions& options)
@@ -405,10 +402,10 @@ ImageBitmap* ImageBitmap::create(HTMLCanvasElement* canvas, const IntRect& cropR
return new ImageBitmap(canvas, normalizedCropRect, options);
}

-ImageBitmap* ImageBitmap::create(ImageData* data, const IntRect& cropRect, const ImageBitmapOptions& options, const bool& isImageDataPremultiplied, const bool& isImageDataOriginClean)
+ImageBitmap* ImageBitmap::create(ImageData* data, const IntRect& cropRect, const ImageBitmapOptions& options, const bool& isImageDataOriginClean)
{
IntRect normalizedCropRect = normalizeRect(cropRect);
- return new ImageBitmap(data, normalizedCropRect, options, isImageDataPremultiplied, isImageDataOriginClean);
+ return new ImageBitmap(data, normalizedCropRect, options, isImageDataOriginClean);
}

ImageBitmap* ImageBitmap::create(ImageBitmap* bitmap, const IntRect& cropRect, const ImageBitmapOptions& options)
Index: third_party/WebKit/Source/core/frame/ImageBitmap.h
diff --git a/third_party/WebKit/Source/core/frame/ImageBitmap.h b/third_party/WebKit/Source/core/frame/ImageBitmap.h
index 1e6e3db9db56924decb8f44f2124148eb065a267..3b7f1a5b33149c7b4ecc88406ed19b91cff8f493 100644
--- a/third_party/WebKit/Source/core/frame/ImageBitmap.h
+++ b/third_party/WebKit/Source/core/frame/ImageBitmap.h
@@ -37,7 +37,7 @@ public:
static ImageBitmap* create(HTMLImageElement*, const IntRect&, Document*, const ImageBitmapOptions& = ImageBitmapOptions());
static ImageBitmap* create(HTMLVideoElement*, const IntRect&, Document*, const ImageBitmapOptions& = ImageBitmapOptions());
static ImageBitmap* create(HTMLCanvasElement*, const IntRect&, const ImageBitmapOptions& = ImageBitmapOptions());
- static ImageBitmap* create(ImageData*, const IntRect&, const ImageBitmapOptions& = ImageBitmapOptions(), const bool& isImageDataPremultiplied = false, const bool& isImageDataOriginClean = true);
+ static ImageBitmap* create(ImageData*, const IntRect&, const ImageBitmapOptions& = ImageBitmapOptions(), const bool& isImageDataOriginClean = true);
static ImageBitmap* create(ImageBitmap*, const IntRect&, const ImageBitmapOptions& = ImageBitmapOptions());
static ImageBitmap* create(PassRefPtr<StaticBitmapImage>);
static ImageBitmap* create(PassRefPtr<StaticBitmapImage>, const IntRect&, const ImageBitmapOptions& = ImageBitmapOptions());
@@ -82,7 +82,7 @@ private:
ImageBitmap(HTMLImageElement*, const IntRect&, Document*, const ImageBitmapOptions&);
ImageBitmap(HTMLVideoElement*, const IntRect&, Document*, const ImageBitmapOptions&);
ImageBitmap(HTMLCanvasElement*, const IntRect&, const ImageBitmapOptions&);
- ImageBitmap(ImageData*, const IntRect&, const ImageBitmapOptions&, const bool&, const bool&);
+ ImageBitmap(ImageData*, const IntRect&, const ImageBitmapOptions&, const bool&);
ImageBitmap(ImageBitmap*, const IntRect&, const ImageBitmapOptions&);
ImageBitmap(PassRefPtr<StaticBitmapImage>);
ImageBitmap(PassRefPtr<StaticBitmapImage>, const IntRect&, const ImageBitmapOptions&);

[jbr...@chromium.org]

I'm a little confused here. Why read even premultiplied bitmaps back as
unpremultiplied?


https://codereview.chromium.org/2039983002/diff/20001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2039983002/diff/20001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode261
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:261: // The last

parameter are used for structure-cloning.

nit: "The last parameter is...", or better yet, "isImageDataOriginClean
is...".

https://codereview.chromium.org/2039983002/diff/20001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode339
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:339:
m_image->setOriginClean(isImageDataOriginClean);
Is this change related to this CL?

https://codereview.chromium.org/2039983002/diff/20001/third_party/WebKit/Source/core/frame/ImageBitmap.h
File third_party/WebKit/Source/core/frame/ImageBitmap.h (right):

https://codereview.chromium.org/2039983002/diff/20001/third_party/WebKit/Source/core/frame/ImageBitmap.h#newcode40
third_party/WebKit/Source/core/frame/ImageBitmap.h:40: static

ImageBitmap* create(ImageData*, const IntRect&, const
ImageBitmapOptions& = ImageBitmapOptions(), const bool&
isImageDataOriginClean = true);

nit: not new here, but booleans should just be passed by value; it's not
necessary to pass them by const reference (i.e. "bool
isImageDataOriginClean").

https://codereview.chromium.org/2039983002/

[xida...@chromium.org]

Using an example is probably the best way to demonstrate why we read back the
un-premultiplied data.

Say that an ImageBitmap is created from ImageData(255, 0, 0, 128). Without this
CL, what happens is this:
1. Read premultiplied data: [128, 0, 0, 128]
2. Call ImageBitmap::create() with the above data and saying that this data is
already premultiplied. The code path goes into the huge if (!premultiplyAlpha)
block at line 269, which is specially designed to handle the request that do not
apply premultiply on the input data. However, in this case, the underline
SkImage is set to be kUnpremul_SkAlphaType, and that is problematic because this
SkImage should really be kPremul_SkAlphaType.

So the solution in this CL is:
1. Read un-premultiplied data [255, 0, 0, 128]
2. Call ImageBitmap::create() saying that this data is not premultiplied and
needs to be premultiplied. The underline SkImage is kPremul_SkAlphaType.

I hope that makes sense.

https://codereview.chromium.org/2039983002/diff/20001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2039983002/diff/20001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode261
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:261: // The last
parameter are used for structure-cloning.

On 2016/06/27 19:16:16, jbroman wrote:
> nit: "The last parameter is...", or better yet,
"isImageDataOriginClean is...".

Done.

https://codereview.chromium.org/2039983002/diff/20001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode339
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:339:
m_image->setOriginClean(isImageDataOriginClean);

On 2016/06/27 19:16:16, jbroman wrote:
> Is this change related to this CL?

Without this CL, the code path for structured-cloning will never gets
here, so it is not needed to set the origin clean flag. With this CL,
structured-cloning an ImageBitmap can get here.

https://codereview.chromium.org/2039983002/diff/20001/third_party/WebKit/Source/core/frame/ImageBitmap.h
File third_party/WebKit/Source/core/frame/ImageBitmap.h (right):

https://codereview.chromium.org/2039983002/diff/20001/third_party/WebKit/Source/core/frame/ImageBitmap.h#newcode40
third_party/WebKit/Source/core/frame/ImageBitmap.h:40: static
ImageBitmap* create(ImageData*, const IntRect&, const
ImageBitmapOptions& = ImageBitmapOptions(), const bool&
isImageDataOriginClean = true);

On 2016/06/27 19:16:16, jbroman wrote:
> nit: not new here, but booleans should just be passed by value; it's
not
> necessary to pass them by const reference (i.e. "bool
isImageDataOriginClean").

Done.

https://codereview.chromium.org/2039983002/

[xida...@chromium.org]

This CL is now ready for re-review. Thanks.

https://codereview.chromium.org/2039983002/

[jbr...@chromium.org]

https://codereview.chromium.org/2039983002/diff/100001/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
File
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2039983002/diff/100001/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp#newcode1761
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1761:
if (m_position + *pixelDataLength > m_length)
nit: Not introduced in this CL, but beware of overflow here.

Something like "m_position >= m_length || m_length - m_position <
*pixelDataLength" might be safer, because it doesn't cause overflow in
the case where m_position and m_length are close are close to
std::numeric_limits<unsigned>::max().

https://codereview.chromium.org/2039983002/diff/100001/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp#newcode1799
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1799:
DCHECK(pixelData);
DCHECK isn't necessary here; the renderer will crash if the allocation
fails.

https://codereview.chromium.org/2039983002/diff/100001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2039983002/diff/100001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode262
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:262: //
isImageBitmapPremultiplied indicates whether the original ImageBitmap is
premultiplied or not.
Comments like this normally go in the header, since they describe how to
call the function correctly. I'd also specify in the comment that it
specifies whether the provided data contains premultiplied or
unpremultiplied pixel data.

https://codereview.chromium.org/2039983002/diff/100001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode267
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:267:
swizzleImageData(data.get(), height, info.bytesPerPixel() * width,
false);
What's the purpose of this swizzling? Why aren't we serializing data in
the most convenient format to read it?

https://codereview.chromium.org/2039983002/diff/100001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode269
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:269:
swizzleImageData(data.get(), height, info.bytesPerPixel() * width,
false);
Why unswizzle the data when we're about to delete it anyhow?

https://codereview.chromium.org/2039983002/diff/100001/third_party/WebKit/Source/core/frame/ImageBitmap.h
File third_party/WebKit/Source/core/frame/ImageBitmap.h (right):

https://codereview.chromium.org/2039983002/diff/100001/third_party/WebKit/Source/core/frame/ImageBitmap.h#newcode45
third_party/WebKit/Source/core/frame/ImageBitmap.h:45: static
ImageBitmap* create(std::unique_ptr<uint8_t[]>, uint32_t, uint32_t,
bool, bool);
Please name these parameters, here and below. "uint32_t" and "bool" does
not explain what these are. Omitting parameter names is okay only when
the name would be obvious (e.g. so you don't have to write
"ImageBitmapOptions imageBitmapOptions").

https://codereview.chromium.org/2039983002/

[xida...@chromium.org]

https://codereview.chromium.org/2039983002/diff/100001/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
File
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2039983002/diff/100001/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp#newcode1761
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1761:
if (m_position + *pixelDataLength > m_length)

On 2016/06/28 18:50:22, jbroman wrote:
> nit: Not introduced in this CL, but beware of overflow here.
>
> Something like "m_position >= m_length || m_length - m_position <
> *pixelDataLength" might be safer, because it doesn't cause overflow in
the case
> where m_position and m_length are close are close to
> std::numeric_limits<unsigned>::max().

Done.

https://codereview.chromium.org/2039983002/diff/100001/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp#newcode1799
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1799:
DCHECK(pixelData);

On 2016/06/28 18:50:21, jbroman wrote:
> DCHECK isn't necessary here; the renderer will crash if the allocation
fails.

Done.

https://codereview.chromium.org/2039983002/diff/100001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2039983002/diff/100001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode262
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:262: //
isImageBitmapPremultiplied indicates whether the original ImageBitmap is
premultiplied or not.

On 2016/06/28 18:50:22, jbroman wrote:
> Comments like this normally go in the header, since they describe how
to call
> the function correctly. I'd also specify in the comment that it
specifies
> whether the provided data contains premultiplied or unpremultiplied
pixel data.

Done.

https://codereview.chromium.org/2039983002/diff/100001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode267
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:267:
swizzleImageData(data.get(), height, info.bytesPerPixel() * width,
false);

On 2016/06/28 18:50:22, jbroman wrote:
> What's the purpose of this swizzling? Why aren't we serializing data
in the most
> convenient format to read it?

Done.

https://codereview.chromium.org/2039983002/diff/100001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode269
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:269:
swizzleImageData(data.get(), height, info.bytesPerPixel() * width,
false);

On 2016/06/28 18:50:22, jbroman wrote:
> Why unswizzle the data when we're about to delete it anyhow?

Done.

https://codereview.chromium.org/2039983002/diff/100001/third_party/WebKit/Source/core/frame/ImageBitmap.h
File third_party/WebKit/Source/core/frame/ImageBitmap.h (right):

https://codereview.chromium.org/2039983002/diff/100001/third_party/WebKit/Source/core/frame/ImageBitmap.h#newcode45
third_party/WebKit/Source/core/frame/ImageBitmap.h:45: static
ImageBitmap* create(std::unique_ptr<uint8_t[]>, uint32_t, uint32_t,
bool, bool);

On 2016/06/28 18:50:22, jbroman wrote:
> Please name these parameters, here and below. "uint32_t" and "bool"
does not
> explain what these are. Omitting parameter names is okay only when the
name
> would be obvious (e.g. so you don't have to write "ImageBitmapOptions
> imageBitmapOptions").

Done.

https://codereview.chromium.org/2039983002/

[jbr...@chromium.org]

https://codereview.chromium.org/2039983002/diff/120001/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
File
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2039983002/diff/120001/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp#newcode1761
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1761:
if (m_position + *pixelDataLength > m_length || m_length - m_position <
*pixelDataLength)
nit: I don't think you need to do the full check twice here, just
account for overflow.

The latter, "m_length - m_position < *pixelDataLength" is sufficient, as
long as you can rely on m_position not to be greater than m_length
(which would underflow). I think doReadUint32 promises that, so just
that condition should work. (My original suggestion also verified it,
but that's not strictly necessary).

As long as that underflow doesn't happen, this is fine and we don't need
to do the check with addition as well.

https://codereview.chromium.org/2039983002/diff/120001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2039983002/diff/120001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode263
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:263: SkImageInfo
info = SkImageInfo::Make(width, height, kBGRA_8888_SkColorType,
isImageBitmapPremultiplied ? kPremul_SkAlphaType :
kUnpremul_SkAlphaType);
Replied offline with questions about the use of kBGRA_8888_SkColorType
here.

https://codereview.chromium.org/2039983002/diff/120001/third_party/WebKit/Source/core/frame/ImageBitmap.h
File third_party/WebKit/Source/core/frame/ImageBitmap.h (right):

https://codereview.chromium.org/2039983002/diff/120001/third_party/WebKit/Source/core/frame/ImageBitmap.h#newcode60
third_party/WebKit/Source/core/frame/ImageBitmap.h:60:
std::unique_ptr<uint8_t[]> copyBitmapData(AlphaDisposition alphaOp =
DontPremultiplyAlpha, DataColorFormat format = RGBAColorType);
By the way, you could just use SkAlphaType and SkColorType if you didn't
want to add additional ones here.

https://codereview.chromium.org/2039983002/

[xida...@chromium.org]

https://codereview.chromium.org/2039983002/diff/120001/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
File
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2039983002/diff/120001/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp#newcode1761
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1761:
if (m_position + *pixelDataLength > m_length || m_length - m_position <
*pixelDataLength)

On 2016/06/29 15:10:33, jbroman wrote:
> nit: I don't think you need to do the full check twice here, just
account for
> overflow.
>
> The latter, "m_length - m_position < *pixelDataLength" is sufficient,
as long as
> you can rely on m_position not to be greater than m_length (which
would
> underflow). I think doReadUint32 promises that, so just that condition
should
> work. (My original suggestion also verified it, but that's not
strictly
> necessary).
>
> As long as that underflow doesn't happen, this is fine and we don't
need to do
> the check with addition as well.

Done.

https://codereview.chromium.org/2039983002/diff/120001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2039983002/diff/120001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode263
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:263: SkImageInfo
info = SkImageInfo::Make(width, height, kBGRA_8888_SkColorType,
isImageBitmapPremultiplied ? kPremul_SkAlphaType :
kUnpremul_SkAlphaType);

On 2016/06/29 15:10:33, jbroman wrote:
> Replied offline with questions about the use of kBGRA_8888_SkColorType
here.

Changed to kN32_SkColorType, will have another CL to change the
constructor that takes ImageData.

https://codereview.chromium.org/2039983002/

[jbr...@chromium.org]

Okay, LGTM (with comments).

Please do find time to double-check that things work correctly on Android at
some point (due to the RGBA/BGRA difference on that platform).


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

https://codereview.chromium.org/2039983002/diff/140001/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h#newcode512
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h:512:
bool doReadImageDataProperties(uint32_t*, uint32_t*, uint32_t*);
Please name parameters here as well; it's not obvious what they are.

https://codereview.chromium.org/2039983002/diff/140001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2039983002/diff/140001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode263
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:263: SkImageInfo
info = SkImageInfo::Make(width, height, kN32_SkColorType,
isImageBitmapPremultiplied ? kPremul_SkAlphaType :
kUnpremul_SkAlphaType);
super-nit: this can be written slightly shorter:

SkImageInfo info = SkImageInfo::MakeN32(width, height,
isImageBitmapPremultiplied ? kPremul_SkAlphaType :
kUnpremul_SkAlphaType);

Doesn't really matter, but there are some convenient shortcuts for N32
and N32Premul.

https://codereview.chromium.org/2039983002/diff/140001/third_party/WebKit/Source/core/frame/ImageBitmap.h
File third_party/WebKit/Source/core/frame/ImageBitmap.h (right):

https://codereview.chromium.org/2039983002/diff/140001/third_party/WebKit/Source/core/frame/ImageBitmap.h#newcode98
third_party/WebKit/Source/core/frame/ImageBitmap.h:98:
ImageBitmap(std::unique_ptr<uint8_t[]>, uint32_t, uint32_t, bool, bool);
Please give the parameters names here as well.

https://codereview.chromium.org/2039983002/

[xida...@chromium.org]

haraken@: gentle ping. Please take a look at bindings/ changes.

https://codereview.chromium.org/2039983002/

[har...@chromium.org]

On 2016/07/01 00:55:21, xidachen wrote:
> haraken@: gentle ping. Please take a look at bindings/ changes.

Sorry, I missed this one. LGTM.


https://codereview.chromium.org/2039983002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2039983002/160001


https://codereview.chromium.org/2039983002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #9 (id:160001)

https://codereview.chromium.org/2039983002/

[commit-bot@chromium.org via codereview.chromium.org]

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

https://codereview.chromium.org/2039983002/