Bug 30823 – Fix alpha handling when using wxWidgets 2.9

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit wx

[Vadim Zeitlin]

Created attachment 41955 [details]
Patch fixing alpha transparency handling for wx port

Transparency in PNG images didn't work in wxWebKit because the wxBitmap "has alpha" internal flag was not set before wxGraphicsBitmap was created from it. This flag is set in wxPixelData dtor and, generally speaking, bitmap data can't be accessed at all while it is locked for the raw access via wxPixelData so we must ensure that the local "data" variable is destroyed before using bitmap in any way.

This patch does just this: in spite of many apparent changes it simply adds a block around "data" variable to ensure that it is destroyed (this should be more clear if the patch is applied and "diff -b" is done).

[Vadim Zeitlin]

Sorry, I forgot to mention that this patch was done on top of bug 30822. However even if 30822 is not applied, this one should still be as it fixes a real user-visible bug. Please let me know if I should redo the patch against the base line (OTOH it basically just adds two lines with "{" and "}" so maybe there is no need for this).

[Eric Seidel (no email)]

Comment on attachment 41955 [details]
Patch fixing alpha transparency handling for wx port

Your ChangeLog does not make it clear what this patch does.

I believe webkit style is to use spaces around operators:
+            p.Red() = bytes[i+2];
+            p.Green() = bytes[i+1];
+            p.Blue() = bytes[i+0];
+            p.Alpha() = bytes[i+3];

[Vadim Zeitlin]

(In reply to comment #2)
> (From update of attachment 41955 [details])
> Your ChangeLog does not make it clear what this patch does.

I've tried to make it more precise while still remaining brief. I hope it's more clear now.
 
> I believe webkit style is to use spaces around operators:
> +            p.Red() = bytes[i+2];
> +            p.Green() = bytes[i+1];
> +            p.Blue() = bytes[i+0];
> +            p.Alpha() = bytes[i+3];

Sorry, I just indented this code, I didn't modify it at all. I've reformatted it now and will attach the new patch in a moment.

[Vadim Zeitlin]

Created attachment 41997 [details]
Patch fixing alpha transparency handling for wx port (v2)

[Kevin Ollivier]

Hi Vadim,

The patch itself does resolve the problem, but this patch won't apply cleanly because you made the patch on top of a branch that has your changes to NativeImagePtr in it. If you submit a fixed patch against stock trunk sources, I'll be happy to r+ and land it.

[Vadim Zeitlin]

Created attachment 42682 [details]
"Standalone" version of the patch 

Here is the version of the patch which should apply cleanly. As I wrote, it's completely trivial: it just adds a scope (and also spaces around "+" operator as requested although IMO this shouldn't be applied together with this patch at all as it has nothing to do with it...).

[Kevin Ollivier]

Landed in r50677, thanks! Note that the patch didn't conform to WebKit style since the code inside the braces was not indented, but as I wanted to get this landed, I fixed the issue before commit this time. However, most reviewers will r- a patch that doesn't conform, so things will go quicker if you make sure style issues are addressed before submission.