[webkit-reviews] review denied: [Bug 21816] Clean up ImageBuffer.h so there do not have to be separate ifdefs for each platform : [Attachment 24580] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Brett Wilson (Google)
<brettw at chromium.org>'s request for review:
Bug 21816: Clean up ImageBuffer.h so there do not have to be separate ifdefs
for each platform
https://bugs.webkit.org/show_bug.cgi?id=21816

Attachment 24580: Patch
https://bugs.webkit.org/attachment.cgi?id=24580&action=edit

------- Additional Comments from Sam Weinig <sam at webkit.org>
-	 IntSize size() const { return m_size; }
+	 const IntSize& size() const { return m_size; }
What is the reason for this change, the ChangeLog doesn't say?	On many systems
it is possible to pass an IntSize in a register. 

+	 mutable std::auto_ptr<PlatformImageBuffer> m_platform;
This shouldn't be an auto_ptr and should instead just be a member.  It could
also use a better name.  Perhaps  PlatformImageBuffer m_platformImageBuffer;

+	 ImageBuffer(std::auto_ptr<PlatformImageBuffer>, const IntSize&,
std::auto_ptr<GraphicsContext>);
This should not take auto_ptrs.  I don't even think it needs to take a
PlatformImageBuffer as a parameter.  It should just instantiate one itself.

The rest of the patch is based on the model above and thus should be changed as
well.  As such, I will only list the style issues from here on.


+class PlatformImageBuffer
+{
The { should go on the same line as the class name.  This is repeated a bunch.

+    unsigned int m_bytesPerRow;
Should just be unsigned.

+    QPainter painter() { return m_painter; }
This should probably return a QPainter*.

+    QPainter* m_painter;
This should be an OwnPtr.