Last Comment Bug 795102 - Use MailServices.js where possible
: Use MailServices.js where possible
Status: RESOLVED FIXED
:
Product: Calendar
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- minor (vote)
: 2.2
Assigned To: Sebastian H. [:aryx][:archaeopteryx]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-27 14:02 PDT by Sebastian H. [:aryx][:archaeopteryx]
Modified: 2012-11-28 11:49 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
switch to MailServives, patch v1 (38.04 KB, patch)
2012-09-27 14:02 PDT, Sebastian H. [:aryx][:archaeopteryx]
philipp: review+
Details | Diff | Splinter Review
switch to MailServives, patch v2 (38.74 KB, patch)
2012-10-09 03:56 PDT, Sebastian H. [:aryx][:archaeopteryx]
ssitter: review+
Details | Diff | Splinter Review

Description Sebastian H. [:aryx][:archaeopteryx] 2012-09-27 14:02:45 PDT
Created attachment 665637 [details] [diff] [review]
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] 2012-09-29 10:07:57 PDT
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 2012-10-07 10:39:21 PDT
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 H. [:aryx][:archaeopteryx] 2012-10-09 03:56:47 PDT
Created attachment 669470 [details] [diff] [review]
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 H. [:aryx][:archaeopteryx] 2012-10-19 07:46:11 PDT
Fallen: So if Sunbird builds with MailServices.js, please review it again or check it in.
Comment 5 Philipp Kewisch [:Fallen] 2012-11-28 11:49:40 PST
Pushed to comm-central changeset b49465fd7e1a

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