Last Comment Bug 646351 - browser.download.manager.addToRecentDocs = false, doesn't work, still adds to recent docs
: browser.download.manager.addToRecentDocs = false, doesn't work, still adds to...
Status: NEW
:
Product: Toolkit
Classification: Components
Component: Download Manager (show other bugs)
: unspecified
: x86 Windows XP
: -- normal with 1 vote (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on: 651093
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-29 23:08 PDT by al_9x
Modified: 2011-05-23 13:30 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1 - content utils (1015 bytes, patch)
2011-05-23 13:30 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Review

Description al_9x 2011-03-29 23:08:37 PDT
User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0) Gecko/20100101 Firefox/4.0

Fx 4.0.0 & 3.6.16


Reproducible: Always

Steps to Reproduce:
1. xp sp3, Fx 4.0.0, new profile
2. set browser.download.manager.addToRecentDocs = false
3. right click on a link (e.g. "Our Mission" - http://www.mozilla.org/about/mission.html on http://www.mozilla.org/)
4. "Save Link As..."
5. after it's downloaded, mission.html is added to recent docs
Comment 1 XtC4UaLL [:xtc4uall] 2011-03-30 09:47:25 PDT
Can you test against 3.5/3.0 too?
Comment 2 al_9x 2011-03-30 10:22:54 PDT
(In reply to comment #1)
> Can you test against 3.5/3.0 too?

same
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-03-30 14:03:43 PDT
See bug 310071 comment 6 - we don't explicitly make the call to add the document if the pref is false, but if you use the file picker to save the file, Windows will add the file to recent docs on its own - I don't think we can control that. 

Our setting should take effect when you have the download manager automatically save files to your downloads folder without asking.

Can you confirm that that explains what you're seeing?
Comment 4 al_9x 2011-03-30 15:11:14 PDT
(In reply to comment #3)
> See bug 310071 comment 6 - we don't explicitly make the call to add the
> document if the pref is false, but if you use the file picker to save the file,
> Windows will add the file to recent docs on its own - I don't think we can
> control that.

OFN_DONTADDTORECENT
http://msdn.microsoft.com/en-us/library/ms646839%28VS.85%29.aspx

> 
> Our setting should take effect when you have the download manager automatically
> save files to your downloads folder without asking.

If it's as easy as passing OFN_DONTADDTORECEN to GetSaveFileName, your setting should take effect regardless.
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-04-16 18:13:46 PDT
Ah, good find. The file picker code (where we call GetSaveFileName) is lower level and generic (used by all gecko platform consumers, not just the download manager), so ideally we'd make that behavior controllable for nsIFilePicker users, but perhaps as a first step we should just make OFN_DONTADDTORECEN the default.
Comment 6 Shawn Wilsher :sdwilsh 2011-04-17 11:54:42 PDT
(In reply to comment #5)
> Ah, good find. The file picker code (where we call GetSaveFileName) is lower
> level and generic (used by all gecko platform consumers, not just the download
> manager), so ideally we'd make that behavior controllable for nsIFilePicker
> users, but perhaps as a first step we should just make OFN_DONTADDTORECEN the
> default.
Rob or Jim, do you have thoughts on the best approach here?
Comment 7 Robert Strong [:rstrong] (use needinfo to contact me) 2011-04-18 16:24:21 PDT
I personally don't think OFN_DONTADDTORECEN should be the default. Besides that comment #5 seems like the way to go.
Comment 8 Jim Mathies [:jimm] 2011-04-18 16:42:24 PDT
IMHO, we should tie these settings to privacy mode. If not active, we should populate the information in windows and get rid of the pref.
Comment 9 al_9x 2011-04-18 16:51:52 PDT
(In reply to comment #7)
> I personally don't think OFN_DONTADDTORECEN should be the default. Besides that
> comment #5 seems like the way to go.

It's pretty senseless behavior for GetSaveFileName to add to recent docs by default, because all it does is construct a path, the actual file may never be created.  If you cancel a download in Fx, the recent docs entry remains for a nonexistent file.

Why should recent docs be polluted with phantom entries?  If a recent docs entry is desired, it should be created manually upon a successful saving of the file (as already happens), not by GetSaveFileName.  So OFN_DONTADDTORECENT is quite a sensible default.  Why are you against it?
Comment 10 Robert Strong [:rstrong] (use needinfo to contact me) 2011-04-18 16:57:12 PDT
It wasn't clear that saved files would be added by default to the recent list. If they are on success then even better.
Comment 11 al_9x 2011-04-18 17:03:07 PDT
(In reply to comment #8)
> IMHO, we should tie these settings to privacy mode. If not active, we should
> populate the information in windows and get rid of the pref.

It's not (only) a privacy issue.  I only want to see docs I explicitly opened in recent docs, keeping downloads out has utility.

But even from a privacy perspective, there are prefs to individually control all privacy related modules (cookies, history, cache, downloads, etc) when not in privacy mode, why would you take away that ability for recent docs?
Comment 12 al_9x 2011-04-18 17:09:01 PDT
(In reply to comment #10)
> It wasn't clear that saved files would be added by default to the recent list.
> If they are on success then even better.

Not sure if I understood you, so you agree now, knowing that there is a manual add to recent on download finish, that OFN_DONTADDTORECENT would be a good default?
Comment 13 Robert Strong [:rstrong] (use needinfo to contact me) 2011-04-18 17:22:08 PDT
Correct. I would go as far as to say that if all call sites handle success / failure that it would be good to just use OFN_DONTADDTORECENT and require call sites to add themselves to the recent list.
Comment 14 Jim Mathies [:jimm] 2011-04-18 18:06:17 PDT
(In reply to comment #11)
> (In reply to comment #8)
> > IMHO, we should tie these settings to privacy mode. If not active, we should
> > populate the information in windows and get rid of the pref.
> 
> It's not (only) a privacy issue.  I only want to see docs I explicitly opened
> in recent docs, keeping downloads out has utility.
> 
> But even from a privacy perspective, there are prefs to individually control
> all privacy related modules (cookies, history, cache, downloads, etc) when not
> in privacy mode, why would you take away that ability for recent docs?

Because we have too many prefs which add complexity to the code making things slower, bloated, and hard to maintain. When the recent docs code in download manager was written we didn't have a privacy mode. Now we do, so maybe it's time to eliminate the pref and rely solely on privacy settings.
Comment 15 al_9x 2011-04-18 19:11:44 PDT
(In reply to comment #14)
> Because we have too many prefs which add complexity to the code making things
> slower, bloated, and hard to maintain.

The following code is not complex, bloated, slow, or hard to maintain.  Are you overwhelmed by it? 
______________________________________________________________________
PRBool addToRecentDocs = PR_TRUE;
if (pref)
  pref->GetBoolPref(PREF_BDM_ADDTORECENTDOCS, &addToRecentDocs);

if (addToRecentDocs &&
    !nsDownloadManager::gDownloadManagerService->mInPrivateBrowsing) {
  ::SHAddToRecentDocs(SHARD_PATHW, path.get());
}
______________________________________________________________________


> When the recent docs code in download
> manager was written we didn't have a privacy mode. Now we do, so maybe it's
> time to eliminate the pref and rely solely on privacy settings.

You didn't have privacy mode when most or all the prefs controlling functionality with privacy implications were added.  Do you want to take them all away?  Leave the user with no control over cookies, history, cache, downloads in normal browsing mode?

And did you miss the part about this being not only a privacy feature?  There is value in trying to limit recent docs to files you explicitly open.
Comment 16 Shawn Wilsher :sdwilsh 2011-04-18 21:06:30 PDT
(In reply to comment #15)
> The following code is not complex, bloated, slow, or hard to maintain.  Are you
> overwhelmed by it? 
I am, and I'm the module owner of this code, although I'm not advocating for its removal at this time.
Comment 17 al_9x 2011-04-18 22:22:16 PDT
(In reply to comment #16)
> I am, and I'm the module owner of this code, although I'm not advocating for
> its removal at this time.

I guess I shouldn't be surprised, you found the following beyond your capacity to maintain in Bug 178506
______________________________________________________________________

PRBool setLastModifiedTime = PR_FALSE;
if (pref)
    pref->GetBoolPref(PREF_BDM_SETLASTMODIFIEDTIME,&setLastModifiedTime);
if (setLastModifiedTime)
    (void)file->SetLastModifiedTime(GetLastModifiedTime(mRequest));
______________________________________________________________________

The two snippets of pref code I quoted are as trivial as it gets.  The idea that you are genuinely overwhelmed by their complexity, bloat, and unmaintainability is absurd.  I am seeing this "pref=(slow, bloated, unmaintainable)" mantra repeated more and more often, but it's an excuse only, because it's a lie.  There is perhaps much bloated, slow, unmaintainable code in Mozilla, but these trivial, tiny snippets of pref code are not it.
Comment 18 Jim Mathies [:jimm] 2011-04-19 04:39:51 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > Ah, good find. The file picker code (where we call GetSaveFileName) is lower
> > level and generic (used by all gecko platform consumers, not just the download
> > manager), so ideally we'd make that behavior controllable for nsIFilePicker
> > users, but perhaps as a first step we should just make OFN_DONTADDTORECEN the
> > default.
> Rob or Jim, do you have thoughts on the best approach here?

I really don't have strong feelings on this, although I suspect for every user we have that thinks this should be the default, we have an equal number of users who rely on the behavior and would be upset if we changed it. We've probably had this since version 1.0. I don't think though that we should add yet another pref to control whether or not we use it. :)

I'm more curious if this represents a privacy leak while in privacy mode. That option should be set when saving a file in that case. I'll do some testing to see if it is.

On removing the pref, that's out of scope for this bug, so I'll file a new one. Recent Docs is pretty much obsolete on Win7, it's not visible by default and the option to turn it on is buried deep in taskbar settings. In general it's been replace by relevant search through the start menu.
Comment 19 Jim Mathies [:jimm] 2011-05-23 13:30:17 PDT
Created attachment 534544 [details] [diff] [review]
part 1 - content utils

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