Closed Bug 670275 Opened 9 years ago Closed 8 years ago

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

Categories

(Toolkit :: Downloads API, defect)

All
Other
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: federico, Assigned: hub)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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.
We do this on Windows using the SHAddToRecentDocs API. Is there a suitable equivalent for Linux?
Status: UNCONFIRMED → NEW
Component: General → Download Manager
Ever confirmed: true
Product: Firefox → Toolkit
QA Contact: general → download.manager
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.
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 :)
Karl can probably give some feedback.
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.
Attachment #544933 - Flags: feedback+
Version: 5 Branch → Trunk
This is the reworked patch according to the feedback. Tested on Fedora. Ready for review.
Assignee: nobody → 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.
Attachment #604290 - Flags: review?(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.
Attachment #604290 - Flags: review?(karlt) → review+
Landed Inbound
https://hg.mozilla.org/integration/mozilla-inbound/
Target Milestone: --- → mozilla13
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
Target Milestone: mozilla13 → ---
I'll submit a new patch when I have solution.
Do you know what the problem is?
I don't seem to have the logs on tbpl.
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>
Target Milestone: --- → mozilla13
That's all what tbpl gives.

It works fine here on my local machine :-(
Target Milestone: mozilla13 → ---
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?
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.
(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".
(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;
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.
Attachment #604290 - Attachment is obsolete: true
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.
Attachment #624294 - Flags: review?(karlt)
The tbpl log with that patch

https://tbpl.mozilla.org/?tree=Try&rev=08b4732031e1
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.
Attachment #624294 - Flags: review?(karlt) → review-
mmm. I missed comment 9 completely, even in the previous r+ patch.

Will fix.
Attachment #624294 - Attachment is obsolete: true
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)
Attachment #624461 - Flags: review?(karlt)
Attachment #624461 - Flags: review?(karlt) → review+
https://hg.mozilla.org/mozilla-central/rev/bc49f6d61297
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Blocks: 756837
You need to log in before you can comment on or make changes to this bug.