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

[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 49971: patch3
https://bugs.webkit.org/attachment.cgi?id=49971&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> Index: WebKit/chromium/src/WebImageDecoder.cpp
...
> +void WebImageDecoder::init(Type type)
> +{
> +    m_private = 0;
> +    if (type == TypeBMP)
> +	   m_private = static_cast<ImageDecoder*>(new BMPImageDecoder());
> +    else if (type == TypeICO)
> +	   m_private = new ICOImageDecoder();

nit: please use a switch/case above so that if the enum gets a new value,
the compiler will complain about an unimplemented case in the switch
statement.

why the static cast in the BMP case?  That shouldn't be necessary given
that BMPImageDecoder inherits from ImageDecoder.



> +WebSize WebImageDecoder::size() const
> +{
> +    ASSERT(m_private);
> +    return WebSize(m_private->size().width(), m_private->size().height());

WebSize has a constructor that takes an IntSize, so you should be able to
just write:

  return m_private->size();


> +bool WebImageDecoder::isFrameCompleteAtIndex(int index) const
> +{
> +    ASSERT(m_private);
> +    WebCore::RGBA32Buffer* const frameBuffer =
m_private->frameBufferAtIndex(index);

^^^ nit: no need for the "WebCore::" prefix

> +    if (!frameBuffer)
> +	   return false;
> +    return (frameBuffer->status() == WebCore::RGBA32Buffer::FrameComplete);

^^^ ditto


> +WebImage WebImageDecoder::getImage(int index = 0) const
> +{
> +    ASSERT(m_private);
> +    WebCore::RGBA32Buffer* const frameBuffer =
m_private->frameBufferAtIndex(index);

^^^ ditto


> Index: WebKit/chromium/public/WebImageDecoder.h
...
> +class WebImageDecoder : public WebNonCopyable {
> +public:
> +    enum Type {
> +	   TypeBMP = 1,
> +	   TypeICO = 2
> +    };

nit: no need to assign numbers to the enum values.  just let
the default 0 and 1 be used.


> +    WEBKIT_API void setData(const WebData* data, bool allDataReceived);

normally, WebData is passed using a "const WebData&".  why make it a
pointer instead?


> +    WEBKIT_API WebImage getImage(int index) const;

maybe this method should be called getFrameAtIndex for consistency with
the frameCount and isFrameCompleteAtIndex methods.