1119088 - Set As Desktop Background does not work on OS X

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[(no longer active)]

STR: Right click on an image and select "Set As Desktop Background".

Comment 2

[:Gijs (he/him)]

Re-nomming. This is secondary UI, but it's going to be very visible if we roll out e10s to everyone and this doesn't work at all. The dupe says the button in the dialog is also broken so the functionality itself is just dead in the water.

Comment 3

[Tracy Walker [:tracy]]

STR's:
1) Do a search for "images"
2) Right click any of the images to get the image context menu.
3) Click Set As Desktop Background... to get Preview dialog of image.
4) Click Set Desktop Background button

Expected result: 
The image is saved and the desktop background is set to the new image.

Tested result: 
The Set Desktop Background button changes to "Saving Picture..." but the image is not saved and the desktop background is never set to the new image.

Mac 10.9.5 Nightly 47.0a1 20160125060632 (e10s and non-e10s) - FAIL
Mac 10.9.5 Firefox Release 26.0 (non-e10s) - FAIL
Mac 10.9.5 Firefox Release 20.0.1 (non-e10s) - FAIL

Windows 7 Nightly 47.0a1 20160125060632 (e10s) - PASS
Ubuntu 14 Nightly 47.0a1 20160126030244 (e10s) - PASS

This has not worked correctly on Mac in a long time, if ever (see the old releases tested above). It works as expected on Windows and Linux. Note: The Windows and Linux Preview dialogs have additional options for Position (sizing) and Color.  Those dialogs also have a Cancel button.  None of those option are available in the Mac Preview dialog.

Comment 4

[Jim Mathies [:jimm]]

If we can't get this to work on mac we should remove the option.

Comment 5

[:Gijs (he/him)]

FWIW, testing on OSX, the preview does show up, and it does so on Windows on e10s as well. I'll just morph this about making this work on OS X when actually doing the thing, then... :-)

Comment 6

[Jonathan Chan [:jyc]]

This still happens on Nightly, it seems.

Comment 7

[Mike Kaply [:mkaply]]

And as Jim said, if we can't fix this, we need to remove it. We shouldn't be shipping features that just don't work.

I'm getting:

NS_ERROR_NOT_IMPLEMENTED: Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIShellService.setDesktopBackground]

which implies we're going down this path:

http://searchfox.org/mozilla-central/source/browser/components/shell/nsMacShellService.cpp#12

Comment 8

[Mike Kaply [:mkaply]]

So I was correct that we are going down that path, but I don't know why.

If I open

https://static.storied.co/JQl670PRVQPWKNk1/projects/w1Z975BX7O2nOBqm/assets/images/1022dc170e8769727e615489e1f07e7d_extra_large.jpg

In a page and select "Set as Desktop background", the image URI obtained here:

http://searchfox.org/mozilla-central/source/browser/components/shell/nsMacShellService.cpp#112

Is a data URI, it is not the original URI (as I would have expected it to be).

So we end up going down this path:

http://searchfox.org/mozilla-central/source/browser/components/shell/nsMacShellService.cpp#128

Comment 9

[Mike Kaply [:mkaply]]

I've tried some idea, but nothing is working. I don't really understand the code, so I'll have to punt.

What I've discovered:

1. Even if you get the HTMLImageElement and check the src, it's still a data URL.
2. Right clicking on an image and setting as desktop background still uses a data URL.
3. I can't use mozregression to track this down because old version of Firefox don't run on current Sierra due to bug 1286613.

So I think the key here is getting the real image URL, I just don't know how to get it.

Comment 10

[Stephen A Pohl [:spohl]]

(In reply to Mike Kaply [:mkaply] from comment #9)
> I've tried some idea, but nothing is working. I don't really understand the
> code, so I'll have to punt.

Thanks, Mike! I'm taking a look.

Comment 11

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb3c01591f59e7cc547d44b206dd32c868dbf461

Comment 12

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c631e07a3cb071941d81790afd7f795324a9e06b

Comment 13

[Stephen A Pohl [:spohl]]

Attachment: Patch

This didn't work because we were trying to get the leaf name from the original URL of the image, but the URL in setDesktopBackground is a data URL. This patch passes the leaf name along to setDesktopBackground as an additional argument. This is ignored on Windows (where we retrieve a localized file name and convert the image to a .bmp) and Linux (where we generate the filename based on the brand name and convert the image to a .png).

Try run (comment 12) is green.

Comment 14

[Markus Stange [:mstange]]

Comment on attachment 8894264 [details] [diff] [review]
Patch

Review of attachment 8894264 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/content.js
@@ +1117,5 @@
> +      }
> +      imageNameEnd = imageName.lastIndexOf("#");
> +      if (imageNameEnd > 0) {
> +        imageName = imageName.substr(0, imageNameEnd);
> +      }

(new URL(url)).pathname can save you some work here; you'll still need to cut off leading directories, though.

::: browser/components/shell/nsMacShellService.cpp
@@ +136,3 @@
>  
>    // and add the imgage file name itself:
>    mBackgroundFile->Append(fileNameUnicode);

What happens if the supplied filename is empty, too long, has a file extension that's different than the actual image format, or contains characters that are not valid within filenames?

I'm also a bit surprised that we just blindly overwrite an existing file with the same filename in ~/Pictures and don't make any effort to avoid collisions, but I suppose that's a different issue.

Comment 15

[:Gijs (he/him)]

Is there any chance we could add an automated test for this? Or is that silly levels of difficult?

Comment 16

[Mike Kaply [:mkaply]]

Do we know what actually broke this? This doesn't sound like it would have ever worked? Did something change to always pass data URLs to set as background?

Comment 17

[Jeff Griffiths (:canuckistani) (:⚡︎)]

I'd like to get this done in this cycle, if possible.

Comment 18

[Stephen A Pohl [:spohl]]

Attachment: Patch

(In reply to Markus Stange [:mstange] from comment #14)
> Comment on attachment 8894264 [details] [diff] [review]
> Patch
> 
> Review of attachment 8894264 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/content.js
> @@ +1117,5 @@
> > +      }
> > +      imageNameEnd = imageName.lastIndexOf("#");
> > +      if (imageNameEnd > 0) {
> > +        imageName = imageName.substr(0, imageNameEnd);
> > +      }
> 
> (new URL(url)).pathname can save you some work here; you'll still need to
> cut off leading directories, though.

Done, thank you!

> ::: browser/components/shell/nsMacShellService.cpp
> @@ +136,3 @@
> >  
> >    // and add the imgage file name itself:
> >    mBackgroundFile->Append(fileNameUnicode);
> 
> What happens if:
> the supplied filename is empty,

Nothing. No file is created and the desktop background image remains unchanged. We fail here:
https://dxr.mozilla.org/mozilla-central/source/dom/webbrowserpersist/nsWebBrowserPersist.cpp#2313

> too long,

Nothing. No file is created and the desktop background image remains unchanged. We fail here:
https://dxr.mozilla.org/mozilla-central/source/dom/webbrowserpersist/nsWebBrowserPersist.cpp#2313

> has a file extension that's different than the actual image format,

I've experimentally been able to confirm that macOS does not seem to care about the file extension and will display the correct background if the image is of a valid image type for backgrounds. Even .txt works.

> or contains characters that are not valid within filenames?

Nothing. No file is created and the desktop background image remains unchanged. We fail here:
https://dxr.mozilla.org/mozilla-central/source/dom/webbrowserpersist/nsWebBrowserPersist.cpp#2313

> I'm also a bit surprised that we just blindly overwrite an existing file
> with the same filename in ~/Pictures and don't make any effort to avoid
> collisions, but I suppose that's a different issue.

Right. I've basically kept the current behavior. Note that OSX is different from Linux and Windows here. On Windows and Linux, we create a background image with a specific filename. When the user chooses to change the background to a different image, this same file is overwritten. On OSX, the different files persist on the system unless there is a name collision.

Comment 19

[Stephen A Pohl [:spohl]]

(In reply to Mike Kaply [:mkaply] from comment #16)
> Do we know what actually broke this? This doesn't sound like it would have
> ever worked? Did something change to always pass data URLs to set as
> background?

Still looking into it.

(In reply to :Gijs from comment #15)
> Is there any chance we could add an automated test for this? Or is that
> silly levels of difficult?

Trying to write one now.

Comment 20

[Stephen A Pohl [:spohl]]

(In reply to Mike Kaply [:mkaply] from comment #16)
> Do we know what actually broke this? This doesn't sound like it would have
> ever worked? Did something change to always pass data URLs to set as
> background?

Okay, I was finally able to track this down. This initially broke when we enabled e10s but continued to work in non-e10s. That's when this bug was filed. This eventually broke completely on macOS/OSX when we attempted to fix it in bug 1135933 for all platforms by switching to data URLs. Although the preview would now show an image (which it failed to do in e10s before), it was no longer able to save the image because macOS/OSX never supported saving background images from data URLs.

Comment 21

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b25b4ffef9a1b360e07e0933292fa31eeb9ac6e

Comment 22

[Stephen A Pohl [:spohl]]

Attachment: Tests

This adds a test for macOS and updates an existing test for Linux. Try run in comment 21.

Comment 23

[Markus Stange [:mstange]]

Comment on attachment 8894635 [details] [diff] [review]
Patch

Review of attachment 8894635 [details] [diff] [review]:
-----------------------------------------------------------------

Should probably get a quick review from a frontend person as well, for the JavaScript parts.

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8894635 [details] [diff] [review]
Patch

Review of attachment 8894635 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: browser/base/content/content.js
@@ +1114,2 @@
>        sendAsyncMessage("ContextMenu:SetAsDesktopBackground:Result",
> +                       { dataUrl, imageName});

Nit - space before }

Comment 25

[Markus Stange [:mstange]]

Comment on attachment 8895000 [details] [diff] [review]
Tests

Review of attachment 8895000 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/shell/test/browser_1119088.js
@@ +8,5 @@
> +  let dirSvc = Cc["@mozilla.org/file/directory_service;1"].
> +               getService(Ci.nsIDirectoryServiceProvider);
> +
> +  let desktopBackgroundDb = dirSvc.getFile(NS_MAC_USER_LIB_DIR, {});
> +  desktopBackgroundDb.append("Application\ Support");

I think an escaped space in a JavaScript string literal is just a space. The backslash doesn't actually go into the string (you'd need \\ for that), and it's not necessary.

@@ +48,5 @@
> +    let dockArg = ["Dock"];
> +    let process =
> +      Cc["@mozilla.org/process/util;1"].createInstance(Ci.nsIProcess);
> +    process.init(killall);
> +    process.run(true, dockArg, 1);

Oh wow, that's intense. I find it hard to predict whether that's going to cause problems on our test machines. I suppose we can still back this out if we start seeing strange issues.

@@ +58,5 @@
> +
> +function test() {
> +  gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser);
> +  gBrowser.selectedBrowser.addEventListener("load", onPageLoad, true);
> +  content.location = "about:logo";

Nice choice of picture.

Comment 26

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=244a61b7ba24a2d868df6ab39852c2fb974fb0b6

Comment 27

[Stephen A Pohl [:spohl]]

Attachment: Patch

Thanks for the reviews! Addressed nit. Carrying over r+.

Comment 28

[Stephen A Pohl [:spohl]]

Attachment: Tests

Removed escaped space, thanks!

I think I messed up browser.ini in the previous patch because my new macOS-only test ran on Linux on try (see comment 21). I've launched a new try run (see comment 26) and Linux looks good now. Still waiting for macOS tests to run. Markus, would you mind taking one last look at these changes? Thanks!

(In reply to Markus Stange [:mstange] from comment #25)
> Comment on attachment 8895000 [details] [diff] [review]
> Tests
> 
> Review of attachment 8895000 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/shell/test/browser_1119088.js
> @@ +48,5 @@
> > +    let dockArg = ["Dock"];
> > +    let process =
> > +      Cc["@mozilla.org/process/util;1"].createInstance(Ci.nsIProcess);
> > +    process.init(killall);
> > +    process.run(true, dockArg, 1);
> 
> Oh wow, that's intense. I find it hard to predict whether that's going to
> cause problems on our test machines. I suppose we can still back this out if
> we start seeing strange issues.

Yes, I completely agree. I'm hoping this will stick. Otherwise, we may be able to load the database file as a sql database and determine the current background image before running the test. However, I'm not sure this is documented and I suppose the format of the database file could change.

Comment 29

[Stephen A Pohl [:spohl]]

(In reply to Stephen A Pohl [:spohl] from comment #28)
> I think I messed up browser.ini in the previous patch because my new
> macOS-only test ran on Linux on try (see comment 21). I've launched a new
> try run (see comment 26) and Linux looks good now. Still waiting for macOS
> tests to run.

The try run is green. I've confirmed that the test only ran on macOS and it did so successfully.

Comment 30

[Markus Stange [:mstange]]

Comment on attachment 8895207 [details] [diff] [review]
Tests

Review of attachment 8895207 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, let's not depend on the database format.

Comment 31

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e59e491465e89c549f21d8bf22b9d8725198412b
Bug 1119088: Fix ability to set an image as desktop background via context menu on macOS. r=mstange,mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/4da996b87a5e3d89c487fa5848e24b23c6f90080
Bug 1119088: Update test for setDesktopBackground() and add test for macOS. r=mstange

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e59e491465e8
https://hg.mozilla.org/mozilla-central/rev/4da996b87a5e

Comment 33

[:Gijs (he/him)]

Does this mean bug 820515 is also fixed? :-)

Comment 34

[Stephen A Pohl [:spohl]]

(In reply to :Gijs from comment #33)
> Does this mean bug 820515 is also fixed? :-)

It does indeed! Thanks for pointing this out.