377015 - Simplify nsSVGClipPathFrame API and make clip reference loop detection more robust

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Attachment: patch

Could do with an AutoxxxReferencer as per marker frames. Also the API could be simplified now that we are no longer using interfaces.

Comment 1

[Jonathan Watt [:jwatt]]

Comment on attachment 261132 [details] [diff] [review]
patch

>+nsSVGClipPathFrame::AutoClipPathReferencer::AutoClipPathReferencer(
>+    nsSVGClipPathFrame *aFrame)
>+      : mFrame(aFrame)
>+{

Can you add:

  NS_ASSERTION(mFrame->mInUse == PR_FALSE, "reference loop!");

>+  mFrame->mInUse = PR_TRUE;
>+}
>+
>+nsSVGClipPathFrame::AutoClipPathReferencer::~AutoClipPathReferencer()
>+{
>+  mFrame->mInUse = PR_FALSE;
>+}

Also please just move the definition of the class to the header so it gets inlined and is easier to understand when first encountered.

Comment 2

[Jonathan Watt [:jwatt]]

Comment on attachment 261132 [details] [diff] [review]
patch

>+  // VC6 does not allow the inner class to access protected members
>+  // of the outer class
>+  class AutoClipPathReferencer;
>+  friend class AutoClipPathReferencer;

Oh, and VC6 is no longer supported for building trunk, so you can probably remove this clutter.

Comment 3

[Robert Longson [:longsonr]]

Attachment: address review comments

Comment 4

[tor]

Comment on attachment 261508 [details] [diff] [review]
address review comments

>+  nsresult ClipPaint(nsSVGRenderState* aContext,
>+                     nsISVGChildFrame* aParent,
>+                     nsCOMPtr<nsIDOMSVGMatrix> aMatrix);
>+
>+  PRBool ClipHitTest(nsISVGChildFrame* aParent,
>+                     nsCOMPtr<nsIDOMSVGMatrix> aMatrix,
>+                     float aX, float aY);

While you're changing the signature, you could also remove the nsCOMPtrs and replace with pointers.  With that, sr=tor.

Comment 5

[Robert Longson [:longsonr]]

Attachment: version for check in

Comment 6

[Robert Longson [:longsonr]]

Attachment: right version for check in

Comment 7

[Robert Longson [:longsonr]]

checked in.

Comment 8

[Robert Longson [:longsonr]]

Attachment: vc6 compatibility in case anyone is interested

Comment 9

[Jeff Walden [:Waldo]]

This seems more like code cleanup than known bug-fixing, so marking in-testsuite-.  If someone can find things that were broken before this, tho, that'd be better than not adding any tests.  :-)