Replace "typeof ... == undefined" checks

RESOLVED FIXED in Firefox 4.0b10

Status

Firefox Graveyard
Panorama
P5
trivial
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: mitcho, Assigned: Martin Cerdeira (mRt))

Tracking

unspecified
Firefox 4.0b10

Details

(Whiteboard: [good first bug][qa-])

Attachments

(1 attachment, 2 obsolete attachments)

As per Dolske's suggestion in bug 602777: we have many instances where we check for something existing (for example in some options) using `typeof ... == 'undefined'`.

This bug is for replacing these with `if ('...' in options)` if it's a property, or `if (foo === undefined)` otherwise.
(Reporter)

Updated

7 years ago
Severity: normal → trivial
Priority: -- → P5
Whiteboard: [good first bug] → [good first bug][qa-]
(Assignee)

Comment 1

7 years ago
Created attachment 494898 [details] [diff] [review]
Multiple files Diff

This is my first bug. =)

This is the correct way to submit a diff for a review?
(Reporter)

Comment 2

7 years ago
(In reply to comment #1)
> Created attachment 494898 [details] [diff] [review]
> Multiple files Diff
> 
> This is my first bug. =)
> 
> This is the correct way to submit a diff for a review?

Martin, thanks for contributing! You'll want to request review of your patch, most likely from Ian. You can do this by entering ian@iangilman.com into the review? field of the attachment. I'll do that for you for this patch, though. ;)

Just a quick comment on the patch, though: looks like some instances were replaced with == (two equals), not ===... I'm pretty sure we want those to be ===.
(Reporter)

Updated

7 years ago
Attachment #494898 - Flags: review?(ian)
(In reply to comment #2)
> Just a quick comment on the patch, though: looks like some instances were
> replaced with == (two equals), not ===... I'm pretty sure we want those to be
> ===.

Only if you want to exclude 'null', which you probably don't want most of the time.
Comment on attachment 494898 [details] [diff] [review]
Multiple files Diff

> function GroupItem(listOfEls, options) {
>-  if (typeof options == 'undefined')
>+  if (options == undefined)
>     options = {};

Looks like you want this here:

if (!options)
  options = {};

>   _stackArrange: function GroupItem__stackArrange(bb, options) {
>     var animate;
>-    if (!options || typeof options.animate == 'undefined')
>+    if (!options || options.animate == undefined)
>       animate = true;
>     else
>       animate = options.animate;
> 
>-    if (typeof options == 'undefined')
>+    if (options == undefined)
>       options = {};

Ditto, and it should be moved to the very top of the function.

>   _resize: function UI__resize(force) {
>-    if (typeof force == "undefined")
>+    if (force == undefined)
>       force = false;

This looks like it should just be dropped.
I agree with Dao's comments. 

I'm confused about why we would change: 

  typeof foo == "undefined"

... to: 

  foo === undefined

Is undefined a new reserved keyword? If not, then it's just a variable, like any other, that happens to be undefined, unless someone has defined it (which, I'll admit, seems unlikely, but still it's pretty flimsy).

Sorry I'm late to the party. Thank you for getting involved, Martin!
(Assignee)

Comment 6

7 years ago
(In reply to comment #2)

> Just a quick comment on the patch, though: looks like some instances were
> replaced with == (two equals), not ===... I'm pretty sure we want those to be
> ===.

Yes, you are right, it must be ===. I'm going to fix it.
Thanks to you all!
=)
(Assignee)

Comment 7

7 years ago
Created attachment 495265 [details] [diff] [review]
2nd diff, following sugestions
Attachment #494898 - Attachment is obsolete: true
Attachment #495265 - Flags: review?(ian)
Attachment #494898 - Flags: review?(ian)
Comment on attachment 495265 [details] [diff] [review]
2nd diff, following sugestions

>@@ -811,17 +811,17 @@ GroupItem.prototype = Utils.extend(new I
>         $el = iQ(a);
>         item = Items.item($el);
>       }
>       Utils.assertThrow(!item.parent || item.parent == this,
>           "shouldn't already be in another groupItem");
> 
>       item.removeTrenches();
> 
>-      if (typeof options == 'undefined')
>+      if (options == undefined)
>         options = {};

if (!options)

>@@ -927,17 +927,17 @@ GroupItem.prototype = Utils.extend(new I
>       if (a.isAnItem) {
>         item = a;
>         $el = iQ(item.container);
>       } else {
>         $el = iQ(a);
>         item = Items.item($el);
>       }
> 
>-      if (typeof options == 'undefined')
>+      if (options == undefined)
>         options = {};

if (!options)

>   _stackArrange: function GroupItem__stackArrange(bb, options) {
>+    if (!options)
>+      options = {};    
>+      

lots of trailing spaces

>     var animate;
>-    if (!options || typeof options.animate == 'undefined')
>+    if (!options || options.animate == undefined)
>       animate = true;
>     else
>       animate = options.animate;

!options won't be true here anymore and you should use |"animate" in options|.
I'd recommend:
var animate = "animate" in options ? options.animate : true;


>   arrange: function Items_arrange(items, bounds, options) {
>-    if (typeof options == 'undefined')
>+    if (options == undefined)
>       options = {};
> 
>     var animate = true;
>-    if (typeof options.animate != 'undefined')
>+    if ("animate" in options)
>       animate = options.animate;

same comments as above
Comment on attachment 495265 [details] [diff] [review]
2nd diff, following sugestions

I agree with Dao's comments; otherwise looks good. Zpao has assured me that "=== undefined" is common practice at Mozilla, so I'm okay with that.
Attachment #495265 - Flags: review?(ian) → review-
(Assignee)

Comment 10

7 years ago
Created attachment 495676 [details] [diff] [review]
3erd diff, following sugestions
Attachment #495265 - Attachment is obsolete: true
Attachment #495676 - Flags: review?(ian)
Comment on attachment 495676 [details] [diff] [review]
3erd diff, following sugestions

Looks great! Thank you for doing this, Martin.
Attachment #495676 - Flags: review?(ian)
Attachment #495676 - Flags: review+
Attachment #495676 - Flags: approval2.0?
Comment on attachment 495676 [details] [diff] [review]
3erd diff, following sugestions

a=beltzner
Attachment #495676 - Flags: approval2.0? → approval2.0+
Martin, can you package this for checkin?
Assignee: nobody → martincerdeira
Status: NEW → ASSIGNED
(Assignee)

Comment 14

7 years ago
(In reply to comment #13)
> Martin, can you package this for checkin?

Yes, sorry for the delay.
(Assignee)

Updated

7 years ago
Keywords: checkin-needed

Comment 15

7 years ago
http://hg.mozilla.org/mozilla-central/rev/a5092e4ae324
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b10
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.