Closed
Bug 524408
Opened 15 years ago
Closed 15 years ago
input[type=file] should remember last used directory on a site-by-site basis
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
People
(Reporter: darktrojan, Assigned: darktrojan)
References
Details
Attachments
(1 file, 5 obsolete files)
8.83 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | 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?
Assignee | ||
Comment 2•15 years ago
|
||
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.
Assignee | ||
Comment 3•15 years ago
|
||
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.
Assignee | ||
Comment 5•15 years ago
|
||
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)
Assignee | ||
Updated•15 years ago
|
Attachment #408321 -
Attachment description: WIP patch → Patch V2
Attachment #408321 -
Attachment is obsolete: true
Comment 6•15 years ago
|
||
Benjamin, is it ok for core gecko code to assume that toolkit services like "@mozilla.org/content-pref/service;1" are present? I'd assume not...
Comment 7•15 years ago
|
||
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?
Comment 8•15 years ago
|
||
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.
Comment 9•15 years ago
|
||
Don't fallback, just skip the remembering.
Assignee | ||
Comment 10•15 years ago
|
||
Something a little like this?
Comment 11•15 years ago
|
||
Comment on attachment 408692 [details] [diff] [review] patch v3 >+ nsresult prefServiceResult; >+ nsCOMPtr<nsIContentPrefService> contentPrefService = >+ do_GetService("@mozilla.org/content-pref/service;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("@mozilla.org/variant;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("@mozilla.org/file/local;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; PR_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("@mozilla.org/variant;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-
Updated•15 years ago
|
Attachment #408317 -
Flags: superreview?(bzbarsky) → superreview-
Comment 12•15 years ago
|
||
Oh, and to be clear this looks pretty good in general; just need to pick the nits. ;)
Assignee | ||
Comment 13•15 years ago
|
||
(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)
Comment 14•15 years ago
|
||
> Also I think this .h file is created after nsFileControlFrame.cpp is compiled,
Benjamin, what should the ordering be there?
Comment 15•15 years ago
|
||
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 16•15 years ago
|
||
Comment on attachment 409658 [details] [diff] [review] patch v4 + nsCOMPtr<nsILocalFile> localFile = do_CreateInstance("@mozilla.org/file/local;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;
Comment 17•15 years ago
|
||
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 18•15 years ago
|
||
Comment on attachment 409658 [details] [diff] [review] patch v4 >+NS_IMETHODIMP >+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("@mozilla.org/content-pref/service;1"); >+ if (!contentPrefService) >+ return NS_OK; NS_ERROR_NOT_AVAILABLE would make more sense here, I think. >+NS_IMETHODIMP >+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+
Assignee | ||
Comment 19•15 years ago
|
||
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)
Assignee | ||
Comment 20•15 years ago
|
||
Bugzilla doesn't show the file rename ... I assume hg can handle it though, right?
Assignee | ||
Comment 21•15 years ago
|
||
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)
Comment 22•15 years ago
|
||
> 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 23•15 years ago
|
||
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+
Comment 24•15 years ago
|
||
Pushed http://hg.mozilla.org/mozilla-central/rev/566bc0cea950 Geoff, thanks for working on this!
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 26•15 years ago
|
||
Filed bug 536567 to make this behave correctly in interaction with the private browsing mode.
Comment 27•14 years ago
|
||
(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?
Assignee | ||
Comment 28•14 years ago
|
||
That is correct, and yes, 3.7.
Comment 29•14 years ago
|
||
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.
Comment 30•14 years ago
|
||
Removing user-doc-needed. Got no answer to comment 29.
Keywords: user-doc-needed
Comment 31•14 years ago
|
||
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.
Comment 32•14 years ago
|
||
Awesome! Thanks.
Updated•5 years ago
|
Component: HTML: Form Submission → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•