670275 - Downloading a file should put it in the system's recently-used list

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Federico Mena-Quintero]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 2011061500

Steps to reproduce:

When I download a file with Firefox, it would be useful to later have that file show up in the system's various recently-used file lists.  Currently it seems that Firefox doesn't add downloaded files to the recently-used list.

Comment 1

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

We do this on Windows using the SHAddToRecentDocs API. Is there a suitable equivalent for Linux?

Comment 2

[Federico Mena-Quintero]

Yes; the GTK+ version can use gtk_recent_manager_add_*() (and this will work for KDE as well, as the data is stored in the same place).  I'll try to produce a patch.

Comment 3

[Federico Mena-Quintero]

Attachment: firefox-downloads-to-recent-files.diff

This is an untested patch - I hope it's a good starting point.  It has been a while since I last touched the Mozilla sources, so here are the things that I'm not sure are correct here:

* is XP_GNOME the right platform-specific macro I want?

* I'm doing path.get() and passing that to a function that expects a (char*) - is that correct?

* I'm not sure whether the GTK+ includes are included at that point.

I would really appreciate it if someone could help sanitize this patch :)

Comment 4

[JP Rosevear [:jpr]]

Karl can probably give some feedback.

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 544933 [details] [diff] [review]
firefox-downloads-to-recent-files.diff

(In reply to comment #3)
> This is an untested patch - I hope it's a good starting point.

Yes, looks like the right approach.
Normally we use 8 lines of context in patches to make review easier.
Thanks for the function names :)

> * is XP_GNOME the right platform-specific macro I want?

MOZ_WIDGET_GTK2 is the one that means that GTK is available.

> * I'm doing path.get() and passing that to a function that expects a (char*)
> - is that correct?

.get() would provide a char* if path were an nsCString but in this case it is
a UTF16 nsString.

Here, fileURL->GetFilePath(nsACString & aFilePath) is probably simplest.
(We could get all concerned about the hostname because the browser could be
running on a remote machine, but it seems unlikely that someone would often
download with a browser on a remote machine.)

> * I'm not sure whether the GTK+ includes are included at that point.

Probably not.  I expect the Makefile.in will need something like

CXXFLAGS += $(TK_CFLAGS)

In C++, usually variable declarations are deferred to the initialization, and,
in Gecko, there is usually no space between function names and the "(". e.g.

GtkRecentManager manager = gtk_recent_manager_get_default();

Don't free GTK's GtkRecentManager.

Comment 6

[Hubert Figuiere [:hub]]

Attachment: Downloading a file should put it in the system's recently-used list. [f=karlt]

Comment 7

[Hubert Figuiere [:hub]]

This is the reworked patch according to the feedback. Tested on Fedora. Ready for review.

Comment 8

[Hubert Figuiere [:hub]]

Comment on attachment 604290 [details] [diff] [review]
Downloading a file should put it in the system's recently-used list. [f=karlt]

Karl, if you are not the person to review, let me know who I shall request it to. Thanks.

This is the reworked patch based on Federico.

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 604290 [details] [diff] [review]
Downloading a file should put it in the system's recently-used list. [f=karlt]

>+            char* uri = g_strconcat("file://", NS_ConvertUTF16toUTF8(path).get(), NULL);

Can you make this a gchar* please, to be explicit?

And please use g_filename_to_uri as I expect gtk_recent_manager_add_item() wants an escaped ASCII-encoded URI
(because GTK is GLib-based).

gchar* uri = g_filename_to_uri(NS_ConvertUTF16toUTF8(path).get(), NULL, NULL);

|uri| will then need to be null-checked.

Comment 10

[Hubert Figuiere [:hub]]

Landed Inbound
https://hg.mozilla.org/integration/mozilla-inbound/

Comment 11

[Hubert Figuiere [:hub]]

I meant
https://hg.mozilla.org/integration/mozilla-inbound/rev/118f97e6d4ed

Comment 12

[Jonathan Kew [:jfkthame]]

This turned xpcshell tests orange on Linux (apparently hanging in a download manager test), so I backed it out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6da02c90a5de

Comment 13

[Hubert Figuiere [:hub]]

I'll submit a new patch when I have solution.

Comment 14

[Karl Tomlinson (:karlt)]

Do you know what the problem is?
I don't seem to have the logs on tbpl.

Comment 15

[Hubert Figuiere [:hub]]

Sorry I got sidetracked, I forgot this bug.

Here is the xpcshell log.

TEST-PASS | /home/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/components/downloads/test/unit/test_bug_401582.js | test passed (time: 872.173ms)
TEST-INFO | /home/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/components/downloads/test/unit/test_bug_406857.js | running test ...
TEST-PASS | /home/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/components/downloads/test/unit/test_bug_406857.js | test passed (time: 2237.524ms)
TEST-INFO | /home/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/components/downloads/test/unit/test_bug_409179.js | running test ...
TEST-PASS | /home/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/components/downloads/test/unit/test_bug_409179.js | test passed (time: 755.346ms)
TEST-INFO | /home/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/components/downloads/test/unit/test_bug_420230.js | running test ...

command timed out: 1200 seconds without output, attempting to kill
process killed by signal 9
program finished with exit code -1
elapsedTime=1834.551374
TinderboxPrint: xpcshell<br/><em class="testfail">T-FAIL</em>
buildbot.slave.commands.TimeoutError: command timed out: 1200 seconds without output, attempting to kill
TinderboxPrint: xpcshell<br/><em class="testfail">timeout</em>

Comment 16

[Karl Tomlinson (:karlt)]

No stack?

Comment 17

[Hubert Figuiere [:hub]]

That's all what tbpl gives.

It works fine here on my local machine :-(

Comment 18

[Karl Tomlinson (:karlt)]

Hmm, I guess xpcshell tests don't have the crashreporter.

Possibilities include
1) An issue with no GDK display opened for the xpcshell tests.
2) Something particular about the test system that results in the hang.

Does the xpcshell unit test run fine on your machine or do you just mean that the browser runs fine?

Comment 19

[Hubert Figuiere [:hub]]

the xpcshell unit test run fine here. But yeah I suspect it might be something like "gnome" not being available.

I don't know what the gtk_recent_manager relies on to run.

Comment 20

[Karl Tomlinson (:karlt)]

(In reply to Hub Figuiere [:hub] from comment #19)
> the xpcshell unit test run fine here. But yeah I suspect it might be
> something like "gnome" not being available.

You may be right, but it would be sad if that caused a hang.

The log said that the earlier test_bug_401430.js passed, but I don't understand why that should pass on any platform without "SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\Explorer\\RecentDocs".

Comment 21

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #20)
> The log said that the earlier test_bug_401430.js passed, but I don't
> understand why that should pass on any platform without
> "SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\Explorer\\RecentDocs".

Oh, now I see if (httpPH.platform != "Windows") return;

Comment 22

[Hubert Figuiere [:hub]]

Now it fails locally too. I hang on test_privatebrowsing.js
and when I control-C the test, I get this: 

(process:32719): GLib-GObject-CRITICAL **: gtype.c:2715: You forgot to call g_type_init()

(process:32719): GLib-CRITICAL **: g_once_init_leave: assertion `initialization_value != 0' failed

(process:32719): GLib-GObject-CRITICAL **: g_object_new: assertion `G_TYPE_IS_OBJECT (object_type)' failed


which I suspect is the actual problem as we might not initialize gtype in xpcshell mode.

I'll submit a new patch.

Comment 23

[Hubert Figuiere [:hub]]

Attachment: Downloading a file should put it in the system's recently-used list.

Comment 24

[Hubert Figuiere [:hub]]

Comment on attachment 624294 [details] [diff] [review]
Downloading a file should put it in the system's recently-used list.

Karl, this version actually fixed the xpcshell tests: I call g_type_init() when creating the download service to make sure we have a valid g_type system.

Comment 25

[Hubert Figuiere [:hub]]

The tbpl log with that patch

https://tbpl.mozilla.org/?tree=Try&rev=08b4732031e1

Comment 26

[Karl Tomlinson (:karlt)]

Comment on attachment 624294 [details] [diff] [review]
Downloading a file should put it in the system's recently-used list.

Thanks for tracking down the g_type_init().

Comment 9 isn't addressed.

gtk_recent_manager_get_default won't return NULL.

Comment 27

[Hubert Figuiere [:hub]]

mmm. I missed comment 9 completely, even in the previous r+ patch.

Will fix.

Comment 28

[Hubert Figuiere [:hub]]

Attachment: Downloading a file should put it in the system's recently-used list.

Comment 29

[Hubert Figuiere [:hub]]

Comment on attachment 624461 [details] [diff] [review]
Downloading a file should put it in the system's recently-used list.

addressed the comments.

tbpl seems to be green (still running)

Comment 30

[Hubert Figuiere [:hub]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc49f6d61297

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bc49f6d61297