[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 bugzilla-daemon at webkit.org
Wed Oct 22 18:04:35 PDT 2008

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

Attachment 24580: Patch

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

More information about the webkit-reviews mailing list