Multiple Authors
	Description • 2 years ago • Edited

+++ This bug was initially created as a clone of Bug #849540 +++

In bug 849540, a number of issues were left unresolved, mainly because the feature was needed in Thunderbird 38, and desired changes could not be done due to string and interface freezes. This bug will address any remaining post-freeze issues and other disputes that arose during that bug's review.

Looking over the unresolved issues in bug 849540 comment 82, specific issues that I would like to see addressed from that comment are (these are quotes from that bug):

1) ::: mailnews/base/prefs/content/accountcreation/verifyConfig.js
(Diff revision 2)
> +        "chrome://messenger/locale/accountCreationModel.properties")

Please use a specific error message here.

2) ::: mailnews/base/public/msgIOAuth2Module.idl
(Diff revision 2)
> +   * Initialize the OAuth2 parameters from an SMTP server, and return whether or

... Please also write that you expect exactly one of the two functions being called. Also, describe briefly what the functions do, e.g. "Reads the OAuth settings for this server"

3) ::: mailnews/base/public/msgIOAuth2Module.idl
(Diff revision 2)
> +  void connect(in boolean aWithUI, in msgIOAuth2ModuleListener aCallback);

it's unclear at which steps you would call connect() vs. build*()

4) ::: mailnews/compose/src/nsSmtpProtocol.cpp
(Diff revision 2)
> +nsresult nsSmtpProtocol::OnSuccess(const nsACString &aAccessToken)

Rename this function (and OnFailure()). It's specific to OAuth, and should have OAuth in its name

5) ::: mailnews/imap/src/nsImapProtocol.cpp
(Diff revision 2)
> +    NS_ASSERTION(mOAuth2Support,

Funny message, but better would be: "Lack of oauthmodule should have been caught in [code place]"

(end quotes from bug)

There are a number of other comments that were made there that have been previously resolved, or with which I did not agree, or which are not longer relevant since what landed changed considerably from the code when those comments are made. But anyone who wants to advocate specific changes, within the scope of the functionality that landed for Thunderbird 38 (namely, hard-wired OAuth2 information for GMail servers) feel free to advocate for that here.

One remaining controversy is the naming of the new SMTP_SUSPENDED state in nsSmptProtocol.cpp  Given that currently and for the forseeable future that state is only used by OAuth2, perhaps it would be clearer to give it a name specific to AOuth2 ad BenB suggests, but I don't feel strongly about it. The state really means, as currently defined, "state is waiting for callbacks from OAuth2 methods that will reset the state depending on their results."