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

Status: RESOLVED FIXED

Description

[Geoff Lankow (:darktrojan)]

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: [e-mail]]]

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)]

Created attachment 575127 [details] [diff] [review]
patch v2

Comment 3

[Atte Kemppilä [:atte]]

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)]

Thanks Atte, that's a good point. Throwing when passed null was unintentional. I'll fix it.

Comment 5

[Geoff Lankow (:darktrojan)]

Created attachment 578814 [details] [diff] [review]
patch v3

Comment 6

[:Gavin Sharp [email: [e-mail]]]

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)]

https://hg.mozilla.org/mozilla-central/rev/1e81f179adf2

Comment 8

[David E. Ross]

In which end-user release will this be fixed?

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

(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]

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 12

[Geoff Lankow (:darktrojan)]

(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.