815941 - Remove the usages of nsIDownload.id and nsIDownloadManager.getDownload from the downloads panel code

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

[(no longer active)]

Follow-up from bug 801232 comment 50.

Comment 1

[(no longer active)]

Attachment: WIP

OK this is a WIP, please give me feedback!

One thing that I have not gotten right I think is in the generator in _activeDownloads.  I seem to be getting a null at runtime which breaks the download panel as you might expect.  It could be a result of not knowing this fancy JS stuff. ;-)

Comment 2

[(no longer active)]

Note that I'd also appreciate if you guys can apply the patch and test it while giving feedback to make sure that there isn't anything innocent looking in the patch which breaks the downloads panel.  :-)

Comment 3

[Josh Matthews [:jdm]]

>for each (let download in downloads) {

I believe you want |let download of downloads|.

Comment 4

[(no longer active)]

Attachment: WIP 2

This patch fixes a number of bugs...  There are no more JS errors.  But there are at least three things broken with it:

1. The download button doesn't show the time progress bar.
2. You can't pause and resume downloads from the panel (the pause button does not show up)
3. Retrying a canceled download causes nsIDownload::Retry to throw.

Comment 5

[(no longer active)]

See <https://bugzilla.mozilla.org/attachment.cgi?oldid=686421&action=interdiff&newid=686578&headers=1> for what I changed.

Comment 6

[Josh Matthews [:jdm]]

Comment on attachment 686578 [details] [diff] [review]
WIP 2

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

::: browser/components/downloads/src/DownloadsCommon.jsm
@@ +246,4 @@
>  
>      // If no download has been loaded, don't use the methods of the Download
>      // Manager service, so that it is not initialized unnecessarily.
> +    for (let download in aDownloads) {

This isn't going to work; download will be an index into the array.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 686578 [details] [diff] [review]
WIP 2

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

Most of this looks OK to me so far. Gonna give this one a spin and see if I can figure out why those few things are still busted.

::: browser/components/downloads/content/downloads.js
@@ +1244,5 @@
>      {
>        if (this.dataItem.inProgress) {
> +        this.dataItem.getDownload(function(aDownload) {
> +          aDownload.cancel();
> +          aDownload.remove();

Hold up - are we sure we want to be removing the download at this stage? The user only gave the command to cancel.

::: browser/components/downloads/src/DownloadsCommon.jsm
@@ +1476,5 @@
>      // If no download has been loaded, don't use the methods of the Download
>      // Manager service, so that it is not initialized unnecessarily.
> +    let downloads = DownloadsCommon.data.dataItems;
> +    if (downloads.length > 0) {
> +      for each (let download in downloads) {

I think we're getting a bit confused between downloads and dataItems - I think I'd prefer "downloads" in this context to be renamed dataItems.

Also, I think you can simplify this with:

if (dataItems.length > 0) {
  for (let dataItem of dataItems) {
    yield dataItem;
  }
}

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 686578 [details] [diff] [review]
WIP 2

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

::: browser/components/downloads/src/DownloadsCommon.jsm
@@ +1475,5 @@
>    {
>      // If no download has been loaded, don't use the methods of the Download
>      // Manager service, so that it is not initialized unnecessarily.
> +    let downloads = DownloadsCommon.data.dataItems;
> +    if (downloads.length > 0) {

So this doesn't work because dataItems is an Object, so it doesn't have a .length.

An alternative would be to use one of those fancy new Map things (https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Map) - that'll give us our hashmap, as well as a .size property.

If that doesn't work, you can always check the cardinality of an Object with Object.keys(dataItems).length. Not pretty, but would work here, I think.

Fixing this will fix the indicator button / progress bar thing.

Comment 9

[:Paolo Amadini]

Comment on attachment 686578 [details] [diff] [review]
WIP 2

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

I've also spent some time on getting this patch locally. After applying these changes, the patch seems to work, but please do some thorough testing on the updated version (I've only tried a few significant cases).

::: browser/components/downloads/content/downloads.js
@@ +1236,5 @@
>  
> +      this.dataItem.getDownload(function(aDownload) {
> +        aDownload.cancel();
> +        aDownload.remove();
> +      });

We're calling "this.commands.downloadsCmd_cancel.apply(this);" but this is not going to work anymore in asynchronous mode. We should call getDownload once, and inside it:
- If in progress, call cancel, then remove the file if present _after_ cancel returns.
- After that, in any case, remove the download.

About item removal, globally change "download-manager-remove-download" into "download-manager-remove-download-guid", then use: aSubject.QueryInterface(Ci.nsISupportsCString).data

@@ +1244,5 @@
>      {
>        if (this.dataItem.inProgress) {
> +        this.dataItem.getDownload(function(aDownload) {
> +          aDownload.cancel();
> +          aDownload.remove();

Mike's right.

@@ +1367,5 @@
> +          aDownload.resume();
> +        } else {
> +          aDownload.pause();
> +        }
> +      }.bind(this.dataItem));

Please bind "this" for better readability.

::: browser/components/downloads/src/DownloadsCommon.jsm
@@ +246,4 @@
>  
>      // If no download has been loaded, don't use the methods of the Download
>      // Manager service, so that it is not initialized unnecessarily.
> +    for (let download in aDownloads) {

Yes, let...of is the correct form.

We have a collection of iterable items: "let element of list" is equivalent to "let [index, element] in Iterator(list)".

@@ +705,5 @@
> +            dataItem.getDownload(function(aDownload) {
> +              if (!aDownload) {
> +                this._removeDataItem(dataItem.downloadGuid);
> +              }
> +            }.bind(this));

You don't want to go through the lazy getter here, since this will return a valid reference if the download was accessed previously, but what we care about the current existence state of the item.

Either access nsIDownloadManager directly, or create a different asynchronous method on the dataItem.

@@ +960,5 @@
> +        }
> +      }, Ci.nsIThread.DISPATCH_NORMAL);
> +    } else {
> +      Services.downloads.getDownloadByGUID(this.downloadGuid, function(status, result) {
> +        aCallback(result);

It seems you don't assign _download anywhere.

For better diagnostics:

+        if (!Components.isSuccessCode(status)) {
+          Cu.reportError(new Components.Exception(
+                         "Cannot retrieve download for GUID: " + this.downloadGuid,
+                         status));
+        } else {
+          this._download = result;
+          aCallback(result);
+        }

I'd rather you defined the property explicitly, and checked for null instead of existence. See the "_attention" property below in the same file for style reference.

nit: status -> aStatus

@@ +1482,1 @@
>        }

+      for each (let dataItem in DownloadsCommon.data.dataItems) {
+        if (dataItem && dataItem.inProgress) {
+          yield dataItem;
+        }
+      }

DownloadsCommon.data.dataItems is an object used like a map, not an array, and it can contain properties which map to null values (for removed downloads, to handle a possible race condition in database loading).

There's no need to check for _itemCount anymore: this was an optimization, and if there are no in-progress items, this generator would simply yield no items at all.

Globally, please check that comments still reflect what the code currently does.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 686578 [details] [diff] [review]
WIP 2

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

::: browser/components/downloads/content/downloads.js
@@ +1234,5 @@
>      {
>        this.commands.downloadsCmd_cancel.apply(this);
>  
> +      this.dataItem.getDownload(function(aDownload) {
> +        aDownload.cancel();

In this function, it's possible that aDownload was already cancelled, which will cause an assertion to fail if we try to cancel it again.

You should check the inProgress state of the dataItem like in downloadsCmd_cancel before doing a .cancel().

Comment 11

[Mike Conley (:mconley) (:⚙️)]

(In reply to Ehsan Akhgari [:ehsan] from comment #4)
> 2. You can't pause and resume downloads from the panel (the pause button
> does not show up)

Just FYI - there *is* no pause button. That command only exists in the context menu. The only buttons that will appear on the download item are Cancel, Retry, and Open Containing Folder (if completed).

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

I'm gonna scoop this one from Ehsan and take some pressure off.

So, this patch appears to do the job, and the tests pass (I had to do a bit of fiddling to make the tests work again).

Gonna give 'er a once over and then r?.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2

Updated some documentation to reflect these changes. I think we're good to go here for review.

Comment 14

[:Paolo Amadini]

Comment on attachment 686770 [details] [diff] [review]
Patch v2

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

Looks good overall!

::: browser/components/downloads/content/downloads.js
@@ +1313,2 @@
>          }
> +      });

Missing .bind(this)

::: browser/components/downloads/src/DownloadsCommon.jsm
@@ +708,5 @@
> +
> +          if (dataItemBinding) {
> +            Services.downloads.getDownloadByGUID(dataItemBinding.downloadGuid,
> +                                                 function(aStatus, aResult) {
> +              if (!aResult) {

Per documentation, this should check either !isSuccessCode(aStatus) or more specifically aStatus == Cr.NS_ERROR_NOT_AVAILABLE.

@@ +763,5 @@
>      // When a download is retried, we create a different download object from
>      // the database with the same ID as before. This means that the nsIDownload
>      // that the dataItem holds might now need updating.
> +    if (dataItem._download) {
> +      dataItem._download = aDownload;

nit: clarifying why we assign only if the value is not null can be helpful.

@@ +964,5 @@
> +      Services.tm.mainThread.dispatch({
> +        run: function() {
> +          aCallback(download);
> +        }
> +      }, Ci.nsIThread.DISPATCH_NORMAL);

Given the [function] IDL attribute on nsIRunnable, you can just dispatch "function() aCallback(download)".

@@ +1488,5 @@
>     */
>    _activeDownloads: function DID_activeDownloads()
>    {
>      // If no download has been loaded, don't use the methods of the Download
>      // Manager service, so that it is not initialized unnecessarily.

Outdated comment.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v3 (feedback+ from paolo)

Thanks Paolo! Fixed those issues.

Comment 16

[(no longer active)]

Thanks a lot Mike!  :-)

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 686855 [details] [diff] [review]
Patch v3 (feedback+ from paolo)

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

Nothing particularly bad, some things to cleanup

thank you

::: browser/components/downloads/content/downloads.js
@@ +1231,5 @@
>     */
>    commands: {
>      cmd_delete: function DVIC_cmd_delete()
>      {
> +      this.dataItem.getDownload(function(aDownload) {

really-really-really-global-nit: as code style for this component (and more generally browser) we prefer a whitespace for anon functions like "function ()"

::: browser/components/downloads/src/DownloadsCommon.jsm
@@ +210,3 @@
>     * statistics about that collection.
>     *
> +   * @param aDownloads An iterable collection of DownloadsDataItems.

DownloadsDataItems is a typo, should be DownloadDataItems.. there are 2 instances here, but I actually 
found another one in existing code that I'd appreciate if you could fix while here
http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/src/DownloadsCommon.jsm#1566


The param should be renamed, from aDownloads to aDataItems aDownloadDataItems, whatever you prefer

@@ +271,4 @@
>            download.state != nsIDM.DOWNLOAD_CANCELED &&
>            download.state != nsIDM.DOWNLOAD_FAILED) {
> +        summary.totalSize += download.maxBytes;
> +        summary.totalTransferred += download.currBytes;

Everywhere where we have "download" here we should have dataItem... the input was previously nsIDownloads, it's now DownloadDataItems.

@@ +705,5 @@
> +          // Bug 449811 - We have to bind to the dataItem because Javascript
> +          // doesn't do fresh let-bindings per loop iteration.
> +          let dataItemBinding = dataItem;
> +
> +          if (dataItemBinding) {

I'd prefer if you invert these (assignment and check), the if (dataItem) condition can be checked externally and the "binding" is actually just needed in the callback, so I like it being defined just above it. I honestly thought this was not even considered a bug, we use the same "bindind" method in many code pieces.

@@ +945,3 @@
>      } else {
>        this.resumable = true;
>      }

I'm unsure about this change, to me looks like we should default this.resumable = true, and in the downloading case update it with the callback result to eventually false.
If I'm not wrong this is used for the pause/resume action, is it better to tell the user "we can't resume" when we can, or "we can resume" when we can't? I suppose the latter is the better compromise (well this is likely to be resolved even before the user may look at the button, so minor bug regardless).

@@ +966,5 @@
> +      // Return the download object asynchronously to the caller
> +      let download = this._download;
> +      Services.tm.mainThread.dispatch({
> +        run: function() aCallback(download)
> +      }, Ci.nsIThread.DISPATCH_NORMAL);

As paolo (I think) said, you don't need a runnable object here, just dispatch a function

@@ +1491,5 @@
>    _activeDownloads: function DID_activeDownloads()
>    {
> +    for each (let dataItem in DownloadsCommon.data.dataItems) {
> +      if (dataItem && dataItem.inProgress) {
> +        yield dataItem;

So, this was returning nsIDownloads, it is now returning dataItems... it should be renamed to _activeDataItems, both here (name and label) and where we pass it to summarizeDownloads

@@ +1689,5 @@
>     * to generate statistics about the downloads we care about - in this case,
>     * it's the downloads in this._dataItems after the first few to exclude,
>     * which was set when constructing this DownloadsSummaryData instance.
>     */
>    _downloadsForSummary: function DSD_downloadsForSummary()

as well as this one is no more _downloadsForSummary, it's now _dataItemsForSummary

::: browser/components/downloads/test/browser/head.js
@@ +178,5 @@
>    let statement = Services.downloads.DBConnection.createAsyncStatement(
>                    "INSERT INTO moz_downloads (" + columnNames +
>                                      ") VALUES(" + parameterNames + ")");
>    try {
> +    for (let i = 0; i < aDataRows.length; ++i) {

I'm honestly not sure why we were doing this loop in reverse and now we do it forward... sorting by startTime? I think we may want to startTime++ at each added object, like we already do with endTime.

@@ +193,5 @@
>        }
>  
> +      // Because we're adding downloads directly to the database, we don't
> +      // get GUID's created for us automatically, so we have to do it ourselves.
> +      statement.params.guid = UUIDGenerator.generateUUID().toString();

We don't use UUIDs as guids, we use a special 12 chars GUID format...

but you should be happy since the solution is even simpler:
1. don't add guid to gDownloadRowTemplate
2. change the query to
INSERT INTO moz_downloads (" + columnNames +", guid) VALUES(" + parameterNames +", GENERATE_GUID())

And then you can also remove the uuidGenerator lazyServiceGetter

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v4 (r+'d by mak)

(In reply to Marco Bonardo [:mak] from comment #17)
> Comment on attachment 686855 [details] [diff] [review]
> Patch v3 (feedback+ from paolo)
> 
> Review of attachment 686855 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nothing particularly bad, some things to cleanup
> 
> thank you
> 
> ::: browser/components/downloads/content/downloads.js
> @@ +1231,5 @@
> >     */
> >    commands: {
> >      cmd_delete: function DVIC_cmd_delete()
> >      {
> > +      this.dataItem.getDownload(function(aDownload) {
> 
> really-really-really-global-nit: as code style for this component (and more
> generally browser) we prefer a whitespace for anon functions like "function
> ()"
> 

Ah, I never knew that. Fixed.

> ::: browser/components/downloads/src/DownloadsCommon.jsm
> @@ +210,3 @@
> >     * statistics about that collection.
> >     *
> > +   * @param aDownloads An iterable collection of DownloadsDataItems.
> 
> DownloadsDataItems is a typo, should be DownloadDataItems.. there are 2
> instances here, but I actually 
> found another one in existing code that I'd appreciate if you could fix
> while here
> http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/
> src/DownloadsCommon.jsm#1566
> 

All fixed - good catch.

> 
> The param should be renamed, from aDownloads to aDataItems
> aDownloadDataItems, whatever you prefer
> 
> @@ +271,4 @@
> >            download.state != nsIDM.DOWNLOAD_CANCELED &&
> >            download.state != nsIDM.DOWNLOAD_FAILED) {
> > +        summary.totalSize += download.maxBytes;
> > +        summary.totalTransferred += download.currBytes;
> 
> Everywhere where we have "download" here we should have dataItem... the
> input was previously nsIDownloads, it's now DownloadDataItems.
> 

Done.

> @@ +705,5 @@
> > +          // Bug 449811 - We have to bind to the dataItem because Javascript
> > +          // doesn't do fresh let-bindings per loop iteration.
> > +          let dataItemBinding = dataItem;
> > +
> > +          if (dataItemBinding) {
> 
> I'd prefer if you invert these (assignment and check), the if (dataItem)
> condition can be checked externally and the "binding" is actually just
> needed in the callback, so I like it being defined just above it. I honestly
> thought this was not even considered a bug, we use the same "bindind" method
> in many code pieces.
> 

Done.

> @@ +945,3 @@
> >      } else {
> >        this.resumable = true;
> >      }
> 
> I'm unsure about this change, to me looks like we should default
> this.resumable = true, and in the downloading case update it with the
> callback result to eventually false.
> If I'm not wrong this is used for the pause/resume action, is it better to
> tell the user "we can't resume" when we can, or "we can resume" when we
> can't? I suppose the latter is the better compromise (well this is likely to
> be resolved even before the user may look at the button, so minor bug
> regardless).
> 

I went with your suggestion, and default to resumable = true. I also added a comment about that.

> @@ +966,5 @@
> > +      // Return the download object asynchronously to the caller
> > +      let download = this._download;
> > +      Services.tm.mainThread.dispatch({
> > +        run: function() aCallback(download)
> > +      }, Ci.nsIThread.DISPATCH_NORMAL);
> 
> As paolo (I think) said, you don't need a runnable object here, just
> dispatch a function
> 

Ok, now just dispatching a function.

> @@ +1491,5 @@
> >    _activeDownloads: function DID_activeDownloads()
> >    {
> > +    for each (let dataItem in DownloadsCommon.data.dataItems) {
> > +      if (dataItem && dataItem.inProgress) {
> > +        yield dataItem;
> 
> So, this was returning nsIDownloads, it is now returning dataItems... it
> should be renamed to _activeDataItems, both here (name and label) and where
> we pass it to summarizeDownloads
> 

Done!

> @@ +1689,5 @@
> >     * to generate statistics about the downloads we care about - in this case,
> >     * it's the downloads in this._dataItems after the first few to exclude,
> >     * which was set when constructing this DownloadsSummaryData instance.
> >     */
> >    _downloadsForSummary: function DSD_downloadsForSummary()
> 
> as well as this one is no more _downloadsForSummary, it's now
> _dataItemsForSummary
> 

Fixed.

> ::: browser/components/downloads/test/browser/head.js
> @@ +178,5 @@
> >    let statement = Services.downloads.DBConnection.createAsyncStatement(
> >                    "INSERT INTO moz_downloads (" + columnNames +
> >                                      ") VALUES(" + parameterNames + ")");
> >    try {
> > +    for (let i = 0; i < aDataRows.length; ++i) {
> 
> I'm honestly not sure why we were doing this loop in reverse and now we do
> it forward... sorting by startTime? I think we may want to startTime++ at
> each added object, like we already do with endTime.
> 

Ah, thanks. Ok, now that we're incrementing startTime, I've put the reverse-loop back, as well as the original comment.

> @@ +193,5 @@
> >        }
> >  
> > +      // Because we're adding downloads directly to the database, we don't
> > +      // get GUID's created for us automatically, so we have to do it ourselves.
> > +      statement.params.guid = UUIDGenerator.generateUUID().toString();
> 
> We don't use UUIDs as guids, we use a special 12 chars GUID format...
> 
> but you should be happy since the solution is even simpler:
> 1. don't add guid to gDownloadRowTemplate
> 2. change the query to
> INSERT INTO moz_downloads (" + columnNames +", guid) VALUES(" +
> parameterNames +", GENERATE_GUID())
> 
> And then you can also remove the uuidGenerator lazyServiceGetter

I guess it was kind of a long shot. :) Thanks, yes, that's much better.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v5 (r+'d by mak)

Ok, did a quick self-review, fixed a trailing whitespace, trailing comma, and an unneeded newline.

I think we're good here. I'm gonna land this.

Thanks again, Marco!

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Landed on mozilla-inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/29c8aceb7068

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/29c8aceb7068