Alex Menzies Reporter
	Description • 18 years ago

Upcoming patch removes nsSVGCoordCtxProvider and merges its functionality into nsSVGSVGElement.

	tor Assignee
	Comment 2 • 18 years ago

It would be nice to get rid of nsSVGCoordCtx at the same time, replacing the new calls you've added to nsSVGSVGElement with a GetLength(PRUint8 mCtxType) and GetMMPerPix(PRUint8 mCtxType) - argument for the latter is for bug compatibility with the current code, which I'm not sure is entirely correct.

	Takeshi Kurosawa :takenspc
	Comment 5 • 18 years ago

I知 taking a look for the em/ex units (bug 305859).
My initially work depends on Patch C from this bug.
So, I wonder another patch or anything for this bug is planed?

	Robert Longson [:longsonr]
	Comment 6 • 18 years ago

(In reply to comment #5)
> I知 taking a look for the em/ex units (bug 305859).
> My initially work depends on Patch C from this bug.
> So, I wonder another patch or anything for this bug is planed?
> 

This one has bitrotted slightly. nsSVGMarkerFrame.cpp has changed under it.

	Alex Menzies Reporter
	Comment 7 • 18 years ago

Patch C breaks on Testcase 1.  Testcase 1 should draw a blue rectangle and a green square of the same height.  The green rectangle should be half the width of the blue rectangle and should be centered on top of it.  Instead, the green rectangles size varies depending on browser size.  I have an upcoming version of the patch that fixes this problem.

	Alex Menzies Reporter
	Comment 8 • 18 years ago

Patch C also does not work on testcase 2.  In this case, the blue box should scale when the width and height of the browser window are changed, but does not.

	Alex Menzies Reporter
	Comment 9 • 18 years ago

Patch D corrects the bitrot caused by the change to nsSVGMarkerFrame.cpp and also properly renders testcase 2.  

Patch D also corrects part of the problem observed with testcase 1 -- the size of the green box no longer changes size with the browsers width.  However, firefox and ASV still render testcase 1 inconsistently in two ways.  First, in firefox the green box is only half the height of the blue rectangle.  In ASV, the two rectangles are of equal height (I believe this was a problem even before this patch).  Second, if the height attribute of the svg element is removed, then the green box's height becomes dependent on the height of the browser window in firefox.  In ASV, the height of the green box is increased but does not depend on browse size.  I believe that part of the problem is that we are incorrectly setting the viewbox, as opposed to the viewport, as the coordinate context provider under some conditions.

Unfortunately, I have some other commitments coming up and am unsure as to when I will be able to find the time to continue contributing to SVG so I am leaving this bug up for reassignment.

	Takeshi Kurosawa :takenspc
	Comment 13 • 18 years ago

Comment on attachment 255283 [details] [diff] [review]
update to tip, remove patch from bug 370210

> class nsISVGLengthList : public nsIDOMSVGLengthList
> {
> public:
>   NS_DECLARE_STATIC_IID_ACCESSOR(NS_ISVGLENGTHLIST_IID)
>-
>-  NS_IMETHOD SetContext(nsSVGCoordCtx* ctx)=0;
>+  NS_IMETHOD SetContext(nsSVGSVGElement* ctx, PRUint8 ctxType)=0;
> };
 


I think the |nsSVGElement| should pass into the |SetContext| to support |em| and |ex|.
Because |em| and |ex| are calculated from current |nsSVGElement| at that time.

	chris hofmann
	Comment 15 • 18 years ago

jwatt, can you review.  be good to get this knocked out and the associated problem in https://bugzilla.mozilla.org/show_bug.cgi?id=354777

	Jonathan Watt [:jwatt]
	Comment 16 • 18 years ago

Comment on attachment 255841 [details] [diff] [review]
remove refcount loop, use taken's api suggestion, further cleanup

This looks good, but I'd like to go through it in more detail. Here are some things I noticed on my initial reading.

>-  NS_IMETHOD SetContext(nsSVGCoordCtx* ctx)=0;
>+  NS_IMETHOD SetContext(nsWeakPtr aContextWeak, PRUint8 

The member should be an nsWeakPtr, but I think the argument can be an nsIWeakReference* to save the nsCOMPtr parameter overhead.


Throughout can you use GetMMPerPx instead of GetMMPerPix?


>+nsSVGSVGElement::SetCoordCtxRect(nsIDOMSVGRect* ctxRect)

aCtxRect here and at the declaration please.


>+    GetAnimValue(NS_STATIC_CAST(nsSVGSVGElement*, nsnull));

...

>+    GetAnimValue(NS_STATIC_CAST(nsSVGSVGElement*, nsnull));

I'd just use a C-style casts here since it's null we're casting. *shrug*


>-  SetCoordCtxRect(r);
>+
>+  nsSVGSVGElement *svgElem = NS_STATIC_CAST(nsSVGSVGElement*, mContent);
>+  NS_ENSURE_TRUE(svgElem, NS_ERROR_FAILURE);
>+  svgElem->SetCoordCtxRect(r);

You can assume that mContent always exists and that it is an nsSVGSVGElement. Just use

  NS_STATIC_CAST(nsSVGSVGElement*, mContent)->SetCoordCtxRect(r);


>+    nsIPresShell* presShell = GetPresContext()->PresShell();
>+    presShell->FrameNeedsReflow(this, nsIPresShell::eStyleChange);

I find

  GetPresContext()->PresShell()->
    FrameNeedsReflow(this, nsIPresShell::eStyleChange);

easier to read. I'm not sure that it's eStyleChange we need, but that plays it safe and we can change that later.


>   case eStyleUnit_Percent: {
>-      nsCOMPtr<nsIDOMSVGElement> element = do_QueryInterface(aContent);
>-      nsCOMPtr<nsIDOMSVGSVGElement> owner;
>-      element->GetOwnerSVGElement(getter_AddRefs(owner));
>-      nsCOMPtr<nsSVGCoordCtxProvider> ctx = do_QueryInterface(owner);
>-    
>+      nsRefPtr<nsSVGSVGElement> ctx =
>+        NS_STATIC_CAST(nsSVGElement*, aContent)->GetCtx();

I'd much prefer that the caller be required to do the cast (pass in an nsSVGElement*).

	Jonathan Watt [:jwatt]
	Comment 18 • 18 years ago

Comment on attachment 256965 [details] [diff] [review]
adjust per comments

>+  NS_IMETHOD SetContext(nsIWeakReference *aContextWeak, PRUint8 ctxType)=0;

Just aContext and aType? Same in other places.


>+  nsWeakPtr mContext;  // needs to be weak to avoid reference loop

nsSVGLength no longer keeps a reference to its context, but rather a reference to the element to which it belongs. Can you change this member name to mElement and add a comment along the lines of:

  // owning element (weak reference to avoid reference loop)
  nsWeakPtr mElement;

Please also s/context/element/ as appropriate in the rest of nsSVGLength.h/.cpp.


> void nsSVGLength::MaybeAddAsObserver()
...
> void nsSVGLength::MaybeRemoveAsObserver()
> {
>   if ((mSpecifiedUnitType==SVG_LENGTHTYPE_PERCENTAGE) &&
>       mContext) {
>-    nsCOMPtr<nsIDOMSVGNumber> num = mContext->GetLength();
>-    NS_REMOVE_SVGVALUE_OBSERVER(num);
>+    nsCOMPtr<nsIContent> element = do_QueryReferent(mContext);
>+    nsRefPtr<nsSVGSVGElement> ctx =
>+      NS_STATIC_CAST(nsSVGElement*, element.get())->GetCtx();
>+    if (ctx) {
>+      nsCOMPtr<nsIDOMSVGRect> rect = ctx->GetCtxRect();
>+      NS_REMOVE_SVGVALUE_OBSERVER(rect);
>+    }
>   }
> }

I don't see any mechanism to detect document tree mutations that would mean that nsSVGLength ends up observing the wrong thing. I don't know how much we care about this given the move to nsSVGLength2 and the limited use of nsSVGLength now, but please file a follow up bug at least.

More comments to come, but it's 3:40 am here so I'm going to crash.

	Jonathan Watt [:jwatt]
	Comment 19 • 18 years ago

Comment on attachment 256965 [details] [diff] [review]
adjust per comments

>       // our immediate parent is an SVG element. get our 'x' and 'y' attribs
>-      x = mLengthAttributes[X].GetAnimValue(mCoordCtx);
>-      y = mLengthAttributes[Y].GetAnimValue(mCoordCtx);
>+      x = mLengthAttributes[X].GetAnimValue(NS_STATIC_CAST(nsSVGElement*,
>+                                                           this));
>+      y = mLengthAttributes[Y].GetAnimValue(NS_STATIC_CAST(nsSVGElement*,
>+                                                           this));
...
>-      x = mLengthAttributes[X].GetAnimValue(mCoordCtx);
>-      y = mLengthAttributes[Y].GetAnimValue(mCoordCtx);
>+      x = mLengthAttributes[X].GetAnimValue(NS_STATIC_CAST(nsSVGElement*,
>+                                                           this));
>+      y = mLengthAttributes[Y].GetAnimValue(NS_STATIC_CAST(nsSVGElement*,
>+                                                           this));

The context used should be the ancestor, not itself, no?

> nsSVGGeometryFrame::GetStrokeWidth()
> {
>+  nsRefPtr<nsSVGElement> ctx =
>+    NS_STATIC_CAST(nsSVGElement*,
>+                   GetType() == nsGkAtoms::svgGlyphFrame ?
>+                   mContent->GetParent() : mContent);
>+

Any reason to use an nsRefPtr instead of a C-style pointer?

	tor Assignee
	Comment 20 • 18 years ago

(In reply to comment #19)
> (From update of attachment 256965 [details] [diff] [review])
> >       // our immediate parent is an SVG element. get our 'x' and 'y' attribs
> >-      x = mLengthAttributes[X].GetAnimValue(mCoordCtx);
> >-      y = mLengthAttributes[Y].GetAnimValue(mCoordCtx);
> >+      x = mLengthAttributes[X].GetAnimValue(NS_STATIC_CAST(nsSVGElement*,
> >+                                                           this));
> >+      y = mLengthAttributes[Y].GetAnimValue(NS_STATIC_CAST(nsSVGElement*,
> >+                                                           this));
> ...
> >-      x = mLengthAttributes[X].GetAnimValue(mCoordCtx);
> >-      y = mLengthAttributes[Y].GetAnimValue(mCoordCtx);
> >+      x = mLengthAttributes[X].GetAnimValue(NS_STATIC_CAST(nsSVGElement*,
> >+                                                           this));
> >+      y = mLengthAttributes[Y].GetAnimValue(NS_STATIC_CAST(nsSVGElement*,
> >+                                                           this));
> 
> The context used should be the ancestor, not itself, no?

The context used will be the ancestor, as I'm passing in a nsSVGElement* rather than a nsSVGSVGElement*.

> > nsSVGGeometryFrame::GetStrokeWidth()
> > {
> >+  nsRefPtr<nsSVGElement> ctx =
> >+    NS_STATIC_CAST(nsSVGElement*,
> >+                   GetType() == nsGkAtoms::svgGlyphFrame ?
> >+                   mContent->GetParent() : mContent);
> >+
> 
> Any reason to use an nsRefPtr instead of a C-style pointer?

The usual paranoia about multithreaded programming and objects going away when you least expect them.  While mContent is safe due to "this" holding a reference to it, might the parent vanish on us?

	Jonathan Watt [:jwatt]
	Comment 22 • 18 years ago

Comment on attachment 257079 [details] [diff] [review]
perform suggested renaming

> The context used will be the ancestor, as I'm passing in a nsSVGElement* rather
> than a nsSVGSVGElement*.

Ah, cleaver. Maybe worth a comment after both instances of 

  // our immediate parent is an SVG element. get our 'x' and 'y' attribs

along the lines of:

  // Cast to nsSVGElement so we get our ancestor coord context


> void nsSVGLength::MaybeAddAsObserver()
> void nsSVGLength::MaybeRemoveAsObserver()

Do we even need to observe the context rect? If our element is being referenced by a <use> then a percentage is relative the the <use>'s viewport. I'm sure there's a reason, but I can't see it right now.


> nsSVGLength2::ConvertToUserUnits(float aVal, nsSVGElement *aSVGElement)

While you're touching this can you please make it:

nsSVGLength2::ConvertToUserUnits(float aVal, nsSVGElement *aSVGElement)
{
  if (mSpecifiedUnitType == nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER ||
      mSpecifiedUnitType == nsIDOMSVGLength::SVG_LENGTHTYPE_PX)
    return aVal;

  return ConvertToUserUnits(aVal, aSVGElement->GetCtx());
}


Looks great. Good stuff. :-)

	Jonathan Watt [:jwatt]
	Comment 23 • 18 years ago

Comment on attachment 257079 [details] [diff] [review]
perform suggested renaming

>+NS_IMETHODIMP
>+nsSVGOuterSVGFrame::AttributeChanged(PRInt32         aNameSpaceID,
>+                                     nsIAtom*        aAttribute,
>+                                     PRInt32         aModType)
>+{
>+  if (aNameSpaceID == kNameSpaceID_None &&
>+      (aAttribute == nsGkAtoms::width || aAttribute == nsGkAtoms::height)) {
>+    AddStateBits(NS_FRAME_IS_DIRTY);
>+    GetPresContext()->PresShell()->
>+      FrameNeedsReflow(this, nsIPresShell::eStyleChange);
>+  }
>+
>+  return NS_OK;
>+}

I found this in one of my trees too but with an additional check !(GetStateBits() & NS_FRAME_FIRST_REFLOW). I think we probably want to check that.

	Jonathan Watt [:jwatt]
	Comment 24 • 18 years ago

Oh, and note this comment:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/svg/base/src/nsSVGOuterSVGFrame.h&rev=1.8&mark=84-86#83

	tor Assignee
	Comment 25 • 18 years ago

(In reply to comment #22)
> > void nsSVGLength::MaybeAddAsObserver()
> > void nsSVGLength::MaybeRemoveAsObserver()
> 
> Do we even need to observe the context rect? If our element is being referenced
> by a <use> then a percentage is relative the the <use>'s viewport. I'm sure
> there's a reason, but I can't see it right now.

Yes, I think we need the observers for now at least.  The only place that uses nsSVGLengths, text elements through length lists, need to re-layout the glyph frames when the viewbox for a % length changes.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 27 • 18 years ago

-  nsCOMPtr<nsSVGCoordCtxProvider> ctx;
+  nsRefPtr<nsSVGSVGElement> ctx;

Why use a strong pointer here?

+  return QI_TO_NSSVGSVGELEMENT(svg);

Can't you just use a static cast?

+  nsRefPtr<nsSVGSVGElement> ctx =
+    NS_STATIC_CAST(nsSVGElement*, element.get())->GetCtx();

Why is this strong? Similarly for all the other calls to GetCtx...

Where are we still using nsSVGLength? Which elements are holding references to nsSVGLengths that create circularity issues?

   if ((mSpecifiedUnitType==SVG_LENGTHTYPE_PERCENTAGE) &&
-      mContext) {
-    nsCOMPtr<nsIDOMSVGNumber> num = mContext->GetLength();
-    NS_ADD_SVGVALUE_OBSERVER(num);
+      mElement) {
+    nsCOMPtr<nsIContent> element = do_QueryReferent(mElement);
+    nsRefPtr<nsSVGSVGElement> ctx =
+      NS_STATIC_CAST(nsSVGElement*, element.get())->GetCtx();
+    if (ctx) {
+      nsCOMPtr<nsIDOMSVGRect> rect = ctx->GetCtxRect();

Move this block of code to a shared helper?

+  nsIDOMSVGRect *rv = vb.get();
+  NS_IF_ADDREF(rv);
+  return rv;

Use vb.swap()

+  nsRefPtr<nsSVGElement> ctx =
+    NS_STATIC_CAST(nsSVGElement*,
+                   GetType() == nsGkAtoms::svgGlyphFrame ?
+                   mContent->GetParent() : mContent);

And why is this strong?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 28 • 18 years ago

Please make sure to check in reftest testcases when you check in this one

	tor Assignee
	Comment 29 • 18 years ago

(In reply to comment #27)
> Where are we still using nsSVGLength? Which elements are holding references to
> nsSVGLengths that create circularity issues?

Text positioning elements use them through AnimatedLengthLists.  Cycle is text positioning element-> length list -> nsSVGLength -> element.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 30 • 18 years ago

Note that you can potentially use the cyclecollector to fix such cycles if there are advantages to that.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 32 • 18 years ago

Comment on attachment 257868 [details] [diff] [review]
fix items pointed out by roc

+  if ((mSpecifiedUnitType==SVG_LENGTHTYPE_PERCENTAGE) && mElement) {
+    nsCOMPtr<nsIDOMSVGRect> rect = GetCtxRect();
+    if (rect)
+      NS_ADD_SVGVALUE_OBSERVER(rect);

More code can be shared here. You could have a method that returns null when mSpecifiedUnitType is not percentage or mElement is null.

	tor Assignee
	Comment 34 • 18 years ago

Checked in.