The GetMessageKey() call is redundant and should be removed.
Sorry, for clicking submit button too quickly. I mean the call at http://mxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/nsMsgThread.cpp#209 is redundant.
Comment on attachment 489124 [details] [diff] [review] patch > nsCOMPtr <nsIMsgDBHdr> ancestorHdr = newParentOfOldRoot; > nsMsgKey newRoot; >- newParentOfOldRoot->GetMessageKey(&newRoot); > mdb_pos outPos; > > nsMsgKey newHdrAncestor; > ancestorHdr->GetMessageKey(&newRoot); This seems wrong, looking further below newHdrAncestor is not initialised and used uninitialised. However, these newRoot and newHdrAncestor would then end up the same. That might be right, but then we could just assign rather than getting the message key twice. Although it also could be that just getting the newHdrAncestor is redundant as you've got the patch. In which case I would tend to reorder the variables a bit. So I'm not quite sure which shape this function is meant to be in. So I'm going to pass the review to David and let him have a look.
newHdrAncestor is initialized here: ancestorHdr->GetThreadParent(&newHdrAncestor); ThreadParent is not the same as MessageKey, so I don't think newHdrAncestor and newRoot are the same. So this patch looks OK, other than perhaps moving the decl of newRoot to just before it's used.
Created attachment 489418 [details] [diff] [review] move the decl of newRoot before its first reference
Comment on attachment 489418 [details] [diff] [review] move the decl of newRoot before its first reference thx for the patch.