Last Comment Bug 798663 - Should use presence of X-GM-EXT-1 capability to identify Gmail IMAP server
: Should use presence of X-GM-EXT-1 capability to identify Gmail IMAP server
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Networking: IMAP (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 20.0
Assigned To: Atul Jangra [:atuljangra]
:
Mentors:
Depends on:
Blocks: 798145 815087
  Show dependency treegraph
 
Reported: 2012-10-05 17:23 PDT by David Lechner (:dlech)
Modified: 2013-01-03 13:34 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
18+
fixed


Attachments
Pretty much does the job (6.67 KB, patch)
2012-12-21 17:25 PST, Atul Jangra [:atuljangra]
no flags Details | Diff | Splinter Review
Final Patch (6.57 KB, patch)
2012-12-22 01:42 PST, Atul Jangra [:atuljangra]
no flags Details | Diff | Splinter Review
Final Patch (4.45 KB, patch)
2012-12-22 17:16 PST, Atul Jangra [:atuljangra]
mozilla: review+
Details | Diff | Splinter Review
Final Patch (4.45 KB, patch)
2012-12-23 16:27 PST, Atul Jangra [:atuljangra]
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
standard8: approval‑comm‑esr17+
Details | Diff | Splinter Review

Description David Lechner (:dlech) 2012-10-05 17:23:14 PDT
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.
Comment 1 David Lechner (:dlech) 2012-10-05 17:24:45 PDT
This is followup bug to item 7 on https://bugzilla.mozilla.org/show_bug.cgi?id=721316#c60
Comment 2 David Lechner (:dlech) 2012-10-06 08:33:09 PDT
(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
Comment 3 Atul Jangra [:atuljangra] 2012-11-28 10:32:48 PST
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?
Comment 4 Atul Jangra [:atuljangra] 2012-11-28 10:33:48 PST
Confirming the bug as we are sure we want to do this. Otherwise situations like Bug 815087 can arise.
Comment 5 David Lechner (:dlech) 2012-11-28 10:44:59 PST
(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.
Comment 6 Atul Jangra [:atuljangra] 2012-12-21 17:25:11 PST
Created attachment 695059 [details] [diff] [review]
Pretty much does the job

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. :-)
Comment 7 Atul Jangra [:atuljangra] 2012-12-21 17:29:32 PST
I believe that this will also solve the problem reported in Bug 815087.
Comment 8 David Lechner (:dlech) 2012-12-21 19:08:32 PST
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?
Comment 9 David Lechner (:dlech) 2012-12-21 19:10:57 PST
(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.
Comment 10 Atul Jangra [:atuljangra] 2012-12-22 01:42:23 PST
Created attachment 695157 [details] [diff] [review]
Final Patch

Patch V2.
Addressed Dlech's comments :-)
Comment 11 Atul Jangra [:atuljangra] 2012-12-22 01:44:48 PST
(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.
Comment 12 David :Bienvenu 2012-12-22 08:28:56 PST
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
Comment 13 Atul Jangra [:atuljangra] 2012-12-22 17:14:31 PST
(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)
Comment 14 Atul Jangra [:atuljangra] 2012-12-22 17:16:25 PST
Created attachment 695269 [details] [diff] [review]
Final Patch

Made the required minimal changes.
Comment 15 David :Bienvenu 2012-12-23 09:33:36 PST
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.
Comment 16 Atul Jangra [:atuljangra] 2012-12-23 16:27:01 PST
Created attachment 695374 [details] [diff] [review]
Final Patch

Changes m_capability to capability.
Already got r+. So checkin-needed.
Comment 17 Ryan VanderMeulen [:RyanVM] 2012-12-23 19:07:13 PST
https://hg.mozilla.org/comm-central/rev/ea973e3223b2
Comment 18 Atul Jangra [:atuljangra] 2012-12-23 19:31:53 PST
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 19 Mark Banner (:standard8) 2012-12-24 13:23:54 PST
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

Note You need to log in before you can comment on or make changes to this bug.