Add support for CUBIC_TO_S for rendering vector icons

ui/gfx/paint_vector_icon.cc

Index: ui/gfx/paint_vector_icon.cc
diff --git a/ui/gfx/paint_vector_icon.cc b/ui/gfx/paint_vector_icon.cc
index 8e4b8e96dae8867f3a02ae6f700c0751df7374eb..aba857e73c48f3e92d576dae4f3619dba5d1d89e 100644
--- a/ui/gfx/paint_vector_icon.cc
+++ b/ui/gfx/paint_vector_icon.cc
@@ -48,6 +48,7 @@ CommandType CommandFromString(const std::string& source) {
   RETURN_IF_IS(R_V_LINE_TO);
   RETURN_IF_IS(CUBIC_TO);
   RETURN_IF_IS(R_CUBIC_TO);
+  RETURN_IF_IS(CUBIC_TO_S);
   RETURN_IF_IS(CIRCLE);
   RETURN_IF_IS(ROUND_RECT);
   RETURN_IF_IS(CLOSE);
@@ -91,6 +92,7 @@ void PaintPath(Canvas* canvas,
   std::vector<SkPaint> paints;
   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) {
@@ -222,9 +224,9 @@ void PaintPath(Canvas* canvas,
         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);
+        SkScalar x = path_elements[++i].arg;

[Evan Stade, 2016/09/01 20:21:22]

why the name change?

[tdanderson, 2016/09/01 20:45:00]

For naming consistency with the SVG spec.

[Evan Stade, 2016/09/01 20:53:22]

that was not my goal in choosing these variable names and I am not sure it's
worth attempting to maintain that kind of consistency

[tdanderson, 2016/09/02 16:40:49]

OK, I'll change them back.

+        SkScalar y = path_elements[++i].arg;
+        path.cubicTo(x1, y1, x2, y2, x, y);
         break;
       }
 
@@ -233,9 +235,38 @@ void PaintPath(Canvas* canvas,
         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);
+        SkScalar x = path_elements[++i].arg;
+        SkScalar y = path_elements[++i].arg;
+        path.rCubicTo(x1, y1, x2, y2, x, y);
+        break;
+      }
+
+      case CUBIC_TO_S: {

[Evan Stade, 2016/09/01 20:21:22]

we don't want/need an R_ version?

[tdanderson, 2016/09/01 20:45:00]

That case is handled in the skiafy script.

[Evan Stade, 2016/09/01 20:53:22]

ok. Could you name it SHORTHAND_CUBIC_TO or something more obvious than S?

[tdanderson, 2016/09/02 16:40:49]

Done.

+        // 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.
+        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_S) {
+          delta_x = path_elements[i - 2].arg - path_elements[i - 4].arg;

[Evan Stade, 2016/09/01 20:21:22]

this makes me a little uncomfortable. Can we use SkPath::getPoint to get the
last couple points instead of just getLastPt?

[tdanderson, 2016/09/01 20:45:00]

What worries you about this? Even if getPoint() is used, we'd still need two
array-accesses for the second control point on the previous command; as I
understand it the control points are not considered points on the path.

[Evan Stade, 2016/09/01 20:53:22]

I dunno, I just dislike negative signs in an array access...
they aren't? That's surprising. What do the points on the path look like? This
makes it look like all the control points you pass are added to the path:
https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkPath.cpp?rcl...

[tdanderson, 2016/09/02 16:40:49]

Sorry, it looks like I was mistaken. Changed in the next patch set.

+          delta_y = path_elements[i - 1].arg - path_elements[i - 3].arg;
+        }
+
+        SkPoint last_point;
+        path.getLastPt(&last_point);
+
+        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 x = path_elements[++i].arg;
+        SkScalar y = path_elements[++i].arg;
+        path.cubicTo(x1, y1, x2, y2, x, y);
         break;
       }
 
@@ -291,6 +322,8 @@ void PaintPath(Canvas* canvas,
         NOTREACHED();
         break;
     }
+
+    previous_command_type = command_type;
   }
 
   gfx::ScopedRTLFlipCanvas scoped_rtl_flip_canvas(canvas, canvas_size,