Last Comment Bug 702748 - Use a pref for disabling per-site remembering of download directory
: Use a pref for disabling per-site remembering of download directory
Status: RESOLVED FIXED
: user-doc-needed
Product: Toolkit
Classification: Components
Component: Downloads API (show other bugs)
: unspecified
: All All
: -- normal with 6 votes (vote)
: mozilla11
Assigned To: Geoff Lankow (:darktrojan)
:
: :Paolo Amadini
Mentors:
: 693153 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-15 13:25 PST by Geoff Lankow (:darktrojan)
Modified: 2011-12-28 23:45 PST (History)
6 users (show)
geoff: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (4.96 KB, patch)
2011-11-15 13:25 PST, Geoff Lankow (:darktrojan)
gavin.sharp: review-
Details | Diff | Splinter Review
patch v2 (4.38 KB, patch)
2011-11-17 02:26 PST, Geoff Lankow (:darktrojan)
no flags Details | Diff | Splinter Review
patch v3 (5.44 KB, patch)
2011-12-02 23:59 PST, Geoff Lankow (:darktrojan)
gavin.sharp: review+
Details | Diff | Splinter Review

Description Geoff Lankow (:darktrojan) 2011-11-15 13:25:45 PST
Created attachment 574665 [details] [diff] [review]
patch

Lots of users are complaining about this feature, so we should have a pref to disable it.
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-11-16 16:00:16 PST
Comment on attachment 574665 [details] [diff] [review]
patch

Looks like you moved too much code into the if (isContentPrefEnabled()) block in getFile - doesn't this break the private browsing mode behavior when the pref is flipped?
Comment 2 Geoff Lankow (:darktrojan) 2011-11-17 02:26:39 PST
Created attachment 575127 [details] [diff] [review]
patch v2
Comment 3 Atte Kemppilä [:atte] 2011-11-20 05:32:47 PST
If I'm reading that correctly, currently you can clear the normal last dir pref, but not the per-site pref.

gDownloadLastDir.setFile(null, null) - ok
gDownloadLastDir.setFile(uri, null) - throws

With the above patch applied:

gDownloadLastDir.setFile(null, null) - ok
gDownloadLastDir.setFile(uri, null) - ok or throws depending on savePerSite value

Why not make it possible to clear the per-site pref like just the normal one -> more consistent api.

Something like:

if (aURI && isContentPrefEnabled()) {
  if (aFile instanceof Components.interfaces.nsIFile)
    Services.contentPrefs.setPref(aURI, LAST_DIR_PREF, aFile.path);
  else
    Services.contentPrefs.removePref(aURI, LAST_DIR_PREF);
}


Also, should the comment at the beginning of file updated to include all this per-site stuff.
Comment 4 Geoff Lankow (:darktrojan) 2011-11-23 15:15:30 PST
Thanks Atte, that's a good point. Throwing when passed null was unintentional. I'll fix it.
Comment 5 Geoff Lankow (:darktrojan) 2011-12-02 23:59:36 PST
Created attachment 578814 [details] [diff] [review]
patch v3
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-09 12:28:04 PST
Comment on attachment 578814 [details] [diff] [review]
patch v3

Sorry for the delay, Geoff. I'll be better next time! And if I'm not, yell at me loudly!
Comment 7 Geoff Lankow (:darktrojan) 2011-12-11 20:36:45 PST
https://hg.mozilla.org/mozilla-central/rev/1e81f179adf2
Comment 8 David E. Ross 2011-12-14 11:38:51 PST
In which end-user release will this be fixed?
Comment 9 Jens Hatlak (:InvisibleSmiley) 2011-12-14 12:33:46 PST
(In reply to David E. Ross from comment #8)
> In which end-user release will this be fixed?

As the target milestone says: mozilla11 (FF/TB 11 / SM 2.8).
Comment 10 :aceman 2011-12-28 23:35:24 PST
And the most important information is missing in the comments:
The pref is called "browser.download.lastDir.savePerSite" and is accessible in about:config (if not, create it as a bool).
Comment 11 :aceman 2011-12-28 23:44:23 PST
*** Bug 693153 has been marked as a duplicate of this bug. ***
Comment 12 Geoff Lankow (:darktrojan) 2011-12-28 23:45:56 PST
(In reply to :aceman from comment #10)
> And the most important information is missing in the comments:
> The pref is called "browser.download.lastDir.savePerSite"

Good point.

> and is accessible in about:config (if not, create it as a bool).

Not by default, users will need to add it.

Note You need to log in before you can comment on or make changes to this bug.