1115364 - Code style refresh

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

[:Paolo Amadini]

Attachment: The patch

Making style consistent with the style guide across all JavaScript files in the Downloads Panel folder. In particular, removing all the now redundant function names in favor of the new JavaScript method shorthands.

While there, I fixed a warning in PrefObserver, removed some unneeded lazy getter logic in DownloadsCommon (maybe an artifact of a former refactoring), and fixed a redundant call to DownloadsCommon.error (it calls Cu.reportError already).

Smoketested this and everything seems to work correctly.

Feel free to point out other style fixes I missed or you might want to see as part of this patch.

Comment 1

[:Paolo Amadini]

Attachment: The patch

Hm, maybe including the changes is more informative than an empty patch :-)

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8541255 [details] [diff] [review]
The patch

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

r+ with issues below addressed.

::: browser/components/downloads/DownloadsCommon.jsm
@@ +117,2 @@
>      if (this.prefs.hasOwnProperty(aData)) {
> +      delete this[aData];

Why do we need to delete this property? Are there properties that have setters that we need to make sure don't exist before we overwrite the property?

@@ +548,5 @@
>      if (type) {
>        message = type + "\n\n" + message;
>      }
>  
> +    Services.ww.registerNotification((subj, topic) => {

You can't remove the name of this function ("onOpen"). It's referenced below by Services.ww.unregisterNotification(onOpen);

::: browser/components/downloads/content/allDownloadsViewOverlay.js
@@ +474,1 @@
>               this._dataItem.percentComplete == -1) {

Please adjust the indentation of this line now.

@@ -1090,2 @@
>  
> -      let nodeABoveVisibleArea =

Good find about the s/nodeABoveVisibleArea/nodeAboveVisibleArea/

@@ +1278,5 @@
>    },
>  
> +  nodeIconChanged(aNode) {
> +    this._forEachDownloadElementShellForURI(aNode.uri,
> +                         e => e.placesNodeIconChanged());

Please use a more descriptive name than `e` (here and below). Also, I would prefer if this was only 2-space indented. It's not obvious what this is indented to align with (besides the two functions below it).

@@ +1395,5 @@
>        case "downloadsCmd_clearDownloads":
>          return this._canClearDownloads();
>        default:
> +        return Array.every(this._richlistbox.selectedItems,
> +                           e => e._shell.isCommandEnabled(aCommand));

s/e/element/

::: browser/components/downloads/content/indicator.js
@@ +252,5 @@
>        return;
>      }
>  
> +    DownloadsOverlayLoader.ensureOverlayLoaded(
> +                                 DownloadsButton.kIndicatorOverlay, () => {

This indentation seems awkward. Maybe just go for the 2-space indentation?

Comment 3

[:Paolo Amadini]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> >      if (this.prefs.hasOwnProperty(aData)) {
> > +      delete this[aData];
> 
> Why do we need to delete this property? Are there properties that have
> setters that we need to make sure don't exist before we overwrite the
> property?

It's defined as a getter by the defineLazyGetter below. Any attempt to overwrite a getter with a property generates a warning. In this case, this will happen only if the lazy getter itself is not invoked first, when you change the preference before opening the panel for the first time. The lazy getter code generated by defineLazyGetter already contains the "delete".

> > +    Services.ww.registerNotification((subj, topic) => {
> 
> You can't remove the name of this function ("onOpen"). It's referenced below
> by Services.ww.unregisterNotification(onOpen);

Good catch!

> > +  nodeIconChanged(aNode) {
> > +    this._forEachDownloadElementShellForURI(aNode.uri,
> > +                         e => e.placesNodeIconChanged());
> 
> Please use a more descriptive name than `e` (here and below).

I use one-letter shorthands when the lambda function is simple. Compare:

visibleViewElementsArray.filter(x => x.priority < 2);
visibleViewElementsArray.filter(visibleViewElement => visibleViewElement.priority < 2);

I find the former easier to read, especially when the function or variable is self-documenting. Descriptive names can be useful in other cases:

unrenameableFooBar.filter(visibleViewElement => visibleViewElement.index < 2);

I believe with forEachDownloadElementShellForURI it's quite clear the item is a DownloadElementShell?

So, maybe I should use "x" in these cases instead of an arbitrary letter?

These are my guidelines but they're not official style, so I may also use a longer name if you prefer.

> Also, I would
> prefer if this was only 2-space indented. It's not obvious what this is
> indented to align with (besides the two functions below it).

This is really nitpicking on my part, but I find two-space line continuation is easily confused with two-space control block indentation. On the other hand, any larger indentation would often be arbitrary, so I'm not opposed to using two spaces, and I've not changed instances of it when I found it in this code.

(aside: my aesthetic rule of "align with rightmost usable uppercase letter" is definitely arbitrary.)

> @@ +1395,5 @@
> >        case "downloadsCmd_clearDownloads":
> >          return this._canClearDownloads();
> >        default:
> > +        return Array.every(this._richlistbox.selectedItems,
> > +                           e => e._shell.isCommandEnabled(aCommand));
> 
> s/e/element/

This might be a good place use a more explicit name, I agree.

> ::: browser/components/downloads/content/indicator.js
> @@ +252,5 @@
> >        return;
> >      }
> >  
> > +    DownloadsOverlayLoader.ensureOverlayLoaded(
> > +                                 DownloadsButton.kIndicatorOverlay, () => {
> 
> This indentation seems awkward. Maybe just go for the 2-space indentation?

In other code in the past I've used two-space for the line wrap and then four-space for the function body. I could use this style here.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Paolo Amadini from comment #3)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> > >      if (this.prefs.hasOwnProperty(aData)) {
> > > +      delete this[aData];
> > 
> > Why do we need to delete this property? Are there properties that have
> > setters that we need to make sure don't exist before we overwrite the
> > property?
> 
> It's defined as a getter by the defineLazyGetter below. Any attempt to
> overwrite a getter with a property generates a warning. In this case, this
> will happen only if the lazy getter itself is not invoked first, when you
> change the preference before opening the panel for the first time. The lazy
> getter code generated by defineLazyGetter already contains the "delete".

Thanks for the explanation!

> > > +  nodeIconChanged(aNode) {
> > > +    this._forEachDownloadElementShellForURI(aNode.uri,
> > > +                         e => e.placesNodeIconChanged());
> > 
> > Please use a more descriptive name than `e` (here and below).
> 
> I use one-letter shorthands when the lambda function is simple.

That's reasonable. How about instead of 'x' or 'e', we use 'des'?

> > ::: browser/components/downloads/content/indicator.js
> > @@ +252,5 @@
> > >        return;
> > >      }
> > >  
> > > +    DownloadsOverlayLoader.ensureOverlayLoaded(
> > > +                                 DownloadsButton.kIndicatorOverlay, () => {
> > 
> > This indentation seems awkward. Maybe just go for the 2-space indentation?
> 
> In other code in the past I've used two-space for the line wrap and then
> four-space for the function body. I could use this style here.

Yeah, four-space works here.

Comment 5

[:Paolo Amadini]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> That's reasonable. How about instead of 'x' or 'e', we use 'des'?

Sounds good!

> > +    this._forEachDownloadElementShellForURI(aNode.uri,
> > +                         e => e.placesNodeIconChanged());

By the way, I've just noticed that nodeAnnotationChanged is going away soon and I can just align this with "aNode.uri", so no special considerations for this case.

Comment 6

[:Paolo Amadini]

Attachment: Updated patch

Updated. Thanks for the review!

Note I'll not be able to land this before tomorrow.

Comment 7

[:Paolo Amadini]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6b0c577720f7

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

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bb24f0be1bcb

Comment 9

[Marco Mucci [:MarcoM]]

Hi Paolo, can you provide a point value.