60639 - javascript strict warnings in AccountManager.js

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Account Configuration, defect, P3)

Description

[Henrik Gemal]

JavaScript strict warning:
chrome://messenger/content/AccountManager.js line 491: reference to undefined 
property accountValues[type]

JavaScript strict warning:
chrome://messenger/content/AccountManager.js line 495: reference to undefined 
property accountValues[type][slot]

JavaScript strict warning:
chrome://messenger/content/AccountManager.js line 530: reference to undefined 
property accountValues[type][slot]

JavaScript strict warning:
chrome://messenger/content/AccountManager.js line 547: reference to undefined 
property top.frames.contentFrame.onPreInit

JavaScript strict warning:
chrome://messenger/content/AccountManager.js line 639: reference to undefined 
property formElement.defaultChecked

JavaScript strict warning:
chrome://messenger/content/AccountManager.js line 711: reference to undefined 
property formElement.defaultValue

JavaScript strict warning:
chrome://messenger/content/AccountManager.js line 762: reference to undefined 
property accountArray[serverId]

JavaScript strict warning:
chrome://messenger/content/AccountManager.js line 563: reference to undefined pr
operty top.frames.contentFrame.onInit

JavaScript strict warning:
chrome://messenger/content/AccountManager.js line 639: reference to undefined pr
operty formElement.defaultChecked

Comment 1

[(not reading, please use [e-mail] instead)]

mass re-assign of account manager bugs to racham.

Comment 2

[[e-mail] (gone - use [e-mail] instead)]

When you fix this, can someone verify, thanks!

Comment 3

[Henrik Gemal]

adding steps to reproduce:

1) Just start "Mail/News Account Settings"

Comment 4

[rossi]

taking this bug. fix is almost done, but maybe i can only finish it after my
holiday on monday.

Comment 5

[rossi]

now i really reassign it...

Comment 6

[rossi]

Attachment: proposed patch

// hack hack - save the prefs file NOW in case we crash

maybe we should change this too some time...

Comment 7

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 76089 [details] [diff] [review]
proposed patch

>+          else
>+          if (slot == "realUsername") {
>+            oldUser = accountValues[type][slot];
>+            newUser = getFormElementValue(pageElements[i]);
>+            uIndx = i;
>+          }
>+          else
>+          if (slot == "type")
>+            newType = getFormElementValue(pageElements[i]);

The else's really should be on the same line as the if statements.  I know
that's not your doing, but that should probably be cleaned up here as well.


>@@ -762,11 +766,13 @@
>             break;
>           }
>       }
>-      else {
>-      accountValues[type][slot] = source[slot];
>+      else if (slot in source) {
>+        accountValues[type][slot] = source[slot];
>+      } else {
>+        accountValues[type][slot] = null;
>     }
>+  } else { accountValues[type][slot] = null; }
>   }
>-  }

Spacing above could be cleaned up too (again not your fault)

>   var value = accountValues[type][slot];
>   //dump("Array->Form: accountValues[" + type + "][" + slot + "] = " + value + "\n");
>   return value;

>@@ -860,9 +868,14 @@
>     else return null;
>   }
> 
>-  else {
>+  else if ("value" in formElement) {
>     return formElement.value;
>   }
>+  
>+  else {
>+    return null;
>+  }
>+  

Please don't add multiple spaces on a line by themselves.  Actually, those two
empty lines shouldn't even exist.  The else's should be on the line right
underneath the previous brace.


Other than those minor nits, it looks good.  r=caillon

Comment 8

[rossi]

Attachment: cleaned up patch

did the cosmetics...

Comment 9

[David :Bienvenu]

these changes are just enough more than cosmetic that you should get a review
from the module owner, [e-mail].

Comment 10

[rossi]

with cosmetics i only meant the changes from attachment 76089 [details] [diff] [review] to attachment
76178 [details] [diff] [review] caillon reviewed the former... but i mailed racham... hope this makes 1.0 ...

Comment 11

[rossi]

Attachment: the patch

after excessive testing i fixed another line in the file which also resulted in
strict warnings... racham ... please review....

Comment 12

[Mike Shaver (:shaver emeritus)]

Comment on attachment 76541 [details] [diff] [review]
the patch

-      if (formElement.defaultValue)
+      if ("defaultValue" in formElement)

Looks like that file usually avoids spaces inside the parens.  Clean that up,
and get a review from radha, please.

Optimistically marking sr=shaver.

Comment 13

[racham]

Comment on attachment 76541 [details] [diff] [review]
the patch

r=racham. Thanks for the cleanup. Hope you have tested (from your previous
updates looks like you did)  moving across all those AccountManager panels and
trying to modify and saving panel items.

Comment 14

[[e-mail] (gone - use [e-mail] instead)]

F-I-X-E-D on the trunk; I'd _so_ love to see someone with a branch to land this.

Comment 15

[Stephen Walker]

verified fixed on the trunk, great job!

Comment 16

[(not reading, please use [e-mail] instead)]

whoops, part of that last patch caused a bad regression.  see bug #166912

-  if (accountValues[type][slot]== undefined) {
+  if (!(slot in accountValues[type]) || !accountValues[type][slot]) {