393986 - Unnecessarily creating arrays using map in page info (should use forEach)

Status: RESOLVED FIXED

(Firefox :: Page Info Window, defect)

Description

[Jeff Walden [:Waldo]]

I see a lot of |list.map(fun)| in Page Info; there's nothing wrong with this if you're using the return value of |map|, but everything here isn't, so an array of |undefined| items is being created for each of these, to hang around until garbage collection destroys it.  There's no real harm, but if |forEach| were used instead of |map| the temporary wouldn't need to be created or GC'd.

Comment 1

[Florian Quèze [:florian]]

Attachment: patch v1

Comment 2

[Asaf Romano (gone)]

Comment on attachment 278615 [details] [diff] [review]
patch v1

>Index: browser/base/content/pageinfo/pageInfo.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/pageinfo/pageInfo.js,v
>retrieving revision 1.7
>diff -U 8 -p -r1.7 pageInfo.js
>--- browser/base/content/pageinfo/pageInfo.js	6 Aug 2007 16:13:05 -0000	1.7
>+++ browser/base/content/pageinfo/pageInfo.js	28 Aug 2007 16:32:48 -0000
>@@ -333,17 +333,17 @@ function loadPageInfo()
> 
>   // and then the hard stuff
>   makeTabs(gDocument, gWindow);
> 
>   initFeedTab();
>   onLoadPermission();
> 
>   /* Call registered overlay init functions */
>-  onLoadRegistry.map(function(func) { func(); });
>+  onLoadRegistry.forEach(function(func) { func(); });

This could be simplified to .forEach(func) in all instances which don't take parameters.


r=mano otherwise.

Comment 3

[Florian Quèze [:florian]]

(In reply to comment #2)
> (From update of attachment 278615 [details] [diff] [review])

> >   /* Call registered overlay init functions */
> >-  onLoadRegistry.map(function(func) { func(); });
> >+  onLoadRegistry.forEach(function(func) { func(); });
> 
> This could be simplified to .forEach(func) in all instances which don't take
> parameters.
> 

How would this work? forEach expects a callback function that it can call for each element of the array.

I've tried it just to be sure it doesn't magically work, and I get this error:
 Error: func is not defined
 Source File: chrome://browser/content/pageinfo/pageInfo.js
 Line: 301

Comment 4

[Daniel Brooks [:db48x]]

he must have just misread it, seeing instead the common 'mistake'.

Comment 5

[Asaf Romano (gone)]

yeah, oops, sorry.

Comment 6

[Nickolay_Ponomarev]

Checking in browser/base/content/pageinfo/pageInfo.js;
/cvsroot/mozilla/browser/base/content/pageinfo/pageInfo.js,v  <--  pageInfo.js
new revision: 1.8; previous revision: 1.7
done