851466 - Import "downloads.sqlite" to "downloads.json"

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

Implement an import path for the new JavaScript API for downloads.

After the initial import is finished, there is no need to export downloads to
the old format, because download items are per session and the long-term
download history is stored in the Places database. There is no strong need
to preserve paused downloads on downgrade, thus "downloads.sqlite" should
also be deleted.

If both the "downloads.json" and "downloads.sqlite" files exist, whether
downloads are imported and merged, or the old file is ignored, is to be
defined.

Comment 2

[:Paolo Amadini]

As noted in bug 836487, we should also delete the obsolete download storage files
from older profiles.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Attachment: wip v1

First WIP patch of the DB importer module. Main code should work but it might need more error handling. Also figure out what to do with currBytes,maxBytes,startTime,endTime and state, as AFAICT it's not available on the new API yet.

There's also a rough idea of where we could trigger the migration as we discussed, here using the nsBrowserGlue's UI migration. But after seeing it implemented I'm not sure it's the best way because:

 - there's no strong guarantee that the DownloadStore load will always be called only after the UI migration (but that's probably true);

 - but more problematic, on the opposite side of the spectrum, no guarantee that after UI migration runs, the same session will initialize the Downloads service and thus trigger the migration. So we might miss the hint to migrate

Comment 4

[:Paolo Amadini]

(In reply to :Felipe Gomes from comment #3)
> Also figure out what to do with
> currBytes,maxBytes,startTime

These should probably be added to the fields that are serialized and
deserialized (with their new names), also to keep them across browser restarts.

> endTime

This can be ignored.

> state

We should import only downloads that are paused (at least, this works for
Firefox for Desktop). If I remember correctly, there is a field that tells
us if we should also restart a paused download immediately, because it was
suspended during shutdown.

>  - but more problematic, on the opposite side of the spectrum, no guarantee
> that after UI migration runs, the same session will initialize the Downloads
> service and thus trigger the migration. So we might miss the hint to migrate

True, so I think we need to set a preference in the migration code (like
"browser.download.needImport" or something similar), then check if it is set
in getPublicDownloadList, and trigger the import if so.

We should clear the preference when we remove the "downloads.sqlite" file. We
may choose to remove the file after we read its data, or to be safer we may do
this after we've written the JSON file to disk, if the "needImport" preference
is still set, before clearing the preference.

Comment 5

[:Paolo Amadini]

Comment on attachment 786096 [details] [diff] [review]
wip v1

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

::: toolkit/components/jsdownloads/src/DownloadStore.jsm
@@ +101,5 @@
>    {
> +    if (DownloadIntegration.pendingImport) {
> +      DownloadIntegration.pendingImport = false;
> +      return DownloadStore_DBMigrator.import(this.list);
> +    }

It's better to implement this in getPublicDownloadList rather than DownloadStore.

::: toolkit/components/jsdownloads/src/DownloadStore_DBMigrator.jsm
@@ +6,5 @@
> +
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = [
> +  "DownloadStore_DBMigration",

And I'd just call this module "DownloadImport", as they do different things.

DownloadImport may call into DownloadStore to request saving soon, we still have to figure out that part.

@@ +51,5 @@
> +      } catch (ex) {
> +        // XXX i don't know what kinds of error openConnection can throw,
> +        // but I imagine we don't care about any..
> +        // XXX possibly we'll want to remove the file here even if we don't
> +        // import anything from it? or leave it there for debugging

I think we should consider any error fatal, in that we should not retry to import the data.

In case of any error (not only connection open), we should either delete the file or move it for debugging ("download.sqlite.old"). Not sure which of the two options is better.

@@ +56,5 @@
> +        throw new Task.Result(0);
> +      }
> +
> +      let schemaVersion = yield connection.getSchemaVersion();
> +      // xxx do we care about older schemas?

The latest schema update happened in bug 780533 for Firefox 19. The next ESR versions are 24 and 31, and only the latter will have the new Downloads API, this means that in all the usual cases we'll probably be migrating from version 9.

If we can handle older schemas (ignoring optional columns that don't exist), it's better, but it's not a requirement.

@@ +80,5 @@
> +
> +        // XXX is any more data validation necessary?
> +        if (!source || !target) {
> +          continue;
> +        }

I don't think other validation is necessary.

We should catch and report errors at each iteration of the loop, so that if createDownload fails we skip to the next row.

@@ +101,5 @@
> +
> +        count++;
> +      }
> +
> +      yield connection.close();

You should have a single connection.close in a "finally" block so that you don't need to do this explicitly when returning.

@@ +105,5 @@
> +      yield connection.close();
> +
> +      try {
> +        yield OS.File.remove(dbFilePath);
> +      } catch (ex) { }

As mentioned, we may remove or move the file as appropriate, either here or after the JSON file is saved.

If we can't remove the file, this should count as a fatal error but in the end we should reset the "needImport" preference in any case, to avoid importing it again. Manual (or automatic) testing with a read-only "downloads.sqlite" file may be useful.

@@ +112,5 @@
> +    });
> +  }
> +}
> +
> +function fileURLtoPath(fileURL) {

Technically, the argument can be both a file URL or a file path, depending on the schema version. If we only support the latest schema, we probably don't care about the second case, but this means that we should fail with an error if we encounter a path instead of an URL.

In either case, we'll need a comment to explain why we do this for the target field.

nit: in general here we implement helpers as "_" prefixed functions in the object. This can also be inlined.

@@ +117,5 @@
> +  try {
> +    let targetURI = NetUtil.newURI(fileURL);
> +    if (targetURI.scheme == "file") {
> +      return target.path;
> +    }

This should do QueryInterface(Ci.nsIFileURL).path to ensure that the "path" field is available. Also, if this is a URL, it is always a "file:" URL, so no need to check the scheme.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Attachment: v2

Ok, so I went back and forth on the design here and I arrived at something slightly different from what was suggested, for a couple of reasons.

We can't really use the nsBrowserGlue.migrateUI code because it's Firefox only, and therefore other apps using the download service wouldn't get the DB migration unless they added explicit support for it, which is not ideal.

The way I solved it (similar to what was done in AddonRepository) is by using one pref only to store the current schema of the json DB. Turns out we should be doing this anyways to allow for future updates to the DB, so this will also cover that. And it gets rid of needing to use a specific pref to mark a "import is needed" flag (the absence of the schema pref indicates that now)


Also, I started implementing it in Downloads.getPublicDownloadList, but that code doesn't have enough access to everything needed so it got messy. Instead I implemented in loadPersistent, because that allowed to enforce the steps:

          import -> force save -> delete old DB

very easily. (and it could equally be implemented in DownloadStore)


I hope it differs from the previous discussions for valid reasons, but let me know what you think.

This patch is already based on top of bug 836443.

Comment 7

[:Paolo Amadini]

(In reply to :Felipe Gomes from comment #6)
> We can't really use the nsBrowserGlue.migrateUI code because it's Firefox
> only, and therefore other apps using the download service wouldn't get the
> DB migration unless they added explicit support for it, which is not ideal.

This makes a lot of sense!

> The way I solved it (similar to what was done in AddonRepository) is by
> using one pref only to store the current schema of the json DB. Turns out we
> should be doing this anyways to allow for future updates to the DB

I don't think we need a schema version - we can freely add and remove fields
in new application versions, or even individual download items, keeping or
breaking both forward and backwards compatibility as appropriate. A boolean
preference like importDone or jsonMigrationCompleted seems better to me.

I'm not sure how to avoid the existence check for "downloads.sqlite" in new
profiles. But since we do this only once, and after the application started,
I think it may be not worth adding more complexity for that.

> Instead I implemented in loadPersistent, because that allowed to enforce the
> steps:
>           import -> force save -> delete old DB

Looks good. I wonder if we should rename loadPersistent to
initializePublicDownloadList?

Comment 8

[:Paolo Amadini]

Comment on attachment 789365 [details] [diff] [review]
v2

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

I'm doing a preliminary review of the patch, though I know this is a work in progress and you may have already noticed many things.

::: browser/components/nsBrowserGlue.js
@@ +73,5 @@
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "Downloads",
> +                                  "resource://gre/modules/Downloads.jsm");
> +
> +

leftover

::: toolkit/components/jsdownloads/src/DownloadImport.jsm
@@ +26,5 @@
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "Downloads",
> +                                  "resource://gre/modules/Downloads.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "DownloadIntegration",
> +                                  "resource://gre/modules/DownloadIntegration.jsm");

DownloadIntegration is not used here anymore, check other modules as well.

@@ +37,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
> +                                  "resource://gre/modules/NetUtil.jsm");
> +
> +
> +this.DownloadImport = {

I think we should make DownloadImport creatable like DownloadStore, with the aList and aPath parameters, and "list" and "path" members (simple names).

@@ +44,5 @@
> +                        DB_FILENAME);
> +  },
> +  /**
> +   * xxx todo DOC
> +   * @resolves to the number of items imported

Since we're not using this value, no point in returning it, may just resolve to undefined.

Specify in the comment that failure means that the database, or at least one download item, is corrupt. The caller should take an appropriate action (like renaming the file for debugging).

@@ +47,5 @@
> +   * xxx todo DOC
> +   * @resolves to the number of items imported
> +   */
> +  import: function (aList) {
> +    return Task.spawn(function import_downloads() {

We should use anonymous functions, they are properly reported in stack traces now.

@@ +57,5 @@
> +      let connection = null, importedItemsCount = 0;
> +
> +      try {
> +
> +        connection = yield Sqlite.openConnection({ path: this.dbFilePath });

Put "try" after the assignment and don't null-check in "finally".

@@ +66,5 @@
> +        // - Version 8 added the columns X,Y in 2007
> +        // - Version 9 added a unique GUID text column that is not
> +        // used here
> +        if (schemaVersion < 8 || schemaVersion > 9) {
> +          throw new Error("Unsupported schema version");

We're probably be able to import from a schemaVersion greater than 9, if it only adds new columns. It is unlikely that it will ever happen, anyways.

nit: Error("Unable to import in-progress downloads because the existing profile is too old.")

I wonder if this case should count as success, so that we just delete the old database from the profile. In this case, just Cu.reportError and return.

@@ +75,5 @@
> +        for (let row of rows) {
> +
> +          // Get the DB row data
> +          let source = row.getResultByName("source");
> +          let target = _fileURLtoPath(row.getResultByName("target"));

Just inline this as:

let sourceUrl = row.getResultByName("source");
let targetPath = NetUtil.newURI(row.getResultByName("target"))
                        .QueryInterface(Ci.nsIFileURL).path;

@@ +81,5 @@
> +          let startTime = row.getResultByName("startTime");
> +          let state = row.getResultByName("state");
> +          let referrer = row.getResultByName("referrer");
> +          let currBytes = row.getResultByName("currBytes");
> +          let maxBytes = row.getResultByName("maxBytes");          

nit: whitespace at end of line

@@ +88,5 @@
> +          let preferredAction = row.getResultByName("preferredAction");
> +          let autoResume = row.getResultByName("autoResume");
> +
> +          // We'll only import paused downloads
> +          if (state != DOWNLOAD_PAUSED || !source || !target) {

No need to check for empty values here anymore.

@@ +96,5 @@
> +          // Transform the data
> +          let launchWhenSucceeded = (preferredAction != Ci.nsIMIMEInfo.saveToDisk);
> +
> +          // xxx name not used? currBytes not serialized?
> +          // xxx guid unused

Yes. We should detect currentBytes just like DownloadStore does (and ignore failures, we should keep the download even if we can't access the ".part" file).

@@ +100,5 @@
> +          // xxx guid unused
> +
> +          // xxx is this the right way to check for hasPartialData? Or should we
> +          // check if target/tempPath exists?
> +          let hasPartialData = currBytes > 0;

We can just assume that paused downloads have partial data, as they can't be be paused otherwise.

@@ +127,5 @@
> +        }
> +
> +        throw "success";
> +      } catch (ex) {
> +        throw new Task.Result(importedItemsCount);

For error handling, "catch" and report errors inside the "for" loop.

If any of the downloads failed, throw an error at the end.

@@ +137,5 @@
> +    });
> +  },
> +
> +  eraseDB: function() {
> +    return OS.File.remove(this.dbFilePath).then(null, Cu.reportError);

nit: no need for a helper function, the caller can easily remove the file.

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
@@ +138,3 @@
>     */
>    loadPersistent: function DI_loadPersistent(aList)
>    {

At this point, this should be rewritten as a Task for clarity.

@@ +152,5 @@
>      this._store.onsaveitem = this.shouldPersistDownload.bind(this);
>  
> +    let loadPromise = null;
> +
> +    if (!this._store.needImport) {

We may just read and write the preference here, or put a helper function nearby, no need to make the DownloadStore module aware of import.

@@ +164,5 @@
> +
> +      loadPromise = DownloadImport.import(aList).then(
> +        finishImport.bind(this),
> +        finishImport.bind(this)
> +      );

If import fails, we should rename the file to "download.sqlite.corrupt". If it succeeds, we should delete the file.

Comment 9

[:Paolo Amadini]

Very important for restarting the download from where it stopped, we should
restore the entityID column:

saver: {
  type: "copy",
  entityID: entityID,
},

There is also this mapping of download database states done in
nsDownloadManager.cpp, in order:

- SCANNING => FINISHED
- FINISHED => Reset autoResume
- NOTSTARTED, QUEUED, DOWNLOADING => Set autoResume

So, we should import PAUSED, NOTSTARTED, QUEUED, and DOWNLOADING rows, and
automatically start those that are NOTSTARTED, QUEUED, or DOWNLOADING, or
that are PAUSED with the autoResume column set.

For the record, there's a piece of code in nsDownloadManager.cpp that says
"Clean up any old downloads.rdf files from before Firefox 3", but I don't
think we need that anymore, we can just ignore the file if present.

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Attachment: DownloadImport

Ok, with this patch I think I've covered all the feedback on the bug and our talks. It doesn't contain tests yet but I'm working on it, but aside from that (and two questions I left in the patch), I've manually tested the code extensively and it should be ready for a first review

Comment 11

[:Paolo Amadini]

Comment on attachment 791144 [details] [diff] [review]
DownloadImport

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

It looks like this should be ready with the changes outlined below, rebased on top of the new patch in bug 836443.

Can you file a separate bug for adding the xpcshell tests?

::: toolkit/components/jsdownloads/src/DownloadImport.jsm
@@ +17,5 @@
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;
> +const Cr = Components.results;
> +
> +const DB_FILENAME = "downloads.sqlite";

leftover

@@ +28,5 @@
> + */
> +const DOWNLOAD_NOTSTARTED = -1;
> +const DOWNLOAD_DOWNLOADING = 0;
> +const DOWNLOAD_PAUSED = 4;
> +const DOWNLOAD_QUEUED = 5;

nit: constants after module imports and module getters

@@ +45,5 @@
> +                                  "resource://gre/modules/NetUtil.jsm");
> +
> +/**
> + * Provides an object that has a method to import downloads
> + * from the previous SQLite storage format.

nit: "//////" separator before the object, like in other files

@@ +67,5 @@
> +   * @resolves When the operation has completed (i.e., every download
> +   *           from the previous database has been read and added to
> +   *           the DownloadList)
> +   * @rejects  With a NS_ERROR_FILE_NOT_FOUND exception if the
> +   *           specified database file doesn't exist.

If the file does not exist, it is an expected case (for example on a new profile), and should just import nothing.

@@ +73,5 @@
> +  import: function () {
> +    return Task.spawn(function task_DI_import() {
> +      let exists = yield OS.File.exists(this.path);
> +      if (!exists) {
> +        throw new Components.Exception(Cr.NS_ERROR_FILE_NOT_FOUND);

nit: For future reference, the first argument is the message, the result is the second. While here, can you fix the following unrelated instance by adding an empty string as the first parameter?

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadCore.jsm#952

@@ +82,5 @@
> +      try {
> +        let schemaVersion = yield connection.getSchemaVersion();
> +        // We don't support schemas older than version 8 (from 2007)
> +        // or greater than 9 as that's the last known version
> +        // - Version 8 added the columns X,Y in 2007

X,Y to be named?

@@ +86,5 @@
> +        // - Version 8 added the columns X,Y in 2007
> +        // - Version 9 added a unique GUID text column that is not
> +        // used here
> +        if (schemaVersion < 8) {
> +          throw new Error("Unsupported schema version");

nit: Fix the error message as suggested.

@@ +102,5 @@
> +            let state = row.getResultByName("state");
> +            let referrer = row.getResultByName("referrer");
> +            let maxBytes = row.getResultByName("maxBytes");
> +            let contentType = row.getResultByName("mimeType");
> +            let launcherPath = row.getResultByName("preferredApplication");

nit: sometimes you use the old column name, sometimes the equivalent new name. I think we might just use the old column names for clarity.

@@ +161,5 @@
> +            let download = yield Downloads.createDownload(downloadOptions);
> +
> +            try {
> +              // XXX question: when the download is occuring in place (no temp file),
> +              // is partFilePath also filled or should we check target if partFilePath is empty?

In the latest patch on bug 836443 I added a new method, since you needed to do the same thing in this bug.

Just remove the try-catch block here, then call "yield download.refresh();" in the else block of "if (resumeDownload)" (no need to catch additional errors there, the refresh method never fails).

@@ +181,5 @@
> +
> +          } catch (ex) {
> +            Cu.reportError("Error importing download: " + ex);
> +          } // try around for-loop
> +        } // for-loop

nit: no need for these closing comments.

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
@@ +92,5 @@
>  const kSaveDelayMs = 1500;
>  
> +const DOWNLOAD_SQLITE_IMPORTED = "browser.download.importedFromSqlite";
> +const DOWNLOAD_SQLITE_FILENAME = "downloads.sqlite";
> +const DOWNLOAD_RDF_FILENAME = "downloads.rdf";

No need for the filename constants, may just specify the name in place like we do for "downloads.json".

The "browser.download.importedFromSqlite" constant should be named kPrefImportedFromSqlite.

@@ +141,5 @@
>     * @return {Promise}
>     * @resolves When the list has been populated.
>     * @rejects JavaScript exception.
>     */
> +  loadPublicDownloadList: function(aList) {

I'd name this initializePublicDownloadList or preparePublicDownloadList, because not only we load, but we also create the DownloadAutoSaveView.

@@ +179,5 @@
> +          }
> +
> +          // No need to wait for the file removal.
> +          OS.File.remove(sqliteDBpath).then(null, Cu.reportError);
> +        } finally {

I think there's a missing "catch" here (on new profiles we would not add the DownloadAutoSaveView).

@@ +601,5 @@
>      }
>      return Promise.resolve();
> +  },
> +
> +  get needSqliteImport() {

Use a private getter with the same value of the preference: "_importedFromSqlite"

@@ +604,5 @@
> +
> +  get needSqliteImport() {
> +    if (this.testMode) {
> +      return false;
> +    }

No need for checking test mode here.

@@ +616,5 @@
> +  },
> +
> +  set needSqliteImport(val) {
> +    if (val) {
> +      Services.prefs.clearUserPref(DOWNLOAD_SQLITE_IMPORTED);

We never enter this code path, we may just have a "_setImportedFromSqlite()" function.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

> @@ +67,5 @@
> > +   * @resolves When the operation has completed (i.e., every download
> > +   *           from the previous database has been read and added to
> > +   *           the DownloadList)
> > +   * @rejects  With a NS_ERROR_FILE_NOT_FOUND exception if the
> > +   *           specified database file doesn't exist.
> 
> If the file does not exist, it is an expected case (for example on a new
> profile), and should just import nothing.

The reason I added this is because it makes it easy on DownloadIntegration to skip the file saving and deleting if the DB file doesn't exist.  But yeah it doesn't make much sense to throw an error here. So what you say I check for the file existence in DownloadIntegration instead of the import() function? And then import() will always succeed.


> Just remove the try-catch block here, then call "yield download.refresh();"
> in the else block of "if (resumeDownload)" (no need to catch additional
> errors there, the refresh method never fails).

Is it not necessary to call download.refresh() for the no-autoResume case? won't them show up in the panel as paused without the progress information?

> 
> The "browser.download.importedFromSqlite" constant should be named
> kPrefImportedFromSqlite.

Ok. I forgot to mention one thing: the name of this pref starting with "browser." is technically not ideal because it's not browser/ only, but I'm following the convention as all other prefs by the download service seem to be named like this.

> @@ +604,5 @@
> > +
> > +  get needSqliteImport() {
> > +    if (this.testMode) {
> > +      return false;
> > +    }
> 
> No need for checking test mode here.

I thought of doing this because otherwise most of the tests that end up calling getPublicDownloadList will be exercisizing the migration path instead of the more common load path. That is, if there wasn't the "this.dontLoad" check. Is that why you say it's not necessary?  Are there any tests that don't set the "dontLoad" pref?


> 
> @@ +616,5 @@
> > +  },
> > +
> > +  set needSqliteImport(val) {
> > +    if (val) {
> > +      Services.prefs.clearUserPref(DOWNLOAD_SQLITE_IMPORTED);
> 
> We never enter this code path, we may just have a "_setImportedFromSqlite()"
> function.

I planned on using this for tests, and having a getter/setter seems simpler than a getter and a different function

Comment 13

[:Paolo Amadini]

(In reply to :Felipe Gomes from comment #12)
> The reason I added this is because it makes it easy on DownloadIntegration
> to skip the file saving and deleting if the DB file doesn't exist.  But yeah
> it doesn't make much sense to throw an error here. So what you say I check
> for the file existence in DownloadIntegration instead of the import()
> function? And then import() will always succeed.

Sounds good.
 
> > Just remove the try-catch block here, then call "yield download.refresh();"
> > in the else block of "if (resumeDownload)" (no need to catch additional
> > errors there, the refresh method never fails).
> 
> Is it not necessary to call download.refresh() for the no-autoResume case?
> won't them show up in the panel as paused without the progress information?

Yes, "refresh" is required only if resumeDownload is false.

> Ok. I forgot to mention one thing: the name of this pref starting with
> "browser." is technically not ideal because it's not browser/ only, but I'm
> following the convention as all other prefs by the download service seem to
> be named like this.

Exactly, we're keeping the namespace for historical reasons.

> > @@ +604,5 @@
> > > +
> > > +  get needSqliteImport() {
> > > +    if (this.testMode) {
> > > +      return false;
> > > +    }
> > 
> > No need for checking test mode here.
> 
> I thought of doing this because otherwise most of the tests that end up
> calling getPublicDownloadList will be exercisizing the migration path
> instead of the more common load path. That is, if there wasn't the
> "this.dontLoad" check. Is that why you say it's not necessary?  Are there
> any tests that don't set the "dontLoad" pref?

Yes, dontLoad is set during all the tests.

> > @@ +616,5 @@
> > > +  },
> > > +
> > > +  set needSqliteImport(val) {
> > > +    if (val) {
> > > +      Services.prefs.clearUserPref(DOWNLOAD_SQLITE_IMPORTED);
> > 
> > We never enter this code path, we may just have a "_setImportedFromSqlite()"
> > function.
> 
> I planned on using this for tests, and having a getter/setter seems simpler
> than a getter and a different function

xpcshell tests don't enter the migration function (because of dontLoad), so I
think the preference would not affect them. In any case any simulation of the
preference value could be implemented as an override in the getter.

Comment 14

[:Felipe Gomes (needinfo for replies!)]

Attachment: DownloadImport v2

Done the changes:
 - import() never throws (and file existence check is in DownloadIntegration)
 - added yield download.refresh()
 - added support for schema version 7 (which didn't have autoResume column)
 - vars match column names
 - As _setImportedFromSqlite would be used only once I simply inlined the setBoolPref
 - fixed other nits

Comment 15

[:Paolo Amadini]

Comment on attachment 791321 [details] [diff] [review]
DownloadImport v2

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

Thanks a lot for the excellent work!

::: toolkit/components/jsdownloads/src/DownloadImport.jsm
@@ +60,5 @@
> +
> +this.DownloadImport.prototype = {
> +  /**
> +   * Imports unfinished downloads from the previous SQLite storage
> +   * format (supporting schemas 8 and up), to the new Download object

nit: 7 and up

@@ +111,5 @@
> +              // autoResume wasn't present in schema version 7
> +            }
> +
> +            if (!source) {
> +              continue;

nit: wouldn't it make more sense to throw here?

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
@@ +169,5 @@
> +        let sqliteDBpath = OS.Path.join(OS.Constants.Path.profileDir,
> +                                        "downloads.sqlite");
> +        let exists = yield OS.File.exists(this.path);
> +
> +        if (exists) {

exists(sqliteDBpath)?

nit: we usually inline: "if (yield OS.File.exists(sqliteDBpath))"

Comment 16

[:Felipe Gomes (needinfo for replies!)]

Attachment: DownloadImport - version to land

(nits fixed to land)

Comment 17

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/integration/fx-team/rev/92b20164931f

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/92b20164931f

Comment 19

[Mihai Morar, (:MihaiMorar)]

Is it enought to test that the current functionality hasn't regressed?

Comment 20

[:Paolo Amadini]

(In reply to Mihai Morar, QA (:MihaiMorar) [needinfo? me for any requests] from comment #19)
> Is it enought to test that the current functionality hasn't regressed?

The type of test to do here involves having some paused and in-progress downloads in a Firefox 25 profile, and see that they are still present after migration to Firefox 26.

Comment 21

[Mihai Morar, (:MihaiMorar)]

I made a couple of investigations using Windows 7 x64 and upgraded Aurora 25 (2013/09/05) to latest Aurora 26. I can confirm that all the active downloads are still present after the migration but all of them started from 0% and they were saved as "file"(2) 

Is this expected?

Comment 22

[:Paolo Amadini]

(In reply to Mihai Morar, QA (:MihaiMorar) [needinfo? me for any requests] from comment #21)
> I made a couple of investigations using Windows 7 x64 and upgraded Aurora 25
> (2013/09/05) to latest Aurora 26. I can confirm that all the active
> downloads are still present after the migration but all of them started from
> 0% and they were saved as "file"(2) 
> 
> Is this expected?

Uh, definitely not. The downloads should continue just like after a normal browser restart. Did you test on a profile that was never upgraded to Aurora 26 before?

I wonder if there may be a platform difference? In any case, if this is confirmed, a separate new bug should be filed with detailed steps to reproduce and problem description.

Comment 23

[Mihai Morar, (:MihaiMorar)]

Attachment: Nightly results..jpg

I continued to investigate this problem using the following STR:

1. Install Aurora 25 (2013/08/30)
2. Start these 2 downloads: http://www.softpedia.com/dyn-postdownload.php?p=84329&t=4&i=1 and 
http://www.softpedia.com/dyn-postdownload.php?p=177073&t=4&i=1
3. While downloading those 2 files, Go to Help -> About Aurora and Update to latest version (2013/09/30)
4. When updates are done Restart FF.
5. Open Download Manager

Actual Results:
After step 5 both downloads started in step 2 are FAILED.

Expected Results:
After step 5 both downloads must continue in terms of which remained after step 4.

Using latest nightly for same steps after step 5 a Dialog Box is open. 

If a press "Don't exit" option and then Close FF, after Restart both downloads from step 2 continue in terms of which remained after step 4. If I press "Cancel the 2 Downloads" I get same results as if I choose "Don't exit"

Comment 24

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

Mihai: can you file a new bug with those details?

Comment 25

[Mihai Morar, (:MihaiMorar)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #24)
> Mihai: can you file a new bug with those details?

Filed Bug 922137. Thanks!