Bug 79188 – CSS3 calc(): handle non-negative values

• Component: New Bugs
• Status: RESOLVED FIXED

[Mike Lawther]

CSS3 calc(): handle non-negative values

[Mike Lawther]

Created attachment 128121 [details]
Patch

[Mike Lawther]

Created attachment 128127 [details]
Patch

[Daniel Bates]

Comment on attachment 128127 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Some CSS properties (eg padding) are required to be non-negative. These

Nit: eg => e.g.

("e.g." is an acronym for the Latin phrase "example gratia")

> Source/WebCore/ChangeLog:16
> +        (WebCore::CSSCalcValue::clampToRange):

For your consideration, I suggest adding the word "Added."  to right of the ':' for all methods that were added in this patch. This makes it straightforward to identify the added methods from reading the change log message and also helps when searching on Trac for a change that introduced a method.

> Source/WebCore/css/CSSCalculationValue.cpp:84
> +double CSSCalcValue::clampToRange(double value) const
> +{
> +    return m_nonNegative && value < 0 ? 0 : value;
> +}    

I don't really like the name of this method or doubleValue() as they are somewhat disingenuous, but I can't think of a better name at the moment. I mean, the value is only clamped to 0 if its negative and its permitted value range is non-negative.

> Source/WebCore/css/CSSCalculationValue.cpp:90
>  double CSSCalcValue::doubleValue() const 
>  { 
> +    return clampToRange(doubleValueUnclamped());
> +}
> +

I take it that you've ensured that all call sites of doubleValue() have been changed to doubleValueUnclamped() since you've changed the meaning of doubleValue() from being an unclamped value to a "clamped value" (see my remark for clampToRange() above).

> Source/WebCore/css/CSSCalculationValue.cpp:91
> +double CSSCalcValue::doubleValueUnclamped() const 

You may want to consider inlining this function in the header since it's a simple expression and its referenced in doubleValue().

> Source/WebCore/css/CSSParser.cpp:710
> +    if (!parseCalculation(value, unitflags & FNonNeg ? CalculationRangeNonNegative : CalculationRangeAll))

I suggest defining a variable, say isNonNegativeValue, for the expression "unitflags & FNonNeg" since it appears three times in this code block and is evaluated at most twice.

[Mike Lawther]

Comment on attachment 128127 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        Some CSS properties (eg padding) are required to be non-negative. These
> 
> Nit: eg => e.g.
> 
> ("e.g." is an acronym for the Latin phrase "example gratia")

Done.

>> Source/WebCore/ChangeLog:16
>> +        (WebCore::CSSCalcValue::clampToRange):
> 
> For your consideration, I suggest adding the word "Added."  to right of the ':' for all methods that were added in this patch. This makes it straightforward to identify the added methods from reading the change log message and also helps when searching on Trac for a change that introduced a method.

Done. I wasn't aware that this was a convention. Sounds like something worth teaching the Changelog creation script about :)

>> Source/WebCore/css/CSSCalculationValue.cpp:84
>> +}    
> 
> I don't really like the name of this method or doubleValue() as they are somewhat disingenuous, but I can't think of a better name at the moment. I mean, the value is only clamped to 0 if its negative and its permitted value range is non-negative.

renamed to clampToPermittedRange().

>> Source/WebCore/css/CSSCalculationValue.cpp:90
>> +
> 
> I take it that you've ensured that all call sites of doubleValue() have been changed to doubleValueUnclamped() since you've changed the meaning of doubleValue() from being an unclamped value to a "clamped value" (see my remark for clampToRange() above).

All existing call sites of doubleValue() really wanted the value clamped - that's the bug this patch fixes.

>> Source/WebCore/css/CSSCalculationValue.cpp:91
>> +double CSSCalcValue::doubleValueUnclamped() const 
> 
> You may want to consider inlining this function in the header since it's a simple expression and its referenced in doubleValue().

Renamed to isNegative(), and inlined.

>> Source/WebCore/css/CSSParser.cpp:710
>> +    if (!parseCalculation(value, unitflags & FNonNeg ? CalculationRangeNonNegative : CalculationRangeAll))
> 
> I suggest defining a variable, say isNonNegativeValue, for the expression "unitflags & FNonNeg" since it appears three times in this code block and is evaluated at most twice.

Done. I called it mustBeNonNegative, since it's a requirement, rather than a fact. It makes the below checks of 'if mustBeNonNegative && isNegative() then fail' read better as well.

[Mike Lawther]

Created attachment 128593 [details]
Patch for landing

[Daniel Bates]

Comment on attachment 128593 [details]
Patch for landing

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

> Source/WebCore/css/CSSCalculationValue.cpp:80
> +    

Nit: I would remove this extraneous empty line as it doesn't improve the readability of this file.

[Daniel Bates]

Comment on attachment 128593 [details]
Patch for landing

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

> Source/WebCore/css/CSSCalculationValue.h:101
> +                        

Nit: This line has extraneous whitespace.

[Mike Lawther]

Comment on attachment 128593 [details]
Patch for landing

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

>> Source/WebCore/css/CSSCalculationValue.cpp:80
>> +    
> 
> Nit: I would remove this extraneous empty line as it doesn't improve the readability of this file.

Oops. Fixed.

>> Source/WebCore/css/CSSCalculationValue.h:101
>> +                        
> 
> Nit: This line has extraneous whitespace.

Fixed.

[Daniel Bates]

Comment on attachment 128593 [details]
Patch for landing

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

> Source/WebCore/css/CSSParser.cpp:710
> +    bool mustBeNonNegative = unitflags & FNonNeg;

Nit: This is OK as-is. That being said, I don't like the prefix "must" because it implies a requirement, but this boolean variable could evaluate to false, which contradicts the meaning of "must". For your consideration, I suggest naming this variable, isNonNegative.

[Mike Lawther]

Created attachment 128597 [details]
Patch for landing

[Mike Lawther]

Comment on attachment 128593 [details]
Patch for landing

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

>> Source/WebCore/css/CSSParser.cpp:710
>> +    bool mustBeNonNegative = unitflags & FNonNeg;
> 
> Nit: This is OK as-is. That being said, I don't like the prefix "must" because it implies a requirement, but this boolean variable could evaluate to false, which contradicts the meaning of "must". For your consideration, I suggest naming this variable, isNonNegative.

It is a requirement isn't it? The function here is 'validCalculationUnit', and its purpose is to determine whether the current calculated value is valid as per the requirements passed in the flags.

I take your point about the negative of 'mustBeNonNegative' being potentially confusing. I don't think it's a contradiction though as it's a requirement about the value, not a fact about the value. Maybe I should reuse the enum I defined that has the values CalculationRangeNonNegative and CalculationRangeAll for this?

[Daniel Bates]

(In reply to comment #11)
> (From update of attachment 128593 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128593&action=review
> 
> >> Source/WebCore/css/CSSParser.cpp:710
> >> +    bool mustBeNonNegative = unitflags & FNonNeg;
> > 
> > Nit: This is OK as-is. That being said, I don't like the prefix "must" because it implies a requirement, but this boolean variable could evaluate to false, which contradicts the meaning of "must". For your consideration, I suggest naming this variable, isNonNegative.
> 
> It is a requirement isn't it? The function here is 'validCalculationUnit', and its purpose is to determine whether the current calculated value is valid as per the requirements passed in the flags.
> 
> I take your point about the negative of 'mustBeNonNegative' being potentially confusing.

Yes, I felt the name is confusing.

> I don't think it's a contradiction though as it's a requirement about the value, not a fact about the value. Maybe I should reuse the enum I defined that has the values CalculationRangeNonNegative and CalculationRangeAll for this?

The use of a boolean seems sufficient. Feel free to improve this code and increase its readability.

[WebKit Review Bot]

Comment on attachment 128597 [details]
Patch for landing

Clearing flags on attachment: 128597

Committed r108750: <http://trac.webkit.org/changeset/108750>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.