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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> 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 24624: Patch v2
https://bugs.webkit.org/attachment.cgi?id=24624&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Thanks for taking this on. It's good to reduce the use of #if in the platform
layer.

I'm not sure it improves things to make the create function's body shared
cross-platform. It's a simple small function, and in return for making it
cross-platform we've added an unconventional creation idiom where there's a
boolean to express success. Is it really a good tradeoff?

On of the weakest things about the WebCore platform layer is all the different
ways the word "platform" is used and the different ways PlatformXXX is used. I
think it's strange to have a class ImageBuffer and another class
PlatformImageBuffer with such different roles; the second one is just a
structure to hold data members to make it possible to move them out of the
ImageBuffer.h header. Maybe the PlatformImageBuffer class should be called
ImageBufferData or something like that? I think a good way to name
PlatformImageBuffer would be to say a sentence out loud about what the class is
for and figure out what the key words are. Similarly, the name m_platform for a
member that has all the data in it isn't so great. I know it's
platform-specific data, but still, if the name is "platform" then it should be
"a platform" and it seems clear this is not a platform.

> +	    happens in the cunstructor which sets a flag that create uses to
know

Misspelling here.

> +	       if (buf->m_initialized)
> +		 return buf;

This return statement is indented two spaces but should be indented four.

> +	   // The ImageBuffer constructor should set this to true if
construction
> +	   // succeeded, and check the value in create() to see if the object
or
> +	   // a null pointer should be returned.
> +	   bool m_initialized;

If the constructor needs to return a value, we should use an "out parameter"
from the constructor; pass in a reference to a bool. It doesn't seem smart to
use a data member for the entire lifetime of the object just because we wanted
a way to return a value from the constructor.

> +    m_initialzed = true;

This typo means the Cairo version wouldn't compile.

I'm going to say review- because I'd like to see m_initialized be removed, but
this patch seems basically fine.