631437 - Make SVGxxxList array indexable like NodeList (enable list[i] so authors can avoid list.getItem(i))

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

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

Comment 1

[Jonathan Watt [:jwatt]]

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

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Add length & item() to SVGXXXList interfaces and make them respond to array indexing

Comment 3

[Cameron McCormack (:heycam)]

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 4

[Jonathan Watt [:jwatt]]

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

Comment 5

[Robert Longson [:longsonr]]

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.

Comment 6

[Cameron McCormack (:heycam)]

Attachment: Add length & item() to SVGXXXList interfaces and make them respond to array indexing (v2)

(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.

Comment 7

[Jonathan Watt [:jwatt]]

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

Only content/base and dom/base.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[Cameron McCormack (:heycam)]

(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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 11

[Cameron McCormack (:heycam)]

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’

Comment 12

[Cameron McCormack (:heycam)]

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Cameron McCormack (:heycam)]

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?

Comment 15

[Jonathan Watt [:jwatt]]

(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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> 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

Comment 17

[Cameron McCormack (:heycam)]

Attachment: Add length to SVGXXXList interfaces and make them respond to array indexing (v3)

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

Comment 18

[Jonathan Watt [:jwatt]]

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 19

[Jonathan Watt [:jwatt]]

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 20

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 21

[Cameron McCormack (:heycam)]

Attachment: Add length to SVGXXXList interfaces and make them respond to array indexing (v4)

Fixed the test, carrying over r+.

Comment 22

[Cameron McCormack (:heycam)]

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?

Comment 23

[Boris Zbarsky [:bzbarsky]]

If we supported compiling against Gecko version N and then running against version N+1, then yes.

As it is, imo no.

Comment 24

[Cameron McCormack (:heycam)]

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

Comment 25

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/149b54c11f2a

Comment 26

[Eric Shepherd [:sheppy]]

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.