[webkit-reviews] review granted: [Bug 106842] adding support for DiscardablePixelRef for caching lazily decoded images : [Attachment 185049] Patch

[bugzilla-daemon at webkit.org]

Stephen White <senorblanco at chromium.org> has granted Min Qin
<qinmin at chromium.org>'s request for review:
Bug 106842: adding support for DiscardablePixelRef for caching lazily decoded
images
https://bugs.webkit.org/show_bug.cgi?id=106842

Attachment 185049: Patch
https://bugs.webkit.org/attachment.cgi?id=185049&action=review

------- Additional Comments from Stephen White <senorblanco at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=185049&action=review


This patch seems ok with nits fixed.  r=me

> Source/WebCore/ChangeLog:8
> +	   This change allows using discardable memory in deferred image
decoding path.

Nit:  in deferred -> in the deferred

> Source/WebCore/ChangeLog:11
> +	   Separate caching policy for discardable memory

This is not a sentence.

> Source/WebCore/ChangeLog:12
> +	   Discardable memory allocation could fail, fall back to heap
allocation in that case.

Nit:  comma splice:  use semicolon instead of comma, or break this into two
sentences.

> Source/WebCore/ChangeLog:13
> +	   Separate cache size limit for heap entries, no limit on discardable
entries

Same here.  This is also not a sentence.

> Source/WebCore/ChangeLog:18
> +	   (WebCore::DiscardablePixelRefAllocator::allocPixelRef):

You really should have a description of the per-file changes here.

> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:64
> +DiscardablePixelRef::DiscardablePixelRef(SkColorTable* ctable,
PassOwnPtr<SkMutex> mutex)

Kind of weird to wrap Skia stuff in OwnPtrs, but I guess it's ok in this case
since it's not refcounted.

> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:110
> +    if (pixelRef && pixelRef->getURI() &&
discardable.equals(pixelRef->getURI()))
> +	   return true;
> +    return false;

Could just be

return pixelRef && pixelRef->getURI() && discardable.equals(pixelRef->getURI())


> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:101
> +	   image->bitmap().lockPixels();
> +	   SkPixelRef* pixelRef = image->bitmap().pixelRef();
> +	   if (pixelRef->pixels()) {

I think you should check image->bitmap().pixels() here.  Once you've called
lockPixels(), pixels() should be non-zero.  You shouldn't have to poke around
into the pixelref to find out.

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:116
> +	   }

Not new to this patch, but it seems really unfortunate to leave the pixels
locked here, and require the caller to pair up the lock/unlock.  IWBN to have
some kind of guard class that ensures they're always matched (ignore me if such
a thing already exists).