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

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied 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 165165: Patch
https://bugs.webkit.org/attachment.cgi?id=165165&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=165165&action=review


Sorry it's taken so long to get to this.  I've left a bunch of comments, mostly
about style and sticking to WebKit conventions.  There are also some layering
violations to address.

The one big design question I have is the design of ImageDecoderChromium.  It
closely mirrors ImageDecoder's interface and embeds an ImageDecoder but
otherwise has no relationship to ImageDecoder in the type system.  All of the
public functions on ImageDecoderChromium mimic public virtual functions on
ImageDecoder AFAICT.  What is the plan here?  Do you intend to make
ImageDecoderChromium a subclass of ImageDecoder?  If so, why doesn't this patch
do that?  If not, why does it look so closely related?

> Source/WebKit/chromium/ChangeLog:39
> +2012-09-07  Alpha Lam  <hclam at chromium.org>

you have a double changelog here

> Source/WebCore/page/Settings.h:435
> +#if PLATFORM(CHROMIUM)

if you have a setting that's only used in chromium, put it on
WebKit::WebSettings (which is chromium-specific) not WebCore::Settings (which
is not)

> Source/WebCore/platform/graphics/ImageSource.h:56
> +#if USE(SKIA) && PLATFORM(CHROMIUM)

USE(SKIA) && PLATFORM(CHROMIUM) is redundant. chromium always sets USE(SKIA)

> Source/WebCore/platform/graphics/chromium/ImageDecoderChromium.cpp:30
> +#include "Settings.h"

this is a layering violation.  Settings is a WebCore/page concept and this is
in WebCore/platform - the rule for /platform is it can only depend on WTF and
other things in WebCore/platform but shouldn't know about anything above that

> Source/WebCore/platform/graphics/chromium/ImageDecoderChromium.cpp:53
> +    ImageDecoder* actualDecoder = ImageDecoder::create(data, alphaOption,
gammaAndColorOption);
> +    if (!actualDecoder)
> +	   return 0;
> +    return new ImageDecoderChromium(actualDecoder);

use OwnPtr<> / PassOwnPtr<> for the ImageDecoder here, not raw. When using
WebCore objects it's extremely rare (and extremely suspicious) to be dealing
with raw pointers unless there is no ownership transfer taking place

> Source/WebCore/platform/graphics/chromium/ImageDecoderChromium.cpp:64
> +    if (m_actualDecoder.get())

why are we null checking m_actualDecoder in this function? it seems that it
can't be null, looking at the ::create()

> Source/WebCore/platform/graphics/chromium/ImageDecoderChromium.cpp:110
> +    } else {
> +	   ASSERT(!m_data.get());

i don't see how we could get into this branch

> Source/WebCore/platform/graphics/chromium/ImageDecoderChromium.cpp:119
> +    if (m_actualDecoder.get())

no .get() needed here or on any of the other m_actualDecoder null checks

> Source/WebCore/platform/graphics/chromium/ImageDecoderChromium.h:44
> +    // Used in test only. Ownership of decoder is transferred.
> +    static ImageDecoderChromium* create(ImageDecoder*);

If ownership is transfered encode that in the type i.e. use a
PassOwnPtr<ImageDecoder> instead of a raw one

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:63
> +    WTF::deleteAllValues(m_frameGenerators);
> +    WTF::deleteAllValues(m_frameCache);

instead of these, why not make m_frameGenerators and m_frameCache have
OwnPtr<>s as values?

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

we don't typically use this type of comment in WebKit code

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:92
> +SkBitmap
ImageDecodingStore::createLazyDecodedSkBitmap(WTF::PassOwnPtr<ImageDecoder>
decoder)

no WTF:: needed. <wtf/PassOwnPtr.h> has a using statement to put PassOwnPtr<>
in the global namespace - that's the common pattern for all WTF types

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:107
> +    bitmap.pixelRef()->setURI(labelLazyDecoded);

this seems odd - in what sense is "lazy" a URI?  why do pixelrefs have URIs?

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:123
> +    // Note: Because row bytes is fixed to SkBitmap this means we have to
scale the
> +    // entire image.

I can't quite follow what this comment is saying

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:199
> +    if (this == instanceOnMainThread() && WTF::isMainThread())

no WTF::

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:200
> +	   return true;

why not just "return this == instanceOnMainThread && isMainThread();" ?

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:204
> +void ImageDecodingStore::createFrameGenerator(WTF::PassOwnPtr<ImageDecoder>
decoder)

no WTF:: - I won't comment on the rest but please find+remove them all

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:208
> +    ImageFrameGenerator* generator = new ImageFrameGenerator(decoder);
> +    ++m_nextId;
> +    m_frameGenerators.add(m_nextId, generator);

what's the ownership of the ImageFrameGenerator?

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:237
> +	       delete m_frameCache[i];

if m_frameCache had OwnPtr values you wouldn't need this explicit delete. smart
pointers are great, use 'em!

> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:53
> +    if (m_decoder.get())

just if (m_decoder) - WTF smart pointers all have operator bool()s
implementations that do the right thing

> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.h:41
> +    ImageFrameGenerator(PassOwnPtr<ImageDecoder>);

explicit

> Source/WebCore/platform/graphics/chromium/LazyDecodingPixelRef.cpp:32
> +SK_DECLARE_GLOBAL_MUTEX(gLazyDecodingImageRefMutex);

this looks like it creates a static initializer and destructor. double-yuck! 
why do we have to use a skia macro here?

we don't name globals with "gFooBar" in webkit - if it's a static member
variable do s_fooBar, otherwise just fooBar

> Source/WebCore/platform/graphics/chromium/LazyDecodingPixelRef.h:39
> +class LazyDecodingPixelRef : public SkPixelRef {

SkPixelRef means it's implicit SkRefCnt'd.  Could we try to keep it in
SkAutoTUnref's when it's not owned by another object?

> Source/WebCore/platform/image-decoders/ImageDecoder.h:150
> +#if PLATFORM(CHROMIUM)
> +	   void setSkBitmap(const SkBitmap& bitmap)

it feels a bit weird to make ImageDecoder even more aware of skia, but i guess
it already knows about SkBitmap in a few ways