Allow eviction of ImageBitmaps that are created from ImageElements.

Allow eviction of ImageBitmaps that are created from ImageElements.

We hold a reference to the HTMLImageElement in the ImageBitmap but elevate the priority of 
its CachedResource. Cropping is handled at drawing time.

Some style changes are also being introduced in this patch.
 
BUG=259947, 166658
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155546

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155629

[arbesfeld, 2013-07-16 19:24:40]

We only hold a reference to the HTMLImageElement so the cache should be able to
release its image. However, I'm not sure if the "setPriority" lines actually
change the caching behavior because the telemetry test still runs fast with
these lines commented out, which suggests that the pixels are still being pinned
to RAM.

[Justin Novosad, 2013-07-16 22:45:21]

QQ: what is the impact of this change on the performance of the many images
benchmark?

[arbesfeld, 2013-07-16 22:54:19]

On 2013/07/16 22:45:21, junov wrote:
> QQ: what is the impact of this change on the performance of the many images
> benchmark?

From what I can tell, none. Even commenting out the "setPriority" lines doesn't
degrade the telemetry performance. That is why that I suspect this patch is not
achieving its goal.

[Justin Novosad, 2013-07-16 23:05:38]

On 2013/07/16 22:54:19, arbesfeld wrote:
> On 2013/07/16 22:45:21, junov wrote:
> > QQ: what is the impact of this change on the performance of the many images
> > benchmark?
> 
> From what I can tell, none. Even commenting out the "setPriority" lines
doesn't
> degrade the telemetry performance. That is why that I suspect this patch is
not
> achieving its goal.

Actually you need a case that triggers eviction from the decode cache. You can
make tweaked version of many images that uses larger images in order to
deliberately trigger eviction.  In the directory where the image set is located,
there is a script that generates the images.  You can fiddle with that to
generate larger image files.  In the trace graph, you will see continuously
occurring decode tasks appear on the main thread when the eviction happens.

It would be nice to add a "many large images" test so that we can track this.

[Justin Novosad, 2013-07-16 23:05:53]

https://codereview.chromium.org/19393004/diff/3001/Source/core/page/ImageBitm...
File Source/core/page/ImageBitmap.cpp (right):

https://codereview.chromium.org/19393004/diff/3001/Source/core/page/ImageBitm...
Source/core/page/ImageBitmap.cpp:108: m_bitmap = cropImage(bitmapImage.get(),
adjustedCropRect);
You can do better here.  If the source ImageBitmap refers to an image element,
then the newly created ImageBitmap should too, rather than create a snapshot,
which will be pinned in memory.

https://codereview.chromium.org/19393004/diff/3001/Source/core/page/ImageBitm...
Source/core/page/ImageBitmap.cpp:151: if (m_imageElement)
This is a good place for an ASSERT to validate that m_bitmap and m_imageElement
are mutually exclusive

https://codereview.chromium.org/19393004/diff/3001/Source/core/page/ImageBitm...
File Source/core/page/ImageBitmap.h (right):

https://codereview.chromium.org/19393004/diff/3001/Source/core/page/ImageBitm...
Source/core/page/ImageBitmap.h:52: RefPtr<Image> m_bitmap;
Need a comment to explain that m_bitmap is not used when tracking an image
element.

[Justin Novosad, 2013-07-16 23:09:07]

Overall, I think this patch is on the right track, but we need tests to verify
that it does what we think it does.  We need to prove that the image buffers are
in fact evictable.  We also need to prove that when there are some image that
have ImageBitmaps and some that do not, those that do not are evicted first.  A
unit test would be best for this.

[arbesfeld, 2013-07-17 23:14:16]

Please take a look at this patch.

[Justin Novosad, 2013-07-19 21:09:53]

https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
File Source/core/loader/cache/CachedImage.cpp (right):

https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
Source/core/loader/cache/CachedImage.cpp:406: // produces a different image,
m_image != image for ImageBitmaps
We need a better condition. Something like !m_image.sharesDataWith(image)

https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
File Source/core/loader/cache/CachedImageTest.h (right):

https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
Source/core/loader/cache/CachedImageTest.h:42: class MockCachedImageClient :
public WebCore::CachedImageClient {
Rename this file to MockCachedImageClient.h

https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
File Source/core/loader/cache/CachedResource.cpp (right):

https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResource.cpp:108:
CachedResource::CachedResource(const ResourceRequest& request, Type type,
DecodeCachePriority decodeCachePriority)
No need for extra construction arg. Just initialize with low priority by
default. Client code can raise priority explicitly if needed.

https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
File Source/core/loader/cache/CachedResource.h (right):

https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResource.h:83: enum DecodeCachePriority {
I'd call this just Priority.

https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResource.h:328: DecodeCachePriority
m_decodeCachePriority;
Put ": 2" at the end.

https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
File Source/core/loader/cache/MemoryCache.cpp (right):

https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
Source/core/loader/cache/MemoryCache.cpp:482: for (CachedResource* next =
m_liveDecodedResources.m_head; next; next = next->m_nextInLiveResourcesList) {
This algorithm is not good, you are making successive insertion into an O(N^2)
process. You should make m_liveDecodedResources into an array in order to have
one list per priority level. That way successive insertion could still be done
in linear time. IT would also keep the code as almost as simple as before

https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
File Source/core/loader/cache/MemoryCache.h (right):

https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
Source/core/loader/cache/MemoryCache.h:28: #include "core/platform/Timer.h"
Why is this extra include needed? there are no other changes to this file.

https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
File Source/core/loader/cache/MemoryCacheTest.cpp (right):

https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
Source/core/loader/cache/MemoryCacheTest.cpp:131: // Verifies that
CachedResources are eviced from the decode catch
catch -> cache

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

https://codereview.chromium.org/19393004/diff/15001/Source/core/page/ImageBit...
Source/core/page/ImageBitmap.cpp:41: // this will also elevate the decode cache
priority of the original HTMLImageElement
This is fine, no need to write this comment.

https://codereview.chromium.org/19393004/diff/15001/Source/core/page/ImageBit...
Source/core/page/ImageBitmap.cpp:43:
cachedImage->setDecodeCachePriority(CachedResource::DecodeCachePriorityHigh);
I don't think this code works.  The unit test you wrote for the cache is fine,
but you need an additional test that exercises this code.  When you set the
cache priority on an item that is already inserted in the cache, its position in
the cache's linked list remains unchanged. So this probably has no effect.

https://codereview.chromium.org/19393004/diff/15001/Source/core/page/ImageBit...
Source/core/page/ImageBitmap.cpp:159: ASSERT(!m_imageElement || !m_bitmap);
To be more accurate, this should be an exclusive or.  There is no xor logical
operator in C++, so you could write it as
ASSERT(m_imageElement || m_bitmap && (!m_imageElement || !m_bitmap));

https://codereview.chromium.org/19393004/diff/15001/Source/core/page/ImageBit...
File Source/core/page/ImageBitmap.h (right):

https://codereview.chromium.org/19393004/diff/15001/Source/core/page/ImageBit...
Source/core/page/ImageBitmap.h:54: // The Image and HTMLImageElement are
mutually exclusive
Missing period.

https://codereview.chromium.org/19393004/diff/15001/Source/core/page/ImageBit...
Source/core/page/ImageBitmap.h:55: // ImageBitmaps derived from
HTMLImageElements use m_imageElement
"derived" is a loaded term in C++. Say "constructed from" instead

https://codereview.chromium.org/19393004/diff/15001/Source/core/page/ImageBit...
Source/core/page/ImageBitmap.h:57: HTMLImageElement* m_imageElement;
You should have a LayoutTest that creates an ImageBitmap from a temporary Image,
the triggers a garbage collect after the temporary image is out of scope.  This
code should fail that test. I think it should pass if you make m_imageElement a
RefPtr

[arbesfeld, 2013-07-22 21:50:50]

On 2013/07/19 21:09:53, junov wrote:
>
https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
> File Source/core/loader/cache/CachedImage.cpp (right):
> 
>
https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
> Source/core/loader/cache/CachedImage.cpp:406: // produces a different image,
> m_image != image for ImageBitmaps
> We need a better condition. Something like !m_image.sharesDataWith(image)

I changed ImageBitmap to hold its own CachedImage so it resolved this issue.

>
https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
> File Source/core/loader/cache/CachedResource.cpp (right):
> 
>
https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
> Source/core/loader/cache/CachedResource.cpp:108:
> CachedResource::CachedResource(const ResourceRequest& request, Type type,
> DecodeCachePriority decodeCachePriority)
> No need for extra construction arg. Just initialize with low priority by
> default. Client code can raise priority explicitly if needed.

It would be nice if we could initialize CachedResources with certain priorities.
That way you wouldn't have to remove and do a reinsertion every time that a
non-low-priority CachedResource is created.

>
https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
> File Source/core/loader/cache/CachedResource.h (right):
> 
>
https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
> Source/core/loader/cache/CachedResource.h:83: enum DecodeCachePriority {
> I'd call this just Priority.

There is already a "ResourceLoadPriority". I changed it to DecodePriority, but
maybe DecodeCachePriority is more understandable?

>
https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
> Source/core/loader/cache/CachedResource.h:328: DecodeCachePriority
> m_decodeCachePriority;
> Put ": 2" at the end.

The style checker tells me "Please declare all bitfields as unsigned."

>
https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
> File Source/core/loader/cache/MemoryCache.cpp (right):
> 
>
https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
> Source/core/loader/cache/MemoryCache.cpp:482: for (CachedResource* next =
> m_liveDecodedResources.m_head; next; next = next->m_nextInLiveResourcesList) {
> This algorithm is not good, you are making successive insertion into an O(N^2)
> process. You should make m_liveDecodedResources into an array in order to have
> one list per priority level. That way successive insertion could still be done
> in linear time. IT would also keep the code as almost as simple as before

Done.

>
https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
> File Source/core/loader/cache/MemoryCache.h (right):
> 
>
https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
> Source/core/loader/cache/MemoryCache.h:28: #include "core/platform/Timer.h"
> Why is this extra include needed? there are no other changes to this file.

If you enable the "MEMORY_CACHE_STATS" define then it requires this header file.
I guess we can wrap it in #define?

[Justin Novosad, 2013-07-23 15:35:48]

On 2013/07/22 21:50:50, arbesfeld wrote:
> 
> 
> The style checker tells me "Please declare all bitfields as unsigned."

That is fine. Assigning a enum value to an unsigned is perfectly legal. The
typecast is implicit.
 

> >
>
https://codereview.chromium.org/19393004/diff/15001/Source/core/loader/cache/...
> > Source/core/loader/cache/MemoryCache.h:28: #include "core/platform/Timer.h"
> > Why is this extra include needed? there are no other changes to this file.
> 
> If you enable the "MEMORY_CACHE_STATS" define then it requires this header
file.
> I guess we can wrap it in #define?

Where are you enabling MEMORY_CACHE_STATS?

[Justin Novosad, 2013-07-23 16:55:12]

https://codereview.chromium.org/19393004/diff/28001/Source/core/loader/cache/...
File Source/core/loader/cache/CachedResource.cpp (left):

https://codereview.chromium.org/19393004/diff/28001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResource.cpp:554: 
Don't do whitespace edits unless you are changing the immediately surrounding
code.

https://codereview.chromium.org/19393004/diff/28001/Source/core/loader/cache/...
File Source/core/loader/cache/CachedResource.h (right):

https://codereview.chromium.org/19393004/diff/28001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResource.h:83: enum DecodePriority {
Hmmm... this name sounds like it determines which image will be decoded before
the others, which is not what it is for. How about calling it CachePriority?

https://codereview.chromium.org/19393004/diff/28001/Source/core/loader/cache/...
File Source/core/loader/cache/MemoryCache.cpp (left):

https://codereview.chromium.org/19393004/diff/28001/Source/core/loader/cache/...
Source/core/loader/cache/MemoryCache.cpp:55: static const double
cMinDelayBeforeLiveDecodedPrune = 1; // Seconds.
This variable should still exist, but it should be called
cDefaultDelayBeforeLiveDecodedPrune.  Use this constant in the constructor below
instead of the hard coded 1.

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

https://codereview.chromium.org/19393004/diff/28001/Source/core/page/ImageBit...
Source/core/page/ImageBitmap.cpp:39: m_bitmap = new
CachedImage(cropImage(image->cachedImage()->image(), cropRect).get());
Why a new cached image? can we not take a ref on the original cached image. 
What happens if we try to use the ImageBitmap after its cached image is dropped
from the decode cache?  Expected behavior is for the image to be re-decoded in
that case. I don't see how that can happen with this code since we are losing
track of the source resource.

https://codereview.chromium.org/19393004/diff/28001/Source/core/page/ImageBit...
Source/core/page/ImageBitmap.cpp:58: m_bitmap = new
CachedImage(m_buffer->copyImage(DontCopyBackingStore).get());
When creating from a video, we have no way to regenerate the image if it is ever
discarded from cache, so this should not be a cached image. It should continue
to be a copy store in a BitmapImage.

https://codereview.chromium.org/19393004/diff/28001/Source/core/page/ImageBit...
Source/core/page/ImageBitmap.cpp:74: m_bitmap = new
CachedImage(m_buffer->copyImage(DontCopyBackingStore).get());
Same here.  This should not ba a cached image.

https://codereview.chromium.org/19393004/diff/28001/Source/core/page/ImageBit...
Source/core/page/ImageBitmap.cpp:90: m_bitmap = new
CachedImage(m_buffer->copyImage(DontCopyBackingStore).get());
Should not be a cached image

https://codereview.chromium.org/19393004/diff/28001/Source/core/page/ImageBit...
Source/core/page/ImageBitmap.cpp:102: m_bitmap = new
CachedImage(cropImage(bitmap->bitmapImage(), adjustedCropRect).get());
This function needs to be more complex. It should share resource when possible
and it should only have a CachedImage if the source has a CachedImage.

[arbesfeld, 2013-07-24 22:06:14]

New patch. The HTMLImageElement notifies the ImageBitmap when its source image
changes so that the ImageBitmap can save the image to memory. I also fixed a
problem with the rectangle cropping when multiple createImageBitmap calls are
strung together.

[Justin Novosad, 2013-07-25 15:23:58]

On 2013/07/24 22:06:14, arbesfeld wrote:
> New patch. The HTMLImageElement notifies the ImageBitmap when its source image
> changes so that the ImageBitmap can save the image to memory. I also fixed a
> problem with the rectangle cropping when multiple createImageBitmap calls are
> strung together.

As discussed, we need to fix and test cases with crop rectangles and source
rectangle overlap source boundaries. in drawImage

[arbesfeld, 2013-07-25 15:47:47]

Ok, I added a test to check that which assumes that canvas to canvas draws are
handled correctly.

[Justin Novosad, 2013-07-25 17:30:24]

On 2013/07/25 15:47:47, arbesfeld wrote:
> Ok, I added a test to check that which assumes that canvas to canvas draws are
> handled correctly.

LGTM, but we need a Blink OWNER to sign-off.  Senorblanco?

[Stephen White, 2013-07-25 18:01:06]

I don't feel qualified to review all of this, so I've added japhet@ for the
ImageLoader changes and hclam@ for the MemoryCache changes. Feel free to pass
this on to someone else if you feel appropriate.

https://codereview.chromium.org/19393004/diff/45001/LayoutTests/fast/canvas/c...
File LayoutTests/fast/canvas/canvas-createImageBitmap-drawImage.html (left):

https://codereview.chromium.org/19393004/diff/45001/LayoutTests/fast/canvas/c...
LayoutTests/fast/canvas/canvas-createImageBitmap-drawImage.html:205: function
checkImmutable(element) {
This function now seems to be unused; please remove.

https://codereview.chromium.org/19393004/diff/45001/LayoutTests/fast/canvas/c...
File LayoutTests/fast/canvas/canvas-createImageBitmap-immutable-expected.html
(right):

https://codereview.chromium.org/19393004/diff/45001/LayoutTests/fast/canvas/c...
LayoutTests/fast/canvas/canvas-createImageBitmap-immutable-expected.html:11:
ctx.fillRect(0, 0, 100, 100);
Consider making these rects (and the canvas) smaller; looks like they could be
10x10 and still have same coverage.

https://codereview.chromium.org/19393004/diff/45001/LayoutTests/fast/canvas/c...
File LayoutTests/fast/canvas/canvas-createImageBitmap-immutable.html (right):

https://codereview.chromium.org/19393004/diff/45001/LayoutTests/fast/canvas/c...
LayoutTests/fast/canvas/canvas-createImageBitmap-immutable.html:7: // Create
auxiliary canvas to draw to and create an image from.
Grammar nit: ending a sentence with a preposition is an error up with which I
will not put.

https://codereview.chromium.org/19393004/diff/45001/LayoutTests/fast/canvas/c...
LayoutTests/fast/canvas/canvas-createImageBitmap-immutable.html:30: image.src =
aCanvas.toDataURL(); // set a data URI of the base64 enconded image as the
source
Nit: enconded. Also, no punctuation

https://codereview.chromium.org/19393004/diff/45001/LayoutTests/fast/canvas/c...
File
LayoutTests/fast/canvas/canvas-createImageBitmap-out-of-bounds-src-expected.html
(right):

https://codereview.chromium.org/19393004/diff/45001/LayoutTests/fast/canvas/c...
LayoutTests/fast/canvas/canvas-createImageBitmap-out-of-bounds-src-expected.html:3:
<canvas id="c1" width="250" height="250"></canvas>
These could probably be smaller too (to accommodate Mesa slowness.. sorry!)

https://codereview.chromium.org/19393004/diff/45001/LayoutTests/fast/canvas/c...
File LayoutTests/fast/canvas/canvas-createImageBitmap-out-of-bounds-src.html
(right):

https://codereview.chromium.org/19393004/diff/45001/LayoutTests/fast/canvas/c...
LayoutTests/fast/canvas/canvas-createImageBitmap-out-of-bounds-src.html:48:
ctx.drawImage(imageBitmap, -100, 0,    200, 100, -25, 75,  100, 100);
Nit: spacing is a bit weird here.

https://codereview.chromium.org/19393004/diff/45001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/19393004/diff/45001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1296:
normalizedDstRect.height() / normalizedSrcRect.height() * intersectRect.height()
/ bitmapRect.height());
Should we be checking for zero in normalizedSrcRect.width() and
normalizedSrcRect.height()? Could be a div-by-zero here.

https://codereview.chromium.org/19393004/diff/45001/Source/core/loader/ImageL...
File Source/core/loader/ImageLoader.h (right):

https://codereview.chromium.org/19393004/diff/45001/Source/core/loader/ImageL...
Source/core/loader/ImageLoader.h:81: void addClient(ImageLoaderClient* client) {
m_clients.add(client); }
I'm not sure if the ImageLoader is really the right place for this, since it's
not really related to image loading per se. IIUC, you want to watch changes to
the Image element's src attribute. I'm wondering if there's an observer or
something at the DOM level that you could use for this.

https://codereview.chromium.org/19393004/diff/45001/Source/core/loader/cache/...
File Source/core/loader/cache/MemoryCacheTest.cpp (right):

https://codereview.chromium.org/19393004/diff/45001/Source/core/loader/cache/...
Source/core/loader/cache/MemoryCacheTest.cpp:131: // Verifies that
CachedResources are eviced from the decode cache
Nit: eviced.

[arbesfeld, 2013-07-25 18:30:57]

Thanks. 

>
https://codereview.chromium.org/19393004/diff/45001/LayoutTests/fast/canvas/c...
> File
>
LayoutTests/fast/canvas/canvas-createImageBitmap-out-of-bounds-src-expected.html
> (right):
> 
>
https://codereview.chromium.org/19393004/diff/45001/LayoutTests/fast/canvas/c...
>
LayoutTests/fast/canvas/canvas-createImageBitmap-out-of-bounds-src-expected.html:3:
> <canvas id="c1" width="250" height="250"></canvas>
> These could probably be smaller too (to accommodate Mesa slowness.. sorry!)

Using small source rectangles is causing color bleeding for the cropped canvas
to canvas draws. If this test runs slow we can try to find a work-around or just
use text results with smaller canvases.

Regarding watching the Image's src attribute, I was not sure if there are other
cases where the HTMLImageElement's image changes. Watching the ImageLoader
covers all possible changes to the source image.

[arbesfeld, 2013-07-26 15:47:58]

The canvas-createImageBitmap-recursive test was timing out so I decreased the
dimensions of the canvas.

[Nate Chapin, 2013-07-26 17:05:11]

https://codereview.chromium.org/19393004/diff/69001/Source/core/loader/cache/...
File Source/core/loader/cache/CachedResource.h (right):

https://codereview.chromium.org/19393004/diff/69001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResource.h:83: enum CachePriority {
I know there was already debate about it, but...I'm not sure I like this name.
:( It's not very descriptive, and I had to read most of the patch to figure out
how it is used.

Maybe invert it and call it DecodedDataDeletionPriority?
CacheLiveResourcePriority?

At the very last, a comment explaining how it's used seems appropriate.

https://codereview.chromium.org/19393004/diff/69001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResource.h:85: CachePriorityMedium,
I can't find any uses of this value?

https://codereview.chromium.org/19393004/diff/69001/Source/core/loader/cache/...
File Source/core/loader/cache/MemoryCache.cpp (right):

https://codereview.chromium.org/19393004/diff/69001/Source/core/loader/cache/...
Source/core/loader/cache/MemoryCache.cpp:174: for (int i = 0; i < 3; i++) {
for (CachePriority priority = CachePriorityLow; priority <= CachePriorityHigh;
priority++) {

?

https://codereview.chromium.org/19393004/diff/69001/Source/core/loader/cache/...
File Source/core/loader/cache/MemoryCache.h (right):

https://codereview.chromium.org/19393004/diff/69001/Source/core/loader/cache/...
Source/core/loader/cache/MemoryCache.h:190: LRUList m_liveDecodedResources[3];
Boo magic numbers :(

Maybe add a value to the enum, call it CachePriorityCount, and initialize this
to that? Or [CachePriorityHigh + 1].

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

https://codereview.chromium.org/19393004/diff/69001/Source/core/page/ImageBit...
Source/core/page/ImageBitmap.cpp:40:
m_imageElement->cachedImage()->setCachePriority(CachedResource::CachePriorityHigh);
Is there any way that we could get setting this value closer to the cache? This
class knows almost nothing about CachedImages, and it feels wrong to reach so
far down into the cache.

[Justin Novosad, 2013-07-29 14:57:59]

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

https://codereview.chromium.org/19393004/diff/69001/Source/core/page/ImageBit...
Source/core/page/ImageBitmap.cpp:40:
m_imageElement->cachedImage()->setCachePriority(CachedResource::CachePriorityHigh);
On 2013/07/26 17:05:12, Nate Chapin wrote:
> Is there any way that we could get setting this value closer to the cache?
This
> class knows almost nothing about CachedImages, and it feels wrong to reach so
> far down into the cache.

On top of that, the current implementation lacks a means of returning the image
to low cache priority once the image bitmaps are destroyed.
I think addClient/removeClient should take care of setting/unsetting the high
priority based on the presence of clients that require high priority.  To this
end, there should be a virtual method on ImageLoaderClient to signal whether
high priority is requested by the client. ImageBitmap should overload that
method to return true.  To make the implementation efficient, the image element
should probably maintain a counter of the number of high priority clients, and
set the priority back to loww when the counter goes back to 0.

[arbesfeld, 2013-07-29 15:20:02]

Great suggestions. New patch up for review.

[Justin Novosad, 2013-07-29 15:29:43]

https://codereview.chromium.org/19393004/diff/78001/Source/core/loader/ImageL...
File Source/core/loader/ImageLoader.h (right):

https://codereview.chromium.org/19393004/diff/78001/Source/core/loader/ImageL...
Source/core/loader/ImageLoader.h:39: virtual bool
isHighPriorityImageLoaderClient() = 0;
the "ImageLoaderClient" part of themname does not provide any useful
information, and it is not clear form the name that we are referring to cache
priority. Suggestion: requestsHighCachePriority
Also, it is not necessary to make this pure virtual.  Default implementation
should return false.

[arbesfeld, 2013-07-29 17:28:49]

On 2013/07/29 15:29:43, junov wrote:
>
https://codereview.chromium.org/19393004/diff/78001/Source/core/loader/ImageL...
> File Source/core/loader/ImageLoader.h (right):
> 
>
https://codereview.chromium.org/19393004/diff/78001/Source/core/loader/ImageL...
> Source/core/loader/ImageLoader.h:39: virtual bool
> isHighPriorityImageLoaderClient() = 0;
> the "ImageLoaderClient" part of themname does not provide any useful
> information, and it is not clear form the name that we are referring to cache
> priority. Suggestion: requestsHighCachePriority
> Also, it is not necessary to make this pure virtual.  Default implementation
> should return false.

Done.

[Justin Novosad, 2013-07-29 17:47:57]

https://codereview.chromium.org/19393004/diff/86001/Source/core/loader/ImageL...
File Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/19393004/diff/86001/Source/core/loader/ImageL...
Source/core/loader/ImageLoader.cpp:434: if (m_image)
Better to only call setCacheLiveResourcePriority if necessary. Suggestion: if
(m_image && !m_highPriorityClientCount++)

[arbesfeld, 2013-07-29 18:04:02]

Done.

[arbesfeld, 2013-07-30 18:46:14]

New patch for review.

[Justin Novosad, 2013-07-30 19:04:00]

Looks fine to me.  Nate, anything to add?

[arbesfeld, 2013-08-02 15:10:22]

Re-based the tests to use Promises. I still need a Blink OWNER to sign off.
Nate/Alpha, any thoughts?

[Nate Chapin, 2013-08-05 17:49:09]

LGTM

[commit-bot: I haz the power, 2013-08-05 18:17:14]

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

[commit-bot: I haz the power, 2013-08-05 22:27:06]

Change committed as 155546

[commit-bot: I haz the power, 2013-08-06 15:17:48]

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

[commit-bot: I haz the power, 2013-08-06 15:19:50]

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

[commit-bot: I haz the power, 2013-08-06 20:07:57]

Change committed as 155629