483054 - downloads.rdf import requires DateEnded but xpfe files don't have it

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Robert Kaiser]

We currently fail to import any download data from xpfe's downloads.rdf files as we require both DateStarted and DateEnded to be written to the database, but xpfe didn't save any of those, and the only fallback we have is to use DateEnded for both if DateStarted isn't found.

The code is http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp#735 and below.

What we probably should do is using the current date/time at import for both when we don't find any of them.

Comment 1

[Frank Wein [:mcsmurf]]

Attachment: Patch

Sets import time as download time when reading start and end time failed.

Comment 2

[Robert Kaiser]

Thanks Frank, from what I see here, we correctly import an xpfe downloads.rdf file into a SeaMonkey build using the toolkit downloadmanager (my build has both a variant of the bug 381157 patch and the bug 472001 patch applied).

I assume we should put up a patch to enhance the tests to catch that case as well, though...

Comment 3

[Shawn Wilsher :sdwilsh]

Yes, need a test for this case for it to be safe for 1.9.1 (doesn't even apply to mozilla-central)

Comment 4

[Frank Wein [:mcsmurf]]

Attachment: Patch

I extended an already existing test for this one.

Comment 5

[Frank Wein [:mcsmurf]]

Attachment: Patch

Forgot to remove one line in the test file, tests of course still pass :).

Comment 6

[Shawn Wilsher :sdwilsh]

Comment on attachment 367211 [details] [diff] [review]
Patch

I'm fine with the code changes, but I'd like you to make a new unit test file for this instead of piggy-backing on an old one please.

Comment 7

[Frank Wein [:mcsmurf]]

Attachment: Patch

Comment 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 367891 [details] [diff] [review]
Patch

>+ * Contributor(s):
>+ *   Shawn Wilsher <[e-mail]> (Original Author)
add yourself here please

Also, consider doing an hg copy here and then making your changes?

>+function test_count_entries()
>+{
>+  var stmt = dm.DBConnection.createStatement("SELECT COUNT(*) " +
>+                                             "FROM moz_downloads");
>+  stmt.executeStep();
should do_check_true this

>+  stmt.reset();
change to stmt.finalize() please


>+  stmt.bindStringParameter(0, "Google.htm");
>+  stmt.executeStep();
do_check_true again

>+  // Actual check of dates being the same and non-empty
>+  do_check_eq(stmt.getInt64(4), stmt.getInt64(5));
>+  do_check_false(!stmt.getInt64(4));
>+
>+  stmt.reset();
again with finalize

r=sdwilsh with these changes

Comment 9

[Frank Wein [:mcsmurf]]

Comment on attachment 367891 [details] [diff] [review]
Patch

Cannot land/test this on m-c as the code no longer exists on m-c.

Comment 10

[Frank Wein [:mcsmurf]]

Risk: This changes the import code, so the code runs when using Firefox 3 for the first time (when the user used FF 2 before) or importing a SeaMonkey profile. The patch fixes a case which should only happen when importing a SeaMonkey profile.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 367891 [details] [diff] [review]
Patch

a191=beltzner

Comment 12

[Frank Wein [:mcsmurf]]

Attachment: Patch for check-in

Comment 13

[Frank Wein [:mcsmurf]]

Pushed to mozilla-1.9.1, changeset b300bd96cb97.