Last Comment Bug 790438 - Assign names to anonymous functions in Download manager module aboutDownloads.js
: Assign names to anonymous functions in Download manager module aboutDownloads.js
Status: RESOLVED FIXED
[good first bug][lang=js]
:
Product: Firefox for Android
Classification: Client Software
Component: Download Manager (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 18
Assigned To: Aishwarya
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-11 15:07 PDT by Mark Capella [:capella]
Modified: 2012-09-27 04:00 PDT (History)
4 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (4.57 KB, patch)
2012-09-15 09:32 PDT, Aishwarya
margaret.leibovic: feedback-
Details | Diff | Review
Patch - Updated (8.92 KB, patch)
2012-09-24 06:53 PDT, Aishwarya
margaret.leibovic: review-
Details | Diff | Review
Patch - Latest (5.67 KB, patch)
2012-09-25 00:57 PDT, Aishwarya
margaret.leibovic: feedback+
Details | Diff | Review
Patch (5.68 KB, patch)
2012-09-25 17:10 PDT, Aishwarya
margaret.leibovic: review+
Details | Diff | Review

Description Mark Capella [:capella] 2012-09-11 15:07:04 PDT
New standards discourage use of anonymously named functions. Please update module aboutDownloads.js.

Refer back to comment 2 in bug 781762.
Comment 1 Aishwarya 2012-09-15 09:16:37 PDT
Hi, should only those anonymous functions created for variables be renamed?
Comment 2 Aishwarya 2012-09-15 09:32:48 PDT
Created attachment 661505 [details] [diff] [review]
Patch
Comment 3 :Margaret Leibovic 2012-09-19 11:53:20 PDT
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 2012-09-24 06:53:24 PDT
Created attachment 664051 [details] [diff] [review]
Patch - Updated
Comment 5 :Margaret Leibovic 2012-09-24 13:39:18 PDT
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 2012-09-25 00:57:32 PDT
Created attachment 664388 [details] [diff] [review]
Patch - Latest
Comment 7 :Margaret Leibovic 2012-09-25 10:43:09 PDT
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 2012-09-25 10:47:31 PDT
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 2012-09-25 17:10:33 PDT
Created attachment 664722 [details] [diff] [review]
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 2012-09-25 17:28:56 PDT
Comment on attachment 664722 [details] [diff] [review]
Patch

Thanks so much, this looks good. Sorry for all the churn!
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-09-26 15:57:18 PDT
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] 2012-09-27 04:00:40 PDT
https://hg.mozilla.org/mozilla-central/rev/d73da596f3fd

Note You need to log in before you can comment on or make changes to this bug.