Bug 79188 - CSS3 calc(): handle non-negative values
Summary: CSS3 calc(): handle non-negative values
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike Lawther
URL:
Keywords:
Depends on:
Blocks: 16662
  Show dependency treegraph
 
Reported: 2012-02-21 19:55 PST by Mike Lawther
Modified: 2012-02-24 01:33 PST (History)
2 users (show)

See Also:


Attachments
Patch (8.99 KB, patch)
2012-02-21 19:59 PST, Mike Lawther
no flags Details | Formatted Diff | Diff
Patch (10.71 KB, patch)
2012-02-21 21:18 PST, Mike Lawther
no flags Details | Formatted Diff | Diff
Patch for landing (10.80 KB, patch)
2012-02-23 16:23 PST, Mike Lawther
no flags Details | Formatted Diff | Diff
Patch for landing (10.78 KB, patch)
2012-02-23 16:33 PST, Mike Lawther
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Lawther 2012-02-21 19:55:42 PST
CSS3 calc(): handle non-negative values
Comment 1 Mike Lawther 2012-02-21 19:59:18 PST
Created attachment 128121 [details]
Patch
Comment 2 Mike Lawther 2012-02-21 21:18:12 PST
Created attachment 128127 [details]
Patch
Comment 3 Daniel Bates 2012-02-23 15:17:00 PST
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.
Comment 4 Mike Lawther 2012-02-23 16:22:05 PST
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.
Comment 5 Mike Lawther 2012-02-23 16:23:04 PST
Created attachment 128593 [details]
Patch for landing
Comment 6 Daniel Bates 2012-02-23 16:26:09 PST
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.
Comment 7 Daniel Bates 2012-02-23 16:27:38 PST
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.
Comment 8 Mike Lawther 2012-02-23 16:32:44 PST
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.
Comment 9 Daniel Bates 2012-02-23 16:32:55 PST
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.
Comment 10 Mike Lawther 2012-02-23 16:33:28 PST
Created attachment 128597 [details]
Patch for landing
Comment 11 Mike Lawther 2012-02-23 16:42:08 PST
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?
Comment 12 Daniel Bates 2012-02-23 19:41:44 PST
(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.
Comment 13 WebKit Review Bot 2012-02-24 01:33:11 PST
Comment on attachment 128597 [details]
Patch for landing

Clearing flags on attachment: 128597

Committed r108750: <http://trac.webkit.org/changeset/108750>
Comment 14 WebKit Review Bot 2012-02-24 01:33:16 PST
All reviewed patches have been landed.  Closing bug.