Support interpolate of clip-path basic shapes in CSS animations and transitions

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: krit, Assigned: jwatt)

Tracking

(Blocks 3 bugs, {dev-doc-complete})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 6 obsolete attachments)

865 bytes, patch
dholbert
: review+
Details | Diff | Splinter Review
3.38 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
1.30 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
4.31 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
5.25 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
5.65 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
4.67 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
37.79 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
(Reporter)

Updated

4 years ago
Blocks: 1072894
(Reporter)

Comment 1

4 years ago
Posted patch clip-path-animation-001.patch (obsolete) — Splinter Review
The initial patch for interpolating basic shapes.
Attachment #8535227 - Flags: review?(dholbert)
Attachment #8535227 - Flags: review?(dbaron)
Comment on attachment 8535227 [details] [diff] [review]
clip-path-animation-001.patch

Haven't done a thorough review yet, but here's what I've got so far:

>+++ b/layout/style/StyleAnimationValue.cpp
>+static inline uint32_t
>+ShapeArguments(nsCSSKeyword aShapeFunction)
>+{

This should have "Num" or "Count" in the name. Maybe "ShapeArgumentCount"?

>+  switch (aShapeFunction) {
>+    case eCSSKeyword_ellipse:
>+      return 3;
>+    case eCSSKeyword_circle:
>+    case eCSSKeyword_polygon:
>+      return 2;
>+    case eCSSKeyword_inset:
>+      return 5;

The order here seems a bit odd. Maybe order as "polygon/circle: ellipse: inset:"?

That seems more logical from the perspective of
 (a) argument-count strictly increasing
 (b) being consistent with the ordering that we're already using in existing code, in nsCSSValue::AppendToString, and in nsStyleBasicShape::GetFunctionName().

(While you're at it, you might want to reorder the cases in AddShapeFunction() [also added in this patch], and maybe other places I'm missing, to also be consistent with this ordering.)
 
>+    default:
>+      NS_ABORT_IF_FALSE(false, "unknown shape function");
>+      return 0;

We no longer need to assert "false", to force debug builds to abort.  Use MOZ_ASSERT_UNREACHABLE("unknown shape function") -- that's now the preferred way to do this. (It avoids needing a bogus "false" arg, which is great.)

I think this applies to 3 different places in this patch, in total.

>+static bool
>+AddCSSLengthPercentageValue(const uint32_t aRestrictions,
>+                            double aCoeff1, const nsCSSValue& aValue1,
>+                            double aCoeff2, const nsCSSValue& aValue2,
>+                            nsCSSValue& aResultArg)
>+{

This function's name is very similar to AddCSSValuePixelPercentCalc -- we need a comment here, to explain the difference & to help someone know which one to use.  (The existing "PixelPercentCalc" version is already pretty explicit about requiring those three types, but this function's exact expectations & behavior are harder to grok from the name.)

>+  nsCSSUnit unit;
>+  if (aValue1.GetUnit() == eCSSUnit_Enumerated ||
>+      aValue2.GetUnit() == eCSSUnit_Enumerated) {
>+    return false;
>+  }
>+  if (aValue1.GetUnit() == aValue2.GetUnit()) {
>+    unit = aValue1.GetUnit();
>+  } else {
>+    // If units differ, we'll just combine them with calc().
>+    unit = eCSSUnit_Calc;
>+  }
>+  return AddCSSValuePixelPercentCalc(aRestrictions,
>+                                     unit,
>+                                     aCoeff1, aValue1,
>+                                     aCoeff2, aValue2,
>+                                     aResultArg);

Correct me if I'm wrong, but I think there's an implicit assumption here that our args *must* be either _Enumerated,  _Percent,  _Calc, or _Pixel-valued, yes?  (In particular, we assume that our args are either _Enumerated, or suitable for passing to a "PixelPositionCalc" function.)  We should probably have an assertion to validate this assumption about the args.

>+static void
>+AddPositions(double aCoeff1, const nsCSSValue& aPos1,
>+             double aCoeff2, const nsCSSValue& aPos2,
>+             nsCSSValue& aResultPos)
>+{
>+  const nsCSSValue::Array* posArray1 = aPos1.GetArrayValue();
>+  const nsCSSValue::Array* posArray2 = aPos2.GetArrayValue();
>+  nsCSSValue::Array* resultPosArray = nsCSSValue::Array::Create(4);
>+  aResultPos.SetArrayValue(resultPosArray, eCSSUnit_Array);
>+
>+  /* Only iterate over elements 1 and 3. The <position> is
>+   * 'uncomputed' to only those elements.
>+   */
>+  for (size_t i = 1; i < 4; i += 2) {
>+    const nsCSSValue& v1 = posArray1->Item(i);
>+    const nsCSSValue& v2 = posArray2->Item(i);

At the beginning of this function, please assert that aPos1 and aPos2 have "array" units here & have 4 entries [since we seem to walk off the end of an array if that assumption doesn't hold up].

>+static bool
>+AddPairList(nsCSSProperty aProperty, uint32_t aRestrictions,
>+            double aCoeff1, const nsCSSValuePairList* aList1,
>+            double aCoeff2, const nsCSSValuePairList* aList2,
>+            nsCSSValuePairList* aResult)
>+{
[...]
>+    aResult->mNext = new nsCSSValuePairList;
>+    aResult = aResult->mNext;
>+  } while (aList1 && aList2);
>+  if (aList1 || aList2) {
>+    // We can't interpolate lists of different lengths.
>+    return false;
>+  }
>+
>+  return true;
>+}

We shouldn't modify |aResult| until we know that we're going to succeed. Let's use a nsAutoPtr (or UniquePtr) here to store the result as we built it up, and then only stick it on |aResult| when we know we're going to succeed. 

>+static bool
>+AddCSSValuePair(nsCSSProperty aProperty, uint32_t aRestrictions,
>+                double aCoeff1, const nsCSSValuePair* aPair1,
>+                double aCoeff2, const nsCSSValuePair* aPair2,
>+                nsCSSValuePair* aResult)
>+{
[...]
>+  for (uint32_t i = 0; i < 2; ++i) {
>+    nsCSSValue nsCSSValuePair::*member = pairValues[i];
>+    if (!AddCSSValuePixelPercentCalc(aRestrictions, unit[i],
>+                                     aCoeff1, aPair1->*member,
>+                                     aCoeff2, aPair2->*member,
>+                                     aResult->*member) ) {
>+      NS_ABORT_IF_FALSE(false, "unexpected unit");
>+      return false;
>+    }
>+  }
>+  return true;

Same here. (This is also one of the NS_ABORT_IF_FALSE(false, ...) --> MOZ_ASSERT_UNREACHABLE(...) spots.)

>@@ -3843,16 +4184,17 @@ StyleAnimationValue::operator==(const St
>     case eUnit_Dasharray:
>+    case eUnit_Shape:
>     case eUnit_Filter:
>     case eUnit_Shadow:
>     case eUnit_BackgroundPosition:
>       return nsCSSValueList::Equal(mValue.mCSSValueList,
>                                    aOther.mValue.mCSSValueList);

The ordering here (in operator==) is inconsistent -- in the enum definition in StyleAnimationValue.h, eUnit_Shape comes after eUnit_Shadow, so let's insert it after _Shadow here as well.
Comment on attachment 8535227 [details] [diff] [review]
clip-path-animation-001.patch

No need for two reviewers here, and it looks like dholbert has it under control.

It's worth looking at whether there are ways to reduce the amount of added code here.
Attachment #8535227 - Flags: review?(dbaron)
Comment on attachment 8535227 [details] [diff] [review]
clip-path-animation-001.patch

>+++ b/layout/style/nsStyleStruct.cpp
> // --------------------
>+// nsBasicShape
>+
>+nsCSSKeyword nsStyleBasicShape::GetFunctionName() const
>+{
>+  switch (mType) {

Two nits here:
 1) The comment has a typo -- it's missing "Style" in the typename.
    s/nsBasicShape/nsStyleBasicShape/

 2) Add a newline after the return type (nsCSSKeyword).  (Not all of the contextual code follows this style, but some of it does, and it's the standard moz style 

>+++ b/layout/style/nsRuleNode.cpp
>@@ -8815,16 +8815,19 @@ nsRuleNode::SetStyleClipPathToCSSValue(n
>                                        nsPresContext* aPresContext,
>                                        bool& aCanStoreInRuleTree)
> {
>   NS_ABORT_IF_FALSE(aValue->GetUnit() != eCSSUnit_ListDep ||
>                     aValue->GetUnit() != eCSSUnit_List,
>                     "expected a basic shape or reference box");
> 
>   const nsCSSValueList* cur = aValue->GetListValue();
>+  if (!cur) {
>+    return;
>+  }
> 
>   uint8_t sizingBox = NS_STYLE_CLIP_SHAPE_SIZING_NOBOX;
>   nsStyleBasicShape* basicShape = nullptr;

This doesn't look animation/interpolation related -- what's this change for?  (i.e. what does it mean to have an empty list here?)  A comment would be nice, to explain what's going on.

(I'm guessing this is something we already needed, which you caught while writing animation tests; is it possible to add a test for this to property_database.js, so we have non-animation coverage of this requirement?)
Comment on attachment 8535227 [details] [diff] [review]
clip-path-animation-001.patch

>+++ b/layout/style/StyleAnimationValue.cpp
>@@ -823,16 +823,20 @@ StyleAnimationValue::ComputeDistance(nsC
>       aDistance = sqrt(squareDistance);
>       return true;
>     }
>+    // CSS Shapes does not define paced animations for shape functions.
>+    case eUnit_Shape: {
>+      return false;
>+    }

Add "spec" to this comment, to make the pluralization make more sense here.

So: s/CSS Shapes does not/CSS Shapes spec does not/
                                     ^^^^
Comment on attachment 8535227 [details] [diff] [review]
clip-path-animation-001.patch

>+++ b/layout/style/StyleAnimationValue.cpp
>@@ -1735,16 +1739,259 @@ AddFilterFunction(double aCoeff1, const 
>+static void
>+AddPositions(double aCoeff1, const nsCSSValue& aPos1,
>+             double aCoeff2, const nsCSSValue& aPos2,
>+             nsCSSValue& aResultPos)
>+{
[...]
>-static void
>-AddPositions(double aCoeff1, const nsCSSValue& aPos1,
>-             double aCoeff2, const nsCSSValue& aPos2,
>-             nsCSSValue& aResultPos)
>-{

It looks like you're moving the AddPositions() helper-function, un-modified, to a different (higher-up) spot in this file -- perhaps so that it can be called by AddShapeFunction()  (which you're adding higher up than the current location of AddPositions).

Let's not intermix a "move-existing-code-untouched" sort of change into a patch that also "adds a bunch of new code" -- it makes hg archeology more confusing, and it muddies what's actually changing.

So, please split out the AddShapeFunction() code-move into its own "part 1" patch, and then layer the main patch on top of that.
Comment on attachment 8535227 [details] [diff] [review]
clip-path-animation-001.patch

>+++ b/layout/style/StyleAnimationValue.cpp
>+static bool
>+AddShapeFunction(nsCSSProperty aProperty,
[...]
>+  switch (shapeFunction) {
>+    case eCSSKeyword_ellipse:
>+      // We fail if the length value is an enumeration.
>+      if (!AddCSSLengthPercentageValue(CSS_PROPERTY_VALUE_NONNEGATIVE,
>+                                       aCoeff1, a1->Item(2),
>+                                       aCoeff2, a2->Item(2),
>+                                       result->Item(2))) {
>+        return false;
>+      }
>+    case eCSSKeyword_circle: {

Add an explicit "// Fall through:" comment before the 'case eCSSKeyword_circle:' line here, to make it clear that that we're falling through (since it's easy to miss) and that this is intentional.

>+      if (!AddCSSLengthPercentageValue(CSS_PROPERTY_VALUE_NONNEGATIVE,
>+                                       aCoeff1, a1->Item(1),
>+                                       aCoeff2, a2->Item(1),
>+                                       result->Item(1))) {
>+        return false;
>+      }

Why do we need this new AddCSSLengthPercentageValue function here? Is it just to prevent us from trying to add Enum-united values?

I think we can (and should) handle this by calling GetCommonUnit(), and then passing the resulting unit to AddCSSValuePixelPercentCalc() (which returns false if the passed-in common unit is Enum, or anything else non-recognized).  This combination of functions seems to be what we use elsewhere for adding lengths-whose-units-we-don't-know.  Seems better to stick with that pattern consistently than to add a new & subtly different codepath for adding lengths. It's a little more verbose, but only slightly.  And if you want to keep it shorter, maybe we could wrap GetCommonUnit()/AddCSSValuePixelPercentCalc() in a helper-method (with a name more like "AddLengthArbitraryUnits", or something like that, whose name & documentation are clearer about what it does & how it differs from existing addition methods).

We'd probably want to use this function elsewhere, too, if we added it, so I'd suggest just using GetCommonUnit()/AddCSSValuePixelPercentCalc() for now, and filing a followup on that helper-wrapper-function if you like. (Or you could do the followup first, if you prefer)

>+    case eCSSKeyword_polygon: {
>+      if (a1->Item(1).GetIntValue() != a2->Item(1).GetIntValue()) {
>+        return false;
>+      }
>+      result->Item(1).SetIntValue(a1->Item(1).GetIntValue(),
>+                                  eCSSUnit_Enumerated);

I don't see any clear mention in this file, saying what Item(1) represents for _polygon values.  I'm guessing it's the fill rule? Can you add a comment here mentioning it, and maybe use a variable as well? e.g.:

  int32_t fillRule = a1->Item(1).GetIntValue();
  if (fillRule != a2->Item(1).GetIntValue()) {
    return false; // Can't add values w/ different fill rules.
  }
  result->Item(1).SetIntValue(fillRule, eCSSUnit_Enumerated);

>+      result->Item(1).SetIntValue(a1->Item(1).GetIntValue(),
>+                                  eCSSUnit_Enumerated);
>+      const nsCSSValuePairList* list1 = a1->Item(2).GetPairListValue();
>+      const nsCSSValuePairList* list2 = a2->Item(2).GetPairListValue();
>+      if (!AddPairList(aProperty, 0,
>+                       aCoeff1, list1, aCoeff2, list2,
>+                       result->Item(2).SetPairListValue())) {
>+        return false;
>+      }

I'd rather we just pass "result->Item(2)" here (as a nsCSSValue&), and then assign it to a local nsCSSValue inside of AddPairList (taking its value from a local var), when we know we're going to succeed. Or something like that.

This change (passing the nsCSSValue) will make it easier to avoid modifying the outparam until we know we're going to succeed (per note in the middle of comment 2).

>+    case eCSSKeyword_inset: {
>+      for (size_t i = 1; i <= 4; ++i) {
>+        if (!AddCSSLengthPercentageValue(CSS_PROPERTY_VALUE_NONNEGATIVE,
>+                                         aCoeff1, a1->Item(i),
>+                                         aCoeff2, a2->Item(i),
>+                                         result->Item(i))) {
>+          return false;
[...]
>+      nsCSSValue::Array* arr = nsCSSValue::Array::Create(4);
>+      result->Item(5).SetArrayValue(arr, eCSSUnit_Array);
>+      for (size_t i = 0; i < 4; ++i) {


Two things:
 (1) Add a short comment here & elsewhere in this _Inset clause, to document what the Item()s that you're adding are actually representing. (We do Item(1)-Item(4) here, and then Item(5)'s array value below. Not clear what any of these mean. After looking at the spec, I'd guess that items 1-4 are offsets from edges, and item5 is an array of border-radiuses.  That should all be clearer in this code, via comments and/or variable-naming.)

 (2) Per above, I don't think we should be using this new "AddCSSLengthPercentageValue" function here -- we should be using GetCommonUnit / AddCSSValuePixelPercentCalc, or a wrapper for those two.

>+        const nsCSSValuePair& pair1 = pos1->Item(i).GetPairValue();
>+        const nsCSSValuePair& pair2 = pos2->Item(i).GetPairValue();
>+        nsAutoPtr<nsCSSValuePair> pairResult(new nsCSSValuePair);
>+        if (!AddCSSValuePair(aProperty, 0,
>+                             aCoeff1, &pair1,
>+                             aCoeff2, &pair2,
>+                             pairResult)) {

The "0" here is for "restrictions". (so, no restrictions) Do we really not have any restrictions on these pair values?

If these are <border-radius> values (which I think (?) they are, per above), I'd imagine they should be restricted in the same way the "border-radius" property is.  And we have CSS_PROPERTY_VALUE_NONNEGATIVE in nsCSSPropList.h for the various border-radius sub-properties. So I'd think we should use that restriction here as well.  (Perhaps via a call to nsCSSProps::ValueRestrictions(eCSSProperty_border_top_left_radius), with a comment indicating that this is a <'border-radius'> value, so we're just using one of the border-radius properties to get the correct restrictions for this type.)

>@@ -2081,44 +2306,26 @@ StyleAnimationValue::AddWeighted(nsCSSPr
>     case eUnit_CSSValuePair: {
>-      const nsCSSValuePair *pair1 = aValue1.GetCSSValuePairValue();
>-      const nsCSSValuePair *pair2 = aValue2.GetCSSValuePairValue();
>-      nsCSSUnit unit[2];
>-      unit[0] = GetCommonUnit(aProperty, pair1->mXValue.GetUnit(),
>-                              pair2->mXValue.GetUnit());
>-      unit[1] = GetCommonUnit(aProperty, pair1->mYValue.GetUnit(),
>-                              pair2->mYValue.GetUnit());
>-      if (unit[0] == eCSSUnit_Null || unit[1] == eCSSUnit_Null ||
>-          unit[0] == eCSSUnit_URL || unit[0] == eCSSUnit_Enumerated) {
>+
>+      nsAutoPtr<nsCSSValuePair> result(new nsCSSValuePair);
>+      uint32_t restrictions = nsCSSProps::ValueRestrictions(aProperty);
>+      if (!AddCSSValuePair(aProperty, restrictions,
>+                           aCoeff1, aValue1.GetCSSValuePairValue(),
>+                           aCoeff2, aValue2.GetCSSValuePairValue(),
>+                           result)) {
[...etc...]

It looks like we're factoring out existing logic here into a new "AddCSSValuePair" function, right?

If so, that refactoring should ideally be in its own non-functional helper-patch.

>+    case eUnit_Shape: {
>+      const nsCSSValueList *list1 = aValue1.GetCSSValueListValue();
>+      const nsCSSValueList *list2 = aValue2.GetCSSValueListValue();
>+
>+      if (!list1 || !list2 ||
>+          list1->mValue.GetUnit() != eCSSUnit_Function ||
>+          list2->mValue.GetUnit() != eCSSUnit_Function) {
>+        // If we don't have shape-functions, we can't interpolate.
>+        return false;
>+      }

I'm unclear on what it means to:
 - have eUnit_Shape, but have a null list value.
 - have eUnit_Shape and a non-null list, but to have a non-function unit inside that list

We check for both of those things here, so I assume they're both conceivably possible. Can you tweak/extend your comment here ("If we don't have shape-functions...") to indicate what these scenarios actually mean?

>@@ -3282,16 +3527,111 @@ StyleAnimationValue::ExtractComputedValu
>+        case eCSSProperty_clip_path: {
>+          const nsStyleSVGReset *svgReset =
>+            static_cast<const nsStyleSVGReset*>(styleStruct);
>+          const nsStyleClipPath& clipPath = svgReset->mClipPath;

Nit: pointer-type asterisks should be type-hugging, per https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Declarations

(So "nsStyleSVGReset *svgReset" should be "nsStyleSVGReset* svgReset, and "nsCSSValueList **resultTail" should be "nsCSSValueList** resultTail")

This is pretty minor, & the existing code here is inconsistent on this, so I won't hold r+ on this particular point; just mentioning it because I noticed it in various places in the patch.  (If you can clean it up in the new code here, great; if not, I won't lose sleep over it. :))

>+          nsAutoPtr<nsCSSValueList> result;
>+          nsCSSValueList **resultTail = getter_Transfers(result);
>+          const int32_t type = clipPath.GetType();
>+          if (type != NS_STYLE_CLIP_PATH_NONE &&
>+              type != NS_STYLE_CLIP_PATH_URL) {
>+            nsCSSValueList *item = new nsCSSValueList;
>+            *resultTail = item;
>+            resultTail = &item->mNext;
>+            if (type == NS_STYLE_CLIP_PATH_SHAPE) {
[...code-that-sets-up-'item'-depending-on-the-type-of-shape...]
>+              item = new nsCSSValueList;
>+              *resultTail = item;
>+              resultTail = &item->mNext;
>+            }
>+            item->mValue.SetIntValue(clipPath.GetSizingBox(), eCSSUnit_Enumerated);
>+          }
>+          aComputedValue.SetAndAdoptCSSValueListValue(result.forget(),
>+                                                      eUnit_Shape);

A few issues here:
 (1) If we fail the he "== NS_STYLE_CLIP_PATH_SHAPE" check, we still go ahead and call aComputedValue's setter, with eUnit_Shape. Why?

 (2) Why are we using a nsCSSValueList here? (as the backing for eUnit_Shape)? It looks to me like this list is guaranteed to have two values (except in the mysterious case called out in (1) above -- so maybe a list makes more sense if I understood that better).  The first list-value always has information about the shape (maybe with its own separate list or whatever), and the second value always has information about the sizing-box.  Given this, I'd imagine a 2-entry nsCSSValue::Array would  make more sense.

 (3) Even if we keep this a nsCSSValueList, I wonder if we can simplify away the result/resultTail/item boilerplate code here. I think that pattern makes sense when we're setting up a long list of values (particularly when we have a helper-method that may or may not append to a list), but when we're only populating two values and all the setting is done locally, the result/resultTail/item stuff seems unnecessarily abstract.

 (4) If type is NS_STYLE_CLIP_PATH_NONE or NS_STYLE_CLIP_PATH_URL, it seems like we end up setting aComputedValue to null, with unit eUnit_Shape. That doesn't make sense to me -- I'd think we should only use eUnit_Shape if we have a shape.

>+              switch (shape->GetShapeType()) {
>+                case nsStyleBasicShape::Type::eCircle:
>+                case nsStyleBasicShape::Type::eEllipse: {
>+                  const nsTArray<nsStyleCoord>& coords = shape->Coordinates();
>+                  for (unsigned i = 0; i < coords.Length(); ++i) {

s/unsigned/size_t/

>+                    if (coords[i].GetUnit() == eStyleUnit_Enumerated) {
>+                      functionArray->Item(i + 1).SetIntValue(
>+                                                   coords[i].GetIntValue(),
>+                                                   eCSSUnit_Enumerated);
>+                      continue;
>+                    }
>+                    if (!StyleCoordToCSSValue(coords[i],
>+                                              functionArray->Item(i + 1))) {
>+                      return false;
>+                    }
>+                  }

I think the "continue; } if" can & should be replaced with just "} else if {" here.

Also: We should assert that we're not walking off the end of the Item() list here -- that functionArray->Length() is what we expect it to be, with respect to coords->Length().

Also: It seems like it might be better to initialize functionArray *inside of* the "case nsStyleBasicShape::Type::[whatever]" code-blocks, based on the number of coords (as-appropriate), instead of using ShapeArguments(). (and in the polygon case, we'd set it to a hardcoded 2-entry array, and use a list in one of those entries to store the (arbitrarily-long-amount-of) coords, like you already do.)

>+                case nsStyleBasicShape::Type::ePolygon: {
>+                  functionArray->Item(1).SetIntValue(shape->GetFillRule(),
>+                                                     eCSSUnit_Enumerated);
>+                  nsCSSValuePairList* list =
>+                    functionArray->Item(2).SetPairListValue();
>+                  const nsTArray<nsStyleCoord>& coords = shape->Coordinates();
>+                  for (size_t i = 0; i < coords.Length(); i += 2) {
[...]
>+                    if (!StyleCoordToCSSValue(coords[i], list->mXValue) ||
>+                        !StyleCoordToCSSValue(coords[i + 1], list->mYValue)) {
>+                      return false;
>+                    }

Seems like we should assert that coords.Length() is an even number, somewhere around here, since we seem to assume that. (Any standalone straggler at the end will just get skipped over in this list, and seems like it'd probably be a sign of a bug in our input data, right?)

>+                case nsStyleBasicShape::Type::eInset: {
>+                  const nsTArray<nsStyleCoord>& coords = shape->Coordinates();
>+                  NS_ABORT_IF_FALSE(coords.Length() == 4, "unexpected offset");
>+                  for (size_t i = 0; i < 4; ++i) {
>+                    if (!StyleCoordToCSSValue(coords[i],
>+                                              functionArray->Item(i + 1))) {
>+                      return false;
>+                    }
>+                  }
[...]
>+                  functionArray->Item(5).SetArrayValue(radiusArray, eCSSUnit_Array);

As noted for circle/ellipse above, I'd prefer that we initialize functionArray here, inside this case statement, with its length *based on* the size of |coords|.  Then we can assert about coords.Length(), but our logic (and e.g. this loop up to hardcoded "4") won't have to be quite as dependent on that assertion holding up.  Then we can just iterate with "i < coords.Length()" and not worry as much about walking off the end of our Item() array, or coords, here.

>@@ -3605,22 +3945,23 @@ StyleAnimationValue::operator=(const Sty
>+    case eUnit_Shape:
>     case eUnit_Filter:
>     case eUnit_Dasharray:
>     case eUnit_Shadow:
>     case eUnit_BackgroundPosition:
>       NS_ABORT_IF_FALSE(mUnit == eUnit_Shadow || mUnit == eUnit_Filter ||
>-                        aOther.mValue.mCSSValueList,
>+                        mUnit == eUnit_Shape || aOther.mValue.mCSSValueList,
>                         "value lists other than shadows and filters may not be null");

If you're updating the list of exceptions in the assertion here, you need to update the list of exceptions in the assertion-message as well (to mention shapes along with shadow & filter).

(Though as noted above, I think (?) this can only happen if we have NS_STYLE_CLIP_PATH_NONE or URL, and then it doesn't seem like eUnit_Shape is the right unit. So I'd think this exception should go away, and eUnit_Shape should just be able to assume it doesn't have a null value.)

> StyleAnimationValue::SetAndAdoptCSSValueListValue(nsCSSValueList *aValueList,
>                                                   Unit aUnit)
> {
>   FreeValue();
>   NS_ABORT_IF_FALSE(IsCSSValueListUnit(aUnit), "bad unit");
>   NS_ABORT_IF_FALSE(aUnit == eUnit_Shadow || aUnit == eUnit_Filter ||
>-                    aValueList != nullptr,
>+                    aUnit == eUnit_Shape || aValueList != nullptr,
>                     "value lists other than shadows and filters may not be null");

Same here -- if you update this assertion, you need to update the text as well to reflect the change.

>+++ b/layout/style/nsComputedDOMStyle.cpp
>@@ -5293,17 +5270,17 @@ nsComputedDOMStyle::CreatePrimitiveValue
>           shapeFunctionString.Append(coordString);
>         }
>         break;
>       }
>       case nsStyleBasicShape::Type::eCircle:
>       case nsStyleBasicShape::Type::eEllipse: {
>         const nsTArray<nsStyleCoord>& radii = aStyleBasicShape->Coordinates();
>         NS_ABORT_IF_FALSE(radii.Length() ==
>-                          (nsStyleBasicShape::Type::eCircle ? 1 : 2),
>+                          (nsStyleBasicShape::Type::eCircle == type ? 1 : 2),
>                           "wrong number of radii");

This -- the only change in this file -- seems like an important bugfix, and seems largely unrelated to the main changes here, aside from the fact that it's exposed by testing that gets enabled by the code here.

This probably deserves its own patch, separate from the main "implement clip-path animation" patch here.  (rs=me on that patch; no need to request review.)  (maybe even deserves its own bug, but it can be a helper-patch here if you like)

>+++ b/layout/style/test/test_transitions_per_property.html
>+  // mimatch boxes
>+  { start: "circle(100px at 100px 100px) border-box",

Typo -- s/mimatch/mismatch/ (or mismatching)

>+  // shape to refernce box
>+  { start: "circle(20px)", end: "content-box", expected: ["content-box"] },

Typo: s/refernce/reference/

>+    is(matches.pop(), expectedList.pop(), "Check if reference boxes match");

Mochitest messages should describe the expectation -- so this should say, "Reference boxes should match", instead of "Check if [etc.]"

>+function test_clip_path_transition(prop) {
[...]
>+    ok(test_clip_path_equals(actual, test.expected),
>+                                   "Clip-path property is " + actual + " expected values of " +
>+                                   test.expected);
         ^---------------------------'

The message there ("Clip-path property...") is indented way too much. It should slide all the way to the left, to be just inside ok's open-paren (position shown by my ASCII art).
Version: unspecified → Trunk
Comment on attachment 8535227 [details] [diff] [review]
clip-path-animation-001.patch

[Marking review- to take this out of my pending-review-queue.  This is looking good & on the right track, though!]
Attachment #8535227 - Flags: review?(dholbert) → review-

Comment 9

3 years ago
(This comment is just DevAdvocacy)

Is there any news on this bug's status?
(Assignee)

Updated

3 years ago
See Also: → 1075457
(Assignee)

Updated

3 years ago
Summary: Interpolate basic shapes for clip-path → Support interpolate of clip-path basic shapes in CSS animations and transitions
(Assignee)

Comment 10

3 years ago
From bug 1075457:

(In reply to Jonathan Watt [:jwatt] from bug 1075457 comment #49)
> It seems like this should be implemented by
> extending the capabilities of DisplayListClipState and DisplayItemClip.
> Doing it this way would also provide better layers integration of the
> clipping which should give us better performance.

This is more important in the case of animation/transitions.
(Assignee)

Updated

3 years ago
(Assignee)

Comment 11

3 years ago
An example that requires this bug that's being talked about on Twitter just now is http://codepen.io/thebabydino/pen/RWvaZW
(Assignee)

Updated

3 years ago
Assignee: nobody → jwatt
(Assignee)

Comment 13

3 years ago
Attachment #8535227 - Attachment is obsolete: true
Attachment #8720110 - Flags: review?(dholbert)
(Assignee)

Comment 15

3 years ago
(The rest will come soon but I've not quite finished working through all the review comments yet.)
(Assignee)

Comment 19

3 years ago
Attachment #8720281 - Flags: review?(dholbert)
(Assignee)

Comment 20

3 years ago
(In reply to Daniel Holbert [:dholbert] from comment #2)
> Comment on attachment 8535227 [details] [diff] [review]
> clip-path-animation-001.patch
>
> Correct me if I'm wrong, but I think there's an implicit assumption here
> that our args *must* be either _Enumerated,  _Percent,  _Calc, or
> _Pixel-valued, yes?  (In particular, we assume that our args are either
> _Enumerated, or suitable for passing to a "PixelPositionCalc" function.)  We
> should probably have an assertion to validate this assumption about the args.

Isn't this code subject to being passed user defined types based on how the user writes the animation? It returns false rather than asserting makes sense to me.

(In reply to Daniel Holbert [:dholbert] from comment #7)
> Why do we need this new AddCSSLengthPercentageValue function here? Is it
> just to prevent us from trying to add Enum-united values?

It looks like the only reason is because AddCSSValuePixelPercentCalc requires the common unit to be passed explicitly but the callers of AddCSSLengthPercentageValue may have values with different units. The enum testing code seems like it's just an optimization since AddCSSValuePixelPercentCalc will return false for that anyway.

> I think we can (and should) handle this by calling GetCommonUnit(), and then
> passing the resulting unit to AddCSSValuePixelPercentCalc() (which returns
> false if the passed-in common unit is Enum, or anything else
> non-recognized).  This combination of functions seems to be what we use
> elsewhere for adding lengths-whose-units-we-don't-know.  Seems better to
> stick with that pattern consistently than to add a new & subtly different
> codepath for adding lengths.

Agreed, and done.

> If these are <border-radius> values (which I think (?) they are, per above),
> I'd imagine they should be restricted in the same way the "border-radius"
> property is.  And we have CSS_PROPERTY_VALUE_NONNEGATIVE in nsCSSPropList.h
> for the various border-radius sub-properties. So I'd think we should use
> that restriction here as well.  (Perhaps via a call to
> nsCSSProps::ValueRestrictions(eCSSProperty_border_top_left_radius), with a
> comment indicating that this is a <'border-radius'> value, so we're just
> using one of the border-radius properties to get the correct restrictions
> for this type.)

I think this is reasonable and what I've done.

> I'm unclear on what it means to:
>  - have eUnit_Shape, but have a null list value.
>  - have eUnit_Shape and a non-null list, but to have a non-function unit
> inside that list
> 
> We check for both of those things here, so I assume they're both conceivably
> possible. Can you tweak/extend your comment here ("If we don't have
> shape-functions...") to indicate what these scenarios actually mean?

I'm not sure about this either so I didn't modify the comment.
Attachment #8720110 - Flags: review?(dholbert) → review+
Comment on attachment 8720111 [details] [diff] [review]
part 2 - Move AddPositions() further up StyleAnimationValue.cpp and add asserts

Review of attachment 8720111 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, one nit:

::: layout/style/StyleAnimationValue.cpp
@@ +1756,5 @@
> +             double aCoeff2, const nsCSSValue& aPos2,
> +             nsCSSValue& aResultPos)
> +{
> +  MOZ_ASSERT(aPos1.GetUnit() == eCSSUnit_Array &&
> +             aPos2.GetUnit() == eCSSUnit_Array);

This assertion (simply stating an assumption without an explanation) doesn't add much value, in its current state; the GetArrayValue() call directly below it will already make the same assertion (pretty much), internally.

If you add an explanatory message, though, then this assertion would add value -- e.g.
 "Args should be CSS <position>s, encoded as arrays".
...then it'd add value.

(The check would still be redundant,  but it'd include a justification & it'd print something more useful/actionable than the GetArrayValue assertion if & when we trip this assertion.)

So: please consider adding a message like that. (message-less assertions should be extremely rare IMO, and mostly for things like null checks)
Attachment #8720111 - Flags: review?(dholbert) → review+
Comment on attachment 8720278 [details] [diff] [review]
part 3 - Factor out a AddCSSValuePair helper in StyleAnimationValue.cpp

Review of attachment 8720278 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/StyleAnimationValue.cpp
@@ +1779,5 @@
>    }
>  }
>  
> +static bool
> +AddCSSValuePair(nsCSSProperty aProperty, uint32_t aRestrictions,

Could you structure this function in such a way that it returns the result directly? (following the example of AddDifferentTransformLists).

Right now it's returning a success/failure indication (a bool) separately from a generated resource (outparma aResult).  Really, the non-nullness of the generated outparam should *be * our success/failure code.

(That would save us a copy operation, too -- right now this has to copy the local-var |result| into the outparam, but it wouldn't have to if this function were responsible for creating & returning the nsCSSValuePair.)

(It's a bit potentially-leaky that we'd be returning a raw nsCSSValuePair*, like how AddDifferentTransformLists returns a raw pointer.  Ideally we should be returning a UniquePtr<nsCSSValuePair>, which the caller should be assigning to its own UniquePtr local-var. But this file doesn't use UniquePtr yet -- so, bonus points if you want to use it here, but cribbing from AddDifferentTransformLists's style is fine too.)
Comment on attachment 8720279 [details] [diff] [review]
part 4 - Fix MOZ_ASSERT bug in nsComputedDOMStyle.cpp

Review of attachment 8720279 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on part 4
Attachment #8720279 - Flags: review?(dholbert) → review+
Comment on attachment 8720280 [details] [diff] [review]
part 5 - Factor out a nsStyleBasicShape::GetFunctionName method

Review of attachment 8720280 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on part 5, with the following addressed as you see fit. (Make sure to update the commit message, if you rename the function, too.)

::: layout/style/nsComputedDOMStyle.cpp
@@ +5817,5 @@
>      // Shape function name and opening parenthesis.
>      nsAutoString shapeFunctionString;
> +    AppendASCIItoUTF16(nsCSSKeywords::GetStringValue(
> +                         aStyleBasicShape->GetFunctionName()),
> +                       shapeFunctionString);

Consider tweaking this to use a local variable, so this doesn't end up 3-functions-deep in terms of wrapping, with awkward indentation.

e.g.
    nsCSSKeyword shapeName = aStyleBasicShape->GetFunctionName();
    AppendASCIItoUTF16((nsCSSKeywords::GetStringValue(shapeName),
                       shapeFunctionString);

::: layout/style/nsStyleStruct.h
@@ +3268,5 @@
>      mPosition.SetInitialPercentValues(0.5f);
>    }
>  
>    Type GetShapeType() const { return mType; }
> +  nsCSSKeyword GetFunctionName() const;

IMO we should call this new method "GetShapeTypeKeyword()", or "GetShapeTypeAsKeyword()", or perhaps " "GetShapeTypeName()".

Justification: this new method is extremely similar to GetShapeType() -- both methods return an enum that represents our shape-type. So, it seems odd for them to have completely-different names -- any naming-difference should just reflect the difference between the types of enums that they return.
Attachment #8720280 - Flags: review?(dholbert) → review+
Comment on attachment 8720278 [details] [diff] [review]
part 3 - Factor out a AddCSSValuePair helper in StyleAnimationValue.cpp

Review of attachment 8720278 [details] [diff] [review]:
-----------------------------------------------------------------

[marking part 3 "r-" for now, per comment 22. If there's some reason we really need to keep its current design -- with a separate outparam vs. return-code -- then I'd reconsider, but for now I'd much prefer that we collapse the return-code and the outparam together, for better foolproofing & to avoid an unnecessary copy operation]
Attachment #8720278 - Flags: review?(dholbert) → review-
(Assignee)

Comment 26

3 years ago
Attachment #8720278 - Attachment is obsolete: true
Attachment #8722289 - Flags: review?(dholbert)
Comment on attachment 8722289 [details] [diff] [review]
part 3 - Factor out a AddCSSValuePair helper in StyleAnimationValue.cpp

Review of attachment 8722289 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! Just one minor issue:

::: layout/style/StyleAnimationValue.cpp
@@ +1811,5 @@
> +                                     aCoeff1, aPair1->*member,
> +                                     aCoeff2, aPair2->*member,
> +                                     result.get()->*member) ) {
> +      MOZ_ASSERT(false, "unexpected unit");
> +      return result; // nullptr (returning |result| for RVO)

result is not actually nullptr here. You need to add "result.reset()" before this line.
Attachment #8722289 - Flags: review?(dholbert) → review+
(Assignee)

Updated

3 years ago
Keywords: leave-open
(Assignee)

Comment 29

3 years ago
(In reply to Daniel Holbert [:dholbert] from comment #27)
> result is not actually nullptr here. You need to add "result.reset()" before
> this line.

Thanks for catching that!
Comment on attachment 8720281 [details] [diff] [review]
part 6 - Support CSS animation of clip-path basic shapes

[Canceling review on part 6, since it's going to be changing a bit, per IRC discussion yesterday.]
Attachment #8720281 - Flags: review?(dholbert)

Comment 32

3 years ago
Don't support this Demo[1] yet? 

[1]http://codepen.io/thebabydino/pen/RWvaZW
Indeed, this bug isn't fixed yet - jwatt landed 5 patches here, but the final patch isn't quite done (and hasn't landed), as noted in comment 30.

Comment 34

3 years ago
(In reply to Daniel Holbert [:dholbert] from comment #33)
> Indeed, this bug isn't fixed yet - jwatt landed 5 patches here, but the
> final patch isn't quite done (and hasn't landed), as noted in comment 30.

Thank you for your reply, very much looking forward to this function.
(Assignee)

Comment 35

3 years ago
(In reply to Daniel Holbert [:dholbert] from comment #7)
> A few issues here:
>  (1) If we fail the he "== NS_STYLE_CLIP_PATH_SHAPE" check, we still go
> ahead and call aComputedValue's setter, with eUnit_Shape. Why?
> 
> [...]
> 
>  (4) If type is NS_STYLE_CLIP_PATH_NONE or NS_STYLE_CLIP_PATH_URL, it seems
> like we end up setting aComputedValue to null, with unit eUnit_Shape. That
> doesn't make sense to me -- I'd think we should only use eUnit_Shape if we
> have a shape.

Yeah, this bit of code is quite broken and causes failure of the existing texts in test_smilCSSFromTo.xhtml that animate a clip-path from one URL to another.

With this patch we make clip-path eStyleAnimType_Custom and, when failing tests in test_smilCSSFromTo.xhtml, we hit this code under a stack like:

  mozilla::StyleAnimationValue::ExtractComputedValue
  ComputeValuesFromStyleRule
  mozilla::StyleAnimationValue::ComputeValue
  ValueFromStringHelper
  nsSMILCSSValueType::ValueFromString
  nsSMILCSSProperty::ValueFromString
  nsSMILAnimationFunction::ParseAttr
  nsSMILAnimationFunction::GetValues
  nsSMILAnimationFunction::ComposeResult
  nsSMILCompositor::ComposeAttribute
  nsSMILAnimationController::DoSample
  nsSMILAnimationController::Resample
  nsSMILAnimationController::FlushResampleRequests
  nsSVGElement::FlushAnimations

Prior to the patch in StyleAnimationValue::ComputeValue we would set:

  aComputedValue.SetUnparsedStringValue(nsString(aSpecifiedValue))

and return early in due to the eStyleAnimType_None check. We don't pass aSpecifiedValue through to StyleAnimationValue::ExtractComputedValue though. Instead we parse aSpecifiedValue and pass through a StyleRule.

To fix this problem we could enhance the eStyleAnimType_None check in StyleAnimationValue::ExtractComputedValue to check if the clip-path type is one that does not animate. Perhaps that could be wrapped up in a PropertyValueCanAnimate helper. It seems a bit weird that clip-path would (currently) be the only "special" property that requires this check though. And this ignores the fact that there are a bunch of other ways that we can enter ExtractComputedValue.

Alternatively we could pass through aSpecifiedValue to ExtractComputedValue and do the same SetUnparsedStringValue thing that the current code ends up doing.

Or perhaps we should make ExtractComputedValue handle StyleRule objects for clip-path URLs/none/geometry-box values. That would involve a bunch of serialization work for the various types if we want to continue to call SetUnparsedStringValue though. Maybe we can do something similar to what we do for eCSSProperty_filter and call SetURLValue etc. instead? That's not clear to me.

Any thoughts on any of the above, Daniel?
Flags: needinfo?(dholbert)
(In reply to Jonathan Watt [:jwatt] from comment #35)
> Prior to the patch in StyleAnimationValue::ComputeValue we would set:
> 
>   aComputedValue.SetUnparsedStringValue(nsString(aSpecifiedValue))
> 
> and return early in due to the eStyleAnimType_None check.

Right -- this is what happens for non-interpolatable properties.  We just capture the property-value's string representation -- no point trying to process it into some numeric form for interpolation, if interpolation isn't possible at all for the property. Otherwise, we proceed & capture the computed value of the property (and this computed value may or may not be interpolatable, either at all or with certain other computed values).

> We don't pass
> aSpecifiedValue through to StyleAnimationValue::ExtractComputedValue though.
> Instead we parse aSpecifiedValue and pass through a StyleRule.

(Now you're talking about after the patch, I assume.)

We do this (using a style rule instead of just directly passing in the string) to handle cases where aSpecifiedValue is "inherit" or "initial" or some other keyword that isn't directly interpolatable, but which *computes* to something interpolatable.  So we generate a style rule and pull the computed value out of it, via ComputeValuesFromStyleRule & ExtractComputedValue.

Also, when there's a transition, I believe *all we have* is the style rule -- we don't have the specified begin/end string values.

So, this all makes sense so far.
 
> To fix this problem we could enhance the eStyleAnimType_None check in
> StyleAnimationValue::ExtractComputedValue to check if the clip-path type is
> one that does not animate.

RE "To fix this problem" -- wait, you didn't mention a problem yet (or I missed it).  I'm not clear what the problem is that you're trying to fix.  From later in your comment -- it sounds like the problem is that there are some classes of values that are maybe cumbersome to work with, which we *know* we can't interpolate, so you'd prefer to avoid adding code to ExtractComputedValue to handle them -- is that correct?

> we could enhance the eStyleAnimType_None check in StyleAnimationValue::ExtractComputedValue
> to check if the clip-path type is one that does not animate.
> It seems a bit weird that clip-path would
> (currently) be the only "special" property that requires this check though.
> And this ignores the fact that there are a bunch of other ways that we can
> enter ExtractComputedValue.

Agreed, I'd rather not have a special-case for clip-path at that level of granularity.

> Alternatively we could pass through aSpecifiedValue to ExtractComputedValue
> and do the same SetUnparsedStringValue thing that the current code ends up
> doing.

That might work, though I suspect you're right that some ExtractComputedValue callers (particularly for transitions) can't provide a useful specified-value string.

> Or perhaps we should make ExtractComputedValue handle StyleRule objects for
> clip-path URLs/none/geometry-box values.

Yes. This is the "right" thing to do.

> That would involve a bunch of
> serialization work for the various types if we want to continue to call
> SetUnparsedStringValue though.

Wait, no. We don't want to use SetUnparsedStringValue in these cases, I don't think. We should just use some representation of the value that's approximately similar to our internal computed-style representation.

> Maybe we can do something similar to what we
> do for eCSSProperty_filter and call SetURLValue etc. instead? That's not
> clear to me.

Yes, we should be doing something like that.
Flags: needinfo?(dholbert)
(Assignee)

Comment 37

3 years ago
Attachment #8720281 - Attachment is obsolete: true
Attachment #8740506 - Flags: review?(dholbert)
Comment on attachment 8740506 [details] [diff] [review]
part 6 - Make StyleAnimationValue support css::URLValue

Review of attachment 8740506 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, one commit-message nit:
(there's no commit message inside the patch, so I'm assuming the attachment-name is your tentative commit message, "part 6 - Make StyleAnimationValue support css::URLValue")

Nit: this sounds to me like StyleAnimationValue is holding a pointer directly to a css::URLValue object, which is not actually the case.

Please rephrase to instead talk about "URL-backed nsCSSValue objects", or something else that mentions nsCSSValue (the actual type that we're wrapping here).
Attachment #8740506 - Flags: review?(dholbert) → review+
(Assignee)

Comment 40

3 years ago
I believe this addresses all outstanding review comments except for this one:

(In reply to Daniel Holbert [:dholbert] from comment #4)
> Comment on attachment 8535227 [details] [diff] [review]
> >+++ b/layout/style/nsRuleNode.cpp
> >@@ -8815,16 +8815,19 @@ nsRuleNode::SetStyleClipPathToCSSValue(n
> >                                        nsPresContext* aPresContext,
> >                                        bool& aCanStoreInRuleTree)
> > {
> >   NS_ABORT_IF_FALSE(aValue->GetUnit() != eCSSUnit_ListDep ||
> >                     aValue->GetUnit() != eCSSUnit_List,
> >                     "expected a basic shape or reference box");
> > 
> >   const nsCSSValueList* cur = aValue->GetListValue();
> >+  if (!cur) {
> >+    return;
> >+  }
> > 
> >   uint8_t sizingBox = NS_STYLE_CLIP_SHAPE_SIZING_NOBOX;
> >   nsStyleBasicShape* basicShape = nullptr;
> 
> This doesn't look animation/interpolation related -- what's this change for?
> (i.e. what does it mean to have an empty list here?)  A comment would be
> nice, to explain what's going on.
> 
> (I'm guessing this is something we already needed, which you caught while
> writing animation tests; is it possible to add a test for this to
> property_database.js, so we have non-animation coverage of this requirement?)

I've pushed to Try without this early return to see what, if anything, fails:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cabdccc06337

Hopefully that will inform any comment we add to document this return.
Attachment #8740737 - Flags: review?(dholbert)
(Assignee)

Updated

3 years ago
Depends on: 1264317
Comment on attachment 8740737 [details] [diff] [review]
part 7 - Support CSS animation of clip-path basic shapes

Review of attachment 8740737 [details] [diff] [review]:
-----------------------------------------------------------------

Review notes, part 1:

::: layout/style/StyleAnimationValue.cpp
@@ +1769,5 @@
> +      return 3;
> +    case eCSSKeyword_inset:
> +      return 5;
> +    default:
> +      MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE("Unknown shape type");

Please please please do not use this macro here. It is a footgun, and if we reach it somehow, we will potentially *do completely unsafe exploitable stuff*. (If we reach it, things are already "weird", but we're not necessarily into dangerous territory. This macro puts us well into dangerous territory.)

If we expected this ShapeArgumentCount function to be *extremely* hot, then there *might* be justification to use this macro -- but I don't think that's the case right now.

So: please just use MOZ_ASSERT_UNREACHABLE() (which is just an assertion), and return something reasonable in a never-expected-to-actually-be-reached return statement.  Maybe "return 0", if all the callers can handle that without crashing in the scenario where aShapeFunction is something unrecognized. (I think they might, because the callers have their own switch statements with trivial "default:" cases.  But I haven't sanity-checked that thoroughly yet.)

@@ +2022,5 @@
> +      }
> +      break;
> +    }
> +    default:
> +      MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE("Unknown shape type");

As above, OMG do not use this. Use MOZ_ASSERT_UNREACHABLE.

@@ +3891,5 @@
> +                }
> +                break;
> +              }
> +              default:
> +                MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE("Unknown shape type");

As above, do not use this. Use MOZ_ASSERT_UNREACHABLE.

::: layout/style/nsCSSPropList.h
@@ +3940,5 @@
>      "",
>      0,
>      nullptr,
>      CSS_PROP_NO_OFFSET,
> +    eStyleAnimType_Custom)

I'll request, on heycam's behalf, that you rebase this on top of his patch in bug 1264238 (which reshuffles the contents of this file).

I expect his patch should be landing shortly, and it'll be much easier for you to rebase this one-line change on top of him, rather than for him to rebase his patch (which moves this chunk of the file) on top of you.

::: layout/style/nsRuleNode.cpp
@@ +9370,5 @@
> +    // this to property_database.js, so we have non-animation coverage of this
> +    // requirement?
> +    // And add a comment explaining what it means to have an empty list here?
> +    return;
> +  }

Per IRC and the successful-ish Try run in comment 40 (just infra/intermittent failures I think): this chunk of the patch can simply go away, it seems.

No need to touch nsRuleNode at all in this patch.
(In reply to Daniel Holbert [:dholbert] from comment #42)
> I'll request, on heycam's behalf, that you rebase this on top of his patch
> in bug 1264238 (which reshuffles the contents of this file).
> 
> I expect his patch should be landing shortly, and it'll be much easier for
> you to rebase this one-line change on top of him, rather than for him to
> rebase his patch (which moves this chunk of the file) on top of you.

Thanks. :)  I just landed this on inbound.
Comment on attachment 8740737 [details] [diff] [review]
part 7 - Support CSS animation of clip-path basic shapes

Review of attachment 8740737 [details] [diff] [review]:
-----------------------------------------------------------------

Review notes, part 2 (I've looked at everything in detail except for AddCSSValuePairList and AddShapeFunction, which I'll look at tomorrow)

::: layout/style/StyleAnimationValue.cpp
@@ +1765,5 @@
> +    case eCSSKeyword_circle:
> +    case eCSSKeyword_polygon:
> +      return 2;
> +    case eCSSKeyword_ellipse:
> +      return 3;

Could you document what these magic numbers represent? It's totally non-obvious why e.g. a circle and a polygon would have the same count of arguments. (Actually, it's quite counterintuitive.) Or why 3 and 5 make sense for ellipse & inset.

Also, for better documentability, it's probably best to split eCSSKeyword_circle into its own separate return statement from eCSSKeyword_polygon, so you can document them separately.

I'd like something like the following (my code-comments here are based on studying your ExtractComputedValue code, and I think they're correct, but I'm not 100% sure):

  case eCSSKeyword_circle:
    return 2;  // radius, center-position
  case eCSSKeyword_polygon:
    return 2;  // pairlist-of-points, fill-rule
etc.

@@ +2559,5 @@
> +      const nsCSSValue::Array* array1 = aValue1.GetCSSValueArrayValue();
> +      const nsCSSValue::Array* array2 = aValue2.GetCSSValueArrayValue();
> +      if (!array1 || array1->Count() != 2 ||
> +          !array2 || array2->Count() != 2) {
> +        return false; // can't interpolate

I'm not clear on what this early-return clause represents, at first glance. Can this actually happen?  How do we end up here with a eUnit_Shape but a null array? How do we end up here with eUnit_Shape but an array whose length is something other than 2?

In ExtractComputedValue()'s section on eUnit_Shape, it looks like eUnit_Shape always uses a non-null array of length 2, and always uses something non-null if we succeed... So, maybe this can't actually happen, & this clause is actually unnecessary?

(If it *can* happen, it definitely needs to be more clearly explained. But if it can't happen, it should probably be replaced with assertions.)

@@ +2566,5 @@
> +      const nsCSSValue& func1 = array1->Item(0);
> +      const nsCSSValue& func2 = array2->Item(0);
> +      if (func1.GetUnit() != eCSSUnit_Function ||
> +          func2.GetUnit() != eCSSUnit_Function) {
> +        return false; // can't interpolate without shape functions

Similarly, I'm not sure this clause is necessary. In ExtractComputedValue's eUnit_Shape code, we seem to always set the array's 0th entry to a function.  Is there any scenario where we'd get here and find anything else in Item(0)?

If not, this early-return should probably be converted into a function as well.

@@ +3116,5 @@
>          }
>        }
>        break;
> +    case eUnit_Shape:
> +      {

Nit: it looks like the other cases in this this switch statement are pretty consistent about placing the open-brace on the same line as the "case" statement, rather than on the next line. Best to match that, for consistency.  So, bump this brace up to the previous line here.

@@ +3122,5 @@
> +          aComputedValue.GetCSSValueArrayValue();
> +        if (computedArray) {
> +          aSpecifiedValue.SetArrayValue(computedArray, eCSSUnit_Array);
> +        } else {
> +          aSpecifiedValue.SetNoneValue();

As asked elsewhere here, is it possible for us to have eUnit_Shape and yet have a null css-array-value?  It doesn't seem to me like it is (though I may be missing something). So I don't think we need this "if/else".

@@ +3807,5 @@
> +              new mozilla::css::URLValue(clipPath.GetURL(),
> +                                         uriAsStringBuffer,
> +                                         doc->GetDocumentURI(),
> +                                         doc->NodePrincipal());
> +            nsAutoPtr<nsCSSValue> result(new nsCSSValue);

We're officially moving away from "nsAutoPtr...new", in favor of UniquePtr/MakeUnique (including in this file, per bug 1143008).

So, newly-written code like this patch should use UniquePtr up-front, or else it'll just create more work for ourselves in bug 1143008.  Fortunately, it's easy to fix it in this case -- this should just be:
  UniquePtr<nsCSSValue> result = MakeUnique<nsCSSValue>();

And the "result.forget()" call a few lines lower should be
  result.release()
...which is the UniquePtr version of nsAutoPtr::forget(). (not to be confused with the refcounting Release method which is completely different)

@@ +3813,5 @@
> +            aComputedValue.SetAndAdoptCSSValueValue(result.forget(), eUnit_URL);
> +          } else if (type == NS_STYLE_CLIP_PATH_BOX) {
> +            aComputedValue.SetIntValue(clipPath.GetSizingBox(), eUnit_Enumerated);
> +          } else if (type == NS_STYLE_CLIP_PATH_SHAPE) {
> +            const nsStyleBasicShape* shape = clipPath.GetBasicShape();

This NS_STYLE_CLIP_PATH_SHAPE clause is long enough (~180 lines) & complex enough & deeply-indented enough that I think it'd benefit greatly from having most of its logic factored out into a helper function. That would do a lot for readability of the broader big-picture flow here, IMO (and it would save on indentation & allow linewrapping to be saner in the refactored-out code, too).

If we had a refactored helper-function, I'm imagining this whole clause might just collapse down to something like the following:

  } else if (type == NS_STYLE_CLIP_PATH_SHAPE) {
    const nsStyleBasicShape* shape = clipPath.GetBasicShape();
    RefPtr<nsCSSValue::Array> result = nsCSSValue::Array::Create(2);
    if (!StyleBasicShapeToCSSArray(shape, result)) {
      return false;
    }
    aComputedValue.SetAndAdoptCSSValueArrayValue(result.forget().take(),
                                                 eUnit_Shape);

...where the "StyleBasicShapeToCSSArray" call is replacing ~170 lines with a single call.

@@ +3836,5 @@
> +                    return false;
> +                  }
> +                }
> +                SetPositionValue(shape->GetPosition(),
> +                                 functionArray->Item(functionArray->Count() - 1));

Please add a comment explicitly stating what's going on with functionArray here -- e.g.:
  // We reserve an extra bonus entry in |functionArray| to hold the shape's position.
or something like that.

(There are so many nested arrays within arrays here -- with different nesting between different types of shapes. So, it's nice to clarify things like this that are a bit unexpected -- here, we're concatenating this position onto the end of our shape's coordinate-list, which isn't an intuitively obvious thing to do. This comment would also help explain why we initialized the function with a "+ 1" length a few lines up -- I didn't initially understand what the point of that + 1 was about. (It's about this line right here, I think.))

@@ +3874,5 @@
> +                }
> +                RefPtr<nsCSSValue::Array> radiusArray =
> +                  nsCSSValue::Array::Create(4);
> +                functionArray->Item(functionArray->Count() - 1).
> +                               SetArrayValue(radiusArray, eCSSUnit_Array);

Similarly to my previous comment, please add a comment explicitly stating what's going on with functionArray here -- e.g.: "We reserve an extra bonus entry in |functionArray| to hold *another* array which contains the inset-area's radius values."
...or something like that.

(I think we're 3 arrays deep here -- radiusArray the final entry of functionArray, which is the first entry of |result|. So, a bit mind-bending, & definitely nice to be helpful with a comment at the point where we add a new nesting-level. :))

@@ +3877,5 @@
> +                functionArray->Item(functionArray->Count() - 1).
> +                               SetArrayValue(radiusArray, eCSSUnit_Array);
> +                const nsStyleCorners& radii = shape->GetRadius();
> +                NS_FOR_CSS_FULL_CORNERS(corner) {
> +                  nsAutoPtr<nsCSSValuePair> pair(new nsCSSValuePair);

As above, this should use UniquePtr/MakeUnique, and the "pair.forget()" call 9 lines down should then become "pair.release()".

@@ +4275,5 @@
>        }
>        break;
> +    case eUnit_Shape:
> +      MOZ_ASSERT(mUnit == eUnit_Shape || aOther.mValue.mCSSValueList,
> +                 "value lists other than shapes may not be null");

I think there's a mistake in the assertion here.

This is inside a "switch (mUnit) {" statement, and we're inside of "case eUnit_Shape" -- so it doesn't seem particularly useful to assert that mUnit == eUnit_Shape. :)

I think you really want to be asserting that aOther.mValue.mCSSValueArray is non-null here (*not* ValueList), since ValueArray is what we use for eUnit_Shape.  ("SetAndAdoptCSSValueArrayValue")

@@ +4277,5 @@
> +    case eUnit_Shape:
> +      MOZ_ASSERT(mUnit == eUnit_Shape || aOther.mValue.mCSSValueList,
> +                 "value lists other than shapes may not be null");
> +      if (aOther.mValue.mCSSValueList) {
> +        mValue.mCSSValueArray = aOther.mValue.mCSSValueArray;

So here, the "mCSSValueList" null-check seems busted.  But I also think it's unnecessary, because eUnit_Shape always creates a non-null ValueArray. So I think you can just drop this null check.

@@ +4280,5 @@
> +      if (aOther.mValue.mCSSValueList) {
> +        mValue.mCSSValueArray = aOther.mValue.mCSSValueArray;
> +        mValue.mCSSValueArray->AddRef();
> +      } else {
> +        mValue.mCSSValueList = nullptr;

And you can drop this null assignment (which seems nonsensical anyway, since eUnit_Shape has nothing to do with mCSSValueList at this point).

@@ +4424,5 @@
> +{
> +  FreeValue();
> +  MOZ_ASSERT(IsCSSValueArrayUnit(aUnit), "bad unit");
> +  MOZ_ASSERT(aUnit == eUnit_Shape || aValue != nullptr,
> +             "value arrays other than shapes may not be null");

Two questions...
 (1) are there any value arrays other than shapes?   (I don't think so)
 (2) Do we ever call this function with null arg for arrays?  (I don't think so)


So I think this assert might just want to be:
  MOZ_ASSERT(aValue != nullptr,
	     "value arrays may not be null");

@@ +4522,5 @@
>      case eUnit_CSSRect:
>        return *mValue.mCSSRect == *aOther.mValue.mCSSRect;
>      case eUnit_Dasharray:
>      case eUnit_Shadow:
> +    case eUnit_Shape:

This change is wrong, now that you're using an array unit here (not a value list). Right now, this new case statement drops down into a "nsCSSValueList::Equal()" comparison, which made sense when we were using a valuelist as our backing store, but we're not using a valuelist anymore.

So: you need to bump "case eUnit_Shape" up a few lines here (to be before eUnit_Dasharray) and give it its own dedicated return statement here, which I think can just be:
  return *mValue.mCSSValueArray == *aOther.mValue.mCSSValueArray;

::: layout/style/StyleAnimationValue.h
@@ +280,5 @@
>      eUnit_CSSValueTriplet, // nsCSSValueTriplet* (never null)
>      eUnit_CSSRect, // nsCSSRect* (never null)
>      eUnit_Dasharray, // nsCSSValueList* (never null)
>      eUnit_Shadow, // nsCSSValueList* (may be null)
> +    eUnit_Shape,  // nsCSSValue::Array* (may be null)

Is "may be null" here really true? How do we end up with a null array value?  This might really want to say "never null".
Comment on attachment 8740737 [details] [diff] [review]
part 7 - Support CSS animation of clip-path basic shapes

Review of attachment 8740737 [details] [diff] [review]:
-----------------------------------------------------------------

Review notes, part 3 (of 3):

::: layout/style/StyleAnimationValue.cpp
@@ +1842,5 @@
>    return result;
>  }
>  
> +static bool
> +AddCSSValuePairList(nsCSSProperty aProperty, uint32_t aRestrictions,

Two concerns:
 (1) I think you can drop the aRestrictions parameter here, and just look it up locally with...
  uint32_t restrictions = nsCSSProps::ValueRestrictions(aProperty);

(2) This function looks like it's largely based on AddWeighted()'s CSSValuePairList code, which lives here:
http://mxr.mozilla.org/mozilla-central/source/layout/style/StyleAnimationValue.cpp?rev=092022aae047#2473

BUT, this patch is leaving that original code in place, which means we're duplicating all of this logic, effectively.  Seems like that old code in AddWeighted should be calling out to this new helper-function.

So, could you spin this helper-function out into its own patch, which is just purely about refactoring out this piece of AddWeighted? And then your final main patch here can simply be adding a new call to this method.

(You can consider my remaining nits for this function as applying to that proposed new refactring-patch which will be creating this function.)

@@ +1848,5 @@
> +                    double aCoeff2, const nsCSSValuePairList* aList2,
> +                    nsCSSValue& aResult)
> +{
> +  MOZ_ASSERT(aList1, "expected coordinate");
> +  MOZ_ASSERT(aList2, "expected coordinate");

Please replace the assertion messages here with "Expected non-null list", or "Can't add a null list" or something like that.

(The current assertion message -- "expected coordinate" -- didn't initially make sense to me.  It sounds like it's saying aList1 is a coordinate (singular), despite the fact that aList is probably really something plural.  And it's unclear from the context here why we're expecting these lists to be coordinates in particular, since the function-name is more generic than that (AddCSSValuePairList).)

@@ +1851,5 @@
> +  MOZ_ASSERT(aList1, "expected coordinate");
> +  MOZ_ASSERT(aList2, "expected coordinate");
> +
> +  // To ensure that we don't touch aResult unless we know that we're going
> +  // succeed we use this temporary.

Please add comma after "succeed", for readability & clarity.

@@ +1864,5 @@
> +    for (uint32_t i = 0; i < ArrayLength(pairListValues); ++i) {
> +      const nsCSSValue &v1 = aList1->*(pairListValues[i]);
> +      const nsCSSValue &v2 = aList2->*(pairListValues[i]);
> +
> +      nsCSSValue &vr = resultPtr->*(pairListValues[i]);

Nit: ampersand should be type-hugging here (shifted to the left side of the space character), for the v1/v2/vr declarations.

Should be e.g.
  nsCSSValue& v1
etc.

@@ +1872,5 @@
> +        return false;
> +      }
> +      if (!AddCSSValuePixelPercentCalc(aRestrictions, unit,
> +                                       aCoeff1, v1,
> +                                       aCoeff2, v2, vr) ) {

Drop stray space character between close-parens here.

@@ +1876,5 @@
> +                                       aCoeff2, v2, vr) ) {
> +        if (v1 != v2) {
> +          return false;
> +        }
> +        vr = v1;

I don't understand what this case is doing.  (i.e. I don't understand why we need to check for addition failure, and then proceed if the values were equal) Could you add a comment to explain?

(It looks like this is copypasted from the AddWeighted ValuePairList case [which this code seems to be based on as noted above].  In that area of the code, I'm guessing it's to handle enum-typed units inside of a ValuePairList, since that code is primarily for handling "background-size" right now IIUC, and that property can take enum values in its value-pair-list I think.)

@@ +1897,5 @@
> +  nsCSSValuePairList* outResult = aResult.SetPairListValue();
> +  outResult->mXValue = result.mXValue;
> +  outResult->mYValue = result.mYValue;
> +  outResult->mNext = result.mNext;
> +  result.mNext = nullptr; // so that the list we just transfered isn't deleted!

This "result" ownership-transfer feels hacky.  I'd prefer we use AdoptPairListValue, whose internals look a lot like your code here.  That would abstract away this ugliness, and let us share the code that does this ownership transfer, making it more likely to be correct & tested & maintained.

In order for us to be able to use AdoptPairListValue, the local varible |result| (the thing we're adopting from) needs to be heap-allocated instead of stack-allocated, because AdoptPairListValue deletes its arg after stealing from it.  So, |result| should be a UniquePtr, and we can steal from it by calling...
  aResult.AdoptPairListValue(result.release());

(We already do something like this elsewhere in this file, BTW -- here:
http://mxr.mozilla.org/mozilla-central/source/layout/style/StyleAnimationValue.cpp?rev=092022aae047&mark=2915-2915,2918-2918#2913
)

@@ +1920,5 @@
> +             "expected geometry-box");
> +  MOZ_ASSERT(aArray1->Item(1).GetIntValue() == aArray2->Item(1).GetIntValue(),
> +             "expected matching geometry-box values");
> +  RefPtr<nsCSSValue::Array> func1 = aArray1->Item(0).GetArrayValue(),
> +                            func2 = aArray2->Item(0).GetArrayValue();

func1 and func2 here should probably be declared as "const nsCSSValue::Array*" -- not as RefPtr<nsCSSValue::Array>.

 - They should be "const" because we should not let ourselves modify these arrays.  They're a piece of our args (the things we're adding together) which are intended to be const.

 - They can (& should IMO) be raw pointers, because they're kept alive by our args aArray1/aArray2, and we're already implicitly assuming that those *args* must stay alive for the duration of this function (or else it wouldn't be safe for us to use them).  So -- if we use RefPtr here, it just means we'll incur extra addref/release churn, which is is a cost with no demonstrable benefit.

@@ +1934,5 @@
> +                                      ShapeArgumentCount(shapeFuncName));
> +  switch (shapeFuncName) {
> +    case eCSSKeyword_ellipse:
> +      // We fail if the length value is an enumeration.
> +      if (!AddCSSValuePixelPercentCalc(CSS_PROPERTY_VALUE_NONNEGATIVE,

This code would benefit from a "Add [whatever]:"  above each of the AddCSSValuePixelPercentCalc statements.  (Right now you've sort of got these documented, but only lower down in the failure conditions. and the documentation is helpful since there are so many 1's and 2's mixed into the function-calls, and it's hard to keep track of what is what, in terms of func2->Item(1) etc.)

So, please broaden the comment above the function-call here to something like:
 // Add ellipses' |ry| values (but fail if we encounter an enum):

@@ +1945,5 @@
> +        return nullptr; // failed to add ry
> +      }
> +      MOZ_FALLTHROUGH;  // to handle rx and center point
> +    case eCSSKeyword_circle: {
> +      if (!AddCSSValuePixelPercentCalc(CSS_PROPERTY_VALUE_NONNEGATIVE,

And this should get a comment like:
 // Add circles' |r| (or ellipses' |rx|) values:

@@ +1998,5 @@
> +      MOZ_ASSERT(func1->Item(5).GetUnit() == eCSSUnit_Array &&
> +                 func2->Item(5).GetUnit() == eCSSUnit_Array,
> +                 "Expected two arrays");
> +      RefPtr<nsCSSValue::Array> radii1 = func1->Item(5).GetArrayValue(),
> +                                radii2 = func2->Item(5).GetArrayValue();

These variables should have type "const nsCSSValue::Array*", for the same reason discussed above for func1 / func2.

@@ +2011,5 @@
> +                                     // we use an arbitrary border-radius
> +                                     // property here to get the appropriate
> +                                     // restrictions for radii since this is a
> +                                     // <border-radius> value.
> +                                     nsCSSProps::ValueRestrictions(eCSSProperty_border_top_left_radius),

This line is much too long. Let's just look up these restrictions a bit earlier & store the result in a const local variable.  Then this can be formatted a bit more nicely (and the comment may end up a bit shorter, too, since it won't need to be super-indented).

Moreover, we might as well pull that local variable up outside of the "for" loop.  (We don't really need to call ValueRestrictions repeatedly for each loop iteration here.)
Attachment #8740737 - Flags: review?(dholbert) → review-
(Assignee)

Comment 46

3 years ago
(In reply to Daniel Holbert [:dholbert] from comment #44)
> I'm not clear on what this early-return clause represents, at first glance.
> Can this actually happen?

I don't think so and the MOZ_ASSERTs in AddShapeFunction cover all these cases.

> This comment would also help explain why we
> initialized the function with a "+ 1" length a few lines up -- I didn't
> initially understand what the point of that + 1 was about. (It's about this
> line right here, I think.))

Correct.

> I think you really want to be asserting that aOther.mValue.mCSSValueArray is
> non-null here (*not* ValueList)

Good catch! And I thought I was very careful during the conversion from list to array...

> So here, the "mCSSValueList" null-check seems busted.

Huh, I fixed that locally but somehow that didn't make it into the patch. Must have forgot a qrefresh.

> @@ +4424,5 @@
> > +{
> > +  FreeValue();
> > +  MOZ_ASSERT(IsCSSValueArrayUnit(aUnit), "bad unit");
> > +  MOZ_ASSERT(aUnit == eUnit_Shape || aValue != nullptr,
> > +             "value arrays other than shapes may not be null");
> 
> Two questions...
>  (1) are there any value arrays other than shapes?   (I don't think so)

Not currently, but it would be nice for this code to be structured to handled things in case we do in future.

>  (2) Do we ever call this function with null arg for arrays?  (I don't think
> so)

Not currently.

> This change is wrong, now that you're using an array unit here (not a value
> list). Right now, this new case statement drops down into a
> "nsCSSValueList::Equal()" comparison, which made sense when we were using a
> valuelist as our backing store, but we're not using a valuelist anymore.
> 
> So: you need to bump "case eUnit_Shape" up a few lines here (to be before
> eUnit_Dasharray) and give it its own dedicated return statement here, which
> I think can just be:
>   return *mValue.mCSSValueArray == *aOther.mValue.mCSSValueArray;

Yeah, I had this fixed locally. I think we even discussed part of this on IRC. Sorry it didn't make it into the attached patch.
(Assignee)

Comment 47

3 years ago
(In reply to Daniel Holbert [:dholbert] from comment #45)
> @@ +1876,5 @@
> > +                                       aCoeff2, v2, vr) ) {
> > +        if (v1 != v2) {
> > +          return false;
> > +        }
> > +        vr = v1;
> 
> I don't understand what this case is doing.  (i.e. I don't understand why we
> need to check for addition failure, and then proceed if the values were
> equal) Could you add a comment to explain?
> 
> (It looks like this is copypasted from the AddWeighted ValuePairList case
> [which this code seems to be based on as noted above].  In that area of the
> code, I'm guessing it's to handle enum-typed units inside of a
> ValuePairList, since that code is primarily for handling "background-size"
> right now IIUC, and that property can take enum values in its
> value-pair-list I think.)

I don't know the reason for this case either, but I've addressed everything else so putting up an updated patch for review while I consider what that comment should say.
(Assignee)

Comment 48

3 years ago
Thanks for the very thorough review.
Attachment #8740737 - Attachment is obsolete: true
Attachment #8742985 - Flags: review?(dholbert)
(Assignee)

Comment 49

3 years ago
Anticipating that you'd prefer the factoring out to be a separate patch. :)
Attachment #8743007 - Flags: review?(dholbert)
(Assignee)

Comment 50

3 years ago
Attachment #8742985 - Attachment is obsolete: true
Attachment #8742985 - Flags: review?(dholbert)
Attachment #8743009 - Flags: review?(dholbert)
Comment on attachment 8743007 [details] [diff] [review]
part 7 - Factor out code for adding two nsCSSValuePairList lists

Review of attachment 8743007 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for factoring this out! r=me, with nits addressed:

::: layout/style/StyleAnimationValue.cpp
@@ +1829,5 @@
> +{
> +  MOZ_ASSERT(aList1, "Can't add a null list");
> +  MOZ_ASSERT(aList2, "Can't add a null list");
> +
> +  UniquePtr<nsCSSValuePairList> result = MakeUnique<nsCSSValuePairList>();

This should be:
  auto result = MakeUnique<nsCSSValuePairList>();

(This is the recommended idiom (and most common current usage AFAICT) for MakeUnique, for brevity & to avoid needlessly repeating the same type on a single line. There's no loss of clarity from using "auto" here, since the type is unambiguously specified by the MakeUnique expression. This is semi-documented in the UniquePtr.h comments, under the benefits of MakeUnique.)

This applies to all of the lines where we call MakeUnique in the other patch, as well (part 8) -- please replace the variable-type at the beginning of those lines with "auto".

@@ +1840,5 @@
> +    };
> +    uint32_t restrictions = nsCSSProps::ValueRestrictions(aProperty);
> +    for (uint32_t i = 0; i < ArrayLength(pairListValues); ++i) {
> +      const nsCSSValue &v1 = aList1->*(pairListValues[i]);
> +      const nsCSSValue &v2 = aList2->*(pairListValues[i]);

Per comment 45, the ampersand should be type-hugging for "v1" and "v2" decls here.

@@ +1854,5 @@
> +                                       aCoeff2, v2, vr)) {
> +        if (v1 != v2) {
> +          return nullptr;
> +        }
> +        vr = v1;

This clause (everything inside of the AddCSSValuePixelPercentCalc failure case) still needs an explanatory comment, to justify its existence & explain what it's doing.  (Sounds like you're on planning on adding something here, per comment 47.)
Attachment #8743007 - Flags: review?(dholbert) → review+
Comment on attachment 8743009 [details] [diff] [review]
part 8 - Support CSS animation of clip-path basic shapes

Review of attachment 8743009 [details] [diff] [review]:
-----------------------------------------------------------------

Part 8 review feedback, "wave 1 of 2" (I think):

::: layout/style/StyleAnimationValue.cpp
@@ +1923,5 @@
> +  RefPtr<nsCSSValue::Array> resultArray = nsCSSValue::Array::Create(2);
> +
> +  nsCSSValue::Array* result =
> +    resultArray->Item(0).InitFunction(shapeFuncName,
> +                                      ShapeArgumentCount(shapeFuncName));

Hmm, I'm now realizing that "result" and "resultArray" are very confusingly named.  They're both versions of this function's result, and they're both pointers to nsCSSValue::Array.  So, their names ("result" vs "resultArray") doesn't help to distinguish them at all -- they're both "result arrays".

I think we should rename them as follows:
 (1) |result| should be renamed to |resultFuncArgs|, since that's what it is (the shape-function's arguments, for the result value).  This variable gets used with "func1" and "func2", so it's nice for it to have "func" in its name for consistency.
 (2) |resultArray| should probably be shortened to simply |result|, since it *is* in fact the variable that we return from this function.

@@ +1955,5 @@
> +                   aCoeff2, func2->Item(posIndex),
> +                   result->Item(posIndex));
> +      break;
> +    }
> +    case eCSSKeyword_polygon: {

This polygon clause could use an "Add [something]" line of documentation (like the ones you added above for circle/ellipse cases), to sum up what's happening in this clause.

Something like:
    // Add polygons' corresponding points (if the fill rule matches):

(The final "inset" case is a bit clearer up-front about what it's doing, so I'm less concerned with that one needing documentation.)

@@ +1965,5 @@
> +
> +      const nsCSSValuePairList* list1 = func1->Item(2).GetPairListValue();
> +      const nsCSSValuePairList* list2 = func2->Item(2).GetPairListValue();
> +      UniquePtr<nsCSSValuePairList> center =
> +        AddCSSValuePairList(aProperty, aCoeff1, list1, aCoeff2, list2);

Two things:
 (1) "center" is definitely the wrong name for this variable here. (copypaste error I think?)  This is for the "polygon" case, and we're adding two point lists. ("list1" and "list2")  So we should probably capture the result in something called "resultPointList".

 (2) While we're at it -- if you like, "list1" and "list2" could be more clearly-named "pointList1" / "pointList2", to make it clearer what they're lists of.  (not a big deal if you'd rather leave these as-is, though.)

@@ +1996,5 @@
> +                 "Expected two arrays");
> +      const nsCSSValue::Array* radii1 = func1->Item(5).GetArrayValue();
> +      const nsCSSValue::Array* radii2 = func2->Item(5).GetArrayValue();
> +      MOZ_ASSERT(radii1->Count() == 4 && radii2->Count() == 4);
> +      nsCSSValue::Array* arr = nsCSSValue::Array::Create(4);

This is the 3rd level of nsCSSValue::Array nesting (resultArray contains result, which contains arr, each of which is an array).  So we should probably use a clearer name than "arr" to distinguish what this array is.

How about "resultRadii"? (for consistency with the inputs "radii1" & "radii2" declared just above it)

@@ +1998,5 @@
> +      const nsCSSValue::Array* radii2 = func2->Item(5).GetArrayValue();
> +      MOZ_ASSERT(radii1->Count() == 4 && radii2->Count() == 4);
> +      nsCSSValue::Array* arr = nsCSSValue::Array::Create(4);
> +      result->Item(5).SetArrayValue(arr, eCSSUnit_Array);
> +      // we use an arbitrary border-radius property here to get the appropriate

Nit: capitalize "we" at the start here (otherwise it looks a bit like this might've been the second half of a longer comment that got truncated).

@@ +2018,5 @@
> +      break;
> +    }
> +    default:
> +      MOZ_ASSERT_UNREACHABLE("Unknown shape type");
> +  }

We should probably "return nullptr" in this default case here, for safety's sake if we add a new shape & forget to update this switch statement. (Then we'll be indicating failure in a way that the caller understands, rather than returning an awkward non-null partially-populated result value for a shape that we don't recognize).

@@ +2552,5 @@
>      }
> +    case eUnit_Shape: {
> +      MOZ_ASSERT(aValue1.GetUnit() == eUnit_Shape &&
> +                 aValue2.GetUnit() == eUnit_Shape,
> +                 "We should have created an nsCSSValue::Array for clip shapes");

tl;dr: I think this assertion wants to go away, or have its message rewritten.

This assertion doesn't make sense to me right now, for two reasons:
 (1) Its condition is trivially satisfied -- it's essentially a tautology.  (We are in a "switch" statement over the common unit of aValue1 & aValue2.  So, inside the case statement for common-unit==eUnit_Shape, clearly aValue1/aValue2 must have type eUnit_Shape. Put another way: we don't bother asserting this condition in any of the other case statements here, and I don't see why there's any special reason we need to assert it here.)

 (2) The assertion message (about us having created an array) doesn't really match the condition that we're actually checking (about eUnit_Shape).

I think you maybe meant for this assertion to be checking that the values have non-null arrays...?  But that's not really even necessary, because AddShapeFunction will already perform that assertion for us.

So, I think this assertion can just be dropped.
Attachment #8743009 - Flags: review?(dholbert) → review+
Comment on attachment 8743009 [details] [diff] [review]
part 8 - Support CSS animation of clip-path basic shapes

(Er, didn't mean to set r+ on part 8 quite yet; though, this is likely r+ after I've gotten through it.)
Attachment #8743009 - Flags: review+ → review?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #51)
> > +  UniquePtr<nsCSSValuePairList> result = MakeUnique<nsCSSValuePairList>();
> 
> This should be:
>   auto result = MakeUnique<nsCSSValuePairList>();

(I just realized this contradicts what I'd initially suggested in comment 44. Sorry for leading you semi-astray on that at first!  I should have suggested "auto" up-front.)
Comment on attachment 8743009 [details] [diff] [review]
part 8 - Support CSS animation of clip-path basic shapes

Review of attachment 8743009 [details] [diff] [review]:
-----------------------------------------------------------------

"wave 2" of comments (not done yet):

::: layout/style/StyleAnimationValue.cpp
@@ +3396,5 @@
>  }
>  
> +static bool
> +StyleClipBasicShapeToCSSArray(const nsStyleClipPath& aClipPath,
> +                              RefPtr<nsCSSValue::Array>& aResult)

aResult should just have type "nsCSSValue::Array*".

As long as we're not modifying the refcount here (or changing |aResult| to point at something else), that's simpler and better-matches our standard practices (as compared to passing a RefPtr<>&).

@@ +3403,5 @@
> +             "Expected array to be presized for a function and the sizing-box");
> +
> +  const nsStyleBasicShape* shape = aClipPath.GetBasicShape();
> +  nsCSSKeyword functionName = shape->GetShapeTypeName();
> +  RefPtr<nsCSSValue::Array> functionArray;

I think we can & should assign functionArray immediately, like so:
  RefPtr<nsCSSValue::Array> functionArray =
    aResult->Item(0).InitFunction(functionName,
                                  ShapeArgumentCount(functionName));

I believe we do exactly this inside of the various "case" statements below this, except we hardcode the argument count for polygon & inset, kind of (we have hardcoded 2 and 4 inside those cases).

So -- I'd prefer to only call InitFunction once (showing the common structure regardless of shape -- functionName + ShapeArgumentCount args), and then use functionArray->Count() in place of hardcoded numbers (and in place of explicit extra ShapeArgumentCount invocations) in the switch cases below this.

@@ +3421,5 @@
> +                                         functionArray->Item(i + 1))) {
> +          return false;
> +        }
> +      }
> +      // Set the last item in functionArray to the ellipse's center point:

"ellipse" isn't quite correct here --this is the case for eCircle *and* eEllipse. So, this comment should say "ellipse or circle".

You can keep it a one-liner if you reword like so:
      // Set functionArray's last item to the ellipse or circle's center point:
Comment on attachment 8743009 [details] [diff] [review]
part 8 - Support CSS animation of clip-path basic shapes

Review of attachment 8743009 [details] [diff] [review]:
-----------------------------------------------------------------

One last review comment, which I think you already addressed locally.

r=me, though I should perhaps take a final look at the functionArray/SheetArgument changes (which as you noted in IRC, shouldn't be factored out quite as much as I just suggested, per an older review comment of mine)

::: layout/style/StyleAnimationValue.cpp
@@ +3449,5 @@
> +    case nsStyleBasicShape::Type::eInset: {
> +      const nsTArray<nsStyleCoord>& coords = shape->Coordinates();
> +      MOZ_ASSERT(coords.Length() == 4, "Unexpected offset count");
> +      functionArray =
> +      aResult->Item(0).InitFunction(functionName, coords.Length() + 1);

(This line is mis-indented.)
Attachment #8743009 - Flags: review?(dholbert) → review+
(Assignee)

Comment 57

3 years ago
Attachment #8743009 - Attachment is obsolete: true
Attachment #8743052 - Flags: review?(dholbert)
Comment on attachment 8743052 [details] [diff] [review]
part 8 - Support CSS animation of clip-path basic shapes

Review of attachment 8743052 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/StyleAnimationValue.cpp
@@ +3450,5 @@
> +    }
> +    case nsStyleBasicShape::Type::eInset: {
> +      const nsTArray<nsStyleCoord>& coords = shape->Coordinates();
> +      MOZ_ASSERT(coords.Length() == ShapeArgumentCount(functionName),
> +                 "Unexpected offset count");

I think this assertion needs a "-1", since coords.Length() is 4 (according to the older patch's version of this assertion) but ShapeArgumentCount() returns 5 (it includes a slot for the radius array).

@@ +3844,5 @@
> +          } else if (type == NS_STYLE_CLIP_PATH_BOX) {
> +            aComputedValue.SetIntValue(clipPath.GetSizingBox(), eUnit_Enumerated);
> +          } else if (type == NS_STYLE_CLIP_PATH_SHAPE) {
> +            RefPtr<nsCSSValue::Array> result = nsCSSValue::Array::Create(2);
> +            if (!StyleClipBasicShapeToCSSArray(clipPath, result.get())) {

(I didn't think you'd need to add "get()" here -- I think you should be able to directly pass |result| here, and it'll cast from RefPtr to the pointer type.  Though maybe that's changed...)
Attachment #8743052 - Flags: review?(dholbert) → review+
Sorry, two late-breaking nits on StyleClipBasicShapeToCSSArray

First, I'll walk back my "-1" suggestion here:

(In reply to Daniel Holbert [:dholbert] from comment #58)
> ::: layout/style/StyleAnimationValue.cpp
> @@ +3450,5 @@
> > +    }
> > +    case nsStyleBasicShape::Type::eInset: {
> > +      const nsTArray<nsStyleCoord>& coords = shape->Coordinates();
> > +      MOZ_ASSERT(coords.Length() == ShapeArgumentCount(functionName),
> > +                 "Unexpected offset count");
> 
> I think this assertion needs a "-1", since coords.Length() is 4 (according
> to the older patch's version of this assertion) but ShapeArgumentCount()
> returns 5 (it includes a slot for the radius array).

Instead of adding a -1 on the right side,, it'd be better to add a "+1" on the *left* side of this comparison -- because that will better explain/correspond-to the "coords.Length() + 1" allocation on the next line.

Same goes for the "Unexpected radii count" assertion in the circle/ellipse case, higher up in this function -- that assertion should have "coords.Length() + 1" on the left side (and no -1 on the right side), because we immediately do an allocation of size "coords.Length() + 1" after it, and it's nice if that allocation clearly matches the quantity that we're asserting about.
Flags: needinfo?(jwatt)
(comment 59 could be addressed in a trivial followup with rs=me, if you like -- just moving the "-1" on the right to a "+1" on the left in those two assertions about coords.Length() in StyleClipBasicShapeToCSSArray.)

(also, I think "leave-open" keyword can be removed now, yes?)
(Assignee)

Comment 63

3 years ago
Sorry, missed those last comments. And yes, leave-open can be removed.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: needinfo?(jwatt)
Keywords: leave-open
Resolution: --- → FIXED

Comment 64

3 years ago
It's strange, if the unit is not in the same format (one in percent & second in other unit), the animation does not work.

For example: 

div {
  transition: all 0.4s;
  clip-path: circle(10px at 50% 50%);
}
div:hover {
  clip-path: circle(50% at 50% 50%);
}

- 10px -> 50%  : KO
- 5rem -> 50%  : KO
- 10px -> 5em  : OK
- 10px -> 5pc  : OK
- 10px -> 5ch  : OK
- 10px -> 5rem : OK
- 10%  -> 50%  : OK

Comment 65

3 years ago
A live example: http://codepen.io/chriscoyier/pen/d70643d71d123cbf13ad9b1980438526
Replace "0" to "0%" in div:hover and this example pass.
Comment hidden (obsolete)
Sorry, disregard comment 67; the final patch here (part 8) did include tests, but I'd just forgotten about them since they weren't the piece that had changed substantially recently.

--> setting "in-testsuite:+".
Flags: needinfo?(jwatt)
Flags: in-testsuite?
Flags: in-testsuite+
No longer blocks: 1266868
Depends on: 1266868
Jonathan: so this doesn't we can now animate (or transition) clip-path with <basic-shape> (not activated yet by default, and w/ the restriction of bug 1266570)?
Flags: needinfo?(jwatt)
(Assignee)

Comment 70

3 years ago
teoli: your question seemed to get a bit jumbled, but if I ignore the words "so this doesn't", then you are correct.
Flags: needinfo?(jwatt)
Oh yes, I don't know where these words came from. Thanks!
Blocks: 1289049
Blocks: 1313619

Updated

2 years ago
Blocks: basic-shape
You need to log in before you can comment on or make changes to this bug.