Used strongly-typed enum for CSS arithmetic operators

Current code uses weakly-typed CalcOperator enum for arithmetic
operators, and casts them between chars interchangeably. This
makes it hard to read, and prevents extending the operators with
more math functions.

Hence, this patch changes CalcOperator to a strongly-typed enum,
stops all the direct casting between chars, and renames it to
CSSMathOperator as a preparation to add new math functions.

Bug: 825895
Change-Id: Ie2ea58163f800cf5f480ac34a9c49a3e677e1946
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1680294Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#673341}

third_party/blink/renderer/core/animation/length_interpolation_functions.cc

@@ -233,8 +233,8 @@ const CSSValue* LengthInterpolationFunctions::CreateCSSValue(
     if (!root_node) {
       root_node = CSSCalcValue::CreateExpressionNode(first_value);
     }
-    root_node =
-        CSSCalcValue::CreateExpressionNode(root_node, current_node, kCalcAdd);
+    root_node = CSSCalcValue::CreateExpressionNode(root_node, current_node,
+                                                   CSSMathOperator::kAdd);
   }
 
   if (root_node) {

third_party/blink/renderer/core/css/BUILD.gn

@@ -108,6 +108,8 @@ blink_core_sources("css") {
     "css_layout_function_value.h",
     "css_markup.cc",
     "css_markup.h",
+    "css_math_operator.cc",
+    "css_math_operator.h",
     "css_media_rule.cc",
     "css_media_rule.h",
     "css_namespace_rule.cc",

third_party/blink/renderer/core/css/css_calculation_value.h

@@ -32,6 +32,7 @@
 #define THIRD_PARTY_BLINK_RENDERER_CORE_CSS_CSS_CALCULATION_VALUE_H_
 
 #include "third_party/blink/renderer/core/core_export.h"
+#include "third_party/blink/renderer/core/css/css_math_operator.h"
 #include "third_party/blink/renderer/core/css/css_primitive_value.h"
 #include "third_party/blink/renderer/core/css/css_value.h"
 #include "third_party/blink/renderer/core/css/parser/css_parser_token_range.h"
@@ -42,13 +43,6 @@ namespace blink {
 
 class CalculationValue;
 
-enum CalcOperator {
-  kCalcAdd = '+',
-  kCalcSubtract = '-',
-  kCalcMultiply = '*',
-  kCalcDivide = '/'
-};
-
 // The order of this enum should not change since its elements are used as
 // indices in the addSubtractResult matrix.
 enum CalculationCategory {
@@ -84,7 +78,7 @@ class CSSCalcExpressionNode : public GarbageCollected<CSSCalcExpressionNode> {
   virtual Type GetType() const = 0;
   virtual const CSSCalcExpressionNode* LeftExpressionNode() const = 0;
   virtual const CSSCalcExpressionNode* RightExpressionNode() const = 0;
-  virtual CalcOperator OperatorType() const = 0;
+  virtual CSSMathOperator OperatorType() const = 0;
 
   CalculationCategory Category() const { return category_; }
   virtual CSSPrimitiveValue::UnitType TypeWithCalcResolved() const = 0;
@@ -116,7 +110,7 @@ class CORE_EXPORT CSSCalcValue : public GarbageCollected<CSSCalcValue> {
                                                      bool is_integer = false);
   static CSSCalcExpressionNode* CreateExpressionNode(CSSCalcExpressionNode*,
                                                      CSSCalcExpressionNode*,
-                                                     CalcOperator);
+                                                     CSSMathOperator);
   static CSSCalcExpressionNode* CreateExpressionNode(double pixels,
                                                      double percent);
 

third_party/blink/renderer/core/css/css_calculation_value_test.cc

@@ -101,7 +101,7 @@ TEST(CSSCalculationValue, AccumulatePixelsAndPercent) {
               CSSPrimitiveValue::Create(20,
                                         CSSPrimitiveValue::UnitType::kPixels),
               true),
-          kCalcAdd),
+          CSSMathOperator::kAdd),
       150, 0);
 
   TestAccumulatePixelsAndPercent(
@@ -115,7 +115,7 @@ TEST(CSSCalculationValue, AccumulatePixelsAndPercent) {
               CSSPrimitiveValue::Create(2,
                                         CSSPrimitiveValue::UnitType::kNumber),
               true),
-          kCalcMultiply),
+          CSSMathOperator::kMultiply),
       960, 0);
 
   TestAccumulatePixelsAndPercent(
@@ -130,7 +130,7 @@ TEST(CSSCalculationValue, AccumulatePixelsAndPercent) {
                   CSSPrimitiveValue::Create(
                       0.25, CSSPrimitiveValue::UnitType::kNumber),
                   false),
-              kCalcMultiply),
+              CSSMathOperator::kMultiply),
           CSSCalcValue::CreateExpressionNode(
               CSSCalcValue::CreateExpressionNode(
                   CSSPrimitiveValue::Create(
@@ -140,8 +140,8 @@ TEST(CSSCalculationValue, AccumulatePixelsAndPercent) {
                   CSSPrimitiveValue::Create(
                       40, CSSPrimitiveValue::UnitType::kPercentage),
                   false),
-              kCalcSubtract),
-          kCalcSubtract),
+              CSSMathOperator::kSubtract),
+          CSSMathOperator::kSubtract),
       -37.5, 40);
 }
 

third_party/blink/renderer/core/css/css_math_operator.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "third_party/blink/renderer/core/css/css_math_operator.h"
+
+#include "third_party/blink/renderer/core/css/parser/css_parser_token.h"
+#include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"
+
+namespace blink {
+
+CSSMathOperator ParseCSSArithmeticOperator(const CSSParserToken& token) {
+  if (token.GetType() != kDelimiterToken)
+    return CSSMathOperator::kInvalid;
+  switch (token.Delimiter()) {
+    case '+':
+      return CSSMathOperator::kAdd;
+    case '-':
+      return CSSMathOperator::kSubtract;
+    case '*':
+      return CSSMathOperator::kMultiply;
+    case '/':
+      return CSSMathOperator::kDivide;
+    default:
+      return CSSMathOperator::kInvalid;
+  }
+}
+
+String ToString(CSSMathOperator op) {
+  switch (op) {
+    case CSSMathOperator::kAdd:
+      return "+";
+    case CSSMathOperator::kSubtract:
+      return "-";
+    case CSSMathOperator::kMultiply:
+      return "*";
+    case CSSMathOperator::kDivide:
+      return "/";
+    default:
+      NOTREACHED();
+      return String();
+  }
+}
+
+}  // namespace blink

third_party/blink/renderer/core/css/css_math_operator.h

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef THIRD_PARTY_BLINK_RENDERER_CORE_CSS_CSS_MATH_OPERATOR_H_
+#define THIRD_PARTY_BLINK_RENDERER_CORE_CSS_CSS_MATH_OPERATOR_H_
+
+#include "third_party/blink/renderer/platform/wtf/forward.h"
+
+namespace blink {
+
+class CSSParserToken;
+
+enum class CSSMathOperator { kAdd, kSubtract, kMultiply, kDivide, kInvalid };
+
+CSSMathOperator ParseCSSArithmeticOperator(const CSSParserToken& token);
+String ToString(CSSMathOperator);
+
+}  // namespace blink
+
+#endif  // THIRD_PARTY_BLINK_RENDERER_CORE_CSS_CSS_MATH_OPERATOR_H_

third_party/blink/renderer/core/css/css_primitive_value_test.cc

@@ -25,7 +25,8 @@ CSSPrimitiveValue* CreateAddition(UnitValue a, UnitValue b) {
   return CSSPrimitiveValue::Create(
       CSSCalcValue::Create(CSSCalcValue::CreateExpressionNode(
           CSSCalcValue::CreateExpressionNode(Create(a)),
-          CSSCalcValue::CreateExpressionNode(Create(b)), kCalcAdd)));
+          CSSCalcValue::CreateExpressionNode(Create(b)),
+          CSSMathOperator::kAdd)));
 }
 
 TEST(CSSPrimitiveValueTest, IsTime) {

third_party/blink/renderer/core/css/cssom/css_math_product.cc

@@ -88,11 +88,12 @@ CSSCalcExpressionNode* CSSMathProduct::ToCalcExpressionNode() const {
 
   CSSCalcExpressionNode* node = CSSCalcValue::CreateExpressionNode(
       NumericValues()[0]->ToCalcExpressionNode(),
-      NumericValues()[1]->ToCalcExpressionNode(), kCalcMultiply);
+      NumericValues()[1]->ToCalcExpressionNode(), CSSMathOperator::kMultiply);
 
   for (wtf_size_t i = 2; i < NumericValues().size(); i++) {
     node = CSSCalcValue::CreateExpressionNode(
-        node, NumericValues()[i]->ToCalcExpressionNode(), kCalcMultiply);
+        node, NumericValues()[i]->ToCalcExpressionNode(),
+        CSSMathOperator::kMultiply);
   }
 
   return node;

third_party/blink/renderer/core/css/cssom/css_math_sum.cc

@@ -106,11 +106,12 @@ CSSCalcExpressionNode* CSSMathSum::ToCalcExpressionNode() const {
 
   CSSCalcExpressionNode* node = CSSCalcValue::CreateExpressionNode(
       NumericValues()[0]->ToCalcExpressionNode(),
-      NumericValues()[1]->ToCalcExpressionNode(), kCalcAdd);
+      NumericValues()[1]->ToCalcExpressionNode(), CSSMathOperator::kAdd);
 
   for (wtf_size_t i = 2; i < NumericValues().size(); i++) {
     node = CSSCalcValue::CreateExpressionNode(
-        node, NumericValues()[i]->ToCalcExpressionNode(), kCalcAdd);
+        node, NumericValues()[i]->ToCalcExpressionNode(),
+        CSSMathOperator::kAdd);
   }
 
   return node;

third_party/blink/renderer/core/css/cssom/css_numeric_value.cc

@@ -79,10 +79,10 @@ CSSUnitValue* MaybeMultiplyAsUnitValue(const CSSNumericValueVector& values) {
   return CSSUnitValue::Create(final_value, unit_other_than_number);
 }
 
-CalcOperator CanonicalOperator(CalcOperator op) {
-  if (op == kCalcAdd || op == kCalcSubtract)
-    return kCalcAdd;
-  return kCalcMultiply;
+CSSMathOperator CanonicalOperator(CSSMathOperator op) {
+  if (op == CSSMathOperator::kAdd || op == CSSMathOperator::kSubtract)
+    return CSSMathOperator::kAdd;
+  return CSSMathOperator::kMultiply;
 }
 
 bool CanCombineNodes(const CSSCalcExpressionNode& root,
@@ -94,12 +94,12 @@ bool CanCombineNodes(const CSSCalcExpressionNode& root,
                                      CanonicalOperator(node.OperatorType());
 }
 
-CSSNumericValue* NegateOrInvertIfRequired(CalcOperator parent_op,
+CSSNumericValue* NegateOrInvertIfRequired(CSSMathOperator parent_op,
                                           CSSNumericValue* value) {
   DCHECK(value);
-  if (parent_op == kCalcSubtract)
+  if (parent_op == CSSMathOperator::kSubtract)
     return CSSMathNegate::Create(value);
-  if (parent_op == kCalcDivide)
+  if (parent_op == CSSMathOperator::kDivide)
     return CSSMathInvert::Create(value);
   return value;
 }
@@ -162,7 +162,8 @@ CSSNumericValue* CalcToNumericValue(const CSSCalcExpressionNode& root) {
   // Our algorithm collects the children in reverse order, so we have to reverse
   // the values.
   std::reverse(values.begin(), values.end());
-  if (root.OperatorType() == kCalcAdd || root.OperatorType() == kCalcSubtract)
+  if (root.OperatorType() == CSSMathOperator::kAdd ||
+      root.OperatorType() == CSSMathOperator::kSubtract)
     return CSSMathSum::Create(std::move(values));
   return CSSMathProduct::Create(std::move(values));
 }

third_party/blink/renderer/core/css/parser/sizes_calc_parser.cc

@@ -20,10 +20,10 @@ float SizesCalcParser::Result() const {
   return result_;
 }
 
-static bool OperatorPriority(UChar cc, bool& high_priority) {
-  if (cc == '+' || cc == '-')
+static bool OperatorPriority(CSSMathOperator cc, bool& high_priority) {
+  if (cc == CSSMathOperator::kAdd || cc == CSSMathOperator::kSubtract)
     high_priority = false;
-  else if (cc == '*' || cc == '/')
+  else if (cc == CSSMathOperator::kMultiply || cc == CSSMathOperator::kDivide)
     high_priority = true;
   else
     return false;
@@ -35,7 +35,8 @@ bool SizesCalcParser::HandleOperator(Vector<CSSParserToken>& stack,
   // If the token is not an operator, then return. Else determine the
   // precedence of the new operator (op1).
   bool incoming_operator_priority;
-  if (!OperatorPriority(token.Delimiter(), incoming_operator_priority))
+  if (!OperatorPriority(ParseCSSArithmeticOperator(token),
+                        incoming_operator_priority))
     return false;
 
   while (!stack.IsEmpty()) {
@@ -45,7 +46,8 @@ bool SizesCalcParser::HandleOperator(Vector<CSSParserToken>& stack,
     if (top_of_stack.GetType() != kDelimiterToken)
       break;
     bool stack_operator_priority;
-    if (!OperatorPriority(top_of_stack.Delimiter(), stack_operator_priority))
+    if (!OperatorPriority(ParseCSSArithmeticOperator(top_of_stack),
+                          stack_operator_priority))
       return false;
     // ...if op1 is left-associative (all currently supported
     // operators are) and its precedence is equal to that of op2, or
@@ -81,7 +83,7 @@ bool SizesCalcParser::AppendLength(const CSSParserToken& token) {
 
 void SizesCalcParser::AppendOperator(const CSSParserToken& token) {
   SizesCalcValue value;
-  value.operation = token.Delimiter();
+  value.operation = ParseCSSArithmeticOperator(token);
   value_list_.push_back(value);
 }
 
@@ -182,7 +184,8 @@ bool SizesCalcParser::CalcToReversePolishNotation(CSSParserTokenRange range) {
   return true;
 }
 
-static bool OperateOnStack(Vector<SizesCalcValue>& stack, UChar operation) {
+static bool OperateOnStack(Vector<SizesCalcValue>& stack,
+                           CSSMathOperator operation) {
   if (stack.size() < 2)
     return false;
   SizesCalcValue right_operand = stack.back();
@@ -191,28 +194,28 @@ static bool OperateOnStack(Vector<SizesCalcValue>& stack, UChar operation) {
   stack.pop_back();
   bool is_length;
   switch (operation) {
-    case '+':
+    case CSSMathOperator::kAdd:
       if (right_operand.is_length != left_operand.is_length)
         return false;
       is_length = (right_operand.is_length && left_operand.is_length);
       stack.push_back(
           SizesCalcValue(left_operand.value + right_operand.value, is_length));
       break;
-    case '-':
+    case CSSMathOperator::kSubtract:
       if (right_operand.is_length != left_operand.is_length)
         return false;
       is_length = (right_operand.is_length && left_operand.is_length);
       stack.push_back(
           SizesCalcValue(left_operand.value - right_operand.value, is_length));
       break;
-    case '*':
+    case CSSMathOperator::kMultiply:
       if (right_operand.is_length && left_operand.is_length)
         return false;
       is_length = (right_operand.is_length || left_operand.is_length);
       stack.push_back(
           SizesCalcValue(left_operand.value * right_operand.value, is_length));
       break;
-    case '/':
+    case CSSMathOperator::kDivide:
       if (right_operand.is_length || right_operand.value == 0)
         return false;
       stack.push_back(SizesCalcValue(left_operand.value / right_operand.value,
@@ -227,7 +230,7 @@ static bool OperateOnStack(Vector<SizesCalcValue>& stack, UChar operation) {
 bool SizesCalcParser::Calculate() {
   Vector<SizesCalcValue> stack;
   for (const auto& value : value_list_) {
-    if (value.operation == 0) {
+    if (value.operation == CSSMathOperator::kInvalid) {
       stack.push_back(value);
     } else {
       if (!OperateOnStack(stack, value.operation))

third_party/blink/renderer/core/css/parser/sizes_calc_parser.h

@@ -6,6 +6,7 @@
 #define THIRD_PARTY_BLINK_RENDERER_CORE_CSS_PARSER_SIZES_CALC_PARSER_H_
 
 #include "third_party/blink/renderer/core/core_export.h"
+#include "third_party/blink/renderer/core/css/css_math_operator.h"
 #include "third_party/blink/renderer/core/css/media_values.h"
 #include "third_party/blink/renderer/core/css/parser/css_parser_token.h"
 #include "third_party/blink/renderer/core/css/parser/css_parser_token_range.h"
@@ -15,14 +16,14 @@ namespace blink {
 
 struct SizesCalcValue {
   DISALLOW_NEW();
-  double value;
-  bool is_length;
-  UChar operation;
+  double value = 0;
+  bool is_length = false;
+  CSSMathOperator operation = CSSMathOperator::kInvalid;
 
-  SizesCalcValue() : value(0), is_length(false), operation(0) {}
+  SizesCalcValue() = default;
 
   SizesCalcValue(double numeric_value, bool length)
-      : value(numeric_value), is_length(length), operation(0) {}
+      : value(numeric_value), is_length(length) {}
 };
 
 class CORE_EXPORT SizesCalcParser {