258769 - Try using nsString more in addrbook to reduce conversions

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Address Book & Contacts, defect)

Description

[Mikael Parknert]

Attached patch is an attempt to use nsStrings instead
of unichars to accomplish
1. less conversions
2. smaller code (text and compiled bytes)

Comment 1

[Mikael Parknert]

Attachment: patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 158451 [details] [diff] [review]
patch

mikael, I'm not currently doing super-reviews, except in components where I'm a
peer... see the super-reviewer's page.

Comment 3

[[e-mail]]

Comment on attachment 158451 [details] [diff] [review]
patch

alecf isn't working on Mozilla for the rest of the year...

>     case 'P':
>       switch (attrname[2]) { 
>         case 'e':
>-	  PRUint32 format;
>+        {
>+          PRUint32 format;
>           rv = GetPreferMailFormat(&format);
>-          const PRUnichar *formatStr;
>+          nsAutoString formatStr;
> 
>           switch (format) {
>             case nsIAbPreferMailFormat::html:
>-              formatStr = NS_LITERAL_STRING("html").get();
>+              formatStr.AssignLiteral("html");
>               break;
>             case nsIAbPreferMailFormat::plaintext :
>-              formatStr = NS_LITERAL_STRING("plaintext").get();
>+              formatStr.AssignLiteral("plaintext");
>               break;
>             case nsIAbPreferMailFormat::unknown:
>             default :
>-              formatStr = NS_LITERAL_STRING("unknown").get();
>+              formatStr.AssignLiteral("unknown");
>               break;
>           }
>-          *value = nsCRT::strdup(formatStr);
>+          *value = ToNewUnicode(formatStr);
>           break;
>+        }
I suppose anything has to be better than the existing code ;-)
I would have guessed at this phrasing:
*value = ToNewUnicode(NS_LITERAL_STRING("html"));

>+  xmlStr.AppendLiteral("<?xml version=\"1.0\"?>\n");
>+  xmlStr.AppendLiteral("<?xml-stylesheet type=\"text/css\" href=\"chrome://messenger/content/addressbook/print.css\"?>\n");
>+  xmlStr.AppendLiteral("<directory>\n");
Is it worth combining these into one call?

>+  xmlStr.AppendLiteral(" ");
xmlStr.Append(PRUnichar(' ')); might be better still, need to find out.

>-nsresult nsAbCardProperty::AppendSection(AppendItem *aArray, PRInt16 aCount, const PRUnichar *aHeading, mozITXTToHTMLConv *aConv, nsString &aResult) 
>+nsresult nsAbCardProperty::AppendSection(AppendItem *aArray, PRInt16 aCount, const nsAString& aHeading, mozITXTToHTMLConv *aConv, nsString &aResult) 
Should be const nsAFlatString& aHeading, also kill the trailing space.

>-    rv = bundle->GetStringFromName(aHeading, getter_Copies(heading));
>+    rv = bundle->GetStringFromName(PromiseFlatString(aHeading).get(), getter_Copies(heading));
... this saves you from having to use PromiseFlatString here.

>+    nsDependentCString theURI(aURI);
>     // Extract the scheme
>-	nsCAutoString scheme;
>-    rv = nsService->ExtractScheme (nsDependentCString(aURI), scheme);
>+    nsCAutoString scheme;
>+    rv = nsService->ExtractScheme (theURI, scheme);
>     NS_ENSURE_SUCCESS(rv,rv);
> 
>-    // TODO 
>-    // Change to use string classes
>-
>     // Try to find a factory using the component manager.
>-    static const char kAbDirFactoryContractIDPrefix[]
>-        = NS_AB_DIRECTORY_FACTORY_CONTRACTID_PREFIX;
>+    nsCAutoString contractID;
> 
>-    PRInt32 pos = scheme.Length();
>-    PRInt32 len = pos + sizeof(kAbDirFactoryContractIDPrefix) - 1;
>-
>-    // Safely convert to a C-string for the XPCOM APIs
>-    char buf[128];
>-    char* contractID = buf;
>-    if (len >= PRInt32(sizeof buf))
>-        contractID = NS_STATIC_CAST(char *,nsMemory::Alloc(len + 1));
>-
>-    if (contractID == nsnull)
>-        return NS_ERROR_OUT_OF_MEMORY;
>-
>-    PL_strcpy(contractID, kAbDirFactoryContractIDPrefix);
>-    PL_strncpy(contractID + sizeof(kAbDirFactoryContractIDPrefix) - 1, aURI, pos);
>-    contractID[len] = '\0';
>+    contractID.AssignLiteral(NS_AB_DIRECTORY_FACTORY_CONTRACTID_PREFIX);
>+    contractID.Append(scheme);
> 
>     nsCID cid;
>-    rv = nsComponentManager::ContractIDToClassID(contractID, &cid);
>+    rv = nsComponentManager::ContractIDToClassID(contractID.get(), &cid);
>     NS_ENSURE_SUCCESS(rv,rv);
> 
>-    if (contractID != buf)
>-        nsCRT::free(contractID);
>-
>     nsCOMPtr<nsIFactory> factory;
>     rv = nsComponentManager::FindFactory(cid, getter_AddRefs(factory));
>     NS_ASSERTION(NS_SUCCEEDED(rv), "factory registered, but couldn't load");
This code is an utter mess. This part and the entire rest of the function could
probably be rewritten in a couple of lines like this:
// Extract the scheme
nsCAutoString contractID;
rv = nsService->ExtractScheme(nsDependentString(aURI), contractID);
NS_ENSURE_SUCCESS(rv,rv);

contractID.Insert(NS_AB_DIRECTORY_FACTORY_CONTRACTID_PREFIX, 0);
rv = CallCreateInstance(contractID.get(), aDirFactory);
return rv;

Comment 4

[Mikael Parknert]

Attachment: patch adressing review comments

Thanks for your comments Neil.

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 158668 [details] [diff] [review]
patch adressing review comments

You'd probably be better off finding another superreviewer for this -- I may
not get to it for a while.

Comment 6

[[e-mail]]

Comment on attachment 158668 [details] [diff] [review]
patch adressing review comments

>+  aResult.AppendLiteral("<");
>+  aResult.Append(attrNameStr);
>+  aResult.AppendLiteral(">");
Sorry for not making it clear before... there are lots of these, I wasn't going
to comment on them individually :-)

>     nsXPIDLString _value;
>     rv = card->GetCardValue (name.get (), getter_Copies (_value));
>     NS_ENSURE_SUCCESS(rv, rv);
>     nsString value (_value.get ());
> 
>-    if (!value.get () || value.Length () == 0)
>+    if (!value.get () || value.IsEmpty())
I've sought advice and this section can be simplified because a) nsXPIDLString
is a subclass of nsString, and b) value.IsEmpty() is always true if
!value.get() so you can rename _value to value, remove the nsString and remove
the !value.get() test.

>+    contractID.Insert(NS_AB_DIRECTORY_FACTORY_CONTRACTID_PREFIX, 0);
And the advice I got here was to leave the scheme code alone and use
AppendLiteral and Append(scheme) to build up the contractID. I still don't
actually know for sure if CallCreateInstance is the correct replacement.

Comment 7

[[e-mail]]

Yes, it appears that that specific code is indeed a long-winded way of saying
CallCreateInstance, although CallGetService would probably be a better idea.

Comment 8

[Mikael Parknert]

Attachment: patch

have looked through this again. I hope this is ok.

Comment 9

[[e-mail]]

Comment on attachment 159333 [details] [diff] [review]
patch

>+    return CallGetService(contractID.get(), nsnull, aDirFactory);
Minor nit: I believe there's a newer, 2-argument version of this function in
nsIServiceManagerUtils.h (rather than nsIServiceManager.h) so it would be nice
if you could tweak this and the include before check in.

Comment 10

[Mikael Parknert]

Attachment: new patch

old patch is out of date cause of checkins from bug 267040

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 164750 [details] [diff] [review]
new patch

Carrying over neil's r+

This looks great.

Comment 12

[Daniel Brooks [:db48x]]

checked in