421977 - nsGNOMEShellService::GetDesktopBackgroundColor should support GConf's actual format

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[Sylvain Pasche]

Attachment: Patch to support GConf's actual format

This is a spin off from bug 420786 to address the change in GConf format.

I'm attaching caillon's patch from attachment 308420 [details] [diff] [review]

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

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.

Comment 3

[Sylvain Pasche]

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 4

[Reed Loden [:reed]]

Comment on attachment 308501 [details] [diff] [review]
Patch to support GConf's actual format

gavin, do the above comments address your issues?

Comment 5

[Sylvain Pasche]

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

Comment 6

[Sylvain Pasche]

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

Comment 7

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Let GDK deal with it's own colors

Sigh, the correct patch this time...

Comment 9

[:Gavin Sharp [email: [e-mail]]]

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

Comment 10

[Sylvain Pasche]

Attachment: 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 :-/

Comment 11

[Reed Loden [:reed]]

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.

Comment 12

[Sylvain Pasche]

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

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

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.

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Updated to check GTK+ version

Hm, I thought I attached this already...

Comment 16

[Alexander Sack]

Comment on attachment 312156 [details] [diff] [review]
Updated to check GTK+ version

moving review from gavin to mconnor.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 312156 [details] [diff] [review]
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?

Comment 18

[Adam Guthrie]

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

Comment 19

[Sylvain Pasche]

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

Comment 20

[Benjamin Smedberg]

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

Comment 21

[Benjamin Smedberg]

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

Comment 22

[Boris Zbarsky [:bzbarsky]]

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.

Comment 23

[Sylvain Pasche]

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

Comment 24

[:Gavin Sharp [email: [e-mail]]]

checkin-needed, then?

Comment 25

[Sylvain Pasche]

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

Yes, it should be

Comment 26

[Dão Gottwald [:dao]]

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

Comment 29

[Fabien Tassin]

Comment on attachment 334578 [details] [diff] [review]
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 30

[Daniel Veditz [:dveditz]]

Comment on attachment 334578 [details] [diff] [review]
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 31

[Mike Hommey [:glandium]]

Comment on attachment 334578 [details] [diff] [review]
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");

??

Comment 33

[Mike Hommey [:glandium]]

(In reply to comment #31)
> (From update of attachment 334578 [details] [diff] [review])
> 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.

Comment 34

[Mike Hommey [:glandium]]

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);

Comment 35

[:Gavin Sharp [email: [e-mail]]]

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

Comment 36

[Mike Hommey [:glandium]]

Filed bug #461663