[Webkit-unassigned] [Bug 106842] adding support for DiscardablePixelRef for caching lazily decoded images

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=106842





--- Comment #18 from Hin-Chung Lam <hclam at google.com>  2013-01-17 15:21:26 PST ---
(From update of attachment 183223)
View in context: https://bugs.webkit.org/attachment.cgi?id=183223&action=review

> Source/Platform/chromium/public/Platform.h:-160
> -

nit: Don't remove this line.

> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:65
> +    return m_discardable;

Is it possible that after this is allocated that it is immediately discard before lock? I think allocateAndLock has to be one operation.

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

nit: No need to add empty lines.

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:114
> +    if (m_discardableMemoryEnabled && !(*cachedImage)->bitmap().pixelRef()) {

Is this correct? I think SkBitmap always has a pointer to SkPixelRef but SkBitmap::getPixels() can return 0 because lock on SkPixelRef failed.

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:115
> +        removeCacheEntry(cacheEntry);

I believe you have to unlock the SkBitmap no matter what, before you can remove it.

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:-114
> -

nit: Don't remove empty lines.

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h:76
> +    bool discardableMemoryEnabled() { return m_discardableMemoryEnabled; }

I would like to see some unit tests, even if it is just for Android.

> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:-202
> -

nit: Don't remove empty lines.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.