Bug 239019 – calc(): Serialize top level min/max/hypot as calc()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nikos Mouchtaris]

calc(): Serialize top level min/max/hypot as calc()

[Nikos Mouchtaris]

Created attachment 457124 [details]
Patch

[Darin Adler]

Comment on attachment 457124 [details]
Patch

fast/css/calc-parsing.html is failing and some other two; we should review once we have a patch with all tests passing

[Nikos Mouchtaris]

Created attachment 457289 [details]
Patch

[Nikos Mouchtaris]

Created attachment 457645 [details]
Patch

[Darin Adler]

Comment on attachment 457645 [details]
Patch

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

> Source/WebCore/css/calc/CSSCalcOperationNode.h:75
> +    void makeTopLevelCalc() { m_operator = CalcOperator::Add; }

Needs a comment explaining why the way to make something a top-level call is to set the operator to add, since that's not obvious from the name. I might move the body out of the class to the bottom of the header just to make room for the comment.

    void makeTopLevelCalc();

...

inline void CSSCalcOperationNode::makeTopLevelCalc()
{
    // Top level calc nodes where we need not preserve the function are changed into add nodes because that’s the best way to make them serialize as "calc(xxx)" and also evaluate them efficiently.
    <<<or whatever the comment should say>>>
    m_operator = CaclOperator::Add;
}

[Darin Adler]

Comment on attachment 457645 [details]
Patch

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

>> Source/WebCore/css/calc/CSSCalcOperationNode.h:75
>> +    void makeTopLevelCalc() { m_operator = CalcOperator::Add; }
> 
> Needs a comment explaining why the way to make something a top-level call is to set the operator to add, since that's not obvious from the name. I might move the body out of the class to the bottom of the header just to make room for the comment.
> 
>     void makeTopLevelCalc();
> 
> ...
> 
> inline void CSSCalcOperationNode::makeTopLevelCalc()
> {
>     // Top level calc nodes where we need not preserve the function are changed into add nodes because that’s the best way to make them serialize as "calc(xxx)" and also evaluate them efficiently.
>     <<<or whatever the comment should say>>>
>     m_operator = CaclOperator::Add;
> }

Also, these can be private rather than public, I think. And the inline function bodies could be in the .cpp file rather than in the .h file if they are only used there.

[Nikos Mouchtaris]

Created attachment 457646 [details]
Patch

[Nikos Mouchtaris]

(In reply to Darin Adler from comment #5)
> Comment on attachment 457645 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457645&action=review
> 
> > Source/WebCore/css/calc/CSSCalcOperationNode.h:75
> > +    void makeTopLevelCalc() { m_operator = CalcOperator::Add; }
> 
> Needs a comment explaining why the way to make something a top-level call is
> to set the operator to add, since that's not obvious from the name. I might
> move the body out of the class to the bottom of the header just to make room
> for the comment.
> 
>     void makeTopLevelCalc();
> 
> ...
> 
> inline void CSSCalcOperationNode::makeTopLevelCalc()
> {
>     // Top level calc nodes where we need not preserve the function are
> changed into add nodes because that’s the best way to make them serialize as
> "calc(xxx)" and also evaluate them efficiently.
>     <<<or whatever the comment should say>>>
>     m_operator = CaclOperator::Add;
> }

Fixed.

[Nikos Mouchtaris]

(In reply to Darin Adler from comment #6)
> Comment on attachment 457645 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457645&action=review
> 
> >> Source/WebCore/css/calc/CSSCalcOperationNode.h:75
> >> +    void makeTopLevelCalc() { m_operator = CalcOperator::Add; }
> > 
> > Needs a comment explaining why the way to make something a top-level call is to set the operator to add, since that's not obvious from the name. I might move the body out of the class to the bottom of the header just to make room for the comment.
> > 
> >     void makeTopLevelCalc();
> > 
> > ...
> > 
> > inline void CSSCalcOperationNode::makeTopLevelCalc()
> > {
> >     // Top level calc nodes where we need not preserve the function are changed into add nodes because that’s the best way to make them serialize as "calc(xxx)" and also evaluate them efficiently.
> >     <<<or whatever the comment should say>>>
> >     m_operator = CaclOperator::Add;
> > }
> 
> Also, these can be private rather than public, I think. And the inline
> function bodies could be in the .cpp file rather than in the .h file if they
> are only used there.

Fixed.

[Darin Adler]

Comment on attachment 457646 [details]
Patch

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

> Source/WebCore/css/calc/CSSCalcOperationNode.h:74
> +    bool shouldNotPreserveFunction() const { return isMinOrMaxNode() || isHypotNode(); }

I think this can also be private.

[Nikos Mouchtaris]

Created attachment 457647 [details]
Patch

[Nikos Mouchtaris]

(In reply to Darin Adler from comment #10)
> Comment on attachment 457646 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457646&action=review
> 
> > Source/WebCore/css/calc/CSSCalcOperationNode.h:74
> > +    bool shouldNotPreserveFunction() const { return isMinOrMaxNode() || isHypotNode(); }
> 
> I think this can also be private.

Fixed.

[EWS]

Committed r292893 (249663@main): <https://commits.webkit.org/249663@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 457647 [details].

[Radar WebKit Bug Importer]

<rdar://problem/91784817>

[Nikos Mouchtaris]

*** Bug 230365 has been marked as a duplicate of this bug. ***