Closed Bug 461663 Opened 11 years ago Closed 11 years ago
16 bits colour channel values should not have lower bits zeroed
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 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).
Sounds sensible. This new patch does the whole.
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?(gavin.sharp) → review+
Here you are
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 a1.9.1b2=beltzner
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+
Status: NEW → RESOLVED
Closed: 11 years ago
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.