207634 - Want AllocateRawSortKey function

Status: RESOLVED FIXED

(Core :: Internationalization: Localization, enhancement, P4)

Description

[[e-mail]]

All callers of CreateRawSortKey call GetRawSortKeyLen first, allocate the key,
then create the raw sort key. Since these two functions share code it should be
more efficient to replace or supplement these with a new AllocateRawSortKey (or
whatever naming convention) function.

Comment 1

[[e-mail]]

Attachment: First go at a patch

With this patch sorting 13270 messages by subject dropped from 1.414745s to
1.07754s (single test).

Comment 2

[[e-mail]]

Attachment: Nearly there!

Comment 3

[[e-mail]]

I don't understand the nsCollationMacUC code though, I can't figure out the
parameters to UCGetCollationKey so that I can allocate a collation key.

Comment 4

[[e-mail]]

Attachment: Improved CompareRawSortKey

Comment 5

[[e-mail]]

My apologies to anyone looking for the about:config filtering bug 207840

Comment 6

[[e-mail]]

Attachment: Ready for testing

Comment 7

[Simon Montagu :smontagu]

Comment on attachment 125749 [details] [diff] [review]
Ready for testing

There's something funny going on here:

>+nsresult nsCollactionMac::CompareString()
>+{
>+
>+nsresult nsCollationMac::CompareString(const nsCollationStrength strength, 
>+                                       const nsAString& string1, const nsAString& string2, 

more substantive comments to follow...

Comment 8

[Simon Montagu :smontagu]

>-  // order could be -2, -1, 0, 1, or 2, 
>-  // *result can only be -1, 0 or 1
>-  *result = (order == 0) ? 0 : ((order > 0) ? 1 : -1);

Is it safe to remove this? Do callers only depend on the result being <0 / ==0 /
>0 ?

>+  // set our default collaion options

Nit: might as well fix this typo if you will have cvsblame for the line anyway

You will need mac- and OS/2-specific review and pre-checkin testing, but I am
very happy with this overall.

Comment 9

[Simon Montagu :smontagu]

Comment on attachment 125749 [details] [diff] [review]
Ready for testing

I discovered a few typos trying to build this on Windows:

>+    if (retval) {
>+      res = NS_ERROR_OK;

s/ERROR_//


>+      if (NS_SUCCEEDED(res) && Cstr2 != nsnull) {
>+        retval = CompareStringA(mLCID, dwMapFlags, Cstr1, -1, CStr2, -1);

s/CStr2/Cstr2/

>+    buffer = PR_Malloc(len);

s/len/byteLen/
(two of these)

Also, defining CompareString in nsCollationWin.cpp collides with a Windows (and
I think also a Mac) #define. Using the same name is silly IMNSHO, and has
caused bustage before now
(c/nsBookmarksService.cpp#1.288">http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/xpfe/components/bookmarks/sr c/nsBookmarksService.cpp#1.288)
Maybe it would be a good idea to change the name?

Just to get it to compile for now I added #undef CompareString after the
#include <windows.h> in nsCollationWin.cpp. Now testing...

Comment 10

[Simon Montagu :smontagu]

Attachment: Stack trace

Running with this on Windows, when I open mailnews I get 
###!!! ASSERTION: Should only be initialized once.: 'mCollation == NULL', file
c:/MozillaWork/shaping1/mozilla/intl/locale/src/windows/nsCollationWin.cpp,
line 70

followed by a crash with this stack

Comment 11

[[e-mail]]

Attachment: Updated patch

Perhaps your error was caused by an incomplete build - I had forgotton to diff
nsCharsetMenu.cpp (and nsFontMetricsOS2.cpp). This patch should also
incorporate your previous comments.

Comment 12

[Simon Montagu :smontagu]

Comment on attachment 126122 [details] [diff] [review]
Updated patch

OK, this one builds out of the box and sorts my mail without crashing (after I
built from toplevel, which I should probably have done in the first place --
sorry!)

Comment 13

[Kai Engert [:KaiE:]]

I'm not sure I understand what this bug is about, but...

Neil gave me a patch, it compiled on a 1.4 branch Mac OSX tree from 2nd of June,
and we tried "sort by subject" order in mail window. I saw that "Re: " and case
were ignored in terms of sort order, but besides that the sort order looked fine.

Comment 14

[[e-mail]]

Attachment: Patch with Mac fixes

Thanks to [e-mail] for testing this on the Mac.

Comment 15

[Mike Kaply [:mkaply]]

Very strange. We crash on startup on OS/2.

For some reason, the UniStrlen on the Unistrxfrmed string is very large.

We're still investigating.

Comment 16

[[e-mail]]

Attachment: Patch with OS2 fixes

Comment 17

[Mike Kaply [:mkaply]]

This should be good for OS/2.

Comment 18

[[e-mail]]

Comment on attachment 127179 [details] [diff] [review]
Patch with OS2 fixes

Asking for Seth's sr because of minor mailnews impact.

Comment 19

[(not reading, please use [e-mail] instead)]

Comment on attachment 127179 [details] [diff] [review]
Patch with OS2 fixes

r/sr=sspitzer on the mailnews parts.

just a few questions:

1)

-  if (abcard->primaryCollationKey)
-    nsMemory::Free(abcard->primaryCollationKey);
-  if (abcard->secondaryCollationKey)
-    nsMemory::Free(abcard->secondaryCollationKey);
+  PR_FREEIF(abcard->primaryCollationKey);
+  PR_FREEIF(abcard->secondaryCollationKey);
   PR_FREEIF(abcard);

Is PR_Free() the right thing?  Does it match the allocator?  (We don't want any
mis-matched memory frees)


2)

   // XXX can we avoid this copy?
   nsAutoString sourceString(aSource);
-  rv = mCollationKeyGenerator->GetSortKeyLen(kCollationCaseInSensitive,
sourceString, aKeyLen);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  *aKey = (PRUint8*) nsMemory::Alloc(*aKeyLen);
-  if (!aKey)
-    return NS_ERROR_OUT_OF_MEMORY;
-
-  rv = mCollationKeyGenerator->CreateRawSortKey(kCollationCaseInSensitive,
sourceString, *aKey, aKeyLen);
+  rv = mCollationKeyGenerator->AllocateRawSortKey(kCollationCaseInSensitive,
sourceString, aKey, aKeyLen);

Can we remove that nsAutoString now?

Comment 20

[(not reading, please use [e-mail] instead)]

see also bug #103451

Comment 21

[[e-mail]]

1) AllocateRawSortKey uses PR_Malloc, so all the callers have to use PR_Free
2) It looks as if nsDependentString(aSource) should always have worked anyway.
I'll try to remember to test and roll it into the patch.

Comment 22

[Simon Montagu :smontagu]

Comment on attachment 127179 [details] [diff] [review]
Patch with OS2 fixes

r=smontagu

Comment 23

[[e-mail]]

Patch is in... well, attachment 127179 [details] [diff] [review] was missing the mac fixes :-[
plus also thanks to mkaply for spotting the missing #include "nsCRT.h"

Comment 24

[[e-mail]]

*** Bug 207512 has been marked as a duplicate of this bug. ***

Comment 25

[[e-mail]]

*** Bug 99383 has been marked as a duplicate of this bug. ***

Comment 26

[[e-mail]]

*** Bug 195008 has been marked as a duplicate of this bug. ***