Allow ImageBitmaps derived from HTMLImageElements to be evicted from RAM.

Allow ImageBitmaps derived from HTMLImageElements to be evicted from RAM.

Since we were pre-cropping the HTMLImageElement before drawing, the 
CachedImage::didDraw(Image) command exited early because the Image argument
was different from the image referenced in the CachedImage. This has been fixed
by not pre-cropping the image and allowing drawBitmapRectToRect to handle the 
cropping.

TEST=manual
BUG=259947, 166658
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155805

[Justin Novosad, 2013-08-07 19:00:12]

Seems like you should be able to write a unit test for this.

[arbesfeld, 2013-08-07 21:49:12]

Done.

[Justin Novosad, 2013-08-08 13:04:35]

On 2013/08/07 21:49:12, arbesfeld wrote:
> Done.

There was a similarity blunder.  Re-upload with --similarity 100

[Justin Novosad, 2013-08-08 13:18:27]

https://codereview.chromium.org/22613002/diff/5001/Source/core/page/ImageBitm...
File Source/core/page/ImageBitmapTest.cpp (right):

https://codereview.chromium.org/22613002/diff/5001/Source/core/page/ImageBitm...
Source/core/page/ImageBitmapTest.cpp:46: // Verifies that the cached image help
by an ImageBitmap is the same as the
help -> held

https://codereview.chromium.org/22613002/diff/5001/Source/core/page/ImageBitm...
Source/core/page/ImageBitmapTest.cpp:51: CachedImage* cachedImage = new
CachedImage(BitmapImage::create().get());
I think this code leaks the cachedImage. Should put it in an OwnPtr.

Also, you should call the version of ImageBitmap::create that takes a
NativeImageSkia, so that we actually have something to crop from.

https://codereview.chromium.org/22613002/diff/5001/Source/core/page/ImageBitm...
Source/core/page/ImageBitmapTest.cpp:53: RefPtr<ImageBitmap> imageBitmap =
ImageBitmap::create(imageElement.get(), IntRect(0, 0, 50, 50));
Would be good to test different scenarios here:
crop rect == image rect.
crop rect included in image rect
crop rect outside of image rect (In this case perhaps
imageBitmap->bitmapImage().get() == 0?)
crop rect intersecting image rect

[arbesfeld, 2013-08-08 14:23:48]

On 2013/08/08 13:18:27, junov wrote:
>
https://codereview.chromium.org/22613002/diff/5001/Source/core/page/ImageBitm...
> File Source/core/page/ImageBitmapTest.cpp (right):
> 
>
https://codereview.chromium.org/22613002/diff/5001/Source/core/page/ImageBitm...
> Source/core/page/ImageBitmapTest.cpp:46: // Verifies that the cached image
help
> by an ImageBitmap is the same as the
> help -> held
> 
>
https://codereview.chromium.org/22613002/diff/5001/Source/core/page/ImageBitm...
> Source/core/page/ImageBitmapTest.cpp:51: CachedImage* cachedImage = new
> CachedImage(BitmapImage::create().get());
> I think this code leaks the cachedImage. Should put it in an OwnPtr.
> 
> Also, you should call the version of ImageBitmap::create that takes a
> NativeImageSkia, so that we actually have something to crop from.
> 
>
https://codereview.chromium.org/22613002/diff/5001/Source/core/page/ImageBitm...
> Source/core/page/ImageBitmapTest.cpp:53: RefPtr<ImageBitmap> imageBitmap =
> ImageBitmap::create(imageElement.get(), IntRect(0, 0, 50, 50));
> Would be good to test different scenarios here:
> crop rect == image rect.
> crop rect included in image rect
> crop rect outside of image rect (In this case perhaps
> imageBitmap->bitmapImage().get() == 0?)
> crop rect intersecting image rect

Done. The case of "crop rect outside of image rect" still returns the same
CachedImage because we don't do any of the cropping inside ImageBitmap. This
case would just result in an early exit from drawImage().

[Justin Novosad, 2013-08-08 15:22:48]

On 2013/08/08 14:23:48, arbesfeld wrote:
> Done. The case of "crop rect outside of image rect" still returns the same
> CachedImage because we don't do any of the cropping inside ImageBitmap. This
> case would just result in an early exit from drawImage().

That would be be a good thing to optimize in a follow-up patch to avoid
needlessly holding a ref on the cached image and promoting its cache priority.
At image bitmap construction time, you could just verify whether the crop rect
intersects the source image and do the appropriate short circuiting right there.

As far as this patch is concerned: lgtm
senorblanco?

[Stephen White, 2013-08-08 15:32:11]

LGTM. Nits/questions are at your discretion.

https://codereview.chromium.org/22613002/diff/12001/Source/core/page/ImageBit...
File Source/core/page/ImageBitmap.cpp (right):

https://codereview.chromium.org/22613002/diff/12001/Source/core/page/ImageBit...
Source/core/page/ImageBitmap.cpp:51: , m_bitmapOffset(IntPoint())
Out of curiosity, why don't we set a bitmap offset in the video and
canvas-to-canvas cases?

https://codereview.chromium.org/22613002/diff/12001/Source/core/page/ImageBit...
Source/core/page/ImageBitmap.cpp:104: , m_imageElement(bitmap->imageElement())
Nit: might be clearer to initialize m_bitmapOffset to IntPoint(), then only
assign it in the first case below.

[arbesfeld, 2013-08-08 15:41:12]

On 2013/08/08 15:32:11, Stephen White wrote:
> LGTM. Nits/questions are at your discretion.
> 
>
https://codereview.chromium.org/22613002/diff/12001/Source/core/page/ImageBit...
> File Source/core/page/ImageBitmap.cpp (right):
> 
>
https://codereview.chromium.org/22613002/diff/12001/Source/core/page/ImageBit...
> Source/core/page/ImageBitmap.cpp:51: , m_bitmapOffset(IntPoint())
> Out of curiosity, why don't we set a bitmap offset in the video and
> canvas-to-canvas cases?
> 
>
https://codereview.chromium.org/22613002/diff/12001/Source/core/page/ImageBit...
> Source/core/page/ImageBitmap.cpp:104: , m_imageElement(bitmap->imageElement())
> Nit: might be clearer to initialize m_bitmapOffset to IntPoint(), then only
> assign it in the first case below.

I added a comment to explain bitmap offset:     
    // The offset by which the desired Image is stored internally.
    // ImageBitmaps constructed from HTMLImageElements reference the entire
CachedImage and may have a non-zero bitmap offset.
    // ImageBitmaps not constructed from HTMLImageElements always pre-crop and
store the image at (0, 0).

[Justin Novosad, 2013-08-08 15:45:27]

On 2013/08/08 15:32:11, Stephen White wrote:

> Out of curiosity, why don't we set a bitmap offset in the video and
> canvas-to-canvas cases?

Because we do deep copies of the crop region in those cases, so the copies don't
need to be offset.

[commit-bot: I haz the power, 2013-08-08 15:47:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/arbesfeld@chromium.org/22613002/18007

[Stephen White, 2013-08-08 16:05:57]

On 2013/08/08 15:45:27, junov wrote:
> On 2013/08/08 15:32:11, Stephen White wrote:
> 
> > Out of curiosity, why don't we set a bitmap offset in the video and
> > canvas-to-canvas cases?
> 
> Because we do deep copies of the crop region in those cases, so the copies
don't
> need to be offset.

Great, thanks for the explanation.

[commit-bot: I haz the power, 2013-08-08 18:10:31]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_layout.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-08 18:37:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/arbesfeld@chromium.org/22613002/42001

[commit-bot: I haz the power, 2013-08-09 00:04:23]

Change committed as 155805