The default bug view has changed. See this FAQ.

Implement nsICollation interface on MacOS X by using Unicode Utility

RESOLVED FIXED in mozilla0.9.6

Status

()

Core
Internationalization
P4
normal
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: Frank Tang, Assigned: Frank Tang)

Tracking

Trunk
mozilla0.9.6
PowerPC
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: MacOS X)

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

16 years ago
Our current implementation of nsICollation is not unicode based (See 
/intl/locale/src/mac/nsCollationMac.cpp 
/intl/locale/src/mac/nsCollationMac.h 
)
we should use the following APIs in the Unicode Utilities (see 
http://developer.apple.com/techpubs/macosx/Carbon/text/UnicodeUtilities/Unicode_
Utilities/index.html ) to implement them on MacOS X

We should implement
NS_IMETHOD Initialize(nsILocale* locale)
by calling 
UCCreateCollator

We should implment 
NS_IMETHOD CompareString(const nsCollationStrength strength, 
                         const nsString& string1, const nsString& string2, 
                         PRInt32* result) = 0;

by using UCCompareText

implement 
NS_IMETHOD GetSortKeyLen(const nsCollationStrength strength, 
                         const nsString& stringIn, PRUint32* outLen) = 0;
and 
NS_IMETHOD CreateRawSortKey(const nsCollationStrength strength, 
                           const nsString& stringIn, PRUint8* key, 
                            PRUint32 *outLen) = 0;
by calling UCGetCollationKey 

implement the destructor by calling UCDisposeCollator

Implment  
NS_IMETHOD CompareRawSortKey(const PRUint8* key1, const PRUint32 len1, 
                             const PRUint8* key2, const PRUint32 len2, 
                             PRInt32* result) = 0;

by calling UCCompareCollationKeys
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
Whiteboard: MacOS X
(Assignee)

Comment 1

16 years ago
Created attachment 46293 [details]
intl/locale/src/mac/nsCollationMacUC.cpp
(Assignee)

Comment 2

16 years ago
Created attachment 46294 [details]
intl/locale/src/mac/nsCollationMacUC.cpp
(Assignee)

Comment 3

16 years ago
The 2nd attachment is   intl/locale/src/mac/nsCollationMacUC.h not .cpp
(Assignee)

Comment 4

16 years ago
Created attachment 46295 [details] [diff] [review]
use the UC version on Carbon build
(Assignee)

Comment 5

16 years ago
The patch work. However, I encounter bad crash (with corrupt stacktrack) with 
this patch while I select View:Character Set:Auto-Detect menu items. Don't check 
in untill we find out why it crash. I am sure it is related to this patch since I 
cannot crash without it. 
frank, there is a known bug with carbonlib prior to 1.4 where if we make any 
changes to a submenu via the onCreate handler (thus causing the menu to be marked 
'dirty' and rebuilt) then it corrupts the heap when the menu goes away. Could 
this be what you're running into? Are you seeing this on fizzilla running on os9 
or only on osx?
(Assignee)

Comment 7

16 years ago
I see the crash on MacOS 9, I have not try MacOS X yet. I will do it today. 
Thanks for your info. It sound's related.
(Assignee)

Comment 8

16 years ago
MacOS X run without any problem. pinkerton- can you code review this one so I
can land it for m94?
(Assignee)

Comment 9

16 years ago
*** Bug 21066 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

16 years ago
Keywords: nsbranch
Priority: -- → P4
Target Milestone: --- → mozilla0.9.6

Comment 10

16 years ago
nsbranch- since Frank moved it to 0.9.5
Keywords: nsbranch → nsbranch-
(Assignee)

Comment 11

16 years ago
nhotta, can you r= this one ?
(Assignee)

Updated

16 years ago
Blocks: 103669

Comment 12

16 years ago
1) In nsCollationMacUC::StrengthToOptions, avoid assign to *aOptions twice.
Change as below then the initial assignment would not be needed.
 switch( aStrength) {
    case kCollationCaseSensitive:
    default:
      *aOptions = kUCCollateStandardOptions;
      break;

2) In nsCollationMacUC::ConvertLocale, 
Change to use NS_LITERAL_STRING for "NSILOCALE_COLLATE",
 collate.AssignWithConversion("NSILOCALE_COLLATE");
  nsresult res = aNSLocale->GetCategory(collate.get(), &aLocaleString);


3) In nsCollationMacUC::GetSortKeyLen,
 // According to the document, the timension should typically be
  // at least 5 * textLength
What is 'timension'?

Also please coordinate with the nsString -> nsAString change (bug 103222) if
possible.

No longer blocks: 103669
(Assignee)

Updated

16 years ago
Blocks: 104056
(Assignee)

Updated

16 years ago
Attachment #46293 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #46294 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #46295 - Attachment is obsolete: true
(Assignee)

Comment 13

16 years ago
Created attachment 53192 [details]
v2 of nsCollationMacUC.h
(Assignee)

Comment 14

16 years ago
Created attachment 53193 [details]
v2 of nsCollationMacUC.cpp
(Assignee)

Comment 15

16 years ago
Created attachment 53194 [details] [diff] [review]
v2 of patch
(Assignee)

Comment 16

16 years ago
nhotta, can you r= this one ?

Comment 17

16 years ago
Comment on attachment 53193 [details]
v2 of nsCollationMacUC.cpp

r=nhotta for this and other v2 patches.
Attachment #53193 - Flags: review+

Comment 18

16 years ago
Comments:

class nsCollationMacUC : public nsICollation {
...
  nsCollationMacUC();
  virtual ~nsCollationMacUC(); 

Minor nit: i prefer to see ctor and dtor first, before other interface methods.

private:
  PRBool mInit;
  PRBool mHasCollator;

Minor nit: use PRPackedBool (8 bits, not 32) to possibly save space.

nsCollationMacUC::nsCollationMacUC() 
{
  NS_INIT_REFCNT(); 
  mInit = PR_FALSE;
  mHasCollator = PR_FALSE;
}

Why do you need 'mHasCollator'? Isn't checking mCollator for null good enough?

Minor nit: prefer initializors:

nsCollationMacUC::nsCollationMacUC()
: mInit(PR_FALSE)
, mHasCollator(PR_FALSE)
{
  NS_INIT_REFCNT(); 
}

Messy:
  nsAutoString collate;
  PRUnichar* aLocaleString = nsnull;
  nsresult res = aNSLocale->
GetCategory(NS_LITERAL_STRING("NSILOCALE_COLLATE").get(), &aLocaleString);
  NS_ENSURE_SUCCESS(res, res);
  NS_ENSURE_ARG_POINTER(aLocaleString);
  nsCAutoString tmp;
  tmp.AssignWithConversion(aLocaleString);
  nsMemory::Free(aLocaleString);
  nsCAutoString changed;
  tmp.ReplaceChar('-', '_');
  OSStatus err;
  err = ::LocaleRefFromLocaleString( tmp.get(), aMacLocale);
  NS_ENSURE_TRUE((err == noErr), NS_ERROR_FAILURE);

here,
 PRUnichar* aLocaleString = nsnull;
Use nsXPIDLString and getter_Copies.
  nsCAutoString changed;
This is never used. Watch the compiler warnings!

  NS_ENSURE_TRUE((err == noErr), NS_ERROR_FAILURE);
is this really clearer than:
  if (err != noErr)
    return NS_ERROR_FAILURE ?
It's certainly harder to put breakpoints in. I recommend only using NS_ENSURE
macros for argument checking at the top of a function.

  NS_ENSURE_TRUE(mInit, NS_ERROR_FAILURE);
NS_ERROR_NOT_INITIALIZED?

  if (mHasCollator) 
  {
    err = ::UCDisposeCollator( &mCollator );
    mHasCollator = PR_FALSE;
See comment above about mHasCollator.

  NS_ENSURE_TRUE((!mInit), NS_ERROR_FAILURE);
NS_ERROR_ALREADY_INITIALIZED?

  // According to the document, the lenght of the key should typically be
typo.

Fix those, and sr=sfraser
(Assignee)

Comment 19

16 years ago
>Why do you need 'mHasCollator'? Isn't checking mCollator for null good enough?

You are assuming mCollator (which is in type CollatorRef) is a pointer and nsnull 
mean we don't have it. However, I cannot find any documentation say 0 value in a 
CollatorRef mean it is an illegal value so I am not dare to make such assumption
(Assignee)

Updated

16 years ago
Blocks: 104148
No longer blocks: 104056
(Assignee)

Updated

16 years ago
Blocks: 104060
No longer blocks: 104148
(Assignee)

Comment 20

16 years ago
fixed and check in 
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 21

16 years ago
Switching qa contact to ftang for now. Frank, can this be verified within 
development or can you provide QA with a test case? Thanks.
QA Contact: andreasb → ftang
(Assignee)

Updated

16 years ago
No longer blocks: 104060
You need to log in before you can comment on or make changes to this bug.