310071 - Use system calls to add files to the recent documents folder

Status: RESOLVED FIXED

(Toolkit :: Downloads API, enhancement)

Description

[Mike Connor [:mconnor]]

We want to consider using the SHAddToRecentDocs call via the shell service to
add downloaded files to the recent documents folder on Windows.  There will be a
hidden boolean pref to disable this behaviour if desired.

This is a 2.0 enhancement, and is a matter of playing nice with the system. 
Users concerned with privacy already have this disabled/hidden on their systems,
since most Windows apps support this well.

Comment 1

[Mike Connor [:mconnor]]

*** Bug 309798 has been marked as a duplicate of this bug. ***

Comment 2

[Mike Connor [:mconnor]]

*** Bug 310072 has been marked as a duplicate of this bug. ***

Comment 3

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

Attachment: patch

This works. I don't know if the error handling should be better, and I've noticed that sometimes the downloadCompleted handler isn't called when files are downloaded quickly. Maybe this would be better called from nsDownloadManager, but I don't see a better place offhand.

Comment 4

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

Attachment: smaller patch

Asaf pointed out that I can't depend on browser from toolkit, so I just put this in nsDownloadManager directly.

Comment 5

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

Comment on attachment 221914 [details] [diff] [review]
smaller patch

Hrm, actually, I see the document added to the "recent docs" list without this patch. Seems windows is doing this itself without our help...

Comment 6

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

Ok, the reason I was seeing this work without the patch is because I had "Ask me where to save every file" turned on, which opened the system "Save As" dialog, which apparently has hooks to add any saved items to the most recent documents list.

Comment 7

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

Attachment: patch v2

Comment 8

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

The last patch only worked because SHAddToRecentDocs takes a path in the platform native encoding (Windows-1252, as I understand it), so converting to UTF-8 isn't correct, and would not work for filenames that cannot be expressed using Windows-1252. There is no SHAddToRecentDocsW, so it looks like we need pass a PIDL that represents the path to the file to SHAddToRecentDocs instead of a path, and I'm not sure how to get one of those. I need help from someone who knows the windows shell APIs more than I do.

Comment 9

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

Attachment: slightly better patch

There are still a lot of things wrong with this patch, but it works.
- Putting win32 API calls in nsDownloadManager doesn't feel right
- SHParseDisplayName only works in >= WinXP
- String usage is probably wrong

Comment 10

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

Attachment: better patch

This uses a different method that will work in anything greater than 95, and makes sure not to leak the pidl.

Comment 11

[Benjamin Smedberg]

Comment on attachment 224365 [details] [diff] [review]
better patch

>Index: toolkit/components/downloads/src/nsDownloadManager.cpp

> #define PREF_BDM_FLASHCOUNT "browser.download.manager.flashCount"
>+#define PREF_BDM_ADDTORECENTDOCS "browser.download.manager.addtorecentdocs"

other prefs here use mixedCase, you should too.

>+    if (addToRecentDocs) {
>+      LPSHELLFOLDER lpShellFolder = NULL;
>+
>+      if (SUCCEEDED(::SHGetDesktopFolder(&lpShellFolder))) {
>+        PRUnichar *filePath = ToNewUnicode(path);
>+        LPITEMIDLIST lpItemIDList = NULL;
>+        if (SUCCEEDED(lpShellFolder->ParseDisplayName(NULL, NULL, filePath, NULL, &lpItemIDList, NULL))) {

Does this work even if the file is not saved to the desktop?

Comment 12

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

(In reply to comment #11)
> other prefs here use mixedCase, you should too.

will do.

> Does this work even if the file is not saved to the desktop?

Yes, the Desktop folder is special in that its implementation of ParseDisplayName accepts absolute paths to any file on disk.

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/shellcc/platform/shell/reference/ifaces/ishellfolder/parsedisplayname.asp 

Comment 13

[Mike Connor [:mconnor]]

pushing out non-critical-path bugs to b2

Comment 14

[Asaf Romano (gone)]

Comment on attachment 224365 [details] [diff] [review]
better patch


>+      if (SUCCEEDED(::SHGetDesktopFolder(&lpShellFolder))) {
>+        PRUnichar *filePath = ToNewUnicode(path);
>+        LPITEMIDLIST lpItemIDList = NULL;
>+        if (SUCCEEDED(lpShellFolder->ParseDisplayName(NULL, NULL, filePath, NULL, &lpItemIDList, NULL))) {

Hrm, i'm pretty sure you need W2COLE here to be on the safe side.

Comment 15

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

(In reply to comment #14)
> Hrm, i'm pretty sure you need W2COLE here to be on the safe side.

I discussed this with Asaf, and determined it wouldn't be necessary.

Checked in on the trunk.

mozilla/toolkit/components/build/Makefile.in 	1.37
mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp 	1.67

Comment 16

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

Comment on attachment 224365 [details] [diff] [review]
better patch

This has been on the trunk for nearly two weeks, with no known regressions.

Comment 17

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

mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp 	1.53.2.10
mozilla/toolkit/components/build/Makefile.in 	1.24.2.10

Comment 18

[Mel Reyes]

Had someone actually report this as a bug in our XULRunner app.  "browser.download.manager.addtorecentdocs" should be renamed as this applies to the toolkit not just "browser".

Comment 19

[Shawn Wilsher :sdwilsh]

All the download manager prefs are prefixed with browser, so I'm not so sure I agree...

Comment 20

[Mel Reyes]

maybe we should take the opportunity pre3.0 to realign preferences from browser to toolkit for portability.

Comment 21

[Aiko]

(In reply to comment #8)
> (...) There is no SHAddToRecentDocsW, so it looks like we need
> pass a PIDL that represents the path to the file to SHAddToRecentDocs instead
> of a path, and I'm not sure how to get one of those. I need help from someone
> who knows the windows shell APIs more than I do.

Gavin, sorry about the very belated comment but here goes: In this case Microsoft didn't implement ANSI/Unicode as two functions but instead they split the flag in two: if UNICODE is defined SHARD_PATH is set to SHARD_PATHW otherwise it's SHARD_PATHA. 

So, I believe lines 2209-2222 in nsDownloadManager.cpp could be replaced with

if (addToRecentDocs)
  ::SHAddToRecentDocs(SHARD_PATHW, path.get());

Comment 22

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

Aiko, could you file a new bug about that? Seems reasonable to me.