795102 - Use MailServices.js where possible

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: switch to MailServives, patch v1

This is similar to bug 792266 but about the mail specific mailServices.jsm.
See http://mxr.mozilla.org/comm-central/source/mailnews/base/util/mailServices.js for more information.

Please tell me if I chopped off too much.

Comment 1

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 665637 [details] [diff] [review]
switch to MailServives, patch v1

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

r=philipp with feedback from Stefan

::: calendar/base/content/dialogs/calendar-event-dialog-attendees.xml
@@ -290,5 @@
>  
>            // construct the display string from common name and/or email address.
>            var inputValue = aAttendee.commonName;
>            var regexp = new RegExp("^mailto:(.*)", "i");
> -          if (inputValue && this.mHeaderParser) {

I believe this is from times of Sunbird. The header parser isn't available then, the same goes for other similar instances.

Given Sunbrid is deprecated, I think we can get away with this, but please at least ensure Seamonkey has this too. I'll leave the final decision on this to ssitter, he knows more about the lobby of lost Sunbird users :)

@@ +583,2 @@
>  
> +            let abDir=this._findListInAddrBooks(MailServices.ab, names.value[0]);

While you are here, could you add spaces around the = ?

@@ +650,5 @@
>                }
>                if (in_list(attendees, thisId)) continue;
>  
>                if (abCard.displayName.length > 0) {
> +                let ml=this._findListInAddrBooks(MailServices.ab, abCard.displayName);

Same here

Comment 2

[Stefan Sitter [:ssitter]]

Comment on attachment 665637 [details] [diff] [review]
switch to MailServives, patch v1

>+++ b/calendar/base/content/dialogs/calendar-event-dialog-attendees.xml

When and how does the file mailServices.js must be loaded? For example you load it in the constructor but don't use it. In the property getter for "attendees" you don't load it but you use it. In the property getter for "organizer" you load and use it. 

There are two places where MailServices.ab is passed to _findListInAddrBooks(). Maybe the function can be reworked to just use it directly instead of passing it as a parameter.

>+++ b/calendar/itip/calItipEmailTransport.js
...
>-  let accountMgrSvc = Components.classes["@mozilla.org/messenger/account-manager;1"]
>-                                .getService(Components.interfaces.nsIMsgAccountManager);
>   this.mDefaultAccount = accountMgrSvc.defaultAccount;
>   this.mDefaultIdentity = this.mDefaultAccount.defaultIdentity;

accountMgrSvc is still used in the very next line.

I think this should have showed up during your test. At least I hope you do some basic testing to ensure functionality of the places you have changed.

Comment 3

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: switch to MailServives, patch v2

(In reply to Philipp Kewisch [:Fallen] from comment #1)
> ::: calendar/base/content/dialogs/calendar-event-dialog-attendees.xml
> @@ -290,5 @@
> >  
> >            // construct the display string from common name and/or email address.
> >            var inputValue = aAttendee.commonName;
> >            var regexp = new RegExp("^mailto:(.*)", "i");
> > -          if (inputValue && this.mHeaderParser) {
> 
> I believe this is from times of Sunbird. The header parser isn't available
> then, the same goes for other similar instances.
> 
> Given Sunbrid is deprecated, I think we can get away with this, but please
> at least ensure Seamonkey has this too. I'll leave the final decision on
> this to ssitter, he knows more about the lobby of lost Sunbird users :)
SeaMonkey knows nsIMsgHeaderParser (I evaluated the getService in its error console). So it's ssitters decision here.

> @@ +583,2 @@
> >  
> > +            let abDir=this._findListInAddrBooks(MailServices.ab, names.value[0]);
> 
> While you are here, could you add spaces around the = ?
Fixed.

> @@ +650,5 @@
> >                }
> >                if (in_list(attendees, thisId)) continue;
> >  
> >                if (abCard.displayName.length > 0) {
> > +                let ml=this._findListInAddrBooks(MailServices.ab, abCard.displayName);
> 
> Same here
Fixed.



(In reply to Stefan Sitter from comment #2)
> Comment on attachment 665637 [details] [diff] [review]
> switch to MailServives, patch v1
> 
> >+++ b/calendar/base/content/dialogs/calendar-event-dialog-attendees.xml
> 
> When and how does the file mailServices.js must be loaded? For example you
> load it in the constructor but don't use it. In the property getter for
> "attendees" you don't load it but you use it. In the property getter for
> "organizer" you load and use it. 
Sorry, that was my fault and has been fixed.

> There are two places where MailServices.ab is passed to
> _findListInAddrBooks(). Maybe the function can be reworked to just use it
> directly instead of passing it as a parameter.
Fixed.

> >+++ b/calendar/itip/calItipEmailTransport.js
> ...
> >-  let accountMgrSvc = Components.classes["@mozilla.org/messenger/account-manager;1"]
> >-                                .getService(Components.interfaces.nsIMsgAccountManager);
> >   this.mDefaultAccount = accountMgrSvc.defaultAccount;
> >   this.mDefaultIdentity = this.mDefaultAccount.defaultIdentity;
> 
> accountMgrSvc is still used in the very next line.
> 
> I think this should have showed up during your test. At least I hope you do
> some basic testing to ensure functionality of the places you have changed.
Sorry, this has been fixed. Paenglab provided the details how to build and test Lightning on Thunderbird-Try, here are the results: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=e933b21518e7 - the orange in build is due to https://bugzilla.mozilla.org/show_bug.cgi?id=788939 Don't test in Daily 20121008 because of bug 799208.

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Fallen: So if Sunbird builds with MailServices.js, please review it again or check it in.

Comment 5

[Philipp Kewisch [:Fallen][📱📆]]

Pushed to comm-central changeset b49465fd7e1a