255134 - make nsICollation scriptable

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

nsICollation/nsICollationFactory are implemented as C++ interfaces only, instead
of in IDL; there's no way to do a locale-sensitive string compare from JS as a
result.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: idlice-icollation-0.patch

Moves the interface to IDL, and does some munging of the enum constants (since
their scope changed).  Allows for scripting this interface.

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 155774 [details] [diff] [review]
idlice-icollation-0.patch

Drive-by nit: JavaDoc-style comments would be nice for the interface...

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: nsICollation.idl with jdoc style comments

Everyone's a critic.. (good idea) ;)

Comment 4

[Jungshik Shin]

Oops, I also worked on this and came up with an almost identical fix but I can't
make 'cvs diff' at the moment because my public key hasn't been yet added
although I sent it over two months ago. I wonder why I didn't get bug mails for
your patch upload.  Anyway, here are a couple of comments:

You don't need to specify 'const' for in parameters in idl. 'const' for
non-pointer/non-reference type argument is redundant, too.   

Btw,(it's not your code)  I'm afraid the usage pattern of 'compareRawSortKey'
and its declaration(in idl and previously in C++ header)/implementations(in
intl/locale/src/*/nsCollation*) that use strcmp-like functions are inconsistent.
For instance, it's used to compare nsIRDFBlobs which are binary  (i.e. they can
have embedded nulls)

 http://lxr.mozilla.org/seamonkey/source/content/xul/templates/src/nsXULSortService.cpp#498

Perhaps, this has to be a separate bug (it's been like this for a long time) 

Comment 5

[Jungshik Shin]

(In reply to comment #4)
> although I sent it over two months ago. I wonder why I didn't get bug mails for
> your patch upload.  

Aha. I was not explicitly on Cc, but I got your bug report because it's assigned
to smontagu (whose bug mail I'm also getting) by default. Later, you made
yourself the assignee but didn't add smontagu to Cc so that I didn't notice you
had uploaded your patches. Next time you do, please add the previous assignee to
Cc. 

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #4)
> You don't need to specify 'const' for in parameters in idl. 'const' for
> non-pointer/non-reference type argument is redundant, too.   

Ah, I added the consts there to make the resuling C++ class match exactly with
what nsICollation.h looked like before (minus the strength enum), to minimize
the amount of impl code I'd need to change.  I can go through and remove the
consts if you'd prefer, though.

> Btw,(it's not your code)  I'm afraid the usage pattern of 'compareRawSortKey'
> and its declaration(in idl and previously in C++ header)/implementations(in
> intl/locale/src/*/nsCollation*) that use strcmp-like functions are inconsistent.
> For instance, it's used to compare nsIRDFBlobs which are binary  (i.e. they can
> have embedded nulls)
> 
> 
http://lxr.mozilla.org/seamonkey/source/content/xul/templates/src/nsXULSortService.cpp#498
> 
> Perhaps, this has to be a separate bug (it's been like this for a long time) 

Oof.. probably should be a separate bug; there's no functional change with this
patch, I'd rather not actually try to fix something ;)

Let me know if you want me to de-constify the idl; sorry about the Cc mixup, too
.  Should I punt review over to you?  blizzard was the only person's name that I
found associated with intl on the moz.org pages, so I pegged him :)

Comment 7

[Jungshik Shin]

(In reply to comment #6)
> (In reply to comment #4)
> > You don't need to specify 'const' for in parameters in idl. 'const' for
> > non-pointer/non-reference type argument is redundant, too.   
> 
> to minimize
> the amount of impl code I'd need to change.  I can go through and remove the
> consts if you'd prefer, though.

Yes, I think it's better to do that. Actually, it's rather samll amount of work.
 You just have to remove 'const' for in parameters that are passed by value in
implementation files (in addition to idl files)
 
> > Btw,(it's not your code)  I'm afraid the usage pattern of 'compareRawSortKey'
...

> > Perhaps, this has to be a separate bug (it's been like this for a long time) 
 
> Oof.. probably should be a separate bug; there's no functional change with this
Ok. I'll file a separate bug. 

As for r/sr, you may as well ask blizzard for sr and me (or smontagu) for r. See  
http://www.mozilla.org/owners.html

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: idlize-icollation-1.patch (intl dir only)

New patch, sans const.	I did a diff of the intl dir only; the handful of enum
fixups in other files stay as in the first patch.

Comment 9

[Jungshik Shin]

Comment on attachment 156198 [details] [diff] [review]
idlize-icollation-1.patch (intl dir only)


>Index: intl/locale/idl/nsICollation.idl

>+ * the License at http://www.mozilla.org/NPL/

For a new file, you have to use MPL/GPL/* tri-license instead of NPL/*/*. 


compareRawSortKey([const,array,size_is(len1)] in octet key1, in unsigned long
len1,
>+                                    [const,array,size_is(len2)] in octet key2, in unsigned long len2);

  const here is redundant for in parameters. 

BTW, nsICollation.idl is in ISO-8859-1 (there are a couple of Latin letters
with diacrtic marks). Please, turn it to UTF-8.   


>Index: intl/locale/src/mac/nsCollationMac.cpp

>-  if (strength != kCollationCaseSensitive) {
>+  if (strength != nsICollation::kCollationCaseSensitive) {

Although it'd not hurt, you don't need to qualify kCollationCaseSensitive here
because nsCollationXXX inherits nsICollation, do you? 

>Index: intl/locale/src/mac/nsCollationMac.h
>
> public: 
>   NS_DECL_ISUPPORTS

Why don't you just use |NS_DECL_NSICOLLATION| here and in other implemenation
header files? That way, you can remove most of declarations. I thought you did.
I'd have mentioned it if I had noticed it.


>+  NS_IMETHOD CompareRawSortKey(const PRUint8* key1, PRUint32 len1, 
>+                               const PRUint8* key2, PRUint32 len2, 
>                                PRInt32* result) 
>                                {*result = PL_strcmp((const char *)key1, (const char *)key2); return NS_OK;}

With |NS_DECL_NSICOLLATION|, this has to move to cpp file. 


>   nsCollationMac();

xpidl now generates a template with private  dtor so that you may want to
(well, you don't have to, but it'll silence some compiler warnings, I guess)
add 

  private:
    ~nsCollationMac();


>Index: intl/locale/src/mac/nsCollationMacUC.h

> // According to the documentation, the length of the key should typically be
> // at least 5 * textLength
>-const PRUint32 kCollationValueSizeFactor = 5;
>+const PRUint32 nsICollation::kCollationValueSizeFactor = 5;

This constant is implementation-specific, isn't it? Why don't you just declare
it inside nsCollationMacUC (or in *cpp file)?

Comment 10

[Jungshik Shin]

(In reply to comment #9)
 
> xpidl now generates a template with private  dtor so that you may want to
> (well, you don't have to, but it'll silence some compiler warnings, I guess)
> add 
> 
>   private:
>     ~nsCollationMac();

Oops. It's the other way around. 'virtual dtor' was used, in the past, to quiet
some compilers, but we don't need it any more (see bug 229866). Besides, we can
save some space (see bug 229875). nsCollation* classes don't need virtual dtor
for sure because they're not subclassed.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: idlize-icollation-2.patch (intl dir only)

Ok, updated.. I left the const in there for those buffer params, because "const
PRUint8*" is fairly canonical on the C++ side meaning a buffer of things you
shouldn't touch...

Went through and cleaned up the headers and de-virtualized the destructors. 
Also fixed the extraneous nsICollation:: -- they were the result of an
overzealous perl string replace when I was looking for usage of the constants
through the ocde :)

Comment 12

[Jungshik Shin]

Comment on attachment 156228 [details] [diff] [review]
idlize-icollation-2.patch (intl dir only)

(In reply to comment #11)
> Created an attachment (id=156228)
> idlize-icollation-2.patch (intl dir only)

Even if nothing changed from the previous patch outside intl/, IMHO, it'd be a
bit nicer to  future code archaelogists to include the whole thing (unless 
you're gonna commit them separately, which is not the case here.). How about
sr? (S)he has to take a look at multiple attachments instead of just this one. 

You also didn't include nsICollation.idl in this attachment.   

> I left the const in there for those buffer params, because "const
> PRUint8*" is fairly canonical on the C++ side meaning a buffer of things you
> shouldn't touch...

You missed my point. When generating a C++ header file from an IDL source,
xpidl will automatically add |const| for in parameters (ref. or pointers) so
that you do NOT need to have 'const' in IDL. It doesn't hurt, but you don't
need that.

With 'const' removed in IDL, r=jshin

Comment 13

[Jungshik Shin]

(In reply to comment #12)

> You also didn't include nsICollation.idl in this attachment.   

> With 'const' removed in IDL, r=jshin

 Pls, don't forget to use MPL/GPL/* instead of NPL/* for nsICollation.idl. Also,
turn it to UTF-8 from ISO-8859-1 as I mentioned in comment #9
 

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #12)
> (From update of attachment 156228 [details] [diff] [review])
> (In reply to comment #11)
> > Created an attachment (id=156228)
> > idlize-icollation-2.patch (intl dir only)
> 
> Even if nothing changed from the previous patch outside intl/, IMHO, it'd be a
> bit nicer to  future code archaelogists to include the whole thing (unless 
> you're gonna commit them separately, which is not the case here.). How about
> sr? (S)he has to take a look at multiple attachments instead of just this one. 

Yep; usually I attach a final patch before asking for sr (as I will do in this
case), which will also be the historical one .  That way the person doing sr can
see the final patch (i.e. the one with the little bits mentioned here applied).

> You also didn't include nsICollation.idl in this attachment.   

Whoops, my bad; forgot -N on the diff line.  I did change the license, as well
converted it to UTF8.

> > I left the const in there for those buffer params, because "const
> > PRUint8*" is fairly canonical on the C++ side meaning a buffer of things you
> > shouldn't touch...
> 
> You missed my point. When generating a C++ header file from an IDL source,
> xpidl will automatically add |const| for in parameters (ref. or pointers) so
> that you do NOT need to have 'const' in IDL. It doesn't hurt, but you don't
> need that.
>
> With 'const' removed in IDL, r=jshin

It will if the C++ type the IDL type translates to is a ref/ptr.  In this case,
the IDL type is just "in octet" -- which is PRUint8, for which const is
unnecessary.  Without the [const], this gets translated to "PRUint8 *key1". 
This may be a xpidl bug.

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: idlize-icollation-3.patch (final)

final patch

Comment 16

[Jungshik Shin]

(In reply to comment #14)

> It will if the C++ type the IDL type translates to is a ref/ptr.  In this case,
> the IDL type is just "in octet" -- which is PRUint8, for which const is
> unnecessary.  Without the [const], this gets translated to "PRUint8 *key1". 

  Sorry for the noise. I was mistaken that 'const' would be added automatically
even  in that case.  You're absolutely right. 

> This may be a xpidl bug.

  May or may not be. We have to ask some IDL experts.
 

Comment 17

[chris hofmann]

is this needed for livebook marks?  time is getting short on the preview release

Comment 18

[Hussam Al-Tayeb]

(In reply to comment #17)
> is this needed for livebook marks?  time is getting short on the preview release

I think its needed for the bookmarks sorting for firefox/

Comment 19

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

patch for this is fairly trivial, someone just needs to look it over for sr --
blizzard mentioned he won't have time, so perhaps jshin's r= can work for sr as
well if noone else is interested/able/whatever to look this over.

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 156231 [details] [diff] [review]
idlize-icollation-3.patch (final)

sr=[e-mail].

/be

Comment 21

[Mike Kaply [:mkaply]]

Can you give me a little info as to why you would want this in 1.7? This is a
very large patch.

Comment 22

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #21)
> Can you give me a little info as to why you would want this in 1.7? This is a
> very large patch.

Only from an interest in keeping the firefox 1.0/Aviary versions of Gecko in
sync -- the patch doesn't change any core functionality, but it does expose
nsICollation to javascript, for a Firefox feature impl (bug 211023).

Comment 23

[chris hofmann]

Comment on attachment 156231 [details] [diff] [review]
idlize-icollation-3.patch (final)

a=chofmann for aviary branch.

Comment 24

[Darin Fisher]

It looks to me as if the vtable for nsICollation changed as a result of this
patch.  Did that need to happen?  It looks like the Initialize method was moved
down a few slots.  Were there other changes?  Shouldn't we try to preserve the
interface?  You never know who might be using this interface.  If you tweak an
interface (even one not defined in IDL) you really ought to rev the IID
otherwise you make it very difficult on embedders and extension authors who wish
to (need to) make use of non-frozen functionality.

Comment 25

[Jungshik Shin]

ooops. I should have caught it when reviewing. I thought IIDs had been changed ....

Comment 26

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Oops, my bad.. I had intended to try to keep the interfaces the same at one
point, but that fell by the wayside.  New IIDs:

nsICollationFactory: 04971e14-d6b3-4ada-8cbb-c3a13842b349
nsICollation: b0132cc0-3786-4557-9874-910d7def5f93

sorry 'bout that

Comment 27

[Mike Kaply [:mkaply]]

darin, do you know of any embeddors that are using nsicollation?

Comment 28

[Darin Fisher]

why not just keep the vtable the same?  can't you just move Initialize back up?
 or were the method signatures changed?

mkaply: nope, i don't know of any moz embedding, extension or plugin that is
using this interface, but that doesn't mean that it isn't the case.  it may be
that some future moz embedding or extension may wish to use it with moz 1.4 and
above.  if we change the IIDs or keep this interface the same, then such an
extension could be created.

Comment 29

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: reorder-nsicollation.patch

Patch to reorder the initialize method, also to put back the old IIDs.

Comment 30

[Asa Dotzler [:asa]]

Comment on attachment 156231 [details] [diff] [review]
idlize-icollation-3.patch (final)

a=asa for 1.7.x checkin.

Comment 31

[Mike Kaply [:mkaply]]

vlad, was the reorder patch checked in anywhere?

Comment 32

[Mike Kaply [:mkaply]]

I'm very confused.

the reorder definitely didn't go in aviary, but the aviary IIds are:

[scriptable, uuid(04971e14-d6b3-4ada-8cbb-c3a13842b349)]
[scriptable, uuid(b0132cc0-3786-4557-9874-910d7def5f93)]

but the iids in the patch are:

[scriptable, uuid(D4CF2F80-A98B-11d2-9119-006008A6EDF6)]
[scriptable, uuid(CDBFD3F0-A4FE-11d2-9119-006008A6EDF6)]

anyone know why this is?

Comment 33

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #32)
> I'm very confused.
> 
> the reorder definitely didn't go in aviary, but the aviary IIds are:
> 
> [scriptable, uuid(04971e14-d6b3-4ada-8cbb-c3a13842b349)]
> [scriptable, uuid(b0132cc0-3786-4557-9874-910d7def5f93)]
> 
> but the iids in the patch are:
> 
> [scriptable, uuid(D4CF2F80-A98B-11d2-9119-006008A6EDF6)]
> [scriptable, uuid(CDBFD3F0-A4FE-11d2-9119-006008A6EDF6)]
> 
> anyone know why this is?

The reorder didn't go in; for the iids, I forgot to change the iids in the
original patch (see comment #26).  I can't remember why the reorder didn't go
in, there was something said on irc.. but it's been a while.  I'll get the
original patch in on 1.7 tomorrow (later today); this got lost in my bugspam.

Comment 34

[Mike Kaply [:mkaply]]

(In reply to comment #33)
>  I'll get the
> original patch in on 1.7 tomorrow (later today); this got lost in my bugspam.

Thanks!

All we ask is that it matches the aviary patch exactly including uuids - thanks
again.

Comment 35

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

landed/synced on 1.7