474230 - Cleanup scale and translate in nsSVGElement similar to the new style SVG classes

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Craig Topper]

Attachment: Patch

Scale and translate in nsSVGSVGElement are stored as an IDOMSVGNumber and an IDOMSVGPoint. nsSVGValue and nsISVGValueObserver are used to track changes to these. These should be cleaned up similar to the other new style classes to remove the observers.

Comment 1

[Craig Topper]

Attachment: Updated to tip

Comment 2

[Jonathan Watt [:jwatt]]

Comment on attachment 360444 [details] [diff] [review]
Updated to tip

>+++ b/content/svg/content/src/nsDOMSVGZoomEvent.cpp
>-#include "nsIDOMSVGSVGElement.h"

Probably shouldn't remove this include.


>+nsSVGTranslatePoint::DOMVal::SetX(float aValue)
>+{
>+  NS_ENSURE_FINITE(aValue, NS_ERROR_ILLEGAL_VALUE);
>+  mElement->RecordCurrentScaleTranslate();
>+  mVal->SetX(aValue);
>+  mElement->DidChangeTranslate();

Why not just use |mElement->SetCurrentTranslate(aValue, mVal->GetY());|? That will reduce the number of lines of code here and remove the need for nsSVGSVGElement::DidChangeTranslate.


>+nsSVGTranslatePoint::DOMVal::MatrixTransform(nsIDOMSVGMatrix *matrix,
>+                                             nsIDOMSVGPoint **_retval)
>+{
>+  NS_ENSURE_TRUE(matrix, NS_ERROR_DOM_SVG_WRONG_TYPE_ERR);

The NS_ENSURE_TRUE macro is used to handle programming errors (it asserts). You should just use an 'if' statement here.


>   // initialise "Previous" values
>   RecordCurrentScaleTranslate();

I don't see why this is necessary. Just remove it? In fact that means nsSVGSVGElement::Init() can die.


> nsSVGSVGElement::SetCurrentScale(float aCurrentScale)
> {
>   NS_ENSURE_FINITE(aCurrentScale, NS_ERROR_ILLEGAL_VALUE);

Can you add a check for |aCurrentScale == mCurrentScale| here, and return if that's the case?


>+  // now dispatch an SVGZoom event if we are the root element
>+  nsIDocument* doc = GetCurrentDoc();
>+  if (doc) {
>+    nsCOMPtr<nsIPresShell> presShell = doc->GetPrimaryShell();
>+    NS_ASSERTION(presShell, "no presShell");
>+    if (presShell && IsRoot()) {
>+      nsEventStatus status = nsEventStatus_eIgnore;
>+      nsGUIEvent event(PR_TRUE, NS_SVG_ZOOM, 0);
>+      event.eventStructType = NS_SVGZOOM_EVENT;
>+      presShell->HandleDOMEventWithTarget(this, &event, &status);
>+      InvalidateTransformNotifyFrame();

The InvalidateTransformNotifyFrame() call should happen regardless of whether we succeed in sending an event. Just move it to the end of the function. This comment and the previous comment also apply to SetCurrentScaleTranslate and SetCurrentTranslate.


>+  struct DOMVal : public nsIDOMSVGPoint {
>+    NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>+    NS_DECL_CYCLE_COLLECTION_CLASS(DOMVal)
>+
>+    DOMVal(nsSVGTranslatePoint* aVal, nsSVGSVGElement *aElement)
>+      : mVal(aVal), mElement(aElement) {}
>+
>+    nsSVGTranslatePoint *mVal;
>+    nsRefPtr<nsSVGSVGElement> mElement;

Please put the members at the end.


>+    NS_IMETHOD GetX(float *aValue)
>+      { *aValue = mVal->GetX(); return NS_OK; }
>+    NS_IMETHOD GetY(float *aValue)
>+      { *aValue = mVal->GetY(); return NS_OK; }
>+
>+    NS_IMETHOD SetX(float aValue);
>+    NS_IMETHOD SetY(float aValue);
>+
>+    NS_IMETHOD MatrixTransform(nsIDOMSVGMatrix *matrix,
>+                               nsIDOMSVGPoint **_retval);
>+  };
>+
>+public:
>+  nsresult ToDOMVal(nsIDOMSVGPoint **aResult,
>+                    nsSVGSVGElement *aElement)

I think standard practice is to make the out-param the last parameter.


>+    {
>+      *aResult = new DOMVal(this, aElement);
>+      NS_ENSURE_TRUE(*aResult, NS_ERROR_OUT_OF_MEMORY);

The NS_ENSURE_TRUE macro is used to handle programming errors (it asserts). You should just use an 'if' statement for OOM.


>+  PRPackedBool mHasScaleTranslate;

I think this would be better named "mIsRootContent".


I'd like to do a second pass over this patch, so once you've addressed the comments above please re-request review from me.

Comment 3

[Craig Topper]

(In reply to comment #2)
> >+  // now dispatch an SVGZoom event if we are the root element
> >+  nsIDocument* doc = GetCurrentDoc();
> >+  if (doc) {
> >+    nsCOMPtr<nsIPresShell> presShell = doc->GetPrimaryShell();
> >+    NS_ASSERTION(presShell, "no presShell");
> >+    if (presShell && IsRoot()) {
> >+      nsEventStatus status = nsEventStatus_eIgnore;
> >+      nsGUIEvent event(PR_TRUE, NS_SVG_ZOOM, 0);
> >+      event.eventStructType = NS_SVGZOOM_EVENT;
> >+      presShell->HandleDOMEventWithTarget(this, &event, &status);
> >+      InvalidateTransformNotifyFrame();
> 
> The InvalidateTransformNotifyFrame() call should happen regardless of whether
> we succeed in sending an event. Just move it to the end of the function. This
> comment and the previous comment also apply to SetCurrentScaleTranslate and
> SetCurrentTranslate.
> 
> 

The behavior I have is consistent with what DidModifySVGObservable did previously.

I will post the rest of the changes soon.

Comment 4

[Craig Topper]

Attachment: Addresses jwatt's comments

Comment 5

[Robert Longson [:longsonr]]

Comment on attachment 360661 [details] [diff] [review]
Addresses jwatt's comments

>diff --git a/content/svg/content/src/nsSVGSVGElement.h b/content/svg/content/src/nsSVGSVGElement.h
>--- a/content/svg/content/src/nsSVGSVGElement.h
>+++ b/content/svg/content/src/nsSVGSVGElement.h
>@@ -41,31 +41,88 @@
> #define __NS_SVGSVGELEMENT_H__
> 
> #include "nsSVGStylableElement.h"
> #include "nsIDOMSVGSVGElement.h"
> #include "nsIDOMSVGFitToViewBox.h"
> #include "nsIDOMSVGLocatable.h"
> #include "nsIDOMSVGZoomAndPan.h"
> #include "nsIDOMSVGMatrix.h"
>+#include "nsIDOMSVGNumber.h"
>+#include "nsIDOMSVGPoint.h"

Why do you need these lines?

Comment 6

[Craig Topper]

(In reply to comment #5)
> (From update of attachment 360661 [details] [diff] [review])
> >diff --git a/content/svg/content/src/nsSVGSVGElement.h b/content/svg/content/src/nsSVGSVGElement.h
> >--- a/content/svg/content/src/nsSVGSVGElement.h
> >+++ b/content/svg/content/src/nsSVGSVGElement.h
> >@@ -41,31 +41,88 @@
> > #define __NS_SVGSVGELEMENT_H__
> > 
> > #include "nsSVGStylableElement.h"
> > #include "nsIDOMSVGSVGElement.h"
> > #include "nsIDOMSVGFitToViewBox.h"
> > #include "nsIDOMSVGLocatable.h"
> > #include "nsIDOMSVGZoomAndPan.h"
> > #include "nsIDOMSVGMatrix.h"
> >+#include "nsIDOMSVGNumber.h"
> >+#include "nsIDOMSVGPoint.h"
> 
> Why do you need these lines?

nsIDOMSVGPoint.h is needed. nsIDOMSVGNumber.h is not. I believe the latter was left in from an earlier version of this patch I was working on before I realized scale didn't need to be a number at all.

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: patch addressing jwatt's second round of comments

Sorry Craig. I really dropped the ball on this one. :-( I have a few other comments, but I feel bad about asking you to incorporate them at this stage. Rather than do that I've updated your patch to tip for you and integrated my comments while I was doing that. A list of those changes follow. If you're happy with them then let me know and I'll r+ it and we can get your patch pushed. So here's what I changed:

I removed the RecordCurrentScaleTranslate call from the nsDOMSVGZoomEvent ctor since actually that's not needed. Any script generated SVGZoom events can not set the zoom (currently), in which case it seems fine that they see "previous" values from the last "real" SVGZoom event (if any). If we add a createSVGZoomEvent function in future, we can update the previous values in that function. I also moved the new lines around a little.

I changed SetCurrentScale and SetCurrentTranslate so that they just call SetCurrentScaleTranslate to eliminate code duplication. That made SetCurrentScaleTranslate the only caller of RecordCurrentScaleTranslate, so I merged RecordCurrentScaleTranslate into SetCurrentScaleTranslate too. I added sanity checks and early returns to SetCurrentScaleTranslate - since SetCurrentTranslate and SetCurrentScaleTranslate should be exposed to extensions at some point, that's the right thing to do anyway.

I removed the '#include "nsIDOMSVGNumber.h"' from nsSVGSVGElement.h that longsonr pointed out.

I changed nsSVGTranslatePoint to put everything public at the top, and all members at the bottom of their respective class/struct.

Let me know if you're okay with my changes. Apologies again. I know you probably feel less familiar with the code having not touched it for a while which is a pain. In future, please do feel very free to ping me about getting review again if I've not got to it within a week.

Comment 8

[Craig Topper]

I'm ok with your changes. Sorry for not nagging you to review it. ;)

Comment 9

[Jonathan Watt [:jwatt]]

Hah. :-P

Okay, I pushed http://hg.mozilla.org/mozilla-central/rev/c6fe0a8a5929

Thanks a lot Craig, the zoom/pan code is significantly cleaner now!