[MASTER] Firefox 3 "set as Desktop background" does not work properly

[Christer Edwards]

Binary package hint: firefox-3.0

I navigated to a .jpg online and used right-click, set as desktop background to try and update my desktop wallpaper. Nothing actually happens. I can see the preview in the window for all the options (scale, stretch, center) but selecting the option to save the changes seems to ignore my input. Although, if you select a different color the background seems to apply properly, regardless of new or previous color selection.

ii firefox-3.0 3.0~b4+nobinon lightweight web browser based on Mozilla
2.6.24-12-generic

[Caillon (caillon)]

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

[Caillon (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.

[Sylvain Pasche (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.

[Reed Loden (reed)]

Comment on attachment 308501
Patch to support GConf's actual format

gavin, do the above comments address your issues?

[Sylvain Pasche (sylvain-pasche)]

Created attachment 308760
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.

[Sylvain Pasche (sylvain-pasche)]

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

[Caillon (caillon)]

Created attachment 309596
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.

[Caillon (caillon)]

Created attachment 309597
Let GDK deal with it's own colors

Sigh, the correct patch this time...

[Gavin Sharp (gavin-sharp)]

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

[Sylvain Pasche (sylvain-pasche)]

Created attachment 309807
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 :-/

[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.

[Sylvain Pasche (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().

[Caillon (caillon)]

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

[Caillon (caillon)]

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.

[Christer Edwards (christer.edwards)]

Binary package hint: firefox-3.0

I navigated to a .jpg online and used right-click, set as desktop background to try and update my desktop wallpaper. Nothing actually happens. I can see the preview in the window for all the options (scale, stretch, center) but selecting the option to save the changes seems to ignore my input.

ii firefox-3.0 3.0~b4+nobinon lightweight web browser based on Mozilla
2.6.24-12-generic

[Andrea Colangelo (warp10)]

I can confirm this bug on the same release (Hardy beta) and with the same firefox version.

[Doug-NSF (dbaggett)]

It's actually a bit stranger for me. When I choose "Set as Background Image" the window to set the background image comes up but the window is empty (Screenshot attached). Then when I change the option in that window from "Tile" (the image shows up in the window) and then back to "center" the image shows up like it should in the window, but still clicking the the "set Desktop background" button does not work.

Note that setting the background image via Appearances preferences does work fine.

[Christer Edwards (christer.edwards)]

Yeah, mine acts in a similar way. I do have to select one of the options before the graphic shows up.

Saving the graphic locally and then setting it via one of the local applications (gthumb) it works just fine..

[Gordon (masterlight)]

Here is my magical issue . . . <a href="http://asnd16.com/shiz/screenshots/noresponse.jpg"><img src="http://asnd16.com/shiz/screenshots/noresponse.jpg">http://asnd16.com/shiz/screenshots/noresponse.jpg</a>

[Charles Brophy (charlesbrophy)]

I can confirm this. You click the "Set as Background" button and its completely unresponsive.

[JD (jacobdorne)]

Exactly the same for me as Doug-NSF said above.

I'm using Ubuntu 8.04 beta with the latest updates until this post.

[Caillon (caillon)]

Created attachment 312156
Updated to check GTK+ version

Hm, I thought I attached this already...

[Pedro Villavicencio (pedro)]

thanks for your report.

[Alexander Sack (asac)]

works in 3.0b5

[Breno Leitão (breno-leitao)]

I' ve just installed Ubuntu 8.04 and I got the same problem described.
Firefox 3.0b5 doesn' t change my wallpaper, and also doesn't save the file as Firefox_wallpaper.png.

[Breno Leitão (breno-leitao)]

I' ve found that as soon as you click on " Set as Default Background", the next windows doesn' t show the image inside that monitor. Take a look at the screen shot.
If you click on "Center" in the position field, then everything goes fine.

[John Vivirito (gnomefreak)]

Can you please provide a link to a picture you tried to set as
background i would like to try this with that picture first than i will
see what i can come up with on why he thinks its fixed, it could be the
pics your using but i would need one or more (as many as possible up to
4 or 5 links

--
Sincerely Yours,
    John Vivirito

https://launchpad.net/~gnomefreak
https://wiki.ubuntu.com/JohnVivirito
Linux User# 414246

[Alexander Sack (asac)]

could you please test upstream builds? Does it work with their 3.0b5 build?

[th (daimoni)]

The bug is still in Hardy final (with all updates).

Tried to set http://www.cgpolis.com/artworks/digital/sailing_ship.jpg as background, nothing happens when clicking Set Desktop background.

[kindofabuzz (kindofabuzz)]

There is a temp fix for this: edit gconf key /desktop/gnome/background/primary_color to a 6 digit instead of a 12 digit. There is also a patch that will perm fix this mentioned here: https://bugzilla.mozilla.org/show_bug.cgi?id=421977 the devs need to include this patch into firefox and all would be well

[Alexander Sack (asac)]

https://bugzilla.mozilla.org/attachment.cgi?id=312156 is the upstream patch for this.

[Alexander Sack (asac)]

potential cherry-pick suitable for hardy. not (yet) milestoning for now.

[Alexander Sack (asac)]

important feature broken.

[kindofabuzz (kindofabuzz)]

seems fixed now. might have been one of the hardy proposed updates that fixed it for me.

[kindofabuzz (kindofabuzz)]

ignore my previous comment. i was in windows when i tried it. lol

[Alexander Sack (asac)]

Comment on attachment 312156
Updated to check GTK+ version

moving review from gavin to mconnor.

[Gavin Sharp (gavin-sharp)]

Comment on attachment 312156
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?

[canishk (canishk)]

the same problem occurred when trying firefox 3 in Ubuntu 7.10 - the Gutsy Gibbon version also..

[Steven Rose (steveydoteu)]

I get the same issue as Doug-NSF.

[Sergio Zanchetta (primes2h)]

It's not fixed for me with hardy proposed updates installed.
Firefox 3.0.1

[John Vivirito (gnomefreak)]

Not fixed in any 3.0.1 that we package until mozilla marks it fix released the fix wont be released so there is really no use saying it doesnt work or its not fixed looking at top of bug you can see its not fixed

[wmensah (wmensah-wvu)]

got the same problem.....doesn't work in the browser but when i save to desktop, for example, and then try to set as wallpaper after opening the image, it works.

[Adam Guthrie (ispiked)]

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

[Sylvain Pasche (sylvain-pasche)]

Created attachment 334243
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.

[Benjamin Smedberg (Mozilla) [:bs] (benjamin-smedbergs)]

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

[Benjamin Smedberg (Mozilla) [:bs] (benjamin-smedbergs)]

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

[Bzbarsky (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.

[Sylvain Pasche (sylvain-pasche)]

Created attachment 334578
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.

[Gavin Sharp (gavin-sharp)]

checkin-needed, then?

[Sylvain Pasche (sylvain-pasche)]

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

Yes, it should be

[Forest (fdenger-gmail)]

Confirmed. 8.04 ubuntu, with all updates on 8/27/08. firefox version 3.0.1

I get no reaction from clicking the apply the wallpaper. And the preview doesn't work until I select tile or another mode.

My system works fine to update walpaper the normal way through right clicking on the desktop.

[Colin Watson (cjwatson)]

From yesterday's foundations team meeting:

<asac> actually i thought the background image thing was fix committed
<slangasek> asac: an easy bug report to resolve, then ;)
<asac> slangasek: yeah ;)
<asac> ah. i think the idea was to get that committed upstream and let that sink down to us.

[Dao (dao)]

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

[Dao (dao)]

*** Bug 437292 has been marked as a duplicate of this bug. ***

[Dao (dao)]

*** Bug 454929 has been marked as a duplicate of this bug. ***

[homophoni (davidkirby)]

i am also experiencing this bug in ubuntu intrepid ibex alpha 6.

[Alexander Sack (asac)]

fix committed to firefox-3.0.head branch (rev 343)

[Launchpad Janitor (janitor)]

This bug was fixed in the package firefox-3.0 - 3.0.2+build6+nobinonly-0ubuntu2

---------------
firefox-3.0 (3.0.2+build6+nobinonly-0ubuntu2) intrepid; urgency=low

  * fix LP: #273907 - abrowser does not start: :$pkglibdir/abrowser link
    missing; we introduce the proper link in the firefox-3.0 package
    - update debian/rules
  * fix LP: #269795 - abrowser shouldn't display the EULA; we fix this by
    using brand.properties to determine whether we have official branding or
    not. In turn the about:rights notification only gets displayed when
    official build is running.
    - update debian/patches/lp269656_know_your_rights.patch
  * fix LP: #206191 - Firefox 3 "set as Desktop background" does not work
    properly; we cherry-pick latest upstream patch from bugzilla
    - add debian/patches/bz421977_att334578.patch
    - update debian/patches/series

 -- Alexander Sack <email address hidden> Wed, 24 Sep 2008 18:48:23 +0200

[Fta+bugzilla (fta+bugzilla)]

Comment on attachment 334578
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

[Dveditz (dveditz)]

Comment on attachment 334578
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.

[Mh+mozilla (mh+mozilla)]

Comment on attachment 334578
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");

??

[Mh+mozilla (mh+mozilla)]

*** Bug 461657 has been marked as a duplicate of this bug. ***

[Mh+mozilla (mh+mozilla)]

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

[Mh+mozilla (mh+mozilla)]

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

[Gavin Sharp (gavin-sharp)]

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

[Mh+mozilla (mh+mozilla)]

Filed bug #461663

[Michael Langerhorst (m-langerhorst)]

I still have exactly this described problem in Firefox 3.0.3 with all current updates.
Seems not to be fixed for all occasions.

[kindofabuzz (kindofabuzz)]

Works for me now. I don't know when it started to work because I actually forgot all about the bug. Using Intrepid with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081111 Minefield/3.1b2pre ID:20081111020802

[captaintrav (captaintrav)]

Maybe this is a regression in 3.0.3? I just finished setting up a fresh install of Hardy (i386) , and finished applying all updates, and have this issue with Firefox 3.0.3..

[KIAaze (zohn-joidberg)]

It's fixed for me with firefox v3.0.3.

$apt-cache policy firefox-3.0
firefox-3.0:
  Installed: 3.0.3+nobinonly-0ubuntu2
  Candidate: 3.0.3+nobinonly-0ubuntu2
  Version table:
 *** 3.0.3+nobinonly-0ubuntu2 0
        500 http://de.archive.ubuntu.com intrepid/main Packages
        100 /var/lib/dpkg/status

--
Unlock your computing: http://www.getgnulinux.org/

[KIAaze (zohn-joidberg)]

Just in case, I tested it with different image sizes and tiled,
centered, stretched:
http://img108.imageshack.us/img108/6733/tux9jd.jpg (1024x768)
http://www.imagecows.com/uploads/2995-tux_avatars.png (429x377)

Everything worked, so I guess it's not related to non-power of 2 textures. :)

[KIAaze (zohn-joidberg)]

Oh, and I don't use compiz. I don't know if this is important to
mention for this bug, but I remember it affecting other bugs.