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

RESOLVED FIXED in mozilla11

Status

()

Toolkit
Downloads API
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

({user-doc-needed})

unspecified
mozilla11
user-doc-needed
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 574665 [details] [diff] [review]
patch

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-
(Assignee)

Comment 2

6 years ago
Created attachment 575127 [details] [diff] [review]
patch v2
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.
(Assignee)

Comment 4

6 years ago
Thanks Atte, that's a good point. Throwing when passed null was unintentional. I'll fix it.
(Assignee)

Comment 5

6 years ago
Created attachment 578814 [details] [diff] [review]
patch v3
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+
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/mozilla-central/rev/1e81f179adf2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla11

Comment 8

5 years ago
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).

Comment 10

5 years ago
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).

Updated

5 years ago
Duplicate of this bug: 693153
(Assignee)

Comment 12

5 years ago
(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.