Last Comment Bug 861767 - Use new nsIMsgIncomingServer.protocolInfo attribute where useful
: Use new nsIMsgIncomingServer.protocolInfo attribute where useful
Status: RESOLVED FIXED
[good first bug][mentor=aceman][lang=...
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: Thunderbird 24.0
Assigned To: Suyash Agarwal (:sshagarwal)
:
:
Mentors:
Depends on: 832034 1008835
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-15 01:00 PDT by :aceman
Modified: 2014-05-12 01:59 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (JS) (15.09 KB, patch)
2013-05-12 13:57 PDT, Suyash Agarwal (:sshagarwal)
mkmelin+mozilla: review+
acelists: feedback+
Details | Diff | Splinter Review
Patch (CPP) (4.28 KB, patch)
2013-05-12 13:58 PDT, Suyash Agarwal (:sshagarwal)
acelists: feedback+
Details | Diff | Splinter Review
patch for checkUser (3.41 KB, patch)
2013-05-12 14:02 PDT, :aceman
iann_bugzilla: review+
Details | Diff | Splinter Review
Patch v2(JS) (15.28 KB, patch)
2013-05-14 04:57 PDT, Suyash Agarwal (:sshagarwal)
iann_bugzilla: review+
syshagarwal: review+
acelists: feedback+
Details | Diff | Splinter Review
Patch v2(CPP) (4.10 KB, patch)
2013-05-14 06:52 PDT, Suyash Agarwal (:sshagarwal)
neil: review+
Details | Diff | Splinter Review
Patch v2(CPP) revised (4.28 KB, patch)
2013-05-17 13:24 PDT, Suyash Agarwal (:sshagarwal)
syshagarwal: review+
Details | Diff | Splinter Review
add protocolInfo to im (1.10 KB, patch)
2013-05-19 07:53 PDT, :aceman
florian: review+
Details | Diff | Splinter Review
patch for checkUser v2 (3.47 KB, patch)
2013-05-21 12:31 PDT, :aceman
mkmelin+mozilla: feedback+
Details | Diff | Splinter Review
implement requiresUsername in RSS (961 bytes, patch)
2013-05-30 12:59 PDT, :aceman
neil: review+
Details | Diff | Splinter Review
HTML with JavaScript produces Type Error, and code to bypass it (2.53 KB, text/html)
2013-05-30 15:57 PDT, WADA
no flags Details
implement requiresUsername in RSS v2 [ready for check-in] (965 bytes, patch)
2013-06-01 05:36 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review

Description :aceman 2013-04-15 01:00:41 PDT
Bug 832034 added a new attribute to nsIMsgIncomingServer called protocolInfo that returns the specific nsIMsgProtocolInfo service for the server in question.

Use it where appropriate instead of getting the service directly:

Components.classes["@mozilla.org/messenger/protocol/info;1?type=" + server.type].getService(Components.interfaces.nsIMsgProtocolInfo)

This is mostly useful where we already have the server object. This also makes the code more general as it does not need to hardcode the way how the protocol info service for a server type is retrieved.

This needs to be done in the C++ and also Javascript code so can be split in 2 patches. Or more, depending on the number of places where it is used.
Comment 1 WADA 2013-04-16 02:47:54 PDT
FYI. Dump data of nsIMsgIncomingServer.protocolInfo(Gmail IMAP account)
> CurrentServer.protocolInfo : [xpconnect wrapped nsIMsgProtocolInfo]
>   CurrentServer.protocolInfo / Property
>     canDelete = true
>     canDuplicate = true
>     canGetIncomingMessages = true
>     canGetMessages = true
>     canLoginAtStartUp = true
>     defaultDoBiff = true
>     defaultLocalPath = [xpconnect wrapped nsIFile]
>     foldersCreatedAsync = true
>     preflightPrettyNameWithEmailAddress = true
>     requiresUsername = true
>     serverIID = {ea6a0765-07b8-40df-924c-9004ed707251}
>     showComposeMsgLink = true
>   CurrentServer.protocolInfo / Function
>     QueryInterface = QueryInterface()
>     getDefaultServerPort = getDefaultServerPort()
Comment 2 :aceman 2013-05-10 05:13:39 PDT
So for the javascript part, replace code like this:
(server is a nsIMsgIncomingServer object)
...
protocolInfo = Components
.classes["@mozilla.org/messenger/protocol/info;1?type=" + server.type]
.getService(Components.interfaces.nsIMsgProtocolInfo);
var wantedData = protocolInfo.canGetMessages;

with:

var wantedData = server.protocolInfo.canGetMessages;

--------------------------------------------------
For the C++ part:
If you already have a server like this:
nsCOMPtr<nsIMsgIncomingServer> server;
rv = GetServer(getter_AddRefs(server));

nsCString type;
rv = server->GetType(type);

Then replace:
nsCOMPtr<nsIMsgProtocolInfo> protocolInfo = do_GetService("@mozilla.org/messenger/protocol/info;1?type=" + type, &rv);

with:

nsCOMPtr<nsIMsgProtocolInfo> protocolInfo;
rv = server->GetProtocolInfo(getter_AddRefs(protocolInfo));
NS_ENSURE_SUCCESS(rv, rv);

and it may also be possible to eliminate 'type' variable and the GetType() call.
Comment 3 Randy Thomas 2013-05-11 09:56:53 PDT
I'd like to take this ...
Comment 4 :aceman 2013-05-12 11:58:50 PDT
I am sorry, Suyash is already working on this, he just didn't yet get to mark it here. So I fix it.

Suyash, please attach the patches soon.
Comment 5 Suyash Agarwal (:sshagarwal) 2013-05-12 13:57:45 PDT
Created attachment 748588 [details] [diff] [review]
Patch (JS)

Patch involving the javascript files.
Comment 6 Suyash Agarwal (:sshagarwal) 2013-05-12 13:58:52 PDT
Created attachment 748589 [details] [diff] [review]
Patch (CPP)

Patch involving the c++ files.
Comment 7 :aceman 2013-05-12 14:02:50 PDT
Created attachment 748590 [details] [diff] [review]
patch for checkUser

One small change to functionality in account manager, that is why I add this as a separate patch.
Comment 8 :aceman 2013-05-13 07:33:10 PDT
Try run with all these patches:
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=d13fae67e261

The linux failures also happen without the patches. The WinXP debug crashes are strange.
Comment 9 :aceman 2013-05-13 07:35:47 PDT
Comment on attachment 748588 [details] [diff] [review]
Patch (JS)

I'm happy with this patch.
Comment 10 :aceman 2013-05-13 07:37:14 PDT
Comment on attachment 748589 [details] [diff] [review]
Patch (CPP)

I'm happy with this patch. Thanks Suyash.
Comment 11 Magnus Melin 2013-05-13 11:09:24 PDT
Comment on attachment 748588 [details] [diff] [review]
Patch (JS)

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

r=mkmelin with a couple of nits and such

::: mailnews/base/content/msgAccountCentral.js
@@ +62,5 @@
> +    try {
> +      protocolInfo = server.protocolInfo;
> +    } catch (e) {
> +      exceptions.push (e);
> +    }

What's all this for? (And if valid, nit about the space after push + you don't seem to use the protocolInfo so it doesn't need to be declared at all)

::: mailnews/base/prefs/content/AccountWizard.js
@@ +450,5 @@
> +      try {
> +        IID = srcServer.protocolInfo.serverIID;
> +      } catch (ex) {
> +        Components.utils.reportError("Could not get IID for " + srcServer.type + ": " + ex);
> +        IID = null;

no need to null IID here
Comment 12 neil@parkwaycc.co.uk 2013-05-13 11:17:31 PDT
Comment on attachment 748589 [details] [diff] [review]
Patch (CPP)

>--- a/mailnews/base/src/nsMsgAccountManagerDS.cpp
>+++ b/mailnews/base/src/nsMsgAccountManagerDS.cpp
>   bool canGetMessages = false;
>-  protocolInfo->GetCanGetMessages(&canGetMessages);
>+  rv = protocolInfo->GetCanGetMessages(&canGetMessages);
>+  NS_ENSURE_SUCCESS(rv, false);
Nit: if you rv-check then you don't need to initialise the bool. (2x)

>--- a/mailnews/base/src/nsMsgPurgeService.cpp
>+++ b/mailnews/base/src/nsMsgPurgeService.cpp
>-        nsAutoCString contractid(NS_MSGPROTOCOLINFO_CONTRACTID_PREFIX);
>-        contractid.Append(type);
>-
>-        nsCOMPtr<nsIMsgProtocolInfo> protocolInfo =
>-          do_GetService(contractid.get(), &rv);
>+        nsCOMPtr<nsIMsgProtocolInfo> protocolInfo;
>+        rv = server->GetProtocolInfo(getter_AddRefs(protocolInfo));
protocolInfo is unused so you might as well remove it completely.

>--- a/mailnews/imap/src/nsImapIncomingServer.cpp
>+++ b/mailnews/imap/src/nsImapIncomingServer.cpp
>-  nsCOMPtr <nsIMsgProtocolInfo> protocolInfo = do_GetService("@mozilla.org/messenger/protocol/info;1?type=imap", &rv);
I'm in two minds about this one - the advantage here is that we know the final contract ID already, which makes it more efficient than going through the generic but more readable version.
Comment 13 neil@parkwaycc.co.uk 2013-05-13 11:20:17 PDT
Comment on attachment 748588 [details] [diff] [review]
Patch (JS)

>+    if (server.protocolInfo.canDelete)
>       canDelete = true;
>     else
>       canDelete = server.canDelete;
[Perhaps write this as
   canDelete = server.protocolInfo.canDelete || server.canDelete;
 ?]
Comment 14 Suyash Agarwal (:sshagarwal) 2013-05-14 04:57:03 PDT
Created attachment 749222 [details] [diff] [review]
Patch v2(JS)

Sir,
Thanks for your comments. I have made the changes.
Melin sir, I think protocolInfo is used at lines 76, 83, 90 and few more, so I have left it.
Neil sir, your || operator suggestion was nice thanks.
:aceman sir, please provide the feedback if something needs to be changed.
Carrying over review from mkmelin.
Comment 15 Magnus Melin 2013-05-14 05:18:59 PDT
Ah yes protocolInfo was an argument to the function before. But why the try-catch?
Comment 16 Suyash Agarwal (:sshagarwal) 2013-05-14 05:29:07 PDT
(In reply to Magnus Melin from comment #15)
> Ah yes protocolInfo was an argument to the function before. But why the
> try-catch?
Sir,

It was there before I made these changes so I left it as such. Should I remove it?
Comment 17 :aceman 2013-05-14 05:44:47 PDT
(In reply to Magnus Melin from comment #15)
> But why the try-catch?

Gettting of protocolInfo was in a try catch block previously. Also, the account central is intentionally hardened with try catch blocks where possible against misbehaving servers or undefined attributes. See ArrangeAccountCentralItems. So I would suggest keeping it.
Comment 18 :aceman 2013-05-14 05:58:58 PDT
Comment on attachment 749222 [details] [diff] [review]
Patch v2(JS)

Yes, seems to fix what the reviewers found.
Comment 19 Suyash Agarwal (:sshagarwal) 2013-05-14 06:52:35 PDT
Created attachment 749259 [details] [diff] [review]
Patch v2(CPP)

Sir,

I have made the changes as you suggested.
And for the changes in /mailnews/imap/src/nsImapIncomingServer.cpp,
I have preferred efficiency & added a comment for it.
Comment 20 :aceman 2013-05-14 07:18:20 PDT
Comment on attachment 749259 [details] [diff] [review]
Patch v2(CPP)

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

::: mailnews/imap/src/nsImapIncomingServer.cpp
@@ +2839,1 @@
>    nsCOMPtr <nsIMsgProtocolInfo> protocolInfo = do_GetService("@mozilla.org/messenger/protocol/info;1?type=imap", &rv);

OK, in that case, is it possible to use the NS_IMAPPROTOCOLINFO_CONTRACTID define here? It is defined in nsMsgImapCID.h which is included here...
Comment 21 Magnus Melin 2013-05-14 12:08:21 PDT
(In reply to :aceman from comment #17)
> (In reply to Magnus Melin from comment #15)
> > But why the try-catch?
> 
> Gettting of protocolInfo was in a try catch block previously. Also, the
> account central is intentionally hardened with try catch blocks where
> possible against misbehaving servers or undefined attributes. See
> ArrangeAccountCentralItems. So I would suggest keeping it.

Maybe someone else want to weigh in, but it sure looks bogus to me. Surely 3rd party account types have to make sure there's a protocol info for their type. Otherwise things would blow up all over the place.
Comment 22 :aceman 2013-05-14 12:28:43 PDT
Actually protocolInfo is an attribute of nsMsgIncomingServer. Can we assume all descendent server types inherit it automatically? What about javascript ones, like IM? I probably forgot to define it for IM in bug 832034...
Comment 23 Kent James (:rkent) 2013-05-17 12:15:12 PDT
syshagarwal had a discussion with me on IRC about comment 20. My take on that is that, whenever possible, it would be best to eliminate specific references to the contractID of protocolInfo for a specific type. The case is not as strong in the context of nsImapIncomingServer.cpp as it would be in UI code, bit still I would prefer that instead of seeing a contractID there, that this line:

>    nsCOMPtr <nsIMsgProtocolInfo> protocolInfo = do_GetService("@mozilla.org/messenger/protocol/info;1?type=imap", &rv);

used GetProtocolInfo instead.
Comment 24 Suyash Agarwal (:sshagarwal) 2013-05-17 12:38:26 PDT
Neil sir, 

From comment 12, comment 20, comment 23, I am unable to decide whether to keep 
>nsCOMPtr <nsIMsgProtocolInfo> protocolInfo = do_GetService("@mozilla.org/messenger/protocol/info;1?type=imap", &rv);

as is, or use GetProtocolInfo for this. 
Please help me in deciding.
Comment 25 neil@parkwaycc.co.uk 2013-05-17 12:45:58 PDT
Comment on attachment 749259 [details] [diff] [review]
Patch v2(CPP)

Well, I'm happy with this patch. If between the three of you you want to change nsImapIncomingServer.cpp then just file a separate patch.
Comment 26 Suyash Agarwal (:sshagarwal) 2013-05-17 13:24:31 PDT
Created attachment 751196 [details] [diff] [review]
Patch v2(CPP) revised

I have added the #define NS_IMAPPROTOCOLINFO_CONTRACTID.
Carrying over review from Neil.
Comment 27 Florian Quèze [:florian] [:flo] 2013-05-18 00:48:08 PDT
(In reply to :aceman from comment #22)
> Actually protocolInfo is an attribute of nsMsgIncomingServer. Can we assume
> all descendent server types inherit it automatically? What about javascript
> ones, like IM? I probably forgot to define it for IM in bug 832034...

Indeed, it looks like you wanted to add something in http://mxr.mozilla.org/comm-central/source/mail/components/im/imIncomingServer.js
Comment 28 :aceman 2013-05-19 07:53:59 PDT
Created attachment 751466 [details] [diff] [review]
add protocolInfo to im
Comment 29 Florian Quèze [:florian] [:flo] 2013-05-19 11:13:28 PDT
Comment on attachment 751466 [details] [diff] [review]
add protocolInfo to im

Thanks!
Comment 30 Ian Neal 2013-05-20 16:08:48 PDT
Comment on attachment 748590 [details] [diff] [review]
patch for checkUser

>+++ b/mailnews/base/prefs/content/AccountManager.js
>@@ -486,16 +486,18 @@ function checkUserServerChanges(showAler
>   var accountValues = getValueArrayFor(currentAccount);
>   if (!accountValues)
>     return true;
> 
>   var pageElements = getPageFormElements();
>   if (!pageElements)
>     return true;
> 
>+  let currentServer = currentAccount.incomingServer;
Isn't there certain circumstances where currentAccount is null? (It used to be null checked further down in the code you are replacing)

>+  // There is no username needed for e.g. news so reset it.
>+  if (!currentServer.protocolInfo.requiresUsername) {
Null check currentServer?

>-      if (currentAccount && checkUser) {
>-        filterList = currentAccount.incomingServer.getEditableFilterList(null);
>+      if (checkUser) {
>+        filterList = currentServer.getEditableFilterList(null);
Should the validity of currentServer be checked here?

r=me with those addressed / answered.
Comment 31 :aceman 2013-05-21 12:31:33 PDT
Created attachment 752316 [details] [diff] [review]
patch for checkUser v2

Ok, thanks.
Comment 32 Magnus Melin 2013-05-23 03:28:09 PDT
Comment on attachment 752316 [details] [diff] [review]
patch for checkUser v2

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

I'm not a reviewer for this code, but looks ok to me.
Comment 34 :aceman 2013-05-30 12:59:31 PDT
Created attachment 756155 [details] [diff] [review]
implement requiresUsername in RSS

The patch using protocolInfo.RequiresUsername uncovered that the attribute is unimplemented for RSS server types. That causes an exception (that is hidden) in the account manager now and makes it unable to leave the RSS server settings pane.

So I implement it here to return false. I assume RSS does not use username and there is no field for it in the account manager.
Comment 35 neil@parkwaycc.co.uk 2013-05-30 13:34:52 PDT
(In reply to aceman from comment #34)
> The patch using protocolInfo.RequiresUsername uncovered that the attribute
> is unimplemented for RSS server types. That causes an exception (that is
> hidden) in the account manager now and makes it unable to leave the RSS
> server settings pane.
Which patch and why didn't it cause an exception before?
Comment 36 :aceman 2013-05-30 14:02:35 PDT
Patch https://bugzilla.mozilla.org/attachment.cgi?id=752316 and I don't know why the exception is not visible. Something hides it. When I added try {} catch (e) { reportError(e) } around whole code of oAccountManager.js::nAccountTreeSelect it was shown. The STRs are click the RSS account main settings pane. Then click anything else. It will not change to the new pane as the exception is thrown.
Comment 37 WADA 2013-05-30 15:44:38 PDT
(In reply to :aceman from comment #36)
> Patch https://bugzilla.mozilla.org/attachment.cgi?id=752316 and I don't know
> why the exception is not visible. Something hides it. When I added try {}
> catch (e) { reportError(e) } around whole code of
> oAccountManager.js::nAccountTreeSelect it was shown.

Following code produces "TypeError: currentServer.protocolInfo is undefined", when currentServer.protocolInfo is undefined.
> if (currentServer && !currentServer.protocolInfo.requiresUsername)
In Tb 17, when code like above in add-on produces such error, error is reported to Error Console by Tb.
However, in Tb trunk nightly, error is not reported to Error Console by Tb, and JS code execution silently fails.
So, check of my coding error by trunk is hard in some cases, and I must check with Tb 17 when funny phenomenon happens after code change.

Needless to say, above Type Error due to "referred property is undefined" is bypassed by one of followings.
(A) existence check of higher level property in if
> if (currentServer && currentServer.protocolInfo && !currentServer.protocolInfo.requiresUsername)
(B) try/ctach for part of code
> try
> {
>    if (currentServer && !currentServer.protocolInfo.requiresUsername){ ... }
> } catch(e){ report error, and continue execution }

This case?
Comment 38 WADA 2013-05-30 15:57:13 PDT
Created attachment 756242 [details]
HTML with JavaScript produces Type Error, and code to bypass it
Comment 39 alta88 2013-05-30 16:03:47 PDT
(In reply to :aceman from comment #34)
> Created attachment 756155 [details] [diff] [review]
> implement requiresUsername in RSS
> 
> The patch using protocolInfo.RequiresUsername uncovered that the attribute
> is unimplemented for RSS server types. That causes an exception (that is
> hidden) in the account manager now and makes it unable to leave the RSS
> server settings pane.
> 
> So I implement it here to return false. I assume RSS does not use username
> and there is no field for it in the account manager.

Currently feeds don't know about username.  Though there have been requests for auth capability for https:// sites and internal feeds.
Comment 40 :aceman 2013-05-30 23:43:50 PDT
WADA, currentServer.protocolInfo should not fail, only currentServer.protocolInfo.requiresUsername, and my patch fixes it. What kind of server are you trying and is your TB updated to trunk?

Why exceptions are not shown globally in the Account manager (at least in this area of code) is a separate problem.
Comment 41 WADA 2013-05-31 00:10:54 PDT
(In reply to :aceman from comment #40)
> What kind of server are you trying and is your TB updated to trunk?

That is not actual currentServer.protocolInfo == undefined case. As seen in attached HTML and Javacript, that is simply an example of Type Error in "if(Obj&&!Obj.PropA.PropB)" when Obj.PropA is undefined.

However, "phenomenon of 'currentServer.PropertyX which should not be undefined' is returned as undefined to JavaScript code" is actual phenomenon which occurs on currentServer.trashFolderName, currentServer.isGmail, currentServer.personalNamespace etc.(properties on the nsIImapIncomingServer interface), even when currentServer is actual Gmail IMAP server.
See bug 831664.
Comment 42 neil@parkwaycc.co.uk 2013-05-31 13:36:59 PDT
Comment on attachment 756155 [details] [diff] [review]
implement requiresUsername in RSS

> NS_IMETHODIMP nsRssService::GetRequiresUsername(bool *aRequiresUsername)
> {
>-    return NS_ERROR_NOT_IMPLEMENTED;
>+    NS_ENSURE_ARG_POINTER(aRequiresUsername);
>+
>+    *aRequiresUsername = false;
>+    return NS_OK;
Nit: file style seems to be to not leave a blank line (compare e.g. GetShowComposeMsgLink). r=me with that fixed.
Comment 43 :aceman 2013-06-01 05:36:18 PDT
Created attachment 756963 [details] [diff] [review]
implement requiresUsername in RSS v2 [ready for check-in]

OK, thanks.
Comment 44 Ryan VanderMeulen [:RyanVM] 2013-06-03 05:41:13 PDT
https://hg.mozilla.org/comm-central/rev/7ecbbc9bd30d

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