[webkit-reviews] review denied: [Bug 35415] [Chromium] Create WebKit API for WebCore::ImageDecoder : [Attachment 49734] patch2

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Kavita Kanetkar
<kkanetkar at chromium.org>'s request for review:
Bug 35415: [Chromium] Create WebKit API for WebCore::ImageDecoder
https://bugs.webkit.org/show_bug.cgi?id=35415

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> Index: WebKit/chromium/src/WebImageDecoder.cpp
...
> +using namespace WebCore;
> +
> +namespace WebKit {
> +
> +WebImageDecoder::~WebImageDecoder()
> +{
> +    WTF::OwnPtr<WebCore::ImageDecoder> ownedDecoder(m_decoder);

nit: no need for the WTF:: prefix or the WebCore:: prefix in .cpp files.


> +    ownedDecoder.clear();

you could also just call 'delete m_decoder;'


> +}
> +
> +WebImageDecoder* WebImageDecoder::create(ImageDecoderType type)
> +{
> +    WebImageDecoder* webImageDecoder = 0;
> +    if (type == BMP)
> +	 webImageDecoder = new WebImageDecoder(new BMPImageDecoder());
> +    else if (type == ICO)
> +	 webImageDecoder = new WebImageDecoder(new ICOImageDecoder());

nit: indentation is wrong


> +void WebImageDecoder::setData(const WebData* data, bool allDataReceived)
> +{
> +    RefPtr<WebCore::SharedBuffer>
buffer(WebCore::SharedBuffer::create(data->data(), data->size()));
> +    m_decoder->setData(buffer.get(), allDataReceived);
> +}
> +
> +bool WebImageDecoder::isFailed() const
> +{
> +    return m_decoder->failed();
> +}

^^^ here and elsewhere, please add ASSERT(m_decoder) before dereferencing.


> +bool WebImageDecoder::isFrameCompleteAtIndex(int index) const
> +{
> +    WebCore::RGBA32Buffer* const frameBuffer =
m_decoder->frameBufferAtIndex(index);
> +    if (!frameBuffer)
> +	 return false;
> +    else
> +	 return (frameBuffer->status() ==
WebCore::RGBA32Buffer::FrameComplete);
> +}
> +
> +WebImage WebImageDecoder::getImage(int index = 0) const
> +{
> +    WebCore::RGBA32Buffer* const frameBuffer =
m_decoder->frameBufferAtIndex(index);
> +    if (!frameBuffer)
> +	 return WebImage();
> +    else 
> +#if WEBKIT_USING_SKIA
> +	 return WebImage(*(frameBuffer->asNewNativeImage()));
> +#elif WEBKIT_USING_CG
> +	 return WebImage(frameBuffer->asNewNativeImage());
> +#endif

^^^ indentation is wrong, here and elsewhere


> Index: WebKit/chromium/ChangeLog
...
> +	   Create WebKit API for  WebCore::ImageDecoder
> +	   https://bugs.webkit.org/show_bug.cgi?id=35415
> +
> +	   Related to Chromium bug:
http://code.google.com/p/chromium/issues/detail?id=28063

^^^ nit: we usually don't reference the chromium bugs here.  the webkit
bug should be enough.


> +
> +	   * WebKit.gyp:
> +	   * public/WebImageDecoder.h: Added.
> +	   (WebKit::WebImageDecoder::):
> +	   (WebKit::WebImageDecoder::WebImageDecoder):
> +	   * src/WebImageDecoder.cpp: Added.
> +	   (WebKit::WebImageDecoder::~WebImageDecoder):
> +	   (WebKit::WebImageDecoder::create):
> +	   (WebKit::WebImageDecoder::setData):
> +	   (WebKit::WebImageDecoder::isFailed):
> +	   (WebKit::WebImageDecoder::isSizeAvailable):
> +	   (WebKit::WebImageDecoder::frameCount):
> +	   (WebKit::WebImageDecoder::isFrameCompleteAtIndex):
> +	   (WebKit::WebImageDecoder::getImage):

^^^ nit: when adding new files, it is customary to remove the lines
that mention the functions.


> Index: WebKit/chromium/public/WebImageDecoder.h

> +#ifndef WebImageDecoder_h
> +#define WebImageDecoder_h
> +
> +#include "WebCommon.h"
> +#include "WebImage.h"
> +
> +namespace WebCore { class ImageDecoder; }
> +
> +namespace WebKit {
> +
> +class WebData;
> +
> +class WebImageDecoder {
> +public:
> +
> +virtual ~WebImageDecoder();

^^^ fix indentation

also this destructor should not be virtual since there are no other
virtual methods declared on this class.


> +
> +    enum ImageDecoderType {
> +	   BMP = 1,
> +	   ICO = 2
> +    };

This enum should just be named Type since it is already in the
namespace WebImageDecoder.  The values should be TypeBMP and
TypeICO.  That way code that uses these will read like this:
WebImageDecoder::TypeBMP.


> +    // Caller owns the returned pointer.
> +    WEBKIT_API static WebImageDecoder* create(ImageDecoderType type);

I think this should just be a constructor.  Like WebImage, the constructor
should just be inline and it should call an "init" method that is decorated
with WEBKIT_API.  The destructor should call a "reset" method that is similarly

decorated with WEBKIT_API.  (This way our DLL would not export constructor and
destructor methods.)


> +
> +    // Returns true if image decoding failed.
> +    WEBKIT_API bool isFailed() const;
> +
> +    // Sets data contents for underlying decoder. 
> +    WEBKIT_API void setData(const WebData* data, bool allDataReceived);

it seems like most of the methods on this class are not useful
until the user has called setData.  i would list setData just
below create since you have to call setData after create before
calling any other method.


> +
> +    // Returns true if size information is available for the decoder.
> +    WEBKIT_API bool isSizeAvailable() const;

It seems strange for this API to provide this method but it doesn't
provide a way to actually read the size of the image.


> +
> +    // Gives frame count for the image. For multiple frames, decoder scans
the image data for the count.
> +    WEBKIT_API size_t frameCount() const;
> +
> +    // Returns if the frame at given index is completely decoded.
> +    WEBKIT_API bool isFrameCompleteAtIndex(int index) const;
> +
> +    // Creates and returns WebImage from buffer at the index.
> +    WEBKIT_API WebImage getImage(int index) const;
> +
> +private:
> +    typedef WebCore::ImageDecoder WebImageDecoderPrivate;

since this is declared in the WebImageDecoder namespace, it is not
necessary for its typename to also start with WebImageDecoder.
alternatively, you can just move this typedef to the WebKit namespace,
like we do elsewhere (e.g., see WebString.h).


> +    WebImageDecoder(WebImageDecoderPrivate* imageDecoder) :
m_decoder(imageDecoder) { }
> +
> +    WebImageDecoderPrivate* m_decoder;

m_decoder -> m_private to match conventions used elsewhere in the
API.

Since WebCore::ImageDecoder is not reference counted, you should
make sure that WebImageDecoder cannot be copied using the copy
constructor or assignment operator.