Closed Bug 421977 Opened 12 years ago Closed 12 years ago

nsGNOMEShellService::GetDesktopBackgroundColor should support GConf's actual format

Categories

(Firefox :: Shell Integration, defect)

Other
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 3.1b1

People

(Reporter: sylvain.pasche, Assigned: caillon)

References

Details

Attachments

(1 file, 6 obsolete files)

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]
Might as well take the bug... :-)
Assignee: nobody → caillon
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 [details] [diff] [review]
Patch to support GConf's actual format

gavin, do the above comments address your issues?
Attachment #308501 - Flags: review?(gavin.sharp)
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.
Attachment #308501 - Attachment is obsolete: true
Attachment #308760 - Flags: review?(gavin.sharp)
Attachment #308501 - Flags: review?(gavin.sharp)
The attached patch will conflict with bug 420786 (for the test part). I'll update afterward.
Attached patch Nothing to see here... (obsolete) — Splinter Review
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.
Sigh, the correct patch this time...
That latest patch looks like a better approach. Does it still pass the tests?
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 :-/
Attachment #309597 - Attachment is obsolete: true
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....
Hardware: PC → Other
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.
Status: NEW → ASSIGNED
Attached patch Updated to check GTK+ version (obsolete) — Splinter Review
Hm, I thought I attached this already...
Attachment #312156 - Attachment is patch: true
Attachment #312156 - Attachment mime type: application/octet-stream → text/plain
Attachment #312156 - Flags: review?(gavin.sharp)
Comment on attachment 312156 [details] [diff] [review]
Updated to check GTK+ version

moving review from gavin to mconnor.
Attachment #312156 - Flags: review?(gavin.sharp) → review?(mconnor)
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?
Attachment #312156 - Flags: review?(mconnor) → review+
Attachment #308760 - Attachment is obsolete: true
Attachment #308760 - Flags: review?(gavin.sharp)
Attachment #309807 - Attachment is obsolete: true
caillon, I guess this patch just fell through the cracks, or do you need help landing it?
(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.
Attachment #312156 - Attachment is obsolete: true
Keywords: checkin-needed
pushed to mozilla-central: https://hg.mozilla.org/mozilla-central/index.cgi/rev/dd6d74c34c8f
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Backed out due to unit-test failure:
*** TEST-UNEXPECTED-FAIL | ../../../../_tests/xpcshell-simple/test_browser_shell/unit/test_421977.js | #000000 == #000000000000
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
thanks for the investigation. This new version uses the 48bit format in both cases, which should fix the test failure.
Attachment #334243 - Attachment is obsolete: true
checkin-needed, then?
Keywords: checkin-needed
(In reply to comment #24)
> checkin-needed, then?

Yes, it should be
http://hg.mozilla.org/mozilla-central/rev/6860b99699e9
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1b1
Flags: in-testsuite+
No longer blocks: 437292
Duplicate of this bug: 437292
Duplicate of this bug: 454929
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
Attachment #334578 - Flags: approval1.9.0.4?
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.
Attachment #334578 - Flags: approval1.9.0.4? → approval1.9.0.4-
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");

??
Duplicate of this bug: 461657
(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.
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?
You need to log in before you can comment on or make changes to this bug.