Bug 239019

Created attachment 457124 [details]
Patch

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

Created attachment 457289 [details]
Patch

Created attachment 457645 [details]
Patch

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;
}

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.

Created attachment 457646 [details]
Patch

(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.

(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.

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.

Created attachment 457647 [details]
Patch

(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.

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].

<rdar://problem/91784817>

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