Bug 30822 – Change NativeImagePtr definition for wx port

• Component: WebKit wx
• Status: RESOLVED WONTFIX

[Vadim Zeitlin]

Created attachment 41954 [details]
Patch changing NativeImagePtr definition for wx port

This patch changes NativeImagePtr definition from a pointer to wx[Graphics]Bitmap to just the object itself for wx port. The reason for doing this is that these classes are already smart pointer-like in wx and it just doesn't make sense to have another layer of indirection and extra heap allocations when using them -- they're supposed to be passed around by value.

I didn't try to build any other ports after this change but I hope to not having broken them as the definitions of the types didn't change for them so the existing code should continue to work and the changes to common, port-independent, parts were tested as part of wxWebKit rebuild.

Finally, this is, of course, hardly a critical issue and it does have some drawbacks: for one, I don't know if making a type with "Ptr" name not a pointer is acceptable. Second, the common code should take care to use "NativeImagePtr()" instead of "NULL" when initializing invalid native image objects and use the new "IsValidNativeImagePtr()" function for testing them instead of just comparing with "NULL" (although the latter can still be done in port-specific code for ports other than wx, of course). But I still hope this patch can be applied because allocating these objects on the heap is just unnecessary and the code doing it is painful to read.

TIA!

[Kevin Ollivier]

While this change brings the code more in line with how wx expects the objects to be used, as a consequence it (as you mentioned)  makes the Ptr a misnomer at times and worse makes it an object which can be a pointer on some ports and sometimes not (confusing for people who are kind enough to try and update wx port when they make changes that affect it), and ideally requires code changes to some or all of the other ports to adapt to using NativeImagePtr() and realistically should be tested by other ports.

Given all that, I think it's best to just stick with the current system of allocating on the heap to keep things simple and straightforward.

[Vadim Zeitlin]

I didn't measure this (it's difficult to see how to do it even remotely accurately) but I'd expect the extra heap allocations to have noticeable effect on performance so this change has more merits than just making the code more sensible.

I'd also like to mention that CE port already uses a class (smart pointer class but still) for this type so wx code wouldn't be the only exception. Of course, there is the difference that CE NativeImagePtr can still be implicitly constructed from NULL and converted to bool but realistically failing to use NativeImagePtr() instead of NULL in common code can't remain unnoticed for long as it would immediately break wxWebKit build.

Anyhow, I wonder if it's worth measuring performance gain from this change or if it's not going to change your decision in any case?

[Kevin Ollivier]

(In reply to comment #2)
> I didn't measure this (it's difficult to see how to do it even remotely
> accurately) but I'd expect the extra heap allocations to have noticeable effect
> on performance so this change has more merits than just making the code more
> sensible.
> 
> I'd also like to mention that CE port already uses a class (smart pointer class
> but still) for this type so wx code wouldn't be the only exception. Of course,
> there is the difference that CE NativeImagePtr can still be implicitly
> constructed from NULL and converted to bool but realistically failing to use
> NativeImagePtr() instead of NULL in common code can't remain unnoticed for long
> as it would immediately break wxWebKit build.
> 
> Anyhow, I wonder if it's worth measuring performance gain from this change or
> if it's not going to change your decision in any case?

I'm always interested in comparing performance of different approaches, if you feel there's a real, significant performance gain to be had here. Note though that the average web page probably will not have more than a couple dozen images attached to it, though, so even though heap allocation is more expensive, I'm not sure that we're doing it so often that the hits will add up to anything really noticeable. Also, the tests of this should probably happen once we get the fastMalloc issue resolved, as obviously that affects performance as well.