161085 - get rid of m_onlineBaseFolderExists, it's not used

Status: VERIFIED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Henry Jia]

In nsImapProtocol::DiscoverMailboxSpec in
mozilla/mailnews/imap/src/nsImapProtocol.cpp, mozilla only handles the specific
namespace delimiter '/'. This is not correct. See the code below.

  const char *nsPrefix = ns ? ns->GetPrefix() : 0;
  
  nsCString canonicalSubDir;
  if (nsPrefix)
  {
    PRUnichar slash = '/';
    canonicalSubDir = nsPrefix;
    if (canonicalSubDir.Length() && canonicalSubDir.Last() == slash)
      canonicalSubDir.SetLength((PRUint32) canonicalSubDir.Length()-1);
  }

Comment 1

[Henry Jia]

Attachment: patch for review

Comment 2

[Henry Jia]

Attachment: patch for review

Make canonicalSubDir in canonical format.

Comment 3

[David :Bienvenu]

setting the hierarchy delimiter to 0 doesn't seem like a good idea, since it
will never work:
+    char delimiter = ns ? ns->GetDelimiter() : 0;

what's the bug that occurs because of this?

Comment 4

[Henry Jia]

1. setting the hierarchy delimiter to 0 doesn't seem like a good idea, since it
will never work:
+    char delimiter = ns ? ns->GetDelimiter() : 0;

Here we are handling namespace prefix. We want the canonical format of the
prefix with the last delimiter omitted. I set delimiter to 0 (where there is no
namespace) just for the continuous checking to see if we should go on to change
the prefix's format.

Indeed, after I re-checked the patch, there is no need to check if ns is nsnull,
because if nsPrefix is not nsnull, ns must exist. I'll give a new patch soon.

2. what's the bug that occurs because of this?
We uses namespace's prefix to check if we should set m_onlineBaseFolderExists to
true, if we don't change it to canonical format, "m_onlineBaseFolderExists =
PR_TRUE" will never be executed when the delimiter is not '/'. Though I really
don't see the error caused by this, from the theory, it's not correct.

Comment 5

[Henry Jia]

Attachment: new patch for review

Comment 6

[Henry Jia]

Add navin & cavin to CC list.

Pls r= the new patch. Thx.

Comment 7

[Navin Gupta]

Comment on attachment 94281 [details] [diff] [review]
new patch for review

r=naving

Comment 8

[philip zhao]

We didn't use m_onlineBaseFolderExists at all. Onle set 0 or 1 to it.

Comment 9

[Henry Jia]

What do you mean, Philip?

Comment 10

[philip zhao]

m_onlienBaseFolderExists only can be seen in these lines.

/mailnews/imap/src/nsImapProtocol.cpp, line 405 -- m_onlineBaseFolderExists =
PR_FALSE;
/mailnews/imap/src/nsImapProtocol.cpp, line 4156 -- m_onlineBaseFolderExists =
PR_TRUE;
/mailnews/imap/src/nsImapProtocol.cpp, line 4271 -- m_onlineBaseFolderExists =
PR_TRUE;
/mailnews/imap/src/nsImapProtocol.h, line 652 -- PRBool m_onlineBaseFolderExists;

So I think it won't lead to errors if we didn't set it. Onle(only) so:-)

Comment 11

[Henry Jia]

Philip, you're right. We don't use m_onlineBaseFolderExists now.

Hi, all, who know what is m_onlineBaseFolderExists ever used for? Or What will
m_onlineBaseFolderExists be potentially used for? If it doesn't have any usage
now, let's get rid of it. If it is potentially for some usage, then the patch
(attachment 94281 [details] [diff] [review]) is needed. It correctly handle the conanical conversion. Here
is the data output from debug without the patch.

(gdb) p adoptedBoxSpec->allocatedPathName
$4 = 0x8a28468 "INBOX/Trash"
(gdb) p canonicalSubDir
$5 = {<nsAFlatCString> = {<nsASingleFragmentCString> = {<nsACString> = {
        _vptr.nsACString = 0x402cd9c8}, <No data fields>}, <No data fields>},
<nsStr> = {{
      mStr = 0x8d445c0 "INBOX.", mUStr = 0x8d445c0}, mLength = 6,
    mCapacityAndFlags = 1073741830}, <No data fields>}

It is obvious that the conversion hasn't been done.

Comment 12

[David :Bienvenu]

we should just remove that variable. It wasn't even used in 4.x, so I think it's
safe to remove it.

Comment 13

[Henry Jia]

Change the summary according to the comments above.

Comment 14

[Henry Jia]

Attachment: patch for review

Here is the patch.

naving & David, please r= & sr=. Thx.

Comment 15

[Henry Jia]

Comment on attachment 106046 [details] [diff] [review]
patch for review

Please r= & sr=. Thx.

Comment 16

[David :Bienvenu]

Comment on attachment 106046 [details] [diff] [review]
patch for review

sr=bienvenu

Comment 17

[Antonio.Xu]

Comment on attachment 106046 [details] [diff] [review]
patch for review

Ok, looks good to me.:-)

Comment 18

[Henry Jia]

In trunk.

Comment 19

[Stephen Donner]

verified fixed using lxr (code removal verification)