Closed
Bug 662324
Opened 13 years ago
Closed 12 years ago
Add "Fill" and "Fit" position support for "Set As Desktop Background..."
Categories
(Firefox :: Shell Integration, enhancement)
Firefox
Shell Integration
Tracking
()
RESOLVED
FIXED
Firefox 13
People
(Reporter: Kwan, Assigned: Kwan)
References
Details
Attachments
(1 file, 4 obsolete files)
9.05 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•13 years ago
|
||
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?
Attachment #537596 -
Flags: feedback?(gavin.sharp)
Comment 2•13 years ago
|
||
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).
Attachment #537596 -
Flags: feedback?(gavin.sharp) → feedback+
Assignee | ||
Comment 3•13 years ago
|
||
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.
Assignee | ||
Comment 4•13 years ago
|
||
(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.
Attachment #537596 -
Attachment is obsolete: true
Attachment #537824 -
Flags: review?(gavin.sharp)
Comment 5•13 years ago
|
||
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.
Attachment #537824 -
Flags: review?(gavin.sharp) → review+
Updated•13 years ago
|
Assignee: nobody → moz-ian
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 6•12 years ago
|
||
update to hide on windows pre-7 via hidden attribute rather than CSS per comment 5
Attachment #537824 -
Attachment is obsolete: true
Comment 7•12 years ago
|
||
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/gsharp@mozilla.com-9cd06c9dbcc1
Assignee | ||
Comment 8•12 years ago
|
||
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.
Attachment #589055 -
Attachment is obsolete: true
Updated•12 years ago
|
Attachment #589955 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 9•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4045a049e9e
Keywords: checkin-needed
Target Milestone: --- → Firefox 12
Comment 10•12 years ago
|
||
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
Target Milestone: Firefox 12 → ---
Assignee | ||
Comment 11•12 years ago
|
||
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•12 years ago
|
||
(In reply to Ian Moody (:Kwan) from comment #11) IRC
Assignee | ||
Comment 13•12 years ago
|
||
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
Attachment #589955 -
Attachment is obsolete: true
Attachment #592905 -
Flags: review?(gavin.sharp)
Comment 14•12 years ago
|
||
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=a9a9c8f020ee
Updated•12 years ago
|
Attachment #592905 -
Flags: review?(gavin.sharp) → review+
Comment 15•12 years ago
|
||
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/philringnalda@gmail.com-a350cc1f8e05 Timed out after 06 hours without completing.
Assignee | ||
Comment 16•12 years ago
|
||
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.
Keywords: checkin-needed
Comment 17•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/240237fb07cb
Keywords: checkin-needed
Target Milestone: --- → Firefox 13
Comment 18•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/240237fb07cb
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•