336803 - Remove nsISVGGradient

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[tor]

Comment 1

[tor]

Attachment: nsISVGGradient removal

Comment 2

[Jonathan Watt [:jwatt]]

Comment on attachment 220977 [details] [diff] [review]
nsISVGGradient removal

Nice improvements. :-) r=jwatt with the following addressed.


Can you replace all occurances of nsLayoutAtoms with nsGkAtoms while you're changing those lines please?

>+  if (frame->GetType() == nsLayoutAtoms::svgLinearGradientFrame ||
>+      frame->GetType() == nsLayoutAtoms::svgRadialGradientFrame ||
>+      frame->GetType() == nsLayoutAtoms::svgGradientFrame) {
>     // Yes, we need to handle this differently
>-    if (mFillGradient == gradient) {
>+    if (mFillGradient == frame) {

Is there any need to check GetType at all? Might as well just do two comparisons against mFillGradient and mStrokeGradient, no? Same for mFillPattern. In fact you could use a nice fast switch to make four pointer comparisons, since we'd no longer be using atoms.


>   // If we reference another gradient and it's going away, null out mNextGrad
>-  nsCOMPtr<nsISVGGradient> gradient;
>-  if (mNextGrad && aModType == nsISVGValue::mod_die &&
>-      (gradient = do_QueryInterface(observable))) {
>-    nsISVGGradient *grad;
>-    CallQueryInterface(mNextGrad, &grad);
>-    if (grad == gradient) {
>+  nsIFrame *gradient = nsnull;
>+  CallQueryInterface(observable, &gradient);
>+  if (mNextGrad && aModType == nsISVGValue::mod_die && gradient) {
>+    if (mNextGrad == gradient) {
>       mNextGrad = nsnull;

The QI is the expensive operation in the comparison, so could you put it after the mNextGrad and aModType check? This also means you eliminate the check for gradient!=null. Something like:

    if (mNextGrad && aModType == nsISVGValue::mod_die) {
      nsIFrame *gradient;
      CallQueryInterface(observable, &gradient);
      if (mNextGrad == gradient) {
         mNextGrad = nsnull;


>-    *aX2 = nsSVGUtils::UserSpace(mSourceContent,
>-                                 &element->mLengthAttributes[nsSVGLinearGradientElement::X2]);
>+    return nsSVGUtils::UserSpace(mSourceContent,
>+                                 &element->mLengthAttributes[aEnumName]);
>     return NS_OK;

Need to remove the above line.


>+float
>+nsSVGRadialGradientFrame::GradientLookupAttribute(nsIAtom *aAtomName,
>+                                                  PRUint16 aEnumName,
>+                                                  nsIContent *aElement)
>+{
>+  nsIContent *gradient;
>+
>+  if (aElement) {
>+    gradient = aElement;
>+  } else {
>+    gradient = GetRadialGradientWithAttr(aAtomName);
>+    if (!gradient)
>+      gradient = mContent;  // use our gradient to get the correct default value

I think I'd prefer to replace aElement with a |PR_Bool *aAttrFound| parameter. IMHO that would make the code easier to read and it's intuitive that a "found" out param would go with a "lookup attr" function, whereas I had to follow the code to see exactly why you would want to pass in an aElement. Besides that I really want to disuade anyone from getting any clever ideas if future and calling GradientLookupAttribute on anything other than |this| (since we could end up getting attributes from the wrong inherited gradient type). So I'd prefer:

  nsIContent *gradient = GetRadialGradientWithAttr(aAtomName);
  if (aAttrFound)
    *aAttrFound = !!gradient;
  if (!gradient)
    gradient = mContent;  // use our gradient to get the correct default value

That should simplify the calling code too. This comment also applies to nsSVG*Linear*GradientFrame of course.


>+  float fX1, fY1, fX2, fY2;
>+  aLgrad->GetParameters(&fX1, &fY1, &fX2, &fY2);

While your touching this, can you take the opportunity to loose the "f" in the variable names? I'd prefer just "x1" etc. and the "f" won't make sense if we ever get to using double internally (as the spec recommends "high quality renderers do"). Okay, maybe it's just because it'll bug me less. ;-)


>+  float fCx, fCy, fR, fFx, fFy;
>+  aRgrad->GetParameters(&fCx, &fCy, &fR, &fFx, &fFy);

Same here.

Comment 3

[tor]

(In reply to comment #2)
> >+float
> >+nsSVGRadialGradientFrame::GradientLookupAttribute(nsIAtom *aAtomName,
> >+                                                  PRUint16 aEnumName,
> >+                                                  nsIContent *aElement)
> >+{
> >+  nsIContent *gradient;
> >+
> >+  if (aElement) {
> >+    gradient = aElement;
> >+  } else {
> >+    gradient = GetRadialGradientWithAttr(aAtomName);
> >+    if (!gradient)
> >+      gradient = mContent;  // use our gradient to get the correct default value
> 
> I think I'd prefer to replace aElement with a |PR_Bool *aAttrFound| parameter.
> IMHO that would make the code easier to read and it's intuitive that a "found"
> out param would go with a "lookup attr" function, whereas I had to follow the
> code to see exactly why you would want to pass in an aElement. Besides that I
> really want to disuade anyone from getting any clever ideas if future and
> calling GradientLookupAttribute on anything other than |this| (since we could
> end up getting attributes from the wrong inherited gradient type). So I'd
> prefer:
> 
>   nsIContent *gradient = GetRadialGradientWithAttr(aAtomName);
>   if (aAttrFound)
>     *aAttrFound = !!gradient;
>   if (!gradient)
>     gradient = mContent;  // use our gradient to get the correct default value

The problem with this suggestion is that we'll end up doing wasted work, since the method will compute the value of fx or fy when they aren't set.  Those return values would then be thrown away in favor of cx or cy.

Comment 4

[tor]

Attachment: apply review comments

Comment 5

[Jonathan Watt [:jwatt]]

Comment on attachment 221014 [details] [diff] [review]
apply review comments

Okay. I can't think of an alternative way that doesn't waste cycles at this stage, so just ignore that comment.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  if (frame == mFillGradient) {
+    if (aModType == nsISVGValue::mod_die)
+      mFillGradient = nsnull;
+    UpdateGraphic(nsSVGGeometryFrame::UPDATEMASK_FILL_PAINT);
+  }
+  else if (frame == mStrokeGradient) {
+    if (aModType == nsISVGValue::mod_die)
+      mStrokeGradient = nsnull;
+    UpdateGraphic(nsSVGGeometryFrame::UPDATEMASK_STROKE_PAINT);
+  }
+  else if (frame == mFillPattern) {
+    if (aModType == nsISVGValue::mod_die)
+      mFillPattern = nsnull;
+    UpdateGraphic(nsSVGGeometryFrame::UPDATEMASK_FILL_PAINT);
+  }
+  else if (frame == mStrokePattern) {
+    if (aModType == nsISVGValue::mod_die)
+      mStrokePattern = nsnull;
+    UpdateGraphic(nsSVGGeometryFrame::UPDATEMASK_STROKE_PAINT);

Should these be 'else's? You could actually want to update both the stroke and fill gradient, right? Also the stroke and fill patter.

+PRUint16
+nsSVGGradientFrame::GetGradientType()
 {
-  if (GetType() == nsGkAtoms::svgLinearGradientFrame) {
-    *aType = nsISVGGradient::SVG_LINEAR_GRADIENT;
-    return NS_OK;
-  }
-  if (GetType() == nsGkAtoms::svgRadialGradientFrame) {
-    *aType = nsISVGGradient::SVG_RADIAL_GRADIENT;
-    return NS_OK;
-  }
-  NS_NOTREACHED("Unknown gradient type!");
-  *aType = nsISVGGradient::SVG_UNKNOWN_GRADIENT;
-  return NS_ERROR_UNEXPECTED;
+  if (GetType() == nsGkAtoms::svgLinearGradientFrame)
+    return SVG_LINEAR_GRADIENT;
+
+  return SVG_RADIAL_GRADIENT;
 }

It would be slightly cleaner to make this one function virtual and override it in the radial/linear subclasses.

Comment 7

[Jonathan Watt [:jwatt]]

> Should these be 'else's? You could actually want to update both the stroke and
> fill gradient, right? Also the stroke and fill patter.

Good point! Actually mFillGradient should be "elsed" with mFillPattern, and mStrokeGradient with mStrokePattern since you can't use both a gradient and a pattern as a paint server.

Comment 8

[tor]

Attachment: more review comment adjustments

Comment 9

[tor]

Checked in.