Closed
Bug 258769
Opened 20 years ago
Closed 20 years ago
Try using nsString more in addrbook to reduce conversions
Categories
(SeaMonkey :: MailNews: Address Book & Contacts, defect)
SeaMonkey
MailNews: Address Book & Contacts
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mikael, Assigned: mikael)
Details
(Keywords: memory-footprint)
Attachments
(1 file, 3 obsolete files)
30.32 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
Attached patch is an attempt to use nsStrings instead of unichars to accomplish 1. less conversions 2. smaller code (text and compiled bytes)
Assignee | ||
Comment 1•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #158451 -
Flags: superreview?(bzbarsky)
Attachment #158451 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Updated•20 years ago
|
Status: NEW → ASSIGNED
Comment 2•20 years ago
|
||
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.
Attachment #158451 -
Flags: superreview?(bzbarsky)
Assignee | ||
Updated•20 years ago
|
Attachment #158451 -
Flags: superreview?(alecf)
Comment 3•20 years ago
|
||
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;
Assignee | ||
Updated•20 years ago
|
Attachment #158451 -
Flags: superreview?(alecf)
Attachment #158451 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Comment 4•20 years ago
|
||
Thanks for your comments Neil.
Attachment #158451 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #158668 -
Flags: superreview?(dbaron)
Attachment #158668 -
Flags: review?(neil.parkwaycc.co.uk)
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•20 years ago
|
||
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•20 years ago
|
||
Yes, it appears that that specific code is indeed a long-winded way of saying CallCreateInstance, although CallGetService would probably be a better idea.
Assignee | ||
Updated•20 years ago
|
Attachment #158668 -
Flags: superreview?(dbaron)
Attachment #158668 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Comment 8•20 years ago
|
||
have looked through this again. I hope this is ok.
Attachment #158668 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #159333 -
Flags: superreview?(Henry.Jia)
Attachment #159333 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 9•20 years ago
|
||
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.
Attachment #159333 -
Flags: review?(neil.parkwaycc.co.uk) → review+
Assignee | ||
Updated•20 years ago
|
Attachment #159333 -
Flags: superreview?(Henry.Jia) → superreview?(bienvenu)
Assignee | ||
Updated•20 years ago
|
Attachment #159333 -
Flags: superreview?(bienvenu) → superreview?(tor)
Assignee | ||
Updated•20 years ago
|
Attachment #159333 -
Flags: superreview?(tor) → superreview?(dmose)
Assignee | ||
Comment 10•20 years ago
|
||
old patch is out of date cause of checkins from bug 267040
Attachment #159333 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #159333 -
Flags: superreview?(dmose)
Assignee | ||
Updated•20 years ago
|
Attachment #164750 -
Flags: superreview?(bienvenu)
Updated•20 years ago
|
Product: Browser → Seamonkey
Assignee | ||
Updated•20 years ago
|
Attachment #164750 -
Flags: superreview?(bienvenu) → superreview?(roc)
Comment on attachment 164750 [details] [diff] [review] new patch Carrying over neil's r+ This looks great.
Attachment #164750 -
Flags: superreview?(roc)
Attachment #164750 -
Flags: superreview+
Attachment #164750 -
Flags: review+
Comment 12•20 years ago
|
||
checked in
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•