Get rid of error-prone ReleaseParsedCalcValueCondition argument in CSSParser.
Created attachment 242702 [details] Patch
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.
Created attachment 242704 [details] Patch
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.
Created attachment 242718 [details] Patch
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.
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.
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().
Created attachment 242877 [details] Patch
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.
Created attachment 243402 [details] Patch
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.
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.
Created attachment 243608 [details] Patch
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.
Created attachment 243609 [details] Patch
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.
Comment on attachment 243609 [details] Patch Clearing flags on attachment: 243609 Committed r177623: <http://trac.webkit.org/changeset/177623>
All reviewed patches have been landed. Closing bug.
Chris, in the r177623 patch in ValueWithCalculation::setCalculation, it says isCalculation(m_value) where I think we wanted ASSERT(isCalculation(m_value)).
Reopening to attach new patch.
Created attachment 243612 [details] Patch
(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.
Comment on attachment 243612 [details] Patch Clearing flags on attachment: 243612 Committed r177628: <http://trac.webkit.org/changeset/177628>