Status

()

defect
RESOLVED FIXED
13 years ago
11 years ago

People

(Reporter: malex, Assigned: tor)

Tracking

Trunk
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 9 obsolete attachments)

Reporter

Description

13 years ago
Upcoming patch removes nsSVGCoordCtxProvider and merges its functionality into nsSVGSVGElement.
Reporter

Comment 1

13 years ago
Posted patch Initial patch (obsolete) — Splinter Review
Assignee

Comment 2

13 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.
Reporter

Comment 3

13 years ago
Reporter

Comment 4

13 years ago
Posted patch Patch C (obsolete) — Splinter Review
Attachment #239053 - Attachment is obsolete: true
Attachment #240513 - Attachment is obsolete: true
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?
(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.
Reporter

Comment 7

13 years ago
Posted image Testcase 1
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.
Reporter

Comment 8

13 years ago
Posted image Testcase 2
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.
Reporter

Comment 9

13 years ago
Posted patch Patch DSplinter Review
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.
Attachment #247233 - Attachment is obsolete: true
Reporter

Updated

13 years ago
Assignee: malex → general
Assignee

Comment 10

13 years ago
Posted patch update to tip (obsolete) — Splinter Review
Assignee

Comment 11

13 years ago
Posted patch work in progress (obsolete) — Splinter Review
Attachment #250904 - Attachment is obsolete: true
Assignee

Updated

12 years ago
Blocks: 369402
Assignee

Updated

12 years ago
Blocks: 354777
Assignee

Comment 12

12 years ago
Attachment #254844 - Attachment is obsolete: true
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.
Assignee

Comment 14

12 years ago
Attachment #255283 - Attachment is obsolete: true
Attachment #255841 - Flags: review?(jwatt)

Comment 15

12 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
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*).
Assignee

Comment 17

12 years ago
Posted patch adjust per comments (obsolete) — Splinter Review
Assignee: general → tor
Attachment #255841 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #256965 - Flags: review?(jwatt)
Attachment #255841 - Flags: review?(jwatt)
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.
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?
Assignee

Comment 20

12 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?
Assignee

Comment 21

12 years ago
Attachment #256965 - Attachment is obsolete: true
Attachment #257079 - Flags: review?(jwatt)
Attachment #256965 - Flags: review?(jwatt)
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. :-)
Attachment #257079 - Flags: review?(jwatt) → review+
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.
Assignee

Comment 25

12 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.
Assignee

Comment 26

12 years ago
Posted patch apply jwatt's comments (obsolete) — Splinter Review
Attachment #257547 - Flags: superreview?(roc)
-  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?

Please make sure to check in reftest testcases when you check in this one
Flags: in-testsuite?
Assignee

Comment 29

12 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.
Note that you can potentially use the cyclecollector to fix such cycles if there are advantages to that.
Assignee

Comment 31

12 years ago
Attachment #257547 - Attachment is obsolete: true
Attachment #257868 - Flags: superreview?(roc)
Attachment #257547 - Flags: superreview?(roc)
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.
Attachment #257868 - Flags: superreview?(roc)
Attachment #257868 - Flags: superreview+
Attachment #257868 - Flags: review+
Assignee

Comment 34

12 years ago
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Depends on: 373485
Depends on: 373634
Duplicate of this bug: 363484
Depends on: 374271

Updated

12 years ago
No longer depends on: 374271
Depends on: 381447
Depends on: 420697
You need to log in before you can comment on or make changes to this bug.