[webkit-reviews] review granted: [Bug 233987] Don't do simplification for percentage comparison resolution against negative reference values : [Attachment 446504] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Joonghun Park
<jh718.park at samsung.com>'s request for review:
Bug 233987: Don't do simplification for percentage comparison resolution
against negative reference values
https://bugs.webkit.org/show_bug.cgi?id=233987

Attachment 446504: Patch

https://bugs.webkit.org/attachment.cgi?id=446504&action=review




--- Comment #7 from Darin Adler <darin at apple.com> ---
Comment on attachment 446504
  --> https://bugs.webkit.org/attachment.cgi?id=446504
Patch

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

I’d be OK landing this, but I think we can do better.

> Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:67
> +    std::function<void(CSSCalcExpressionNode&)>
setAllowsNegativePercentageReferenceIfNeeded =
[&setAllowsNegativePercentageReferenceIfNeeded](CSSCalcExpressionNode&
expression) {

Type here should be "auto", not "std::function<...>".

> Source/WebCore/css/calc/CSSCalcExpressionNodeParser.h:39
> +enum class NegativePercentageReference : uint8_t;

bool (see also my naming suggestion above).

> Source/WebCore/css/calc/CSSCalcExpressionNodeParser.h:49
> +    RefPtr<CSSCalcExpressionNode> parseCalc(CSSParserTokenRange, CSSValueID
function, NegativePercentageReference allowsNegativePercentageReference);

With the right enum class name (see my naming suggestion above), we would not
need an argument name here. That is a goal. Not great to say things twice.

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:836
>	   auto combinedUnitType = m_children[0]->primitiveType();
> +	   auto involvesPercentageComparisons = [&]() {
> +	       return combinedUnitType == CSSUnitType::CSS_PERCENTAGE &&
m_children.size() > 1;
> +	   };
> +
> +	   if (isMinOrMaxNode() && allowsNegativePercentageReference() &&
involvesPercentageComparisons())
> +	       return;

Not really sure about the paragraphing here. The lack of blank line before
definition the involvesPercentageComparisons, and the blank line after.

Also, pretty sure we typically write [&] { ... }, without the () for the empty
argument list.

> Source/WebCore/css/calc/CSSCalcOperationNode.h:76
> +    bool allowsNegativePercentageReference() { return
m_allowsNegativePercentageReference; }

Should be a const member function.

> Source/WebCore/css/calc/CSSCalcValue.h:54
> +    static RefPtr<CSSCalcValue> create(CSSValueID function, const
CSSParserTokenRange&, CalculationCategory destinationCategory, ValueRange,
const CSSCalcSymbolTable&, NegativePercentageReference =
NegativePercentageReference::Forbid);

Could use overloading here rather than an argument with a default value; one
create function with and one without. That would remove the need to include the
enum class definition in a header.

> Source/WebCore/css/calc/NegativePercentageReference.h:33
> +enum class NegativePercentageReference : uint8_t {
> +    Allow,
> +    Forbid
> +};

Should try to avoid having to create an entire header file just for this single
enum class.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.h:96
> +RefPtr<CSSPrimitiveValue> consumeLengthOrPercent(CSSParserTokenRange&,
CSSParserMode, ValueRange, UnitlessQuirk = UnitlessQuirk::Forbid,
NegativePercentageReference = NegativePercentageReference::Forbid);

Here this could be a boolean; no one is calling this with a constant, so the
enumeration is not needed. We don’t have to use the enumerations all the way
down unless you think it’s preferable.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.h:134
> +std::optional<PositionCoordinates>
consumePositionCoordinates(CSSParserTokenRange&, CSSParserMode, UnitlessQuirk,
PositionSyntax, NegativePercentageReference =
NegativePercentageReference::Forbid);

Ditto.