Refactor some vector icon parsing code for reuse

ui/gfx/paint_vector_icon.cc

 // Copyright 2015 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 "ui/gfx/paint_vector_icon.h"

[tdanderson, 2017/05/08 20:13:20]

general comment for follow-up CL(s): I think the addition of unit testing to the
vector icon code would be a good idea. Have you given any thought to that?

[Evan Stade, 2017/05/08 22:13:37]

Did you have something in mind that you think is important to test?

I like to write tests for things that have a high chance of regressing and/or
not being noticed when regressing. I don't think vector icons have either of
these attributes. Have you seen many, if any, regressions in the routines in
this file?

The other benefit of unit tests is that they provide greater assurance that your
changes still prove functional when you're refactoring or otherwise overhauling
existing code. But they create the burden of updating them when you're making
tweaks to the code, which is often more work than changing the code itself. I
don't think it's worthwhile to write a comprehensive test suite that will
probably just get in the way for a refactoring that's that the current date
purely hypothetical.

I also buy into the notion that tests, especially poorly written ones, create a
false sense of security which, overall, is harmful. Thus if there is not a clear
and present need for tests I err on the side of not writing them.

Also many mistakes made during development are caught by the DCHECKs sprinkled
throughout this file, so our implicit test coverage (chances of turning some bot
red due to an error) is not zero just because explicit test coverage is zero.

[tdanderson, 2017/05/09 16:42:49]

Thank you for sharing this philosophy. I will take an AI to read over the vector
icon code in more detail with testing in mind, and will follow up on this with
you in a separate discussion.

 
 #include <map>
 #include <tuple>
 
 #include "base/i18n/rtl.h"
 #include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "cc/paint/paint_canvas.h"
 #include "cc/paint/paint_flags.h"
 #include "third_party/skia/include/core/SkPath.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/image/canvas_image_source.h"
 #include "ui/gfx/scoped_canvas.h"
 #include "ui/gfx/vector_icon_types.h"
 
 namespace gfx {
 
 namespace {
 
+// Helper that simplifies iterating over a sequence of PathElements.

[tdanderson, 2017/05/08 20:13:20]

optional: "...of the form 'command, (argument,)*'" or similar?

[Evan Stade, 2017/05/08 22:13:37]

shrug, I can't think of any extra wording that adds much clarity.

[tdanderson, 2017/05/09 16:42:49]

Acknowledged.

+class PathParser {
+ public:
+  PathParser(const PathElement* path_elements)
+      : path_elements_(path_elements) {}
+  ~PathParser() {}
+
+  bool Advance() {
+    if (command_index_ < 0)

[tdanderson, 2017/05/08 20:13:20]

From a usage point of view I would expect that: 

PathParser parser(path_elements);
const CommandType command_type = parser.CurrentCommand();

would set |command_type| as the first thing in |path_elements|. It seems a bit
strange that I need to call Advance() once before being able to read the first
element.

[Evan Stade, 2017/05/08 22:13:37]

simplified this and the loop below. Once again proving that while loops are just
poorly-written for loops.

+      command_index_ = 0;
+    else if (CurrentCommand() != END)
+      command_index_ += GetArgumentCount() + 1;
+    return CurrentCommand() != END;
+  }
+
+  CommandType CurrentCommand() const {
+    return path_elements_[command_index_].command;
+  }
+
+  SkScalar GetArgument(int index) const {

[tdanderson, 2017/05/08 20:13:20]

Consider GetArgumentOfCurrentCommand() for increased clarity here.

[Evan Stade, 2017/05/08 22:13:37]

Acknowledged.

+    DCHECK_LT(index, GetArgumentCount());
+    return path_elements_[command_index_ + 1 + index].arg;
+  }
+
+ private:
+  int GetArgumentCount() const {
+    switch (CurrentCommand()) {
+      case STROKE:
+      case H_LINE_TO:
+      case R_H_LINE_TO:
+      case V_LINE_TO:
+      case R_V_LINE_TO:
+      case CANVAS_DIMENSIONS:
+        return 1;
+
+      case MOVE_TO:
+      case R_MOVE_TO:
+      case LINE_TO:
+      case R_LINE_TO:
+        return 2;
+
+      case CIRCLE:
+        return 3;
+
+      case PATH_COLOR_ARGB:
+      case CUBIC_TO_SHORTHAND:
+      case CLIP:
+        return 4;
+
+      case ROUND_RECT:
+        return 5;
+
+      case CUBIC_TO:
+      case R_CUBIC_TO:
+        return 6;
+
+      case ARC_TO:
+      case R_ARC_TO:
+        return 7;
+
+      case NEW_PATH:

[tdanderson, 2017/05/08 20:13:20]

nit: move the 0-argument block above the 1-argument block so the blocks are in
order

[Evan Stade, 2017/05/08 22:13:37]

I like this here because it's what I consider a default case, and default
usually comes at the end (of course we don't have an actual |default| for the
normal reason). Also this matches the order on my keyboard number row. Why do
you argue it should be at the top?

[tdanderson, 2017/05/09 16:42:49]

I am not sure how that is related. If I had a switch for chars, I would not
order the cases as 'q', 'w', 'e', 'r', ... to match physical keyboard key
ordering.
To me the most readable organization here is for the return values to be in
increasing order. But as you have explained above, you feel it is more readable
when 0 is treated as 'default'. Since neither way is *un*readable, leaving it
as-is works for me.

+      case PATH_MODE_CLEAR:
+      case CAP_SQUARE:
+      case CLOSE:
+      case DISABLE_AA:
+      case FLIPS_IN_RTL:
+      case END:
+        return 0;
+    }
+
+    NOTREACHED();
+    return 0;
+  }
+
+  const PathElement* path_elements_;
+  int command_index_ = -1;
+
+  DISALLOW_COPY_AND_ASSIGN(PathParser);
+};
+
 // Translates a string such as "MOVE_TO" into a command such as MOVE_TO.
 CommandType CommandFromString(const std::string& source) {
 #define RETURN_IF_IS(command) \
   if (source == #command)     \
     return command;
 
   RETURN_IF_IS(NEW_PATH);
   RETURN_IF_IS(PATH_COLOR_ARGB);
   RETURN_IF_IS(PATH_MODE_CLEAR);
   RETURN_IF_IS(STROKE);
@@ skipping 49 matching lines @@
   SkPath path;
   path.setFillType(SkPath::kEvenOdd_FillType);
 
   int canvas_size = kReferenceSizeDip;
   std::vector<SkPath> paths;
   std::vector<cc::PaintFlags> flags_array;
   SkRect clip_rect = SkRect::MakeEmpty();
   bool flips_in_rtl = false;
   CommandType previous_command_type = NEW_PATH;
 
-  for (size_t i = 0; path_elements[i].type != END; i++) {
-    if (paths.empty() || path_elements[i].type == NEW_PATH) {
+  PathParser parser(path_elements);
+  auto arg = [&parser](int i) { return parser.GetArgument(i); };
+
+  while (parser.Advance()) {
+    const CommandType command_type = parser.CurrentCommand();
+    if (paths.empty() || command_type == NEW_PATH) {
       paths.push_back(SkPath());
       paths.back().setFillType(SkPath::kEvenOdd_FillType);
 
       flags_array.push_back(cc::PaintFlags());
       flags_array.back().setColor(color);
       flags_array.back().setAntiAlias(true);
       flags_array.back().setStrokeCap(cc::PaintFlags::kRound_Cap);
     }
 
     SkPath& path = paths.back();
     cc::PaintFlags& flags = flags_array.back();
-    CommandType command_type = path_elements[i].type;
     switch (command_type) {
       // Handled above.
       case NEW_PATH:
-        continue;
+        break;
 
-      case PATH_COLOR_ARGB: {
-        int a = SkScalarFloorToInt(path_elements[++i].arg);
-        int r = SkScalarFloorToInt(path_elements[++i].arg);
-        int g = SkScalarFloorToInt(path_elements[++i].arg);
-        int b = SkScalarFloorToInt(path_elements[++i].arg);
-        flags.setColor(SkColorSetARGB(a, r, g, b));
+      case PATH_COLOR_ARGB:
+        flags.setColor(SkColorSetARGB(
+            SkScalarFloorToInt(arg(0)), SkScalarFloorToInt(arg(1)),
+            SkScalarFloorToInt(arg(2)), SkScalarFloorToInt(arg(3))));
         break;
-      }
 
-      case PATH_MODE_CLEAR: {
+      case PATH_MODE_CLEAR:
         flags.setBlendMode(SkBlendMode::kClear);
         break;
-      };
 
-      case STROKE: {
+      case STROKE:
         flags.setStyle(cc::PaintFlags::kStroke_Style);
-        SkScalar width = path_elements[++i].arg;
-        flags.setStrokeWidth(width);
+        flags.setStrokeWidth(arg(0));
         break;
-      }
 
-      case CAP_SQUARE: {
+      case CAP_SQUARE:
         flags.setStrokeCap(cc::PaintFlags::kSquare_Cap);
         break;
-      }
 
-      case MOVE_TO: {
-        SkScalar x = path_elements[++i].arg;
-        SkScalar y = path_elements[++i].arg;
-        path.moveTo(x, y);
+      case MOVE_TO:
+        path.moveTo(arg(0), arg(1));
         break;
-      }
 
-      case R_MOVE_TO: {
+      case R_MOVE_TO:
         if (previous_command_type == CLOSE) {
           // This triggers injectMoveToIfNeeded() so that the next subpath
           // will start at the correct place. See [
           // https://www.w3.org/TR/SVG/paths.html#PathDataClosePathCommand ].
           path.rLineTo(0, 0);
         }
 
-        SkScalar x = path_elements[++i].arg;
-        SkScalar y = path_elements[++i].arg;
-        path.rMoveTo(x, y);
+        path.rMoveTo(arg(0), arg(1));
         break;
-      }
 
       case ARC_TO:
       case R_ARC_TO: {
-        SkScalar rx = path_elements[++i].arg;
-        SkScalar ry = path_elements[++i].arg;
-        SkScalar angle = path_elements[++i].arg;
-        SkScalar large_arc_flag = path_elements[++i].arg;
-        SkScalar arc_sweep_flag = path_elements[++i].arg;
-        SkScalar x = path_elements[++i].arg;
-        SkScalar y = path_elements[++i].arg;
+        SkScalar rx = arg(0);
+        SkScalar ry = arg(1);
+        SkScalar angle = arg(2);
+        SkScalar large_arc_flag = arg(3);
+        SkScalar arc_sweep_flag = arg(4);
+        SkScalar x = arg(5);
+        SkScalar y = arg(6);
 
         auto path_fn =
             command_type == ARC_TO
                 ? static_cast<void (SkPath::*)(
                       SkScalar, SkScalar, SkScalar, SkPath::ArcSize,
                       SkPath::Direction, SkScalar, SkScalar)>(&SkPath::arcTo)
                 : &SkPath::rArcTo;
         (path.*path_fn)(
             rx, ry, angle,
             large_arc_flag ? SkPath::kLarge_ArcSize : SkPath::kSmall_ArcSize,
             arc_sweep_flag ? SkPath::kCW_Direction : SkPath::kCCW_Direction, x,
             y);
         break;
       }
 
-      case LINE_TO: {
-        SkScalar x = path_elements[++i].arg;
-        SkScalar y = path_elements[++i].arg;
-        path.lineTo(x, y);
+      case LINE_TO:
+        path.lineTo(arg(0), arg(1));
         break;
-      }
 
-      case R_LINE_TO: {
-        SkScalar x = path_elements[++i].arg;
-        SkScalar y = path_elements[++i].arg;
-        path.rLineTo(x, y);
+      case R_LINE_TO:
+        path.rLineTo(arg(0), arg(1));
         break;
-      }
 
       case H_LINE_TO: {
         SkPoint last_point;
         path.getLastPt(&last_point);
-        SkScalar x = path_elements[++i].arg;
-        path.lineTo(x, last_point.fY);
+        path.lineTo(arg(0), last_point.fY);
         break;
       }
 
-      case R_H_LINE_TO: {
-        SkScalar x = path_elements[++i].arg;
-        path.rLineTo(x, 0);
+      case R_H_LINE_TO:
+        path.rLineTo(arg(0), 0);
         break;
-      }
 
       case V_LINE_TO: {
         SkPoint last_point;
         path.getLastPt(&last_point);
-        SkScalar y = path_elements[++i].arg;
-        path.lineTo(last_point.fX, y);
+        path.lineTo(last_point.fX, arg(0));
         break;
       }
 
-      case R_V_LINE_TO: {
-        SkScalar y = path_elements[++i].arg;
-        path.rLineTo(0, y);
+      case R_V_LINE_TO:
+        path.rLineTo(0, arg(0));
         break;
-      }
 
-      case CUBIC_TO: {
-        SkScalar x1 = path_elements[++i].arg;
-        SkScalar y1 = path_elements[++i].arg;
-        SkScalar x2 = path_elements[++i].arg;
-        SkScalar y2 = path_elements[++i].arg;
-        SkScalar x3 = path_elements[++i].arg;
-        SkScalar y3 = path_elements[++i].arg;
-        path.cubicTo(x1, y1, x2, y2, x3, y3);
+      case CUBIC_TO:
+        path.cubicTo(arg(0), arg(1), arg(2), arg(3), arg(4), arg(5));
         break;
-      }
 
-      case R_CUBIC_TO: {
-        SkScalar x1 = path_elements[++i].arg;
-        SkScalar y1 = path_elements[++i].arg;
-        SkScalar x2 = path_elements[++i].arg;
-        SkScalar y2 = path_elements[++i].arg;
-        SkScalar x3 = path_elements[++i].arg;
-        SkScalar y3 = path_elements[++i].arg;
-        path.rCubicTo(x1, y1, x2, y2, x3, y3);
+      case R_CUBIC_TO:
+        path.rCubicTo(arg(0), arg(1), arg(2), arg(3), arg(4), arg(5));
         break;
-      }
 
       case CUBIC_TO_SHORTHAND: {
         // Compute the first control point (|x1| and |y1|) as the reflection
         // of the second control point on the previous command relative to
         // the current point. If there is no previous command or if the
         // previous command is not a cubic Bezier curve, the first control
         // point is coincident with the current point. Refer to the SVG
         // path specs for further details.
         SkPoint last_point;
         path.getLastPt(&last_point);
         SkScalar delta_x = 0;
         SkScalar delta_y = 0;
         if (previous_command_type == CUBIC_TO ||
             previous_command_type == R_CUBIC_TO ||
             previous_command_type == CUBIC_TO_SHORTHAND) {
           SkPoint last_control_point = path.getPoint(path.countPoints() - 2);
           delta_x = last_point.fX - last_control_point.fX;
           delta_y = last_point.fY - last_control_point.fY;
         }
 
         SkScalar x1 = last_point.fX + delta_x;
         SkScalar y1 = last_point.fY + delta_y;
-        SkScalar x2 = path_elements[++i].arg;
-        SkScalar y2 = path_elements[++i].arg;
-        SkScalar x3 = path_elements[++i].arg;
-        SkScalar y3 = path_elements[++i].arg;
-        path.cubicTo(x1, y1, x2, y2, x3, y3);
+        path.cubicTo(x1, y1, arg(0), arg(1), arg(2), arg(3));
         break;
       }
 
-      case CIRCLE: {
-        SkScalar x = path_elements[++i].arg;
-        SkScalar y = path_elements[++i].arg;
-        SkScalar r = path_elements[++i].arg;
-        path.addCircle(x, y, r);
+      case CIRCLE:
+        path.addCircle(arg(0), arg(1), arg(2));
         break;
-      }
 
-      case ROUND_RECT: {
-        SkScalar x = path_elements[++i].arg;
-        SkScalar y = path_elements[++i].arg;
-        SkScalar w = path_elements[++i].arg;
-        SkScalar h = path_elements[++i].arg;
-        SkScalar radius = path_elements[++i].arg;
-        path.addRoundRect(SkRect::MakeXYWH(x, y, w, h), radius, radius);
+      case ROUND_RECT:
+        path.addRoundRect(SkRect::MakeXYWH(arg(0), arg(1), arg(2), arg(3)),
+                          arg(4), arg(4));
         break;
-      }
 
-      case CLOSE: {
+      case CLOSE:
         path.close();
         break;
-      }
 
-      case CANVAS_DIMENSIONS: {
-        SkScalar width = path_elements[++i].arg;
-        canvas_size = SkScalarTruncToInt(width);
+      case CANVAS_DIMENSIONS:
+        canvas_size = SkScalarTruncToInt(arg(0));
         break;
-      }
 
-      case CLIP: {
-        SkScalar x = path_elements[++i].arg;
-        SkScalar y = path_elements[++i].arg;
-        SkScalar w = path_elements[++i].arg;
-        SkScalar h = path_elements[++i].arg;
-        clip_rect = SkRect::MakeXYWH(x, y, w, h);
+      case CLIP:
+        clip_rect = SkRect::MakeXYWH(arg(0), arg(1), arg(2), arg(3));
         break;
-      }
 
-      case DISABLE_AA: {
+      case DISABLE_AA:
         flags.setAntiAlias(false);
         break;
-      }
 
-      case FLIPS_IN_RTL: {
+      case FLIPS_IN_RTL:
         flips_in_rtl = true;
         break;
-      }
 
       case END:
         NOTREACHED();
         break;
     }
 
     previous_command_type = command_type;
   }
 
   gfx::ScopedRTLFlipCanvas scoped_rtl_flip_canvas(canvas, canvas_size,
@@ skipping 151 matching lines @@
 
 ImageSkia CreateVectorIconFromSource(const std::string& source,
                                      int dip_size,
                                      SkColor color) {
   return CanvasImageSource::MakeImageSkia<VectorIconSource>(source, dip_size,
                                                             color);
 }
 
 int GetDefaultSizeOfVectorIcon(const gfx::VectorIcon& icon) {
   const PathElement* one_x_path = icon.path_1x_ ? icon.path_1x_ : icon.path_;
-  return one_x_path[0].type == CANVAS_DIMENSIONS ? one_x_path[1].arg
-                                                 : kReferenceSizeDip;
+  return one_x_path[0].command == CANVAS_DIMENSIONS ? one_x_path[1].arg
+                                                    : kReferenceSizeDip;
 }
 
 }  // namespace gfx