Currently, Gmail server is identified by presence of \AllMail special folder returned by XLIST command https://mxr.mozilla.org/comm-central/ident?i=kImapAllMail&tree=comm-central&filter=nsImapServerResponseParser https://mxr.mozilla.org/comm-central/ident?i=SetIsGMailServer&tree=comm-central&filter=nsImapIncomingServer This should be changed for two reasons: 1. Gmail may drop XLIST in favor of LIST-EXTENDED extension in the future, which would break the current implementation. 2. As of bug 721316, we are using features of X-GM-EXT-1, so it makes sense that the isGmail function/attribute should come from this capability.

This is followup bug to item 7 on https://bugzilla.mozilla.org/show_bug.cgi?id=721316#c60

(In reply to David Lechner (:dlech) from comment #0) > 1. Gmail may drop XLIST in favor of LIST-EXTENDED extension in the future, More specifically, RFC 6154

So you are suggesting that we should use X-GM-EXT-1 instead of XLIST(All Mail). And we should also clear the pref if we are not getting X-GM-EXT-1? Also, can you point to some code on c-c?

Confirming the bug as we are sure we want to do this. Otherwise situations like Bug 815087 can arise.

(In reply to Atul Jangra [:atuljangra] from comment #3) > So you are suggesting that we should use X-GM-EXT-1 instead of XLIST(All > Mail). Yes. > And we should also clear the pref if we are not getting X-GM-EXT-1? > Also, can you point to some code on c-c? Delete this: https://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapIncomingServer.cpp#1188 Add X-GM-EXT1 capability here: https://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapCore.h#147 Make an ServerIsGmailServerthat looks like ServerIsAOLServer here: https://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapServerResponseParser.h#97 Use GetServerStateParser().ServerIsGmailServer() instead of m_isGmailServer here: https://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapProtocol.cpp#3452 And there is still the business of calling setIsGMailServer() somewhere.

Few changes that uses X-GM-EXT-1 as a check for gmail server instead of X-LIST. Passes the tests locally. Tests specific to gmail: test_gmailAttributes.js and test_gmailMsgOfflineStore.js Will push to try as soon as possible, or if someone else can push in the meantime then that would be awesome too. :-)

I believe that this will also solve the problem reported in Bug 815087.

Comment on attachment 695059 [details] [diff] [review] Pretty much does the job Review of attachment 695059 [details] [diff] [review]: ----------------------------------------------------------------- Just being nosy :) ::: mailnews/imap/src/nsImapProtocol.cpp @@ +675,5 @@ > nsCOMPtr<nsIImapIncomingServer> imapServer = do_QueryInterface(server); > nsCOMPtr<nsIStreamListener> aRealStreamListener = do_QueryInterface(aConsumer); > m_runningUrl->GetMockChannel(getter_AddRefs(m_mockChannel)); > + eIMAPCapabilityFlags server_capabilityFlags = GetServerStateParser().GetCapabilityFlag(); > + m_isGmailServer = ((server_capabilityFlags & kGmailImapCapability) != 0); You should be able to use the function you created in nsImapServerResponseParser.h like this... m_isGmailServer = GetServerStateParser().ServerIsGMailServer(); instead of the two lines above (678 and 679). @@ +676,5 @@ > nsCOMPtr<nsIStreamListener> aRealStreamListener = do_QueryInterface(aConsumer); > m_runningUrl->GetMockChannel(getter_AddRefs(m_mockChannel)); > + eIMAPCapabilityFlags server_capabilityFlags = GetServerStateParser().GetCapabilityFlag(); > + m_isGmailServer = ((server_capabilityFlags & kGmailImapCapability) != 0); > + imapServer->SetIsGMailServer(m_isGmailServer); For the sake of me learning, I am curious to know why you choose to call SetIsGMailServer() here?

(In reply to Atul Jangra [:atuljangra] from comment #7) > I believe that this will also solve the problem reported in Bug 815087. I tried it out, repeating the steps to cause Bug 815087 and the is_gmail preference changes to false as we hoped, so unless you change something, I can say that this patch _definitely_ fixes Bug 815087.

Patch V2. Addressed Dlech's comments :-)

(In reply to David Lechner (:dlech) from comment #8) > Just being nosy :) > > ::: mailnews/imap/src/nsImapProtocol.cpp > @@ +675,5 @@ > > nsCOMPtr<nsIImapIncomingServer> imapServer = do_QueryInterface(server); > > nsCOMPtr<nsIStreamListener> aRealStreamListener = do_QueryInterface(aConsumer); > > m_runningUrl->GetMockChannel(getter_AddRefs(m_mockChannel)); > > + eIMAPCapabilityFlags server_capabilityFlags = GetServerStateParser().GetCapabilityFlag(); > > + m_isGmailServer = ((server_capabilityFlags & kGmailImapCapability) != 0); > > You should be able to use the function you created in > nsImapServerResponseParser.h like this... > > m_isGmailServer = GetServerStateParser().ServerIsGMailServer(); > > instead of the two lines above (678 and 679). Done. > @@ +676,5 @@ > > nsCOMPtr<nsIStreamListener> aRealStreamListener = do_QueryInterface(aConsumer); > > m_runningUrl->GetMockChannel(getter_AddRefs(m_mockChannel)); > > + eIMAPCapabilityFlags server_capabilityFlags = GetServerStateParser().GetCapabilityFlag(); > > + m_isGmailServer = ((server_capabilityFlags & kGmailImapCapability) != 0); > > + imapServer->SetIsGMailServer(m_isGmailServer); > > For the sake of me learning, I am curious to know why you choose to call > SetIsGMailServer() here? Because this is the code where we are in a state when we have got the capabilities flag and we are going to request headers. You can see a similar pattern with AOLServers in the code.

Seems like a more natural place to do it would be here: http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapIncomingServer.cpp#2172

(In reply to David :Bienvenu from comment #12) > Seems like a more natural place to do it would be here: > > http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/ > nsImapIncomingServer.cpp#2172 Yes,. I was looking for exactly the same place. I've updated the patch. Also, now there is no need of ServerIsGMailServer() anymore as we can check gmail server by imapServer->GetIsGMailServer(&m_isGmailServer); ( as we did earlier)

Made the required minimal changes.

Comment on attachment 695269 [details] [diff] [review] Final Patch looks reasonable, thx, except that you might as well use capability instead of m_capability, to be consistent.

Changes m_capability to capability. Already got r+. So checkin-needed.

Comment on attachment 695374 [details] [diff] [review] Final Patch Since this caused problems to users namely Bug 815087, I was hoping that we can land this earlier. Thus requesting beta approval.

Comment on attachment 695374 [details] [diff] [review] Final Patch [Triage Comment] Ok, this seems reasonably safe and should be obvious if we hit regressions, a=Standard8

https://hg.mozilla.org/releases/comm-aurora/rev/32e809bfb669 https://hg.mozilla.org/releases/comm-beta/rev/58a404dbb9d1 https://hg.mozilla.org/releases/comm-esr17/rev/1d8bcbd56570