[MASTER] Firefox 3 "set as Desktop background" does not work properly

Might as well take the bug... :-)

Sylvain, does this appear to be needed even after your crash fix? The patch is correct AFAICT, and does "the right thing" but I'm still wondering why it works in Fx2 on the same system without this patch. Either way, if it is needed, I think we should attempt to take it.

Yes it should be needed because it fixes another issue. It is not about crashing but rather how the Set Desktop Background is able to read the background color from Gconf. Here's some steps how I can reproduce the issue:
1) Open gnome-appearance-properties and set the Desktop background to a red Solid color for instance
2) Launch Fx2 or Fx3, right click on an image and choose "Set As Desktop Background"
3) Select "Center" so that you can see the color around the image in the small screen image.
4) Expected: should be red / Actual: grey color.

Now try to manually set the gconf key /desktop/gnome/background/primary_color to "#f00" and do the above steps again.

I just typed this, and now that I'm trying the second step on Fx3, I'm always getting a black background color on the monitor icon (even if I have #f00 in gconf) without an error in the console :-/. Might be yet another bug hiding there.

Comment on attachment 308501
Patch to support GConf's actual format

gavin, do the above comments address your issues?

Created attachment 308760
New GConf format, fix color computation

I found two other issues in nsGNOMEShellService: HexToNum was wrong for alphanumerical values and the bit shifting in HexToRGB is apparently not done in the correct direction. I also added support for the 9 sized color format and added a test. Now I'm not getting the black background issue I mentioned previously any more.

The attached patch will conflict with bug 420786 (for the test part). I'll update afterward.

Created attachment 309596
Nothing to see here...

Actually, I think we should do it this way. GDK has parse and to_string methods which we can use here. Not only does this cut out a lot of code, but it writes out to the format that GNOME and GConf expects. The math is slightly more tricky, though, since GDK has 16 bits per channel, so we need to do an additional shift. Macros to the rescue, though.

Created attachment 309597
Let GDK deal with it's own colors

Sigh, the correct patch this time...

That latest patch looks like a better approach. Does it still pass the tests?

Created attachment 309807
added tests to Christopher's patch

Yes, this looks like a better way. I had to adapt the tests because unspecified bits take other values when using gdk. I also added tests for the other direction shell -> gconf.

Hmm, I'm just seeing now in the API of gdk_color_to_string (): Since 2.12. But configure.in says: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/configure.in&rev=1.1959&mark=120#120 that seems bad :-/

If support for this was only added in 2.12, we can't take this patch. If you want, you can support everything that's in 2.10 normally and then use gtk_check_version() to conditionally support the 2.12 stuff, but we cannot support any gtk changes after 2.10 as required.

ok, gdk_color_parse is in 2.10 so a large part of the patch is still fine. ColorToHex should be kept instead of using gdk_color_to_string().

Meh. Right. It was late and I somehow confused gdk as part of glib, which had a req of 2.12....

I'll post a new patch tonight to check the version at compile time. I'd rather use the GDK codepath where possible to support future versions that may or may not change the way things are displayed as strings, especially since we know that GdkColor's fields won't be changing since it needs to maintain compat; thus we can rely on it being our intermediary here though we can't rely on the strings.

Created attachment 312156
Updated to check GTK+ version

Hm, I thought I attached this already...

Comment on attachment 312156
Updated to check GTK+ version

moving review from gavin to mconnor.

Comment on attachment 312156
Updated to check GTK+ version

>diff -up mozilla/browser/components/shell/src/nsGNOMEShellService.cpp.gconf mozilla/browser/components/shell/src/nsGNOMEShellService.cpp

>+ColorToCString(PRUint32 aColor, nsCString& aResult)

>+ gchar *colorString = gdk_color_to_string(&color);
>+ aResult.Adopt(colorString);
>+ g_free(colorString);

Shouldn't you copy the string here?

Looks fine otherwise. Make sure to land the test too?

caillon, I guess this patch just fell through the cracks, or do you need help landing it?

Created attachment 334243
Christopher's version with test and Assign

(In reply to comment #17)
> >+ gchar *colorString = gdk_color_to_string(&color);
> >+ aResult.Adopt(colorString);
> >+ g_free(colorString);
>
> Shouldn't you copy the string here?

yeah, adopting doesn't look right according to the comment in ./string/public/nsXPCOMStrings.h:
  /* Data passed into NS_StringContainerInit2 is not copied; instead, the
   * string takes ownership over the data pointer. The caller must have
   * allocated the data array using the XPCOM memory allocator (nsMemory).
   * This flag should not be combined with NS_STRING_CONTAINER_INIT_DEPEND. */
  NS_STRING_CONTAINER_INIT_ADOPT = (1 << 2),

And I'm getting glibc double free errors with it. I've replaced it with Assign and added the tests.

pushed to mozilla-central: https://hg.mozilla.org/mozilla-central/index.cgi/rev/dd6d74c34c8f

Backed out due to unit-test failure:
*** TEST-UNEXPECTED-FAIL | ../../../../_tests/xpcshell-simple/test_browser_shell/unit/test_421977.js | #000000 == #000000000000

The #if/#else in ColorToCString does different things on different GTK versions. In particular, gdk_color_to_string is documented as returning #rrrrggggbbbb while the sprintf is using %02 formats. Hence the test failure: on tinderbox we're taking the #else branch, I would guess.

Created attachment 334578
fix test failure by using #rrrrggggbbbb everywhere

thanks for the investigation. This new version uses the 48bit format in both cases, which should fix the test failure.

checkin-needed, then?

(In reply to comment #24)
> checkin-needed, then?

Yes, it should be

http://hg.mozilla.org/mozilla-central/rev/6860b99699e9

*** Bug 437292 has been marked as a duplicate of this bug. ***

*** Bug 454929 has been marked as a duplicate of this bug. ***

Comment on attachment 334578
fix test failure by using #rrrrggggbbbb everywhere

Requesting approval for 1.9.0.4. We are using this patch downstream in Ubuntu now.
See https://bugs.launchpad.net/ubuntu/+source/firefox-3.0/+bug/206191

Comment on attachment 334578
fix test failure by using #rrrrggggbbbb everywhere

Doesn't appear to match security update release criteria, not approving, 3.1 will be out soon enough.

Comment on attachment 334578
fix test failure by using #rrrrggggbbbb everywhere

Shouldn't the expected results be
>+ checkShellToGConfColor("#000000", "#000000000000");
>+ checkShellToGConfColor("#0000FF", "#00000000ffff");
>+ checkShellToGConfColor("#FFFFFF", "#ffffffffffff");
>+ checkShellToGConfColor("#0A0B0C", "#0a0a0b0b0c0c");
>+ checkShellToGConfColor("#A0B0C0", "#a0a0b0b0c0c0");
>+ checkShellToGConfColor("#AABBCC", "#aaaabbbbcccc");

instead of
>+ checkShellToGConfColor("#000000", "#000000000000");
>+ checkShellToGConfColor("#0000FF", "#00000000ff00");
>+ checkShellToGConfColor("#FFFFFF", "#ff00ff00ff00");
>+ checkShellToGConfColor("#0A0B0C", "#0a000b000c00");
>+ checkShellToGConfColor("#A0B0C0", "#a000b000c000");
>+ checkShellToGConfColor("#AABBCC", "#aa00bb00cc00");

??

*** Bug 461657 has been marked as a duplicate of this bug. ***

(In reply to comment #31)
> (From update of attachment 334578 [details])
> Shouldn't the expected results be
(...)
> >+ checkShellToGConfColor("#AABBCC", "#aaaabbbbcccc");
>
> instead of
(...)
> >+ checkShellToGConfColor("#AABBCC", "#aa00bb00cc00");
>
> ??

In which case replacing
#define COLOR_8_TO_16_BIT(_c) ((_c) << 8)
with
#define COLOR_8_TO_16_BIT(_c) ((_c) << 8 | (_c))
would be enough.

PR_snprintf(buf, 14, "#%02x00%02x00%02x00", red, green, blue);
would also need to be changed to
PR_snprintf(buf, 14, "#%02x%02x%02x%02x%02x%02x", red, red, green, green, blue, blue);

Mike: could you file a new bug for that and CC the relevant people from this one?

Filed bug #461663