466613 - Remove nsStringArray from mailnews

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Fred Jendrzejewski]

we started to replace nsStringArray by nsTArray<nsString> in bug #461047
Now they have to be removed from mailnews to remove the class completely.

Comment 1

[Fred Jendrzejewski]

Attachment: replace - v1

Replaced every array

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 349947 [details] [diff] [review]
replace - v1

> BOOL nsAbWinHelper::GetPropertiesUString(const nsMapiEntry& aObject, const ULONG *aPropertyTags,
>-                                         ULONG aNbProperties, nsStringArray& aNames)
>+                                         ULONG aNbProperties, nsTArray<nsString>& aNames)
> {
>     aNames.Clear() ;
>     LPSPropValue values = NULL ;
>     ULONG valueCount = 0 ;
> 
>     if (!GetMAPIProperties(aObject, aPropertyTags, aNbProperties, values, valueCount)) { 
>         return FALSE ; 
>     }
>     if (valueCount == aNbProperties && values != NULL) {
>         ULONG i = 0 ;
> 
>         for (i = 0 ; i < valueCount ; ++ i) {
>             if (PROP_ID(values [i].ulPropTag) == PROP_ID(aPropertyTags [i])) {
>                 if (PROP_TYPE(values [i].ulPropTag) == PT_STRING8)
>-                    aNames.AppendString(NS_ConvertASCIItoUTF16(values [i].Value.lpszA));
>+                    aNames.AppendElement(NS_ConvertASCIItoUTF16(values [i].Value.lpszA));

So once we get to the start of the for loop, we know how many elements we're appending to the array. So lets use .SetCapacity(valueCount) to minimise the reallocations of the array.

>     // Get multiple string MAPI properties in one call.
>     BOOL GetPropertiesUString(const nsMapiEntry& aObject, const ULONG *aPropertiesTag, 
>-                              ULONG aNbProperties, nsStringArray& aValues) ;
>+                              ULONG aNbProperties, nsTArray<nsString>& aValues) ;

nit: Please drop the space before the ; whilst you're here.

> NS_IMETHODIMP
> nsMsgPrintEngine::AddPrintURI(const PRUnichar *aMsgURI)
> {
>-  mURIArray.AppendString(nsDependentString(aMsgURI));
>+  mURIArray.AppendElement(nsDependentString(aMsgURI));

nit: not your fault, but please add NS_ENSURE_ARG_POINTER(aMsgURI); at the start of this function, because otherwise nsDependentString will crash.

>   // First, check if we are at the end of this stuff!
>-  if ( mCurrentlyPrintingURI >= mURIArray.Count() )
>+  if ( mCurrentlyPrintingURI >= PRInt32(mURIArray.Length()) )

nit: please drop the spaces just inside the brackets.

>-  if ( mCurrentlyPrintingURI < mURIArray.Count())
>+  if ( mCurrentlyPrintingURI < PRInt32(mURIArray.Length()))

nit: please drop space after the initial (

>diff --git a/mailnews/compose/src/nsMsgRecipientArray.cpp b/mailnews/compose/src/nsMsgRecipientArray.cpp
>diff --git a/mailnews/compose/src/nsMsgRecipientArray.h b/mailnews/compose/src/nsMsgRecipientArray.h

Can you drop the changes to these files please? I've just raised bug 467538 to kill this class and interface completely as all it happens to be is a component/interface wrapper around nsStringArray, and we've got better ways of handling that.

>-PRBool nsAbIPCCard::EqualsAfterUnicodeConversion(nsABCOMCardStruct * card, nsStringArray & differingAttrs)
>+PRBool nsAbIPCCard::EqualsAfterUnicodeConversion(nsABCOMCardStruct * card, nsTArray<nsString> & differingAttrs)

nit: no space after & please (please check for other instances in the patch as well).

>   // Now unsubscribe & subscribe.
>-  int i, cnt=groupList.Count();
>-  nsAutoString groupStr;
>+  PRUint32 i, cnt=groupList.Length();

nit: please fix the spacing around =

Comment 3

[Fred Jendrzejewski]

Attachment: replace - v2

Cleaned it up and what do you mean with nit ?

Comment 4

[Justin Wood (:Callek)]

(In reply to comment #3)
> Created an attachment (id=350984) [details]
> replace - v2
> 
> Cleaned it up and what do you mean with nit ?

Nit is basically "nit-pick" meaning, "if you were not already here, it wouldn't matter; but since you are lets fix this very minor issue x" ;-)

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 350984 [details] [diff] [review]
replace - v2

Looking better, though a couple of problems still:

>+    if (valueCount == aNbProperties && values != NULL)
>+    {
>+        aNames.SetCapacity(valueCount);
>+        for (PRUint32 i = 0; i < valueCount; ++ i)
>+        {
>+            if (PROP_ID(values[i].ulPropTag) == PROP_ID(aPropertyTags[i]))
>+            {
>+                if (PROP_TYPE(values[i].ulPropTag) == PT_STRING8)
>+                    aNames[i] = NS_ConvertASCIItoUTF16(values[i].Value.lpszA);

I'm fairly sure this won't work as you expect. SetCapacity doesn't change the size of the array, only ensures that there is enough memory allocated for the requested capacity. Therefore, even if using [i] works to assign the values, the length of the array is going to be zero after the changes.

>+                else if (PROP_TYPE(values[i].ulPropTag) == PT_UNICODE) 
>+                    aNames[i] = NS_ConvertUTF8toUTF16(values[i].Value.lpszW);
>+                else
>+                    aNames[i] = NS_LITERAL_STRING("");
...
>+                aNames[i] = NS_LITERAL_STRING("");

These should be EmptyString().

>+        }
>+        FreeBuffer(values);
>     }

As you've basically reformatted the function, please make it all 2-space indent.

>-PRBool nsAbIPCCard::EqualsAfterUnicodeConversion(nsABCOMCardStruct * card, nsStringArray & differingAttrs)
>+PRBool nsAbIPCCard::EqualsAfterUnicodeConversion(nsABCOMCardStruct * card, nsTArray<nsString>& differingAttrs)

Not quite right on the space adjustments, please make these: nsTArray<nsString> &differingAttrs. etc (several places that you've modified). You could remove the space after * on the card attribute whilst you're here as well.

Comment 6

[[e-mail]]

(In reply to comment #5)
>(From update of attachment 350984 [details] [diff] [review])
>>+                    aNames[i] = NS_LITERAL_STRING("");
>...
>>+                aNames[i] = NS_LITERAL_STRING("");
>These should be EmptyString().
Actually had they been necessary they should have used .Truncate()
(they are unnecessary because they are or will be new strings anyway)

Comment 7

[Mark Banner (:standard8)]

Attachment: [checked in] Fix nsMsgDBView

I didn't realise this bug was here when I was poking around at some of these.

So I've got some smaller patches that break this up, but should do the same thing.

Comment 8

[Mark Banner (:standard8)]

Attachment: [checked in] News fix

Comment 9

[Joshua Cranmer [:jcranmer]]

Comment on attachment 512276 [details] [diff] [review]
[checked in] News fix

I see no problems with the patch, although if you're working on removing nsStringArray, you should also remove its sister nsCStringArray as well...

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 512276 [details] [diff] [review]
[checked in] News fix

Checked in: http://hg.mozilla.org/comm-central/rev/cd01be66eec1

Comment 11

[Mark Banner (:standard8)]

(In reply to comment #9)
> I see no problems with the patch, although if you're working on removing
> nsStringArray, you should also remove its sister nsCStringArray as well...

That's a bigger chunk of work which we'll get to soon (a few more long plane flights I suspect). There's only a few nsStringArray instances left, so we may be able to remove that definition from core fairly soon.

Comment 12

[Mark Banner (:standard8)]

Attachment: Outlook Address Book fix

Comment 13

[[e-mail]]

Comment on attachment 512432 [details] [diff] [review]
Outlook Address Book fix

>diff --git a/mailnews/addrbook/src/nsAbOutlookDirectory.cpp b/mailnews/addrbook/src/nsAbOutlookDirectory.cpp
>--- a/mailnews/addrbook/src/nsAbOutlookDirectory.cpp
>+++ b/mailnews/addrbook/src/nsAbOutlookDirectory.cpp
>@@ -1504,34 +1504,34 @@ nsresult OutlookCardForURI(const nsACStr
>   nsMapiEntry mapiData;
>   mapiData.Assign(entry);
> 
>-  nsStringArray unichars;
>+  nsTArray<nsString> unichars;
>   if (mapiAddBook->GetPropertiesUString(mapiData, OutlookCardMAPIProps,
>                                         index_LastProp, unichars))
As a followup, I noticed that mapiAddBook::SetPropertiesUString actually takes an nsString[], it might be worth using that here, since we always know we want index_LastProp strings. (ModifyCard dynamically allocates it for some reason.)

>+          aNames.AppendElement(nsDependentString(values[i].Value.lpszW));
Nit: don't need this because the nsTArray method is a template and will construct its internal nsString directly from the lpszW pointer.

>+        else
>+          aNames.AppendElement(EmptyString());
>+      }
>+      else
>+        aNames.AppendElement(EmptyString());
Nit: don't need EmptyString(), AppendElement will default-construct anyway.

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 512273 [details] [diff] [review]
[checked in] Fix nsMsgDBView

Checked in: http://hg.mozilla.org/comm-central/rev/69dabc041bb2

Comment 15

[Mark Banner (:standard8)]

Attachment: [checked in] Outlook Address Book fix v2

Might as well just switch to nsString[] now ;-)

Comment 16

[[e-mail]]

Comment on attachment 513431 [details] [diff] [review]
[checked in] Outlook Address Book fix v2

>+  nsString* unichars = new nsString[index_LastProp];
>+  if (!unichars)
>+    return NS_ERROR_OUT_OF_MEMORY;
...
>+  delete unichars;
>+  unichars = 0;
[Could allocate this on the stack instead of the heap]
nsString unichars[index_LastProp];

>+          aNames[i] = NS_ConvertASCIItoUTF16(values[i].Value.lpszA);
[Could use CopyASCIItoUTF16]

>+        else
>+          aNames[i].Truncate();
>+      }
>+      else
>+        aNames[i].Truncate();
[Not sure it's worth truncating these as they're empty anyway]

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 513431 [details] [diff] [review]
[checked in] Outlook Address Book fix v2

Checked in: http://hg.mozilla.org/comm-central/rev/08febdd371e9

Comment 18

[Mark Banner (:standard8)]

This is now fixed, the only code that remains that uses it is code we don't currently compile.