[webkit-reviews] review granted: [Bug 94240] [chromium] Implement deferred image decoding for WebKit Chromium : [Attachment 169363] Patch

[bugzilla-daemon at webkit.org]

Stephen White <senorblanco at chromium.org> has granted Hin-Chung Lam
<hclam at google.com>'s request for review:
Bug 94240: [chromium] Implement deferred image decoding for WebKit Chromium
https://bugs.webkit.org/show_bug.cgi?id=94240

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

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


I still find this design more confusing than it needs to be, although perhaps
I'm just not getting it.  It seems you've just pushed the imageID problem to
another class.	Instead of a unique ID, perhaps you could consider using a hash
of the original SkBitmap (or perhaps the NativeImageSkia) and the requested
size?  This might help you avoid the O(N) lookups in the frame cache as well,
and allow you to get rid of the static s_nextImageId, which I think is going to
be problematic for impl-side playback.

All that said, it is behind a flag, and I don't want to slow you down, so I'm
setting this r+ for now.  You can consider my bikeshedding suggestions of names
for a future patch.

> Source/WebCore/ChangeLog:76
> +	   ImageFrameCache
> +
> +	   A container for a scaled image fragment. In addition to bitmap
pixels
> +	   it contains information about the ID of the image, scale and
clipping.

This is not really a cache, but more of a cache entry, no?  It could be named
ImageFrameCacheEntry, but I find that a bit unwieldy.  ImageFragment, or
ScaledImageFragment, maybe?

> Source/WebCore/ChangeLog:81
> +	   ImageFrameGenerator
> +
> +	   This object is responsible for generating decoded pixels. It is also

> +	   a container for encoded image data and corresponding image decoder.

Would it be strange to call this class ImageFrameDecoder?  Generator is kind of
a generic name.

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h:79
> +    HashMap<int, OwnPtr<ImageFrameGenerator> > m_frameGenerators;

Nit:  This appears to be unused.

> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:46
> +    m_size = SkISize::Make(m_decoder->size().width(),
m_decoder->size().height());

Is this always the original size of the bitmap?  If so, perhaps this could be
named m_fullSize instead to make it clear.  Just a suggestion, though.