Bug 102019

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.