We should make SVGxxxList array indexable like NodeList. For that we need to use nsNodeListSH, I believe: http://mxr.mozilla.org/mozilla-central/search?string=%28NodeList,+nsNodeListSH,+ARRAY_SCRIPTABLE_FLAGS%29

Opera has apparently done this now, and also added .length and .item() members.

Comment on attachment 520606 [details] [diff] [review] Add length & item() to SVGXXXList interfaces and make them respond to array indexing I ended up having to add a number of headers to the EXPORTS in content/base/src/Makefile.in; don't know if that's something that should be avoided (or can be), if nsDOMClassInfo needs to include DOMSVGLengthList.h etc.

Comment on attachment 520606 [details] [diff] [review] Add length & item() to SVGXXXList interfaces and make them respond to array indexing >+ // Behavioral compatibility with NodeList etc. >+ readonly attribute unsigned long length; >+ nsIDOMSVGPoint item(in unsigned long index); Personally I don't think we should add item(). Array style indexing should be the encouraged method IMO. >+DOMSVGLengthList::GetItem_WithoutAddRef(PRUint32 aIndex) Just make that GetItemWithoutAddRef - we don't use underscore in names. You should also make the DOM GetItem use this method and addref the object returned, or return the index error if nothing is returned (abort on OOM makes this safe). You need to get review for the changes under the DOM module from a DOM peer: https://wiki.mozilla.org/Modules/Core#Document_Object_Model

Please don't add more MOZ_SVG defines, we're trying to remove the ones that are already there. Compiling with SVG on is mandatory. > // If this assertion fires the QI implementation for the object in // If this assertion fires, the QI implementation for the object in > NS_ASSERTION(list_qi == list, "Uh, fix QI!"); So that might as well be ABORT_IF_FALSE then if we'll crash anyway.

(In reply to comment #4) > Personally I don't think we should add item(). Array style indexing should be > the encouraged method IMO. OK, removed. > >+DOMSVGLengthList::GetItem_WithoutAddRef(PRUint32 aIndex) > > Just make that GetItemWithoutAddRef - we don't use underscore in names. OK. (I just copied the naming from an example in https://developer.mozilla.org/en/Using_nsCOMPtr/Reference_Manual#.22Out.22_Parameters.3a_getter_AddRefs .) > You should also make the DOM GetItem use this method and addref the object > returned, or return the index error if nothing is returned (abort on OOM makes > this safe). Good idea! Done. > You need to get review for the changes under the DOM module from a DOM peer: > https://wiki.mozilla.org/Modules/Core#Document_Object_Model Boris can you review my changes in content/ and dom/? (In reply to comment #5) > Please don't add more MOZ_SVG defines, we're trying to remove the ones that are > already there. Compiling with SVG on is mandatory. OK, I've removed them (as well as other MOZ_SVG ifdefs in the files I touched). > > // If this assertion fires the QI implementation for the object in > > > NS_ASSERTION(list_qi == list, "Uh, fix QI!"); > > So that might as well be ABORT_IF_FALSE then if we'll crash anyway. OK. There are a few copies of this NS_ASSERTION in nsDOMClassInfo.cpp, so I've changed all of them to be NS_ABORT_IF_FALSE.

(In reply to comment #6) > Boris can you review my changes in content/ and dom/? Only content/base and dom/base.

Comment on attachment 520750 [details] [diff] [review] Add length & item() to SVGXXXList interfaces and make them respond to array indexing (v2) Why are we exporting nsAttrAndChildArray? I don't think we want to do that.... Similar for nsAttrValue, nsDOMAttributeMap, nsGenericElement, nsNodeUtils, nsStyledElement. These are NOT public headers! Similar for the headers in content/svg/content. The checkin comment needs to not mention item(). The rest looks fine.

(In reply to comment #8) > Why are we exporting nsAttrAndChildArray? I don't think we want to do that.... > Similar for nsAttrValue, nsDOMAttributeMap, nsGenericElement, nsNodeUtils, > nsStyledElement. These are NOT public headers! I wanted to have #include "DOMSVGLengthList.h" #include "DOMSVGNumberList.h" #include "DOMSVGPathSegList.h" #include "DOMSVGPointList.h" in DOMClassInfo.h, because the typedefs I've added right at the bottom of DOMClassInfo.h need the full class definition and not just a forward declaration, apparently (since they're used as template parameters). And I just followed the dependency chain to its logical conclusion! :) Is there a way I can avoid #including those headers without de-templatizing nsSVGListSH? > Similar for the headers in content/svg/content. > > The checkin comment needs to not mention item(). Oops, will fix.

> Is there a way I can avoid #including those headers without de-templatizing > nsSVGListSH? Yes. You could move the class decl (or the typedefs) into nsDOMClassInfo.cpp, since it's not used outside that file. You could change LOCAL_INCLUDES lines in makefiles as needed to include whatever needs to be included. But none of that should be needed; you should be able to just use the forward declarations here. What compiler did you see it fail with, exactly? What was the error message?

If I replace those #includes with namespace mozilla { class DOMSVGLengthList; class DOMSVGNumberList; class DOMSVGPathSegList; class DOMSVGPointList; } then I get: /z/mozilla/dom/base/nsDOMClassInfo.cpp: In member function ‘nsISupports* nsSVGListSH<ListInterfaceType, ListType>::GetItemAt(nsISupports*, PRUint32, nsWrapperCache**, nsresult*) [with ListInterfaceType = nsIDOMSVGPointList, ListType = mozilla::DOMSVGPointList]’: /z/mozilla/dom/base/nsDOMClassInfo.cpp:10787: instantiated from here /z/mozilla/dom/base/nsDOMClassInfo.cpp:10775: error: invalid static_cast from type ‘nsIDOMSVGPointList*’ to type ‘mozilla::DOMSVGPointList*’ /z/mozilla/dom/base/nsDOMClassInfo.cpp:10787: instantiated from here /z/mozilla/dom/base/nsDOMClassInfo.cpp:10787: error: invalid use of incomplete type ‘struct mozilla::DOMSVGPointList’ /z/mozilla/dom/base/nsDOMClassInfo.h:60: error: forward declaration of ‘struct mozilla::DOMSVGPointList’

So it's the fact that they're template parameters, and they're used in a static_cast like that, I suppose.

Well, you need to includes those headers in nsDOMClassInfo.cpp, right? Doesn't sound like you did.

No, I didn't. :) I added them now, and it all compiles if I keep those added EXPORT entries in content/svg/content/src/Makefile.in. Cool. jwatt: is it OK for all 14 of those .h files to be exported, or are some of them really internal? If the answer is that we shouldn't export all of those, do I then just add those ones to LOCAL_INCLUDES in dom/base/Makefile.in?

(In reply to comment #14) > do I then just add those ones to LOCAL_INCLUDES in dom/base/Makefile.in? Yeah, that's what we should do.

> if I keep those added EXPORT entries in content/svg/content/src/Makefile.in Add the relevant paths to DOM_SRCDIRS in dom/dom-config.mk

Adding content/svg/content/src/ to DOM_SRCDIRS did the trick, without needing to export anything.

Comment on attachment 521062 [details] [diff] [review] Add length to SVGXXXList interfaces and make them respond to array indexing (v3) >+++ b/dom/interfaces/svg/nsIDOMSVGPointList.idl >@@ -35,20 +35,21 @@ > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ > > #include "domstubs.idl" > > interface nsIDOMSVGPoint; > >-[scriptable, uuid(4c12af24-0fc2-4fe7-b71d-5d6b41d463c1)] >+[scriptable, uuid(7bb28750-7238-4083-b5f4-4def4646a637)] > interface nsIDOMSVGPointList : nsISupports > { > readonly attribute unsigned long numberOfItems; >+ readonly attribute unsigned long length; // synonym for numberOfItems I believe you're only supposed to append to the interface for binary compatibility, but you should check with someone that knows the rules here.

Comment on attachment 521062 [details] [diff] [review] Add length to SVGXXXList interfaces and make them respond to array indexing (v3) >+function checkList(list, desc, expr) >+{ >+ is(list.numberOfItems, list.length, desc + ": " + expr + ".length"); >+ for (let i = 0; i < list.length; i++) { >+ let item = list.getItem(i); >+ ok(list[i] === item, desc + ": " + expr + "[" + i + "]"); >+ } >+ ok(list[list.length] === void 0, desc + ": " + expr + "[" + expr + ".length]"); >+} I think rather than checking .length == .numberOfItems you should pass in the expected length to this function, or use a global var. (Or test both if you like.) You'll then discover a bug in your test. Okay, okay, I'll point that out too... :) >+var tests = [ >+ function() { }, >+ function() { >+ text.setAttribute("x", "40"); >+ text.setAttribute("rotate", "10"); >+ path.setAttribute("path", "M50,50"); >+ path.setAttribute("polygon", "100,100"); The attribute names are "d" and "points". ;) Same in the other function. >+for each (let fn in tests) { >+ fn(); Please s/fn/setAttribs/ (or some other more meaningful name). Possibly also s/tests/attribSetters/ too? Other than that, r=jwatt for the content/svg changes.

Comment on attachment 521062 [details] [diff] [review] Add length to SVGXXXList interfaces and make them respond to array indexing (v3) r=me

Fixed the test, carrying over r+.

Boris, do you know if Jonathan's comment 18 about only adding members to the end of the interface for binary compatibility is something I need to worry about?

If we supported compiling against Gecko version N and then running against version N+1, then yes. As it is, imo no.

Landed on cedar: http://hg.mozilla.org/projects/cedar/rev/149b54c11f2a

We don't have docs for these SVG interfaces at this time, although those are forthcoming as part of the ongoing work on the SVG Reference. However, these changes are now mentioned on Firefox 5 for developers.