581654 - Fix places in browser-places.js that use for each...in to loop over arrays

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

see bug 431909 for details

Comment 1

[Marco Bonardo [:mak]]

Comment on attachment 460019 [details] [diff] [review]
patch

>diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js

>   // list of command elements (by id) to disable when the panel is opened
>-  _blockedCommands: ["cmd_close", "cmd_closeWindow"],
>+  _blockedCommandIds: ["cmd_close", "cmd_closeWindow"],
>+  get _blockedCommands() {
>+    return this._blockedCommandIds.map(function (id) this._element(id), this);
>+  },

this applies _element to all entries even if the loop exits earlierm and reapplies it at each request...
At this point would not be better to delete the getter and replace it with already wrapped _element() values?

>   _blockCommands: function SU__blockCommands() {
>-    for each(var key in this._blockedCommands) {
>-      var elt = this._element(key);
>+    this._blockedCommands.forEach(function (elt) {
>       // make sure not to permanently disable this item (see bug 409155)
>       if (elt.hasAttribute("wasDisabled"))
>-        continue;
>+        return;
>       if (elt.getAttribute("disabled") == "true")
>         elt.setAttribute("wasDisabled", "true");
>       else {
>         elt.setAttribute("wasDisabled", "false");
>         elt.setAttribute("disabled", "true");
>       }

while here please wrap if in braces since else is braced

>-    }
>+    }, this);

the scope doesn't look used, is it just for avoiding future error-prone paths?

>   _restoreCommandsState: function SU__restoreCommandsState() {
>-    for each(var key in this._blockedCommands) {
>-      var elt = this._element(key);
>+    this._blockedCommands.forEach(function (elt) {
>       if (elt.getAttribute("wasDisabled") != "true")
>         elt.removeAttribute("disabled");
>       elt.removeAttribute("wasDisabled");
>-    }
>+    }, this);
>   },

same question for scope

Comment 2

[Dão Gottwald [:dao]]

(In reply to comment #1)
> this applies _element to all entries even if the loop exits earlierm

I'm not sure what this means. Single iteration will be skipped, but not the whole loop.

> and reapplies it at each request...

I'm not sure what this means either. Maybe the same as below?

> At this point would not be better to delete the getter and replace it with
> already wrapped _element() values?

_blockedCommands could be modified at runtime, but maybe this isn't supposed to be supported?

> while here please wrap if in braces since else is braced

will do

> >-    }
> >+    }, this);
> 
> the scope doesn't look used, is it just for avoiding future error-prone paths?

Good catch, it's not needed.

Comment 3

[Dão Gottwald [:dao]]

> _blockedCommands could be modified at runtime, but maybe this isn't supposed to
> be supported?

s/_blockedCommands/_blockedCommandIds/

Comment 4

[Marco Bonardo [:mak]]

(In reply to comment #2)
> (In reply to comment #1)
> > this applies _element to all entries even if the loop exits earlierm
> 
> I'm not sure what this means. Single iteration will be skipped, but not the
> whole loop.


ehr right, nevermind

> > At this point would not be better to delete the getter and replace it with
> > already wrapped _element() values?
> 
> _blockedCommands could be modified at runtime, but maybe this isn't supposed to
> be supported?

Doesn't look like we do that, no reason to support future stuff until it exists, if we'll need to modify it in future we can just change the getter. For now a self deleting getter is probably more efficient.

Comment 5

[Dão Gottwald [:dao]]

Attachment: patch v2

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 460025 [details] [diff] [review]
patch v2

>diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js

>-  // list of command elements (by id) to disable when the panel is opened
>-  _blockedCommands: ["cmd_close", "cmd_closeWindow"],
>+  // array of command elements to disable when the panel is opened

nit: we love proper phrases as comments, uppercase first and period at the end

>+      if (elt.getAttribute("disabled") == "true") {
>         elt.setAttribute("wasDisabled", "true");
>-      else {
>+      } else {

nit: Places style does not want cuddled else, but since global style allows it, I won't mind.

Comment 7

[Marco Bonardo [:mak]]

oh

> ["cmd_close", "cmd_closeWindow"].map(function (id) this._element(id), this);

function (id) should be function(id)

Comment 8

[Dão Gottwald [:dao]]

(In reply to comment #6)
> >-  // list of command elements (by id) to disable when the panel is opened
> >-  _blockedCommands: ["cmd_close", "cmd_closeWindow"],
> >+  // array of command elements to disable when the panel is opened
> 
> nit: we love proper phrases as comments, uppercase first and period at the end

Well a period doesn't seem to make sense here, as there's no full sentence? Or do you want me to phrase a full sentence (which I think wouldn't improve readability)?

(In reply to comment #7)
> oh
> 
> > ["cmd_close", "cmd_closeWindow"].map(function (id) this._element(id), this);
> 
> function (id) should be function(id)

Our code base uses both ways. I agree with the "If a function literal is anonymous..." part from http://javascript.crockford.com/code.html (which is not mandatory at all for us, but makes sense nonetheless).

Comment 9

[Dão Gottwald [:dao]]

> > function (id) should be function(id)
> 
> Our code base uses both ways.

And unless I screwed up on mxr, the former is actually more popular than the latter in /browser/base/.

Comment 10

[Marco Bonardo [:mak]]

(In reply to comment #8)
> Well a period doesn't seem to make sense here, as there's no full sentence? Or
> do you want me to phrase a full sentence (which I think wouldn't improve
> readability)?

it's our code style, period improves readability because mind is used to interrupt on them, and we move from comment to code. btw it's not a blocking issue :)

> > function (id) should be function(id)
> 
> Our code base uses both ways. I agree with the "If a function literal is
> anonymous..." part from http://javascript.crockford.com/code.html (which is not
> mandatory at all for us, but makes sense nonetheless).

there is no indication in any of the internal code style documents about that (https://developer.mozilla.org/En/Developer_Guide/Coding_Style, https://wiki.mozilla.org/Places/Coding_Style, http://mxr.mozilla.org/mozilla-central/source/storage/style.txt), I find confusing having function use the coding style of control structures personally. Matter of personal preferences :\

Comment 11

[Dão Gottwald [:dao]]

(In reply to comment #10)
> it's our code style, period improves readability because mind is used to
> interrupt on them, and we move from comment to code. btw it's not a blocking
> issue :)

Ok, I'll adjust the comment.

> I find confusing having function use the coding style of control structures

On the other hand, 'function' is a language construct, unlike function names.

Comment 12

[Marco Bonardo [:mak]]

whatever, I can live with that space, at least it has some reason to exist differently from cuddled stuff :p

Comment 13

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/379c41d8de91