convert mailnews/base/prefs/content/AccountManager.js to Services.jsm and MailServices.js (and fix strict Javascript warnings)

RESOLVED FIXED in Thunderbird 15.0

Status

MailNews Core
Account Manager
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

(Blocks: 3 bugs)

Trunk
Thunderbird 15.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

26.34 KB, patch
aceman
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Convert mailnews/base/prefs/content/AccountManager.js to Services.jsm and MailServices.js .

Also update some comments due to landing of IM in
http://hg.mozilla.org/comm-central/rev/8cdfed92867f

Also fix some new strict Javacript warnings:
Timestamp: 23.03.2012 22:23:22
Warning: function onAccountTreeSelect does not always return a value
Source File: chrome://messenger/content/AccountManager.js
Line: 678, Column: 4
Source Code:
    return; 

Timestamp: 23.03.2012 22:23:22
Warning: package is a reserved identifier
Source File: chrome://messenger/content/AccountManager.js
Line: 747, Column: 8
Source Code:
    var package = pageId.split("am-")[1].split(".xul")[0]; 

Timestamp: 23.03.2012 22:23:22
Warning: redefining package is deprecated
Source File: chrome://messenger/content/AccountManager.js
Line: 747, Column: 8
Source Code:
    var package = pageId.split("am-")[1].split(".xul")[0]; 

Timestamp: 23.03.2012 22:23:22
Warning: function getPageFormElements does not always return a value
Source File: chrome://messenger/content/AccountManager.js
Line: 1042

Timestamp: 23.03.2012 22:32:48
Warning: assignment to undeclared variable panel
Source File: chrome://messenger/content/AccountManager.js
Line: 1191

(Depending on bugs that must land first to not make colliding patches.)
(Assignee)

Updated

5 years ago
Depends on: 739573, 740617
(Assignee)

Comment 1

5 years ago
Warning: ReferenceError: reference to undefined property node._account
Source file: chrome://messenger/content/AccountManager.js
Line: 735

When clicking on the SMTP server in the tree.
(Assignee)

Updated

5 years ago
Blocks: 749200
(Assignee)

Comment 2

5 years ago
To save anybody else the work, I have a patch ready for this, but must wait for bug 739573 to land because they would collide.
(Assignee)

Updated

5 years ago
Summary: convert mailnews/base/prefs/content/AccountManager.js to Services.jsm and MailServices.js → convert mailnews/base/prefs/content/AccountManager.js to Services.jsm and MailServices.js (and fix strict Javascript warnings)
(Assignee)

Updated

5 years ago
Duplicate of this bug: 753471
(Assignee)

Comment 4

5 years ago
Created attachment 623424 [details] [diff] [review]
patch
Attachment #623424 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 5

5 years ago
For reviewers: patch to be applied on top of patch in bug 739573.

Comment 6

5 years ago
Comment on attachment 623424 [details] [diff] [review]
patch

Been running with the patch for a number of days and no issues.
On code inspection...
>+++ b/mailnews/base/prefs/content/am-prefs.js
> function getAccountValueIsLocked(element)
> {
>+  let prefstr = "";
>+  let preftype;
Just define these two within the if statement.
>+  let prefval;
Not used.
> 
>+  let prefstring = element.getAttribute("prefstring");
>   if (prefstring) {
>+    preftype = element.getAttribute("preftype");
>     prefstr = substPrefTokens(prefstring, element);
>     // see if the prefstring is locked
>+    if (prefstr)
>+      return Services.prefs.prefIsLocked(prefstr);
>   }
>   return false;
> }
r=me with those issues addressed
Attachment #623424 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 7

5 years ago
Created attachment 624475 [details] [diff] [review]
patch v2
Attachment #623424 - Attachment is obsolete: true
Attachment #624475 - Flags: review?(mconley)
(Assignee)

Updated

5 years ago
Blocks: 80855
Comment on attachment 624475 [details] [diff] [review]
patch v2

The code looks good, but it looks like it's broken our account Mozmill tests:

SUMMARY-UNEXPECTED-FAIL | test-mail-account-setup-wizard.js | test-mail-account-setup-wizard.js::test_mail_account_setup
  EXCEPTION: amc.window.smtpService is undefined
    at: test-mail-account-setup-wizard.js line 146
       subtest_verify_account test-mail-account-setup-wizard.js 146
       WindowWatcher_notify test-window-helpers.js 365
make: *** [mozmill-one] Error 1


Once you fix that (looks pretty simple), could you push to try to make sure we're all green?
Attachment #624475 - Flags: review?(mconley) → review-
(Assignee)

Comment 9

5 years ago
Created attachment 626167 [details] [diff] [review]
patch v3

Fix test. I can't push to try but I ran that one test myself and it failed before and now it passes.
Attachment #624475 - Attachment is obsolete: true
Attachment #626167 - Flags: review?(mconley)
I've pushed this patch to try. If they return, I'll give my r+.

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=deb08a2ee9d2
(Assignee)

Comment 11

5 years ago
So there is a green "B X Z" at the line with this ID. Is that good?
Comment on attachment 626167 [details] [diff] [review]
patch v3

Try build looks OK - the failures that came up are random oranges that we see periodically.
Attachment #626167 - Flags: review?(mconley) → review+
(Assignee)

Comment 13

5 years ago
Great!
Keywords: checkin-needed
(Assignee)

Comment 14

5 years ago
If somebody checking this in reads the comments too thoroughly, then the part of bug 739573 blocking this was already checked in, so this should be free to check-in. The successful try build should confirm that.
(Assignee)

Updated

5 years ago
Blocks: 755885
(Assignee)

Comment 15

5 years ago
Created attachment 628429 [details] [diff] [review]
patch v4

MPL2 bitrot.
Attachment #626167 - Attachment is obsolete: true
Attachment #628429 - Flags: review+
https://hg.mozilla.org/comm-central/rev/3468a34b21bd
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 15.0
You need to log in before you can comment on or make changes to this bug.