591289 - Save chosen download file name and other metadata in Places history

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

Attachment: The patch

We should store metadata about downloads in the Places database, in order
to make them searchable in the Library window.

Once the metadata is in the database, it can also be used for a number of
other purposes, for example suggesting the last used filename when using
"Save As", or opening the target file directly from the Library window if
it is still there.

This patch uses Places annotations to save the metadata about the last time
the item was downloaded. In addition, if the download source has no title
(which is common for downloads that use "Content-Disposition: attachment"),
the title of the download is set to the initially chosen file name, to make
it visible in the new Downloads history folder (patch in bug 564900).

Comment 1

[:Paolo Amadini]

Attachment: The result of this patch combined with bug 564900

Comment 2

[Shawn Wilsher :sdwilsh]

The problem here is that the annotation API is sync, but the download history API is async, which means that when we try to annotate the page, it may not exist in history yet.  The other problem is that this introduces synchronous I/O on the main thread.

Comment 3

[Shawn Wilsher :sdwilsh]

Comment on attachment 469834 [details] [diff] [review]
The patch

The unfortunate thing here is that we don't have an async annotation API yet, but mak could probably spec it out pretty quickly, and it wouldn't be hard to implement (I don't think).

Also, r- due to lack of tests.

Comment 4

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 469837 [details]
The result of this patch combined with bug 564900

Looks good to me :)

Can you test whether adding a tag turns it into a bookmark? (that's what I assume will happen, and that's OK — just want to make sure it doesn't break :)

Comment 5

[:Paolo Amadini]

(In reply to comment #4)
> Comment on attachment 469837 [details]
> The result of this patch combined with bug 564900
> 
> Looks good to me :)
> 
> Can you test whether adding a tag turns it into a bookmark? (that's what I
> assume will happen, and that's OK — just want to make sure it doesn't break :)

Yes, that works, the download then appears in the unsorted bookmarks view.

Comment 6

[:Paolo Amadini]

(In reply to comment #2)
> The problem here is that the annotation API is sync, but the download
> history API is async, which means that when we try to annotate the page, it
> may not exist in history yet.

Actually, the specific download history APIs used here are also synchronous,
meaning that proof-of-concept patch happened to work correctly in all cases.

> The other problem is that this introduces
> synchronous I/O on the main thread.

This might not be deemed as a blocking issue because downloads themselves are
currently executed with synchronous I/O on the main thread, and this function
is executed only before a download starts, so there shouldn't be a significant
overhead.

In any case, before it can be fully implemented, this part of the Download
Manager probably needs a more comprehensive design effort, including
determining if the above issues are blocking or not and what we exactly
need in relation to bug 564900.

Comment 7

[Shawn Wilsher :sdwilsh]

(In reply to comment #6)
> This might not be deemed as a blocking issue because downloads themselves are
> currently executed with synchronous I/O on the main thread, and this function
> is executed only before a download starts, so there shouldn't be a significant
> overhead.
It is, though.  We shouldn't be adding more synchronous disk I/O.

Comment 8

[:Paolo Amadini]

Attachment: Same demo patch, updated to apply on trunk

This patch applies cleanly on trunk. It's just intended as a temporary support
for other patches in the Download Manager queue, because this patch needs to be
rewritten using asynchronously APIs.

Comment 9

[skierpage]

I think bug 402231 should depend on this?

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #9)
> I think bug 402231 should depend on this?

weak relationship, this is needed for interaction but not needed to browse downloads in the Library

Comment 11

[Paul [pwd]]

But this work does ultimately allow for downloads to be browsed in the library, thus shouldn't bug 402231 be either duped to this or WONTFIX'd?

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

no this bug allows for additional interaction on downloads in the Library, while that bug is only about showing them like any other history entry (just in a separate folder)

Comment 13

[:Paolo Amadini]

Attachment: Patch extending the work done in bug 564916

Since changing XPCOM interfaces is now cheaper, and the required interface
change already has sr+ by vlad (see bug 564916 comment 9), we can now implement
this feature much more cleanly, directly in the AddDownload function.

The idea is to land this in the Firefox 8 timeframe. Firstly, this complements
bug 564900, that already landed, avoiding empty descriptions to appear in the
columns of the Downloads folder. Secondly, we start collecting the data that
will be required to implement the additional Library window interactions
(bug 675902), specifically open on double click, making sure that by the
time Firefox 9 is released most users will already have collected the data
required to provide that functionality for their most recent downloads.

Comment 14

[:Paolo Amadini]

As per Marco's indication, this builds upon the existing synchronous AddVisit
and annotation APIs, until AddDownload becomes asynchronous itself (already
filed as bug 672681).

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

The merge from mozilla-central to mozilla-aurora will be happening on August 16. Marco has stated that he will be away from 8-14 August.

Paolo, maybe you should ask for a different reviewer if you want this to land for Fx8?

Comment 16

[:Paolo Amadini]

(In reply to Jared Wein [:jwein] from comment #15)
> Paolo, maybe you should ask for a different reviewer if you want this to
> land for Fx8?

Thanks; do you have someone to recommend for reviewing this change?

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 551317 [details] [diff] [review]
Patch extending the work done in bug 564916

sdwilsh: Can you please review this bug since Marco is on vacation? It would be great to land this before the mozilla-central -> aurora merge since Downloads in the library are new.

Comment 18

[Shawn Wilsher :sdwilsh]

Comment on attachment 551317 [details] [diff] [review]
Patch extending the work done in bug 564916

Review of attachment 551317 [details] [diff] [review]:
-----------------------------------------------------------------

r=sdwilsh

::: docshell/base/nsIDownloadHistory.idl
@@ +60,5 @@
>     *        [optional] The referrer of source URI.
>     * @param aStartTime
>     *        [optional] The time the download was started.  If the start time
>     *        is not given, the current time is used.
> +   * @param aTarget

I'd actually prefer to call this destination, despite the download manager calling it target.

::: toolkit/components/places/nsNavHistory.cpp
@@ +5256,5 @@
>  //// nsIDownloadHistory
>  
>  NS_IMETHODIMP
>  nsNavHistory::AddDownload(nsIURI* aSource, nsIURI* aReferrer,
> +                          PRTime aStartTime, nsIURI* aTarget)

Same here; go with destination.

@@ +5270,5 @@
> +  nsresult rv = AddVisit(aSource, aStartTime, aReferrer, TRANSITION_DOWNLOAD,
> +                         PR_FALSE, 0, &visitID);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (!aTarget)

global nit: brace all ifs (despite the fact that it would make this method inconsistent; trying to update code style is hard).

@@ +5297,5 @@
> +
> +  (void)annosvc->SetPageAnnotationString(
> +                        aSource, TARGETFILEURI_ANNO,
> +                        NS_ConvertUTF8toUTF16(targetURISpec), 0,
> +                        nsIAnnotationService::EXPIRE_WITH_HISTORY);

nit:
(void)annosvc->SetPageAnnotationString(
  aSource,
  TARGETFILEURI_ANNO,
  NS_ConvertUTF8toUTF16(targetURISpec),
  0,
  nsIAnnotationService::EXPIRE_WITH_HISTORY
);

@@ +5301,5 @@
> +                        nsIAnnotationService::EXPIRE_WITH_HISTORY);
> +  (void)annosvc->SetPageAnnotationString(
> +                        aSource, TARGETFILENAME_ANNO,
> +                        targetFileName, 0,
> +                        nsIAnnotationService::EXPIRE_WITH_HISTORY);

ditto

::: toolkit/components/places/tests/unit/test_download_history_details.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */

You should actually just add this test to https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/unit/test_download_history.js and change the license of that test to be PD as well please.

@@ +79,5 @@
> +           .getService(Ci.nsIDownloadHistory);
> +
> +  // Both null values and remote URIs should not cause errors.
> +  dh.addDownload(SOURCE_URI, null, Date.now() * 1000, null);
> +  dh.addDownload(SOURCE_URI, null, Date.now() * 1000, uri("http://localhost/"));

Also test not passing anything as the fourth argument please.

Comment 19

[Shawn Wilsher :sdwilsh]

Comment on attachment 551317 [details] [diff] [review]
Patch extending the work done in bug 564916

oh bother...this needs sr too.

Comment 20

[:Paolo Amadini]

Attachment: Updated patch

Thank you a lot for reviewing this! It was really helpful :-)

I agree that aDestination is clearer than aTarget, as future terminology.
I've updated the annotation names and internal variables as well.

I've unified the two test files, though the new part cannot be run as a
function inside the tests array because it waits for callbacks that are
potentially asynchronous (this is the reason why I thought it was clearer
to create a separate file at first). Please take a look to see if the new
unified file matches what you have in mind :-)

(In reply to Shawn Wilsher :sdwilsh from comment #19)
> oh bother...this needs sr too.

We already have sr+ for the interface changes in bug 564916 comment 9 by vlad,
except that now the parameter name has been changed from aTarget to
aDestination. Not sure whether sr is needed again here.

I'm now running a tryserver build with the updated patch.

Comment 21

[:Paolo Amadini]

The tryserver build has only sparse failures that seem unrelated to this patch:

http://tbpl.mozilla.org/?tree=Try&rev=3ebf1ef8be52

Shawn, I think this is ready to land unless you think there are more nits to
address, or if we need sr again, which should be just a formality in any case.
Can you see that this patch is pushed to mozilla-central in time?

Comment 22

[Shawn Wilsher :sdwilsh]

(In reply to Paolo Amadini from comment #20)
> I've unified the two test files, though the new part cannot be run as a
> function inside the tests array because it waits for callbacks that are
> potentially asynchronous (this is the reason why I thought it was clearer
> to create a separate file at first). Please take a look to see if the new
> unified file matches what you have in mind :-)
You really want to use `add_test` and then call `run_next_test` when each test function is done: http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/head.js#834

You don't need my review for that though, so it'll be good to land with that change.  Thanks!

Comment 23

[:Paolo Amadini]

(In reply to Shawn Wilsher :sdwilsh from comment #22)
> You don't need my review for that though, so it'll be good to land with that
> change.  Thanks!

Do you think we can land the patch as is, and then fix the test in a quick
follow-up? The latest patch has succeeded on tryserver and it seems safer to
land exactly that, and I'm worried about the cyclopic times of a new tryserver
build :-)

Comment 24

[:Paolo Amadini]

Notwithstanding the previous comment:

(In reply to Shawn Wilsher :sdwilsh from comment #22)
> You really want to use `add_test` and then call `run_next_test` when each
> test function is done:

Do you refer to the individual test functions, or the two macro-functions?
In the former case, there is initialization and termination code that should
be executed before and after each individual function in the first set, do
you think I should just copy the code to each function? See:

http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/places/tests/unit/test_download_history.js#148

Comment 25

[Shawn Wilsher :sdwilsh]

(In reply to Paolo Amadini from comment #24)
> Do you refer to the individual test functions, or the two macro-functions?
Individual test functions.  You can move the calls to `uri_in_db` into each method, and you should probably make a new `cleanup` method that does the cleanup work that those tests call as well.

(In reply to Paolo Amadini from comment #23)
> Do you think we can land the patch as is, and then fix the test in a quick
> follow-up? The latest patch has succeeded on tryserver and it seems safer to
> land exactly that, and I'm worried about the cyclopic times of a new
> tryserver
> build :-)
I would prefer for it to land with this fix, and you should be able to land with confidence if the tests pass locally.  If you really want a try server run beforehand though, we can land as-is and get the follow-up in tomorrow after a try server run.

Comment 26

[:Paolo Amadini]

I'll go for the safer route and update the tests as a follow-up :-)

Comment 27

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/213b48aa2c56

Comment 28

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/213b48aa2c56

Comment 29

[:Paolo Amadini]

Thanks to everyone that contributed to make this change land in time, and in
particular to Shawn for reviewing it during his free time :-) I'm following up
in bug 679314 with the suggested test refactoring.

Comment 30

[:Paolo Amadini]

I'm adding the new "addon-compat" bug keyword since this bug changes the
nsIDownloadHistory interface.

Comment 31

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en/Firefox_8_for_developers#Interface_changes
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDownloadHistory#addDownload%28%29