Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla5

Status

()

Core
SVG
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: jwatt, Assigned: heycam)

Tracking

({dev-doc-complete})

unspecified
mozilla5
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [parity-Opera])

Attachments

(1 attachment, 3 obsolete attachments)

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
(Reporter)

Updated

7 years ago
Summary: Make SVGxxxList array indexable like NodeList → Make SVGxxxList array indexable like NodeList (enable list[i] so authors can avoid list.getItem(i))
Opera has apparently done this now, and also added .length and .item() members.
Whiteboard: [parity-Opera]
(Assignee)

Updated

6 years ago
Assignee: nobody → cam
(Assignee)

Comment 2

6 years ago
Created attachment 520606 [details] [diff] [review]
Add length & item() to SVGXXXList interfaces and make them respond to array indexing
(Assignee)

Comment 3

6 years ago
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.
Attachment #520606 - Flags: review?(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

6 years ago
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.
(Assignee)

Comment 6

6 years ago
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.
Attachment #520606 - Attachment is obsolete: true
Attachment #520750 - Flags: review?(jwatt)
Attachment #520750 - Flags: review?(bzbarsky)
Attachment #520606 - Flags: review?(jwatt)
(In reply to comment #6)
> Boris can you review my changes in content/ and dom/?

Only content/base and dom/base.

Comment 8

6 years ago
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.
Attachment #520750 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 9

6 years ago
(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

6 years ago
> 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?
(Assignee)

Comment 11

6 years ago
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’
(Assignee)

Comment 12

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

Comment 13

6 years ago
Well, you need to includes those headers in nsDOMClassInfo.cpp, right?  Doesn't sound like you did.
(Assignee)

Comment 14

6 years ago
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.

Comment 16

6 years ago
> 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
(Assignee)

Comment 17

6 years ago
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.
Attachment #520750 - Attachment is obsolete: true
Attachment #521062 - Flags: review?(jwatt)
Attachment #521062 - Flags: review?(bzbarsky)
Attachment #520750 - Flags: review?(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 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.
Attachment #521062 - Flags: review?(jwatt) → review+

Comment 20

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

r=me
Attachment #521062 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 21

6 years ago
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+.
Attachment #521062 - Attachment is obsolete: true
Attachment #521374 - Flags: review+
(Assignee)

Comment 22

6 years ago
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?
Status: NEW → ASSIGNED

Comment 23

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

As it is, imo no.
(Assignee)

Comment 24

6 years ago
Landed on cedar: http://hg.mozilla.org/projects/cedar/rev/149b54c11f2a
Whiteboard: [parity-Opera] → [parity-Opera][fixed-in-cedar]
http://hg.mozilla.org/mozilla-central/rev/149b54c11f2a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [parity-Opera][fixed-in-cedar] → [parity-Opera]
Target Milestone: --- → mozilla2.2

Updated

6 years ago
Keywords: dev-doc-needed
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.
Keywords: dev-doc-needed → dev-doc-complete
(Reporter)

Updated

6 years ago
Blocks: 722071
You need to log in before you can comment on or make changes to this bug.