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.

This is my first bug. =) This is the correct way to submit a diff for a review?

(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 ===.

(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!

(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! =)

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.

Comment on attachment 495676 [details] [diff] [review] 3erd diff, following sugestions Looks great! Thank you for doing this, Martin.

Comment on attachment 495676 [details] [diff] [review] 3erd diff, following sugestions a=beltzner

Martin, can you package this for checkin?

(In reply to comment #13) > Martin, can you package this for checkin? Yes, sorry for the delay.