Last Comment Bug 631437 - Make SVGxxxList array indexable like NodeList (enable list[i] so authors can avoid list.getItem(i))
: Make SVGxxxList array indexable like NodeList (enable list[i] so authors can ...
Status: RESOLVED FIXED
[parity-Opera]
: dev-doc-complete
Product: Core
Classification: Components
Component: SVG (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla5
Assigned To: Cameron McCormack (:heycam)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 722071
  Show dependency treegraph
 
Reported: 2011-02-03 19:49 PST by Jonathan Watt [:jwatt]
Modified: 2012-01-28 11:07 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add length & item() to SVGXXXList interfaces and make them respond to array indexing (32.46 KB, patch)
2011-03-21 01:40 PDT, Cameron McCormack (:heycam)
no flags Details | Diff | Splinter Review
Add length & item() to SVGXXXList interfaces and make them respond to array indexing (v2) (46.55 KB, patch)
2011-03-21 14:23 PDT, Cameron McCormack (:heycam)
bzbarsky: review-
Details | Diff | Splinter Review
Add length to SVGXXXList interfaces and make them respond to array indexing (v3) (45.19 KB, patch)
2011-03-22 16:05 PDT, Cameron McCormack (:heycam)
jwatt: review+
bzbarsky: review+
Details | Diff | Splinter Review
Add length to SVGXXXList interfaces and make them respond to array indexing (v4) (45.94 KB, patch)
2011-03-23 18:18 PDT, Cameron McCormack (:heycam)
cam: review+
Details | Diff | Splinter Review

Description Jonathan Watt [:jwatt] 2011-02-03 19:49:07 PST
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] 2011-02-28 22:34:30 PST
Opera has apparently done this now, and also added .length and .item() members.
Comment 2 Cameron McCormack (:heycam) 2011-03-21 01:40:54 PDT
Created attachment 520606 [details] [diff] [review]
Add length & item() to SVGXXXList interfaces and make them respond to array indexing
Comment 3 Cameron McCormack (:heycam) 2011-03-21 01:44:41 PDT
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] 2011-03-21 02:42:53 PDT
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 2011-03-21 06:30:04 PDT
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) 2011-03-21 14:23:07 PDT
Created attachment 520750 [details] [diff] [review]
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] 2011-03-21 16:13:42 PDT
(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 [:bz] (still a bit busy) 2011-03-21 20:31:19 PDT
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) 2011-03-21 20:46:12 PDT
(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 [:bz] (still a bit busy) 2011-03-21 21:00:28 PDT
> 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) 2011-03-21 21:08:19 PDT
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) 2011-03-21 21:08:58 PDT
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 [:bz] (still a bit busy) 2011-03-21 21:13:33 PDT
Well, you need to includes those headers in nsDOMClassInfo.cpp, right?  Doesn't sound like you did.
Comment 14 Cameron McCormack (:heycam) 2011-03-21 21:43:52 PDT
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] 2011-03-21 21:55:11 PDT
(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 [:bz] (still a bit busy) 2011-03-21 22:09:17 PDT
> 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) 2011-03-22 16:05:35 PDT
Created attachment 521062 [details] [diff] [review]
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] 2011-03-22 16:40:02 PDT
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] 2011-03-22 17:00:06 PDT
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 [:bz] (still a bit busy) 2011-03-22 19:14:56 PDT
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) 2011-03-23 18:18:51 PDT
Created attachment 521374 [details] [diff] [review]
Add length to SVGXXXList interfaces and make them respond to array indexing (v4)

Fixed the test, carrying over r+.
Comment 22 Cameron McCormack (:heycam) 2011-03-24 18:33:31 PDT
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 [:bz] (still a bit busy) 2011-03-24 18:40:56 PDT
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) 2011-04-07 15:51:17 PDT
Landed on cedar: http://hg.mozilla.org/projects/cedar/rev/149b54c11f2a
Comment 26 Eric Shepherd [:sheppy] 2011-04-21 10:50:53 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.