Closed Bug 524408 Opened 10 years ago Closed 10 years ago

input[type=file] should remember last used directory on a site-by-site basis


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

Not set





(Reporter: darktrojan, Assigned: darktrojan)




(1 file, 5 obsolete files)

Attached patch patch V1 (obsolete) — Splinter Review
So that if I always upload files to site A from directory ~/x, and to site B from directory ~/y/z, I don't have to navigate to from one directory to the other when I want to upload something. Particularly if I've got both sites open in different tabs.
Is this pref exposed in the site-prefs UI too? Or can you get to it through about:config?
I've just realised |oldFiles.Count()| returns 1 even if there's no file, because somebody stores an empty string in it. A bit of rearranging is required.

It's not exposed anywhere, just like page zoom level. It could be, I suppose, but I don't think it needs to be.
Attached patch Patch V2 (obsolete) — Splinter Review
This time it actually does what it's supposed to.
Attachment #408317 - Attachment is obsolete: true
Hmm.. can you find why an empty string gets stored? That sounds like a bug.
Depends on: 524421
Comment on attachment 408317 [details] [diff] [review]
patch V1

Un-obsoleting this patch on the assumption that bug 524421 will be fixed.

Reviewers: I'm new to doing XPCOM from C++, please nitpick as I've probably stuffed up something somewhere. :)
Attachment #408317 - Attachment description: WIP patch → patch V1
Attachment #408317 - Attachment is obsolete: false
Attachment #408317 - Flags: superreview?(bzbarsky)
Attachment #408317 - Flags: review?(jonas)
Attachment #408321 - Attachment description: WIP patch → Patch V2
Attachment #408321 - Attachment is obsolete: true
Benjamin, is it ok for core gecko code to assume that toolkit services like ";1" are present?  I'd assume not...
Well, I'd say yes... there are no configurations in which that code is not actually present. But that depends on what kind of assumptions you're talking about: you'd still deal with a null service gracefully by say, not remembering any directories, right?
Well, the patch here crashes, but that's just a bug.  I guess the question is whether it makes sense to try to fall back to the current behavior or just go with not remembering if no service.
Don't fallback, just skip the remembering.
Attached patch patch v3 (obsolete) — Splinter Review
Something a little like this?
Comment on attachment 408692 [details] [diff] [review]
patch v3

>+  nsresult prefServiceResult;
>+  nsCOMPtr<nsIContentPrefService> contentPrefService =
>+    do_GetService(";1", &prefServiceResult);

Probably better to just ignore the nsresult (don't bother saving it) and null-check contentPrefService as needed.

>+  nsCOMPtr<nsIWritableVariant> uri = do_CreateInstance(";1");
>+  uri->SetAsISupports(doc->GetDocumentURI());

Check for OOM creating URI?

>+  nsString prefName = NS_LITERAL_STRING("lastUploadDirectory");

NS_NAMED_LITERAL_STRING(prefName, "lastUploadDirectory);

>+  } else if (NS_SUCCEEDED(prefServiceResult)) {

So this would become |else if (contentPrefService)|

>+    PRBool hasPref;
>+    contentPrefService->HasPref(uri, prefName, &hasPref);
>+    if (hasPref) {

Can we check that HasPref returned success?  If not, hasPref has a random value and we probably shouldn't branch on i.

>+      nsIVariant* pref;
>+      contentPrefService->GetPref(uri, prefName, &pref);

This leaks |pref|.  Please use nsCOMPtr<nsIVariant> and getter_AddRefs.

>+      nsCOMPtr<nsILocalFile> localFile = do_CreateInstance(";1");
>+      localFile->InitWithPath(prefStr);
>+      filePicker->SetDisplayDirectory(localFile);

There needs to be some error-checking here, no?  Both on the do_CreateInstance and the InitWithPath...

There are various gratuitous whitspace changes in this diff. Can you avoid making those?

>+    PRBool prefSaved = false;


>+        if (!prefSaved && NS_SUCCEEDED(prefServiceResult)) {
>+          nsCOMPtr<nsIFile> parentFile;
>+          file->GetParent(getter_AddRefs(parentFile));
>+          result = parentFile->GetPath(unicodePath);

What's the point of storing the return value in |result| if you then ignore |result|?  Ignoring seems OK; just don't store.

>+            nsCOMPtr<nsIWritableVariant> prefValue = do_CreateInstance(";1");

Need to null-check the return value.

>+      // Save the file's directory to the contentPref service

Can we factor the code out into a static function or something, so we don't have two copies of it?
Attachment #408692 - Flags: review-
Attachment #408317 - Flags: superreview?(bzbarsky) → superreview-
Oh, and to be clear this looks pretty good in general; just need to pick the nits.  ;)
Attached patch patch v4 (obsolete) — Splinter Review
(In reply to comment #11)
> Can we factor the code out into a static function or something, so we don't
> have two copies of it?

Yes we can, in fact I've done the same with the other half of it too. It should make it easier to use if (when?) someone gets around to making a better multiple file upload UI.

> +#include "nsIContentPrefService.h"
If the CPS isn't present (comment #6), build will fail. Also I think this .h file is created after nsFileControlFrame.cpp is compiled, so that also fails. I don't know how to deal with either.
Attachment #408317 - Attachment is obsolete: true
Attachment #408692 - Attachment is obsolete: true
Attachment #409658 - Flags: review?(bzbarsky)
Attachment #408317 - Flags: review?(jonas)
> Also I think this .h file is created after nsFileControlFrame.cpp is compiled,

Benjamin, what should the ordering be there?
nsIContentPrefService.h is currently part of tier_toolkit, which is built after this code (which is in tier_gecko). I'm considering if we should just ditch our tiers entirely (or more specifically have one tier for all of xpcom/necko/gecko/toolkit, and one tier for Firefox).

The expedient way to solve this is to move nsIContentPrefService.idl into tier_gecko somewhere such as dom/interfaces/base
Attachment #409658 - Flags: review-
Comment on attachment 409658 [details] [diff] [review]
patch v4

+    nsCOMPtr<nsILocalFile> localFile = do_CreateInstance(";1");
+    if (!localFile)
+      return NS_ERROR_OUT_OF_MEMORY;

please don't do this,

do use:

nsCOMPtr<nsIFoo> foo(do_CreateInstance(CONTRACT_FOO, &rv));
if (NS_FAILED(rv))
  return rv;
Why? I think that the rv-outparam version of do_CreateInstance is not especially more valuable and should not be used in normal circumstances. I do think that using the _CONTRACTID macro is a good idea.
Comment on attachment 409658 [details] [diff] [review]
patch v4

>+nsFileControlFrame::FetchLastUsedDirectory(nsIURI* aURI, nsILocalFile* aFile)

This doesn't need to be NS_IMETHODIMP.  Just make it return nsresult.

>+  // Attempt to get the CPS, if it's not present we'll just return
>+  nsCOMPtr<nsIContentPrefService> contentPrefService =
>+    do_GetService(";1");
>+  if (!contentPrefService)
>+    return NS_OK;

NS_ERROR_NOT_AVAILABLE would make more sense here, I think.

>+nsFileControlFrame::StoreLastUsedDirectory(nsIURI* aURI, nsIFile* aFile)

And this should return void, since nothing cares about the return value.

> +++ b/layout/forms/nsFileControlFrame.h
>+  /**
>+   * Fetch the last used directory for this location from the
>+   * content pref service, if it is available
>+   * @param aURI URI of the current page
>+   * @param aFile path to the last used directory
>+   */
>+  static NS_IMETHODIMP FetchLastUsedDirectory(nsIURI* aURI,
>+                              nsILocalFile* aFile);

This should more clearly document that the caller must create the aFile object and this function will initialize it to the right file.  Also, please line up the two arguments and make the method just |static nsresult|

>+  static NS_IMETHODIMP StoreLastUsedDirectory(nsIURI* aURI,
>+                              nsIFile* aFile);

|static void|

r=bzbarsky with those nits addressed, the idl file moved per comment 15, and the macro thing from comment 17 addressed.  Thanks for doing this, and sorry for the lag...
Attachment #409658 - Flags: review?(bzbarsky) → review+
Attached patch patch v5 (obsolete) — Splinter Review
I think I've got all that. I also added a call to exists() to avoid an error message.
Attachment #409658 - Attachment is obsolete: true
Attachment #418778 - Flags: review?(bzbarsky)
Bugzilla doesn't show the file rename ... I assume hg can handle it though, right?
Attached patch patch v5.1Splinter Review
Let's try that again without that pointless exists call which I coded wrong, doesn't compile and doesn't achieve anything anyway (even though I swear it worked earlier).
Attachment #418778 - Attachment is obsolete: true
Attachment #418805 - Flags: review?(bzbarsky)
Attachment #418778 - Flags: review?(bzbarsky)
> Bugzilla doesn't show the file rename

You mean bugzilla diff?  It's just broken.  The attached patch has:

diff --git a/toolkit/components/contentprefs/public/nsIContentPrefService.idl b/dom/interfaces/base/nsIContentPrefService.idl
rename from toolkit/components/contentprefs/public/nsIContentPrefService.idl
rename to dom/interfaces/base/nsIContentPrefService.idl

which is good.  ;)
Comment on attachment 418805 [details] [diff] [review]
patch v5.1

r=bzbarsky; I'll fix up the comments for FetchLastUsedDirectory before pushing.
Attachment #418805 - Flags: review?(bzbarsky) → review+

Geoff, thanks for working on this!
Closed: 10 years ago
Resolution: --- → FIXED
This should get a user-doc for the change...
Keywords: user-doc-needed
Depends on: 536567
Filed bug 536567 to make this behave correctly in interaction with the private browsing mode.
(In reply to comment #25)
> This should get a user-doc for the change...

Could someone explain in user-terms what this change is? From what I read, the file-picker (when uploading to a site) opens in the directory the file-picker was in the last time it was used. This bug changes it to open in the directory that was last used for that site. Is that correct?

Also, this will first be seen in Firefox 3.7?
That is correct, and yes, 3.7.
Natch, we don't have any user-docs that need to be updated because of this. Were you thinking of a *new* knowledge base article? If so, I really don't see the need.
Removing user-doc-needed. Got no answer to comment 29.
Keywords: user-doc-needed
Sorry about that, could've sworn I replied...

Anyhow, if you feel there is no need for a user-doc, that's obviously your choice, I just felt that since this would be a user-facing change (one that may either be worthy of noting, or worthy of warning) it should get a user-doc, preempting any questions or complaints, etc.
Awesome! Thanks.
Depends on: 610365
Component: HTML: Form Submission → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.