406169 - Move some of nsIDBFolderInfo to ns*String and help migration to frozen linkage

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Mark Banner (:standard8)]

Attachment: The fix

I was looking at migrating some more of mailnews/base to frozen linkage and it quickly became clear that moving some more of the nsIDBFolderInfo from string to nsCString would be a lot more sensible than reworking the code twice.

Hence patch attached does some string tidy up in nsIDBFolderInfo (and the associated changes) and there's a couple of small frozen linkage changes in there.

Note, I also decided it was approprate to rename {G,S}etCharPtrProperty to {G,S}CharProperty now we're no longer using string pointers there.

Neil, can you review the mailnews changes, Phil could you review the TB-specific changes?

Comment 1

[Mark Banner (:standard8)]

Attachment: The fix v2

Correct version, this has an optimisation to an if statement that I forgot to regenerate the patch for.

Comment 2

[[e-mail]]

Comment on attachment 290875 [details] [diff] [review]
The fix v2

>+  ACString getCharProperty(in ACString propertyName);
>+  void setCharProperty(in ACString aPropertyName, in ACString aPropertyValue);
Although it is correct to make the values ACString, the property names themselves should stay string.

>   void getCharacterSet(out ACString charSet, out boolean overriden);
>-  void setCharacterSet(in string charSet);
>+  void setCharacterSet(in ACString charSet);
> 
>   attribute boolean characterSetOverride;
> 
>-  string getCharPtrCharacterSet();
>+  readonly attribute ACString charPtrCharacterSet;
I don't see anyone using getCharacterSet, so I would change those three methods into an attribute ACString characterSet.

>+static nsCString gDefaultCharacterSet;
No static strings. Try making it a member of gFolderCharsetObserver instead?

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 290875 [details] [diff] [review]
The fix v2

I'll update the patch in a bit.

Comment 4

[[e-mail]]

OK, so as Mark pointed out nsMsgDBFolder.cpp does uses GetCharacterSet, but my MXR search didn't find it because I searched case-sensitively for FolderInfo.

folderInfo->GetCharacterSet(mCharset, &defaultUsed);
if (defaultUsed)
  mCharset.Truncate();

Since this caller only wants the folder info's m_charSet (and not the gDefaultCharacterSet) it makes sense to use attribute ACString characterSet; and change this call to folderInfo->GetCharacterSet(mCharset);

We can then replace the uses of Get[Const]CharPtrCharacterSet with a readonly attribute, perhaps call it effectiveCharacterSet?

Comment 5

[Mark Banner (:standard8)]

Attachment: The fix v3

Revised patch as per discussion with Neil and to fix the static string problem.

Comment 6

[[e-mail]]

Comment on attachment 291225 [details] [diff] [review]
The fix v3

>-  nsCString     m_charSet;
I don't claim to understand the perf/bloat aspects of this change...

>-          if (gDefaultCharacterSet)
>-            nsMemory::Free(gDefaultCharacterSet);
>-          gDefaultCharacterSet = ToNewCString(ucsval);
>+          *gDefaultCharacterSet = NS_ConvertUTF16toUTF8(ucsval);
You don't null-check gDefaultCharacterSet. Also, please use CopyUTF16toUTF8.

>+          if (!gDefaultCharacterSet)
>+            gDefaultCharacterSet = new nsCString;
>+
>+          *gDefaultCharacterSet = NS_ConvertUTF16toUTF8(ucsval);
Same here - the new might fail...

Comment 7

[[e-mail]]

Comment on attachment 291225 [details] [diff] [review]
The fix v3

> NS_IMETHODIMP
>-nsDBFolderInfo::GetCharPtrCharacterSet(char **result)
>+nsDBFolderInfo::GetEffectiveCharacterSet(nsACString &result)
> {
>-  *result = ToNewCString(m_charSet);
This gets called a lot, particularly from nsMsgDBView::GetCellText.
Removing m_charSet isn't really an option.

Comment 8

[Mark Banner (:standard8)]

Attachment: The fix v4

Updated per Neil's comments.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 291949 [details] [diff] [review]
The fix v4

Sorry, attached the wrong patch :-(

Comment 10

[Mark Banner (:standard8)]

Attachment: The fix v4

The right patch this time.

Comment 11

[[e-mail]]

Comment on attachment 292052 [details] [diff] [review]
The fix v4

>+            CopyUTF16toUTF8(ucsval, *gDefaultCharacterSet);
Right :-)

>+            *gDefaultCharacterSet = NS_ConvertUTF16toUTF8(ucsval);
Wrong ;-)

>+  if (NS_FAILED(GetCharProperty(kCharacterSetColumnName, result)) ||
>+      result.IsEmpty() && gDefaultCharacterSet)
You need extra parens here because && has higher precedence than ||.
r+sr=me with that fixed.

Comment 12

[Phil Ringnalda (:philor)]

Comment on attachment 292052 [details] [diff] [review]
The fix v4

r=me on the parts I understand ;)

Comment 13

[Mark Banner (:standard8)]

Patch checked in -> fixed.