1072894 - Implement css parsing for clip-path polygon() basic shapes

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Dirk Schulze]

Attachment: clip-path-polygon.patch

Comment 2

[Cameron McCormack (:heycam)]

Comment on attachment 8495487 [details] [diff] [review]
clip-path-polygon.patch

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

Looking good overall, but I'd like to see another version with these comments addressed.

::: layout/style/nsCSSParser.cpp
@@ +959,5 @@
>    bool IsParsingCompoundProperty(void) const {
>      return mParsingCompoundProperty;
>    }
>  
> +  /* Functions for basic shapes **/

Drop one of the two trailing asterisks.

@@ +13738,5 @@
>    AppendValue(eCSSProperty_transform, value);
>    return true;
>  }
>  
> +/* Reads a function list of arguments of polygon.

"Reads a polygon function's argument list."

@@ +13751,5 @@
> +    numArgs++;
> +
> +    // The fill-rule must be comma separated from the polygon points.
> +    if (!ExpectSymbol(',', true)) {
> +      return false;

Please report a CSS parse error here: REPORT_UNEXPECTED_TOKEN(PEExpectedComma).

Also, whenever we encounter a parse error, we need to make sure that we skip over any balanced content before returning false, if we know that we have already parsed an "opening" token.  So here, since we know that we have parsed the FUNCTION token, we should call |SkipUntil(')')| so that we eat up any balanced content, including the closing paren.

@@ +13761,5 @@
> +  for (;;) {
> +    nsCSSValue xValue, yValue;
> +    if (!ParseVariant(xValue, VARIANT_LPCALC, nullptr) ||
> +        !ParseVariant(yValue, VARIANT_LPCALC, nullptr)) {
> +      return false;

Needs error reporting and a SkipUntil call before returning false.

@@ +13769,5 @@
> +
> +    // See whether to continue or whether to look for end of function.
> +    if (!ExpectSymbol(',', true)) {
> +      // We need to read the closing parenthesis.
> +      if (!ExpectSymbol(')', true)) {

Needs error reporting and a SkipUntil call before returning false.

@@ +13782,5 @@
> +  nsRefPtr<nsCSSValue::Array> functionArray =
> +    aValue.InitFunction(eCSSKeyword_polygon, numArgs);
> +  functionArray->Item(numArgs) = coordinates;
> +  if (numArgs > 1) {
> +    functionArray->Item(1) = fillRuleValue;    

Remove trailing white space.

@@ +13801,5 @@
> +    return false;
> +  }
> +
> +  nsCSSKeyword keyword = nsCSSKeywords::LookupKeyword(mToken.mIdent);
> +  switch(keyword) {

Space before "(".

@@ +13803,5 @@
> +
> +  nsCSSKeyword keyword = nsCSSKeywords::LookupKeyword(mToken.mIdent);
> +  switch(keyword) {
> +  case eCSSKeyword_polygon:
> +    return ParsePolygonFunction(aValue);

(It's a slight shame that we can't use ParseFunction for this, but it doesn't handle keyword values or pair values.)

@@ +13823,5 @@
> +    }
> +
> +    bool shape = false, box = false;
> +    nsCSSValueList* cur = value.SetListValue();
> +    for (unsigned i = 0; i < 2; ++i) {

Just use "int" instead of "unsigned".

@@ +13826,5 @@
> +    nsCSSValueList* cur = value.SetListValue();
> +    for (unsigned i = 0; i < 2; ++i) {
> +      if (ParseBasicShape(cur->mValue) && !shape) {
> +        shape = true;
> +      } else if (ParseEnum(cur->mValue, nsCSSProps::kClipShapeSizingKTable)) {

Should we have "&& !box" at the end of the condition?

@@ +13829,5 @@
> +        shape = true;
> +      } else if (ParseEnum(cur->mValue, nsCSSProps::kClipShapeSizingKTable)) {
> +        box = true;
> +      } else {
> +        return false;

Shouldn't this be a break rather than a return false?

Also we need some error reporting in this function.

@@ +13831,5 @@
> +        box = true;
> +      } else {
> +        return false;
> +      }
> +      if (!GetToken(true)) {

We don't need to check for EOF here for correct parsing -- we should just go around the loop again and get false returned from ParseBasicShape and/or ParseEnum -- but it might be good to keep this here and set a boolean to indicate whether we encountered EOF before breaking.  That would let us do a REPORT_UNEXPECTED_EOF in the |if (!shape && !box)| statement below.

@@ +13839,5 @@
> +      cur->mNext = new nsCSSValueList;
> +      cur = cur->mNext;
> +    }
> +    if (!shape && !box) {
> +      return false;

As well as the REPORT_UNEXPECTED_EOF mentioned above, we should have a REPORT_UNEXPECTED_TOKEN in here for when we encountered a token and neither of the shape and box parsed.

::: layout/style/nsCSSProps.cpp
@@ +1886,5 @@
> +  eCSSKeyword_border_box,  NS_STYLE_CLIP_SHAPE_SIZING_BORDER,
> +  eCSSKeyword_margin_box,  NS_STYLE_CLIP_SHAPE_SIZING_MARGIN,
> +  eCSSKeyword_fill_box,  NS_STYLE_CLIP_SHAPE_SIZING_FILL,
> +  eCSSKeyword_stroke_box,  NS_STYLE_CLIP_SHAPE_SIZING_STROKE,
> +  eCSSKeyword_view_box,  NS_STYLE_CLIP_SHAPE_SIZING_VIEW,

Either use a single space between the values on each line or line them up neatly.  (There is a mix of lining up and not lining up in this file, so choose whichever you prefer.)

::: layout/style/nsCSSProps.h
@@ +536,5 @@
>    static const KTableValue kBoxOrientKTable[];
>    static const KTableValue kBoxPackKTable[];
>    static const KTableValue kDominantBaselineKTable[];
>    static const KTableValue kFillRuleKTable[];
> +  static const KTableValue kClipShapeSizingKTable[];

Keep this list ordered.

::: layout/style/nsCSSValue.cpp
@@ +829,5 @@
>  void
> +nsCSSValue::AppendPolygonToString(nsCSSProperty aProperty, nsAString& aResult,
> +                                  Serialization aSerialization) const
> +{
> +    const nsCSSValue::Array* array = GetArrayValue();

Too much indentation.

@@ +834,5 @@
> +    NS_ABORT_IF_FALSE(array->Count() > 1 && array->Count() <= 3,
> +                      "Polygons must have name and at least one more value.");
> +    size_t index = 1;
> +    if (array->Count() == 3) {
> +      const nsCSSValue& fillRuleValue = array->Item(index);

Add a comment somewhere in here to say that if the array has 2 elements, then the item at index 1 is the coordinate pair list, and if it has 3 elements, then the item at index 1 is the fill rule and index 2 is the coordinate pair list.

@@ +836,5 @@
> +    size_t index = 1;
> +    if (array->Count() == 3) {
> +      const nsCSSValue& fillRuleValue = array->Item(index);
> +      NS_ABORT_IF_FALSE(fillRuleValue.GetUnit() == eCSSUnit_Enumerated,
> +                      "Expected polygon fill rule.");

Line up the argument after the paren on the previous line.

@@ -943,5 @@
> -          break;
> -      }
> -      nsStyleUtil::AppendEscapedCSSIdent(ident, aResult);
> -    } else {
> -      MOZ_ASSERT(false, "should no longer have non-enumerated functions");

Yeah I guess assuming this now is safe.

@@ +976,3 @@
>  
> +      default: {
> +        /* Now, step through the function contents, writing each of them as we go. */

Wrap the comment to fix 80 columns please.

@@ +1104,5 @@
>  
> +    case eCSSProperty_clip_path:
> +      AppendASCIItoUTF16(nsCSSProps::ValueToKeyword(intValue,
> +                                                    nsCSSProps::kClipShapeSizingKTable),
> +                         aResult);

Wrap the lines here to fit 80 columns (maybe like https://hg.mozilla.org/mozilla-central/file/tip/layout/style/nsCSSValue.cpp#l1188).

@@ +2063,5 @@
>      item = item->mNext;
>      if (!item)
>        break;
>  
>      if (nsCSSProps::PropHasFlags(aProperty,

I am wrestling with whether you should just put CSS_PROPERTY_VALUE_LIST_USES_COMMAS in the clip-path definition in nsCSSPropList.h.  It'd be the only one where the property value as a whole is not a list, but something inside takes a list.  I think you're right.  But can you just add a comma saying while we could but CSS_PROPERTY_VALUE_LIST_USES_COMMAS in the property definition that it doesn't quite make sense because of this reason?

::: layout/style/nsCSSValue.h
@@ +427,5 @@
>     */
>    void AppendToString(nsCSSProperty aProperty, nsAString& aResult,
>                        Serialization aValueSerialization) const;
>  
> +  void AppendPolygonToString(nsCSSProperty aProperty, nsAString& aResult,

Make this function private.

@@ +428,5 @@
>    void AppendToString(nsCSSProperty aProperty, nsAString& aResult,
>                        Serialization aValueSerialization) const;
>  
> +  void AppendPolygonToString(nsCSSProperty aProperty, nsAString& aResult,
> +                      Serialization aValueSerialization) const;

Fix indentation.

::: layout/style/nsComputedDOMStyle.cpp
@@ +5159,5 @@
> +    AppendASCIItoUTF16(nsCSSKeywords::GetStringValue(eCSSKeyword_polygon),
> +                       shapeFunctionString);
> +    shapeFunctionString.Append('(');
> +    uint8_t fillRule = aStyleBasicShape->GetFillRule();
> +    if (fillRule) {

fillRule == NS_STYLE_FILL_RULE_EVENODD

@@ +5168,5 @@
> +      if (i > 0 || fillRule) {
> +        shapeFunctionString.AppendLiteral(", ");
> +      }
> +      SetCssTextToCoord(coordString,
> +                        aStyleBasicShape->Coordinates()[0 + i]);

[i + 0] and [i + 1] later would look better I think.

@@ +5188,5 @@
> +    nsCSSProps::ValueToKeyword(aSizingBox,
> +                               nsCSSProps::kClipShapeSizingKTable),
> +                               boxString);
> +  val->SetString(boxString);
> +  valueList->AppendCSSValue(val);

I don't think we want to unconditionally serialize the <geometry-box> keyword since the "Computed:" line in the property definition in the spec doesn't say anything about that and there is no real default keyword to use in the computed value.  Should the spec also say that the order of the [ <basic-shape> || <geometry-box> ] is as given there in the "Value:" line, or is that implied?

@@ +5198,3 @@
>  nsComputedDOMStyle::DoGetClipPath()
>  {
>    nsROCSSPrimitiveValue* val = new nsROCSSPrimitiveValue;

Don't create a new object if you're not going to use it in the NS_STYLE_CLIP_PATH_SHAPE / NS_STYLE_CLIP_PATH_BOX cases; this will leak.

@@ +5199,5 @@
>  {
>    nsROCSSPrimitiveValue* val = new nsROCSSPrimitiveValue;
>  
>    const nsStyleSVGReset* svg = StyleSVGReset();
> +  if (svg->mClipPath.GetType() == NS_STYLE_CLIP_PATH_SHAPE) {

Switch on svg->mClipPath.GetType() rather than having separate if statements.

@@ +5214,1 @@
>      val->SetIdent(eCSSKeyword_none);

Assert in here that the type is NS_STYLE_CLIP_PATH_NONE.

::: layout/style/nsRuleNode.cpp
@@ +8754,5 @@
>  }
>  
> +// Returns true if the nsStyleBasicShape was successfully set using
> +// the nsCSSValue.
> +bool

The CSS parser should ensure that the nsCSSValue is structurally correct.  Do we really need to be doing further checking in this function and returning a bool?  Please use as many assertions as you like though.

@@ +8755,5 @@
>  
> +// Returns true if the nsStyleBasicShape was successfully set using
> +// the nsCSSValue.
> +bool
> +nsRuleNode::SetStyleClipPathToCSSValue(nsStyleClipPath& aStyleClipPath,

SetStyleFilterToCSSValue uses a pointer as its first argument.  Use a pointer here too?

@@ +8767,5 @@
> +                    "expected a basic shape or reference box");
> +
> +  const nsCSSValueList* cur = aValue->GetListValue();
> +
> +  NS_ABORT_IF_FALSE(cur, "at least one basic shape value must be defined");

I don't think GetListValue() can ever return null if the unit is in fact a list.  We'll already NS_ABORT_IF_FALSE inside nsCSSValue::GetListValue if the unit is wrong.  So I think you can remove both the NS_ABORT_IF_FALSEs here.

@@ +8828,5 @@
> +
> +  if (basicShape) {
> +    aStyleClipPath.SetBasicShape(basicShape, sizingBox);
> +  } else {
> +    aStyleClipPath.SetSizingBox(sizingBox);  

Trailing space.

@@ +8938,5 @@
>               mPresContext, aContext, svgReset->mLightingColor,
>               canStoreInRuleTree);
>    }
>  
> +  // clip-path: url, <basic-shape> || box, none, inherit

"<geometry-box>" instead of "box", since that's what it's called in the spec.

@@ +8963,5 @@
> +    }
> +    case eCSSUnit_List:
> +    case eCSSUnit_ListDep: {
> +      svgReset->mClipPath = nsStyleClipPath();
> +      SetStyleClipPathToCSSValue(svgReset->mClipPath, clipPathValue, aContext,

I think you should move all of the Null/Initial/Unset/Inherit/URL cases into SetStyleClipPathToCSSValue too.

::: layout/style/nsStyleStruct.cpp
@@ +1037,5 @@
> +  : mType(NS_STYLE_CLIP_PATH_NONE)
> +  , mURL(nullptr)
> +  , mSizingBox(NS_STYLE_CLIP_SHAPE_SIZING_NOBOX)
> +{
> +  MOZ_COUNT_CTOR(nsStyleClipPath);

No need to count construction/destruction of these objects since we only ever use it compositionally as part of nsStyleSVGReset.

Actually can you make operator new private on this class so that we can't create new nsStyleClipPaths on the heap?

@@ +1076,5 @@
> +  } else if (aOther.mType == NS_STYLE_CLIP_PATH_BOX) {
> +    SetSizingBox(aOther.mSizingBox);
> +  } else {
> +    mURL = nullptr;
> +    mSizingBox = NS_STYLE_CLIP_SHAPE_SIZING_NOBOX;

This will leak if this object is currently set to a URL or shape/box.

I think you should have a SetNone() method that does these two assignments after doing a ReleaseRef().  Then you can call SetNone() rather than assigning a default constructed nsStyleClipPath from within your nsRuleNode method.

@@ +1110,5 @@
> +    mBasicShape->Release();
> +  } else if (mType == NS_STYLE_CLIP_PATH_URL) {
> +    NS_ASSERTION(mURL, "expected pointer");
> +    mURL->Release();
> +  }

I think ReleaseRef should null out the union pointer value.

::: layout/style/nsStyleStruct.h
@@ +2826,5 @@
>      return mFill.mType != eStyleSVGPaintType_None && mFillOpacity > 0;
>    }
>  };
>  
> +class nsStyleBasicShape MOZ_FINAL {

Are you planning to extend nsStyleBasicShape with new data members that cover all of the shapes?  I'm not sure how I feel about having say circle() values still take up memory for the fill rule and coordinate array.  But let's leave it like this for now and I reserve the right to make you split it up into separate classes later on. :-)

::: layout/svg/nsSVGEffects.cpp
@@ +555,5 @@
> +    result.mClipPath =
> +      GetPaintingProperty(style->mClipPath.GetURL(), aFrame, ClipPathProperty());
> +  } else {
> +    result.mClipPath = nullptr;
> +  }

Add an XXX comment in here to say we need to handle the new clip-path types.

::: layout/svg/nsSVGIntegrationUtils.cpp
@@ +152,5 @@
>    // checking the SDL prefs here, since we don't know if we're being called for
>    // painting or hit-testing anyway.
>    const nsStyleSVGReset *style = aFrame->StyleSVGReset();
> +  return (style->HasFilters() ||
> +          style->mClipPath.GetType() == NS_STYLE_CLIP_PATH_URL || style->mMask);

Just make it != NS_STYLE_CLIP_NONE as that'll work for the new types once we support them.

Comment 3

[Dirk Schulze]

Attachment: clip-path-polygon.patch

Comment 4

[Dirk Schulze]

(In reply to Cameron McCormack (:heycam) (away October) from comment #2)
> Comment on attachment 8495487 [details] [diff] [review]
> clip-path-polygon.patch
> 
> Review of attachment 8495487 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good overall, but I'd like to see another version with these
> comments addressed.
> 
> ::: layout/style/nsCSSParser.cpp
> @@ +959,5 @@
> >    bool IsParsingCompoundProperty(void) const {
> >      return mParsingCompoundProperty;
> >    }
> >  
> > +  /* Functions for basic shapes **/
> 
> Drop one of the two trailing asterisks.

Fixed.

> 
> @@ +13738,5 @@
> >    AppendValue(eCSSProperty_transform, value);
> >    return true;
> >  }
> >  
> > +/* Reads a function list of arguments of polygon.
> 
> "Reads a polygon function's argument list."

Changed.

> 
> @@ +13751,5 @@
> > +    numArgs++;
> > +
> > +    // The fill-rule must be comma separated from the polygon points.
> > +    if (!ExpectSymbol(',', true)) {
> > +      return false;
> 
> Please report a CSS parse error here:
> REPORT_UNEXPECTED_TOKEN(PEExpectedComma).
> 
> Also, whenever we encounter a parse error, we need to make sure that we skip
> over any balanced content before returning false, if we know that we have
> already parsed an "opening" token.  So here, since we know that we have
> parsed the FUNCTION token, we should call |SkipUntil(')')| so that we eat up
> any balanced content, including the closing paren.

Thanks, I added the SkipUntils and the reports.

> 
> @@ +13761,5 @@
> > +  for (;;) {
> > +    nsCSSValue xValue, yValue;
> > +    if (!ParseVariant(xValue, VARIANT_LPCALC, nullptr) ||
> > +        !ParseVariant(yValue, VARIANT_LPCALC, nullptr)) {
> > +      return false;
> 
> Needs error reporting and a SkipUntil call before returning false.

Done.

> 
> @@ +13769,5 @@
> > +
> > +    // See whether to continue or whether to look for end of function.
> > +    if (!ExpectSymbol(',', true)) {
> > +      // We need to read the closing parenthesis.
> > +      if (!ExpectSymbol(')', true)) {
> 
> Needs error reporting and a SkipUntil call before returning false.

Done.

> 
> @@ +13782,5 @@
> > +  nsRefPtr<nsCSSValue::Array> functionArray =
> > +    aValue.InitFunction(eCSSKeyword_polygon, numArgs);
> > +  functionArray->Item(numArgs) = coordinates;
> > +  if (numArgs > 1) {
> > +    functionArray->Item(1) = fillRuleValue;    
> 
> Remove trailing white space.

Removed.

> 
> @@ +13801,5 @@
> > +    return false;
> > +  }
> > +
> > +  nsCSSKeyword keyword = nsCSSKeywords::LookupKeyword(mToken.mIdent);
> > +  switch(keyword) {
> 
> Space before "(".

Done.

> 
> @@ +13803,5 @@
> > +
> > +  nsCSSKeyword keyword = nsCSSKeywords::LookupKeyword(mToken.mIdent);
> > +  switch(keyword) {
> > +  case eCSSKeyword_polygon:
> > +    return ParsePolygonFunction(aValue);
> 
> (It's a slight shame that we can't use ParseFunction for this, but it
> doesn't handle keyword values or pair values.)

Right, it is focused on comma separated values of a kind.

> 
> @@ +13823,5 @@
> > +    }
> > +
> > +    bool shape = false, box = false;
> > +    nsCSSValueList* cur = value.SetListValue();
> > +    for (unsigned i = 0; i < 2; ++i) {
> 
> Just use "int" instead of "unsigned".

Changed.

> 
> @@ +13826,5 @@
> > +    nsCSSValueList* cur = value.SetListValue();
> > +    for (unsigned i = 0; i < 2; ++i) {
> > +      if (ParseBasicShape(cur->mValue) && !shape) {
> > +        shape = true;
> > +      } else if (ParseEnum(cur->mValue, nsCSSProps::kClipShapeSizingKTable)) {
> 
> Should we have "&& !box" at the end of the condition?

Oh indeed.

> 
> @@ +13829,5 @@
> > +        shape = true;
> > +      } else if (ParseEnum(cur->mValue, nsCSSProps::kClipShapeSizingKTable)) {
> > +        box = true;
> > +      } else {
> > +        return false;
> 
> Shouldn't this be a break rather than a return false?

No, parsing fails if it ever gets into else. A return is correct, but added the report.

> 
> Also we need some error reporting in this function.

> 
> @@ +13831,5 @@
> > +        box = true;
> > +      } else {
> > +        return false;
> > +      }
> > +      if (!GetToken(true)) {
> 
> We don't need to check for EOF here for correct parsing -- we should just go
> around the loop again and get false returned from ParseBasicShape and/or
> ParseEnum -- but it might be good to keep this here and set a boolean to
> indicate whether we encountered EOF before breaking.  That would let us do a
> REPORT_UNEXPECTED_EOF in the |if (!shape && !box)| statement below.


Done this in combination with the next one.

> 
> @@ +13839,5 @@
> > +      cur->mNext = new nsCSSValueList;
> > +      cur = cur->mNext;
> > +    }
> > +    if (!shape && !box) {
> > +      return false;
> 
> As well as the REPORT_UNEXPECTED_EOF mentioned above, we should have a
> REPORT_UNEXPECTED_TOKEN in here for when we encountered a token and neither
> of the shape and box parsed.

Fixed.

> 
> ::: layout/style/nsCSSProps.cpp
> @@ +1886,5 @@
> > +  eCSSKeyword_border_box,  NS_STYLE_CLIP_SHAPE_SIZING_BORDER,
> > +  eCSSKeyword_margin_box,  NS_STYLE_CLIP_SHAPE_SIZING_MARGIN,
> > +  eCSSKeyword_fill_box,  NS_STYLE_CLIP_SHAPE_SIZING_FILL,
> > +  eCSSKeyword_stroke_box,  NS_STYLE_CLIP_SHAPE_SIZING_STROKE,
> > +  eCSSKeyword_view_box,  NS_STYLE_CLIP_SHAPE_SIZING_VIEW,
> 
> Either use a single space between the values on each line or line them up
> neatly.  (There is a mix of lining up and not lining up in this file, so
> choose whichever you prefer.)

Lining each up looked better. I chose that one.


> 
> ::: layout/style/nsCSSProps.h
> @@ +536,5 @@
> >    static const KTableValue kBoxOrientKTable[];
> >    static const KTableValue kBoxPackKTable[];
> >    static const KTableValue kDominantBaselineKTable[];
> >    static const KTableValue kFillRuleKTable[];
> > +  static const KTableValue kClipShapeSizingKTable[];
> 
> Keep this list ordered.

Fixed.

> 
> ::: layout/style/nsCSSValue.cpp
> @@ +829,5 @@
> >  void
> > +nsCSSValue::AppendPolygonToString(nsCSSProperty aProperty, nsAString& aResult,
> > +                                  Serialization aSerialization) const
> > +{
> > +    const nsCSSValue::Array* array = GetArrayValue();
> 
> Too much indentation.

Changed for all lines.

> 
> @@ +834,5 @@
> > +    NS_ABORT_IF_FALSE(array->Count() > 1 && array->Count() <= 3,
> > +                      "Polygons must have name and at least one more value.");
> > +    size_t index = 1;
> > +    if (array->Count() == 3) {
> > +      const nsCSSValue& fillRuleValue = array->Item(index);
> 
> Add a comment somewhere in here to say that if the array has 2 elements,
> then the item at index 1 is the coordinate pair list, and if it has 3
> elements, then the item at index 1 is the fill rule and index 2 is the
> coordinate pair list.

Done.

> 
> @@ +836,5 @@
> > +    size_t index = 1;
> > +    if (array->Count() == 3) {
> > +      const nsCSSValue& fillRuleValue = array->Item(index);
> > +      NS_ABORT_IF_FALSE(fillRuleValue.GetUnit() == eCSSUnit_Enumerated,
> > +                      "Expected polygon fill rule.");
> 
> Line up the argument after the paren on the previous line.

Done.

> 
> @@ -943,5 @@
> > -          break;
> > -      }
> > -      nsStyleUtil::AppendEscapedCSSIdent(ident, aResult);
> > -    } else {
> > -      MOZ_ASSERT(false, "should no longer have non-enumerated functions");
> 
> Yeah I guess assuming this now is safe.

Right, also, with the NS_ABORT_IF_FALSE, we do not get any further. If this is ever reached, someone made a serious mistake in the usage of the function type. This seemed to ensure that we won't run into it.

> 
> @@ +976,3 @@
> >  
> > +      default: {
> > +        /* Now, step through the function contents, writing each of them as we go. */
> 
> Wrap the comment to fix 80 columns please.

Done.

> 
> @@ +1104,5 @@
> >  
> > +    case eCSSProperty_clip_path:
> > +      AppendASCIItoUTF16(nsCSSProps::ValueToKeyword(intValue,
> > +                                                    nsCSSProps::kClipShapeSizingKTable),
> > +                         aResult);
> 
> Wrap the lines here to fit 80 columns (maybe like
> https://hg.mozilla.org/mozilla-central/file/tip/layout/style/nsCSSValue.
> cpp#l1188).

Done.

> 
> @@ +2063,5 @@
> >      item = item->mNext;
> >      if (!item)
> >        break;
> >  
> >      if (nsCSSProps::PropHasFlags(aProperty,
> 
> I am wrestling with whether you should just put
> CSS_PROPERTY_VALUE_LIST_USES_COMMAS in the clip-path definition in
> nsCSSPropList.h.  It'd be the only one where the property value as a whole
> is not a list, but something inside takes a list.  I think you're right. 
> But can you just add a comma saying while we could but
> CSS_PROPERTY_VALUE_LIST_USES_COMMAS in the property definition that it
> doesn't quite make sense because of this reason?

I know, I wasn't happy either and tried using CSS_PROPERTY_VALUE_LIST_USES_COMMAS. The issue is, as you said, we have comma separated lists and space separated lists nested in each other. This isn't considered by CSS_PROPERTY_VALUE_LIST_USES_COMMAS.

> 
> ::: layout/style/nsCSSValue.h
> @@ +427,5 @@
> >     */
> >    void AppendToString(nsCSSProperty aProperty, nsAString& aResult,
> >                        Serialization aValueSerialization) const;
> >  
> > +  void AppendPolygonToString(nsCSSProperty aProperty, nsAString& aResult,
> 
> Make this function private.

Done.

> 
> @@ +428,5 @@
> >    void AppendToString(nsCSSProperty aProperty, nsAString& aResult,
> >                        Serialization aValueSerialization) const;
> >  
> > +  void AppendPolygonToString(nsCSSProperty aProperty, nsAString& aResult,
> > +                      Serialization aValueSerialization) const;
> 
> Fix indentation.

Fixed.

> 
> ::: layout/style/nsComputedDOMStyle.cpp
> @@ +5159,5 @@
> > +    AppendASCIItoUTF16(nsCSSKeywords::GetStringValue(eCSSKeyword_polygon),
> > +                       shapeFunctionString);
> > +    shapeFunctionString.Append('(');
> > +    uint8_t fillRule = aStyleBasicShape->GetFillRule();
> > +    if (fillRule) {
> 
> fillRule == NS_STYLE_FILL_RULE_EVENODD

Changed.

> 
> @@ +5168,5 @@
> > +      if (i > 0 || fillRule) {
> > +        shapeFunctionString.AppendLiteral(", ");
> > +      }
> > +      SetCssTextToCoord(coordString,
> > +                        aStyleBasicShape->Coordinates()[0 + i]);
> 
> [i + 0] and [i + 1] later would look better I think.

I used [i] and [i + 1]. [i + 0] looks weird, not sure why I didn't see that initially.

> 
> @@ +5188,5 @@
> > +    nsCSSProps::ValueToKeyword(aSizingBox,
> > +                               nsCSSProps::kClipShapeSizingKTable),
> > +                               boxString);
> > +  val->SetString(boxString);
> > +  valueList->AppendCSSValue(val);
> 
> I don't think we want to unconditionally serialize the <geometry-box>
> keyword since the "Computed:" line in the property definition in the spec
> doesn't say anything about that and there is no real default keyword to use
> in the computed value.  Should the spec also say that the order of the [
> <basic-shape> || <geometry-box> ] is as given there in the "Value:" line, or
> is that implied?

At the moment it is implied by the Value: line. This is how we handle it in other places like CSS Background and Borders as well. IIRC CSSOM shall make it more clear in the future.

> 
> @@ +5198,3 @@
> >  nsComputedDOMStyle::DoGetClipPath()
> >  {
> >    nsROCSSPrimitiveValue* val = new nsROCSSPrimitiveValue;
> 
> Don't create a new object if you're not going to use it in the
> NS_STYLE_CLIP_PATH_SHAPE / NS_STYLE_CLIP_PATH_BOX cases; this will leak.

Thanks, an oversight. Fixed together with the switch.

> 
> @@ +5199,5 @@
> >  {
> >    nsROCSSPrimitiveValue* val = new nsROCSSPrimitiveValue;
> >  
> >    const nsStyleSVGReset* svg = StyleSVGReset();
> > +  if (svg->mClipPath.GetType() == NS_STYLE_CLIP_PATH_SHAPE) {
> 
> Switch on svg->mClipPath.GetType() rather than having separate if statements.

Fixed.

> 
> @@ +5214,1 @@
> >      val->SetIdent(eCSSKeyword_none);
> 
> Assert in here that the type is NS_STYLE_CLIP_PATH_NONE.

Added an NOT_REACHED assert.

> 
> ::: layout/style/nsRuleNode.cpp
> @@ +8754,5 @@
> >  }
> >  
> > +// Returns true if the nsStyleBasicShape was successfully set using
> > +// the nsCSSValue.
> > +bool
> 
> The CSS parser should ensure that the nsCSSValue is structurally correct. 
> Do we really need to be doing further checking in this function and
> returning a bool?  Please use as many assertions as you like though.

I don't actually check the boolean since we just don't set nsStyleClipPath in the case of a return false;. I made it a void method and added assertions instead.

> 
> @@ +8755,5 @@
> >  
> > +// Returns true if the nsStyleBasicShape was successfully set using
> > +// the nsCSSValue.
> > +bool
> > +nsRuleNode::SetStyleClipPathToCSSValue(nsStyleClipPath& aStyleClipPath,
> 
> SetStyleFilterToCSSValue uses a pointer as its first argument.  Use a
> pointer here too?


I changed it to a pointer for consistency.

> 
> @@ +8767,5 @@
> > +                    "expected a basic shape or reference box");
> > +
> > +  const nsCSSValueList* cur = aValue->GetListValue();
> > +
> > +  NS_ABORT_IF_FALSE(cur, "at least one basic shape value must be defined");
> 
> I don't think GetListValue() can ever return null if the unit is in fact a
> list.  We'll already NS_ABORT_IF_FALSE inside nsCSSValue::GetListValue if
> the unit is wrong.  So I think you can remove both the NS_ABORT_IF_FALSEs
> here.

Removed.

> 
> @@ +8828,5 @@
> > +
> > +  if (basicShape) {
> > +    aStyleClipPath.SetBasicShape(basicShape, sizingBox);
> > +  } else {
> > +    aStyleClipPath.SetSizingBox(sizingBox);  
> 
> Trailing space.

Fixed.

> 
> @@ +8938,5 @@
> >               mPresContext, aContext, svgReset->mLightingColor,
> >               canStoreInRuleTree);
> >    }
> >  
> > +  // clip-path: url, <basic-shape> || box, none, inherit
> 
> "<geometry-box>" instead of "box", since that's what it's called in the spec.

Changed.

> 
> @@ +8963,5 @@
> > +    }
> > +    case eCSSUnit_List:
> > +    case eCSSUnit_ListDep: {
> > +      svgReset->mClipPath = nsStyleClipPath();
> > +      SetStyleClipPathToCSSValue(svgReset->mClipPath, clipPathValue, aContext,
> 
> I think you should move all of the Null/Initial/Unset/Inherit/URL cases into
> SetStyleClipPathToCSSValue too.

As discussed on IRC, this doesn't seem to match the more general pattern in this file. I kept the switch.

> 
> ::: layout/style/nsStyleStruct.cpp
> @@ +1037,5 @@
> > +  : mType(NS_STYLE_CLIP_PATH_NONE)
> > +  , mURL(nullptr)
> > +  , mSizingBox(NS_STYLE_CLIP_SHAPE_SIZING_NOBOX)
> > +{
> > +  MOZ_COUNT_CTOR(nsStyleClipPath);
> 
> No need to count construction/destruction of these objects since we only
> ever use it compositionally as part of nsStyleSVGReset.

Ok, removed the counters.

> 
> Actually can you make operator new private on this class so that we can't
> create new nsStyleClipPaths on the heap?

Added it to private with MOZ_DELETE.

> 
> @@ +1076,5 @@
> > +  } else if (aOther.mType == NS_STYLE_CLIP_PATH_BOX) {
> > +    SetSizingBox(aOther.mSizingBox);
> > +  } else {
> > +    mURL = nullptr;
> > +    mSizingBox = NS_STYLE_CLIP_SHAPE_SIZING_NOBOX;
> 
> This will leak if this object is currently set to a URL or shape/box.

Yes! This actually caused a crash on the try bots.

> 
> I think you should have a SetNone() method that does these two assignments
> after doing a ReleaseRef().  Then you can call SetNone() rather than
> assigning a default constructed nsStyleClipPath from within your nsRuleNode
> method.

I call ReleaseRef() right at the beginning now. In the else case, I set the type to none specifically. Adding SetNone just for the = operator seemed to be overhead. I hope this is fine.


> 
> @@ +1110,5 @@
> > +    mBasicShape->Release();
> > +  } else if (mType == NS_STYLE_CLIP_PATH_URL) {
> > +    NS_ASSERTION(mURL, "expected pointer");
> > +    mURL->Release();
> > +  }
> 
> I think ReleaseRef should null out the union pointer value.

Done.

> 
> ::: layout/style/nsStyleStruct.h
> @@ +2826,5 @@
> >      return mFill.mType != eStyleSVGPaintType_None && mFillOpacity > 0;
> >    }
> >  };
> >  
> > +class nsStyleBasicShape MOZ_FINAL {
> 
> Are you planning to extend nsStyleBasicShape with new data members that
> cover all of the shapes?  I'm not sure how I feel about having say circle()
> values still take up memory for the fill rule and coordinate array.  But
> let's leave it like this for now and I reserve the right to make you split
> it up into separate classes later on. :-)

There are member variables that can be shared. Others can't. I probably will add structs, not sure yet. I'll refactor the code with the inset() patch.

> 
> ::: layout/svg/nsSVGEffects.cpp
> @@ +555,5 @@
> > +    result.mClipPath =
> > +      GetPaintingProperty(style->mClipPath.GetURL(), aFrame, ClipPathProperty());
> > +  } else {
> > +    result.mClipPath = nullptr;
> > +  }
> 
> Add an XXX comment in here to say we need to handle the new clip-path types.

I think nsSVGEffects just handles URL references and no other values. Looking at Filters, this seems to be the pattern there. I did not add the comment.

> 
> ::: layout/svg/nsSVGIntegrationUtils.cpp
> @@ +152,5 @@
> >    // checking the SDL prefs here, since we don't know if we're being called for
> >    // painting or hit-testing anyway.
> >    const nsStyleSVGReset *style = aFrame->StyleSVGReset();
> > +  return (style->HasFilters() ||
> > +          style->mClipPath.GetType() == NS_STYLE_CLIP_PATH_URL || style->mMask);
> 
> Just make it != NS_STYLE_CLIP_NONE as that'll work for the new types once we
> support them.

Done.

Thanks a lot for your review!

Comment 5

[Cameron McCormack (:heycam)]

Dirk, I think you uploaded the old patch again?

Comment 6

[Dirk Schulze]

Attachment: clip-path-polygon.patch

Comment 7

[Dirk Schulze]

Here are the mochi-test results https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d50bcf0d12d7

Comment 8

[Cameron McCormack (:heycam)]

(In reply to Dirk Schulze from comment #4)
...
> > @@ +13829,5 @@
> > > +        shape = true;
> > > +      } else if (ParseEnum(cur->mValue, nsCSSProps::kClipShapeSizingKTable)) {
> > > +        box = true;
> > > +      } else {
> > > +        return false;
> > 
> > Shouldn't this be a break rather than a return false?
> 
> No, parsing fails if it ever gets into else. A return is correct, but added
> the report.

Hmm, I think this is wrong; see my next review comments.

> > @@ -943,5 @@
> > > -          break;
> > > -      }
> > > -      nsStyleUtil::AppendEscapedCSSIdent(ident, aResult);
> > > -    } else {
> > > -      MOZ_ASSERT(false, "should no longer have non-enumerated functions");
> > 
> > Yeah I guess assuming this now is safe.
> 
> Right, also, with the NS_ABORT_IF_FALSE, we do not get any further. If this
> is ever reached, someone made a serious mistake in the usage of the function
> type. This seemed to ensure that we won't run into it.

Note that NS_ABORT_IF_FALSE only aborts in debug builds though.  So whereas before, we had a safe path even in non-debug builds that would handle this unexpected case, we now don't.  But that's fine; we've had that MOZ_ASSERT there for a year and I don't think we have any function represented by anything other than enumerations now.

> > @@ +5188,5 @@
> > > +    nsCSSProps::ValueToKeyword(aSizingBox,
> > > +                               nsCSSProps::kClipShapeSizingKTable),
> > > +                               boxString);
> > > +  val->SetString(boxString);
> > > +  valueList->AppendCSSValue(val);
> > 
> > I don't think we want to unconditionally serialize the <geometry-box>
> > keyword since the "Computed:" line in the property definition in the spec
> > doesn't say anything about that and there is no real default keyword to use
> > in the computed value.  Should the spec also say that the order of the [
> > <basic-shape> || <geometry-box> ] is as given there in the "Value:" line, or
> > is that implied?
> 
> At the moment it is implied by the Value: line. This is how we handle it in
> other places like CSS Background and Borders as well. IIRC CSSOM shall make
> it more clear in the future.

OK, but what computed value gets serialized for a specified value of "clip-path: polygon(...)" with no box keyword?

> > ::: layout/svg/nsSVGEffects.cpp
> > @@ +555,5 @@
> > > +    result.mClipPath =
> > > +      GetPaintingProperty(style->mClipPath.GetURL(), aFrame, ClipPathProperty());
> > > +  } else {
> > > +    result.mClipPath = nullptr;
> > > +  }
> > 
> > Add an XXX comment in here to say we need to handle the new clip-path types.
> 
> I think nsSVGEffects just handles URL references and no other values.
> Looking at Filters, this seems to be the pattern there. I did not add the
> comment.

OK, looking into this I think you're right; the effects properties are only used to help with invalidation of url() references.

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8496404 [details] [diff] [review]
clip-path-polygon.patch

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

::: dom/locales/en-US/chrome/layout/css.properties
@@ +55,5 @@
>  PECounterExtendsNotIdent=Expected identifier for extends system but found '%1$S'.
>  PECounterASWeight=Each weight in the additive-symbols descriptor must be smaller than the previous weight.
>  PEClassSelEOF=class name
>  PEClassSelNotIdent=Expected identifier for class selector but found '%1$S'.
> +PECoordintatePair=Expected coordinate pair but found '%1$S'.

PECoordinatePair

@@ +107,5 @@
>  PEColorOpacityEOF=opacity in color value
>  PEExpectedNumber=Expected a number but found '%1$S'.
>  PEExpectedCloseParen=Expected ')' but found '%1$S'.
> +PEExpectedBasicShapeOrBox=Expected <basic-shape> or reference box but found '%1$S'.
> +PEFilterEOF=clip-path

PEClipPathEOF

When used with REPORT_UNEXPECTED_EOF, these strings get inserted at the of a sentence that starts "Unexpected end of file while searching for ".  I think the text should be something like "<basic-shape> or reference box".

::: layout/style/nsCSSParser.cpp
@@ +13763,5 @@
> +  for (;;) {
> +    nsCSSValue xValue, yValue;
> +    if (!ParseVariant(xValue, VARIANT_LPCALC, nullptr) ||
> +        !ParseVariant(yValue, VARIANT_LPCALC, nullptr)) {
> +      REPORT_UNEXPECTED_TOKEN(PECoordintatePair);

PECoordinatePair

@@ +13836,5 @@
> +      } else if (ParseEnum(cur->mValue, nsCSSProps::kClipShapeSizingKTable) &&
> +                 !box) {
> +        box = true;
> +      } else {
> +        REPORT_UNEXPECTED_TOKEN(PEExpectedBasicShapeOrBox);

I think the message won't be right if we failed one of the earlier if statement branches because we had already parsed a shape or box.

But per my previous comment, we can't just return false here, since it's not always going to be a parse error if we get in here.  For example, if you have:

  <style>p { clip-path: fill-box; }</style>

then the second time around the loop, we will have got a ';' token.

So I think we need to do something like this (putting the boolean checks at the start of the conditions means we won't unnecessarily try parsing the same thing twice):

  bool eof = false;
  for (int i = 0; ...)
    if (!shape && ParseBasicShape(...)) {
      shape = true;
    } else if (!box && ParseEnum(...)) {
      box = true;
    } else {
      break;
    }
    if (!GetToken(true)) {
      eof = true;
      break;
    }
    ...
  }

and then you can either call REPORT_UNEXPECTED_TOKEN or REPORT_UNEXPECTED_EOF depending on whether eof == true.  And only do it if shape and box are both false.

For things like

  <style>p { clip-path: fill-box junk; }</style>

functions higher up the call stack will report an error on finding junk saying it was looking for a semicolon.

::: layout/style/nsComputedDOMStyle.cpp
@@ +5200,5 @@
>    const nsStyleSVGReset* svg = StyleSVGReset();
> +  switch (svg->mClipPath.GetType()) {
> +    case NS_STYLE_CLIP_PATH_SHAPE:
> +     return CreatePrimitiveValueForClipPath(svg->mClipPath.GetBasicShape(),
> +                                             svg->mClipPath.GetSizingBox());

Fix indentation.

::: layout/style/nsRuleNode.cpp
@@ +8753,5 @@
>    COMPUTE_END_INHERITED(SVG, svg)
>  }
>  
> +// Returns true if the nsStyleBasicShape was successfully set using
> +// the nsCSSValue.

Remove the comment.

@@ +8808,5 @@
> +          NS_ABORT_IF_FALSE(didSetCoordY, "unexpected y coordinate unit");
> +          curPair = curPair->mNext;
> +        }
> +      } else {
> +        // XXX Handle more basi shape functions later.

basic

Comment 10

[Dirk Schulze]

Attachment: clip-path-polygon.patch

Comment 11

[Dirk Schulze]

Fixed comments.(In reply to Cameron McCormack (:heycam) (away October) from comment #8)
> (In reply to Dirk Schulze from comment #4)
> ...
> > > @@ +13829,5 @@
> > > > +        shape = true;
> > > > +      } else if (ParseEnum(cur->mValue, nsCSSProps::kClipShapeSizingKTable)) {
> > > > +        box = true;
> > > > +      } else {
> > > > +        return false;
> > > 
> > > Shouldn't this be a break rather than a return false?
> > 
> > No, parsing fails if it ever gets into else. A return is correct, but added
> > the report.
> 
> Hmm, I think this is wrong; see my next review comments.
> 
> > > @@ -943,5 @@
> > > > -          break;
> > > > -      }
> > > > -      nsStyleUtil::AppendEscapedCSSIdent(ident, aResult);
> > > > -    } else {
> > > > -      MOZ_ASSERT(false, "should no longer have non-enumerated functions");
> > > 
> > > Yeah I guess assuming this now is safe.
> > 
> > Right, also, with the NS_ABORT_IF_FALSE, we do not get any further. If this
> > is ever reached, someone made a serious mistake in the usage of the function
> > type. This seemed to ensure that we won't run into it.
> 
> Note that NS_ABORT_IF_FALSE only aborts in debug builds though.  So whereas
> before, we had a safe path even in non-debug builds that would handle this
> unexpected case, we now don't.  But that's fine; we've had that MOZ_ASSERT
> there for a year and I don't think we have any function represented by
> anything other than enumerations now.
> 
> > > @@ +5188,5 @@
> > > > +    nsCSSProps::ValueToKeyword(aSizingBox,
> > > > +                               nsCSSProps::kClipShapeSizingKTable),
> > > > +                               boxString);
> > > > +  val->SetString(boxString);
> > > > +  valueList->AppendCSSValue(val);
> > > 
> > > I don't think we want to unconditionally serialize the <geometry-box>
> > > keyword since the "Computed:" line in the property definition in the spec
> > > doesn't say anything about that and there is no real default keyword to use
> > > in the computed value.  Should the spec also say that the order of the [
> > > <basic-shape> || <geometry-box> ] is as given there in the "Value:" line, or
> > > is that implied?
> > 
> > At the moment it is implied by the Value: line. This is how we handle it in
> > other places like CSS Background and Borders as well. IIRC CSSOM shall make
> > it more clear in the future.
> 
> OK, but what computed value gets serialized for a specified value of
> "clip-path: polygon(...)" with no box keyword?
> 

Ah, gotcha. When no box is specified, the uint8 is NS_STYLE_CLIP_SHAPE_SIZING_NOBOX which doesn't have an enumeration. Apparently, in this case Gecko adds the empty string, which gives the correct result. I didn't see any assertion, but to be sure, I added a check for NS_STYLE_CLIP_SHAPE_SIZING_NOBOX and return early.

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8496526 [details] [diff] [review]
clip-path-polygon.patch

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

Looks good, thanks!

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/99b68a13246b

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/99b68a13246b

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The parsing code that landed here has significant problems; see my comments in bug 1077388.

Comment 16

[Karl Tomlinson (:karlt)]

Adjusting summary to align with what was fixed here.