Clean up ImageBuffer.h so there do not have to be separate ifdefs for each platform

• Component: Platform
• Status: RESOLVED FIXED

[Brett Wilson (Google)]

ImageBuffer.h should be refactored so that the big list of ifdefs at the bottom of the file are unnecessary.

[Brett Wilson (Google)]

Created attachment 24580 [details]
Patch

Move platform-specific stuff out of ImageBuffer and into a separate per-platform file. This should make everything a little cleaner and nicer for everybody, and allows ports to make changes without having to change these shared files.

[Sam Weinig]

Comment on attachment 24580 [details]
Patch

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

[Brett Wilson (Google)]

(In reply to comment #2)
> (From update of attachment 24580 [details] [edit])
> -        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.

I just thought it was an error. The constructor and create() both take the argument by reference, and this is certainly what I'm used to from other places in the code like GraphicsContext.h. It seems like argument passing conventions should match the return type conventions for this kind of thing. I can change them all to by-value if you want.


> +        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.

I did it this way because create() is supposed to return null if the bitmap couldn't be created. This means that the subset of platform-depenedent work that can fail must happen in create(). Given I wanted to have the same constructor for all platforms, the only option that I can think of is to create the platform-dependent data outside of the constructor and pass it in.

[Brett Wilson (Google)]

I suppose alternatively InageBuffer could somehow have an error state that ::create notices, deletes it, and returns NULL. I'm not sure this is better than what i did here, however.

[Sam Weinig]

My main concern is with the unnecessary allocations that this model requires.  I think you could probably just pass the PlatformImageBuffer by value to the ImageBuffer constructor.

[Brett Wilson (Google)]

Passing by value will create a copy in ImageBuffer. This is possible for CG, but  not for some other platforms. Even for CG, this would require extra work to transfer ownership of the void* pointer in the PlatformImageBuffer to the copy in the object.

Worrying about one more malloc when you're allocating a potentially multi-megabyte bitmap doesn't seem worthwhile to me.

The other option is my suggestion in comment 4. Are you happy with that?

[Sam Weinig]

I don't think an extra new/delete is a good idea when not needed.  There is much smaller overhead to copying the address of a pointer (and potentially an integer) than there is to a malloc.  In all, this change seems to complicate the code and add more calls to the allocator than necessary, both of which we should try and avoid.

[Brett Wilson (Google)]

You still have not answered whether you are happy with my proposal in comment 4 which avoids this.

[Sam Weinig]

It depends on how much complexity it adds.  It doesn't seem ideal and but requiring a delete in the error case, which should not be often would not be the end of world.  I would prefer a solution that did not require it.

[Brett Wilson (Google)]

Created attachment 24624 [details]
Patch v2

This adds a flag to ImageBuffer that create can use to check if it should return true or not. This has the nice effect of making create() cross-platform, so I moved it into the header file. I think the platform-specific code is also more clear as a result, you just have to set the flag properly and you don't have to worry about writing create and returning an enpty auto_ptr.

I left IntSize passed by reference for consistency. I can still change all uses of IntPoint in this class to by-value instead, but that seems less consistent with the rest of the project to me.

[Darin Adler]

Comment on attachment 24624 [details]
Patch v2

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.

[Brett Wilson (Google)]

The reason I added the m_initialized flag is because weinig didn't like the way create created the PlatformImageBuffer and passed it into the constructor. These are the only two reasonable ways to do it.

Passing the PlatformImageBuffer by value is totally unacceptable because for some platforms there will be large amounts of data tied to it. I'm not sure weinig understood this so I will try to make it more clear. In the Qt version, the PlatformImageBuffer has a QPixmap. Copying this structure would imply copying a large pixmap for no effect. It would also imply that we have some kind of pointer ownership transfer to avoid copying the QPainter which would make it much more complicated.

Our Skia port will look very much like the Qt version, where it would be unacceptable to copy PlatformImageBuffer as weinig suggested twice.

If you have an approach that (1) never copies PlatformImageBuffers, (2) never does an extra malloc in create and passes it into the constructor, (3) never uses an extra flag, and (4) has the same constructors for each platform, I'm happy to write it. But I don't know how to do that.

[Brett Wilson (Google)]

Maybe I missed what you were suggesting. You're suggesting that we kill create() and have ImageBuffer have an out parameter that indicates success to the callers?

That seems really weird to me, it doesn't seem like C++ normally works that way, but I'm okay writing it if that's what you want.

[Brett Wilson (Google)]

Created attachment 24695 [details]
Patch v3

The implements Darin's suggestion of deleting the ::create function and making the constructor use an out parameter. I had to change a lot of callers, some of which improved in clarity, some of which got slightly worse as a result of this change. The code in ImageBuffer*.cpp is much shorter.

This adds a one-line ifdef change to allow Skia ports to implement text drawing modes (the change in GraphicsContext.cc).

[Darin Adler]

(In reply to comment #13)
> Maybe I missed what you were suggesting. You're suggesting that we kill
> create() and have ImageBuffer have an out parameter that indicates success to
> the callers?

No, I wasn't suggesting that. Sorry, forgot to cc myself on the message.

I was suggesting that we keep create exactly as it was before, with platform-specific versions as necessary. The create function itself would return 0 if it failed.

My alternate suggestion, if you really feel that create needs to become cross-platform, would be to use an out parameter for the boolean success flag, making ImageBuffer constructors able to indicate failure uniformly at construction time without having to carry that state along for the rest of their lifetimes.

I'm sorry I didn't see this *before* you made a new patch. My fault entirely for not cc'ing myself on the bug!

[Brett Wilson (Google)]

Created attachment 24713 [details]
Patch v4

This is like the v2 patch but using an out parameter from the constructor to communicate with ::create that it hsould fail.

[Darin Adler]

Comment on attachment 24713 [details]
Patch v4

> +    success = false;  // Make early return mean failure.

If you wanted to match the semantics of your earlier patch you could have done this at the call site. I think it's fine the way it is, though.

> +    ASSERT((reinterpret_cast<size_t>(m_data.m_data) & 2) == 0);

You didn't write this code, but for the record and for else reading this patch, the correct type to use here is uintptr_t (or intptr_t) rather than size_t. And also, it's a very strange thing to test. Maybe the person writing it meant to and with 1 or with 3. I don't understand why it's testing only the 2 bit.

> +#include <QPainter>
> +#include <QPixmap>
> +
> +#include "OwnPtr.h"

We'd normally put these includes into a single paragraph, not two paragraphs.

r=me

[Adele Peterson]

I ran into an error building with Visual Studio.

"success" needs to be intialized in ImageBuffer.h.  I made this change, but I think we should consider taking out the other places where we set it to false now that its initialized to false.