Closed Bug 702748 Opened 8 years ago Closed 8 years ago

Use a pref for disabling per-site remembering of download directory

Categories

(Toolkit :: Downloads API, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

(Keywords: user-doc-needed)

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
Lots of users are complaining about this feature, so we should have a pref to disable it.
Attachment #574665 - Flags: review?(gavin.sharp)
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?
Attachment #574665 - Flags: review?(gavin.sharp) → review-
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #574665 - Attachment is obsolete: true
Attachment #575127 - Flags: review?(gavin.sharp)
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.
Thanks Atte, that's a good point. Throwing when passed null was unintentional. I'll fix it.
Attached patch patch v3Splinter Review
Attachment #575127 - Attachment is obsolete: true
Attachment #575127 - Flags: review?(gavin.sharp)
Attachment #578814 - Flags: review?(gavin.sharp)
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!
Attachment #578814 - Flags: review?(gavin.sharp) → review+
https://hg.mozilla.org/mozilla-central/rev/1e81f179adf2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
In which end-user release will this be fixed?
(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).
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).
Duplicate of this bug: 693153
(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.
Keywords: user-doc-needed
You need to log in before you can comment on or make changes to this bug.