Bug 224322 – calc() values resulting from blending mixed type lengths should be simplified

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Antoine Quint]

calc() values resulting from blending mixed type lengths should be simplified

[Antoine Quint]

Created attachment 425494 [details]
Patch

[Antoine Quint]

Created attachment 425504 [details]
Patch

[Antoine Quint]

Created attachment 425505 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 425505 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425505&action=review

> Source/WebCore/ChangeLog:12
> +        When seraliazing calc() values, we now check whether they are the result of blending

seraliazing

> Source/WebCore/css/CSSCalculationValue.cpp:2054
> +    // CSSCalcOperationNode is only ever created in the context of blending
> +    // and requires simplification to match serialization rules.

That doesn't seem accurate. CSSCalcOperationNode::createMinOrMaxOrClamp, CSSCalcOperationNode::createProduct all return CSSCalcOperationNodes

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #4)
> Comment on attachment 425505 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425505&action=review
> 
> > Source/WebCore/css/CSSCalculationValue.cpp:2054
> > +    // CSSCalcOperationNode is only ever created in the context of blending
> > +    // and requires simplification to match serialization rules.
> 
> That doesn't seem accurate. CSSCalcOperationNode::createMinOrMaxOrClamp,
> CSSCalcOperationNode::createProduct all return CSSCalcOperationNodes

Oh! Should we keep a flag of when we create one for blending and only simplify then, or do those cases also need simplification?

[Simon Fraser (smfr)]

Seems like we only need to simplify after blending.

[Sam Weinig]

Comment on attachment 425505 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425505&action=review

>>> Source/WebCore/css/CSSCalculationValue.cpp:2054
>>> +    // and requires simplification to match serialization rules.
>> 
>> That doesn't seem accurate. CSSCalcOperationNode::createMinOrMaxOrClamp, CSSCalcOperationNode::createProduct all return CSSCalcOperationNodes
> 
> Oh! Should we keep a flag of when we create one for blending and only simplify then, or do those cases also need simplification?

It seems really weird that blending would require different rules than other places. Where is this specified?

[Simon Fraser (smfr)]

(In reply to Sam Weinig from comment #7)
> Comment on attachment 425505 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425505&action=review
> 
> >>> Source/WebCore/css/CSSCalculationValue.cpp:2054
> >>> +    // and requires simplification to match serialization rules.
> >> 
> >> That doesn't seem accurate. CSSCalcOperationNode::createMinOrMaxOrClamp, CSSCalcOperationNode::createProduct all return CSSCalcOperationNodes
> > 
> > Oh! Should we keep a flag of when we create one for blending and only simplify then, or do those cases also need simplification?
> 
> It seems really weird that blending would require different rules than other
> places. Where is this specified?

It doesn't require different rules. It's just that we normally simplify after parsing, and blending generates calc() values which are not the result of parsing.

[Antoine Quint]

Created attachment 425525 [details]
Patch

[Darin Adler]

Comment on attachment 425505 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425505&action=review

> Source/WebCore/css/CSSCalculationValue.cpp:2056
> +    if (is<CSSCalcOperationNode>(node))
> +        node = CSSCalcOperationNode::simplify(node.get());

This is what virtual functions are for. Is there a good reason we don’t have a virtual function version of simplify?

[Darin Adler]

Comment on attachment 425505 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425505&action=review

>> Source/WebCore/css/CSSCalculationValue.cpp:2056
>> +        node = CSSCalcOperationNode::simplify(node.get());
> 
> This is what virtual functions are for. Is there a good reason we don’t have a virtual function version of simplify?

Presumably it would just return "this" on non-calc nodes.

[Darin Adler]

Comment on attachment 425505 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425505&action=review

>>>>> Source/WebCore/css/CSSCalculationValue.cpp:2054
>>>>> +    // and requires simplification to match serialization rules.
>>>> 
>>>> That doesn't seem accurate. CSSCalcOperationNode::createMinOrMaxOrClamp, CSSCalcOperationNode::createProduct all return CSSCalcOperationNodes
>>> 
>>> Oh! Should we keep a flag of when we create one for blending and only simplify then, or do those cases also need simplification?
>> 
>> It seems really weird that blending would require different rules than other places. Where is this specified?
> 
> It doesn't require different rules. It's just that we normally simplify after parsing, and blending generates calc() values which are not the result of parsing.

Could we use a dirty bit to say "already simplified" to deal with this?

[Antoine Quint]

(In reply to Darin Adler from comment #12)
> Comment on attachment 425505 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425505&action=review
> 
> >>>>> Source/WebCore/css/CSSCalculationValue.cpp:2054
> >>>>> +    // and requires simplification to match serialization rules.
> >>>> 
> >>>> That doesn't seem accurate. CSSCalcOperationNode::createMinOrMaxOrClamp, CSSCalcOperationNode::createProduct all return CSSCalcOperationNodes
> >>> 
> >>> Oh! Should we keep a flag of when we create one for blending and only simplify then, or do those cases also need simplification?
> >> 
> >> It seems really weird that blending would require different rules than other places. Where is this specified?
> > 
> > It doesn't require different rules. It's just that we normally simplify after parsing, and blending generates calc() values which are not the result of parsing.
> 
> Could we use a dirty bit to say "already simplified" to deal with this?

We could, it felt to me like more work that necessary to manage setting that bit to false. We generally know that these values need simplification as being, most likely, the result of blending, and this codepath is only used for serializing computed values, so it's unlikely we'll call the simplification code more than necessary.

[Antoine Quint]

(In reply to Darin Adler from comment #11)
> Comment on attachment 425505 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425505&action=review
> 
> >> Source/WebCore/css/CSSCalculationValue.cpp:2056
> >> +        node = CSSCalcOperationNode::simplify(node.get());
> > 
> > This is what virtual functions are for. Is there a good reason we don’t have a virtual function version of simplify?
> 
> Presumably it would just return "this" on non-calc nodes.

I don't follow here. This is not about requiring special work to be done for CSSCalcOperationNode, it's just that in the other cases we know simplification happened already at parse-time, whereas for these we don't simplify when creating them – and don't need to until we serialize them.

[Sam Weinig]

Can you point me to the blending code that is calling this? This really feels like the wrong place to be doing this.

[Antoine Quint]

(In reply to Sam Weinig from comment #15)
> Can you point me to the blending code that is calling this? This really
> feels like the wrong place to be doing this.

https://github.com/WebKit/WebKit/blob/0ebbd8f1ce40d6a78dbe72a826a20a1994c9d0dd/Source/WebCore/platform/Length.cpp#L297

[Sam Weinig]

(In reply to Antoine Quint from comment #16)
> (In reply to Sam Weinig from comment #15)
> > Can you point me to the blending code that is calling this? This really
> > feels like the wrong place to be doing this.
> 
> https://github.com/WebKit/WebKit/blob/
> 0ebbd8f1ce40d6a78dbe72a826a20a1994c9d0dd/Source/WebCore/platform/Length.
> cpp#L297

Hm, can you walk me through it a bit more. That creates a CalcExpressionBlendLength.

Where does the CSSCalcValue come from?

[Antoine Quint]

This is the first time I look at the calc() code, so it’s quite likely there’s a better way to do this. However, it does seem like simplification is directly related to serialization, and this looked like a good place to do this. Another option is to simplify as we create the expression object, but we don’t need to simplify aside from serialization, so it sounds wasteful.

[Antoine Quint]

(In reply to Sam Weinig from comment #17)
> (In reply to Antoine Quint from comment #16)
> > (In reply to Sam Weinig from comment #15)
> > > Can you point me to the blending code that is calling this? This really
> > > feels like the wrong place to be doing this.
> > 
> > https://github.com/WebKit/WebKit/blob/
> > 0ebbd8f1ce40d6a78dbe72a826a20a1994c9d0dd/Source/WebCore/platform/Length.
> > cpp#L297
> 
> Hm, can you walk me through it a bit more. That creates a
> CalcExpressionBlendLength.
> 
> Where does the CSSCalcValue come from?

We really need Simon to help here, I think he wrote most of this code.

[Sam Weinig]

(In reply to Sam Weinig from comment #17)
> (In reply to Antoine Quint from comment #16)
> > (In reply to Sam Weinig from comment #15)
> > > Can you point me to the blending code that is calling this? This really
> > > feels like the wrong place to be doing this.
> > 
> > https://github.com/WebKit/WebKit/blob/
> > 0ebbd8f1ce40d6a78dbe72a826a20a1994c9d0dd/Source/WebCore/platform/Length.
> > cpp#L297
> 
> Hm, can you walk me through it a bit more. That creates a
> CalcExpressionBlendLength.
> 
> Where does the CSSCalcValue come from?

I think I worked it out. I think it probably the CSSPrimitiveValue constructor that takes a Length and when it is a calculated length calls `CSSCalcValue::create(length.calculationValue(), style)`.

I think the right time to do simplification is in CSSCalcValue::create(const CalculationValue&, const RenderStyle&). Just as the CSSCalcValue::create(CSSValueID, const CSSParserTokenRange&, CalculationCategory) does simplification via parser.parseCalc(), the create taking a CalculationValue should do it upfront. This will maintain the invariant that CSSCalcValue is always in its simplified form.

If we want to optimize not simplifying until it is actually needed, we could do that for both forms, but I don't think that is required.

[Sam Weinig]

(In reply to Sam Weinig from comment #20)
> (In reply to Sam Weinig from comment #17)
> > (In reply to Antoine Quint from comment #16)
> > > (In reply to Sam Weinig from comment #15)
> > > > Can you point me to the blending code that is calling this? This really
> > > > feels like the wrong place to be doing this.
> > > 
> > > https://github.com/WebKit/WebKit/blob/
> > > 0ebbd8f1ce40d6a78dbe72a826a20a1994c9d0dd/Source/WebCore/platform/Length.
> > > cpp#L297
> > 
> > Hm, can you walk me through it a bit more. That creates a
> > CalcExpressionBlendLength.
> > 
> > Where does the CSSCalcValue come from?
> 
> I think I worked it out. I think it probably the CSSPrimitiveValue
> constructor that takes a Length and when it is a calculated length calls
> `CSSCalcValue::create(length.calculationValue(), style)`.
> 
> I think the right time to do simplification is in CSSCalcValue::create(const
> CalculationValue&, const RenderStyle&). Just as the
> CSSCalcValue::create(CSSValueID, const CSSParserTokenRange&,
> CalculationCategory) does simplification via parser.parseCalc(), the create
> taking a CalculationValue should do it upfront. This will maintain the
> invariant that CSSCalcValue is always in its simplified form.
> 
> If we want to optimize not simplifying until it is actually needed, we could
> do that for both forms, but I don't think that is required.

Something like this:

RefPtr<CSSCalcValue> CSSCalcValue::create(const CalculationValue& value, const RenderStyle& style)
{
    auto expression = createCSS(value.expression(), style);
    if (!expression)
        return nullptr;

    auto simplifiedExpression = CSSCalcOperationNode::simplify(expression.releaseNonNull());

    auto result = adoptRef(new CSSCalcValue(WTFMove(simplifiedExpression), value.shouldClampToNonNegative()));
    LOG_WITH_STREAM(Calc, stream << "CSSCalcValue::create from CalculationValue: " << *result);
    return result;
}

[Sam Weinig]

(In reply to Antoine Quint from comment #18)
> This is the first time I look at the calc() code, so it’s quite likely
> there’s a better way to do this. However, it does seem like simplification
> is directly related to serialization, and this looked like a good place to
> do this. Another option is to simplify as we create the expression object,
> but we don’t need to simplify aside from serialization, so it sounds
> wasteful.

Given we already simplify all calc()s that are parsed, doing it in this case doesn't sound like a problem. I think you are optimizing for a problem we don't know exists. Ifs its a perf issue here, we should do it for both cases.

[Antoine Quint]

(In reply to Sam Weinig from comment #22)
> (In reply to Antoine Quint from comment #18)
> > This is the first time I look at the calc() code, so it’s quite likely
> > there’s a better way to do this. However, it does seem like simplification
> > is directly related to serialization, and this looked like a good place to
> > do this. Another option is to simplify as we create the expression object,
> > but we don’t need to simplify aside from serialization, so it sounds
> > wasteful.
> 
> Given we already simplify all calc()s that are parsed, doing it in this case
> doesn't sound like a problem. I think you are optimizing for a problem we
> don't know exists. Ifs its a perf issue here, we should do it for both cases.

Maybe so. But we end up creating those blended calc() values for each frame we resolve an animation that blends between mixed types. Meanwhile, serialization may never be needed.

[Antoine Quint]

Created attachment 425590 [details]
Patch

[Antoine Quint]

Committed r275763 (236341@main): <https://commits.webkit.org/236341@main>

[Radar WebKit Bug Importer]

<rdar://problem/76455027>

[Simon Fraser (smfr)]

Comment on attachment 425590 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425590&action=review

> Source/WebCore/css/CSSCalculationValue.cpp:2125
>      LOG_WITH_STREAM(Calc, stream << "CSSCalcValue::create from CalculationValue: " << *result);

Would be nice if the logging revealed that simplification had taken place.