Closed Bug 461663 Opened 11 years ago Closed 11 years ago

16 bits colour channel values should not have lower bits zeroed


(Firefox :: Shell Integration, defect)

Not set



Firefox 3.6a1


(Reporter: glandium, Assigned: glandium)


(Keywords: fixed1.9.1)


(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
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).
Attachment #344767 - Flags: review?(caillon)
Attachment #344767 - Flags: review?(
Assignee: nobody → mh+mozilla
Comment on attachment 344767 [details] [diff] [review]

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).
Attached patch patch v2 (obsolete) — Splinter Review
Sounds sensible. This new patch does the whole.
Attachment #344767 - Attachment is obsolete: true
Attachment #348022 - Flags: review?(
Attachment #344767 - Flags: review?(
Attachment #344767 - Flags: review?(caillon)
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!
Attachment #348022 - Flags: review?( → review+
Attached patch patch v3Splinter Review
Here you are
Attachment #348022 - Attachment is obsolete: true
Attachment #348149 - Flags: approval1.9.1?
Attachment #348149 - Flags: approval1.9.1b2?
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 on attachment 348149 [details] [diff] [review]
patch v3

Attachment #348149 - Flags: approval1.9.1b2? → approval1.9.1b2+
Attachment #348149 - Flags: approval1.9.1b2+ → approval1.9.1b2-
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.
Attachment #348149 - Flags: approval1.9.1? → approval1.9.1+
Keywords: checkin-needed
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs 1.9.1 landing]
Target Milestone: --- → Firefox 3.6a1
You need to log in before you can comment on or make changes to this bug.