Last Comment Bug 224831 - Issue warning to restart (due to Bug 2654) when Local Directory setting is changed
: Issue warning to restart (due to Bug 2654) when Local Directory setting is ch...
Status: RESOLVED FIXED
[gs]
: uiwanted, ux-error-prevention
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: Thunderbird 14.0
Assigned To: :aceman
:
Mentors:
http://gsfn.us/t/pdmw
: 229133 (view as bug list)
Depends on: 739931
Blocks: 2654 577775 738810
  Show dependency treegraph
 
Reported: 2003-11-05 14:06 PST by WADA
Modified: 2014-07-15 13:16 PDT (History)
11 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (12.83 KB, patch)
2012-01-17 15:12 PST, :aceman
bwinton: ui‑review+
Details | Diff | Splinter Review
patch v2, updates per comment 10 (13.15 KB, patch)
2012-01-20 15:31 PST, :aceman
iann_bugzilla: review-
Details | Diff | Splinter Review
patch v3, updates per comment 19 (14.49 KB, patch)
2012-01-22 08:38 PST, :aceman
iann_bugzilla: review+
bwinton: ui‑review-
Details | Diff | Splinter Review
patch v4, with restart (15.76 KB, patch)
2012-02-10 11:48 PST, :aceman
bwinton: ui‑review+
Details | Diff | Splinter Review
patch v5 (15.66 KB, patch)
2012-02-10 13:46 PST, :aceman
iann_bugzilla: review+
acelists: ui‑review+
Details | Diff | Splinter Review
patch v6 (15.68 KB, patch)
2012-02-11 16:24 PST, :aceman
standard8: review-
acelists: ui‑review+
Details | Diff | Splinter Review
patch v7 (13.03 KB, patch)
2012-03-26 12:16 PDT, :aceman
standard8: review+
bugzillamozillaorg_serge_20140323: feedback-
Details | Diff | Splinter Review

Description WADA 2003-11-05 14:06:54 PST
User-Agent:       Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.6a) Gecko/20031028
Build Identifier: Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.6a) Gecko/20031028

Not only many bugs are already closed as DUPE of longlived Bug 2654, but also at
least following bugs are currently opened, which seem to be DUPEs of Bug 2654.
  143485 : Copies & Folders not saved
  189212 : Multiple Mail accounts: Won't save "Copies & Folders"
  221386 : Mozilla doesnt save the " Copies and Folders " page in the " Mail and
     
           Newsgroups Account Settings "
  224369 : Copies & Folders settings not saved
In addtion, many duplicated bugs are expected to be opened periodicaly because
many many questions due to Bug 2654 are found in forums of MozillaZine.

If Bug 2654 is not resolved in near future, warning message should be displayed
when Copies & Folder setting is changed in order to force user to restart of
Mozilla.

Dialog example:
 You changed Copies & Folder setting.
 Sorry but Mozilla currently need to be restarted due to Bug 2654.
 Reply OK to restart Mozilla now, or CANCEL to discard your change.



Reproducible: Always

Steps to Reproduce:
Comment 1 Mike Cowperthwaite 2004-04-04 13:41:57 PDT
*** Bug 229133 has been marked as a duplicate of this bug. ***
Comment 2 WADA 2006-08-20 21:05:55 PDT
Changed to "Core/MailNews: Backend", because Bug 2654 is "Core/MailNews: Backend" and because repeatedly opened DUP bugs of Bug 2654 were for Thunderbird.
Please note that Thunderbird case is worse than Mozilla/SeaMonkey case since still no description about Bug 2654 in Release Notes of Thunderbird, even though all Release Notes of Mozilla/SeaMonkey has pointer to Bug 2654.

(Correction of comment #0. "Copies & Folder" should be "Local Directory".)
Dialog example:
 You changed "Local Directory" setting.
 Sorry but Mozilla currently need to be restarted due to Bug 2654.
 Reply OK to restart Mozilla now, or CANCEL to discard your change.
Comment 3 WADA 2010-09-18 08:26:00 PDT
Setting dependency to Bug 2654 for ease of tracking and search, as this bug will be automatically disapperas if Bug 2654 will be fixed, although such Depends on/Blocks: chain(it should be for dependency of patches) is not valid at B.M.O.
Comment 4 :aceman 2012-01-17 03:20:36 PST
Bwinton, what do you think about this?
Comment 5 Blake Winton (:bwinton) (:☕️) 2012-01-17 07:07:49 PST
My gut feeling is that most people don't change this setting, so it's not the highest-priority thing I know of, but if someone wanted to fix it, I wouldn't stop them…
Comment 6 :aceman 2012-01-17 07:25:38 PST
Exactly because of that there could be a warning, because once somebody changes it, not many people would be able to help him what is going on. See the many dupes of bug 2654.
If this field is done in javascript as the other pref panels, I could try to popup a dialog. Is there any other place we show a dialog in the Account manager (so that I could copy it)?
Comment 7 :aceman 2012-01-17 07:29:03 PST
Yes, there is a warning dialog when I try to delete Server name in Account settings.
Comment 8 :aceman 2012-01-17 07:30:52 PST
Heh, and the warning text there is misleading :) It says an account with that servername and username already exists. Another bug.
Comment 9 :aceman 2012-01-17 15:12:21 PST
Created attachment 589320 [details] [diff] [review]
patch

So here is the patch. It does this:
- adds a warning when the Local Directory is changed. The warning is shown whether user click OK directly, or changes to another account (some other warnings are not shown in that case, I don't know why).
- distinguishes between and empty server/user name and a duplicate server+user name
- adds a "&brandshortname;" title consistently to all warning dialogs in checkUserServerChanges function. Sometimes the title was set to null.
Comment 10 Blake Winton (:bwinton) (:☕️) 2012-01-19 09:44:27 PST
Comment on attachment 589320 [details] [diff] [review]
patch

Review of attachment 589320 [details] [diff] [review]:
-----------------------------------------------------------------

Uh, I guess that was more of a Review than a UI-Review, but as it happens the things I'm asking for are fairly small, so ui-r=me with them fixed.  ;)

Thanks,
Blake.

::: mail/locales/en-US/chrome/messenger/prefs.properties
@@ +4,5 @@
>  accountExists=A mail or newsgroup account with the same user name and server name already exists. Click Back and enter a different server name, or click Cancel.
>  modifiedAccountExists=An account with that user name and server name already exists. Please enter a different user name and/or server name.
>  userNameChanged=Your User Name has been updated. You may also need to update your Email Address and/or User Name associated with this account.
>  serverNameChanged=The server name setting has changed. Please verify that any folders used by filters exist on the new server.
> +# LOCALIZATION NOTE (localDirectoryChanged): %1$S is program name (&brandshortname;)

I would prefer this localization note to use "&brandShortName;" instead of "&brandshortname;".

@@ +5,5 @@
>  modifiedAccountExists=An account with that user name and server name already exists. Please enter a different user name and/or server name.
>  userNameChanged=Your User Name has been updated. You may also need to update your Email Address and/or User Name associated with this account.
>  serverNameChanged=The server name setting has changed. Please verify that any folders used by filters exist on the new server.
> +# LOCALIZATION NOTE (localDirectoryChanged): %1$S is program name (&brandshortname;)
> +localDirectoryChanged=The path to Local Directory has changed. You must restart %1$S for this change to take effect and correctly receive messages.

I think this should be either "The path to the Local Directory", or "The Local Directory path".  And I think we want to remove "and correctly receive messages".

@@ -6,2 +6,5 @@
> >  userNameChanged=Your User Name has been updated. You may also need to update your Email Address and/or User Name associated with this account.
> >  serverNameChanged=The server name setting has changed. Please verify that any folders used by filters exist on the new server.
> > +# LOCALIZATION NOTE (localDirectoryChanged): %1$S is program name (&brandshortname;)
> > +localDirectoryChanged=The path to Local Directory has changed. You must restart %1$S for this change to take effect and correctly receive messages.
> > +serverNameEmpty=The server name and user name must not be empty.

This sounds a little strange to me.  How about "Neither the server name nor the user name can be empty."?

::: mailnews/base/prefs/content/AccountManager.js
@@ +259,2 @@
>          Cc["@mozilla.org/embedcomp/prompt-service;1"]
>            .getService(Ci.nsIPromptService).alert(window, alertTitle, alertMsg);

(Along the lines of a comment below, we could change this to Services.prompt, but since you're not touching this code, we don't need to.)

@@ +314,1 @@
>                             .getService(Ci.nsIMsgAccountManager);

I think we can (and should) use mailServices.accounts for this.

@@ +329,2 @@
>        if (showAlert) {
> +        Cc["@mozilla.org/embedcomp/prompt-service;1"]

This seems like another good candidate for Services.prompt.

@@ +361,5 @@
>            serverChangeText = userChangeText;
>  
>        if (serverChangeText != undefined) {
> +          Cc["@mozilla.org/embedcomp/prompt-service;1"]
> +            .getService(Ci.nsIPromptService)

Blah blah blah Services.prompt.  ;)

@@ +373,5 @@
> +  let oldLocalPath = null;
> +  let newLocalPath = null;
> +  for (let i = 0; i < pageElements.length; i++) {
> +    if (pageElements[i].id) {
> +      let vals = pageElements[i].id.split(".");

I think, instead of splitting, then checking whether it's server and localPath, it might be more efficient to say:
if (/^server\.localPath\./.test(pageElements[i].id)
  oldLocalPath = accountValues.server.localPath.path; // both return nsILocalFile
  newLocalPath = getFormElementValue(pageElements[i]).path;
}

(I think that works out, because you know that type is "server" and slot is "localPath".)

@@ +389,5 @@
> +    let brandName = document.getElementById("bundle_brand").getString("brandShortName");
> +    let alertText = document.getElementById("bundle_prefs")
> +                            .getFormattedString("localDirectoryChanged", [brandName]);
> +    Cc["@mozilla.org/embedcomp/prompt-service;1"]
> +      .getService(Ci.nsIPromptService)

Services.prompt.
Comment 11 :aceman 2012-01-19 11:26:02 PST
Thanks for the review, I'll fix it. But there is exactly one usage of Services.prompt in /mail and 0 in /mailnews (there are many in /suite). Are you sure I should use it? I would be out of place compared to the rest of the codebase.
Comment 12 Jim Porter (:squib) 2012-01-19 11:36:00 PST
(In reply to :aceman from comment #11)
> Thanks for the review, I'll fix it. But there is exactly one usage of
> Services.prompt in /mail and 0 in /mailnews (there are many in /suite). Are
> you sure I should use it? I would be out of place compared to the rest of
> the codebase.

I say definitely use it. Most of the code that uses prompts predates Services.jsm, so that's why they're not using it. (For bonus points, a followup bug to switch everything to use Services.prompts would be nice.)
Comment 13 :aceman 2012-01-19 12:10:01 PST
And what is the advantage of this approach? Speed, memory, shorter code?
Comment 14 Jim Porter (:squib) 2012-01-19 12:10:54 PST
Code cleanliness mostly, but I wouldn't be surprised if it were a tiny bit faster too.
Comment 15 :aceman 2012-01-19 13:22:33 PST
But the js files now need to 'Components.utils.import("resource://gre/modules/Services.jsm");'. Doesn't that add some overhead? Or is that module always loaded (by other components) so it is shared?
Comment 16 Mark Banner (:standard8) 2012-01-19 13:27:16 PST
(In reply to :aceman from comment #15)
> But the js files now need to
> 'Components.utils.import("resource://gre/modules/Services.jsm");'. Doesn't
> that add some overhead? Or is that module always loaded (by other
> components) so it is shared?

Modules are shared so although it is listed, it won't be re-loaded. The other advantage is because the service and its values are then cached in js, we don't have to keep crossing the js/c++ boundary to get the service each time.
Comment 17 :aceman 2012-01-19 14:21:51 PST
I have tried the Services.prompt method in bug 719456 and it seems to work.

So, assign me to that global convert bug :)
Comment 18 :aceman 2012-01-20 15:31:37 PST
Created attachment 590371 [details] [diff] [review]
patch v2, updates per comment 10
Comment 19 Ian Neal 2012-01-21 09:58:22 PST
Comment on attachment 590371 [details] [diff] [review]
patch v2, updates per comment 10

> // Check if the user and/or host names have been changed and
> // if so check if the new names already exists for an account.
Nit: This comment will need updating to reflect additional functionality.

> function checkUserServerChanges(showAlert) {
>   const Cc = Components.classes;
>   const Ci = Components.interfaces;
>+  var alertTitle = document.getElementById("bundle_brand")
>+                           .getString("brandShortName");
Maybe better to have this variable called something like shortName.
You also define:
var bundle = document.getElementById("bundle_prefs");
here and use it throughout the function

>   if (smtpService.defaultServer) {
>     try {
>       var smtpHostName = top.frames["contentFrame"].document.getElementById("smtp.hostname");
>       if (smtpHostName && hostnameIsIllegal(smtpHostName.value)) {
>         var alertMsg = document.getElementById("bundle_prefs")
You can use bundle here instead.

>                                .getString("enterValidHostname");
>         Cc["@mozilla.org/embedcomp/prompt-service;1"]
>           .getService(Ci.nsIPromptService).alert(window, alertTitle, alertMsg);
You changed the other prompt service and not this one :(

>+  var checkUser = true;
>   // There is no username defined for news so reset it.
>+  if (newType == "nntp") {
>     oldUser = newUser = "";
>+    checkUser = false;
>+  }

Define the variable alertText at the start of the if rather defining it several times throughout the function.
>   // If something is changed then check if the new user/host already exists.
>   if ( (oldUser != newUser) || (oldHost != newHost) ) {
>+    let rollback = false;
>+    if ((checkUser && (newUser == "")) || (newHost == "")) {
>+        rollback = true;
>         var alertText = document.getElementById("bundle_prefs")
>+                                .getString("serverNameEmpty");
No need for var here now and again you can also now use bundle here instead.
Nit: indentation
>+    } else {
>+      let newServer = MailServices.accounts.findRealServer(newUser, newHost, newType, 0);
>+      if (newServer) {
You could inline and remove the need for defining newServer.

>+          rollback = true;
>+          var alertText = document.getElementById("bundle_prefs")
>+                                  .getString("modifiedAccountExists");
No need for var here now and again you can also now use bundle here instead.
Nit: indentation
>       }
>+    }
>+    if (rollback) {
You don't actually need to define and use the variable rollback, you could just check that alertText is not null.
>+      if (showAlert)
>+        Services.prompt.alert(window, alertTitle, alertText);
>       // Restore the old values before return
>       if (newType != "nntp")
Just test checkUser instead.
>         setFormElementValue(pageElements[uIndx], oldUser);
>       setFormElementValue(pageElements[hIndx], oldHost);
>       return false;
>     }
> 
>     // If username is changed remind users to change Your Name and Email Address.
>-    // If serve name is changed and has defined filters then remind users to edit rules.
>+    // If server name is changed and has defined filters then remind users to edit rules.
>     if (showAlert) {
>-      var account = currentAccount
>+      var account = currentAccount;
>       var filterList;
>       if (account && (newType != "nntp")) {
Test checkUser rather than newType
>         var server = account.incomingServer;
>         filterList = server.getEditableFilterList(null);
>       }
>       var userChangeText, serverChangeText;
>       var bundle = document.getElementById("bundle_prefs");
This was defined at the beginning of the function now.

>+  if (oldLocalPath && newLocalPath && (oldLocalPath != newLocalPath)) {
>+    let brandName = document.getElementById("bundle_brand").getString("brandShortName");
We already have this from the beginning of the function either as alertTitle or shortName.

>+    let alertText = document.getElementById("bundle_prefs")
>+                            .getFormattedString("localDirectoryChanged", [brandName]);
No need for the let here.

Hope that all makes sense, r- for the moment.
Comment 20 :aceman 2012-01-21 10:16:28 PST
(In reply to Ian Neal from comment #19)
> Comment on attachment 590371 [details] [diff] [review]

I have intentionally not equated alertText with brandname into a single variable. They contain the same text now, but later the alertText could contain something better then generic program name.

Maybe I should do it already, make alertText = "&brandShortName; Account settings" or something like that.

In the other things I agree with you, thanks for review.
Comment 21 :aceman 2012-01-22 05:53:34 PST
I mean alertTitle.
Comment 22 :aceman 2012-01-22 08:38:58 PST
Created attachment 590568 [details] [diff] [review]
patch v3, updates per comment 19

Patch updated with all the nits. Also alertTitle now has better text not equal brandname, I used the existing string of pane name ("Server settings"). Also Cc and Ci were not needed anymore.
Comment 23 Ian Neal 2012-01-23 14:26:53 PST
Comment on attachment 590568 [details] [diff] [review]
patch v3, updates per comment 19

>+  const prefBundle = document.getElementById("bundle_prefs");
>+  const alertTitle = prefBundle.getString("prefPanel-server");
Not 100% sure we can use const for both of these, so maybe worth double checking with standard8 or neil.

>+  var alertText = null;
>   if (smtpService.defaultServer) {
>     try {
>       var smtpHostName = top.frames["contentFrame"].document.getElementById("smtp.hostname");
>       if (smtpHostName && hostnameIsIllegal(smtpHostName.value)) {
>+        alertText = prefBundle.getString("enterValidHostname");
>+        Services.prompt.alert(window, alertTitle, alertText);
You could inline the final argument and remove the need to use alertText variable, then you would not need to reset it to null. Alternatively stick to using alertMsg variable.
Also we do not seem to check if any other hostname is legal but you might want to do that in a separate bug.
Plus we currently have two copies of the hostnameIsIllegal function (one in amUtils.js and one in aw-server.js but I think only the former is now used as AccountWizard.xul loads both scripts), the function needs some work to make it actually do better detection of illegal hostnames - probably best done in a separate bug (bug 80855 perhaps)

r=me with the first issue checked and comments on where you think the illegal hostname work should be done.
Comment 24 :aceman 2012-01-23 15:46:11 PST
I do not touch the logic of checking the legality of the server. So I do not 
currently know how it works.

I'll take bug 80855 and try to fix the duplicates there.

This patch is already far away from the original problem :)
Comment 25 Blake Winton (:bwinton) (:☕️) 2012-02-03 12:04:26 PST
Comment on attachment 590568 [details] [diff] [review]
patch v3, updates per comment 19

I think I have to mark this ui-r-, but mostly on a technicality and request.

First the request.  I think we should give the user the option of restarting now or later in that warning dialog.  Would you be up for a patch that implements that?

And next the technicality.  When I saw the dialog box, I thought clicking "OK" _would_ restart Thunderbird, and was alarmed by the lack of a "I'll do it later!" button.  I think you can fix this with better wording.  (Maybe say something about how they will have to restart Thunderbird themselves, and we're not doing it now?  I dunno.  If you post some choices, I'll pick my favourite, and then give you a ui-r for that.)

Thanks,
Blake.
Comment 26 :aceman 2012-02-04 06:33:24 PST
I could add the restart if I find out how to call a restart of TB.
Comment 27 Magnus Melin 2012-02-04 12:39:30 PST
Why would you like to give the user the option not to restart now as it will mess things up? Just let it be "restart" or "cancel the whole change".
Comment 28 :aceman 2012-02-07 11:28:53 PST
Ok, I have found out how to do a restart.
So can anybody decide what to do?

The Local Directory path has changed. TB needs to restart now for this change to take effect.

[Restart] [Restart later]
Comment 29 Mark Banner (:standard8) 2012-02-07 15:31:11 PST
Comment on attachment 590568 [details] [diff] [review]
patch v3, updates per comment 19

Cancelling review until we've got a ui-review+
Comment 30 Magnus Melin 2012-02-09 00:56:47 PST
(In reply to :aceman from comment #28)
> The Local Directory path has changed. TB needs to restart now for this
> change to take effect.
> 
> [Restart] [Restart later]

The problem is it takes affect, but doesn't work properly until restart, right?
Comment 31 :aceman 2012-02-09 01:09:13 PST
Yes. I immediately takes effect only partly (for some code paths). More in bug 2654.

Do you suggest better wording?
Comment 32 Magnus Melin 2012-02-09 01:29:13 PST
I'd suggest just letting the user chose to restart or cancel, as i don't thing the UI should let the user shoot himself in the foot. Ever so often programs claim they need to restart to work properly, and often it's not really necessary. As it's really necessary here, there should be no choice.

 [Apply & Restart] [Cancel]
Comment 33 :aceman 2012-02-09 01:32:51 PST
Ah, you mean when the user uses "Cancel" then revert the change in the field (in the same way invalid hostname/username are silently reverted)?
Comment 34 Magnus Melin 2012-02-09 01:39:07 PST
Yes.
Comment 35 :aceman 2012-02-09 01:56:13 PST
Thanks, will do.
Comment 36 :aceman 2012-02-09 13:20:42 PST
So what about this:

The Local Directory path has changed. Would you like to restart %1$S now for this change to take effect?

[Yes] [Revert]

This would reuse standard buttons and strings. Or do you really want [Apply & Restart] ?
Comment 37 Magnus Melin 2012-02-09 22:25:31 PST
It's best to use verbs for buttons, not yes/no type of buttons.

How about

%1$S needs to restart to apply the change to the Local Directory.

 [Restart] [Cancel]
Comment 38 :aceman 2012-02-10 00:28:50 PST
I wanted to use some of the standard buttons as defined here:
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/prompts/src/nsPrompter.js#173
But OK, I can add a new "Restart" string.
Comment 39 :aceman 2012-02-10 06:55:41 PST
Bwinton, there are now several proposals in the comments here. You can choose now.

The rest of the code is prepared, with restart or reverting the value in case of "cancel".
Comment 40 Blake Winton (:bwinton) (:☕️) 2012-02-10 07:07:56 PST
Cool.  I think I agree with Magnus' last proposal of Restart and Cancel.
And since that was my main request, I'm going to give it a ui-r=me, sight-unseen.  ;)
Comment 41 :aceman 2012-02-10 10:04:39 PST
Magnus, are you authorized to give reviews in this part? Or should I try standard8 again? :)
Comment 42 :aceman 2012-02-10 11:48:12 PST
Created attachment 596129 [details] [diff] [review]
patch v4, with restart
Comment 43 Blake Winton (:bwinton) (:☕️) 2012-02-10 12:31:02 PST
Comment on attachment 596129 [details] [diff] [review]
patch v4, with restart

Review of attachment 596129 [details] [diff] [review]:
-----------------------------------------------------------------

You didn't _really_ need to ask me for a ui-review, but since you did, I like it.

Of course, I did go through the code, and found the following thing you probably want to change…

::: mailnews/base/prefs/content/AccountManager.js
@@ +257,5 @@
> +                                                      "restart");
> +  if (!cancelQuit.data)
> +    Components.classes["@mozilla.org/toolkit/app-startup;1"]
> +              .getService(nsIAppStartup)
> +              .quit(nsIAppStartup.eRestart | nsIAppStartup.eAttemptQuit);

Services.startup.quit, naturally…  ;)
Comment 44 :aceman 2012-02-10 13:46:18 PST
Created attachment 596164 [details] [diff] [review]
patch v5

Thanks, must have overlooked that one in Services.jsm.
Comment 45 Ian Neal 2012-02-11 16:10:07 PST
Comment on attachment 596164 [details] [diff] [review]
patch v5

>+localDirectoryChanged=%1$S needs to restart now to apply the change to the Local directory.
This doesn't sound quite correct, I think it needs to be:
"1$S now needs to restart to apply the change to the Local directory setting."

r=me with that fixed/addressed
Comment 46 :aceman 2012-02-11 16:24:37 PST
Created attachment 596401 [details] [diff] [review]
patch v6

Sure.
Comment 47 Magnus Melin 2012-02-19 10:38:39 PST
Comment on attachment 596401 [details] [diff] [review]
patch v6

I'm only a reviewer for mail/
Comment 48 Mark Banner (:standard8) 2012-03-26 01:47:53 PDT
Comment on attachment 596401 [details] [diff] [review]
patch v6

Review of attachment 596401 [details] [diff] [review]:
-----------------------------------------------------------------

Generally seems fine, however it has suffered some bitrot. I tried fixing it locally and then tried testing this on a pop3 account and on local folders, but when I selected a different folder I got:

Error: currentServer.localPath is null
Source File: chrome://messenger/content/amUtils.js
Line: 73

::: mailnews/base/prefs/content/AccountManager.js
@@ +248,4 @@
>    return true;
>  }
>  
> +function restartProgram() {

I think you could just change this to be Application.restart() (see extApplication.js)
Comment 49 :aceman 2012-03-26 12:16:20 PDT
Created attachment 609414 [details] [diff] [review]
patch v7

Fixes the bitrot and uses Application.restart();
It seems I am the only one changing amUtils.js recently:
http://hg.mozilla.org/comm-central/log/fef65eed3690/mailnews/base/prefs/content/amUtils.js and I didn't touch currentServer.localPath .
I suspect it is a problem from the IM landing. After I deleted my IM account (of type JS test) the error vanished and I could OK the Account manager (when it restarted).
Comment 50 Mark Banner (:standard8) 2012-03-27 02:58:16 PDT
(In reply to :aceman from comment #49)
> I suspect it is a problem from the IM landing. After I deleted my IM account
> (of type JS test) the error vanished and I could OK the Account manager
> (when it restarted).

Thanks, I filed bug 739555.
Comment 51 :aceman 2012-03-27 03:01:02 PDT
(In reply to :aceman from comment #49)
> After I deleted my IM account
> (of type JS test) the error vanished and I could OK the Account > manager (when it restarted).
I mean "and THEN it restarted TB".
Comment 52 Ryan VanderMeulen [:RyanVM] 2012-03-27 16:03:27 PDT
http://hg.mozilla.org/comm-central/rev/b08d54e349a4

(And let me just say that I love your patches. So painless to check in!)
Comment 53 Serge Gautherie (:sgautherie) 2012-03-28 01:43:47 PDT
Comment on attachment 609414 [details] [diff] [review]
patch v7

Review of attachment 609414 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/prefs/content/AccountManager.js
@@ +61,5 @@
>   */
>  
>  Components.utils.import("resource:///modules/iteratorUtils.jsm");
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +Components.utils.import("resource://gre/modules/mailServices.js");

mailServices.js is not in gre.
Comment 54 :aceman 2012-03-28 04:02:11 PDT
Thanks, filed separately as there is one other occurrence of it.
Strange that it works regardless.
Comment 55 Serge Gautherie (:sgautherie) 2012-03-28 04:15:48 PDT
(In reply to :aceman from comment #54)
> Thanks, filed separately as there is one other occurrence of it.

The other was already filed. Seems like overkill, but anyway.

Note You need to log in before you can comment on or make changes to this bug.