[webkit-reviews] review denied: [Bug 35783] [GStreamer] Use ImageBuffer API to do painting : [Attachment 50222] proposed patch

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 35783: [GStreamer] Use ImageBuffer API to do painting
https://bugs.webkit.org/show_bug.cgi?id=35783

Attachment 50222: proposed patch
https://bugs.webkit.org/attachment.cgi?id=50222&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
 49	private:
 50	    cairo_surface_t* m_surface;
 51	    RefPtr<BitmapImage> m_image;
 52 
 53	};

Drop the needless line.

 1172	  bool success;
 1173	  ImageGStreamer gstImage(m_buffer, &success);

I don't like this. It looks very non-idiomatic. I would prefer to have the
createClass pattern used in, say, ScrollBar. That means create a static
function that instantiates the object, and does whatever
checking/initialization is needed, then return the object or NULL. You can
decide on success based on the return value being NULL or not. Also, you seem
to be leaking the image, aren't you? Should it be OwnPtr?

 1180	  bool useLowQualityScale = false;
 1181	  context->drawImage(reinterpret_cast<Image*>(gstImage.image().get()),
styleColorSpace,
 1182			     rect, op, useLowQualityScale);

This is not very readable. You're saying 'Do not use low quality scale' by
passing a 'useLowQualityScale' variable =D. I understand it's a boolean, of
course, but I got confused reading this. I'd drop the variables, give values
directly.