837932 - Press-hold image option "Save image" does not save to the correct location

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Shell, defect)

Description

[Jim Mathies [:jimm]]

STR:

- press-hold on an image
- select Save Image

result: the image is saved to the firefox Downloads folder. Win7 and Win 8 have Libraries, which are groups of folders banded together. Each Library has a default save-to location (bug 718374 worked with these). We should respect these settings when saving to default locations, starting with the most obvious - Pictures.

This should not take much work to fixup. We can expose the default locations to front end code via directory services.

Comment 1

[Jim Mathies [:jimm]]

Attachment: dir svc patch v.1

Comment 2

[Jim Mathies [:jimm]]

Attachment: frontend patch v.1

Comment 3

[Brian R. Bondy [:bbondy]]

Comment on attachment 710184 [details] [diff] [review]
dir svc patch v.1

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

Looks good overall but r-'ing for now because of the ifndef

::: xpcom/io/SpecialSystemDirectory.cpp
@@ +124,5 @@
>  
>      return NS_NewLocalFile(nsDependentString(path, len), true, aFile);
>  }
>  
> +#ifndef SHLoadLibraryFromKnownFolder

I don't think this does what I think you are trying to do. SHLoadLibraryFromKnownFolder isn't a define like most Win32 API so this check will always be not defined.  Maybe just always go through CLSID_ShellLibrary?

Comment 4

[Brian R. Bondy [:bbondy]]

Comment on attachment 710184 [details] [diff] [review]
dir svc patch v.1

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

BTW I'm not sure if I can review this, might be worth getting another review from someone who can review in /xpcom as well.

Comment 5

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #3)
> I don't think this does what I think you are trying to do.
> SHLoadLibraryFromKnownFolder isn't a define like most Win32 API so this
> check will always be not defined.  Maybe just always go through
> CLSID_ShellLibrary?

Yes, my mistake. From the header it looks like 

#if (NTDDI_VERSION < NTDDI_VISTA)

would be appropriate here.

Comment 6

[Jim Mathies [:jimm]]

Hmm, that's wrong as well, I missed the

#if (NTDDI_VERSION >= NTDDI_WIN7)
#if (_WIN32_IE >= _WIN32_IE_IE70)
..

Comment 7

[Jim Mathies [:jimm]]

Attachment: dir svc patch v.2

I always define it instead.

Switching review to bsmedberg. Mossop usually defers to win devs for windows centric reviews.

Comment 8

[Brian R. Bondy [:bbondy]]

Comment on attachment 710210 [details] [diff] [review]
dir svc patch v.2

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

::: xpcom/io/SpecialSystemDirectory.cpp
@@ +125,5 @@
>      return NS_NewLocalFile(nsDependentString(path, len), true, aFile);
>  }
>  
> +__inline HRESULT
> +SHLoadLibraryFromKnownFolder(REFKNOWNFOLDERID kfidLibrary, DWORD grfMode,

This is prone to a build error for function already has a body if using a newer SDK. Could we just name this something different?

Comment 9

[Benjamin Smedberg]

Comment on attachment 710210 [details] [diff] [review]
dir svc patch v.2

As discussed on IRC, I don't see an obvious reason why we'd want the default open location to be different from the default save location, and so we probably want just one key to do the right thing.

If that's not what we want, then I think we need to be much more clear in the comments about why they are different. e.g. the save-to locations are never displayed to the user, and the default folders are actually the virtual "Library" displays?

Are there actual filesystem paths for the virtual Library folders? Do we need an alternate representation and the directory service is not actually the right API for this?

Comment 10

[Jim Mathies [:jimm]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> Comment on attachment 710210 [details] [diff] [review]
> dir svc patch v.2
> 
> As discussed on IRC, I don't see an obvious reason why we'd want the default
> open location to be different from the default save location, and so we
> probably want just one key to do the right thing.

If these keys are used to save content, then they will be the right keys. If however you want to open content, they may not. For the browser, it's always save-as, so I'm fine updating the older keys to return the library values on Win7 and up.

> If that's not what we want, then I think we need to be much more clear in
> the comments about why they are different. e.g. the save-to locations are
> never displayed to the user, and the default folders are actually the
> virtual "Library" displays?
> 
> Are there actual filesystem paths for the virtual Library folders? Do we
> need an alternate representation and the directory service is not actually
> the right API for this?

Libraries are like virtual folders, which encompass multiple locations. In Windows you can treat these like normal folders assuming you keep them in windows centric objects (e.g. IShellItem). They can't be resolved to a single ascii path we usually deal with. Which shows our file system code is becoming out of date - Windows is moving beyond single folder paths to represent locations in the file system.

I can't think of any examples in firefox where we run into problems. But imagine if someone wrote an mp3 player in xulrunner - if the app wanted to be win7+ compliant when displaying all the music the user has access to, it would want to open the Music Library, and list all items within it. So we would need a way to return a path list for a Library to the app, which we currently don't support.

In my specific case here, I just want to use the right save-to location.

We currently one-off this in the file picker code as well. If you choose a Library from the save-as picker, we resolve it to the default save location.

Comment 11

[Jim Mathies [:jimm]]

Attachment: dir svc patch v.3

updated per comments.

Comment 12

[Jim Mathies [:jimm]]

I just added a comment to GetLibrarySaveToPath explaining what it does as well.

Comment 13

[Jim Mathies [:jimm]]

Attachment: frontend patch v.2

Comment 14

[Jim Mathies [:jimm]]

Comment on attachment 710397 [details] [diff] [review]
frontend patch v.2

noticed some nits, will repost.

Comment 15

[Jim Mathies [:jimm]]

Attachment: frontend patch v.3

I've inlined the args to the ContentAreaUtils.internalPersist call, and touched up some commenting.

Comment 16

[Matt Brubeck (:mbrubeck)]

Comment on attachment 710410 [details] [diff] [review]
frontend patch v.3

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

r=mbrubeck with some minor possible changes:

::: browser/metro/base/content/ContextCommands.js
@@ +93,5 @@
> +    let dirSvc = Components.classes["@mozilla.org/file/directory_service;1"]
> +                           .getService(Components.interfaces.nsIProperties);
> +    let saveLocationPath = dirSvc.get(aType, Components.interfaces.nsIFile);
> +
> +    let fileInfo = new ContentAreaUtils.FileInfo(null);

I believe you can remove the "null" argument here and just pass zero arguments.

@@ +98,5 @@
> +    ContentAreaUtils.initFileInfo(fileInfo, popupState.mediaURL,
> +                                  browser.documentURI.originCharset,
> +                                  null, null, null);
> +
> +    saveLocationPath.append(fileInfo.fileName);

I think we want to pass getNormalizedLeafName(fileInfo.fileName, fileInfo.fileExt).

Comment 17

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d714dcba979

https://hg.mozilla.org/projects/elm/rev/373ffdc2bedb
https://hg.mozilla.org/projects/elm/rev/441bfce0ce6e

Comment 19

[Jim Mathies [:jimm]]

https://hg.mozilla.org/mozilla-central/rev/0d714dcba979

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0d714dcba979