1194893 - Allow customization of the default file picker directory

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Garrett Robinson [:grobinson]]

When the user opens a file picker (e.g. to choose a location to download a file to, or to choose a file to upload for an <input type="file">), Firefox uses nsIContentPrefService2 to determine which directory to show the user at first. The service associates the last directories used for upload/download with each origin. If there is no associated directory, it defaults to the platform-specific Desktop directory, which is identified by NS_OS_DESKTOP_DIR.

I propose that the default location for the file-picker be customizable, e.g. through a pref in about:config.

This is a minor enhancement that is intended to assist in working around an issue with Tor Browser in Tails. If there is no interest in fixing this upstream, please just let me know (WONTFIX) and I will try to land this somewhere downstream.

Link to the issue on the Tails issue tracker: https://labs.riseup.net/code/issues/8917

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Might be easier to say yes or no if you had some code to show :)

Comment 2

[Garrett Robinson [:grobinson]]

(In reply to Olli Pettay [:smaug] (high review load) from comment #1)
> Might be easier to say yes or no if you had some code to show :)

Just wanted to make sure there was at least *some* chance of this landing upstream before taking time to write a patch :) I will write one in the next few weeks.

Comment 3

[Garrett Robinson [:grobinson]]

Attachment: Use user-specified fallback directory for uploads

The default value for the user-specified fallback directory for uploads pref is
"", which results in the default behavior of using the platform-specific
Desktop directory.

If the pref is modified, this code first checks that the user-specified path
points to a valid directory, and otherwise falls back to the default behavior.

Comment 4

[Garrett Robinson [:grobinson]]

Attachment: Add pref for default file picker directory

The default value for the new "dom.input.fallbackUploadsDir" pref is "", which
results in the default behavior of using the platform-specific Desktop
directory.

If the pref is modified, this code first checks that the user-specified path
points to a valid directory, and otherwise falls back to the default behavior.

Comment 5

[Garrett Robinson [:grobinson]]

Comment on attachment 8652696 [details] [diff] [review]
Add pref for default file picker directory

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

Please suggest another reviewer if you think someone else would be better, or you are too busy :)

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8652696 [details] [diff] [review]
Add pref for default file picker directory

Oh, the old code is a bit silly...

> UploadLastDir::ContentPrefCallback::HandleCompletion(uint16_t aReason)
> {
>+  nsresult rv = NS_OK;
>+
>   nsCOMPtr<nsIFile> localFile = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID);
here we create first localFile

>   NS_ENSURE_STATE(localFile);
> 
>   if (aReason == nsIContentPrefCallback2::COMPLETE_ERROR ||
>       !mResult) {
>-    // Default to "desktop" directory for each platform
>-    nsCOMPtr<nsIFile> homeDir;
>-    NS_GetSpecialDirectory(NS_OS_DESKTOP_DIR, getter_AddRefs(homeDir));
>-    localFile = do_QueryInterface(homeDir);
and here we assign some other object to the variable.


>+    // Use the custom fallback directory if it has been set in the
>+    // prefs and the custom path points to a valid directory.
>+    nsAdoptingString fallbackUploadDirPath =
>+      Preferences::GetString("dom.input.fallbackUploadDir");
>+
>+    if (!fallbackUploadDirPath.IsEmpty()) {
>+      nsCOMPtr<nsIFile> fallbackUploadDir = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID);
>+      fallbackUploadDir->InitWithPath(fallbackUploadDirPath);
>+      bool exists = false;
>+      rv = fallbackUploadDir->Exists(&exists);
>+      if (NS_SUCCEEDED(rv) && exists) {
>+        localFile = do_QueryInterface(fallbackUploadDir);
>+      }
>+    }
>+
>+    // XXX: What's the best way to ask "has localFile been initialized yet?"
So, move localFile = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID); to happen in the last 'else', and just declare 
nsCOMPtr<nsIFile> localFile where it is declared now.


>+    nsAutoString localFilePath;
>+    localFile->GetPath(localFilePath);
>+    if (localFilePath.IsEmpty()) {
...then you can just null check localFile here.


and could you write the code in a bit differently so that we don't need to have initWithPath twice.

Comment 7

[Garrett Robinson [:grobinson]]

(In reply to Olli Pettay [:smaug] (high review load) from comment #6)
> Comment on attachment 8652696 [details] [diff] [review]
> and could you write the code in a bit differently so that we don't need to
> have initWithPath twice.

I've implemented the rest of your review (patch incoming), but I'm not sure if I see how to do this. The first initWithPath is needed to create an nsIFile so we can use Exists() to make sure the directory specified by the custom path exists before trying to use it. If the directory doesn't exist, then we'd naturally need to do another InitWithPath with a known good path, so I don't see how this can be done with less than 2 calls to InitWithPath.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Why you actually need Exists? The other initWithPath case doesn't use it.

Could it be something like (in pseudo C++ )
nsCOMPtr<nsIFile> localFile;
nsAutoString prefStr;
if (aReason == nsIContentPrefCallback2::COMPLETE_ERROR || !mResult) {
  prefStr = Preferences::GetString("dom.input.fallbackUploadDir");
  if (prefStr.IsEmpty()) {
    nsCOMPtr<nsIFile> homeDir;
    NS_GetSpecialDirectory(NS_OS_DESKTOP_DIR, getter_AddRefs(homeDir));
    localFile = do_QueryInterface(homeDir);
  }
}

if (!localFile) {
  if (prefStr.IsEmpty() && mResult) {
     nsCOMPtr<nsIVariant> pref;
     mResult->GetValue(getter_AddRefs(pref));
     pref->GetAsAString(prefStr);
  }
  localFile = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID);
  localFile->InitWithPath(prefStr);
}
...

Comment 9

[Garrett Robinson [:grobinson]]

(In reply to Olli Pettay [:smaug] (high review load) from comment #8)
> Why you actually need Exists? The other initWithPath case doesn't use it.

The other initWithPath uses the value from the nsIContentPrefService2 (mresult), so it is a known directory on the filesystem (that was last associated with the given origin). The first InitWithPath uses the value from "dom.input.fallbackUploadDir", which is user-supplied, and so could potentially point to a non-existent location on the filesystem. My thought was that we should check (with Exists) to make sure the custom directory actually exists, and if it doesn't, explicitly fall back on the default "use the platform-specific Desktop" behavior.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

What guarantees the other directory (for which initWithPath is used) actually exists?

I'm mainly worried about doing .Exists() (sync I/O) in the main thread.

Comment 11

[Garrett Robinson [:grobinson]]

(In reply to Olli Pettay [:smaug] (high review load) from comment #10)
> What guarantees the other directory (for which initWithPath is used) actually exists?

On second thought, nothing - that was an unjustified assumption. Patch following the approach you outlined in comment #8 incoming.

Comment 12

[Garrett Robinson [:grobinson]]

Attachment: Add pref for default file picker directory

The default value for the new "dom.input.fallbackUploadsDir" pref is "", which
results in the default behavior of using the platform-specific Desktop
directory.

If the pref is modified, this code first checks that the user-specified path
points to a valid directory, and otherwise falls back to the default behavior.

Comment 13

[Garrett Robinson [:grobinson]]

Attachment: Bug 1194893 - Add pref for default file picker directory

This patch follows the approach outlined in Comment #8.

It does not bother checking if the user-supplied path value points to a valid directory before trying to use it - as :smaug pointed out, there's no reason to assume the value returned by nsIContentPrefService2 is valid at the time of use, and there's no special logic to handle that case either.

From testing on my Mac, opening the file picker when the new pref has an invalid value (e.g. "~/not/a/directory") results in it falling back on a previous value successfully used by the file picker. A debug build also prints a warning:

[Parent 34518] WARNING: An out of range index has been selected. Using the first index instead.: file /Users/garrett/mozilla-central/widget/cocoa/nsFilePicker.mm, line 515

Otherwise, the patch works as intended. When the pref is unset, it uses the original behavior of defaulting to the platform-specific Desktop directory. When the pref is set to a path that points to a valid directory, it is used as the default.

Comment 14

[Garrett Robinson [:grobinson]]

(Apologies for the bugspam from earlier patches - I'm trying to learn how to use hg bzexport, with mixed results).

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8653235 [details] [diff] [review]
Bug 1194893 - Add pref for default file picker directory

>+
>+// Bug 1194893: Allow customization of the fallback directory for file uploads
Drop 'Bug 1194893: '

One can always check the blame to see why the pref was added.

Comment 16

[Garrett Robinson [:grobinson]]

Attachment: Bug 1194893 - Add pref for default file picker directory

Removed unnecessary comment referencing bug number from all.js and added review info to commit message.

Comment 17

[Garrett Robinson [:grobinson]]

Attachment: Bug 1194893 - Add pref for default file picker directory.

Re-add comment for new pref in about.js (just removes the unnecessary "Bug 1194893").

Comment 18

[Garrett Robinson [:grobinson]]

I've rebased this patch and am testing it now. I'm waiting for my Mercurial access to be restored in Bug 1199372 before I can push to MozReview and run it against try. In the meantime, I've noticed that clicking a file upload form field's "Browse..." button results in the following warning on a debug build:

[Parent 22101] WARNING: An out of range index has been selected. Using the first index instead.: file /Users/garrett/mozilla-central/widget/cocoa/nsFilePicker.mm, line 515

I have determined that this error happens both with and without this patch applied, so either it is acceptable behavior or is a preexisting problem that should be resolved in a follow-up issue.

Comment 19

[Garrett Robinson [:grobinson]]

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

Comment 20

[Garrett Robinson [:grobinson]]

Builds failed:

/home/worker/workspace/build/src/dom/html/HTMLInputElement.cpp:298:12: error: unused variable 'rv' [-Werror=unused-variable]

Oops, rv is no longer used after the rewrite suggested in Comment 8. Fixing now.

Comment 21

[Garrett Robinson [:grobinson]]

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

Comment 22

[Garrett Robinson [:grobinson]]

Attachment: 1194893_final.diff

This is the final patch, with the unused variable from Comment 20 removed. Try looks good.

smaug, do I need a review from anyone else to land this? I see you're a DOM peer, so I expect your review is sufficient. Should I write a test?

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

My review is sufficient.

A test would be of course nice if not horribly hard to write.

Comment 24

[Garrett Robinson [:grobinson]]

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

Comment 25

[Garrett Robinson [:grobinson]]

Canceled that try build because "./mach try" only pushed the topmost commit (just the test) and not the prior commit needed to make the test past. I'm not sure how to get "./mach try" to push all commits.

Comment 26

[Garrett Robinson [:grobinson]]

Attachment: 1194893_test.diff

Test for pref that sets the default directory for file uploads. Tests the default directory (NS_OS_DESKTOP_DIR) is used when the pref is unset, and tests that the directory set by the pref is used when the pref is set.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8699154 [details] [diff] [review]
1194893_test.diff

nit, 2 spaces for indentation would be nice.

Better to make sure this passes on all the platforms, or possibly
enable only on desktop.
skip-if = buildapp == 'mulet' || buildapp == 'b2g' || toolkit == 'android'
in the mochitest.ini should work.

Comment 28

[Garrett Robinson [:grobinson]]

Attachment: 1194893_test.diff

Added skip info to mochitest.ini and fixed indentation in test.

Comment 29

[Garrett Robinson [:grobinson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/581264224b8d9bddcc0a17b48c836cf83a28804c
Bug 1194893 - Pref for default file upload directory. r=smaug

Comment 30

[Garrett Robinson [:grobinson]]

I rebased the two patches (for implementation and test) into 581264224b8d and pushed to inbound. Sorry for the weird workflow on this one - I'm used to the old-style of doing everything with patches on Bugzilla. I'll try to use the ReviewBoard for my next bug!

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Personally I still prefer patches in bugzilla. Faster to review code that way.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/581264224b8d