395534 - browser.download.lastDir is ignored when right-clicking and "Save Link As ..." when browser.download.useDownloadDir is true

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

Build ID: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a8pre) Gecko/2007090804 Minefield/3.0a8pre

Summary: browser.download.lastDir is ignored when right-clicking and "Save Link As ..." when browser.download.useDownloadDir is true.

Steps to Reproduce:

1. Set browser.download.useDownloadDir to |false|
2. Set browser.download.lastDir to C:\Users\Owner\Desktop\temp (or whatever)
3. Set browser.download.dir to C:\Users\Owner\Downloads
4. Load any webpage with a link to a file, and right-click the file and choose "Save Link As ..."
5. When the filepicker comes up choose a destination and save the file there.
6. Repeat step 4.

Expected Results:

browser.download.lastDir should be set to the location specified in steps 4/6, and should be used on right-click, "Choose Save Link As ...", overriding browser.download.dir

Actual Results:

When browser.download.useDownloadDir is |true|, right-click "Save Link As ..." always defaults the filepicker's location to the value of browser.download.dir

Comment 1

[Stephen Donner [:stephend] Not actively reading bugmail]

Jim, can you take a look please?  Thanks!

Comment 2

[Jim Mathies [:jimm]]

>Summary: browser.download.lastDir is ignored when right-clicking and "Save Link
>As ..." when browser.download.useDownloadDir is true.

That's correct behavior with the code as is. To default to lastDir on any operation set "always ask me" in the options, which sets useDownloadDir to false.

I think what's being requested here is that lastDir be honored when useDownloadDir is true as well. I'm fine with this, assuming we're cool with the following:

1) Right click, Save Link As
They are promped with a filepicker pointing to lastDir.
2) Click on a link to a file
They are prompted with the file handling dialog, they click 'save to disk' and the file goes to the downloads folder.

Both the current scenario and this new one work fine afaiac. The result would be:

Firefox Behavior:
-----------------

browser.download.useDownloadDir = true or false

  Content area context menu file pickers:
  Save Image As
   - lastDir* (filepicker)
  Save Link As
   - lastDir* (filepicker)
  Save Page As
   - lastDir* (filepicker)

browser.download.useDownloadDir = true

  Unknown content dialog save as:
   - usersDownloadFolder (no filepicker prompt)

  Download Manager window drag and drop:
   - usersDownloadFolder (no filepicker prompt)

browser.download.useDownloadDir = false

  Unknown content dialog save as:
   - lastDir* (filepicker prompt)

  Download Manager window drag and drop:
   - lastDir* (filepicker prompt)

  * If lastDir points to a non-existent directory, defaults to
usersDownloadFolder.

However, there may be a big problem here we haven't considered. One of the reasons why this has all been so confusing is that threads drop into the same routine to handle all this. :) Distinguishing between the various different operations may not be possible. It's all in contentAreaUtils.js if anybody cares to take a look. There are also other places not mentioned here that roll through that same routine, in firefox and in other apps like thunderbird. So messing with contentAreaUtils.js needs to be done carefully.

Comment 3

[pal-moz]

this bug is same thing as https://bugzilla.mozilla.org/show_bug.cgi?id=394910#c7 ?

Comment 4

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #3)
> this bug is same thing as
> https://bugzilla.mozilla.org/show_bug.cgi?id=394910#c7 ?

It is, yes, that's why I split it out into a separate bug.  Peter's bug is different.

Comment 5

[Stephen Donner [:stephend] Not actively reading bugmail]

I really think we want this for Firefox 3, given that it's parity with our other releases...

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #5)
> I really think we want this for Firefox 3, given that it's parity with our
> other releases...
I'm not sure we want to take it.  Just because we did it in the past doesn't mean it's right...

Comment 7

[Jim Mathies [:jimm]]

This might not be too bad, in contentAreaUtils - 

457   var useDownloadDir = prefs.getBoolPref("useDownloadDir");
458   var dir = null;
459   
460   // Default to lastDir if useDownloadDir is false, and lastDir
461   // is configured and valid. Otherwise, use the user's default
462   // downloads directory configured through download prefs.
463   var dnldMgr = Components.classes["@mozilla.org/download-manager;1"]
464                           .getService(Components.interfaces.nsIDownloadManager);
465   try {                          
466     var lastDir = prefs.getComplexValue("lastDir", nsILocalFile);
467     if (!useDownloadDir && lastDir.exists())
468       dir = lastDir;
469     else
470       dir = dnldMgr.userDownloadsDirectory;
471   } catch(ex) {
472     dir = dnldMgr.userDownloadsDirectory;
473   }

line 467 could be 

467     if ((!aSkipPrompt || !useDownloadDir) && lastDir.exists())
..

This would need to be tested in all cases to make sure it doesn't break functionality in others where we want the file to go to the download folder. The helperappdialog should be ok without any change, although that should be checked too. I don't have time for this right at the moment but I can come back to it in a bit. 

It does make sense in one way - "always ask me" may not have anything to do with these routines. The context menu items say "Save XXX As...", that implies a prompt. (Although I wonder why we don't have a "Save Image" option that just dumps the image into the downloads folder? Too much complexity probably.)

Comment 8

[Peter van der Woude [:Peter6]]

This bug and bug the result of (a decision made in ) Bug 308073, I'm not sure if it should be blocking that one i/o Bug 393247

Comment 9

[Stephen Donner [:stephend] Not actively reading bugmail]

Alex, could you comment on whether we want the same behavior here as Firefox 2.0.0.x, which is that right-clicking and saving a file using "Save Link As ..." should override the user's default download location?

Comment 10

[Alex Faaborg [:faaborg] (Firefox UX)]

>which is that right-clicking and saving a file using "Save Link As
>..." should override the user's default download location?

Technically if the user right clicks a link, and selects save as, the save as dialog should be navigated to the last folder the user saved a file to.  However, this does open up some privacy concerns, and could create "firefox ruined my marriage" bugs.  I'll get some feedback from the rest of the ux team and post an answer.

Comment 11

[Jim Mathies [:jimm]]

Hmm, my suggested change doesn't look like it will work. A number places call into this with skip prompt set to null or false. For example the download manager calls into this when the user drags and drops an file into the download manager window. Those files would land in lastDir instead of the correct download directory.

The routine (getTargteFile in contentAreaUtils.js) is accessed in a number of places through various entry points - 

saveURL:
http://lxr.mozilla.org/mozilla/search?string=saveURL%28
saveImageURL:
http://lxr.mozilla.org/mozilla/search?string=saveImageURL%28
saveDocument:
http://lxr.mozilla.org/mozilla/search?string=+saveDocument%28
saveFrameDocument:
http://lxr.mozilla.org/mozilla/search?string=saveFrameDocument%28

Comment 12

[Alex Faaborg [:faaborg] (Firefox UX)]

The UX team is overwhelmingly in favor of navigating the save as dialog to the previous location, and not the user's default download directory.

Comment 13

[Jim Mathies [:jimm]]

Attachment: lastDir patch v.1

Ok, sorry, that last comment was off, I had the flag mixed up. Attached is the patch, I'd welcome folks testing each scenario out before this gets checked in to be sure it's working the way everyone wants it to. With this patch, things should work as follows:

Firefox Behavior:
-----------------

browser.download.useDownloadDir = true or false

  Content area context menu options:
  Save Image As
   - lastDir* (filepicker prompt)
  Save Link As
   - lastDir* (filepicker prompt)

  File menu options:
  Save Page As
   - lastDir* (filepicker prompt)

browser.download.useDownloadDir = true

  Unknown content dialog save as:
   - usersDownloadFolder (no filepicker prompt)

  Download Manager window drag and drop:
   - usersDownloadFolder (no filepicker prompt)

browser.download.useDownloadDir = false

  Unknown content dialog save as:
   - lastDir* (filepicker prompt)

  Download Manager window drag and drop:
   - lastDir* (filepicker prompt)

  * If lastDir points to a non-existent directory, defaults to
usersDownloadFolder.

Comment 14

[Reed Loden [:reed]]

Checking in toolkit/content/contentAreaUtils.js;
/cvsroot/mozilla/toolkit/content/contentAreaUtils.js,v  <--  contentAreaUtils.js
new revision: 1.97; previous revision: 1.96
done

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007102721 Minefield/3.0a9pre

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007102704 Minefield/3.0a9pre

Comment 16

[Stephen Donner [:stephend] Not actively reading bugmail]

https://litmus.mozilla.org/show_test.cgi?id=4726

in-litmus+

Comment 17

[Serge Gautherie (:sgautherie)]

This patch included a "LF -> CRLF" :-(
Can you fix the line ending ?

Comment 18

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #17)
> Can you fix the line ending ?

mozilla/toolkit/content/contentAreaUtils.js 	1.98