Closed Bug 721316 Opened 12 years ago Closed 12 years ago

Use Gmail IMAP Extension of X-GM-MSGID, X-GM-THRID, X-GM-LABELS in addition to XLIST

Categories

(MailNews Core :: Networking: IMAP, enhancement)

Product:
MailNews Core
Component:
Networking: IMAP
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(thunderbird17 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 18.0
Tracking Flags:
Tracking Status
thunderbird17 --- fixed

People

(Reporter: World, Assigned: atuljangra)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

(Keywords: perf)

Attachments

(2 files, 16 obsolete files)

Final Patch v3
68.77 KB, patch
standard8
: approval-comm-beta+
Details | Diff | Splinter Review
External API fixes
1.42 KB, patch
standard8
: review+
standard8
: approval-comm-beta+
Details | Diff | Splinter Review 
Gmail has Gmail IMAP Extensions.
http://code.google.com/apis/gmail/imap/
> Gmail IMAP Extensions
> Extension of the LIST command: XLIST
> Extension of the SEARCH command: X-GM-RAW
> Access to the Gmail unique message ID: X-GM-MSGID
> Access to the Gmail thread ID: X-GM-THRID
> Access to Gmail labels: X-GM-LABELS
Please consider using Gmail IMAP Extension of X-GM-MSGID, X-GM-THRID, X-GM-LABELS in addition to XLIST.

(i) Access to the Gmail unique message ID: X-GM-MSGID
By X-GM-MSGID, relation between copy of mail in a Gmail IMAP folder(Gmail Label) and original mail in [Gmail]/All Mail can be known.
(ii) Access to Gmail labels: X-GM-LABELS
X-GM-LABELS, all Gmail Label added to a mail can be known, although it looks that distinction from IMAP flag(user added keyword) is slightly difficult. 

If (i) and (ii) is used at [Gmail]/All Mail, "Gloda Indexing at [Gmail]/All only" is possible. And if (i) is used at any Gmail Label(Gmail IMAP) folder, association to Gloda entory is possible.

(iii) Access to the Gmail thread ID: X-GM-THRID
By X-GM-THRID, same thread as Gmail's conversation can be obtained. If this is used, Tb's work needed in mail threading is "threading in a conversation" only.
Blocks: tb-gmailWIP
Severity: normal → enhancement
I have been doing some in depth work with Thunderbird and Gmail for an extension that I am developing. I would like to see the same things. Being rather new to the development side of Mozilla/Thunderbird, I am still getting a feel for the overall philosophy. So, I would like to propose the following philosophical question:

Since all Thunderbird users are not Gmail users, does it make sense to include this in the mailnews core or would it be better to create an addon (like http://quetzalcoatal.blogspot.com/search/label/accttype) for Gmail users?

With the extension I am developing, I am currently fetching/displaying the Gmail labels and the Gmail message id. I agree the next logical step is to eliminate the duplicate storage of messages on the local hard drive.

Looking forward to more discussion on the topic.
We already have gmail-specific code in the imap part of our codebase, so I think it's okay to enhance our support for gmail in core Thunderbird code.

One easy win would be to hack the threading code so that it can also take into account the X-GM-THRID header. I don't think it's that hard, but David Bienvenu will certainly have more to say on the matter. Not storing the same message multiple times seems harder to me, and way more invasive :).

Thanks,

jonathan
(In reply to David Lechner from comment #1)

> Since all Thunderbird users are not Gmail users, does it make sense to
> include this in the mailnews core or would it be better to create an addon
> (like http://quetzalcoatal.blogspot.com/search/label/accttype) for Gmail
> users?
> 
Yes, the plan is to put the code in core. We have a fairly large number of gmail Thunderbird users, and non-gmail accounts won't be affected significantly by the gmail changes (other than slightly larger code size, and  a couple additional checks if their server is a gmail server).
should this block Bug 475857 - gloda could specialize for gmail IMAP
Blocks: 475857
Alright,
1. Is anyone actively working on this?
2. Is this the best place to have this discussion?
3. What can I do to help?
I believe a student is working on this as a GSoC project. Bienvenu probably knows more about the details of that.
(In reply to David Lechner (:dlech) from comment #5)
> 3. What can I do to help?

Extension, like JunQuilla, which shows X-GM-MSGID, X-GM-THRID, X-GM-LABELS value in additional thread pane columns, will help us. Code for getting X-GM-MSGID, X-GM-THRID, X-GM-LABELS value by the extension will help Tb's Core code implementation.
(In reply to Jim Porter (:squib) from comment #6)
> I believe a student is working on this as a GSoC project. Bienvenu probably
> knows more about the details of that.

Yes, Atul Jangra is a Google summer of code student who will be working on this bug.
Yes I am working on this bug with David Bienvenu as my mentor. :)
Assignee: nobody → atuljangra66
Summary: Please consider using Gmail IMAP Extension of X-GM-MSGID, X-GM-THRID, X-GM-LABELS in addition to XLIST → Use Gmail IMAP Extension of X-GM-MSGID, X-GM-THRID, X-GM-LABELS in addition to XLIST
This patch downloads the headers ("X-GM-THRID" and "X-GM-MSGID") for a GMail server and passes the value to mailnews core db code so that we can store the MSGID and THRID in the dbmsg hdr for the database.
Excellent. For the next step, I think we want to fetch x-gm-labels as well, and store the set of those as a property for each message. This should be fairly similar to what you've already done, except that parsing the response is a bit harder, since it's a paren group and not an atom. See https://developers.google.com/google-apps/gmail/imap_extensions for the x-gm-labels syntax.

The next step after that is to use this information to only download one copy of a message for offline use. There are several parts to this.

1. When trying to fetch a message, enhance the "do we have a copy of this message offline" logic to know that gmail messages can be in multiple folders, and one of those folders might already have this message offline. And enhance the logic that actually loads the offline message to know where to look for the message.

2. Change autosync to know the same thing so that it doesn't try to download a message we already have.

3. When copying messages within a a gmail account, we most likely don't need to copy the offline store copy of the messages.

There are some subtleties that we should consider. All Mail is an attractive place to store the offline messages because all messages are in All Mail, so even if a message is moved from one imap folder to an other, it will remain in All Mail. The downsides to All Mail is that it can get quite large, which means opening the .msf file can be expensive. It also means that compacting the offline store can be expensive, though I don't think tends to get removed from All Mail frequently. And there are a small number of people who unsubscribe from All Mail, or don't have it somehow.

The alternative is to store the offline message in a first come first serve manner, as it's done today. Most messages would get stored in the inbox, and then moved to other folders offline store when the messages are moved. This would mean that the code that looks for messages would need to know to look in various folders offline store. This is where caching the x-gm-labels would be required.  If we do this approach, I think we probably want to remember where an offline message is stored in the .msf file header information of a message in another folder, so that we don't need to look through the potential db's to find the header. This would just be a hint, though, since the message could get removed from the folder.

The All Mail approach would probably be easier to code, but it won't necessarily scale well. I will think a bit more about this, and would be interested in hearing your thoughts as well, Atul.
This patch also handles X-GM-LABELS in addition to the work previously done. So now we we have requested X-GM-THRID, X-GM-MSGID and X-GM-LABELS, parsed the response from the gmail server and added these values in the hashtable. So now we can use these attributes to improve gmail interoperability.
I've thought a bit more about it, and doing All Mail does add a bit of extra complication, in that we don't keep All Mail up to date in a very timely manner. This is partly because autosync tries to do most of its work while the user is idle. That means that if we went with the approach of storing the messages in the All Mail offline store all the time, we would need some way of storing the information about where the message is in the offline store without having the real message header. This is doable, but tricky. It also means we'd keep the All Mail .msf file open all the time, which I'd rather not have to do.

So I'd like to go with the approach of storing the offline message in the folder it first appears in (most likely the inbox) and just changing the code the checks if a message is offline, and the code that finds a message in the offline store to check in the current folder and the other folders indicated by the x-gm-labels. This won't be perfect (e.g., we don't update the x-gm-labels property after the first time we get a message's headers, so it can get stale) but we're really just trying to make normal usage better.

I'll outline how that would work in my next comment.
Step 1 - add a method to nsIMsgDatabase that gets a message header for a given x-gm-msgid, GetMsgHdrForGMMsgID. This would be closely modeled after nsMsgDatabase::GetMsgHdrForMessageID(). The one difference is that you'd need to call err = m_mdbStore->StringToToken(GetEnv(),  propertyName, &property_token); instead of using the pre-calculated token the way GetMsgHdrForMessageID does.

Step 2 - make trying to fetch/display an imap message know how to look in other offline stores than just the folder its currently in. This means making nsImapMockChannel::ReadFromLocalCache() work in this situation, which means making nsMsgDBFolder::HasMsgOffline(nsMsgKey msgKey, bool *result) know how to look in other folders. To do this, I think nsImapMailFolder should override HasMsgOffline. First, check if the msg is in the current folder, like the base class does, and if not, check if we're a gmail server, and if so, get the x-gm-labels property from the header, parse it into separate labels (the method ParseString with a ' ' delimiter should be helpful here). Then, for each label, find the corresponding folder (nsImapMailFolder::FindOnlineSubFolder from the root imap folder for the server should help there). Don't forget to ignore the label for the folder that we already checked - "this" folder. Then, get the db for the folder you got from the label, call your new GetMsgHdrForGMMsgID method on that.  If you get a header back, and it has the offline flag set, then you can set the out result to true, and return. Since All Mail doesn't show up as a label, you probably want to do one more check using the All Mail folder. Most of this is going to be in a helper routine because you're going to do something very similar for nsMsgDBFolder::GetOfflineFileStream. Override it for imap, check the current folder, using the base class method, if that fails, try the other folders...

That should give you a good place to start. There are optimizations we can make, I'm sure, but lets try to get the basic stuff working.
(In reply to David :Bienvenu from comment #14)
> Don't forget to ignore the label for the folder
> that we already checked - "this" folder. 

This is not a problem. X-GM-LABELS does not include the label for the current folder. 
One complication is that X-GM-LABELS returns "special" values for some system labels, like Inbox or Important. These will need to be mapped to the folder names through the values returned by XLIST.
(In reply to Brian Kennelly from comment #15)

> This is not a problem. X-GM-LABELS does not include the label for the
> current folder. 
cool, thx.
> One complication is that X-GM-LABELS returns "special" values for some
> system labels, like Inbox or Important. These will need to be mapped to the
> folder names through the values returned by XLIST.

Oh, thx, right, like \INBOX. We'll have to deal with the server-side localization of the names, iirc. We persist the flags that correspond to the xlist special folder names, so, Atul, you'll need to map the "special" labels to a folder flag, and find the folder by the flag.  You can use getFolderWithFlags for this. See https://developers.google.com/google-apps/gmail/imap_extensions#extension_of_the_list_command_xlist for the special folders.

For XLIST, we tend to run roughshod over whatever special folders the user may have picked, so using the folder flag should work ok.

Atul, the next step after this is to make autosync not to try to download copies of messages. I'll try to write up that will entail in a bit. It may come mostly for free once you've overridden HasMsgOffline but I doubt we'll be that lucky.
You may have already figured all of this out, but there are a couple things I have learned that may be useful.

1. The Important flag is returned as a label(as mentioned in comment 15), but it does not have an imap folder like the rest of the labels. You may want to treat it like a flag rather than a label/folder.

2. Parsing the data returned by FETCH X-GM-LABELS to get the individual lables is a bit tricky because of the quotes. I found http://stackoverflow.com/a/6464500 to be useful and came up with this in javascript:
reg = /[ ](?=(?:[^"\\]*(?:\\.|"(?:[^"\\]*\\.)*[^"\\]*"))*[^"]*$)/g;

3. Messages in Trash and Spam folders still return any other labels they had before they were moved to Trash or Spam. So, these may require special consideration as well.
(In reply to David Lechner (:dlech) from comment #17)
> 
> 1. The Important flag is returned as a label(as mentioned in comment 15),
> but it does not have an imap folder like the rest of the labels. You may
> want to treat it like a flag rather than a label/folder.
The "\Important" X-GM-LABELS flag does correspond to an IMAP folder. In US-English, it is [Gmail]/Important, but it can be identified from the XLIST response. 
Perhaps you hid that label from IMAP? X-GM-LABELS does return labels that are otherwise hidden from IMAP.
(In reply to Brian Kennelly from comment #18)

> Perhaps you hid that label from IMAP? X-GM-LABELS does return labels that
> are otherwise hidden from IMAP.

Thanks for setting me strait on that one. I changed my Gmail settings. I just did a check and it looks like XLIST returns all folders, including Important.

* XLIST (\HasNoChildren \Important) "/" "[Gmail]/Important"

 I vaguely remember it not being there before. Perhaps google has made a change?
This patch
:Downloads the headers and parses the response. 
:Passes the response into the mailnews core db code.
:Implements HasMsgOffline() function which enables us to look into all the folders/labels for the message rather than just the folder where the message first came. {here we make use of the various headers.}
Oops spelling mistake ;)
HasMsgOffline().
Comment on attachment 635845 [details] [diff] [review]
implemented HasMsfOffline() and the methods required my it.

Great, glad you got this working. Now's a good time to do some review comments so you can learn the right coding style before writing the next batch of code.

You need to add doxygen style comments for any new methods.  See http://mxr.mozilla.org/comm-central/source/mailnews/base/public/nsIMsgFolder.idl#175 for a sample doxygen style comment, where you document what the method does, the argument it takes, and the value it returns
+  nsIMsgDBHdr GetMsgHdrForGMMsgID(in string gmailmessageID);

Also, arguments should start with the name a, so aGmailMessageID is a better name.

modern practice is to use a comptr, so

nsCOMPtr<nsIMsgDBHdr> msgHdr;

and nsCOMPtr<nsIMdbRow> hdrRow;

+  NS_ENSURE_ARG_POINTER(aGMMsgId);
+  NS_ENSURE_ARG_POINTER(aHdr);
+  nsIMsgDBHdr  *msgHdr = nsnull;
+  nsresult rv = NS_OK;
+  mdbYarn gMailMessageIdYarn;

We much prefer early returns for unrecoverable errors like a null m_mdbStore, so instead of 

+  if (m_mdbStore)
+    result = m_mdbStore->StringToToken(GetEnv(), "X-GM-MSGID" , &property_token);
+  else
+    return NS_ERROR_FAILURE;
+  if(NS_SUCCEEDED(result) && m_mdbStore)

do this instead:

NS_ENSURE_TRUE(m_mdbStore, NS_ERROR_NULL_POINTER);
result =  m_mdbStore->StringToToken(GetEnv(), "X-GM-MSGID" , &property_token);
NS_ENSURE_SUCCESS(result, result);

since hdrRow is now a COMPtr,

+    result = m_mdbStore->FindRow(GetEnv(), m_hdrRowScopeToken,
+      property_token, &gMailMessageIdYarn,  &outRowId,
+      &hdrRow);

should be

+    result = m_mdbStore->FindRow(GetEnv(), m_hdrRowScopeToken,
+      property_token, &gMailMessageIdYarn,  &outRowId,
+      getter_AddRefs(hdrRow));

and now

+  *aHdr = msgHdr; // already addreffed above.
+  return NS_OK; // it's not an error not to find a msg hdr.

can be
  msgHdr.forget(aHdr);

which efficiently swaps the reference from msgHdr to aHdr

need doxygen comments for this, and we try to wrap lines longer than 80 characters, which addUidCustomAttribute looks to be.

+  void addUidCustomAttribute(in unsigned long aUid, in ACString aCustomAttributeName, in ACString aCustomAttributeValue);
+  ACString getCustomAttribute(in unsigned long aUid, in ACString aCustomAttributeName);

need a space after the if here: (this occurs several places in the patch)

+  if(m_customAttributesHash.IsInitialized())

space after comma here (similarly, this there are several places where you don't have a space after a ,:

+    m_customAttributesHash.Get(key,&val);

this leaks - I'll try to tell you a better way of doing this, but for now, at least, fix the leak by using Adopt instead of Assign, which makes aCustomAttributeValue take over ownership of the allocated memory instead of just copying the string again.

+      aCustomAttributeValue.Assign(NS_strdup(val.get()));

super long line - please put the comment before the variable declaration:
+    nsDataHashtable<nsCStringHashKey, nsCString> m_customAttributesHash; //Hash table, mapping UID+attributeName to attributeValue.

it looks like you might have tabs here:

-
+		// DEBUG CALL
+		printf("something fishy :-| \n");
+		if(checkIfGmailServer)
+		{
+		  nsCOMPtr <nsIMsgDBHdr> tryHdr;
+		  rv = sourceMailDB->GetMsgHdrForGMMsgID("1403047636320443836", getter_AddRefs(tryHdr));
+		  printf("was here :-| \n");
+		}

we don't allow tabs, and we use 2 space indent.

Also, please remove the printfs, and remove the space after nsCOMPtr.

more tabs, I think - or at least bad indentation:

          if((mLabelNames[i].Equals("\"\\\\Draft\"")))
+          {
+             rt = rootFolder->GetFolderWithFlags(nsMsgFolderFlags::Drafts,getter_AddRefs(sub));
+             if(NS_SUCCEEDED(rt))
+               subFolder=do_QueryInterface(sub);
+		  }
+

can't you use rv here instead of rt?

+             rt = rootFolder->GetFolderWithFlags(nsMsgFolderFlags::Drafts,getter_AddRefs(sub));
+             if(NS_SUCCEEDED(rt))
+               subFolder=do_QueryInterface(sub);
+		  }

It seems like the code would be cleaner if you moved these lines:

+             if(NS_SUCCEEDED(rt))
+               subFolder=do_QueryInterface(sub);
+		  }

out of each individual if, and moved them after all the ifs.

I'm going to stop now, because it's hard to review the rest of the code because of the tabs/bad indentation, so if you could fix all of that, I'll look at the code some more later.
Attached patch Improved Indentation (obsolete) — — Splinter Review 
Improved Indentation and made changes as per David Bienvenu's suggestions.
Comment on attachment 636018 [details] [diff] [review]
Improved Indentation

checkIfGmailServer - this is a boolean member variable? If so, it should have a name like m_checkIfGmailServer, or really, m_isGmailServer.

But, you're initializing it in NormalEndHeaderParseStream and using it in HasMsgOffline - you can't assume NormalEndHeaderParseStream is called before HasMsgOffline. I'd suggest for now leaving isGmailServer as a local variable, and doing this same code in HasMsgOffline:

+  nsCOMPtr<nsIImapIncomingServer> imapServer = do_QueryInterface(server);
+  rv = imapServer->GetIsGMailServer(&isGmailServer);
+  NS_ENSURE_SUCCESS(rv,rv);
+

there's no need for the braces here:

+          if ((mLabelNames[i].Equals("\"\\\\Draft\"")))
+          {
+             rv = rootFolder->GetFolderWithFlags(nsMsgFolderFlags::Drafts,
+                                                 getter_AddRefs(subMsgFolder));
+          }

also, all the other if's should be else if.


the rootFolder stuff should be outside the loop, since it only needs to be done once:

+        nsCOMPtr<nsIMsgFolder> rootFolder;
+        nsCOMPtr<nsIMsgFolder> subMsgFolder;
+        rv = GetRootFolder(getter_AddRefs(rootFolder));


duplicated line:

+            nsCOMPtr<nsIMsgDatabase> db;
+            subMsgFolder->GetMsgDatabase(getter_AddRefs(db));
+            subMsgFolder->GetMsgDatabase(getter_AddRefs(db));


I know you mainly copied this code, but it could be improved:

+    //Get key from row
+    mdbOid outOid;
+    nsMsgKey key=0;
+    if (hdrRow->GetOid(GetEnv(), &outOid) == NS_OK)
+      key = outOid.mOid_Id;
+    rv = GetHdrFromUseCache(key, getter_AddRefs(msgHdr));
+    if (NS_SUCCEEDED(rv) && msgHdr)
+      hdrRow->Release();
+    else
+      rv = CreateMsgHdr(hdrRow, key, getter_AddRefs(msgHdr));
+  }

if GetOid fails, we should return an error.

so, something like

+    mdbOid outOid;
+    rv = hdrRow->GetOid(GetEnv(), &outOid);
+    NS_ENSURE_SUCCESS(rv, rv);  
+    nsMsgKey key = outOid.mOid_Id;

The next step is to break out the code in HasMsgOffline so that it can be used to figure out which folder we have the message in, because we'll need that to stream the message from the right offline store. That should be a new utility function in nsImapMailFolder (I don't know that it needs to be an idl interface method at this point) that takes a msg key and returns as an out param the nsIMsgFolder that has the offline copy of the message. HasMsgOffline can use that utility function.
Made GetOfflineMsgFolder method, tells us about the existence of msg in different folders. 
I am having segmentation fault for this in my machine, I am suspecting that I am missing some small thing, so as suggested by David Bienvenu, I am uploading the patch.
There's nice progress being made here, that's really great. Atul, if you have the time, you may want to consider upgrading gloda to support these attributes as well, because that would be extremely beneficial for people who rely on gloda for getting the related messages in a thread:
- addons such as thread arcs, thunderbird conversations,
- other gsoc projects, e.g. reply manager.

If you don't have enough time, that's ok, but we may want to keep this in mind, as I believe it really is important :).

Cheers,

jonathan
Thanks a lot Jonathan for appreciating the work.
Regarding upgrading gloda to support these attributes, we will surely do that. Currently my I should implement this properly, and then I will head on to make change in gloda. It would be my honor if people are getting benefited by my work. 
Regards
Atul
Sure, don't hesitate to ask me if you've got any questions regarding gloda, I'd be happy to help!
(In reply to Atul Jangra from comment #25)
> Created attachment 637805 [details] [diff] [review]
> Implmented GetOfflineMsgFolder and made few changes
> 
> Made GetOfflineMsgFolder method, tells us about the existence of msg in
> different folders. 
> I am having segmentation fault for this in my machine, I am suspecting that
> I am missing some small thing, so as suggested by David Bienvenu, I am
> uploading the patch.

I forgot to change the uuid in this patch :(. Will do it in next one. #StupidMe
Attached patch Implemented all the required methods. (obsolete) — — Splinter Review 
Does all the thing that we need to do so as to store the message once and point to it from other locations/labels/folders.
Next, I will be writing tests for the code.
@Atul Jangra, you may be interested in the test I have written for bug 778246. I have implemented STORE X-GM-LABELS in the fake IMAP server (imapd.js)
@David Lechner, thanks :) I am going through the test :)
Attached patch Patch to prevent breakage of a couple of tests (obsolete) — — Splinter Review 
@atuljangra: this should make your life easier

@anyone: should this be part of this bug or should I file a new bug?
Attachment #648862 - Flags: feedback?(mozilla)
Comment on attachment 648862 [details] [diff] [review]
Patch to prevent breakage of a couple of tests

part of this bug is fine, I think.

feedback+, modulo removing the do_prints, perhaps.
Attachment #648862 - Flags: feedback?(mozilla) → feedback+
This also has positive performance implications, so kw=perf
Keywords: perf
Attached patch Final Patch v1 (obsolete) — — Splinter Review 
Implements all the required methods
Contains two xpcshell tests
test_gmailAttributes.js: tests fetching of gmail attributes
test_gmailOfflineMsgStore: tests our new message storing scheme.

Currently both the tests are passing locally. 
Hoping for a safe landing after reviews by bienvenu :-)
Cheers
Attachment #651167 - Flags: review?(mozilla)
(In reply to David Lechner (:dlech) from comment #33)
> Created attachment 648862 [details] [diff] [review]
> Patch to prevent breakage of a couple of tests
> 
> @atuljangra: this should make your life easier
> 
> @anyone: should this be part of this bug or should I file a new bug?

Thanks a lot David :-)
Comment on attachment 651167 [details] [diff] [review]
Final Patch v1

Review of attachment 651167 [details] [diff] [review]:
-----------------------------------------------------------------

thx for the patch!, a bunch of comments, mostly nits. I didn't identify all the places where you have spaces at the ends of lines, or missing space after // in comments, so double check the whole next patch before you put it up for review.

::: mailnews/db/msgdb/public/nsIMsgDatabase.idl
@@ +273,5 @@
> +  /**
> +   * This function is used to get a message header for a given Gmail
> +   * message ID that we get from X-GM-MSGID.
> +   */
> +  nsIMsgDBHdr GetMsgHdrForGMMsgID(in string aGmailMessageID);

...get a message header for a Gmail message with the given X-GM-MSGID.

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +4592,5 @@
> +  nsIMsgDBHdr  *msgHdr = nullptr;
> +  nsresult rv = NS_OK;
> +  mdbYarn gMailMessageIdYarn;
> +  gMailMessageIdYarn.mYarn_Buf = (void *) aGMMsgId;
> +  gMailMessageIdYarn.mYarn_Fill = PL_strlen(aGMMsgId);

should just use strlen

@@ +4621,5 @@
> +    else
> +      rv = CreateMsgHdr(hdrRow, key, &msgHdr);
> +  }
> +  *aHdr = msgHdr;
> +  return NS_OK; // it's not an error not to find a msg hdr.

I would say it is an error for this method, just not the method this code was copied from.

::: mailnews/imap/public/nsIImapFlagAndUidState.idl
@@ +50,3 @@
>    string getCustomFlags(in unsigned long uid); // returns space-separated keywords
>    void reset();
>    void clearCustomFlags(in unsigned long uid);

probably should rev the uuid

@@ +54,5 @@
> +   * Adds custom attributes to a hash table for the purpose of storing them
> +   * them. 
> +   * @param aUid   UID of  the associated msg
> +   * @param aCustomAttributeName   Name of the custom attribute value
> +   * @param aCustomAttributeValue   Value of the attribute, 

space at end of line?

@@ +56,5 @@
> +   * @param aUid   UID of  the associated msg
> +   * @param aCustomAttributeName   Name of the custom attribute value
> +   * @param aCustomAttributeValue   Value of the attribute, 
> +   */
> +  void addUidCustomAttribute(in unsigned long aUid, 

several more single spaces at ends of lines...

::: mailnews/imap/src/nsImapFlagAndUidState.cpp
@@ +334,5 @@
> +    nsCString val;
> +    m_customAttributesHash.Get(key, &val);
> +    if (!val.IsEmpty())
> +    {
> +      aCustomAttributeValue.Adopt(NS_strdup(val.get()), val.Length());

I'm sure there is a better way of doing this.

::: mailnews/imap/src/nsImapFlagAndUidState.h
@@ +42,4 @@
>      nsTArray<imapMessageFlagsType> fFlags;
>      // Hash table, mapping uids to extra flags
>      nsDataHashtable<nsUint32HashKey, char *> m_customFlagsHash;
> +    //Hash table, mapping UID+customAttributeName to customAttributeValue.

space after //

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +9550,5 @@
> +}
> +
> +NS_IMETHODIMP nsImapMailFolder::GetOfflineMsgFolder(nsMsgKey msgKey, nsIMsgFolder **aMsgFolder)
> +{
> +  //Check if we have the message in the current folder.

space after //

@@ +9648,5 @@
> +                if ((gmFlags & nsMsgMessageFlags::Offline))
> +                {
> +                  subMsgFolder.forget(aMsgFolder);
> +                  // focus on first positive result.
> +                  return NS_OK; 

space at end of line

@@ +9685,5 @@
> +    {
> +      nsCString gmMsgID;
> +      hdr->GetStringProperty("X-GM-MSGID", getter_Copies(gmMsgID));
> +      nsCOMPtr<nsIMsgDatabase> db;
> +      if(!offlineFolder)

seems like you could check for offline folder being null earlier, like right after you call GetOfflineMsgFolder.

::: mailnews/imap/src/nsImapMailFolder.h
@@ +296,5 @@
> +  * of msg in all the folders/labels that we retrieve from X-GM-LABELS also.
> +  *  @param msgKey key  of the msg for which we are trying to get the folder;
> +  *  @param aMsgFolder  required folder;
> +  */
> +  NS_IMETHOD GetOfflineMsgFolder(nsMsgKey msgKey, nsIMsgFolder **aMsgFolder);

this isn't an NS_IMETHOD, is it? It's not defined in any idl, right? So it's just nsresult, and should be in a different part of the class definition.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +663,4 @@
>      nsCOMPtr<nsIImapIncomingServer> imapServer = do_QueryInterface(server);
>      nsCOMPtr<nsIStreamListener> aRealStreamListener = do_QueryInterface(aConsumer);
>      m_runningUrl->GetMockChannel(getter_AddRefs(m_mockChannel));
> +    nsresult rv = imapServer->GetIsGMailServer(&m_checkGmailServer);

I feel like this bool should be initialized in the constructor, to false, and then you can ignore the error to the call to GetIsGMailServer. And, the member name should be m_isGmailServer, not m_checkGmailServer, which implies that we should be checking if it is a gmail server.

::: mailnews/imap/src/nsImapProtocol.h
@@ +575,4 @@
>    PRUint32 mFolderHighestUID;
>    PRUint32 mFolderNumDeleted;
>  
> +  bool    m_checkGmailServer; // tells us whether we are on GMail server.

again, rename this.

::: mailnews/imap/src/nsImapServerResponseParser.cpp
@@ +1260,5 @@
>        }
> +      else if (!PL_strcasecmp(fNextToken, "X-GM-MSGID"))
> +      {
> +        AdvanceToNextToken();
> +        if (! fNextToken) 

more spaces at end of lines...at least half a dozen in this file...

::: mailnews/imap/test/unit/test_downloadOffline.js
@@ +67,4 @@
>        gIMAPInbox.getOfflineFileStream(header.messageKey, offset, size).close();
>      else
>        do_throw("Message not downloaded for offline use");
> +

is this a line with blanks? this file shouldn't really be changed...

::: mailnews/imap/test/unit/test_fetchCustomAttribute.js
@@ +139,4 @@
>                       .newFileURI(file)
>                       .QueryInterface(Ci.nsIFileURL);
>    return msgfileuri.spec;
> +}

not sure why this is showing up as a diff...

::: mailnews/imap/test/unit/test_gmailAttributes.js
@@ +13,5 @@
> + *
> + * Original Author: Atul Jangra<atuljangra66@gmail.com>
> + */
> +
> +// async support 

space at end of line?

::: mailnews/imap/test/unit/test_gmailOfflineMsgStore.js
@@ +34,5 @@
> +
> +const gMessage1 = "bugmail10"; // message file used as the test message for Inbox and fooFolder
> +const gXGmMsgid1 = "1278455344230334865";
> +const gXGmThrid1 = "1266894439832287888";
> +// We need to have different X-GM-LABELS for different folders. I am doing it here manually, but this issue will be tackled in Bug 781443.

line should be wrapped, over 80 columns

@@ +175,5 @@
> +  {
> +    rootFolder = gIMAPIncomingServer.rootFolder;
> +    fooFolder =  rootFolder.getChildNamed("foo").QueryInterface(Ci.nsIMsgImapMailFolder); //we have created the mailbox earlier.
> +    fooFolder.updateFolderWithListener(null, asyncUrlListener);
> +    yield false;  

space at end of line

::: mailnews/test/fakeserver/imapd.js
@@ +1691,5 @@
> +    var base = this._daemon.getMailbox(args[0]);
> +    if (!base)
> +      return "NO no such mailbox";
> +    if(!this._daemon.getMailbox("[Gmail]/All Mail", {subscribed : true})) {
> +      //No special mailbox exist, so we will create them.

spaces after //
Attachment #651167 - Flags: review?(mozilla) → review-
Attached patch Final Patch v2 (obsolete) — — Splinter Review 
Final patch version-2.
Includes dlech's patch(with some changes)
All the tests are passing locally.
Attachment #651167 - Attachment is obsolete: true
Attachment #652085 - Flags: review?(mozilla)
Comment on attachment 652085 [details] [diff] [review]
Final Patch v2

Review of attachment 652085 [details] [diff] [review]:
-----------------------------------------------------------------

thx for fixing all those issues; I just see the one left. Tests all pass (though they leak a bit, which is generally a sign that they're not cleaned up at the end, or the backend is still talking to the imap server when the test finishes).

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +288,4 @@
>  static bool gExpungeAfterDelete = false;
>  static bool gCheckDeletedBeforeExpunge = false; //bug 235004
>  static PRInt32 gResponseTimeout = 60;
> +static bool m_isGmailServer = false;

this should be a member variable of the protocol object, not a static global variable
Attachment #652085 - Flags: review?(mozilla) → review-
(In reply to David :Bienvenu from comment #40)
> Comment on attachment 652085 [details] [diff] [review]
> Final Patch v2
> 
> Review of attachment 652085 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> thx for fixing all those issues; I just see the one left. Tests all pass
> (though they leak a bit, which is generally a sign that they're not cleaned
> up at the end, or the backend is still talking to the imap server when the
> test finishes).
 How to fix this? Also How can I tests any leaks while debugging?

> ::: mailnews/imap/src/nsImapProtocol.cpp
> @@ +288,4 @@
> >  static bool gExpungeAfterDelete = false;
> >  static bool gCheckDeletedBeforeExpunge = false; //bug 235004
> >  static PRInt32 gResponseTimeout = 60;
> > +static bool m_isGmailServer = false;
> 
> this should be a member variable of the protocol object, not a static global
> variable
Will be fixed in next patch :)
Thanks for the reviews :)
Status: NEW → ASSIGNED
When you run the xpcshell tests in a shell, you should see the leak stats in the console. You first need to figure out why the test is leaking; the easiest thing to do is generate a protocol log and see if it looks like we're still talking to the server after the test finishes, or if we're cleanly logging out.
Antul will you provide an updated patch ?
Attached patch Final Patch v3 (obsolete) — — Splinter Review 
Includes all the code for our new message storage scheme.
Includes two tests test_gmailAttributes.js and test_gmailOfflineMsgStore.js, which are passing locally without any leaks. 
Also includes two modified tests, which earlier used GMail extensions as test case for custom attributes. But with my patch, they are no longer custom.
Attachment #652085 - Attachment is obsolete: true
Attachment #654611 - Flags: superreview?(mbanner)
Attachment #654611 - Flags: review?(mozilla)
I've been reviewing these patches along the way, and Atul has been diligently addressing my review comments, so even though I may not be able to do the final review until this weekend, I believe the patch is ready for sr.
Comment on attachment 654611 [details] [diff] [review]
Final Patch v3

Review of attachment 654611 [details] [diff] [review]:
-----------------------------------------------------------------

In general the patch is looking good, but needs some minor updates, I haven't tested it yet - will do in a bit. It is slightly out of date for the PRUint32 -> uint32_t conversions that happened, so as I've done those to fix bitrot in applying the patch, I'll post an updated version in a minute. I'll also see if I can fix the history for what I mentioned about the test files.

::: mailnews/base/src/nsMessengerUnixIntegration.cpp
@@ -220,4 @@
>    prefBranch->GetBoolPref(SHOW_ALERT_SENDER, &showSender);
>    prefBranch->GetBoolPref(SHOW_ALERT_SUBJECT, &showSubject);
>    prefBranch->GetIntPref(SHOW_ALERT_PREVIEW_LENGTH, &previewLength);
> -

Please drop this blank-line change as it isn't anything to do with this patch.

::: mailnews/db/msgdb/public/nsIMsgDatabase.idl
@@ +272,5 @@
> +
> +  /**
> +   * get a message header for a Gmail message with the given X-GM-MSGID.
> +   */
> +  nsIMsgDBHdr GetMsgHdrForGMMsgID(in string aGmailMessageID);

You'll need to generate a new uuid for this interface file ("/msg firebot uuid" on irc).

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +4588,5 @@
> +NS_IMETHODIMP nsMsgDatabase::GetMsgHdrForGMMsgID(const char *aGMMsgId, nsIMsgDBHdr **aHdr)
> +{
> +  NS_ENSURE_ARG_POINTER(aGMMsgId);
> +  NS_ENSURE_ARG_POINTER(aHdr);
> +  nsIMsgDBHdr  *msgHdr = nullptr;

nit: single space only

@@ +4605,5 @@
> +  result = m_mdbStore->StringToToken(GetEnv(), "X-GM-MSGID",
> +      &property_token);
> +  NS_ENSURE_SUCCESS(result, result);
> +  result = m_mdbStore->FindRow(GetEnv(), m_hdrRowScopeToken,
> +      property_token, &gMailMessageIdYarn, &outRowId,

nit: subsequent lines should be two-space indented or line up with the G just after open bracket on the line above.

@@ +4613,5 @@
> +    // Get key from row
> +    mdbOid outOid;
> +    rv = hdrRow->GetOid(GetEnv(), &outOid);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    nsMsgKey key= outOid.mOid_Id;

nit: space before =

::: mailnews/imap/public/nsIImapFlagAndUidState.idl
@@ +52,5 @@
>    void clearCustomFlags(in unsigned long uid);
> +  /**
> +   * Adds custom attributes to a hash table for the purpose of storing them
> +   * them.
> +   * @param aUid   UID of  the associated msg

nit: double space between of and the (same in the function below)

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +3191,5 @@
> +    aProtocol->GetFlagAndUidState(getter_AddRefs(flagState));
> +    nsCString msgIDValue;
> +    nsCString threadIDValue;
> +    nsCString labelsValue;
> +    flagState->GetCustomAttribute(m_curMsgUid,NS_LITERAL_CSTRING("X-GM-MSGID"), msgIDValue);

nit: space after comma (three places).

@@ +9539,5 @@
> +  NS_ENSURE_ARG_POINTER(_retval);
> +  nsresult rv;
> +  *_retval = false;
> +  nsCOMPtr<nsIMsgFolder> msgFolder;
> +  rv = GetOfflineMsgFolder(msgKey, getter_AddRefs(msgFolder));

nit: define nsresult on this line, as this is the first instance of it and you can assign straight away (same in GetOfflineMsgFolder)

@@ +9542,5 @@
> +  nsCOMPtr<nsIMsgFolder> msgFolder;
> +  rv = GetOfflineMsgFolder(msgKey, getter_AddRefs(msgFolder));
> +  if (NS_FAILED(rv))
> +    return NS_OK;
> +  if (msgFolder)

Why not combine this with the check above and say:

if (NS_SUCCEEDED(rv) && msgFolder)
  *_retval = true;

?

@@ +9596,5 @@
> +      nsCOMPtr<nsIMsgImapMailFolder> subFolder;
> +      for (PRUint32 i = 0; i < mLabelNames.Length(); i++)
> +      {
> +        rv = GetRootFolder(getter_AddRefs(rootFolder));
> +        if (NS_SUCCEEDED(rv)&& (rootFolder))

nit: space before &&

@@ +9617,5 @@
> +                                                 getter_AddRefs(subMsgFolder));
> +          if (mLabelNames[i].Equals("\"\\\\Sent\""))
> +             rv = rootFolder->GetFolderWithFlags(nsMsgFolderFlags::SentMail,
> +                                                 getter_AddRefs(subMsgFolder));
> +          if ((mLabelNames[i].Find(NS_LITERAL_CSTRING("[Imap]/"), true, 0, -1) != kNotFound))

Please use -1 rather than kNotFound, as -1 is compatible with the external API, which we support in some build cases.

::: mailnews/imap/src/nsImapServerResponseParser.cpp
@@ +1260,5 @@
>        }
> +      else if (!PL_strcasecmp(fNextToken, "X-GM-MSGID"))
> +      {
> +        AdvanceToNextToken();
> +        if (! fNextToken)

nit: no space after ! (several places)

@@ +1909,4 @@
>          PR_Free(fLastAlert);
>          fLastAlert = PL_strdup(alertMsg);
>        }
> +      PR_Free(alertMsg);

alertMsg is a pointer to an existing string owned by something else, so you shouldn't be freeing this.

::: mailnews/imap/test/unit/test_customCommandReturnsFetchResponse.js
@@ +4,4 @@
>  
>  /*
>   * Test to ensure that imap customCommandResult function works properly
> + * Bug 778246

Are test_customCommandReturnsFetchReponse.js and test_fetchCustomAttribute.js copies of test_gmailAttributes.js and test_gmailOfflineMsgStore.js?

If so, then you should use the hg copy function to create the files and apply the diffs from there. You'll need to make sure you have git style patches turned on, see here for details:

https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

(the reason we use hg copy, is to preserve history for these files, where they came from etc).
Attachment #654611 - Flags: superreview?(mbanner) → superreview-
Attached patch Unbitrotted, git patch (obsolete) — — Splinter Review 
This fixes the bitrot and I think sorts out the diffs and history for test_fetchCustomAttribute.js and test_gmailAttributes.js.

I couldn't figure out if the test_customCommandReturnsFetchResponse.js changes were meant as a replacement of the existing test or not.
I've also set off a try server build with the unbitrotted patch:

https://hg.mozilla.org/try-comm-central/rev/fb752785d10f
(In reply to Mark Banner (:standard8) from comment #47)
> Created attachment 655027 [details] [diff] [review]
> Unbitrotted, git patch
> 
> This fixes the bitrot and I think sorts out the diffs and history for
> test_fetchCustomAttribute.js and test_gmailAttributes.js.
> 
> I couldn't figure out if the test_customCommandReturnsFetchResponse.js
> changes were meant as a replacement of the existing test or not.
test_customCommandReturnsFetchResponse.js is another test, not a replacement of any test.
In this patch, I have made 3 new tests, test_customCommandReturnsFetchResponse.js, test_gmailAttributes.js, and test_gmailMsgOfflineStore.js. Also, I've modified test_fetchCustomAttribute.js to work well after adding my patch.
I'll upload another patch soon, with fixed nits.
(In reply to Atul Jangra from comment #49)
> (In reply to Mark Banner (:standard8) from comment #47)
> > Created attachment 655027 [details] [diff] [review]
> > Unbitrotted, git patch
> > 
> > This fixes the bitrot and I think sorts out the diffs and history for
> > test_fetchCustomAttribute.js and test_gmailAttributes.js.
> > 
> > I couldn't figure out if the test_customCommandReturnsFetchResponse.js
> > changes were meant as a replacement of the existing test or not.
> test_customCommandReturnsFetchResponse.js is another test, not a replacement
> of any test.
> In this patch, I have made 3 new tests,
> test_customCommandReturnsFetchResponse.js, test_gmailAttributes.js, and
> test_gmailMsgOfflineStore.js. Also, I've modified
> test_fetchCustomAttribute.js to work well after adding my patch.
> I'll upload another patch soon, with fixed nits.

Made one mistake. I've created two tests and modified two tests.
Created: test_gmailAttributes.js, and test_gmailMsgOfflineStore.js.
Modified:  test_fetchCustomAttribute.js, and test_customCommandReturnsFetchResponse.js.
Thus all these four tests should be present with modifications.
Attached patch Final Patch (obsolete) — — Splinter Review 
Fixed the nits pointed out by Standard8.
Also using git style diff as said by Standard8.
Attachment #654611 - Attachment is obsolete: true
Attachment #654611 - Flags: review?(mozilla)
Attachment #655231 - Flags: superreview?(mbanner)
Attachment #655231 - Flags: review?(mozilla)
this patch failed to apply for me -
patching file mailnews/imap/src/nsImapFlagAndUidState.cpp
Hunk #1 FAILED at 79
Hunk #2 succeeded at 303 with fuzz 2 (offset 0 lines).
1 out of 2 hunks FAILED -- saving rejects to file mailnews/imap/src/nsImapFlagAn
dUidState.cpp.rej
patching file mailnews/imap/src/nsImapFlagAndUidState.h
Hunk #1 FAILED at 36
1 out of 1 hunks FAILED -- saving rejects to file mailnews/imap/src/nsImapFlagAn
dUidState.h.rej
patching file mailnews/imap/src/nsImapMailFolder.cpp
Hunk #1 FAILED at 3054
1 out of 3 hunks FAILED -- saving rejects to file mailnews/imap/src/nsImapMailFo
lder.cpp.rej
patching file mailnews/imap/src/nsImapMailFolder.h
Hunk #3 FAILED at 492
1 out of 3 hunks FAILED -- saving rejects to file mailnews/imap/src/nsImapMailFo
lder.h.rej
patching file mailnews/imap/src/nsImapProtocol.cpp
Hunk #4 FAILED at 4219
1 out of 5 hunks FAILED -- saving rejects to file mailnews/imap/src/nsImapProtoc
ol.cpp.rej
patching file mailnews/imap/src/nsImapProtocol.h
Hunk #1 FAILED at 569
1 out of 1 hunks FAILED -- saving rejects to file mailnews/imap/src/nsImapProtoc
ol.h.rej
patching file mailnews/imap/src/nsImapServerResponseParser.h
Hunk #1 FAILED at 235
1 out of 1 hunks FAILED -- saving rejects to file mailnews/imap/src/nsImapServer
ResponseParser.h.rej
patching file mailnews/imap/test/unit/xpcshell.ini
Hunk #1 FAILED at 1
1 out of 1 hunks FAILED -- saving rejects to file mailnews/imap/test/unit/xpcshe
ll.ini.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh name.patch
I'll fix the patch soon.
Attached patch Final Patch (obsolete) — — Splinter Review 
Final patch - git style. 
Applies fine locally.
All the tests are passing without any leak.
Attachment #655231 - Attachment is obsolete: true
Attachment #655231 - Flags: superreview?(mbanner)
Attachment #655231 - Flags: review?(mozilla)
Attachment #655385 - Flags: superreview?(mbanner)
Attachment #655385 - Flags: review?(mozilla)
Comment on attachment 655385 [details] [diff] [review]
Final Patch

+      nsTArray<nsCString> mLabelNames;

getting very close. A few nits:

shouldn't use m as the first char of the var name since that implies it's a member var.

nsImapServerResponseParser has some inconsistent line endings, according to dev studio.

-}
\ No newline at end of file
+}

should fix this.
Attached patch Final Patch (obsolete) — — Splinter Review 
Final patch - git style. 
Applies fine locally.
All the tests are passing without any leak.
Attachment #655385 - Attachment is obsolete: true
Attachment #655385 - Flags: superreview?(mbanner)
Attachment #655385 - Flags: review?(mozilla)
Attachment #656202 - Flags: superreview?(mbanner)
Attachment #656202 - Flags: review?(mozilla)
Comment on attachment 656202 [details] [diff] [review]
Final Patch

thx for fixing my last round of comments!
Attachment #656202 - Flags: review?(mozilla) → review+
If this is acceptable/low risk patch, it would be great in a week or two to drive this into aurora/TB17 and this get it on ESR17 for the perf and disk space win.  http://tinyurl.com/d7tqc8q

Atul, nit: "response" is misspelled twice as "respone" test_fetch...
Attached patch Final Patch v2 (obsolete) — — Splinter Review 
Fixes the spelling mistake.
Rest ready to go!!
Attachment #657837 - Flags: superreview?(mbanner)
Atul. I know you are anxious to get this bug finished. I wish I could have made time for this sooner. :-( 
Here it goes...

I have a number of suggestions/ideas to take into consideration.

The first two are just nits - am I allowed to have nits? :-)

1.  In mailnews/imap/public/nsIImapFlagAndUidState.idl
    You have added two complementary functions
    > +  void addUidCustomAttribute(in unsigned long aUid,
    and 
    > +  ACString getCustomAttribute(in unsigned long aUid,
    
    One function has 'Uid' in the name and one does not. Shouldn't both function
    names be identical except for add/get?
    Same with mailnews/imap/src/nsImapFlagAndUidState.cpp
  
  
2.  In mailnews/test/resources/IMAPpump.js, you removed the requirement for a
    prefix when mixing in extensions on the imap fakeserver.
    > -        if (part.substring(0, 3) == "RFC")
    > -          mixinExtension(handler, eval("IMAP_" + part + "_extension"));
    > +        mixinExtension(handler, eval("IMAP_" + part + "_extension"));

    It would be nice to clean up RFCMOVE, RFCCUSTOM and RFCGMAIL so that they
    are just MOVE, CUSTOM and GMAIL in mailnews/test/fakeserver/imapd.js 
    > -  CUSTOM1: ["RFCMOVE", "RFC4315"],
    > -  GMail: ["RFCGMAIL", "RFC2197", "RFC4315"]
    > +  CUSTOM1: ["RFCMOVE", "RFC4315", "RFCCUSTOM"],
    > +  GMail: ["XLIST", "RFCGMAIL", "RFC2197", "RFC4315"]
    
    
3.  "[Gmail]/Important" is missing in mailnews/test/fakeserver/imapd.js 
    > +      this._daemon.createMailbox("[Gmail]/All Mail", {subscribed : true});
    > +      this._daemon.createMailbox("[Gmail]/Sent Mail", {subscribed : true});
    > +      this._daemon.createMailbox("[Gmail]/Drafts", {subscribed : true});
    > +      this._daemon.createMailbox("[Gmail]/Starred", {subscribed : true});
    > +      this._daemon.createMailbox("[Gmail]/Spam", {subscribed : true});
    > +    }
    > +    var people = base.matchKids(args[1]);
    > +    var response = "";
    > +    var specialFolderFlagsLookupTable = {
    > +      "[Gmail]/All Mail": "AllMail",
    > +      "[Gmail]/Drafts": "Drafts",
    > +      "[Gmail]/Sent Mail": "Sent",
    > +      "[Gmail]/Starred": "Starred",
    > +      "[Gmail]/Spam": "Spam",
    > +      "INBOX": "Inbox"    
    
    Gmail XLIST considers Important as a special folder just as the reset of
    these.
    
4.  I think you need more that just ParseString(...) in
    mailnews/imap/src/nsImapMailFolder.cpp
    > +nsresult nsImapMailFolder::GetOfflineMsgFolder(nsMsgKey msgKey, nsIMsgFolder **aMsgFolder)
    > ...
    > ...
    > +      ParseString(labels, ' ', labelNames);
    
    Gmail labels can have spaces in them. I think in one of my previous comments
    I have a crazy regex that does the job. There may be some function in TB
    that I don't know about that accomplishes the same thing. Hopefully someone
    will chime in if there is.
    
5.  As in 3., "\\Important" needs to be handled in
    mailnews/imap/src/nsImapMailFolder.cpp
    > +          if (labelNames[i].Equals("\"\\\\Draft\""))
    > +             rv = rootFolder->GetFolderWithFlags(nsMsgFolderFlags::Drafts,
    > +                                                 getter_AddRefs(subMsgFolder));
    > +          if (labelNames[i].Equals("\"\\\\Inbox\""))
    > +             rv = rootFolder->GetFolderWithFlags(nsMsgFolderFlags::Inbox,
    > +                                                 getter_AddRefs(subMsgFolder));
    > +          if (labelNames[i].Equals("\"\\\\All Mail\""))
    > +             rv = rootFolder->GetFolderWithFlags(nsMsgFolderFlags::Archive,
    > +                                                 getter_AddRefs(subMsgFolder));
    > +          if (labelNames[i].Equals("\"\\\\Trash\""))
    > +             rv = rootFolder->GetFolderWithFlags(nsMsgFolderFlags::Trash,
    > +                                                 getter_AddRefs(subMsgFolder));
    > +          if (labelNames[i].Equals("\"\\\\Spam\""))
    > +             rv = rootFolder->GetFolderWithFlags(nsMsgFolderFlags::Junk,
    > +                                                 getter_AddRefs(subMsgFolder));
    > +          if (labelNames[i].Equals("\"\\\\Sent\""))
    > +             rv = rootFolder->GetFolderWithFlags(nsMsgFolderFlags::SentMail,
    > +                                                 getter_AddRefs(subMsgFolder));
    > +          if ((labelNames[i].Find(NS_LITERAL_CSTRING("[Imap]/"), true, 0, -1) != -1))
    > +          {
    > +            labelNames[i].ReplaceSubstring("[Imap]/", "");
    > +            imapRootFolder->FindOnlineSubFolder(labelNames[i], getter_AddRefs(subFolder));
    > +            subMsgFolder = do_QueryInterface(subFolder);
    > +          }
    
    FETCH X-GM-LABELS return "\\Important" instead of the localized folder name.
    
    Adding the case for "\\Important" here is non-trivial because there is not a
    nsMsgFolderFlags for Important.
    
6.  Still in mailnews/imap/src/nsImapMailFolder.cpp
    > +nsresult nsImapMailFolder::GetOfflineMsgFolder(nsMsgKey msgKey, nsIMsgFolder **aMsgFolder)
    You don't need to check for "\\All Mail", "\\Trash" or "\\Spam" labels
    because they are never returned by the server. It is assumed that all
    messages are in one of these three folders exclusively.
    
    In fact, you could add some additional conditions here...
    - If the current folder is the Trash folder or the Spam folder, then skip
      iterating the labels because if a message is in Trash or Spam, it still
      has the labels of other folders, but will not actually appear in any other
      folder
    - If the current folder is not Trash or Spam or All Mail, then also check
      the All Mail folder for the offline copy of the message.
    
    
7.  Since we are now specifically looking for X-GM-xxxx, we should change the
    isGmailServer flag to look for the X-GM-EXT-1 server capability.
    https://developers.google.com/google-apps/gmail/imap_extensions

    Currently, isGmailServer is determined by the presence of the AllMail folder
    in XLIST response
    > // only GMail will have an AllMail folder.
    > if (boxFlags & kImapAllMail)
    >   SetIsGMailServer(true);
    I use isGmailServer in an extension and had a problem with the current
    implementation.
    See https://github.com/dlech/gmailbuttons/issues/4
    
    
8.  As far as I know, the X-GM-MSGID and X-GM-THRID attributes don't change for 
    a message, so only checking them once is OK. X-GM-LABELS on the other hand
    can change at any time. For example, if a user changes labels in the gmail
    web interface, the changes should be updated in thunderbird. I think the best
    way to do this would be to fetch X-GM-LABELS whenever you fetch FLAGS.
    
    
9.  I have updated my Gmail Buttons extension to use this patch.
    https://github.com/dlech/gmailbuttons/tree/tb17
    
    It should be a good tool for trying out real life scenarios with this patch.
    It displays the X-GM-MSGID, X-GM-THRID and X-GM-LABELS custom attributes.
        
    So far, I have noticed that not all messages are showing X-GM-MSGID and 
    X-GM-THRID. Also, the X-GM-LABELS are not working as they should - most
    likely because of what I mentioned in 8.
    
    I don't know a way of displaying the offline mail folder at this point, 
    which prevents me from seeing everything that is going on. Do we want to
    make nsImapMailFolder::GetOfflineMsgFolder(...) accessible to extensions?
Comment on attachment 657837 [details] [diff] [review]
Final Patch v2

Review of attachment 657837 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay in finishing this, I was having trouble seeing the patch take effect. I've now seen that, and this looks good.

Re David's comments, I think a couple of them need fixing before we land this, see my comments below. I think the rest we can cover as follow-up bugs as they will just mean that we're not as de-duplicating email storage as efficiently could be.

::: mailnews/imap/public/nsIImapFlagAndUidState.idl
@@ +56,5 @@
> +   * @param aUid   UID of the associated msg
> +   * @param aCustomAttributeName   Name of the custom attribute value
> +   * @param aCustomAttributeValue   Value of the attribute,
> +   */
> +  void addUidCustomAttribute(in unsigned long aUid,

As per David's suggestion, lets go with setCustomAttribute then we've got set/get matching.

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +3063,5 @@
> +
> +  nsCOMPtr<nsIImapIncomingServer> imapServer = do_QueryInterface(server);
> +  rv = imapServer->GetIsGMailServer(&m_isGmailServer);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  

nit: unnecessary whitespace here.
Attachment #657837 - Flags: superreview?(mbanner) → superreview+
Attachment #656202 - Attachment is obsolete: true
Attachment #656202 - Flags: superreview?(mbanner)
Attached patch Final Patch v3 (obsolete) — — Splinter Review 
Fixed the nits :-)
Attachment #631748 - Attachment is obsolete: true
Attachment #632460 - Attachment is obsolete: true
Attachment #635845 - Attachment is obsolete: true
Attachment #636018 - Attachment is obsolete: true
Attachment #637805 - Attachment is obsolete: true
Attachment #639847 - Attachment is obsolete: true
Attachment #648862 - Attachment is obsolete: true
Attachment #655027 - Attachment is obsolete: true
Attachment #657837 - Attachment is obsolete: true
Attachment #665437 - Flags: superreview?(mbanner)
Attachment #665437 - Flags: feedback?
Attached patch Final Patch v3 — — Splinter Review 
Fixed the nits, and improved the silly mistake while uploading the last attachment.
Attachment #665437 - Attachment is obsolete: true
Attachment #665437 - Flags: superreview?(mbanner)
Attachment #665437 - Flags: feedback?
Keywords: checkin-needed
I've checked this in: https://hg.mozilla.org/comm-central/rev/bb45e728a12e

I did a couple of tweaks to fix alignment where you'd changed the function name, and to put the review/superreview items in the comment in the standard format that we use.

When you file the new bug(s) for the follow-up issues, please mark this one as fixed.
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 18.0
Thanks a lot :-)
I'll file th new bugs after a detailed discussion with Bienvenu and David Lechner. :-)
Fantastic work! I'm really excited to see this landing :).
Attached patch External API fixes — — Splinter Review 
Compatibility fixes:
* Used CaseInsensitiveCompare instead of true
* Used MsgReplaceSubstring instead of ReplaceSubstring
Miscellaneous fixes:
* Removed extraneous parentheses
* Removed default arguments to Find (thus avoiding using MsgFind)
Attachment #666359 - Flags: review?(mbanner)
Attachment #666359 - Flags: review?(mbanner) → review+
Neil, Thanks for those edits. :-)
Standard8 I've filed the follow-up bug: https://bugzilla.mozilla.org/show_bug.cgi?id=798145
I've planned to keep Bug 798145 as a single bug to make other enhancements. We also have an option to maintain a set of bugs on Bug 798145 for follow-up from there.
After a discussion on IRC, I'll mark this one as fixed.
Thanks :-)
why is this bug still open if things have been pushed ?
(In reply to Atul Jangra [:atuljangra] from comment #69)
> I've planned to keep Bug 798145 as a single bug to make other enhancements.
> We also have an option to maintain a set of bugs on Bug 798145 for follow-up
> from there.

I'd suggest using that bug for tracking the issues around David's comments (which could be spread across multiple bugs), and then use a separate set of bugs for enhancements (like fully working THRID etc).
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
(In reply to Mark Banner (:standard8) from comment #71)
> I'd suggest using that bug for tracking the issues around David's comments
> (which could be spread across multiple bugs), and then use a separate set of
> bugs for enhancements (like fully working THRID etc).

Okay :-)
Time for a little celebration(after my exams :( )
Blocks: 798635
No longer blocks: 798635
(In reply to Atul Jangra [:atuljangra] from comment #72)
> Time for a little celebration

I think this calls for a woo-hoo or a little \o/ !

(In reply to David Lechner [:dlech] from comment #60)

1. Already fixed
2. I'm working on another bug where I can clean this up, so no new bug required.
3. Combined with 5.
4. Bug 798635
5. Bug 798648
6. Bug 798659
7. Bug 798663
8. Bug 798667
9. Bug 798669
(In reply to David Lechner (:dlech) from comment #73)
> (In reply to Atul Jangra [:atuljangra] from comment #72)
> > Time for a little celebration
> 
> I think this calls for a woo-hoo or a little \o/ !
> 
> (In reply to David Lechner [:dlech] from comment #60)
> 
> 1. Already fixed
> 2. I'm working on another bug where I can clean this up, so no new bug
> required.
> 3. Combined with 5.
> 4. Bug 798635
> 5. Bug 798648
> 6. Bug 798659
> 7. Bug 798663
> 8. Bug 798667
> 9. Bug 798669

Thanks for the bugs :-)
Comment on attachment 665446 [details] [diff] [review]
Final Patch v3

[Triage Comment]
Ok, after much debate that I was hoping to have completed before the end of the aurora cycle. We have decided to move this forward to TB 17. This is because of the expected long cycle for the next release, and the big win to users who will no longer have to store double or more of their real gmail.

Whilst we recognise this changes interfaces, it only adds new attributes/functions, and we are doing this before the start of the beta cycle, which we expect to be the main start of add-on updates for releases.

The primary motivation is the win for users. However, if we find any significant issues, then we'll back it out or disable it.
Attachment #665446 - Flags: approval-comm-beta+
Attachment #666359 - Flags: approval-comm-beta+
Do we have a bug on file for using X-GM-THRID to improve threading in the thread pane?
(In reply to Jonathan Protzenko [:protz] from comment #76)
> Do we have a bug on file for using X-GM-THRID to improve threading in the
> thread pane?

What does that get us that handling threading on our own can't?
(In reply to Jim Porter (:squib) from comment #77)
> 
> What does that get us that handling threading on our own can't?

For one, better user experience. Thunderbird threads are guaranteed to exactly match gmail web client.
(In reply to David Lechner (:dlech) from comment #78)
> (In reply to Jim Porter (:squib) from comment #77)
> > 
> > What does that get us that handling threading on our own can't?
> 
> For one, better user experience. Thunderbird threads are guaranteed to
> exactly match gmail web client.

Ugh for my bugmail, as an example, I have always hated the way GMail is broken with regard to threading. Since Gmail doesn't tie the New: * to the rest of the bugmail, while TB and SeaMonkey do.

Also there are many examples of GMail merging multiple bugs into one thread if they "seem" similar. (e.g. if I file a l10n bug for each l10n team, or create new bugs for IT to do stuff, that look the same, but are tied to different bug dependancies, etc.)

This is stuff where GMail ignores the threading hints that Bugzilla Sends.
Conversely, I often start email discussions with people who have bad MUAs (e.g. mobile), which do not implement References: at all (K9-Mail, Blackberry, etc.). In that case, the Thunderbird threading is completely broken, while the GMail one is just right. Maybe a per-folder setting (use GMail hints for threading ?) would be enough? (I assume you filter your bugmail to a specific folder...).
Yeah, using X-GM-THRID would be really useful sometimes. I don't think there's a bug about it yet.
Also I think Cyrus has something similar in the works (can't remember where I read that discussion though, asuth would know), i.e. server-side thread analysis and thread IDs queryable via IMAP. I think we should open a more general bug about it, although gmail imap servers are probably more common and more widespread.
Gmail & bugzilla may be bug 721549.

Please move the discussion about X-GM-THRID to mailing lists / another bug as this one is now fixed.
(In reply to Mark Banner (:standard8) from comment #75)
> Comment on attachment 665446 [details] [diff] [review]
> Final Patch v3
> 
> [Triage Comment]
> Ok, after much debate that I was hoping to have completed before the end of
> the aurora cycle. We have decided to move this forward to TB 17. This is
> because of the expected long cycle for the next release, and the big win to
> users who will no longer have to store double or more of their real gmail.
> 
> Whilst we recognise this changes interfaces, it only adds new
> attributes/functions, and we are doing this before the start of the beta
> cycle, which we expect to be the main start of add-on updates for releases.
> 
> The primary motivation is the win for users. However, if we find any
> significant issues, then we'll back it out or disable it.

Woot. Thanks :-) I had a little discussion with wsmwk regarding the same approval. :-)
(In reply to Mark Banner (:standard8) from comment #83)
> Please move the discussion about X-GM-THRID to mailing lists / another bug
> as this one is now fixed.

Fair, I misunderstood my initial bugmail reading here after being CC'ed and thought the GMail threading switch was part of what landed.
Blocks: 800003
No longer blocks: 475857
Depends on: 815087
Depends on: 816327
Depends on: 854161
Blocks: 854161
No longer depends on: 854161
No longer blocks: 854161
Depends on: 854161
Depends on: 854162
X-GM-THRID seems to be being stored in a format different than what Gmail expects. If I do  hdr.getStringProperty("X-GM-THRID") on a given message I get 1436691482557425105, but the thread URL in GMail is https://mail.google.com/mail/u/0/#inbox/13f0274a9daa25d1
The same seems to be true for X-GM-MSGID also. It returns 1436580263914636083 when the actual X-GM-MSGID is 13efc22381f80333.
+David,

Decimal vs Hex
Yes, thanks. And sorry I posted before I finished looking at all the options (I forgot to eat lunch :). That said, I've tried using parseInt(hdr.getStringProperty("X-GM-THRID")).toString(16)), but I seem to be missing some digits on the end. For example, I will get 13f02ba6cc5d9f82 from 1436696276524441474, but the Gmail URL will have 13f02ba6cc5d9f82. I started a thread here to discuss - http://forums.mozillazine.org/viewtopic.php?f=19&t=2714021.
Argh, I finally realized I'm running into a JS precision problem - http://www.danvk.org/wp/2012-01-20/accurate-hexadecimal-to-decimal-conversion-in-javascript/. So toString(16) just isn't good enough here. Sorry for all the comments...
See Also: → 839835
See Also: → 1680004
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: