Bug 139334 – Get rid of error-prone ReleaseParsedCalcValueCondition argument in CSSParser

• Component: CSS
• Status: RESOLVED FIXED

[Chris Dumez]

Get rid of error-prone ReleaseParsedCalcValueCondition argument in CSSParser.

[Chris Dumez]

Created attachment 242702 [details]
Patch

[WebKit Commit Bot]

Attachment 242702 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSParser.cpp:1641:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/css/CSSParser.cpp:9357:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/css/CSSParser.h:705:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 242704 [details]
Patch

[WebKit Commit Bot]

Attachment 242704 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSParser.cpp:1641:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/css/CSSParser.cpp:9358:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/css/CSSParser.h:705:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 242718 [details]
Patch

[WebKit Commit Bot]

Attachment 242718 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSParser.cpp:1641:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/css/CSSParser.cpp:9358:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/css/CSSParser.h:705:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 242718 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        the parsed calculation value is not associated with its CSSParserValue.

"is associated" rather than is *not* associated

> Source/WebCore/css/CSSParser.h:695
> +class CSSParserValueWithState {

What does “with state” mean? I am having trouble coming up with a good name, but the current name does not make sense to me. Maybe: CSSParserValueOrCalculation, CSSParserValueAndCalculation, or CSSParserValueWithCalculation.

Maybe we should consider making this a member of the CSSParser class to make the class name shorter in most of the code.

> Source/WebCore/css/CSSParser.h:704
> +    CSSCalcValue* parsedCalculation() const { return m_parsedCalculation.get(); }

Why the word “parsed” here? I don’t see how the calculation is any more “parsed” than the value is.

> Source/WebCore/css/CSSParser.h:705
> +    void setParsedCalculation(RefPtr<CSSCalcValue> calculation) { m_parsedCalculation = calculation; }

This should take PassRefPtr or a raw pointer, or alternatively use WTF::move in the function body.

> Source/WebCore/css/CSSParser.h:709
> +    private:
> +        CSSParserValue& m_value;
> +        RefPtr<CSSCalcValue> m_parsedCalculation;

This is not our normal indentation.

[Chris Dumez]

Comment on attachment 242718 [details]
Patch

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

>> Source/WebCore/css/CSSParser.h:705
>> +    void setParsedCalculation(RefPtr<CSSCalcValue> calculation) { m_parsedCalculation = calculation; }
> 
> This should take PassRefPtr or a raw pointer, or alternatively use WTF::move in the function body.

I hear we are transitioning away from PassRefPtr, which is why I used a RefPtr<> argument. I will add the WTF::move().

[Chris Dumez]

Created attachment 242877 [details]
Patch

[WebKit Commit Bot]

Attachment 242877 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSParser.cpp:1641:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/css/CSSParser.cpp:9356:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/css/CSSParser.h:93:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 243402 [details]
Patch

[WebKit Commit Bot]

Attachment 243402 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSParser.cpp:1641:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/css/CSSParser.cpp:9356:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 243402 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:1584
> +bool CSSParser::validCalculationUnit(ValueWithCalculation& valueWithCalculation, Units unitflags)

I find this function very strange.

It’s name is “valid calculation unit”, so it should return a “calculation unit”. But also it has a side effect of filling in the calculation. That’s really unclear from the name of the function. I have the same problem with the function validUnit and the many other functions that store the calculation into the ValueWithCalculation as a side effect. I think these functions should be named “validate” instead of “valid”, but also, I think they may want to be more explicit about how they fill in the calculation.

It’s also wrong capitalization to have an argument named “unitflags” with all lower case.

> Source/WebCore/css/CSSParser.cpp:1711
> +        ASSERT(isCalculation(valueWithCalculation));

Seems to me that this assertion should be moved into the ValueWithCalculation class instead of done at this call site.

> Source/WebCore/css/CSSParser.cpp:1781
> +    if (isCalculation(valueWithCalculation))

I think this maybe should be checking valueWithCalculation.calculation() for null instead of calling isCalculation.

> Source/WebCore/css/CSSParser.h:90
> +        CSSParserValue& value() const { return m_value; }
> +        operator CSSParserValue&() { return m_value; }

There’s no reason for the CSSParserValue& operator to be non-const. It should be const too.

[Chris Dumez]

Created attachment 243608 [details]
Patch

[WebKit Commit Bot]

Attachment 243608 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSParser.cpp:1642:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/css/CSSParser.cpp:1651:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/css/CSSParser.cpp:9359:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 243609 [details]
Patch

[WebKit Commit Bot]

Attachment 243609 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSParser.cpp:1642:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/css/CSSParser.cpp:1651:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/css/CSSParser.cpp:9359:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 243609 [details]
Patch

Clearing flags on attachment: 243609

Committed r177623: <http://trac.webkit.org/changeset/177623>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Chris, in the r177623 patch in ValueWithCalculation::setCalculation, it says isCalculation(m_value) where I think we wanted ASSERT(isCalculation(m_value)).

[Chris Dumez]

Reopening to attach new patch.

[Chris Dumez]

Created attachment 243612 [details]
Patch

[Chris Dumez]

(In reply to comment #20)
> Chris, in the r177623 patch in ValueWithCalculation::setCalculation, it says
> isCalculation(m_value) where I think we wanted
> ASSERT(isCalculation(m_value)).

Yes indeed. Thanks for double checking and catching it. I uploaded a tiny patch to fix this.

[WebKit Commit Bot]

Comment on attachment 243612 [details]
Patch

Clearing flags on attachment: 243612

Committed r177628: <http://trac.webkit.org/changeset/177628>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.