1074528 - Implement inset shape function for clip-path

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[Dirk Schulze]

Basic shapes make use of some primitives introduced in other specs.

circle() and ellipse() use <position> as specified in CSS Background and Borders. With ParsePositionValue() it was possible to reuse the parsing code from background-position.

inset() makes use of the syntax of the 'border-radius' shorthand. Unlike ParsePositionValue(), ParseBoxCornerRadii() sets the nsCSSValues for the individual longhand properties of border-radius directly. This way I can not reuse the code.

What is the best way forward here? Rewrite the radius parsing code just for inset()? Refactor the existing code to take nsCSSValue*s and don't set the long hand properties directly? Are there and implications in performance with such a refactoring?

Comment 2

[Dirk Schulze]

Attachment: clip-path-inset-moz.patch

The patch requires ellipse/circle patch to land first and is not meant for landing. I would like to get some early feedback to the refactoring of ParseBoxCornerRadii to ParseBoxCornerRadiiInternals and taking an Array* as argument.

Also, I use an bitfield in nsStyleBasicShape now. IIRC GCC is able to put enumerations and integer values into one bitfield. Don't know what happens on Windows. I could turn mType to an int32_t type as well if it should be necessary but it wouldn't be great though.

Furthermore, I moved AppendSidesShorthandToString from Declaration to nsCSSValue as a static member function. nsStyleUtils might be the right place, but it would require to include nsCSSValue.h which seems to be a bigger tradeoff. So nsCSSValue seemed to be the better choice.

I would like to do the real nsComputedDOMStyle string creation for border-radius in a different patch. Right now it returns the right value but doesn't optimize it by reducing the length of the string where possible. Instead it returns 8 length/percentage values all the time, separated with a '/'.

Comment 3

[Dirk Schulze]

I am working on a patch for review based on the feedback to circle/ellipse parsing.

Comment 4

[Dirk Schulze]

Attachment: clip-path-inset.patch

Instead of the bit field I use a union. I also followed the advices from bug 1074522.

Comment 5

[Dirk Schulze]

Attachment: clip-path-inset.patch

Initialize nsStyleCorners if no border radius was specified.

Comment 6

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

Spec links:

http://dev.w3.org/fxtf/css-masking-1/#the-clip-path
http://dev.w3.org/csswg/css-shapes/#basic-shape-functions

Comment 7

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

Comment on attachment 8506199 [details] [diff] [review]
clip-path-inset.patch

Looks good, but I'd like to have a look at the revised patch with these comments addressed, so review-:


It might have been nice to structure this as a series of patches.
For example, moving AppendSidesShorthandToString to nsCSSValue could
have been a separate patch.  (Don't change it now, though.)

Declaration.cpp:
>+      nsCSSValue::AppendSidesShorthandToString(subprops, vals, aValue, aSerialization);
...
>+      nsCSSValue::AppendSidesShorthandToString(subprops, xVals, aValue, aSerialization);
...
>+        nsCSSValue::AppendSidesShorthandToString(subprops, yVals, aValue, aSerialization);

Wrap before 80th column.

nsCSSParser.cpp:

Could you make ParseBoxCornerRadiiInternal take an
nsCSSValue[] instead of an nsCSSValue::Array*, so that the main
caller doesn't have to heap-allocate an array?

(Probably best to add an ItemStorage() accessor to nsCSSValue::Array
so it's clear that some callers depend on the items being contiguous.)


(Even without that change, ParseBoxCornerRadii would have no need for
|value| or for the ".get()".)

>+    if (ParseVariant(functionArray->Item(2), VARIANT_LPCALC, nullptr) &&
>+        ParseVariant(functionArray->Item(3), VARIANT_LPCALC, nullptr) &&
>+        ParseVariant(functionArray->Item(4), VARIANT_LPCALC, nullptr)) {
>+    }

I'd just put this as a statement, but add a comment:

   // Consume up to 4, but only require one.
   ParseVariant(functionArray->Item(2), VARIANT_LPCALC, nullptr) &&
   ParseVariant(functionArray->Item(3), VARIANT_LPCALC, nullptr) &&
   ParseVariant(functionArray->Item(4), VARIANT_LPCALC, nullptr);

(I assume you can do that, though it's rare in C++, I admit.)

+  if (!ExpectSymbol(')', true)) {
+    if (!GetToken(true)) {
+      REPORT_UNEXPECTED_EOF(PEBorderRadiusEOF);

Instead of this REPORT_UNEXPECTED_EOF, how about:

      NS_NOTREACHED("ExpectSymbol should have returned true");

And delete PEBorderRadiusEOF too.

>+        !ParseBoxCornerRadiiInternals(radiusArray.get())) {

No .get() needed.


nsCSSValue.cpp:

>+nsCSSValue::AppendSidesShorthandToString(const nsCSSProperty aProperties[],
>+                             const nsCSSValue* aValues[],
>+                             nsAString& aString,
>+                             nsCSSValue::Serialization aSerialization)

Fix indentation of the later lines in this quote to line up.


nsCSSValue::AppendBasicShapeRadiusToString could perhaps share a little
more code with the border_radius case in Declaration::GetValue.  (If it
does, perhaps what isn't shared could move into AppendInsetToString.)

AppendInsetToString could call AppendSidesShorthandToString; it just
needs to construct a dummy array of the same property repeated 4 times,
and then use the array storage getter I mentioned adding above.

nsComputedDOMStyle.cpp:

Please spell BashicShapeRadiiToString correctly, i.e., as
BasicShapeRadiiToString.

>+  for (uint8_t i = 0; i < 8; i += 2) {
>+    horizontal.AppendElement(aCorners.Get(i));
>+    vertical.AppendElement(aCorners.Get(i + 1));
>+  }

Please write this as:

  NS_FOR_CSS_FULL_CORNERS(corner) {
    horizontal.AppendElement(
      aCorners.Get(NS_FULL_TO_HALF_CORNER(corner, false));
    vertical.AppendElement(
      aCorners.Get(NS_FULL_TO_HALF_CORNER(corner, true));
  }

>+  if (horizontal == vertical) {

Cheaper to compare horizontalString and verticalString.

nsRuleNode.cpp:

>+        int32_t mask = SETCOORD_PERCENT | SETCOORD_LENGTH |
>+                       SETCOORD_STORE_CALC;

const int32_t

(and the same for the existing two chunks, too!)

>+              inset = coords[0];

Please assert that j != 1.

>+            if (SetPairCoords(radius, coordX, coordY,
>+                              nsStyleCoord(), nsStyleCoord(), mask,
>+                              aStyleContext, aPresContext,
>+                              aCanStoreInRuleTree)) {

What if SetPairCoords fails?  I'm guessing that it can't, and that
you should assert it can't.  (Though it's still ok to test it at
runtime.)

nsStyleConsts.h:


>+
>+// Basic Shape has radius
>+#define NS_STYLE_BASIC_SHAPE_NO_RADIUS     0
>+#define NS_STYLE_BASIC_SHAPE_RADIUS        1

This is unused; remove it.


I'm not sure why nsStyleBasicShape has an mFillRule, although
given that it's there, initializing it is good.  But maybe it
can be removed?

>+  bool HasRadius() const {

This should probably assert that type is inset just like the following
2 functions do.

Comment 8

[Dirk Schulze]

Attachment: clip-path-inset.patch

(In reply to David Baron [:dbaron] (UTC-4) (needinfo? for questions) (away/busy Oct. 24-31) from comment #7)
> Comment on attachment 8506199 [details] [diff] [review]
> clip-path-inset.patch
>

I fixed all comments.

> I'm not sure why nsStyleBasicShape has an mFillRule, although
> given that it's there, initializing it is good.  But maybe it
> can be removed?
> 

mFillRule is not used for inset() but needed for polygon(). polygon() has an argument nonzero or evenodd which is encoded in this variable.

Comment 9

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

Comment on attachment 8510644 [details] [diff] [review]
clip-path-inset.patch

Sorry for dropping this one on the floor during TPAC.


Declaration.cpp new changes?

>+      REPORT_UNEXPECTED_EOF(PEBorderRadiusEOF);

You need to delete this line.

>+  // caller depend on the items being contiguous

caller -> callers

>+  void BasicShapeRadiiToString(nsAString& aCssText,
>+                                const nsStyleCorners& aCorners);

Fix indent (one space less on the second line).



r=dbaron with that fixed

Comment 10

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

Don't forget about getting this landed, please.

Comment 11

[Dirk Schulze]

I'll prepare the patch when I have a bit more time. Won't forget it :)

Comment 12

[Dirk Schulze]

Attachment: clip-path-inset-001.patch

Patch for landing. Try bots results here:

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

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c993904ea4f

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3c993904ea4f