780533 - Add GUIDs to downloads

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Josh Matthews [:jdm]]

From bug 722859:

>Firstly, to support multiple concurrent databases we need a different ID, and
>I think using GUIDs is the way to go, in line with what we're doing in Places.
>Let's get that done first. You should file a separate bug blocking this one.

>We should probably proceed like for Places, providing new APIs and gradually
>deprecating existing ones. Let's create a new getDownloadByGUID function in
>nsIDownloadManager, and add methods to nsIDownload directly, to cancel,
>resume, and so on. We'll keep the equivalent nsIDownloadManager methods that
>cannot work by GUID around for some time.

Comment 1

[Josh Matthews [:jdm]]

Attachment: Add GUIDs to download manager entries, and a faux-async retrieval by GUID.

Comment 2

[:Paolo Amadini]

Comment on attachment 649166 [details] [diff] [review]
Add GUIDs to download manager entries, and a faux-async retrieval by GUID.

Looks good! Will go through a detailed review in a few days.

Comment 3

[:Paolo Amadini]

Comment on attachment 649166 [details] [diff] [review]
Add GUIDs to download manager entries, and a faux-async retrieval by GUID.

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

This looks like a good patch so far, there's still some work to be done. Below you'll find the observations I could make on my part.

There are a couple of things for which we still need the help of a more experienced reviewer:

1. Do we need to move "generateGUIDFunction" from components/places to another location, if we want to use it from components/downloads? If there isn't a candidate for such location, should we just copy the function?
2. Should we report error codes through the getDownloadByGUID result callback? If so, would an aResult parameter suffice, so that we can keep the "function" annotation on the interface?

I think Marco might have good answers, I'm setting the additional review flag accordingly.

You can work on the other things meanwhile. Later, the updated patch will need a super-review for the final interface changes. Maybe Gavin can do it, or he can indicate an appropriate reviewer. Marco should also look into the updated patch for the final review.

::: toolkit/components/downloads/Makefile.in
@@ +16,5 @@
>  LIBXUL_LIBRARY = 1
>  
> +LOCAL_INCLUDES = \
> +  -I$(topsrcdir)/toolkit/components/places \
> +  $(NULL)

This refers to question "1" in the overview.

::: toolkit/components/downloads/nsDownloadManager.cpp
@@ +304,5 @@
> +InitSQLFunctions(mozIStorageConnection* aDBConn)
> +{
> +  nsresult rv = mozilla::places::GenerateGUIDFunction::create(aDBConn);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  return NS_OK;

This refers to question "1" in the overview.

@@ +327,1 @@
>    mDBType = DATABASE_MEMORY;

It looks like InitMemoryDB is called only if mDBType is already DATABASE_MEMORY, we should remove the assignment.

@@ +357,5 @@
> +  mDBType = DATABASE_DISK;
> +
> +  // We need to create the GUID function whether the table already exists
> +  // or was just created for the first time.
> +  rv = InitSQLFunctions(mDBConn);

Can't we just call InitSQLFunctions before CreateTable? Also, the mDBType assignment is unnecessary here like in InitMemoryDB. This should simplify the flow to be a single "if".

@@ +362,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  
> +  if (!tableExists) {
> +    // We're done with the initialization now and can skip the remaining
> +    // upgrading logic.

But keep this new comment, it's helpful.

@@ +545,5 @@
> +  case 8:
> +    {
> +      rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +                                      "ALTER TABLE moz_downloads "
> +                                      "ADD COLUMN guid TEXT"));

For SQL indentation, we generally use only two spaces because SQL statements may be long, please adhere to the same style as the other calls here.

@@ +565,5 @@
>    case DM_SCHEMA_VERSION:
>  #endif
>      break;
>  
>    case 0:

Later, in the "default:" section (handling future schema versions) there's a SQL statement that should be updated to include the "guid" column for the downgrade check.

There's another compatibility topic to address. It's possible that the database is used on a browser version that didn't use GUIDs. In that case, downloads could be added with a GUID of NULL. Now, as soon as we detect such a download in the database, we should execute a SQL statement to assign a GUID to all the downloads that don't have one.

Note that we shouldn't always run that statement at startup, for performance reasons, but we should do it lazily as soon as we detect this condition of downgrade followed by upgrade. Unfortunately, I don't think we can detect whether the database was used by a previous browser version after we updated the schema version.

@@ +816,5 @@
> +  rv = guidstmt->BindInt64ByName(NS_LITERAL_CSTRING("id"), id);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  bool unused;
> +  rv = guidstmt->ExecuteStep(&unused);
> +  NS_ENSURE_SUCCESS(rv, rv);

nit: you could assert on hasResult, like it's done here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistory.cpp#496

By the way, we could also generate the new GUID ourselves before executing the addition statement, it would be more efficient. However, it seems we let the database function generate the GUID in all other similar code I've seen, so no need to be different here.

@@ +1469,5 @@
>  
>    PRInt64 id = AddDownloadToDB(dl->mDisplayName, source, target, tempPath,
>                                 dl->mStartTime, dl->mLastUpdate,
> +                               mimeType, persistentDescriptor, action,
> +                               dl->mGUID);

Just add a comment that dl->mGUID is an out parameter, populated here.

@@ +1561,5 @@
> +    nsresult rv = GetDownloadFromDB(aGUID, getter_AddRefs(dl));
> +    if (rv != NS_ERROR_NOT_AVAILABLE)
> +      NS_ENSURE_SUCCESS(rv, rv);
> +
> +    itm = dl.get();

Even if GetDownloadFromDB fails, we shouldn't rely on dl being null.

@@ +1565,5 @@
> +    itm = dl.get();
> +  }
> +
> +  aCallback->HandleResult(itm);
> +  return NS_OK;

And in any case, we should probably report errors like NS_ERROR_NOT_AVAILABLE explicitly to the callback.

This refers to question "2" in the overview.

@@ +1610,5 @@
> +  return dl->Cancel();
> +}
> +
> +nsresult
> +nsDownloadManager::RetryDownload(const nsACString& aGUID)

Please move the implementation of RetryDownload to nsDownload::Retry, so we don't need RetryDownload(aGUID) nor RetryDownload(nsDownload*).

@@ +1702,5 @@
> +
> +  // Notify the UI with the topic but no id
> +  return mObserverService->NotifyObservers(nsnull,
> +                                           "download-manager-remove-download",
> +                                           nsnull);

We must specify the ID in the notification to avoid that we refresh the entire user interface every time one download is removed.

Since you call RemoveDownload(aGUID) only from nsDownload::Remove, you should be able to call RemoveDownload(aID) instead, and get rid of this entire method.

@@ +3063,5 @@
>    return SetState(nsIDownloadManager::DOWNLOAD_PAUSED);
>  }
>  
>  nsresult
> +nsDownload::CancelInternal()

nit: can we find a new name for this, that is more related to what the function actually does?

::: toolkit/components/downloads/nsIDownload.idl
@@ +94,5 @@
>  
>      /**
> +     * The guid of the download that is stored in the database.
> +     */
> +    readonly attribute ACString guid;

You might add some more information about how the GUID looks like.

@@ +118,5 @@
> +
> +    /**
> +     * Instruct the download manager to cancel this download.
> +     */
> +    void cancel();

Please copy all the relevant information from the nsIDownloadManager methods in these interface comments, including @throws information and so on, as if the nsIDownloadManager methods did not exist. Also, feel free to add any information you might have learned while working on the code and that you deem relevant.

::: toolkit/components/downloads/nsIDownloadManager.idl
@@ +17,5 @@
>  interface mozIStorageConnection;
>  
> +[scriptable, function, uuid(0c07ffeb-791b-49f3-ae38-2c331fd55a52)]
> +interface nsIDownloadManagerResult : nsISupports {
> +  void handleResult(in nsIDownload aDownload);

This needs a full and exhaustive comment.

This refers to question "2" in the overview.

::: toolkit/components/downloads/test/unit/test_guid.js
@@ +15,5 @@
> +    dm.getDownloadByGUID("nonexistent", function(result) {
> +      do_check_eq(result, null, "should get back no download");
> +      do_test_finished();
> +    });
> +    do_test_finished();

We usually call do_test_pending and do_test_finished only once for the entire asynchronous test.

Comment 4

[Marco Bonardo [:mak]]

(In reply to Paolo Amadini [:paolo] from comment #3) 
> 1. Do we need to move "generateGUIDFunction" from components/places to
> another location, if we want to use it from components/downloads? If there
> isn't a candidate for such location, should we just copy the function?

yes we shouldn't use places code in other toolkit components, specially cause we "ideally" support building without MOZ_PLACES defined. There is no good candidate to hold shared toolkit code like those helpers, so I think copying it is fine, with a comment pointing to the code it should stay in sync with.
The only alternative I may think of, is to provide it as an helper from Storage that is used by both, though it's sort of a stretch.

> 2. Should we report error codes through the getDownloadByGUID result
> callback? If so, would an aResult parameter suffice, so that we can keep the
> "function" annotation on the interface?

heh, it's sort of hard to guess without seeing use-cases, at first glance looks like it may be enough to have an nsresult param, or even just a boolean aSucceeded if we think consumers won't care about which specific error happened. But yeah, there may be cases were you care whether there is no result or there was an error fetching one.

> Note that we shouldn't always run that statement at startup, for performance
> reasons, but we should do it lazily as soon as we detect this condition of
> downgrade followed by upgrade. Unfortunately, I don't think we can detect
> whether the database was used by a previous browser version after we updated
> the schema version.

I don't remember if downloads on schema downgrade sets back the schema version to the previous ones, so that migration runs again. That would be wise. If it does it's an easy problem to solve, otherwise looks like this db schema migration code needs some help.

I hope I answered what was requested, otherwise please ping me, I can review the next iteration.

Comment 5

[Josh Matthews [:jdm]]

Attachment: Interdiff

Comment 6

[Josh Matthews [:jdm]]

Attachment: Add GUIDs to download manager entries, and a faux-async retrieval by GUID. * *

Here's the folded changes.

Comment 7

[:Paolo Amadini]

Comment on attachment 664215 [details] [diff] [review]
Add GUIDs to download manager entries, and a faux-async retrieval by GUID.  * *

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

::: toolkit/components/downloads/SQLFunctions.cpp
@@ +19,5 @@
> +
> +using namespace mozilla::downloads;
> +
> +// Keep this file in sync with toolkit/places/SQLFunctions.cpp and
> +// toolkit/places/Helpers.cpp!

I suggest putting specular notices in the Places files.

::: toolkit/components/downloads/SQLFunctions.h
@@ +15,5 @@
> +namespace downloads {
> +
> +/**
> + * SQL function to generate a GUID for a place or bookmark item.  This is just
> + * a wrapper around GenerateGUID in Helpers.h.

Helpers.h is SQLFunctions.cpp here.

::: toolkit/components/downloads/nsDownloadManager.cpp
@@ +591,5 @@
> +      if (NS_SUCCEEDED(rv)) {
> +        // We have a database that contains all of the elements that make up
> +        // the latest known schema. Reset the version to force an upgrade
> +        // path if this downgraded database is used in a later version.
> +        mDBConn->SetSchemaVersion(DM_SCHEMA_VERSION);

It's worth noting, in a comment after "case 8", that due to this change, upgrade paths that start from versions greater than or equal to 9 should take into account the case that they're actually working on any database version, including future and unknown versions (i.e., calls to ALTER TABLE to add columns could fail because the columns already exist).

This doesn't solve the issue of older versions potentially creating rows with NULL GUIDs. A drastic approach would be to rename the "id" column so that any downgrade destroys the download history and downgrades the schema version. A softer one is to add the missing GUIDs when we load any download that doesn't have one (remembering that consumers using direct database access like the Downloads window should ensure that as well, and might need a support function to that effect).

@@ +809,5 @@
>  
> +  nsCOMPtr<mozIStorageStatement> guidstmt;
> +  rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
> +    "SELECT guid FROM moz_downloads WHERE id = :id"), getter_AddRefs(guidstmt));
> +  NS_ENSURE_SUCCESS(rv, rv);

We could still call GenerateGUID directly in our code for new downloads, so that we don't need this additional select statement anymore (and we'll get rid of some out parameters as well). The SQL function would be used only when batch updating from a previous schema version. But the performance impact is probably not too bad, and we could always change this later, so I think whether doing this now is your call here.

::: toolkit/components/downloads/nsIDownloadManager.idl
@@ +22,5 @@
> +   * Process an asynchronous result from getDownloadByGUID.
> +   *
> +   * @param aStatus The result code of the operation:
> +   *        * NS_OK: an item was found.
> +   *        * NS_ERROR_NOT_AVAILABLE: no such item was found.

If we guarantee that when no item is found we raise NS_ERROR_NOT_AVAILABLE, we should also verify that in the test. In any case, I'd also mention that we may have other failure codes as well.

::: toolkit/components/downloads/test/unit/test_guid.js
@@ +7,5 @@
> +function run_test()
> +{
> +  let dl = addDownload();
> +  do_test_pending();
> +  dm.getDownloadByGUID(dl.guid, function(result) {

This should be updated with the aStatus parameter, but maybe you noticed that already.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 664215 [details] [diff] [review]
Add GUIDs to download manager entries, and a faux-async retrieval by GUID.  * *

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

::: toolkit/components/downloads/SQLFunctions.cpp
@@ +19,5 @@
> +
> +using namespace mozilla::downloads;
> +
> +// Keep this file in sync with toolkit/places/SQLFunctions.cpp and
> +// toolkit/places/Helpers.cpp!

Not all the file must be in sync, please specify this comment refers to the GUID functions, not the whole cpp modules...

::: toolkit/components/downloads/nsDownloadManager.cpp
@@ +541,5 @@
> +    {
> +      rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +                                      "ALTER TABLE moz_downloads "
> +                                      "ADD COLUMN guid TEXT"));
> +      NS_ENSURE_SUCCESS(rv, rv);

you likely want to add a UNIQUE index for your guid column if you ever want to search by guid, otherwise those searches would be extremely slow. Plus it's good for ensure unique GUIDs. Here you have to create the index explicitly cause ALTER doesn't allow to specify UNIQUE... you have to do the same in the default case where you create the table, otherwise we end up with an autoindex and a manually created index (I mean, you have to CREATE INDEX, not to just add UNIQUE to the column definition).

@@ +591,5 @@
> +      if (NS_SUCCEEDED(rv)) {
> +        // We have a database that contains all of the elements that make up
> +        // the latest known schema. Reset the version to force an upgrade
> +        // path if this downgraded database is used in a later version.
> +        mDBConn->SetSchemaVersion(DM_SCHEMA_VERSION);

This is fine, though what paolo said is true, there's the need to specify in a comment (maybe just above the "case 8") that any upgrade from there on will be reapplied on an upgrade after a downgrade, so they must be safed for multiple applications.

And more specifically your case 8 is wrong already, it should check if the column exists, and alter only if it does not exist.

The query that adds GUIDs should always run, but with an additional "WHERE guid ISNULL", otherwise you are basically replacing all the GUIDs, even existing ones.

@@ +643,5 @@
>        "mimeType TEXT, "
>        "preferredApplication TEXT, "
>        "preferredAction INTEGER NOT NULL DEFAULT 0, "
> +      "autoResume INTEGER NOT NULL DEFAULT 0, "
> +      "guid TEXT"

See above, add a CREATE UNIQUE INDEX ON moz_downloads.guid

@@ +809,5 @@
>  
> +  nsCOMPtr<mozIStorageStatement> guidstmt;
> +  rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
> +    "SELECT guid FROM moz_downloads WHERE id = :id"), getter_AddRefs(guidstmt));
> +  NS_ENSURE_SUCCESS(rv, rv);

I second paolo's idea, just make your guid and bind it, so you already know it.

@@ +1561,5 @@
> +  nsRefPtr<nsDownload> dl;
> +  if (!itm) {
> +    rv = GetDownloadFromDB(aGUID, getter_AddRefs(dl));
> +    if (NS_FAILED(rv))
> +      dl = nullptr;

is this really needed? the assignment is the last thing we do in GetdownloadFromDB, it can't fail there so I'd expect this assignment to be a no-op.

@@ +1565,5 @@
> +      dl = nullptr;
> +    itm = dl.get();
> +  }
> +
> +  aCallback->HandleResult(rv, itm);

So, there's a problem here, and you will hate me for this, but basically this is fake-async. If you really want to get in trouble with add-ons and tests give them a fake-async callback that one day will become real-async.
Now, honestly I don't care if you keep it synchronous for internal cpp callers to speed up this priority job, but the external interface must be really async, even if you have to fake it for now by dispatching to main thread (surely you will  file a followup to make it really async beginning from the queries execution, as it's supposed to be).

@@ +1590,5 @@
> +  // we shouldn't ever have many downloads, so we can loop over them
> +  for (int32_t i = mCurrentDownloads.Count() - 1; i >= 0; --i) {
> +    nsDownload *dl = mCurrentDownloads[i];
> +
> +    if (dl->mGUID == aGUID)

nit: useless newline noise

::: toolkit/components/downloads/nsIDownload.idl
@@ +119,5 @@
> +
> +    /**
> +     * Cancel this download if it's currently in progress.
> +     */
> +    void cancel();

With the next patch please provide a good explanation for the reason we have to expose all the download controlling methods now, it will be useful for the SR you need here.

::: toolkit/components/downloads/test/unit/test_guid.js
@@ +7,5 @@
> +function run_test()
> +{
> +  let dl = addDownload();
> +  do_test_pending();
> +  dm.getDownloadByGUID(dl.guid, function(result) {

may you please check the guid format?
you can copy this regex http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/head_common.js#680

Comment 9

[Josh Matthews [:jdm]]

Attachment: Add GUIDs to download manager entries, and a faux-async retrieval by GUID. 780533 - Add GUIDs to download manager entries, and a faux-async retrieval by GUID. * *

I believe I have addressed all comments. With regards to the additions to nsIDownload, it makes it easier to manipulate download objects without having to care about whether an integer ID or GUID is being used. My one comment from Paolo that I realized I haven't addressed is his concern about the download-manager-remove-download notification, which passes no ID or GUID when a download is removed by GUID. Marco, what's the best way to handle that?

Comment 10

[Marco Bonardo [:mak]]

(In reply to Josh Matthews [:jdm] from comment #9)
> My one
> comment from Paolo that I realized I haven't addressed is his concern about
> the download-manager-remove-download notification, which passes no ID or
> GUID when a download is removed by GUID. Marco, what's the best way to
> handle that?

existing consumers expect there to be the id for a single download or null for multiple downloads, the latter case is slower cause the consumer doesn't know what to change so will update everything, though downloads list is usually not too long.
So I'd say, if it's easy and checp to know the old id, pass it out, otherwise just pass a null subject, the consumer will be less performant but since this is supposed to happen in pb mode shouldn't be an heavy task. We'll need some redesign in future to better homogenize the different ids in the whole service.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 666714 [details] [diff] [review]
Add GUIDs to download manager entries, and a faux-async retrieval by GUID.  780533 - Add GUIDs to download manager entries, and a faux-async retrieval by GUID.  * *

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

I don't think I have more insight to give about this, so r=me with the following addressed, remember you need SR on the API changes

::: toolkit/components/downloads/nsDownloadManager.cpp
@@ +360,5 @@
>      rv = CreateTable();
>      NS_ENSURE_SUCCESS(rv, rv);
> +
> +    // We're done with the initialization now and can skip the remaining
> +    // upgrading logic.

I assume you removed the mDBType = DATABASE_DISK assignments cause we enter InitFileDB() through a switch-case on mDBType itself. If so, why didn't you remove the similar assignment in InitMemoryDB?

@@ +551,5 @@
> +      rv = mDBConn->IndexExists(NS_LITERAL_CSTRING("moz_downloads_guid_uniqueindex"),
> +                                &exists);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +
> +      if (!exists) {

nit: remove newline

@@ +557,5 @@
> +                                         "ALTER TABLE moz_downloads "
> +                                         "ADD COLUMN guid TEXT"));
> +        NS_ENSURE_SUCCESS(rv, rv);
> +
> +        rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(

nit: remove newline

@@ +558,5 @@
> +                                         "ADD COLUMN guid TEXT"));
> +        NS_ENSURE_SUCCESS(rv, rv);
> +
> +        rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +                                         "CREATE UNIQUE INDEX guid_idx "

the name you use here should be the same you checked with indexExists!
nit: indent just by 2 spaces, eventually oneline the query if it fits.

@@ +566,5 @@
> +
> +      rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +                                        "UPDATE moz_downloads "
> +                                        "SET guid = GENERATE_GUID() "
> +                                        "WHERE guid ISNULL"));

nit: indent just by 2 spaces

@@ +569,5 @@
> +                                        "SET guid = GENERATE_GUID() "
> +                                        "WHERE guid ISNULL"));
> +      NS_ENSURE_SUCCESS(rv, rv);
> +
> +      // Finally, update the schemaVersion variable and the database schema

nit: s/the schemaVersion variable and// (useless to repeat code in comments)

@@ +826,5 @@
>  
>    rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("preferredAction"), aPreferredAction);
>    NS_ENSURE_SUCCESS(rv, 0);
>  
> +  nsCString guid;

Use an autostring, the guid is short

@@ +830,5 @@
> +  nsCString guid;
> +  rv = GenerateGUID(guid);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  rv = stmt->BindUTF8StringByName(NS_LITERAL_CSTRING("guid"), guid);
> +  NS_ENSURE_SUCCESS(rv, 0);

something wrong in the previus NS_ENSURE_SUCCESS... the second argument should not be rv

@@ +1029,1 @@
>                 "If it is a current download, you should not call this method!");

while here please convert these NS_ASSERTION to MOZ_ASSERT

@@ +1046,5 @@
> +nsresult
> +nsDownloadManager::GetDownloadFromDB(PRUint32 aID, nsDownload **retVal)
> +{
> +  NS_ASSERTION(!FindDownload(aID),
> +               "If it is a current download, you should not call this method!");

ditto re MOZ_ASSERT

@@ +1172,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // Handle situations where we load a download from a database that has been
> +  // used in an older version and not gone through the upgrade path (ie. it
> +  // contains empty GUID entries).

may this actually happen? The upgrade path always adds GUIDs to any download with a null GUID. If this fails in a test it's more likely the database used in the test needs an update or the upgrade from downgrade code is broken...

This code shouldn't exist, the original bug should be fixed instead, if it exists at all.

@@ +1609,5 @@
>  
>    return NS_OK;
>  }
>  
> +class AsyncResult : public nsRunnable

put this into an anonymous namespace, since it's a temporary local helper

@@ +1668,5 @@
> +  // we shouldn't ever have many downloads, so we can loop over them
> +  for (int32_t i = mCurrentDownloads.Count() - 1; i >= 0; --i) {
> +    nsDownload *dl = mCurrentDownloads[i];
> +
> +    if (dl->mGUID == aGUID)

looks like you removed the newline in the other FindDownload but not here :)

@@ +1783,5 @@
> +
> +  // Notify the UI with the topic but no id
> +  return mObserverService->NotifyObservers(nullptr,
> +                                           "download-manager-remove-download",
> +                                           nullptr);

As I said, I tink you can get the id from dl for free (don't remember off-hand), if so put it out with the notification

Comment 12

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] from comment #11)
> may this actually happen? The upgrade path always adds GUIDs to any download
> with a null GUID. If this fails in a test it's more likely the database used
> in the test needs an update or the upgrade from downgrade code is broken...

All older platform versions (before this patch lands) won't downgrade the schema
version, they just work on the downloads table ignoring fields they don't
know about. For example, if we open a recent profile in an older Firefox version,
and then again in the same recent version, we don't apply any upgrade path and
we may end up with empty GUIDs.

Comment 13

[Marco Bonardo [:mak]]

(In reply to Paolo Amadini [:paolo] from comment #12)
> All older platform versions (before this patch lands) won't downgrade the
> schema

Ah, I indeed missed going back to < 8 and then to 8... fine to keep the check then.

Comment 14

[Josh Matthews [:jdm]]

Comment on attachment 666714 [details] [diff] [review]
Add GUIDs to download manager entries, and a faux-async retrieval by GUID.  780533 - Add GUIDs to download manager entries, and a faux-async retrieval by GUID.  * *

Boris, can you approve the interface changes? The exposure of more control methods on nsIDownload is to help consumers care less about GUID vs. integer id.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 666714 [details] [diff] [review]
Add GUIDs to download manager entries, and a faux-async retrieval by GUID.  780533 - Add GUIDs to download manager entries, and a faux-async retrieval by GUID.  * *

It's worth documenting whether any _success_ values other than NS_OK are possible.  Presumably they're not.

sr=me with that

Comment 16

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=9bac261d5c2f

Comment 17

[Josh Matthews [:jdm]]

Attachment: Add GUIDs to download manager entries, and a faux-async retrieval by GUID. 780533 - Add GUIDs to download manager entries, and a faux-async retrieval by GUID. * *

Bug 780533 - Interdiff

Comment 18

[:Paolo Amadini]

Josh, what's the status of this bug with regard to reviews and/or patch updates?

Comment 19

[Josh Matthews [:jdm]]

I've updated the patch locally to address all comments. I figured I would land it at the same time as bug 722859.

Comment 20

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/549633930ef7

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/549633930ef7