Closed Bug 345085 Opened 14 years ago Closed 14 years ago

Create common base class for path segments

Categories

(Core :: SVG, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: malex, Assigned: malex)

Details

Attachments

(1 file, 6 obsolete files)

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.
Assignee: general → amenzie
Attached patch Patch (obsolete) — Splinter Review
Attachment #229698 - Flags: review?(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.
Attachment #229698 - Flags: review?(tor) → review-
Attached patch Patch with getItem Bug (obsolete) — Splinter Review
Attachment #229698 - Attachment is obsolete: true
Attached patch Updated Patch with getItem fix (obsolete) — Splinter Review
Attachment #230161 - Attachment is obsolete: true
Attachment #230176 - Flags: review?(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.
Attachment #230176 - Flags: review?(tor) → review-
Attached patch Ref Count fix (obsolete) — Splinter Review
Attachment #230176 - Attachment is obsolete: true
Attachment #230595 - Flags: review?(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.
Attachment #230595 - Flags: review?(tor) → review-
Attached patch Updated Reference counting (obsolete) — Splinter Review
Attachment #230595 - Attachment is obsolete: true
Attached patch Patch with stubs (obsolete) — Splinter Review
Update to patch stubs out Will/DidModifySVGObservable
Attachment #230817 - Attachment is obsolete: true
Attachment #231481 - Flags: review?(tor)
Attachment #231481 - Flags: review?(tor) → review+
Attachment #231481 - Flags: superreview?(roc)
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.
Attachment #231481 - Attachment is obsolete: true
Attachment #232917 - Flags: review?(tor)
Attachment #231481 - Flags: superreview?(roc)
Attachment #232917 - Flags: review?(tor) → review+
Attachment #232917 - Flags: superreview?(roc)
Attachment #232917 - Flags: superreview?(roc) → superreview+
Checked in for Alex.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.