Closed
Bug 861767
Opened 11 years ago
Closed 11 years ago
Use new nsIMsgIncomingServer.protocolInfo attribute where useful
Categories
(MailNews Core :: Backend, defect)
MailNews Core
Backend
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)
15.28 KB,
patch
|
iannbugzilla
:
review+
sshagarwal
:
review+
aceman
:
feedback+
|
Details | Diff | Splinter Review |
4.28 KB,
patch
|
sshagarwal
:
review+
|
Details | Diff | Splinter Review |
1.10 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
3.47 KB,
patch
|
mkmelin
:
feedback+
|
Details | Diff | Splinter Review |
2.53 KB,
text/html
|
Details | |
965 bytes,
patch
|
aceman
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
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.
Comment 3•11 years ago
|
||
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
Assignee | ||
Comment 5•11 years ago
|
||
Patch involving the javascript files.
Assignee | ||
Comment 6•11 years ago
|
||
Patch involving the c++ files.
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+
Reporter | ||
Comment 10•11 years ago
|
||
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 11•11 years ago
|
||
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 12•11 years ago
|
||
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•11 years ago
|
||
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; ?]
Assignee | ||
Comment 14•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #749222 -
Flags: review?(iann_bugzilla)
Comment 15•11 years ago
|
||
Ah yes protocolInfo was an argument to the function before. But why the try-catch?
Assignee | ||
Comment 16•11 years ago
|
||
(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?
Reporter | ||
Comment 17•11 years ago
|
||
(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.
Reporter | ||
Comment 18•11 years ago
|
||
Comment on attachment 749222 [details] [diff] [review] Patch v2(JS) Yes, seems to fix what the reviewers found.
Attachment #749222 -
Flags: feedback?(acelists) → feedback+
Assignee | ||
Comment 19•11 years ago
|
||
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)
Reporter | ||
Comment 20•11 years ago
|
||
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•11 years ago
|
||
(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.
Reporter | ||
Comment 22•11 years ago
|
||
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•11 years ago
|
||
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.
Assignee | ||
Comment 24•11 years ago
|
||
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 25•11 years ago
|
||
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)
Assignee | ||
Comment 26•11 years ago
|
||
I have added the #define NS_IMAPPROTOCOLINFO_CONTRACTID. Carrying over review from Neil.
Attachment #749259 -
Attachment is obsolete: true
Attachment #749259 -
Flags: feedback?(acelists)
Assignee | ||
Updated•11 years ago
|
Attachment #751196 -
Flags: review+
Comment 27•11 years ago
|
||
(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
Reporter | ||
Comment 28•11 years ago
|
||
Attachment #751466 -
Flags: review?(florian)
Comment 29•11 years ago
|
||
Comment on attachment 751466 [details] [diff] [review] add protocolInfo to im Thanks!
Attachment #751466 -
Flags: review?(florian) → review+
Comment 30•11 years ago
|
||
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+
Reporter | ||
Comment 31•11 years ago
|
||
Ok, thanks.
Attachment #748590 -
Attachment is obsolete: true
Attachment #752316 -
Flags: review?(mkmelin+mozilla)
Comment 32•11 years ago
|
||
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+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 33•11 years ago
|
||
https://hg.mozilla.org/comm-central/rev/34e7ff6c48e6 https://hg.mozilla.org/comm-central/rev/bffe2188e9e0 https://hg.mozilla.org/comm-central/rev/4a0ad377d047 https://hg.mozilla.org/comm-central/rev/b63bf044ddcc
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 24.0
Reporter | ||
Comment 34•11 years ago
|
||
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)
Comment 35•11 years ago
|
||
(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?
Reporter | ||
Comment 36•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
Updated•11 years ago
|
Attachment #756242 -
Attachment mime type: text/plain → text/html
Comment 39•11 years ago
|
||
(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.
Reporter | ||
Comment 40•11 years ago
|
||
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 → ---
Comment 41•11 years ago
|
||
(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•11 years ago
|
||
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+
Reporter | ||
Comment 43•11 years ago
|
||
OK, thanks.
Attachment #756155 -
Attachment is obsolete: true
Attachment #756155 -
Flags: feedback?(alta88)
Attachment #756963 -
Flags: review+
Keywords: checkin-needed
Comment 44•11 years ago
|
||
https://hg.mozilla.org/comm-central/rev/7ecbbc9bd30d
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•