615493 - Fix style issues and handling of default values introduced by the modal dialog refactoring

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As given on bug 560820 comment 38:

> >--- a/shared-modules/prefs.js
> >+++ b/shared-modules/prefs.js
> ...
> >-function openPreferencesDialog(callback, launcher) {
> >-  if(!callback)
> >+function openPreferencesDialog(controller, callback, launcher) {
> >+  if(!controller)
> >+    throw new Error("No controller given for Preferences Dialog");
> >+  if(typeof callback != "function")
> >     throw new Error("No callback given for Preferences Dialog");
> 
> We should put a space between if and (). Keeps it from looking like a function
> when you scan the code quickly, and I think standards mention it.

> >--- a/shared-modules/utils.js
> >+++ b/shared-modules/utils.js
> ...
> > function handleWindow(type, text, callback, dontClose) {
> ...
> >+    dontClose = dontClose || false;
> 
> foo OR false always equals foo, this line is a no-op. If you really want to
> assign false as a default, you need to go back to the if === undefined
> syntax...
> 
> >+    if (dontClose == false & window != null) {
> 
> ...but pretty sure you don't need to do it at all, since I believe undefined ==
> false.
>

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #0)
> > We should put a space between if and (). Keeps it from looking like a function
> > when you scan the code quickly, and I think standards mention it.

Will be fixed in the upcoming patch.

> > > function handleWindow(type, text, callback, dontClose) {
> > ...
> > >+    dontClose = dontClose || false;
> > 
> > foo OR false always equals foo, this line is a no-op. If you really want to
> > assign false as a default, you need to go back to the if === undefined
> > syntax...

That's not completely true. Take the following combinations into account:

true = true || false
false = false || false
false = null || false
false = undefined || false

That means each value different from true gets evaluated to false. I do not want to end up with null or undefined later. And beside a value of "false" has to be preserved.

> > >+    if (dontClose == false & window != null) {
> > 
> > ...but pretty sure you don't need to do it at all, since I believe undefined ==
> > false.

Well this line is a pretty mess. The '== false" shouldn't be done. Same as "!= null", as given by the style guide. And why we use a binary '&' operator? It has to be fixed indeed.

Handling with double negative values is kinda hard, so I promote to rename the parameter from "dontClose" to "close" and update a couple of tests accordingly.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1

Comment 3

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 493966 [details] [diff] [review]
Patch v1

I'm r+ing this, but I'm not sure I'm nuts about the if typeof close != 'boolean' method of doing defaults. It's a really non-standard idiom, and doesn't at all communicate "set a default" to me without really thinking about what it's doing. The point of clear code is that you don't have to think about it, you can tell at a glance.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #3)
> I'm r+ing this, but I'm not sure I'm nuts about the if typeof close !=
> 'boolean' method of doing defaults. It's a really non-standard idiom, and
> doesn't at all communicate "set a default" to me without really thinking about
> what it's doing. The point of clear code is that you don't have to think about
> it, you can tell at a glance.

So what would you propose? We have to differentiate between a real "false" and null, or undefined. It's not as simple as when we have false as the default value, where we can do "close = close || false;"

Comment 5

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Heh, which wasn't that simple anyway. You make a good point, but truth is, you didn't really need to do anything (i.e. don't set a default at all).  As it is now, "" (false) would reset to true.

If I did anything at all it'd be check for undefined specifically. Chances are if they supplied anything else, they meant to supply it and use honest-to-gosh Javascript truth rules, not redefined truth rules.

However, I'd settle for us being conscious of the fact that the standard idiom is "foo = foo || default" and to comment any other method as "Setting default" since it'd otherwise be unexpected.

Comment 6

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Actually, correcting myself, I was hasty. You do have to check for undefined, of course. But that's all I would have done.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v2

Fix for the scary default check.

Comment 8

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 494595 [details] [diff] [review]
Patch v2

Looks fine, thanks for the much clearer default setting! r+

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Backport v1 (1.9.2)

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Backport v1 (1.9.1)

Comment 11

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 494602 [details] [diff] [review]
Backport v1 (1.9.2)

Looks fine, r+

Comment 12

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 494603 [details] [diff] [review]
Backport v1 (1.9.1)

Also looks fine, r+

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/2fab5c29b836 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/b5bdf9c20873 (1.9.2)
http://hg.mozilla.org/qa/mozmill-tests/rev/bcdf7c2d49fd (1.9.1)

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: breakage fix for 1.9.2/1.9.1

Not sure why this was missing in the backports for the older branches but it caused a couple of test failures.

Comment 15

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 495661 [details] [diff] [review]
breakage fix for 1.9.2/1.9.1

Looks great, r+

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed on older branches:
http://hg.mozilla.org/qa/mozmill-tests/rev/8c43d42eee2f
http://hg.mozilla.org/qa/mozmill-tests/rev/7d0586b8f148