Closed Bug 483054 Opened 13 years ago Closed 13 years ago

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

Categories

(Toolkit :: Downloads API, defect)

1.9.1 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: kairo, Assigned: mcsmurf)

References

Details

(Keywords: fixed1.9.1)

Attachments

(2 files, 3 obsolete files)

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.
Version: Trunk → 1.9.1 Branch
Attached patch Patch (obsolete) — Splinter Review
Sets import time as download time when reading start and end time failed.
Assignee: nobody → bugzilla
Status: NEW → ASSIGNED
Attachment #367120 - Flags: review?(sdwilsh)
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...
Yes, need a test for this case for it to be safe for 1.9.1 (doesn't even apply to mozilla-central)
Attached patch Patch (obsolete) — Splinter Review
I extended an already existing test for this one.
Attachment #367120 - Attachment is obsolete: true
Attachment #367209 - Flags: review?(sdwilsh)
Attachment #367120 - Flags: review?(sdwilsh)
Attached patch Patch (obsolete) — Splinter Review
Forgot to remove one line in the test file, tests of course still pass :).
Attachment #367209 - Attachment is obsolete: true
Attachment #367211 - Flags: review?(sdwilsh)
Attachment #367209 - Flags: review?(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.
Attachment #367211 - Flags: review?(sdwilsh) → review-
Attached patch PatchSplinter Review
Attachment #367211 - Attachment is obsolete: true
Attachment #367891 - Flags: review?(sdwilsh)
Comment on attachment 367891 [details] [diff] [review]
Patch

>+ * Contributor(s):
>+ *   Shawn Wilsher <me@shawnwilsher.com> (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
Attachment #367891 - Flags: review?(sdwilsh) → review+
Comment on attachment 367891 [details] [diff] [review]
Patch

Cannot land/test this on m-c as the code no longer exists on m-c.
Attachment #367891 - Flags: approval1.9.1?
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 on attachment 367891 [details] [diff] [review]
Patch

a191=beltzner
Attachment #367891 - Flags: approval1.9.1? → approval1.9.1+
Pushed to mozilla-1.9.1, changeset b300bd96cb97.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Keywords: fixed1.9.1
You need to log in before you can comment on or make changes to this bug.