Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

(Blocks 1 bug)

Trunk
Firefox 37
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

5 years ago
Posted patch The patch (obsolete) — Splinter Review
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.
Attachment #8541253 - Flags: review?(jaws)
Assignee

Comment 1

5 years ago
Posted patch The patch (obsolete) — Splinter Review
Hm, maybe including the changes is more informative than an empty patch :-)
Assignee: nobody → paolo.mozmail
Attachment #8541253 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8541253 - Flags: review?(jaws)
Attachment #8541255 - Flags: review?(jaws)
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?
Attachment #8541255 - Flags: review?(jaws) → review+
Assignee

Comment 3

5 years ago
(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.
(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.
Assignee

Comment 5

5 years ago
(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.
Assignee

Comment 6

5 years ago
Posted patch Updated patchSplinter Review
Updated. Thanks for the review!

Note I'll not be able to land this before tomorrow.
Attachment #8541255 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/bb24f0be1bcb
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Iteration: --- → 37.3
Flags: qe-verify-
Flags: firefox-backlog+
Hi Paolo, can you provide a point value.
Flags: needinfo?(paolo.mozmail)
Assignee

Updated

5 years ago
Points: --- → 2
Flags: needinfo?(paolo.mozmail)
You need to log in before you can comment on or make changes to this bug.