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

RESOLVED FIXED

Status

Firefox for Metro
Shell
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [completed-elm])

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 710184 [details] [diff] [review]
dir svc patch v.1
Assignee: nobody → jmathies
(Assignee)

Comment 2

5 years ago
Created attachment 710186 [details] [diff] [review]
frontend patch v.1
(Assignee)

Updated

5 years ago
Attachment #710184 - Flags: review?(netzen)
(Assignee)

Updated

5 years ago
Blocks: 838167
(Assignee)

Updated

5 years ago
Whiteboard: metro-mvp?
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?
Attachment #710184 - Flags: review?(netzen) → review-
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.
(Assignee)

Comment 5

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
Hmm, that's wrong as well, I missed the

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

Comment 7

5 years ago
Created attachment 710210 [details] [diff] [review]
dir svc patch v.2

I always define it instead.

Switching review to bsmedberg. Mossop usually defers to win devs for windows centric reviews.
Attachment #710184 - Attachment is obsolete: true
Attachment #710210 - Flags: review?(benjamin)
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

5 years ago
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?
Attachment #710210 - Flags: review?(benjamin) → review-
(Assignee)

Comment 10

5 years ago
(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.
(Assignee)

Updated

5 years ago
Blocks: 833165
(Assignee)

Comment 11

5 years ago
Created attachment 710332 [details] [diff] [review]
dir svc patch v.3

updated per comments.
Attachment #710210 - Attachment is obsolete: true
Attachment #710332 - Flags: review?(benjamin)
(Assignee)

Comment 12

5 years ago
I just added a comment to GetLibrarySaveToPath explaining what it does as well.
(Assignee)

Comment 13

5 years ago
Created attachment 710397 [details] [diff] [review]
frontend patch v.2
Attachment #710186 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #710397 - Flags: review?(mbrubeck)
(Assignee)

Comment 14

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

noticed some nits, will repost.
Attachment #710397 - Flags: review?(mbrubeck)
(Assignee)

Comment 15

5 years ago
Created attachment 710410 [details] [diff] [review]
frontend patch v.3

I've inlined the args to the ContentAreaUtils.internalPersist call, and touched up some commenting.
Attachment #710397 - Attachment is obsolete: true
Attachment #710410 - Flags: review?(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).
Attachment #710410 - Flags: review?(mbrubeck) → review+

Updated

5 years ago
Attachment #710332 - Flags: review?(benjamin) → review+
(Assignee)

Updated

5 years ago
Duplicate of this bug: 800982
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/mozilla-central/rev/0d714dcba979
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.