385734 - Don't clear download status on pause, clean up DownloadProgressListener

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Ed Lee :Mardak]

I had a patch... but apparently something went wrong... but I have the notes I jotted down..... of various OCD nits :p Patch will be after changes for bug 237693 and bug 385733.

wrong/unecessary emacs formatting hint
gStrings ??
space before ( for functions
trailing spaces on a line
80 characters per line
lastUpdate is being compared to .getTime() (which is on orders of trillions)
refactor getDownload from aDownload.id
short circuit/return early if no download (we're only updating UI in this file)
convert var to let (block level variable scoping)
align new lines correctly
create let block scopes for localized variable use (e.g., |now|)
|aMaxTotalProgress| comes in as a PRInt64 (an integer)
same line returns with ifs
collapse bounds check with |Math.min|
no need to hint float with .0 for javascript
space spaces operators (e.g., * /)
|percent| isn't used on the else path
explicitly show what condition we want (not just implicit "non zero")
consolidate common calls with differing arguments
newline between methods
parameter variables are prefixed with |a|
remove spaces just inside function decl/calls
don't create unnecessary variables
more descriptive variable names

Comment 1

[Ed Lee :Mardak]

Attachment: v1

in order list of various things.. not repeated if it occurs in multiple places.

wrong/unecessary emacs formatting hint
unused gStrings and interval
space before ( for functions
trailing spaces on a line
no need to declare doc null in prototype (we set it in the "constructor")
refactor getDownload from aDownload.id (should it take just aDownload??)
convert var to let (block level variable scoping)
short circuit/return early if no download (we're only updating UI in this file)
80 characters per line
align new lines correctly
create let block scopes for localized variable use (e.g., |percent|)
collapse bounds check with |Math.min|
no need to hint float with .0 for javascript
space spaces operators (e.g., * /)
|percent| isn't used on the else path
no need to check if (download) because we bail early now
newline between member methods
parameter variables are prefixed with |a|
same line returns with ifs
remove spaces just inside function decl/calls's parens
don't create unnecessary variables (for replaceInsert)
more descriptive variable names
explicitly show what condition we want (not just implicit "non zero")

Comment 2

[Shawn Wilsher :sdwilsh]

Comment on attachment 270412 [details] [diff] [review]
v1

>@@ -1,9 +1,8 @@
>-# -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
I don't like emacs either, but that's no reason to remove this.  How about fixing it (s/4/2/) and adding the vim ones too?

>-function DownloadProgressListener (aDocument, aStringBundle) 
>+function DownloadProgressListener(aDocument, aStringBundle)
Since you are here, can you add the javadoc style comments above this please?

>-  doc: null,
>-
You use this in the constructor and in _getDownload, so it should stay.  Although, I'd prefer it if it were named mDoc.

>   onDownloadStateChange: function dlPL_onDownloadStateChange(aState, aDownload)
>   {
>-    var downloadID = "dl" + aDownload.id;
>-    var download = this.doc.getElementById(downloadID);
>-    if (download)
>-      download.setAttribute("state", aDownload.state);
>+    let download = this._getDownload(aDownload.id);
>+    if (!download) return;
>+    download.setAttribute("state", aDownload.state);
Please keep this as |if (download) action...| since it is the last line of the function.

>-  onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus, aDownload)
>+  onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus,
>+                          aDownload)
>   {
>     if (aStateFlags & Components.interfaces.nsIWebProgressListener.STATE_STOP) {
>-      var downloadID = "dl" + aDownload.id;
>-      var download = this.doc.getElementById(downloadID);
>-      if (download)
>-        download.setAttribute("status", "");
>+      let download = this._getDownload(aDownload.id);
>+      if (!download) return;
>+      download.setAttribute("status", "");
ditto

>+    if (aIid.equals(Components.interfaces.nsIDownloadProgressListener) ||
>+        aIid.equals(Components.interfaces.nsISupports)) return this;
|return this;| should be on a newline

Comment 3

[Ed Lee :Mardak]

Attachment: v2

(In reply to comment #2)
> (From update of attachment 270412 [details] [diff] [review])
> How about fixing it (s/4/2/) and adding the vim ones too?
Done. w/ vim/vi compatible vim:set

> >+function DownloadProgressListener(aDocument, aStringBundle)
> Since you are here, can you add the javadoc style comments above this please?
Added comment block at the top and comments above methods as well as blocks above each implemented interface.

> >-  doc: null,
> You use this in the constructor and in _getDownload, so it should stay. 
The constructor is creating the member variable, so no need to publicly include the member variables in the prototype. 
> Although, I'd prefer it if it were named mDoc.
Renamed to mDoc as well as all the other _member variables.

> >   onDownloadStateChange: function dlPL_onDownloadStateChange(aState, aDownload)
> >-    if (download)
> >-      download.setAttribute("state", aDownload.state);
> >+    if (!download) return;
> >+    download.setAttribute("state", aDownload.state);
> Please keep this as |if (download) action...| since it is the last line of the
> function.
> >+  onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus,
> ditto
Done. x2

> >+    if (aIid.equals(Components.interfaces.nsIDownloadProgressListener) ||
> >+        aIid.equals(Components.interfaces.nsISupports)) return this;
> |return this;| should be on a newline
Done.

Comment 4

[Shawn Wilsher :sdwilsh]

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

>+/**
>+ * This file implements the nsIDownloadProgressListener interface.
>+ *
>+ * DownloadProgressListener "class" is used to help update download items shown
>+ * in the Download Manager UI such as displaying amount transferred, transfer
>+ * rate, and time left for each download.
>+ */
@param bits would be nice as well

>+  //
>+  // nsISupports methods..
>+  //
nit: Generally, we'll have ///// until we hit the 80 columns, then on the next line have |//// nsISupports| ;)

>+  // see nsIDownloadProgressListener.onDownloadStateChange
nit: I don't care about these because they are spec'd out by the idl, and we say what interface it is earlier

r=sdwilsh with these fixed.

Comment 5

[Ed Lee :Mardak]

Attachment: v2.1

(In reply to comment #4)
> @param bits would be nice as well
Collapsed into constructor comments w/ @params

> >+  //
> >+  // nsISupports methods..
> nit: Generally, we'll have ///// until we hit the 80 columns, then on the next
> line have |//// nsISupports| ;)
Done.

> >+  // see nsIDownloadProgressListener.onDownloadStateChange
> nit: I don't care about these because they are spec'd out by the idl, and we
> say what interface it is earlier
Gone.

Comment 6

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

Comment on attachment 270651 [details] [diff] [review]
v2.1

A lot of the s/var/let/ changes seem gratuitous, I don't think we should be changing to let where it doesn't actually make a difference. The "m" prefix for the properties is also wrong, "_" is the established style for "private" properties. The rest looks fine, I guess.

Comment 7

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

I also happened to notice that Dao is changing another replaceInsert() in a different way in bug 385787, I guess you might as well sync up with that.

Comment 8

[Ed Lee :Mardak]

Thanks for the review. I'll update the patch after Shawn lands the rewrite, which might already include a lot of these changes.

(In reply to comment #6)
> A lot of the s/var/let/ changes seem gratuitous
The only place that it's /really/ gratuitous is in _formatSeconds, which is actually doesn't exist anymore. Otherwise, the |let|s are in places that are getting touched already:

-    var downloadID = "dl" + aDownload.id;
-    var download = this.doc.getElementById(downloadID);
+    let download = this._getDownload(aDownload.id);

> The "m" prefix for the properties is also wrong, "_" is the established
> style for "private" properties.
Okay, I'll not do that change. (sdwilsh: Just a note to Shawn if he started doing "m" instead of "_" for the rewrite.)

(In reply to comment #7)
> I also happened to notice that Dao is changing another replaceInsert() in a
> different way in bug 385787, I guess you might as well sync up with that.
I'll do that.

Comment 9

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

(In reply to comment #8)
> > A lot of the s/var/let/ changes seem gratuitous
> The only place that it's /really/ gratuitous is in _formatSeconds, which is
> actually doesn't exist anymore. Otherwise, the |let|s are in places that are
> getting touched already:

I think you should leave "var"s in places where "let" doesn't actually make a difference, even if you're already touching the line for some other reason.

Comment 10

[Shawn Wilsher :sdwilsh]

unbitrot?

Comment 11

[Ed Lee :Mardak]

Attachment: unbitrot v1

Not as interesting anymore now that sdwilsh already incorporated quite a few changes into the rewrite.

- fix editor hints
- add DownloadProgressListener comments
- remove trailing spaces
- let vs var ("* let trumps var in any hand ;-)" -brendan) ;)
- section titles
- moved QI up
- wrap at 80
- use Ci instead of Components.interfaces
- be consistent with other getDownload calls (dl vs download) [shorter too]
- add {} to all if/else blocks if at least one uses {}s already
- operators are on the previous line (including the dot)
- lines between methods
- prefix argument names with "a"
- update replaceInsert (bug 385787)

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 277673 [details] [diff] [review]
unbitrot v1

s/var/let/ won't fly with other toolkit reviews, so we won't be able to change that - sorry...


>-DownloadProgressListener.prototype = 
>+DownloadProgressListener.prototype =
> {
The JS coding guidelines actually say to put the brace on the same line as the .prototype

>+  //////////////////////////////////////////////////////////////////////////////
>+  //// nsISupports methods..
>+  //////////////////////////////////////////////////////////////////////////////
I am much more in favor of this style of comment for this:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/inspector/resources/content/viewers/accessibleTree/accessibleTree.js&rev=1.2#46

>+
>+  QueryInterface : function(aIid)
>+  {
>+    if (aIid.equals(Ci.nsIDownloadProgressListener) ||
>+        aIid.equals(Ci.nsISupports))
>+      return this;
>+
>+    throw Cr.NS_NOINTERFACE;
>+  },
While we are at it, why don't we just use XPCOMUtils.jsm's generateQI for this

>+  //////////////////////////////////////////////////////////////////////////////
>+  //// private methods..
>+  //////////////////////////////////////////////////////////////////////////////
s/private methods../DownloadProgressListener/

I'm also unsure if changing download to dl is worth the loss in cvs blame :/

Comment 13

[Ed Lee :Mardak]

Attachment: v3

Refactored the call to createDownloadItem -- note that the "d'oh - why this happens is complicated" case actually was missing 2 arguments (status and start time).

Removed logic for onStateChange.. it doesn't really need to set status to "" -- the appropriate onDownloadStateChange will get called correctly setting status anyway (e.g., pause).

(In reply to comment #12)
> s/var/let/ won't fly
Undone.

> >+DownloadProgressListener.prototype =
> > {
> The JS coding guidelines actually say to put the brace on the same line as the
> .prototype
Joined.

> >+  //////////////////////////////////////////////////////////////////////////////
> >+  //// nsISupports methods..
> >+  //////////////////////////////////////////////////////////////////////////////
> I am much more in favor of this style of comment for this:
> http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/inspector/resources/content/viewers/accessibleTree/accessibleTree.js&rev=1.2#46
Removed the last line and "methods.."

> >+  QueryInterface : function(aIid)
> While we are at it, why don't we just use XPCOMUtils.jsm's generateQI for this
Done. And const-ified C* in downloads.js

> s/private methods../DownloadProgressListener/
Done.
 
> I'm also unsure if changing download to dl is worth the loss in cvs blame :/
Undone. (It was also to fix some wrapping issues, but those are fixed with the _createDownloadItem.)

Note: this is a mercurial gitstyle patch (patch -p1)

Comment 14

[Shawn Wilsher :sdwilsh]

Comment on attachment 278918 [details] [diff] [review]
v3

>+      download = this._createDownloadItem(aDownload);
>+      gDownloadsView.insertBefore(download, gDownloadsActiveTitle.nextSibling);
Are you sure this works?  I thought I recalled this not working if I didn't assign the result of insertBefore to download (could be wrong though).

>-var Cc = Components.classes;
>-var Ci = Components.interfaces;
>+const Cc = Components.classes;
>+const Ci = Components.interfaces;
>+const Cu = Components.utils;
>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
You can't constify those because you'll break places in the DM.
Also, the Cu.import should be done in DownloadProgressListener.js since that is where we are actually using it.

I'd suggest review from Mano next.

Comment 15

[Ed Lee :Mardak]

Attachment: v3.1

(In reply to comment #14)
> (From update of attachment 278918 [details] [diff] [review])
> >+      download = this._createDownloadItem(aDownload);
> >+      gDownloadsView.insertBefore(download, gDownloadsActiveTitle.nextSibling);
> Are you sure this works?  I thought I recalled this not working if I didn't
> assign the result of insertBefore to download (could be wrong though).
insertBefore just gives back the same item that you gave it. It works for webpages, so unless there's something different happening in chrome, it should be fine.

> >+const Cu = Components.utils;
> >+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> You can't constify those because you'll break places in the DM.
> Also, the Cu.import should be done in DownloadProgressListener.js
Un-const-ified and Components.utils.import-ing from DPL.js

About not setting status to "" when the state is STOP, status is used in the UI by downloading and paused (download.xml). And in the case of pausing (+ download resume), onDownloadStateChange will fix up the status anyway.

Comment 16

[Shawn Wilsher :sdwilsh]

Comment on attachment 279175 [details] [diff] [review]
v3.1

r=sdwilsh

Comment 17

[Shawn Wilsher :sdwilsh]

Checking in toolkit/mozapps/downloads/content/DownloadProgressListener.js;
new revision: 1.25; previous revision: 1.24