[Webkit-unassigned] [Bug 21816] Clean up ImageBuffer.h so there do not have to be separate ifdefs for each platform

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=21816


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #24624|review?                     |review-
               Flag|                            |




------- Comment #11 from darin at apple.com  2008-10-24 11:01 PDT -------
(From update of attachment 24624)
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.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.