Bug 102019 – [chromium] Refactoring to move logic of creating lazy decoded SkBitmap into DeferredImageDecoder

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Hin-Chung Lam]

The goal is to make ImageDecodingStore be a simple image cache and does nothing more. 

This is the first step to refactor ImageDecodingStore and DeferredImageDecoder. The change involves moving the code and logic of creating a lazily decoded SkBitmap from ImageDecodingStore into DeferredImageDecoder. There should be no new code but just moving stuff around.

[Hin-Chung Lam]

Created attachment 173775 [details]
Patch

[Hin-Chung Lam]

Stephen: do you get a chance to look at this change? We can meet offline too if you have questions for this change.

[Stephen White]

Comment on attachment 173775 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173775&action=review

Looks fine; just some grammar/naming nits for the moved code.

> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:74
> +SkBitmap DeferredImageDecoder::resizeLazyDecodedSkBitmap(const SkBitmap& bitmap, const SkISize& scaledSize, const SkIRect& scaledSubset)

Grammar nit:  "lazy decoded" should either be "lazily decoded" or "lazy decoding".  The latter is probably preferable for consistency with the pixel ref.

Is this function actually doing the resizing?  It seems like it takes one LazyDecodingPixelRef and creates another one for the resized bitmap.  Does it then continue to defer decoding and resize until the first bitmap draw?  If so, maybe its name should reflect the fact that it doesn't actually do the resizing.  createResizedLazyDecodingBitmap()?  Seems awkward; maybe you can think of a better one.

If it does in fact resize, then the code is fine as-is.

> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:207
> +    // Use URI to identify this is a lazily decoded SkPixelRef of type LazyDecodingPixelRef.

nit:  Grammar is awkward.  Perhaps "Use URI to identify this is" -> "Use the URI to identify this as"

> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:208
> +    // FIXME: Give the actual URI is more useful.

nit:  "It would be more useful to give the actual image URI".

> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:216
> +    // do so safely if the image is fully loaded and it is a JPEG image, or image was

"or image" -> "or if the image"

[Hin-Chung Lam]

Created attachment 174490 [details]
Patch

[Hin-Chung Lam]

Comment on attachment 173775 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173775&action=review

>> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:74
>> +SkBitmap DeferredImageDecoder::resizeLazyDecodedSkBitmap(const SkBitmap& bitmap, const SkISize& scaledSize, const SkIRect& scaledSubset)
> 
> Grammar nit:  "lazy decoded" should either be "lazily decoded" or "lazy decoding".  The latter is probably preferable for consistency with the pixel ref.
> 
> Is this function actually doing the resizing?  It seems like it takes one LazyDecodingPixelRef and creates another one for the resized bitmap.  Does it then continue to defer decoding and resize until the first bitmap draw?  If so, maybe its name should reflect the fact that it doesn't actually do the resizing.  createResizedLazyDecodingBitmap()?  Seems awkward; maybe you can think of a better one.
> 
> If it does in fact resize, then the code is fine as-is.

Renamed to createResizedLazyDecodingBitmap.

>> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:207
>> +    // Use URI to identify this is a lazily decoded SkPixelRef of type LazyDecodingPixelRef.
> 
> nit:  Grammar is awkward.  Perhaps "Use URI to identify this is" -> "Use the URI to identify this as"

Done.

>> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:208
>> +    // FIXME: Give the actual URI is more useful.
> 
> nit:  "It would be more useful to give the actual image URI".

Done.

[Stephen White]

Comment on attachment 174490 [details]
Patch

Looks good.  r=me

[WebKit Review Bot]

Comment on attachment 174490 [details]
Patch

Clearing flags on attachment: 174490

Committed r134822: <http://trac.webkit.org/changeset/134822>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.