Implement ImageBitmapOptions resize

Implement ImageBitmapOptions resize

In this CL, we add the options resizeWidth && resizeHeight &&
resizeQuality to the ImageBitmapOptions. All these options are hidding
behind the experimtanl flag.

Also, this CL implements this options for creating an ImageBitmap from
HTMLImageElement, HTMLCanvasElement, ImageBitmap and Blob. For creating
an ImageBitmap from ImageData and HTMLVideoElement is a bit more
complicated and will finish that in a separate CL.

This CL also adds a layout test to ensure the correctness of the behavior.

TBR=junov@chromium.org
BUG=627855
Committed: https://crrev.com/96e0583198a59d305f8442704e35d7d307120cec
Cr-Commit-Position: refs/heads/master@{#405314}

[xidachen, 2016-06-29 20:55:19]

xidachen@chromium.org changed reviewers:
+ jbroman@chromium.org

[xidachen, 2016-06-29 20:55:20]

PTAL

[jbroman, 2016-06-29 21:14:42]

https://codereview.chromium.org/2035113002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/SlowTests (right):

https://codereview.chromium.org/2035113002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/SlowTests:61: crbug.com/24182
virtual/gpu/fast/canvas/canvas-createImageBitmap-resize.html [ Slow ]
By the way, do you happen to know why these tests are slow (despite being in
"fast" :P)? Bug doesn't specify.

https://codereview.chromium.org/2035113002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2035113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:194: RefPtr<SkImage>
scaledImage = fromSkSp(SkImage::MakeFromRaster(scaledPixmap, [](const void*
pixels, void*)
Doing a special pixmap allocation here seems slightly weird, given that we don't
particularly need to control the allocation. Why not use
SkSurface::NewRasterN32, drawBitmapRect into that surface, and then taking a
snapshot from that? It seems equivalent (this is roughly what scalePixels seems
to do internally), but doesn't make this code manage the allocation as much.

(Disregard if Skia people have told you not to do that, obviously.)

(There's also skia::ImageOperations::Resize, but I think new uses of it are
mildly discouraged.)

https://codereview.chromium.org/2035113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:424: parseOptions(options,
flipY, premultiplyAlpha, resizeWidth, resizeHeight, resizeQuality);
This is getting to be quite a few options; have you considered returning a
struct of resolved options instead of a long parameter list?

https://codereview.chromium.org/2035113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:428: if
(options.hasResizeWidth() && options.hasResizeHeight()
nit: It seems a little odd to duplicate the "should we resize" condition here.
Should the parseOptions thing be deciding that and producing a boolean?

https://codereview.chromium.org/2035113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:430:
scaleCropRect(cropRect, input->width(), input->height(), resizeWidth,
resizeHeight);
A lot of this logic is duplicated with the above function; can this be
refactored to duplicate less code?

https://codereview.chromium.org/2035113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:581: if
(options.hasResizeWidth() && options.hasResizeHeight()) {
What if one is provided but not the other? Your spec change seems vague on this
point.

I could imagine a couple behaviors that might be reasonable:
- scale proportionally, like <img> does
- use the original size in the unspecified dimension
- throw an exception

"Silently ignore" seems unlikely to be the right option here. WDYT?

[xidachen, 2016-06-30 15:23:12]

https://codereview.chromium.org/2035113002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/SlowTests (right):

https://codereview.chromium.org/2035113002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/SlowTests:61: crbug.com/24182
virtual/gpu/fast/canvas/canvas-createImageBitmap-resize.html [ Slow ]
On 2016/06/29 21:14:41, jbroman wrote:
> By the way, do you happen to know why these tests are slow (despite being in
> "fast" :P)? Bug doesn't specify.

I made a guess from one line above this. The real reason is that I lumped many
things in this test. Basically I do createImageBitmap from multiple sources,
creates 8 bitmaps and do test on those 8 bitmaps. The tests include comparing
the color of some bitmaps which is quite computational intensive.

https://codereview.chromium.org/2035113002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2035113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:194: RefPtr<SkImage>
scaledImage = fromSkSp(SkImage::MakeFromRaster(scaledPixmap, [](const void*
pixels, void*)
On 2016/06/29 21:14:41, jbroman wrote:
> Doing a special pixmap allocation here seems slightly weird, given that we
don't
> particularly need to control the allocation. Why not use
> SkSurface::NewRasterN32, drawBitmapRect into that surface, and then taking a
> snapshot from that? It seems equivalent (this is roughly what scalePixels
seems
> to do internally), but doesn't make this code manage the allocation as much.
> 
> (Disregard if Skia people have told you not to do that, obviously.)
> 
> (There's also skia::ImageOperations::Resize, but I think new uses of it are
> mildly discouraged.)

Done.

https://codereview.chromium.org/2035113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:424: parseOptions(options,
flipY, premultiplyAlpha, resizeWidth, resizeHeight, resizeQuality);
On 2016/06/29 21:14:42, jbroman wrote:
> This is getting to be quite a few options; have you considered returning a
> struct of resolved options instead of a long parameter list?

Done.

https://codereview.chromium.org/2035113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:428: if
(options.hasResizeWidth() && options.hasResizeHeight()
On 2016/06/29 21:14:41, jbroman wrote:
> nit: It seems a little odd to duplicate the "should we resize" condition here.
> Should the parseOptions thing be deciding that and producing a boolean?

Done.

https://codereview.chromium.org/2035113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:430:
scaleCropRect(cropRect, input->width(), input->height(), resizeWidth,
resizeHeight);
On 2016/06/29 21:14:41, jbroman wrote:
> A lot of this logic is duplicated with the above function; can this be
> refactored to duplicate less code?

Done.

https://codereview.chromium.org/2035113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:581: if
(options.hasResizeWidth() && options.hasResizeHeight()) {
On 2016/06/29 21:14:41, jbroman wrote:
> What if one is provided but not the other? Your spec change seems vague on
this
> point.
> 
> I could imagine a couple behaviors that might be reasonable:
> - scale proportionally, like <img> does
> - use the original size in the unspecified dimension
> - throw an exception
> 
> "Silently ignore" seems unlikely to be the right option here. WDYT?

Changed to scale proportionally, will have a pull request to change the spec.

[jbroman, 2016-06-30 18:52:35]

lgtm

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

https://codereview.chromium.org/2035113002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:189:
cropRect.setWidth(resizeWidth);
Hmm. It seems you're setting parameters to use for cropImage, which will then
turn around and use SkCanvas::drawImage again. Should this be done in one step?

https://codereview.chromium.org/2035113002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:199:
surface->getCanvas()->drawImageRect(skiaImage.get(), srcRect, dstRect, &paint);
You should also clear the canvas to transparent before drawing the image.

https://codereview.chromium.org/2035113002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:366: if
(parsedOptions.shouldScaleInput)
Could the next few lines be bundles into function call that takes "const
ParsedOptions&" and applies the cropping, premultiplication, etc. that are
specified in those options all at once?

https://codereview.chromium.org/2035113002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:537:
parsedOptions.resizeWidth = options.resizeWidth();
What happens if one or both of these is specified, but is zero? Or if one of the
calculations below results in zero after truncation?

https://codereview.chromium.org/2035113002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:541:
parsedOptions.resizeHeight =
static_cast<unsigned>(static_cast<float>(options.resizeWidth()) / width *
height);
What should be happening when the computed size based on the aspect ratio isn't
an integer?

Here you're truncating (which could result in zero). Should it be rounding
(possibly with a min of 1)? Another option (which I might lean against given
that only integers can be directly specified, but still) would be to allow the
resize dimensions to be floats, and have the resulting image have an AA
partially transparent bottom/right edge.

Has this been decided?

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

https://codereview.chromium.org/2035113002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.h:35: bool flipY;
nit: for predictability, mind providing default values for these?

"bool flipY = false;" etc?

Else if one isn't initialized, some unpredictable value will be observed
instead.

[jbroman, 2016-06-30 18:52:55]

Whoops sorry, not lgtm yet. (I hit the wrong button.)

[xidachen, 2016-07-07 13:48:47]

Now that the spec is clear, this patch is ready for a re-review.

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

https://codereview.chromium.org/2035113002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:189:
cropRect.setWidth(resizeWidth);
On 2016/06/30 18:52:34, jbroman wrote:
> Hmm. It seems you're setting parameters to use for cropImage, which will then
> turn around and use SkCanvas::drawImage again. Should this be done in one
step?

Done. The scale && crop are in cropImage(), and it is done in one pass.

https://codereview.chromium.org/2035113002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:199:
surface->getCanvas()->drawImageRect(skiaImage.get(), srcRect, dstRect, &paint);
On 2016/06/30 18:52:34, jbroman wrote:
> You should also clear the canvas to transparent before drawing the image.

This is not needed as SkSurface::MakeRaster() does the clear.

https://codereview.chromium.org/2035113002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:366: if
(parsedOptions.shouldScaleInput)
On 2016/06/30 18:52:34, jbroman wrote:
> Could the next few lines be bundles into function call that takes "const
> ParsedOptions&" and applies the cropping, premultiplication, etc. that are
> specified in those options all at once?

Done.

https://codereview.chromium.org/2035113002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:537:
parsedOptions.resizeWidth = options.resizeWidth();
On 2016/06/30 18:52:34, jbroman wrote:
> What happens if one or both of these is specified, but is zero? Or if one of
the
> calculations below results in zero after truncation?

Spec has been changed, now an exception is thrown when one or both of them is <=
0.

https://codereview.chromium.org/2035113002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:541:
parsedOptions.resizeHeight =
static_cast<unsigned>(static_cast<float>(options.resizeWidth()) / width *
height);
On 2016/06/30 18:52:34, jbroman wrote:
> What should be happening when the computed size based on the aspect ratio
isn't
> an integer?
> 
> Here you're truncating (which could result in zero). Should it be rounding
> (possibly with a min of 1)? Another option (which I might lean against given
> that only integers can be directly specified, but still) would be to allow the
> resize dimensions to be floats, and have the resulting image have an AA
> partially transparent bottom/right edge.
> 
> Has this been decided?

Spec has been changed, so we round up to its nearest integer after scaling.

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

https://codereview.chromium.org/2035113002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.h:35: bool flipY;
On 2016/06/30 18:52:34, jbroman wrote:
> nit: for predictability, mind providing default values for these?
> 
> "bool flipY = false;" etc?
> 
> Else if one isn't initialized, some unpredictable value will be observed
> instead.

Done.

[jbroman, 2016-07-07 19:18:00]

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-resize.html
(right):

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-resize.html:73:
assert_equals(dataMatched, isTheSame, "These two bitmaps should be different.");
What's the purpose of this "isTheSame" parameter, which always seems to be
false?
Why not just assert_false?

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-resize.html:87:
return Promise.all([p1, p2, p3, p4, p5, p6, p7, p8]).then(function() {
super-nit: it's a little funny to have these push their values into a variable
from the surrounding scope, when promises already have a result. WDYT of:

return Promise.all([
    createImageBitmap(source, {resizeWidth: 40, resizeHeight: 40, resizeQuality:
"high"}),
    createImageBitmap(source, {resizeWidth: 40, resizeHeight: 40, resizeQuality:
"medium"}),
    createImageBitmap(source, {resizeWidth: 40, resizeHeight: 40, resizeQuality:
"low"}),
    ...
]).then(([noCropHigh, noCropMedium, noCropLow, ...]) => {
    checkNoCrop(noCropHigh);
    ...
});

Up to you; this is a fairly personal-style sort of thing.

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-resize.html:96:
// Brute-force comparison among all bitmaps is too expense
nit: spelling: "expensive"

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-resize.html:102:
failed(ex);
Is this necessary? promise_test already fails the test on an uncaught promise
rejection

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-resize.html:106:
function initializeTestCanvas(testCanvas)
nit: every call site makes a canvas and then passes it to here. Why not move the
createElement call into this function, and have it return the testCanvas
instead?

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-resize.html:130:
resolve(testImageBitmap(blob));
nit: exceptions thrown in testImageBitmap won't be captured by the promise this
way; suggest making this promise just resolve to the XHR response, and having
testImageBitmap happen afterward, like:

    xhr.onload = function() { resolve(xhr.response); };
}).then(testImageBitmap);

This way the initial promise just deals with the XHR.

By the way, there's a more modern XHR that already uses promises, fetch. If you
used that instead, it would look like this:

return fetch("resources/pattern.png").then(response =>
response.blob()).then(testImageBitmap);

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-resize.html:137:
return new Promise((resolve, reject) => {
testImageBitmap already has all of the asynchronousness here; you don't have to
build another promise (it doesn't break things, but it's longer).

        var testCanvas = document.createElement("canvas");
        initializeTestCanvas(testCanvas);
        return testImageBitmap(testCanvas);

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-resize.html:152:
});
nit: Similar advice to XHR above. It's not a big deal in a layout test (because
it'll fail anyways), but it's ideal to capture exceptions by having them run in
the promise code, rather than in some other callback. If you just have the
onload resolution to "resolve(image)", you can use ".then(testImageBitmap)" at
the end to do the test in a way that exceptions will be captured by the promise.

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-resize.html:161:
resolve(testImageBitmap(bitmap));
Here the asynchronousness already uses promises, so the Promise constructor is
unnecessary here.

promise_test(function() {
    var testCanvas = document.createElement("canvas");
    initializeTestCanvas(testCanvas);
    return createImageBitmap(testCanvas).then(testImageBitmap);
});

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

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:120: static
PassRefPtr<StaticBitmapImage> cropImage(Image* image, IntRect& cropRect, const
ParsedOptions& parsedOptions, AlphaDisposition imageFormat = PremultiplyAlpha,
ImageDecoder::GammaAndColorProfileOption colorSpaceOp =
ImageDecoder::GammaAndColorProfileApplied)
Can you point me at why cropRect is now an in-out parameter? These are pretty
confusing and I'd rather it stayed as a const IntRect& if possible.

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:125: if
(parsedOptions.shouldScaleInput && cropRect.x() == 0 && cropRect.y() == 0 &&
cropRect.width() == image->width() && cropRect.height() == image->height()) {
What if the cropRect _is_ specified in the API call, but is specified to be
equal to (0, 0) width x height?

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:522: ParsedOptions
ImageBitmap::parseOptions(const ImageBitmapOptions& options, const int width,
const int height)
width and height of what?

Suggest something like "sourceWidth" and "sourceHeight", or possibly a single
IntSize named "sourceSize".

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

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.h:38: struct ParsedOptions {
This struct (and the method that makes it) seem private to ImageBitmap.cpp. You
can move them there (in an anonymous namespace) to simplify this header.

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.h:109: ParsedOptions
parseOptions(const ImageBitmapOptions&, const int, const int);
Please name parameters whose use is not obvious, like these "const int"s.

Please also write "int" in the declarations; adding constness to a value
parameter here does not constrain the definition and just makes the declaration
longer.

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp
(right):

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp:91: if
((options.hasResizeWidth() && options.resizeWidth() <= 0) ||
(options.hasResizeHeight() && options.resizeHeight() <= 0)) {
Is checking for negative values necessary? I think [EnforceRange] already does
this, so you should only need to handle the zero case. For user-friendliness,
I'd also suggest specifying which turned out to be zero.

Also, this is repeated several times; could it be moved somewhere else (e.g.
parseOptions)?

[xidachen, 2016-07-07 20:43:30]

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-resize.html
(right):

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-resize.html:73:
assert_equals(dataMatched, isTheSame, "These two bitmaps should be different.");
On 2016/07/07 19:17:59, jbroman wrote:
> What's the purpose of this "isTheSame" parameter, which always seems to be
> false?
> Why not just assert_false?

Done.

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-resize.html:87:
return Promise.all([p1, p2, p3, p4, p5, p6, p7, p8]).then(function() {
On 2016/07/07 19:17:59, jbroman wrote:
> super-nit: it's a little funny to have these push their values into a variable
> from the surrounding scope, when promises already have a result. WDYT of:
> 
> return Promise.all([
>     createImageBitmap(source, {resizeWidth: 40, resizeHeight: 40,
resizeQuality:
> "high"}),
>     createImageBitmap(source, {resizeWidth: 40, resizeHeight: 40,
resizeQuality:
> "medium"}),
>     createImageBitmap(source, {resizeWidth: 40, resizeHeight: 40,
resizeQuality:
> "low"}),
>     ...
> ]).then(([noCropHigh, noCropMedium, noCropLow, ...]) => {
>     checkNoCrop(noCropHigh);
>     ...
> });
> 
> Up to you; this is a fairly personal-style sort of thing.

Done.

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-resize.html:96:
// Brute-force comparison among all bitmaps is too expense
On 2016/07/07 19:18:00, jbroman wrote:
> nit: spelling: "expensive"

Done.

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-resize.html:102:
failed(ex);
On 2016/07/07 19:17:59, jbroman wrote:
> Is this necessary? promise_test already fails the test on an uncaught promise
> rejection

Done.

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-resize.html:106:
function initializeTestCanvas(testCanvas)
On 2016/07/07 19:17:59, jbroman wrote:
> nit: every call site makes a canvas and then passes it to here. Why not move
the
> createElement call into this function, and have it return the testCanvas
> instead?

Done.

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-resize.html:130:
resolve(testImageBitmap(blob));
On 2016/07/07 19:17:59, jbroman wrote:
> nit: exceptions thrown in testImageBitmap won't be captured by the promise
this
> way; suggest making this promise just resolve to the XHR response, and having
> testImageBitmap happen afterward, like:
> 
>     xhr.onload = function() { resolve(xhr.response); };
> }).then(testImageBitmap);
> 
> This way the initial promise just deals with the XHR.
> 
> By the way, there's a more modern XHR that already uses promises, fetch. If
you
> used that instead, it would look like this:
> 
> return fetch("resources/pattern.png").then(response =>
> response.blob()).then(testImageBitmap);

Done.

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-resize.html:137:
return new Promise((resolve, reject) => {
On 2016/07/07 19:18:00, jbroman wrote:
> testImageBitmap already has all of the asynchronousness here; you don't have
to
> build another promise (it doesn't break things, but it's longer).
> 
>         var testCanvas = document.createElement("canvas");
>         initializeTestCanvas(testCanvas);
>         return testImageBitmap(testCanvas);

Done.

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-resize.html:152:
});
On 2016/07/07 19:17:59, jbroman wrote:
> nit: Similar advice to XHR above. It's not a big deal in a layout test
(because
> it'll fail anyways), but it's ideal to capture exceptions by having them run
in
> the promise code, rather than in some other callback. If you just have the
> onload resolution to "resolve(image)", you can use ".then(testImageBitmap)" at
> the end to do the test in a way that exceptions will be captured by the
promise.

Done.

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-resize.html:161:
resolve(testImageBitmap(bitmap));
On 2016/07/07 19:17:59, jbroman wrote:
> Here the asynchronousness already uses promises, so the Promise constructor is
> unnecessary here.
> 
> promise_test(function() {
>     var testCanvas = document.createElement("canvas");
>     initializeTestCanvas(testCanvas);
>     return createImageBitmap(testCanvas).then(testImageBitmap);
> });

Done.

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

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:120: static
PassRefPtr<StaticBitmapImage> cropImage(Image* image, IntRect& cropRect, const
ParsedOptions& parsedOptions, AlphaDisposition imageFormat = PremultiplyAlpha,
ImageDecoder::GammaAndColorProfileOption colorSpaceOp =
ImageDecoder::GammaAndColorProfileApplied)
On 2016/07/07 19:18:00, jbroman wrote:
> Can you point me at why cropRect is now an in-out parameter? These are pretty
> confusing and I'd rather it stayed as a const IntRect& if possible.

Changed to pass by value.

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:125: if
(parsedOptions.shouldScaleInput && cropRect.x() == 0 && cropRect.y() == 0 &&
cropRect.width() == image->width() && cropRect.height() == image->height()) {
On 2016/07/07 19:18:00, jbroman wrote:
> What if the cropRect _is_ specified in the API call, but is specified to be
> equal to (0, 0) width x height?

This cannot happen. If that is the case, then in the API call, the sw or sh
parameter is 0, then an exception will be thrown.

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:522: ParsedOptions
ImageBitmap::parseOptions(const ImageBitmapOptions& options, const int width,
const int height)
On 2016/07/07 19:18:00, jbroman wrote:
> width and height of what?
> 
> Suggest something like "sourceWidth" and "sourceHeight", or possibly a single
> IntSize named "sourceSize".

Done.

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

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.h:38: struct ParsedOptions {
On 2016/07/07 19:18:00, jbroman wrote:
> This struct (and the method that makes it) seem private to ImageBitmap.cpp.
You
> can move them there (in an anonymous namespace) to simplify this header.

Done.

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.h:109: ParsedOptions
parseOptions(const ImageBitmapOptions&, const int, const int);
On 2016/07/07 19:18:00, jbroman wrote:
> Please name parameters whose use is not obvious, like these "const int"s.
> 
> Please also write "int" in the declarations; adding constness to a value
> parameter here does not constrain the definition and just makes the
declaration
> longer.

This line is removed because this function is now in an anonymous namespace.

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp
(right):

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp:91: if
((options.hasResizeWidth() && options.resizeWidth() <= 0) ||
(options.hasResizeHeight() && options.resizeHeight() <= 0)) {
On 2016/07/07 19:18:00, jbroman wrote:
> Is checking for negative values necessary? I think [EnforceRange] already does
> this, so you should only need to handle the zero case. For user-friendliness,
> I'd also suggest specifying which turned out to be zero.
> 
> Also, this is repeated several times; could it be moved somewhere else (e.g.
> parseOptions)?

Done. Now it is a static method in ImageBitmap called isResizeOptionValid and
applies to all call sites. Also added another similar one called isSWSHValid()
which checks sw and sh arguments in the API calls.

[jbroman, 2016-07-07 21:21:46]

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

https://codereview.chromium.org/2035113002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:125: if
(parsedOptions.shouldScaleInput && cropRect.x() == 0 && cropRect.y() == 0 &&
cropRect.width() == image->width() && cropRect.height() == image->height()) {
On 2016/07/07 at 20:43:29, xidachen wrote:
> On 2016/07/07 19:18:00, jbroman wrote:
> > What if the cropRect _is_ specified in the API call, but is specified to be
> > equal to (0, 0) width x height?
> 
> This cannot happen. If that is the case, then in the API call, the sw or sh
parameter is 0, then an exception will be thrown.

I don't mean if sw or sh is zero, but if they match the image dimensions.

Consider these two cases:

// suppose source is 100x100
createImageBitmap(source, 0, 0, 100, 100, { resizeWidth: 200, resizeHeight: 200
});
createImageBitmap(source, { resizeWidth: 200, resizeHeight: 200 });

The comment here claims that the cropRect matching the image dimensions implies
that cropRect wasn't specified, but here I have an example where the source rect
was specified, but happens to be equal to (0, 0, image->width(),
image->height()).

https://codereview.chromium.org/2035113002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-resize.html
(right):

https://codereview.chromium.org/2035113002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-resize.html:78:
var imageBitmaps = {};
nit: this variable is now unused

https://codereview.chromium.org/2035113002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2035113002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:21: namespace {
nit: move this anonymous namespace inside the blink namespace; then you won't
need blink:: qualification on things.

https://codereview.chromium.org/2035113002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:174: bool
ImageBitmap::isResizeOptionValid(const ImageBitmapOptions& options,
ExceptionState& exceptionState)
nit: prefer static functions, or functions in anonymous namespaces, to private
functions if they don't need special access (e.g. to member variables, to
friends, etc.). That way they don't need to be declared in the header.

https://codereview.chromium.org/2035113002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:177:
exceptionState.throwDOMException(InvalidStateError, "The resizeWidth or/and
resizeHeight is less than or equals to 0.");
nit: For developer clarity, please specify which was the issue. Since this
message won't be issued for the < case (I'm assuming -- covering this in layout
tests isn't a bad idea), please say that it is zero rather than <= 0.

https://codereview.chromium.org/2035113002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:183: bool
ImageBitmap::isSWSHValid(int sw, int sh, ExceptionState& exceptionState)
nit: What's a SWSH? Prefer more descriptive names, like "isSourceSizeValid",
"sourceWidth", "sourceHeight", etc.

https://codereview.chromium.org/2035113002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:331:
ImageBitmap::ImageBitmap(HTMLCanvasElement* canvas, IntRect& cropRect, const
ImageBitmapOptions& options)
this is still a non-const ref; could you please change all of these to either
pass-by-value or pass-by-const-ref?

[xidachen, 2016-07-13 14:50:23]

The latest PS is ready for re-review. Thank you.

https://codereview.chromium.org/2035113002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-resize.html
(right):

https://codereview.chromium.org/2035113002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-resize.html:78:
var imageBitmaps = {};
On 2016/07/07 21:21:45, jbroman wrote:
> nit: this variable is now unused

Done.

https://codereview.chromium.org/2035113002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2035113002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:21: namespace {
On 2016/07/07 21:21:45, jbroman wrote:
> nit: move this anonymous namespace inside the blink namespace; then you won't
> need blink:: qualification on things.

Done.

https://codereview.chromium.org/2035113002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:174: bool
ImageBitmap::isResizeOptionValid(const ImageBitmapOptions& options,
ExceptionState& exceptionState)
On 2016/07/07 21:21:45, jbroman wrote:
> nit: prefer static functions, or functions in anonymous namespaces, to private
> functions if they don't need special access (e.g. to member variables, to
> friends, etc.). That way they don't need to be declared in the header.

Per offline discussion, will leave it as it is here because there are call sites
in other classes.

https://codereview.chromium.org/2035113002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:177:
exceptionState.throwDOMException(InvalidStateError, "The resizeWidth or/and
resizeHeight is less than or equals to 0.");
On 2016/07/07 21:21:45, jbroman wrote:
> nit: For developer clarity, please specify which was the issue. Since this
> message won't be issued for the < case (I'm assuming -- covering this in
layout
> tests isn't a bad idea), please say that it is zero rather than <= 0.

Done.

https://codereview.chromium.org/2035113002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:183: bool
ImageBitmap::isSWSHValid(int sw, int sh, ExceptionState& exceptionState)
On 2016/07/07 21:21:45, jbroman wrote:
> nit: What's a SWSH? Prefer more descriptive names, like "isSourceSizeValid",
> "sourceWidth", "sourceHeight", etc.

Done.

https://codereview.chromium.org/2035113002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:331:
ImageBitmap::ImageBitmap(HTMLCanvasElement* canvas, IntRect& cropRect, const
ImageBitmapOptions& options)
On 2016/07/07 21:21:45, jbroman wrote:
> this is still a non-const ref; could you please change all of these to either
> pass-by-value or pass-by-const-ref?

Done.

[jbroman, 2016-07-13 15:18:57]

https://codereview.chromium.org/2035113002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2035113002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:31: IntRect cropRect =
IntRect();
"IntRect cropRect;" suffices; objects with a default constructor will have it
called.

https://codereview.chromium.org/2035113002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:36: static inline IntRect
normalizeRect(WTF::Optional<IntRect> rect)
This method requires that the rect exist; leave it as taking "const IntRect&",
and pass *cropRect at the call site.

https://codereview.chromium.org/2035113002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:66:
parsedOptions.shouldScaleInput = false;
Mind moving this down below, near where the other value for shouldScaleInput is
assigned?

https://codereview.chromium.org/2035113002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:87: if
(static_cast<int>(parsedOptions.resizeWidth) != sourceWidth ||
static_cast<int>(parsedOptions.resizeHeight) != sourceHeight)
nit: with the above, suggested refactor: put the early-return in the if branch,
and eliminate the else branch.

if (static_cast<int>(parsedOptions.resizeWidth) == sourceWidth && ...) {
    parsedOptions.shouldScaleInput = false;
    return parsedOptions;
}

parsedOptions.shouldScaleInput = true;
// ...

https://codereview.chromium.org/2035113002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:588: || (!cropRect &&
!isSourceSizeValid(width(), height(), exceptionState)))
Is it possible for the width and height of the image to not be valid? That seems
rather weird.

https://codereview.chromium.org/2035113002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/ImageBitmap.h (right):

https://codereview.chromium.org/2035113002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.h:41: static ImageBitmap*
create(HTMLImageElement*, WTF::Optional<IntRect>, Document*, const
ImageBitmapOptions& = ImageBitmapOptions());
By the way, like most of WTF, it's imported into the surrounding namespace (for
better or for worse). So you could just write "Optional<IntRect>", but I don't
much mind either way.

https://codereview.chromium.org/2035113002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/imagebitmap/ImageBitmapSource.h (right):

https://codereview.chromium.org/2035113002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/imagebitmap/ImageBitmapSource.h:15: #include
"third_party/WebKit/Source/wtf/Optional.h"
Use paths rooted at third_party/WebKit/Source/ within Blink. Just
"wtf/Optional.h" suffices.

[xidachen, 2016-07-13 15:52:21]

https://codereview.chromium.org/2035113002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2035113002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:31: IntRect cropRect =
IntRect();
On 2016/07/13 15:18:57, jbroman wrote:
> "IntRect cropRect;" suffices; objects with a default constructor will have it
> called.

Done.

https://codereview.chromium.org/2035113002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:36: static inline IntRect
normalizeRect(WTF::Optional<IntRect> rect)
On 2016/07/13 15:18:57, jbroman wrote:
> This method requires that the rect exist; leave it as taking "const IntRect&",
> and pass *cropRect at the call site.

Done.

https://codereview.chromium.org/2035113002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:66:
parsedOptions.shouldScaleInput = false;
On 2016/07/13 15:18:57, jbroman wrote:
> Mind moving this down below, near where the other value for shouldScaleInput
is
> assigned?

Done.

https://codereview.chromium.org/2035113002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:87: if
(static_cast<int>(parsedOptions.resizeWidth) != sourceWidth ||
static_cast<int>(parsedOptions.resizeHeight) != sourceHeight)
On 2016/07/13 15:18:57, jbroman wrote:
> nit: with the above, suggested refactor: put the early-return in the if
branch,
> and eliminate the else branch.
> 
> if (static_cast<int>(parsedOptions.resizeWidth) == sourceWidth && ...) {
>     parsedOptions.shouldScaleInput = false;
>     return parsedOptions;
> }
> 
> parsedOptions.shouldScaleInput = true;
> // ...

Done.

https://codereview.chromium.org/2035113002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:588: || (!cropRect &&
!isSourceSizeValid(width(), height(), exceptionState)))
On 2016/07/13 15:18:57, jbroman wrote:
> Is it possible for the width and height of the image to not be valid? That
seems
> rather weird.

It is a little complicated. Say that we do these steps:
1. bitmap1 = createImageBitmap(ImageData); where ImageData is 100*100
2. bitmap1.close();
3. bitmap2 = createImageBitmap(bitmap1);

Here problem comes because after calling close(), bitmap1's size is now 0 * 0,
we have to take care of this corner case, so that's why we need to check width()
&& height() is valid or not. As a matter of fact, checking width() and height()
is valid or not has to be done no matter cropRect() is specified or not. I am
changing to check width() && height() regardless.

https://codereview.chromium.org/2035113002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/ImageBitmap.h (right):

https://codereview.chromium.org/2035113002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.h:41: static ImageBitmap*
create(HTMLImageElement*, WTF::Optional<IntRect>, Document*, const
ImageBitmapOptions& = ImageBitmapOptions());
On 2016/07/13 15:18:57, jbroman wrote:
> By the way, like most of WTF, it's imported into the surrounding namespace
(for
> better or for worse). So you could just write "Optional<IntRect>", but I don't
> much mind either way.

Done.

[jbroman, 2016-07-13 17:10:29]

lgtm

https://codereview.chromium.org/2035113002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2035113002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:588: || (!cropRect &&
!isSourceSizeValid(width(), height(), exceptionState)))
On 2016/07/13 at 15:52:21, xidachen wrote:
> On 2016/07/13 15:18:57, jbroman wrote:
> > Is it possible for the width and height of the image to not be valid? That
seems
> > rather weird.
> 
> It is a little complicated. Say that we do these steps:
> 1. bitmap1 = createImageBitmap(ImageData); where ImageData is 100*100
> 2. bitmap1.close();
> 3. bitmap2 = createImageBitmap(bitmap1);
> 
> Here problem comes because after calling close(), bitmap1's size is now 0 * 0,
we have to take care of this corner case, so that's why we need to check width()
&& height() is valid or not. As a matter of fact, checking width() and height()
is valid or not has to be done no matter cropRect() is specified or not. I am
changing to check width() && height() regardless.

Is the same exception message suitable in either case? Or should the exception
message specify whether it was the bitmap that had invalid dimensions (because
it was closed), or the call site that provided invalid ones? We should aim to
give the author as much help in determining what went wrong as possible.

[xidachen, 2016-07-13 17:12:01]

Description was changed from

==========
Implement ImageBitmapOptions resize

In this CL, we add the options resizeWidth && resizeHeight &&
resizeQuality to the ImageBitmapOptions. All these options are hidding
behind the experimtanl flag.

Also, this CL implements this options for creating an ImageBitmap from
HTMLImageElement, HTMLCanvasElement, ImageBitmap and Blob. For creating
an ImageBitmap from ImageData and HTMLVideoElement is a bit more
complicated and will finish that in a separate CL.

This CL also adds a layout test to ensure the correctness of the behavior.

BUG=616589
==========

to

==========
Implement ImageBitmapOptions resize

In this CL, we add the options resizeWidth && resizeHeight &&
resizeQuality to the ImageBitmapOptions. All these options are hidding
behind the experimtanl flag.

Also, this CL implements this options for creating an ImageBitmap from
HTMLImageElement, HTMLCanvasElement, ImageBitmap and Blob. For creating
an ImageBitmap from ImageData and HTMLVideoElement is a bit more
complicated and will finish that in a separate CL.

This CL also adds a layout test to ensure the correctness of the behavior.

BUG=627855
==========

[xidachen, 2016-07-13 17:21:38]

On 2016/07/13 17:10:29, jbroman wrote:
> lgtm
> 
>
https://codereview.chromium.org/2035113002/diff/220001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):
> 
>
https://codereview.chromium.org/2035113002/diff/220001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/ImageBitmap.cpp:588: || (!cropRect &&
> !isSourceSizeValid(width(), height(), exceptionState)))
> On 2016/07/13 at 15:52:21, xidachen wrote:
> > On 2016/07/13 15:18:57, jbroman wrote:
> > > Is it possible for the width and height of the image to not be valid? That
> seems
> > > rather weird.
> > 
> > It is a little complicated. Say that we do these steps:
> > 1. bitmap1 = createImageBitmap(ImageData); where ImageData is 100*100
> > 2. bitmap1.close();
> > 3. bitmap2 = createImageBitmap(bitmap1);
> > 
> > Here problem comes because after calling close(), bitmap1's size is now 0 *
0,
> we have to take care of this corner case, so that's why we need to check
width()
> && height() is valid or not. As a matter of fact, checking width() and
height()
> is valid or not has to be done no matter cropRect() is specified or not. I am
> changing to check width() && height() regardless.
> 
> Is the same exception message suitable in either case? Or should the exception
> message specify whether it was the bitmap that had invalid dimensions (because
> it was closed), or the call site that provided invalid ones? We should aim to
> give the author as much help in determining what went wrong as possible.

That's a good point, specially now that we have close, transfer, we should
probably have different error messages. I will commit this CL as it is now and
have a discussion with junov@.

[xidachen, 2016-07-13 17:21:50]

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

[commit-bot: I haz the power, 2016-07-13 17:22:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-13 18:27:39]

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

[commit-bot: I haz the power, 2016-07-13 18:27:40]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[xidachen, 2016-07-13 18:39:53]

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

[commit-bot: I haz the power, 2016-07-13 18:41:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-13 21:12:55]

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

[commit-bot: I haz the power, 2016-07-13 21:12:56]

Dry run: This issue passed the CQ dry run.

[xidachen, 2016-07-13 21:21:04]

The CQ bit was checked by xidachen@chromium.org

[commit-bot: I haz the power, 2016-07-13 21:21:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-13 21:35:11]

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

[commit-bot: I haz the power, 2016-07-13 21:35:13]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[xidachen, 2016-07-13 22:07:47]

Description was changed from

==========
Implement ImageBitmapOptions resize

In this CL, we add the options resizeWidth && resizeHeight &&
resizeQuality to the ImageBitmapOptions. All these options are hidding
behind the experimtanl flag.

Also, this CL implements this options for creating an ImageBitmap from
HTMLImageElement, HTMLCanvasElement, ImageBitmap and Blob. For creating
an ImageBitmap from ImageData and HTMLVideoElement is a bit more
complicated and will finish that in a separate CL.

This CL also adds a layout test to ensure the correctness of the behavior.

BUG=627855
==========

to

==========
Implement ImageBitmapOptions resize

In this CL, we add the options resizeWidth && resizeHeight &&
resizeQuality to the ImageBitmapOptions. All these options are hidding
behind the experimtanl flag.

Also, this CL implements this options for creating an ImageBitmap from
HTMLImageElement, HTMLCanvasElement, ImageBitmap and Blob. For creating
an ImageBitmap from ImageData and HTMLVideoElement is a bit more
complicated and will finish that in a separate CL.

This CL also adds a layout test to ensure the correctness of the behavior.

TBR=junov@chromium.org
BUG=627855
==========

[xidachen, 2016-07-13 22:07:55]

The CQ bit was checked by xidachen@chromium.org

[commit-bot: I haz the power, 2016-07-13 22:09:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-13 22:42:05]

Description was changed from

==========
Implement ImageBitmapOptions resize

In this CL, we add the options resizeWidth && resizeHeight &&
resizeQuality to the ImageBitmapOptions. All these options are hidding
behind the experimtanl flag.

Also, this CL implements this options for creating an ImageBitmap from
HTMLImageElement, HTMLCanvasElement, ImageBitmap and Blob. For creating
an ImageBitmap from ImageData and HTMLVideoElement is a bit more
complicated and will finish that in a separate CL.

This CL also adds a layout test to ensure the correctness of the behavior.

TBR=junov@chromium.org
BUG=627855
==========

to

==========
Implement ImageBitmapOptions resize

In this CL, we add the options resizeWidth && resizeHeight &&
resizeQuality to the ImageBitmapOptions. All these options are hidding
behind the experimtanl flag.

Also, this CL implements this options for creating an ImageBitmap from
HTMLImageElement, HTMLCanvasElement, ImageBitmap and Blob. For creating
an ImageBitmap from ImageData and HTMLVideoElement is a bit more
complicated and will finish that in a separate CL.

This CL also adds a layout test to ensure the correctness of the behavior.

TBR=junov@chromium.org
BUG=627855
==========

[commit-bot: I haz the power, 2016-07-13 22:42:07]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-07-13 22:42:18]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 22:43:42]

Description was changed from

==========
Implement ImageBitmapOptions resize

In this CL, we add the options resizeWidth && resizeHeight &&
resizeQuality to the ImageBitmapOptions. All these options are hidding
behind the experimtanl flag.

Also, this CL implements this options for creating an ImageBitmap from
HTMLImageElement, HTMLCanvasElement, ImageBitmap and Blob. For creating
an ImageBitmap from ImageData and HTMLVideoElement is a bit more
complicated and will finish that in a separate CL.

This CL also adds a layout test to ensure the correctness of the behavior.

TBR=junov@chromium.org
BUG=627855
==========

to

==========
Implement ImageBitmapOptions resize

In this CL, we add the options resizeWidth && resizeHeight &&
resizeQuality to the ImageBitmapOptions. All these options are hidding
behind the experimtanl flag.

Also, this CL implements this options for creating an ImageBitmap from
HTMLImageElement, HTMLCanvasElement, ImageBitmap and Blob. For creating
an ImageBitmap from ImageData and HTMLVideoElement is a bit more
complicated and will finish that in a separate CL.

This CL also adds a layout test to ensure the correctness of the behavior.

TBR=junov@chromium.org
BUG=627855

Committed: https://crrev.com/96e0583198a59d305f8442704e35d7d307120cec
Cr-Commit-Position: refs/heads/master@{#405314}
==========

[commit-bot: I haz the power, 2016-07-13 22:43:43]

Patchset 13 (id:??) landed as
https://crrev.com/96e0583198a59d305f8442704e35d7d307120cec
Cr-Commit-Position: refs/heads/master@{#405314}