790438 - Assign names to anonymous functions in Download manager module aboutDownloads.js

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Download Manager, defect)

Description

[Mark Capella [:capella]]

New standards discourage use of anonymously named functions. Please update module aboutDownloads.js.

Refer back to comment 2 in bug 781762.

Comment 1

[Aishwarya]

Hi, should only those anonymous functions created for variables be renamed?

Comment 2

[Aishwarya]

Attachment: Patch

Comment 3

[:Margaret Leibovic]

Comment on attachment 661505 [details] [diff] [review]
Patch

Thanks for working on a patch! Sorry for the slow feedback, I've been traveling.

Unfortunately, the description of this bug wasn't very clear - we don't need to create separate new function, but rather just give names to the existing functions so that they will be easier to debug if they show up in error messages.

So, for example, instead of:

 init: function () {

we should do:

 init: function dl_init() {

and so on. Putting a "dh_" at the front of the function name will be a helpful way to know that this function is coming from download manager code.

And actually, looking more closely at the functions you modified in this patch, it seems like they don't actually need to exist. Instead of

|function (aTarget) { Downloads.removeDownload(aTarget); }|

we should be able to just use |Downloads.removeDownload|.

So, we can break this bug into two patches:
1) Give member functions (init, uninit, observe...) a name
2) Replace anonymous functions in context menu callbacks with direct references to the callback functions

Comment 4

[Aishwarya]

Attachment: Patch - Updated

Comment 5

[:Margaret Leibovic]

Comment on attachment 664051 [details] [diff] [review]
Patch - Updated

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

Let's just focus on changing the function names in this patch. We can replace the anonymous functions in a separate patch.

::: mobile/android/chrome/content/aboutDownloads.js
@@ +35,4 @@
>      .QueryInterface(Ci.nsIDOMChromeWindow));
>  
>  let Downloads = {
> +  init: function dl__init() {

Nit: Just make this dl_init (same goes for similar methods prefixed with "_").

@@ +61,4 @@
>      // Open shown only for downloads that completed successfully
>      Downloads.openMenuItem = contextmenus.add(gStrings.GetStringFromName("downloadAction.open"),
>                                                contextmenus.SelectorContext("li[state='" + this._dlmgr.DOWNLOAD_FINISHED + "']"),
> +                                                                            Downloads.openDownload

This won't actually work correctly because openDownload won't be called in the same context, so the |this| used in it will be wrong. See bug 781762 comment 7 for explanation of a similar situation. Let's just leave these functions alone in this patch to avoid scope creeping.

Comment 6

[Aishwarya]

Attachment: Patch - Latest

Comment 7

[:Margaret Leibovic]

Comment on attachment 664388 [details] [diff] [review]
Patch - Latest

(For future reference, you should check the "patch" checkbox, so that diff/review features will show up with the attachment)

Comment 8

[:Margaret Leibovic]

Comment on attachment 664388 [details] [diff] [review]
Patch - Latest

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

Getting really close! Just a few errors I noticed.

::: mobile/android/chrome/content/aboutDownloads.js
@@ +178,4 @@
>      ].indexOf(parseInt(aState)) != -1;
>    },
>  
> +  _insertDownloadRow: function dv_insertDownloadRow(aDownload) {

Typo? s/dv/dl

@@ +193,4 @@
>      this._list.insertAdjacentHTML("afterbegin", item);
>    },
>  
> +  _getDownloadSize: function dv_getDownloadSize(aSize) {

s/dv/dl

@@ +258,4 @@
>      return gStrings.GetStringFromName(str);
>    },
>  
> +  _updateItem: function _updateItem(aItem, aValues) {

Forgot to add dl prefix?

@@ +367,5 @@
>      let id = parseInt(aElement.getAttribute("downloadID"));
>      return this._dlmgr.getDownload(id);
>    },
>  
> +  _removeItem: function dv_removeItem(aItem) {

s/dv/dl

@@ -367,5 @@
>      let id = parseInt(aElement.getAttribute("downloadID"));
>      return this._dlmgr.getDownload(id);
>    },
>  
> -  _removeItem: function dv__removeItem(aItem) {

Huh, it's weird that this function was named like this.

@@ +456,4 @@
>      }
>    },
>    
> +  _updateDownloadRow: function dv_updateDownloadRow(aItem){

s/dv/dl

Comment 9

[Aishwarya]

Attachment: Patch

Hi,apologies for the errors. I was trying to follow the existing naming conventions which had dv and dv___name. Thanks for the help :)

Comment 10

[:Margaret Leibovic]

Comment on attachment 664722 [details] [diff] [review]
Patch

Thanks so much, this looks good. Sorry for all the churn!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d73da596f3fd

Thanks for the patch! One request - to make life easier for those checking in on your behalf, please make sure that your future patches follow the directions below. Thanks!
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in

Comment 12

[Ed Morley [:emorley]]

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