662324 - Add "Fill" and "Fit" position support for "Set As Desktop Background..."

Status: RESOLVED FIXED

(Firefox :: Shell Integration, enhancement)

Description

[Ian Moody [:Kwan] (UTC+1)]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1

Both Windows 7 and Gnome support the ability to position wallpapers as "Fill" and "Fit" ("zoom" and "scaled" in Gnome's case) and this should be exposed to the "Set As Desktop Background" feature.

Reproducible: Always

Steps to Reproduce:
1. Open an image or a page containing one in Firefox
2. Right-click it and select "Set As Desktop Background..."
3. Play with the Position options

Actual Results:  
Only "Center", "Stretch" and "Tile" are offered

Expected Results:  
"Fill" and "Fit" should also be available on Gnome and Windows 7

Patch incoming

Comment 1

[Ian Moody [:Kwan] (UTC+1)]

Attachment: add "Fill" and "Fit" support v1

This patch adds support for the two properties to the two ShellServices and also exposes the options in the setDesktopBackground dialog.  One thing I'm not too sure about is the way I'm currently detecting if Windows is at least 7 (since earlier versions don't support these options).  Is there a better way to do this?

Comment 2

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

Comment on attachment 537596 [details] [diff] [review]
add "Fill" and "Fit" support v1

>diff --git a/browser/components/shell/content/setDesktopBackground.js b/browser/components/shell/content/setDesktopBackground.js

>+    // add class to menuPosition if win7
>+    var ntVer = /Windows NT (\d)\.(\d)/.match(window.navigator.oscpu);
>+    if (ntVer[1] >= 6 && ntVer[2] >= 1)
>+      document.getElementById("menuPosition").className = "win7";

This should be #ifdef XP_WIN, and use the system info version as in http://hg.mozilla.org/mozilla-central/annotate/fd50260987f4/toolkit/content/aboutSupport.js#l247 .

You don't need to rev the interface ID to add the constant.

Why is the UI only exposed for Windows 7, if the Windows implementation doesn't seem to differentiate? Should it? Does the GNOME implementation work reliably for all versions of Linux that we care about?

I guess this is OK, but I am loathe to spend much time on this, particularly given the difficulties in testing this automatically. I would generally prefer removing this functionality (easy enough to add in an add-on).

Comment 3

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 537596 [details] [diff] [review]
add "Fill" and "Fit" support v1

Umm, bear in mind that it's obviously supposed to be .exec(), not .match().  Not sure how/when that changed back... :S Sorry.

Comment 4

[Ian Moody [:Kwan] (UTC+1)]

Attachment: add "Fill" and "Fit" support v1.1

(In reply to comment #2)
> Comment on attachment 537596 [details] [diff] [review] [review]
> add "Fill" and "Fit" support v1
> 
> This should be #ifdef XP_WIN, and use the system info version as in
> http://hg.mozilla.org/mozilla-central/annotate/fd50260987f4/toolkit/content/
> aboutSupport.js#l247 .

Ah, this is exactly the sort of thing I was after, thanks, fixed.

> You don't need to rev the interface ID to add the constant.

Fixed
 
> Why is the UI only exposed for Windows 7, if the Windows implementation
> doesn't seem to differentiate? Should it? 

Because Fit and Fill were only introduced in Win7.  While it's possible that the functionality was in previous versions but not exposed, using it would have negative effects for users who then went into the control panel for backgrounds since the position control would not be able to convey the current value.

> Does the GNOME implementation work
> reliably for all versions of Linux that we care about?

The new values here, "zoom" and "scaled" have been in and working in Gnome since 2.16 (checked in Ubuntu 6.10).  While my compile won't run on there, the firefox it comes with does change the existing 3 settings fine, and the other two work fine when changed from the input settings manager.  On Ubuntu 10.04 changing all values with my compile work perfectly.

> I guess this is OK, but I am loathe to spend much time on this, particularly
> given the difficulties in testing this automatically. I would generally
> prefer removing this functionality (easy enough to add in an add-on).

Is it not possible to test because there is no way to read OS settings like these?  I don't think it's that likely to break anyway though, given that it's just setting the values the OS uses, so if it did break it'd more likely be an OS bug rather than mozilla.  I'd argue against removing it as I think it's a very useful feature, and in fact without any kind of usage metrics we have no way of knowing how many users use it. It could well be quite a lot since I'd think 'find image->right-click->set as background->ok' is a much simpler and shorter workflow than 'find image->right-click->save image->open OS background changer (assuming that's one step, may not be)->browsing to save location->ok'.  My instinct is that for the "average" user the first is more likely to be used, but without any sort of data can't know for sure.

Thanks a lot for your help.

Comment 5

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

Comment on attachment 537824 [details] [diff] [review]
add "Fill" and "Fit" support v1.1

Sorry for letting this request linger for so long, Ian.

Let's go ahead and do this.

Rather than hiding elements using CSS, it would probably be simpler to just set "hidden" attribute on the relevant menu items.

r=me with that change, and assuming you've tested this on Windows 7, Windows <7, and a recent Linux (you seem to have this last part covered per comment 4). If you need help testing Windows let me know.

Comment 6

[Ian Moody [:Kwan] (UTC+1)]

Attachment: add "Fill" and "Fit" support v1.2

update to hide on windows pre-7 via hidden attribute rather than CSS per comment 5

Comment 7

[Mozilla RelEng Bot]

Try run for 9cd06c9dbcc1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=9cd06c9dbcc1
Results (out of 1 total builds):
    success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-9cd06c9dbcc1

Comment 8

[Ian Moody [:Kwan] (UTC+1)]

Attachment: add "Fill" and "Fit" support v1.3

Final version, rebased on top of the patch for bug 719538, tested on Linux and Win7 for changing the wallpaper and Win XP for hiding the new options.
Thanks Gavin for all your help.

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4045a049e9e

Comment 10

[Phil Ringnalda (:philor)]

Wups, guess I should have noticed that try run was just one build, not a whole try run with, like, tests and stuff.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3068e997f7fd for browser-chrome failures in "browser_420786.js | Wallpaper position GConf key is correct - Got zoom, expected centered" like https://tbpl.mozilla.org/php/getParsedLog.php?id=8928801&tree=Mozilla-Inbound

Comment 11

[Ian Moody [:Kwan] (UTC+1)]

Argh damn it, this is why I should have requested a try run rather than just running them on my local (Windows) machine.  It doesn't save you guys any time if stuff like this happens, and it means I miss Fx12.  Sorry Phil.
On the plus side, not only does this mean there is a test for this (at least on Linux), which Gavin didn't seem to think was the case, but (it seems) the problem is actually that the test needs updating, rather than I've horribly broken something.  I'll make an updated patch with the test update on my local machine, run the test suite, then request a try run.  What's the easiest way to do that?  IRC?  Or is there a bugzilla keyword?

Comment 12

[Dão Gottwald [:dao]]

(In reply to Ian Moody (:Kwan) from comment #11)
IRC

Comment 13

[Ian Moody [:Kwan] (UTC+1)]

Attachment: add "Fill" and "Fit" support v1.4

modify the test browser_420786.js to handle the new values, tested locally and that test passes, will request try run on IRC for (hopefully) full green

Comment 14

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

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=a9a9c8f020ee

Comment 15

[Mozilla RelEng Bot]

Try run for a350cc1f8e05 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a350cc1f8e05
Results (out of 273 total builds):
    exception: 2
    success: 245
    warnings: 23
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-a350cc1f8e05
 Timed out after 06 hours without completing.

Comment 16

[Ian Moody [:Kwan] (UTC+1)]

From the two try runs in comment 14 and comment 15 it looks like this passes all tests, with failures either being infra-related or due to other bugs and the previous test failure definitely fixed, so if someone could check this in whenever is convenient that would be appreciated :).  Thank you Gavin and Phil.

Comment 17

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/240237fb07cb

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/240237fb07cb