461663 - 16 bits colour channel values should not have lower bits zeroed

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[Mike Hommey [:glandium]]

Attachment: patch

Bug #421977 made nsGNOMEShellService::SetDesktopBackgroundColor set RGB channels with 16bit values to fit new GConf format.
On the other hand, the 8 lower bits for each channel is always zeroed out, which means, for instance, that the brighter value you can get is #ff00ff00ff00, which is not quite exactly white.
I think the 8 lower bits should be set so that they repeat the 8 higher bits.

The attached patch should be doing just that (but I must say I haven't actually tested it).

Comment 1

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

Comment on attachment 344767 [details] [diff] [review]
patch

Looks right to me, but seems like we should have both branches of GetDesktopBackgroundColor use COLOR_8_TO_16_BIT (e.g. by setting red/green/blue outside of the conditional), and just use %04x%04x%04x.

If we do that probably makes sense to just get rid of the call to gdk_color_to_string and use our compat code all the time, since as far as I can tell, all Gdk does is call into Pango, which itself just does a sprintf, and that overhead seems pointless (and gdk_color_to_string's behavior is pretty unlikely to change).

Comment 2

[Mike Hommey [:glandium]]

Attachment: patch v2

Sounds sensible. This new patch does the whole.

Comment 3

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

Comment on attachment 348022 [details] [diff] [review]
patch v2

>diff --git a/browser/components/shell/src/nsGNOMEShellService.cpp b/browser/components/shell/src/nsGNOMEShellService.cpp

>-  // The #rrrrggggbbbb format is used to match gdk_color_to_string()

Seems like we should leave this comment in to explain why we're using this format (or add documentation for the method in general).

>+  PRUint16 red = COLOR_8_TO_16_BIT(aColor >> 16);

I suppose it's probably unlikely to matter (and isn't really related to this patch), but wouldn't really hurt to & 0xFF this too just in case there are some bits hanging around up there that shouldn't be, and perhaps add an NS_ASSERTION(aColor <= 0xFFFFFF) to SetDesktopBackgroundColor().

Please also remove the gtkversion.h include.

Can you change "nsCString colorString;" to "nsCAutoString colorString;" in SetDesktopBackgroundColor() as well? Don't mean to scope creep this too much, but it's minor so I thought I'd throw it in :)

r=me with those. Thanks for the patch!

Comment 4

[Mike Hommey [:glandium]]

Attachment: patch v3

Here you are

Comment 5

[Reed Loden [:reed]]

Drivers, the bug that caused this by adding support for 16-bit color channel values didn't do it correctly, so this follow-up bug is needed to correct some issues.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 348149 [details] [diff] [review]
patch v3

a1.9.1b2=beltzner

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 348149 [details] [diff] [review]
patch v3

We're closing up for beta2 and this hasn't landed yet, so we'll hold off until after we branch.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 348149 [details] [diff] [review]
patch v3

a191=beltzner

Comment 9

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/d1dea23d31d9

Comment 10

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1bf0c63b3981