implement animation of -moz-{border,outline}-radius-* in nsStyleAnimation

RESOLVED FIXED in mozilla1.9.3a1

Status

()

defect
P2
normal
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

(Blocks 1 bug, {dev-doc-complete})

Trunk
mozilla1.9.3a1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Posted patch patch (obsolete) — Splinter Review
This patch implements animation of the border and outline radius properties, i.e., -moz-{border,outline}-radius-{topleft,topright,bottomleft,bottomright}.
Attachment #407377 - Flags: superreview?(bzbarsky)
Attachment #407377 - Flags: review?(dholbert)
Attachment #407377 - Flags: superreview?(bzbarsky) → superreview+
Posted patch patchSplinter Review
This fixes two things:

  * it renames the nsStyleAnimation::Value unit to just be eUnit_CSSValuePair.  (The nsStyleAnimType is still specific to border corners.  This is thus useful in bug 524861

  * it adds two missing |break;| statements in the ComputeDistance function that my test in bug 524861 caught


I don't see the need to request that bz re-sr for that, though.
Attachment #407377 - Attachment is obsolete: true
Attachment #408747 - Flags: review?(dholbert)
Attachment #407377 - Flags: review?(dholbert)
Comment on attachment 408747 [details] [diff] [review]
patch

>+        distance += diff * diff;
>+      }
>+
>+      aDistance = sqrt(distance);
>+      break;

Nit: it's a little confusing having both |aDistance| and |distance|, with one being the square of the other.  Perhaps |distance| should be renamed |squareDistance|? (since that's what it is)


>@@ -1112,20 +1246,34 @@ nsStyleAnimation::Value::operator=(const
>     case eUnit_Dasharray:
>     case eUnit_Shadow:
>+      NS_ABORT_IF_FALSE(mUnit != eUnit_Dasharray || aOther.mValue.mCSSValueList,
>+                        "dasharrays may not be null");
>       mValue.mCSSValueList = aOther.mValue.mCSSValueList
>                                ? aOther.mValue.mCSSValueList->Clone() : nsnull;
>+      if (mUnit == eUnit_Dasharray && !mValue.mCSSValueList) {
>+        mUnit = eUnit_Null;
>+      }
>+      break;
>   }

So this added cleanup doesn't entirely handle one case (though it's admittedly not really dangerous in any way).  Suppose our unit is eUnit_Shadow, and aOther.mValue.mCSSValueList is non-null, **but** the Clone() operation fails.

So the |this| object ends up being a nsStyleAnimation::Value with mUnit == eUnit_Shadow and mValue.mCSSValueList == null.  That's not inherently bad, since shadow can have null pointers, but it's kind of bad because we failed at the assignment and we're continuing with a "half-assigned copy". (unit but not value).

Would it be better to clear mUnit to null for this situation, too, for cleanup purposes?  I'm not sure it would, but it seems like it might.

>+void
> nsStyleAnimation::Value::SetCSSValueListValue(nsCSSValueList *aValueList,
>                                               Unit aUnit,
>                                               PRBool aTakeOwnership)
> {
>   FreeValue();
>-  NS_ASSERTION(IsCSSValueListUnit(aUnit), "bad unit");
>+  NS_ABORT_IF_FALSE(IsCSSValueListUnit(aUnit), "bad unit");
>+  NS_ABORT_IF_FALSE(aUnit != eUnit_Dasharray || aValueList != nsnull,
>+                    "dasharrays may not be null");
>   mUnit = aUnit;
>   if (aTakeOwnership) {
>     mValue.mCSSValueList = aValueList;
>   } else {
>     mValue.mCSSValueList = aValueList ? aValueList->Clone() : nsnull;
>   }
> }

Looks like we could use the fix quoted for my previous comment here, as well -- namely, if the Clone fails and we're a dasharray, we need to clear mUnit.  (And if you agree with my question regarding operator=, above, then we'd need a similar case here for shadow as well.)


r=dholbert with the |distance| variable-rename and the mUnit-clearing in SetCSSValueListValue if Clone fails for dasharray. (and optionally clearing mUnit when Clone fails for Shadow, too, if you think that that's something we should worry about)
Attachment #408747 - Flags: review?(dholbert) → review+
(In reply to comment #2)
> Nit: it's a little confusing having both |aDistance| and |distance|, with one
> being the square of the other.  Perhaps |distance| should be renamed
> |squareDistance|? (since that's what it is)

Done (and in the existing code too).

> Would it be better to clear mUnit to null for this situation, too, for cleanup
> purposes?  I'm not sure it would, but it seems like it might.

I think so.  I've changed the code to:

      if (aOther.mValue.mCSSValueList) {
        mValue.mCSSValueList = aOther.mValue.mCSSValueList->Clone();
        if (!mValue.mCSSValueList) {
          mUnit = eUnit_Null;
        }
      } else {
        mValue.mCSSValueList = nsnull;
      }


> Looks like we could use the fix quoted for my previous comment here, as well --
> namely, if the Clone fails and we're a dasharray, we need to clear mUnit.  (And
> if you agree with my question regarding operator=, above, then we'd need a
> similar case here for shadow as well.)

I'm not going to bother with this; instead, I'm going to just remove the ability to set values with aTakeOwnership as false, since I haven't needed it.  But I'll post that as a separate followup patch.
Attachment #408881 - Flags: review?(dholbert) → review+
http://hg.mozilla.org/mozilla-central/rev/b45765b04da8
http://hg.mozilla.org/mozilla-central/rev/2cb1cc83a6a2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Depends on: 525205
Documented as animatable.
You need to log in before you can comment on or make changes to this bug.