217159 - [FIX]Implement DOM L3 DOMStringList

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P1)

Description

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

I would like to see DOM Level 3 DOMStringList implemented since I need it for
Composer++.

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Why do you need it?  I don't know of anything that takes a DOMStringList off the
top of my head, and it's just a fancified Array.

Comment 2

[Peter Van der Beken [:peterv]]

IIRC there's only one use for DOMStringList and it's in DOM Level 3 Validation
(for enumeratedValues I think). I have an implementation in my DOM Level 3
Validation implementation, but that resides in webservices. Like caillon I
wonder why you'd need it.

Comment 3

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Hey Peter!!!
Because of this: http://daniel.glazman.free.fr/csswg/csseom/csseom.html

Comment 4

[Boris Zbarsky [:bzbarsky]]

I'm going to need this for some alternate stylesheet work I'm doing, so I'll
likely do this in the near future (like tonight).

I don't see an idl definition for this interface; where exactly in webservices
does this live?

Comment 5

[Peter Van der Beken [:peterv]]

It's not checked in yet. I'll see if I can dig it up.

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: Implementation

Comment 7

[Peter Van der Beken [:peterv]]

Attachment: IDL

Comment 8

[Peter Van der Beken [:peterv]]

Note that you'll want to use NS_INTERFACE_MAP_ENTRY_CONTENT_CLASSINFO instead of
NS_INTERFACE_MAP_ENTRY_EXTERNAL_DOM_CLASSINFO. Also, this isn't useable
cross-module, since we'd need an interface for adding the strings. You may not
want to solve that now, but if you're putting this in content I'll probably have
to figure it out in the future when I want to use it in webservices :-). You'll
also need to add it to DOMCI here:

http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsDOMClassInfo.cpp#467
http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsDOMClassInfo.cpp#1455

and we'll probably need a new scriptable helper too for operator [] access from
JS (I have a patch to provide a generic array SH but it's not ready yet).

Comment 9

[Boris Zbarsky [:bzbarsky]]

peterv, thanks!  I'll try to get that trunk-ready tonight, then.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132814 [details] [diff] [review]
IDL

r+sr=bzbarsky; the idl can be landed as-is, I would say.  Perhaps we also want
to add the interfaces to domstubs.idl?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Complete patch

This includes both patches peterv attached, plus the classinfo changes.  I
shamelessly stole Enumerate() from the content list implementation... we really
do need a generic array baseclass.

sr=bzbarsky on the actual implementation of both classes; looks good.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132908 [details] [diff] [review]
Complete patch

caillon, would you review?  peterv, would you check over the classinfo changes?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Taking.

Comment 14

[Peter Van der Beken [:peterv]]

Comment on attachment 132908 [details] [diff] [review]
Complete patch

Looks good to me, though I think we want put Enumerate on nsStringArraySH.
Could you check with jst if he's ok with that?

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 132908 [details] [diff] [review]
Complete patch

r=caillon if you fix the nsDOMLists files to use c-basic-offset: 2.

I also wonder whether nsStringArraySH should inherit from nsArraySH so you
don't even need to provide your own Enumerate() method, but I think there would
have to be some method over-riding, regardless so it may not gain anything. 
But if that doesn't fall through, how about a |static nsresult
DOMArrayEnumerator()| or something that gets called by both so as to save the
extra codesize?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Move Enumerate() into a shared superclass

The only difference is that I renamed nsArraySH to nsSupportsArraySH, added a
generic nsArraySH that just handles Enumerate, and made both nsSupportsArraySH
and nsStringArraySH inherit from it.

Thoughts?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 133307 [details] [diff] [review]
Move Enumerate() into a shared superclass

Give it a once-over?

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 133307 [details] [diff] [review]
Move Enumerate() into a shared superclass

sr=jst

Comment 19

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 133307 [details] [diff] [review]
Move Enumerate() into a shared superclass

Excellent, although I'm not sure I like the name nsSupportsArraySH since
nsISupportsArray is supposedly deprecated, and this sounds dastardly
similar....  How about leaving nsArraySH alone, and calling the superclass
nsGenericArraySH?

Oh, and when I said change the c-basic-offset, I meant not just for the
modelines, but for the actual file contents as well.  ;-)

r=caillon

Comment 20

[Boris Zbarsky [:bzbarsky]]

Made the changes caillon asked for, and checked in.