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

RESOLVED FIXED in Firefox 4.0b3

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 4.0b3
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
Created attachment 460019 [details] [diff] [review]
patch

see bug 431909 for details
Attachment #460019 - Flags: review?(mak77)
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
(Assignee)

Comment 2

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

Comment 3

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

s/_blockedCommands/_blockedCommandIds/
(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.
(Assignee)

Comment 5

8 years ago
Created attachment 460025 [details] [diff] [review]
patch v2
Attachment #460019 - Attachment is obsolete: true
Attachment #460025 - Flags: review?(mak77)
Attachment #460019 - Flags: review?(mak77)
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.
Attachment #460025 - Flags: review?(mak77) → review+
oh

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

function (id) should be function(id)
(Assignee)

Comment 8

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

Comment 9

8 years ago
> > 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/.
(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 :\
(Assignee)

Comment 11

8 years ago
(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.
whatever, I can live with that space, at least it has some reason to exist differently from cuddled stuff :p
(Assignee)

Updated

8 years ago
Attachment #460025 - Flags: approval2.0?
Attachment #460025 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 13

8 years ago
http://hg.mozilla.org/mozilla-central/rev/379c41d8de91
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b3
You need to log in before you can comment on or make changes to this bug.