Bug 35415 – [Chromium] Create WebKit API for WebCore::ImageDecoder

• Component: WebKit API
• Status: RESOLVED FIXED

[Kavita Kanetkar]

Linked to Chromium issue: Issue 28063. Create WebKit API wrapper for ImageDecoder to be used by Chromium.

[Kavita Kanetkar]

Created attachment 49665 [details]
proposed patch

Link to corresponding Chromium bug:
http://code.google.com/p/chromium/issues/detail?id=28063

[Yaar Schnitman]

LG, but I'm not a WK reviewer. Darin?

[Adam Barth]

Comment on attachment 49665 [details]
proposed patch

We usually document methods on the WebKit API.  I know is a bit outside the norm for most WebKit code, but it's helpful to have a one or two line comment about each function in the API.

[Kavita Kanetkar]

Created attachment 49734 [details]
patch2

Added comments in the .h file for the API.

[Darin Fisher (:fishd, Google)]

Comment on attachment 49734 [details]
patch2

> 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.

[Kavita Kanetkar]

Created attachment 49962 [details]
Addressed comments

Thanks for the review. 

Also, WebImageDecoder::getImage() handles #if WEBKIT_USING_SKIA  and #if WEBKIT_USING_CG
Do I need to worry about others?

[Kavita Kanetkar]

Created attachment 49966 [details]
minor fix.

[WebKit Review Bot]

Attachment 49962 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/331107

[Kavita Kanetkar]

Created attachment 49971 [details]
patch3

Correct version. Sorry for multiple uploads.

[Darin Fisher (:fishd, Google)]

Comment on attachment 49971 [details]
patch3

> 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.

[Kavita Kanetkar]

Created attachment 50123 [details]
patch 4


> 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.

I am using VS cpp compiler and the compiler didn't give me any warning about unimplemented switch case. Am I suppressing warnings some how?

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

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

The underlying WebCore::ImageDecoder's setData() was taking a pointer. Changed it now to take a &

Thanks,
Kavita.

[Darin Fisher (:fishd, Google)]

(In reply to comment #11)
> Created an attachment (id=50123) [details]
> patch 4
> 
> 
> > 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.
> 
> I am using VS cpp compiler and the compiler didn't give me any warning about
> unimplemented switch case. Am I suppressing warnings some how?

Ah, yeah... that's just something GCC will complain about.  It is pretty useful, and I wish VS would do the same.


> 
> > +    WEBKIT_API void setData(const WebData* data, bool allDataReceived);
> 
> normally, WebData is passed using a "const WebData&".  why make it a
> pointer instead?
> 
> The underlying WebCore::ImageDecoder's setData() was taking a pointer. Changed
> it now to take a &

OK, great.  thanks!

[WebKit Commit Bot]

Comment on attachment 50123 [details]
patch 4

Clearing flags on attachment: 50123

Committed r55618: <http://trac.webkit.org/changeset/55618>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.