345085 - Create common base class for path segments

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Alex Menzies]

Upcoming patch modifies the various path segment classes defined in nsSVGPathSeg.cpp to inherit from a common base class nsSVGPathSeg instead of nsSVGValue.  Much thanks to tor for providing a partially completed patch which served as a template for many of the changes made here.

Comment 1

[Alex Menzies]

Attachment: Patch

Comment 2

[tor]

I think you'll find with this patch that if you modify a pathseg, the path you see, and that returned by getAttribute, will not be updated.  To do that nsSVGPathSegList::GetValueString() needs to be able to create a string from the current segment list.

This would have been handled with my path data representation path through the nsSVGPathList::GetValueString() method.

While your nsSVGPathSeg has a mParent field used to push updates, it's never set to anything other than nsnull.  When added to a list it needs to be updated, likewise when removed and/or the list goes away.

Comment 3

[Alex Menzies]

Attachment: Patch with getItem Bug

Comment 4

[Alex Menzies]

Attachment: Updated Patch with getItem fix

Comment 5

[tor]

(In reply to comment #4)
> Created an attachment (id=230176) [edit]
> Updated Patch with getItem fix

Ownership in nsSVGPathSegList is confused - since you're now using a nsCOMArray, you shouldn't need to mess with ref count manually anymore.

Comment 6

[Alex Menzies]

Attachment: Ref Count fix

Comment 7

[tor]

Comment on attachment 230595 [details] [diff] [review]
Ref Count fix

>+class nsSVGPathSeg : public nsIDOMSVGPathSeg
>+{
>+  nsISVGValue *mParent;

Having just a raw pointer to the parent will cause problems if the parent goes away.  Example:

  * In JS, find a path element and do a getItem to obtain a variable pointing to a pathseg

  * Delete the path element

  * Try modifying the stored pathseg

Switching to a nsCOMPtr would cause a reference loop between the element and seg list, so you'll probably need to use a nsIWeakReference.

> ////////////////////////////////////////////////////////////////////////
> // nsSVGPathSegList
>-  nsVoidArray mSegments;
>+  nsCOMArray<nsIDOMSVGPathSeg> mSegments;
>+  PRPackedBool mModifyObservableEnabled;

Do we need this flag?  It's needed just for SetValueString to clear the old value.  Could we just pass it as an argument to ReleaseSegments?

Destructor doesn't need to call ReleaseSegments anymore.

Comment 8

[Alex Menzies]

Attachment: Updated Reference counting

Comment 9

[Alex Menzies]

Attachment: Patch with stubs

Update to patch stubs out Will/DidModifySVGObservable

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why not change all NS_NewSVGPathSeg* functions to return a non-addrefed nsIDOMSVGPathSeg* as a direct result? That would save adding/releasing a ref in the nsSVGPathDataParserToDOM::Store* functions. In fact all those functions could be reduced to a call to a helper function like this: 

  return AppendNewSegment(
    absCoords ? NS_NewSVGPathSegLinetoHorizontalAbs(x)
              : NS_NewSVGPathSegLinetoHorizontalRel(x));

where AppendNewSegment would handle conversion of nsnull to NS_ERROR_OUT_OF_MEMORY and the call to AppendSegment (actually I guess it would replace AppendSegment).

It would also simplify nsSVGPathElement::Create* by removing the need for an nsCOMPtr there.

Comment 11

[Alex Menzies]

Attachment: Patch w/NS_NewSVGPathSeg changes

Comment 12

[tor]

Checked in for Alex.