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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 446343: Patch

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




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

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

Change looks good, but there are enough coding style mistakes that I think we
should review an improved version.

> Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:66
> +    auto setAllowsNegativePercentageReferenceIfNeeded =
[](RefPtr<CSSCalcExpressionNode>& expression) {

This is using the wrong type. This should not take a RefPtr&, because that’s
for an in/out argument. This is a "never null" expression node, so the argument
type should just be CSSCalcExpressionNode&.

> Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:67
> +	   if (expression->type() ==
CSSCalcExpressionNode::Type::CssCalcOperation) {

Since we are using downcast below anyway, I recommend using is<> rather than
writing this expression out, making the code easier to read:

    if (is<CSSCalcOperationNode>(*expression)) {

> Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:70
> +	       auto& operationNode =
downcast<CSSCalcOperationNode>(*expression);
> +	       if (operationNode.isMinOrMaxNode())
> +		   operationNode.setAllowsNegativePercentageReference();

A nice way to write this is:

    if (auto& node = downcast<CSSCalcOperationNode>(*expression);
node.isMinOrMaxNode())
	node.setAllowsNegativePercentageReference();

The if statement with the semicolon keeps the local variable scope tight rather
than letting it spill into the code below. The one-word local variable name is
then good when the scope is so narrow; easy to see which node it is, and not
having to write that out in the variable name keeps the code small and likely
more readable.

> Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:77
> +		   if (child->type() ==
CSSCalcExpressionNode::Type::CssCalcOperation) {
> +		       auto& operationNode =
downcast<CSSCalcOperationNode>(child.get());
> +		       if (operationNode.isMinOrMaxNode())
> +			  
operationNode.setAllowsNegativePercentageReference();
> +		   }

Same comments here, except also this code is repeated, so we should be using a
lambda to not write it out twice. When you combine that with WebKit coding
style things like early return, you get this:

    auto setAllowsNegativePercentageReferenceIfNeeded = []
(CSSCalcExpressionNode& expression) {
	auto set = [] (CSSCalcOperationNode& operation) {
	    if (operation.isMinOrMaxNode())
		operation.setAllowsNegativePercentageReference();
	};
	if (is<CSSCalcOperationNode>(expression)) {
	    auto& operation = downcast<CSSCalcOperationNode>(expression);
	    set(operation);
	    for (auto& child : operation.children()) {
		if (is<CSSCalcOperationNode>(child))
		    set(downcast<CSSCalcOperationNode>(child));
	    }
	}
    };

But this code assumes there are no grandchildren. If that’s wrong, then it
should be written with recursion instead:

    auto setAllowsNegativePercentageReferenceIfNeeded = []
(CSSCalcExpressionNode& expression) {
	if (is<CSSCalcOperationNode>(expression)) {
	    auto& operation = downcast<CSSCalcOperationNode>(expression);
	    if (operation.isMinOrMaxNode())
		operation.setAllowsNegativePercentageReference();
	    for (auto& child : operation.children())
		setAllowsNegativePercentageReferenceIfNeeded(child);
	}
    };

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

The argument type bool does not make it clear what the argument is, so it needs
a name, allowNegativePercentageReference, here.

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:838
> +		   if (unitType == CSSUnitType::CSS_PERCENTAGE &&
numberOfChildren > 1)
> +		       return true;
> +
> +		   return false;

No need to write this boolean expression so long. Just this:

    return unitType == CSSUnitType::CSS_PERCENTAGE && numberOfChildren > 1;

Other tiny style thoughts: It can be tricky to use a lambda in a way that the
code *easier* to read; the current version makes it a little too hard. And I
find it a bit arbitrary to do the different checks in two if statements rather
than three or just one. If you do still like the lambda to document things,
then this might read better:

    auto involvesPercentageComparisons = [&] {
	return combinedUnitType == CSSUnitType::CSS_PERCENTAGE &&
m_children.size() > 1;
    };
    if (isMinOrMaxNode() && allowsNegativePercentageReference() &&
involvesPercentageComparisons())
	return;

This should be just as efficient and I think makes it pretty readable. I
believe the five lines are easier to read than the longer code block. Using
capturing rather than arguments for combinedUnitType and m_children does seem
to help rather than hurt..

> Source/WebCore/css/calc/CSSCalcOperationNode.h:149
> +    unsigned m_allowsNegativePercentageReference : 1;

This can just be a bool. There is no value to using a bitfield here, doesn't
save memory and makes bigger code. Also if we are using. bool we can initialize
here and don’t need to change the constructors.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:5562
> +	   auto position = consumePositionCoordinates(range, context.mode,
unitless, PositionSyntax::BackgroundPosition, /*
allowsNegativePercentageReference */ true);

This is why we use enumerations in WebKit for this kind of idiom. We don’t use
comments to make such argument clear, instead we used named enumeration values
that you can understand without a comment.

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

Needs a comment here, not at all obvious that this bool is
allowsNegativePercentageReference.