Closed Bug 861767 Opened 7 years ago Closed 6 years ago

Use new nsIMsgIncomingServer.protocolInfo attribute where useful

Categories

(MailNews Core :: Backend, defect, trivial)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 24.0

People

(Reporter: aceman, Assigned: sshagarwal)

References

Details

(Whiteboard: [good first bug][mentor=aceman][lang=C++ and/or Javascript])

Attachments

(6 files, 5 obsolete files)

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.
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()
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.
I'd like to take this ...
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.
Assignee: nobody → syshagarwal
Status: NEW → ASSIGNED
Attached patch Patch (JS) (obsolete) — Splinter Review
Patch involving the javascript files.
Attached patch Patch (CPP) (obsolete) — Splinter Review
Patch involving the c++ files.
Attached patch patch for checkUser (obsolete) — Splinter Review
One small change to functionality in account manager, that is why I add this as a separate patch.
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.
Attachment #748590 - Flags: review?(iann_bugzilla)
Comment on attachment 748588 [details] [diff] [review]
Patch (JS)

I'm happy with this patch.
Attachment #748588 - Flags: review?(mkmelin+mozilla)
Attachment #748588 - Flags: review?(iann_bugzilla)
Attachment #748588 - Flags: feedback+
Comment on attachment 748589 [details] [diff] [review]
Patch (CPP)

I'm happy with this patch. Thanks Suyash.
Attachment #748589 - Flags: review?(neil)
Attachment #748589 - Flags: feedback+
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
Attachment #748588 - Flags: review?(mkmelin+mozilla) → review+
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 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;
 ?]
Attached patch Patch v2(JS)Splinter Review
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.
Attachment #748588 - Attachment is obsolete: true
Attachment #748588 - Flags: review?(iann_bugzilla)
Attachment #749222 - Flags: review+
Attachment #749222 - Flags: feedback?(acelists)
Attachment #749222 - Flags: review?(iann_bugzilla)
Ah yes protocolInfo was an argument to the function before. But why the try-catch?
(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?
(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 on attachment 749222 [details] [diff] [review]
Patch v2(JS)

Yes, seems to fix what the reviewers found.
Attachment #749222 - Flags: feedback?(acelists) → feedback+
Attached patch Patch v2(CPP) (obsolete) — Splinter Review
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.
Attachment #748589 - Attachment is obsolete: true
Attachment #748589 - Flags: review?(neil)
Attachment #749259 - Flags: review?(neil)
Attachment #749259 - Flags: feedback?(acelists)
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...
(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.
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...
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.
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.
Flags: needinfo?(neil)
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.
Attachment #749259 - Flags: review?(neil) → review+
Flags: needinfo?(neil)
I have added the #define NS_IMAPPROTOCOLINFO_CONTRACTID.
Carrying over review from Neil.
Attachment #749259 - Attachment is obsolete: true
Attachment #749259 - Flags: feedback?(acelists)
Attachment #751196 - Flags: review+
(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
Attachment #751466 - Flags: review?(florian)
Comment on attachment 751466 [details] [diff] [review]
add protocolInfo to im

Thanks!
Attachment #751466 - Flags: review?(florian) → review+
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.
Attachment #748590 - Flags: review?(iann_bugzilla) → review+
Ok, thanks.
Attachment #748590 - Attachment is obsolete: true
Attachment #752316 - Flags: review?(mkmelin+mozilla)
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.
Attachment #752316 - Flags: review?(mkmelin+mozilla) → feedback+
Attachment #749222 - Flags: review?(iann_bugzilla) → review+
Keywords: checkin-needed
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.
Attachment #756155 - Flags: review?(neil)
Attachment #756155 - Flags: feedback?(alta88)
(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?
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.
(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?
Attachment #756242 - Attachment mime type: text/plain → text/html
(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.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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 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.
Attachment #756155 - Flags: review?(neil) → review+
OK, thanks.
Attachment #756155 - Attachment is obsolete: true
Attachment #756155 - Flags: feedback?(alta88)
Attachment #756963 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/7ecbbc9bd30d
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Depends on: 1008835
You need to log in before you can comment on or make changes to this bug.