257386 - Poorly written code in nsImapMailFolder.cpp

Status: VERIFIED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Ugly code.

Comment 1

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Make code nicer

Comment 2

[Ere Maijala (slow)]

Comment on attachment 157373 [details] [diff] [review]
Make code nicer

Why not clean up the usage of rv while at it?

Comment 3

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch v2

Comment 4

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Do we care about the return value here?  It sets rv, but returns openErr
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/imap/src/nsImapMailFolder.cpp#2005

Comment 5

[David :Bienvenu]

we want to return openErr. That's what really indicates the success or failure
of opening the db...

Comment 6

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

I was actually asking about the line:
rv = (*folderInfo)->SetProperty("onlineName", autoOnlineName);

Do we care about rv?  The current code ignores it.

Comment 7

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch v3

Comment 8

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch v3.1

Comment 9

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch v3.2

<biesi-away> that could deal with some vertical whitespace
<biesi-away> s/deal with/use/

Comment 10

[Ere Maijala (slow)]

Comment on attachment 157897 [details] [diff] [review]
Patch v3.2

> +  if(!db || !folderInfo || !mDatabase)

You can't check mDatabase before you have called OpenDatabase().

> +  if (NS_FAILED((*folderInfo)->GetCharPtrProperty("onlineName", getter_Copies(onlineName))))
> +      return NS_ERROR_UNEXPECTED;

Could we return the real error code that GetCharPtrProperty returned?

Comment 11

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch v4

Comment 12

[[e-mail]]

Comment on attachment 157979 [details] [diff] [review]
Patch v4

>+  rv = (*folderInfo)->GetCharPtrProperty("onlineName", getter_Copies(onlineName));
>+  NS_ENSURE_SUCCESS(rv, rv);
Nit: don't use this, use
if (NS_FAILED(rv))
  return rv;

Comment 13

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Unrotted and checked in.

Comment 14

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED via code inspection (LXR).