Closed Bug 1264317 Opened 4 years ago Closed 4 years ago

Make the basic shape clip-path clipping use nsCSSValue::Array instead of nsCSSValueList

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(1 file)

In bug 1110460 Daniel asked that nsCSSValue::Array be used instead of nsCSSValueList:

(In reply to Daniel Holbert [:dholbert] from bug 1110460 comment #7)
>  (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.

Making StyleAnimationValue::UncomputeValue set an nsCSSValue::Array for eUnit_Shape instead of setting an nsCSSValueList means that the result of that function would be incompatible with the existing code for clip path shapes though. If we make that change over there then we also need to change the existing clip path shape code.
Attached patch patchSplinter Review
Attachment #8740971 - Flags: review?(dholbert)
Comment on attachment 8740971 [details] [diff] [review]
patch

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

r=me, a few nits below:

::: layout/style/nsCSSParser.cpp
@@ +15841,4 @@
>      nsCSSValue referenceBox;
>      bool hasBox = ParseEnum(referenceBox, nsCSSProps::kClipShapeSizingKTable);
>  
> +    bool boxCameFirst = hasBox;

Nit: I'd prefer we declare this new bool as "const", to make it clearer that its value never changes (unlike |hasBox|, which does change, which is why we benefit from having this second bool that captures its value).

@@ +15857,5 @@
> +    if (!hasBox) {
> +      hasBox = ParseEnum(referenceBox, nsCSSProps::kClipShapeSizingKTable);
> +    }
> +
> +    RefPtr<nsCSSValue::Array> funcAndRefBox =

Minor concern: "funcAndRefBox" is perhaps a misleading name, since it's not necessarily both of those things (despite "and" in the variable name).

A better name might be "combinedValue" or "fullValue".  But, this is also probably fine as-is, too.

@@ +15866,5 @@
> +      funcAndRefBox->Item(boxCameFirst ? 1 : 0) = basicShape;
> +    } else if (hasBox) {
> +      funcAndRefBox->Item(0) = referenceBox;
> +    } else {
> +      funcAndRefBox->Item(0) = basicShape;

Please add to this clause an assertion like:
   MOZ_ASSERT(hasShape, "should've bailed if we got neither box nor shape");

This would serve as documentation that this is the "hasShape" clause, even though we're not explicitly checking "else if (hasShape)" here.

(We're assuming we don't have to manually check hasShape, and it's nice to make that assumption explicit with an assertion.)

::: layout/style/nsRuleNode.cpp
@@ -9364,1 @@
>               "expected a basic shape or reference box");

(tangent: Oops -- looks like this assertion was accidentally not asserting anything in the old code!  It meant "==", not "!=". And by virtue of using !=, it was asserting a tautology and not actually checking anything.  Anyway -- looks like you caught this and switched to "==" in the new code -- good.)

@@ +9368,5 @@
>  
>    uint8_t sizingBox = NS_STYLE_CLIP_SHAPE_SIZING_NOBOX;
>    RefPtr<nsStyleBasicShape> basicShape;
> +  for (unsigned i = 0; i < array->Count(); ++i) {
> +    if (array->Item(i).GetUnit() == eCSSUnit_Enumerated) {

Nit: s/unsigned/size_t/ for the type of "i" here, please.

(size_t is the type that Count() returns, and also the type that the array->Item(i) call expects.)

@@ +9378,5 @@
> +      }
> +      sizingBox = (uint8_t)type;
> +    } else if (array->Item(i).GetUnit() == eCSSUnit_Function) {
> +      nsCSSValue::Array* shapeFunction = array->Item(i).GetArrayValue();
> +        nsCSSKeyword functionName =

This line is mis-indented -- delete 2 spaces before "nsCSSKeyword" please.
Attachment #8740971 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/mozilla-central/rev/dc8cdaac96f3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.