[Webkit-unassigned] [Bug 106842] adding support for DiscardablePixelRef for caching lazily decoded images
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jan 28 14:15:54 PST 2013
https://bugs.webkit.org/show_bug.cgi?id=106842
Stephen White <senorblanco at chromium.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #185049|review? |review+
Flag| |
--- Comment #85 from Stephen White <senorblanco at chromium.org> 2013-01-28 14:17:51 PST ---
(From update of attachment 185049)
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).
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list