Mark Banner (:standard8) Assignee
	Description • 17 years ago

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?

	Mark Banner (:standard8) Assignee
	Comment 1 • 17 years ago

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

	neil@parkwaycc.co.uk
	Comment 2 • 17 years ago

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?

	Mark Banner (:standard8) Assignee
	Comment 3 • 17 years ago

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

I'll update the patch in a bit.

	neil@parkwaycc.co.uk
	Comment 4 • 17 years ago

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?

	Mark Banner (:standard8) Assignee
	Comment 5 • 17 years ago

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

	neil@parkwaycc.co.uk
	Comment 6 • 17 years ago

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...

	neil@parkwaycc.co.uk
	Comment 7 • 17 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 8 • 17 years ago

Updated per Neil's comments.

	Mark Banner (:standard8) Assignee
	Comment 9 • 17 years ago

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

Sorry, attached the wrong patch :-(

	Mark Banner (:standard8) Assignee
	Comment 10 • 17 years ago

The right patch this time.

	neil@parkwaycc.co.uk
	Comment 11 • 17 years ago

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.

	Phil Ringnalda (:philor)
	Comment 12 • 17 years ago

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

r=me on the parts I understand ;)

	Mark Banner (:standard8) Assignee
	Comment 13 • 17 years ago

Patch checked in -> fixed.