Closed Bug 837932 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox for Metro Graveyard :: Shell, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jimm, Assigned: jimm)

References

Details

(Whiteboard: [completed-elm])

Attachments

(2 files, 4 obsolete files)

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.
Attached patch dir svc patch v.1 (obsolete) — Splinter Review
Assignee: nobody → jmathies
Attached patch frontend patch v.1 (obsolete) — Splinter Review
Attachment #710184 - Flags: review?(netzen)
Blocks: 838167
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.
(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.
Hmm, that's wrong as well, I missed the

#if (NTDDI_VERSION >= NTDDI_WIN7)
#if (_WIN32_IE >= _WIN32_IE_IE70)
..
Attached patch dir svc patch v.2 (obsolete) — Splinter Review
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 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-
(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.
Blocks: 833165
updated per comments.
Attachment #710210 - Attachment is obsolete: true
Attachment #710332 - Flags: review?(benjamin)
I just added a comment to GetLibrarySaveToPath explaining what it does as well.
Attached patch frontend patch v.2 (obsolete) — Splinter Review
Attachment #710186 - Attachment is obsolete: true
Attachment #710397 - Flags: review?(mbrubeck)
Comment on attachment 710397 [details] [diff] [review]
frontend patch v.2

noticed some nits, will repost.
Attachment #710397 - Flags: review?(mbrubeck)
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+
Attachment #710332 - Flags: review?(benjamin) → review+
Duplicate of this bug: 800982
https://hg.mozilla.org/mozilla-central/rev/0d714dcba979
Status: NEW → RESOLVED
Closed: 9 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.