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.
Created attachment 173775 [details] Patch
Stephen: do you get a chance to look at this change? We can meet offline too if you have questions for this change.
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"
Created attachment 174490 [details] Patch
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.
Comment on attachment 174490 [details] Patch Looks good. r=me
Comment on attachment 174490 [details] Patch Clearing flags on attachment: 174490 Committed r134822: <http://trac.webkit.org/changeset/134822>
All reviewed patches have been landed. Closing bug.