Review of attachment 9058392 [details] [diff] [review]: ----------------------------------------------------------------- Looks good too! ::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties @@ +74,5 @@ > ## LOCALIZATION NOTE (smtpTempSizeExceeded): argument %s is the Outgoing server (SMTP) response > smtpTempSizeExceeded=The size of the message you are trying to send exceeds a temporary size limit of the server. The message was not sent; try to reduce the message size or wait some time and try again. The server responded: %s. > > +## LOCALIZATION NOTE (smtpClientid): argument %s is the Outgoing server (SMTP) response > +smtpClientid=An error occurred in outgoing SMTP command (CLIENTID): Server responded: %s Possible alternative wording that I suggested: smtpClientid=The outgoing server (SMTP) detected an error in the CLIENTID command. The message was not sent: Server responded: %s @@ +77,5 @@ > +## LOCALIZATION NOTE (smtpClientid): argument %s is the Outgoing server (SMTP) response > +smtpClientid=An error occurred in outgoing SMTP command (CLIENTID): Server responded: %s > + > +## LOCALIZATION NOTE (smtpClientidPermission): argument %s is the Outgoing server (SMTP) response > +smtpClientidPermission=The outgoing (SMTP) server denied access to your device (CLIENTID): Server responded: %s Possible alternative: smtpClientidPermission=The outgoing server (SMTP) response to CLIENTID command indicates that your device is not permitted to send mail: Server responded: %s Comment: With imap, the idea is for CLIENTID problems to just preclude authenication and not call attention to itself. Would the same apply to SMTP? These possible error string seem to call attention to CLIENTID. ::: mailnews/compose/src/nsComposeStrings.h @@ +66,5 @@ > > #define NS_ERROR_ILLEGAL_LOCALPART NS_MSG_GENERATE_FAILURE(12601) > > +#define NS_ERROR_CLIENTID NS_MSG_GENERATE_FAILURE(12610) > +#define NS_ERROR_CLIENTID_PERMISSION NS_MSG_GENERATE_FAILURE(12611) Had to lookup (again) how these evaluate. It appears they produce error codes 0x80553142 and 0x80553143. Is the gap from the last value (12601 or 0x3139) needed? ::: mailnews/compose/src/nsSmtpProtocol.cpp @@ +1031,5 @@ > +{ > + bool flagDefaultError = true; > + switch (m_responseCode / 100) > + { > + case 2: // 2xx code (250) -- success! Can this be other than 250 and be a success? @@ +1064,5 @@ > + flagDefaultError = false; > + break; > + default: > + // Other 5xx CLIENTID error response - use default (this should never happen) > + MOZ_LOG(SMTPLogModule, mozilla::LogLevel::Error, Maybe include function name: MOZ_LOG(SMTPLogModule, mozilla::LogLevel::Error, ("SendClientIDResponse: Unexpected error occurred, server responded: %s\n", m_responseText.get())); @@ +1073,5 @@ > + default: // CLIENTID error response something other than 2xx, 4xx or 5xx > + // unknwon smtp error - use default (this should never happen) > + m_urlErrorState = NS_ERROR_BUT_DONT_SHOW_ALERT; > + MOZ_LOG(SMTPLogModule, mozilla::LogLevel::Error, > + ("Unexpected error occurred, server responded: %s\n", m_responseText.get())); Add function name: MOZ_LOG(SMTPLogModule, mozilla::LogLevel::Error, ("SendClientIDResponse: Unexpected error occurred, server responded: %s\n", m_responseText.get())); @@ +1250,5 @@ > + buffer = "CLIENTID TB-UUID "; > + nsAutoCString clientID; > + nsresult rv = mozilla::Preferences::GetCString("toolkit.telemetry.cachedClientID", clientID); > + if (NS_FAILED(rv) || clientID.IsEmpty()) { > + // should we log or call ExplainErrorDetails here? Didn't notice this comment when testing before. I've never seen a pref fetch fail but who knows. Guess it couldn't hurt to add a MOZ_LOG here. ::: suite/locales/en-US/chrome/mailnews/compose/composeMsgs.properties @@ +76,5 @@ > +## LOCALIZATION NOTE (smtpClientid): argument %s is the Outgoing server (SMTP) response > +smtpClientid=An error occurred in outgoing SMTP command (CLIENTID): Server responded: %s > + > +## LOCALIZATION NOTE (smtpClientidPermission): argument %s is the Outgoing server (SMTP) response > +smtpClientidPermission=The outgoing (SMTP) server denied access to your device (CLIENTID): Server responded: %s Same comments apply here as the mailnews version of this file.
Bug 1532388 Comment 60 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
Review of attachment 9058392 [details] [diff] [review]: ----------------------------------------------------------------- Looks good too! ::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties @@ +74,5 @@ > ## LOCALIZATION NOTE (smtpTempSizeExceeded): argument %s is the Outgoing server (SMTP) response > smtpTempSizeExceeded=The size of the message you are trying to send exceeds a temporary size limit of the server. The message was not sent; try to reduce the message size or wait some time and try again. The server responded: %s. > > +## LOCALIZATION NOTE (smtpClientid): argument %s is the Outgoing server (SMTP) response > +smtpClientid=An error occurred in outgoing SMTP command (CLIENTID): Server responded: %s Possible alternative wording that I suggested: smtpClientid=The outgoing server (SMTP) detected an error in the CLIENTID command. The message was not sent: Server responded: %s @@ +77,5 @@ > +## LOCALIZATION NOTE (smtpClientid): argument %s is the Outgoing server (SMTP) response > +smtpClientid=An error occurred in outgoing SMTP command (CLIENTID): Server responded: %s > + > +## LOCALIZATION NOTE (smtpClientidPermission): argument %s is the Outgoing server (SMTP) response > +smtpClientidPermission=The outgoing (SMTP) server denied access to your device (CLIENTID): Server responded: %s Possible alternative: smtpClientidPermission=The outgoing server (SMTP) response to CLIENTID command indicates that your device is not permitted to send mail: Server responded: %s Comment: With imap, the idea is for CLIENTID problems to just preclude authenication and not call attention to itself. Would the same apply to SMTP? These possible error string seem to call attention to CLIENTID. ::: mailnews/compose/src/nsComposeStrings.h @@ +66,5 @@ > > #define NS_ERROR_ILLEGAL_LOCALPART NS_MSG_GENERATE_FAILURE(12601) > > +#define NS_ERROR_CLIENTID NS_MSG_GENERATE_FAILURE(12610) > +#define NS_ERROR_CLIENTID_PERMISSION NS_MSG_GENERATE_FAILURE(12611) Had to lookup (again) how these evaluate. It appears they produce error codes 0x80553142 and 0x80553143. Is the gap from the last value (12601 or 0x3139) needed? ::: mailnews/compose/src/nsSmtpProtocol.cpp @@ +1031,5 @@ > +{ > + bool flagDefaultError = true; > + switch (m_responseCode / 100) > + { > + case 2: // 2xx code (250) -- success! Can this be other than 250 and be a success? @@ +1064,5 @@ > + flagDefaultError = false; > + break; > + default: > + // Other 5xx CLIENTID error response - use default (this should never happen) > + MOZ_LOG(SMTPLogModule, mozilla::LogLevel::Error, Maybe include function name: MOZ_LOG(SMTPLogModule, mozilla::LogLevel::Error, ("SendClientIDResponse: Unexpected error occurred, server responded: %s\n", m_responseText.get())); @@ +1073,5 @@ > + default: // CLIENTID error response something other than 2xx, 4xx or 5xx > + // unknwon smtp error - use default (this should never happen) > + m_urlErrorState = NS_ERROR_BUT_DONT_SHOW_ALERT; > + MOZ_LOG(SMTPLogModule, mozilla::LogLevel::Error, > + ("Unexpected error occurred, server responded: %s\n", m_responseText.get())); Add function name: MOZ_LOG(SMTPLogModule, mozilla::LogLevel::Error, ("SendClientIDResponse: Unexpected error occurred, server responded: %s\n", m_responseText.get())); @@ +1250,5 @@ > + buffer = "CLIENTID TB-UUID "; > + nsAutoCString clientID; > + nsresult rv = mozilla::Preferences::GetCString("toolkit.telemetry.cachedClientID", clientID); > + if (NS_FAILED(rv) || clientID.IsEmpty()) { > + // should we log or call ExplainErrorDetails here? Didn't notice this comment when testing before. I've never seen a pref fetch fail but who knows. Guess it couldn't hurt to add a MOZ_LOG here. ::: suite/locales/en-US/chrome/mailnews/compose/composeMsgs.properties @@ +76,5 @@ > +## LOCALIZATION NOTE (smtpClientid): argument %s is the Outgoing server (SMTP) response > +smtpClientid=An error occurred in outgoing SMTP command (CLIENTID): Server responded: %s > + > +## LOCALIZATION NOTE (smtpClientidPermission): argument %s is the Outgoing server (SMTP) response > +smtpClientidPermission=The outgoing (SMTP) server denied access to your device (CLIENTID): Server responded: %s Same comments apply here as ::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties