363839 - Implement all of SVGPathSegList

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jeff Schiller]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1) Gecko/20061023 SUSE/2.0-30 Firefox/2.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1) Gecko/20061023 SUSE/2.0-30 Firefox/2.0

SVGPathSegList has two major implementation issues:

1) SVGPathSegList does not implement insertItemBefore() and replaceItem()

2) As per spec http://www.w3.org/TR/SVG11/paths.html#InterfaceSVGPathSegList, appendItem() is supposed to remove a SVGPathSeg if it is already part of another list.

I have test cases and a patch for these once Bug 363086 is fixed.

Reproducible: Always

Steps to Reproduce:
1. Run test cases
2.
3.

Actual Results:  
Green squares do not result in Firefox 2 or in the Firefox trunk.

Expected Results:  
Test cases should produce green squares.

Comment 1

[Jeff Schiller]

Attachment: Test case for implementations of insertItemBefore() and replaceItem()

Comment 2

[Jeff Schiller]

Attachment: Test case for proper implementation of appendItem()

Comment 3

[Takeshi Kurosawa :takenspc]

duplicate of bug 254712?

Comment 4

[Jeff Schiller]

Seems you are right, though Bug 254712 appears to be broader than SVGPathSegList (Bug 254712 depends on Bug 282247, which says all Lists should have a base class).

I agree with these other two bugs, but it means my pending patch needs to be rewritten - or we can accept my patch for just SVGPathSegList and I will get working on Bugs 282247 and 254712.

I will leave it to tor/jwatt/roc to decide, since I'm new to this.

Comment 5

[Jeff Schiller]

Attachment: Patch (see Comment #4)

Also includes fix for Bug 363086.  See my comment #4 regarding this patch.

Comment 6

[tor]

Comment on attachment 248826 [details] [diff] [review]
Patch (see Comment #4)

>+nsCOMPtr<nsISVGValue>
>+nsSVGPathSeg::GetParent() const
>+{
>+  return do_QueryReferent(mParent);
>+}

nsCOMPtr shouldn't be used as a return value - see "nsCOMPtrs in function signatures" in http://www.mozilla.org/projects/xpcom/nsCOMPtr.html .

> /* nsIDOMSVGPathSeg insertItemBefore (in nsIDOMSVGPathSeg newItem, in unsigned long index); */
> NS_IMETHODIMP nsSVGPathSegList::InsertItemBefore(nsIDOMSVGPathSeg *newItem,
>                                                  PRUint32 index,
>                                                  nsIDOMSVGPathSeg **_retval)
> {
...
>+  // The SVG specs state that 'if newItem is already in a list, it
>+  // is removed from its previous list before it is inserted into this
>+  // list'. 
>+  nsSVGPathSeg *newSeg = NS_STATIC_CAST(nsSVGPathSeg*, newItem);
>+  if(newSeg && newSeg->GetParent()) {

mozilla code style - space after "if".

>+    // newItem's parent must be cast back to a nsSVGPathSegList*
>+    nsSVGPathSegList* oldSegList = NS_STATIC_CAST(nsSVGPathSegList*, newSeg->GetParent().get());
>+    if(oldSegList) {
>+      PRInt32 ix = oldSegList->mSegments.IndexOfObject(newSeg);
>+      if(ix != -1) { oldSegList->RemoveElementAt(ix); }

ditto, plus blocks start a new line.

>+    }
>+  }

This new code that you duplicate a number of times should be turned into a helper function/method.

Comment 7

[Jeff Schiller]

Attachment: Updated based on tor's review

New helper method added to avoid code duplication.  nsQueryReferent now returned from nsSVGPathSeg::GetParent(), I hope this is correct...

Comment 8

[Jeff Schiller]

Attachment: Update based on tor's review

woops - last patch had some cruft in it...

Comment 9

[Jeff Schiller]

Attachment: Conformed to coding convention...

Woops - somehow I missed fixing the coding convention comment.

Comment 10

[Jeff Schiller]

Attachment: Fixed patch file typo

Argh - I swear I'll get better at this... I need a better system to manage patches ;)

Comment 11

[tor]

Comment on attachment 250677 [details] [diff] [review]
Fixed patch file typo

>+void 
>+nsSVGPathSegList::EnsureElementIsParentless(nsIDOMSVGPathSeg* aSeg)
>+{
>+  nsSVGPathSeg *theSeg = NS_STATIC_CAST(nsSVGPathSeg*, aSeg);
>+  nsCOMPtr<nsISVGValue> theParent = theSeg->GetParent();
>+  if (theSeg && theParent) {
>+    // newItem's parent must be cast back to a nsSVGPathSegList*
>+    nsSVGPathSegList* otherSegList = NS_STATIC_CAST(nsSVGPathSegList*, theParent.get());
>+    if (otherSegList) {

If theParent is non-null (already checked), so will this.  Remove the extra test.

With that, r=tor.

Comment 12

[Jeff Schiller]

Attachment: Updated patch per tor's comments

Fixed per tor's comment.  However, I uncovered a bug in the proposed InsertItemBefore implementation:

Per the SVG spec, if InsertItemBefore is called with an out-of-range index, then the item is appended.  I borrowed the implementation from nsSVGNumberList.

Can you re-review?  Does it also need a super-review?

Comment 13

[Jonathan Watt [:jwatt]]

Comment on attachment 251236 [details] [diff] [review]
Updated patch per tor's comments

>+nsCOMPtr<nsISVGValue>
>+nsSVGPathSeg::GetParent() const
>+{
>+  return do_QueryReferent(mParent);
>+}

Somehow you seem to have changed the return type to nsCOMPtr, so your code won't compile.

Not something you have to do because since it wasn't you who named it, but it would be nice if this was GetOwnerList and mOwnerList. That seems like more accurate naming to me, and besides we have enough GetParent functions that it's already hard enough to search for the code you're interested in.


> NS_IMETHODIMP nsSVGPathSegList::InsertItemBefore(nsIDOMSVGPathSeg *newItem,
>                                                  PRUint32 index,
>                                                  nsIDOMSVGPathSeg **_retval)
> {
>-  // null check when implementing - this method can be used by scripts!
>-  // if (!newItem)
>-  //   return NS_ERROR_DOM_SVG_WRONG_TYPE_ERR;
>+  *_retval = newItem;
>+  if (!newItem)
>+    return NS_ERROR_DOM_SVG_WRONG_TYPE_ERR;
> 
>-  NS_NOTYETIMPLEMENTED("nsSVGPathSegList::InsertItemBefore");
>-  return NS_ERROR_NOT_IMPLEMENTED;
>+  EnsureElementIsParentless(newItem);

In my opinion we should not remove the item from its current list unless we successfully add it to this one. Therefore EnsureElementIsParentless should be called last. Actually I think you would be better to only call EnsureElementIsParentless in AppendElement and InsertElementAt (at the end of those methods of course). Then you only need two calls instead of three.


>+  PRInt32 idx = index;
>+  PRInt32 count = mElements.Count();
>+  if (!InsertElementAt(newItem, (idx < count)? idx: count)) {

This is a good idea, but again it won't compile because nsSVGPathSegList::InsertElementAt has void as its return value. I'd suggest just leaving as it was in your previous patch for now. When all List implementations get merged into a base class then they should pick up the improvements in nsSVGTransformList which does check for this type of failure. For now just keep it simple.


>+nsSVGPathSegList::EnsureElementIsParentless(nsIDOMSVGPathSeg* aSeg)
>+{
>+  nsSVGPathSeg *theSeg = NS_STATIC_CAST(nsSVGPathSeg*, aSeg);
>+  nsCOMPtr<nsISVGValue> theParent = theSeg->GetParent();
>+  if (theSeg && theParent) {
>+    // newItem's parent must be cast back to a nsSVGPathSegList*
>+    nsSVGPathSegList* otherSegList = NS_STATIC_CAST(nsSVGPathSegList*, theParent.get());
>+    PRInt32 ix = otherSegList->mSegments.IndexOfObject(theSeg);
>+    if (ix != -1) { otherSegList->RemoveElementAt(ix); }

Please spread that back over three lines as it was.

r- since this won't compile. Please be sure to check your patches compile and are tested before putting them up for review.

Comment 14

[Jeff Schiller]

Thanks, Jonathan.  I agree with your first two suggestions 1) renaming GetParent, SetParent and 2) fixing GetParent() again, which I accidentally regressed from my earlier patch.

Also, sorry about not testing the patch first - you're right, InsertElementAt() would need to be modified for its return type.  I agree that I will do this with the base List class later.

And again, for the coding convention, I apologize for regressing that yet again.  I need a better system of managing my patches, clearly.

As for calling EnsureElementIsParentless() - we can't call that afterwards, because it removes the element from its current list (which is what AppendItem does).  I'll have to figure out a better solution (I guess save the old parent and then remove it from the old list after we've successfully added it to the new list).

Comment 15

[Jeff Schiller]

Attachment: Implemented all of jwatt's comments

Comment 16

[Jeff Schiller]

Btw, i took your suggestion and moved EnsureElementIsParentless() into the append/insert helper functions as you suggested.  This somewhat solved my dilemma since by the time we get to the helper functions we're definitely doing the add/insert (there are no error scenarios there).  I still do it first though within those helper functions.

Comment 17

[Jonathan Watt [:jwatt]]

Comment on attachment 251287 [details] [diff] [review]
Implemented all of jwatt's comments

+nsSVGPathSeg::SetOwnerList(nsISVGValue* aParent)

aParent -> aOwnerList

>+  nsresult SetOwnerList(nsISVGValue* aParent);

same

>+  void EnsureElementIsParentless(nsIDOMSVGPathSeg*);

Again, the word "parent" is in the name. Maybe this should be called RemoveFromCurrentList? Hmm, and then SetOwnerList should be named SetCurrentList. :-/ Actually I do like that better if you don't mind doing it. Note that would change my comments above of course.

>+  EnsureElementIsParentless(aElement);

Can you put a comment before these two calls along the lines of:

  // XXX: we should only remove an item from its current list if we
  // successfully added it to this list

>+nsSVGPathSegList::EnsureElementIsParentless(nsIDOMSVGPathSeg* aSeg)
>+{
>+  nsSVGPathSeg *theSeg = NS_STATIC_CAST(nsSVGPathSeg*, aSeg);
>+  nsCOMPtr<nsISVGValue> theParent = theSeg->GetOwnerList();
>+  if (theSeg && theParent) {

Checking theSeg is redundant.

Comment 18

[Jeff Schiller]

Attachment: Updated for jwatt's suggested naming

Comment 19

[Jonathan Watt [:jwatt]]

Comment on attachment 251294 [details] [diff] [review]
Updated for jwatt's suggested naming

>+  nsSVGPathSeg() : mCurrentList(nsnull) {}
>+  nsresult SetCurrentList(nsISVGValue* aParent);

aCurrentList

>+nsSVGPathSegList::RemoveFromCurrentList(nsIDOMSVGPathSeg* aSeg)
>+{
>+  nsSVGPathSeg *theSeg = NS_STATIC_CAST(nsSVGPathSeg*, aSeg);
>+  nsCOMPtr<nsISVGValue> theParent = theSeg->GetCurrentList();
>+  if (theParent) {

currentList please. Bet that's the last time you listen to me about better naming. ;-)

r=jwatt. Nice. Thanks Jeff!

Comment 20

[Jeff Schiller]

Ugh, I was going to do a text search for "parent" too - looks like I should have, sorry.  Can't re-submit the patch until tonight now though.

Comment 21

[Jeff Schiller]

Attachment: Final patch

Comment 22

[Jonathan Watt [:jwatt]]

Checked in for Jeff.