Initial conic-gradient() implementation

third_party/WebKit/Source/core/css/CSSGradientValue.cpp

Index: third_party/WebKit/Source/core/css/CSSGradientValue.cpp
diff --git a/third_party/WebKit/Source/core/css/CSSGradientValue.cpp b/third_party/WebKit/Source/core/css/CSSGradientValue.cpp
index 9452b23cf42b65f8a242fc67c791345d2dcf0d58..a2e88680394919f53ba57c97ea1ff4d5e52c413f 100644
--- a/third_party/WebKit/Source/core/css/CSSGradientValue.cpp
+++ b/third_party/WebKit/Source/core/css/CSSGradientValue.cpp
@@ -459,15 +459,22 @@ void CSSGradientValue::addStops(CSSGradientValue::GradientDesc& desc,
   Vector<GradientStop> stops(numStops);
 
   bool hasHints = false;

[fs, 2017/03/30 22:09:11]

Nit: Maybe move this down to just before the for() while we're at it.

[f(malita), 2017/03/31 13:56:23]

Done.

+  float gradientLength;
 
-  FloatPoint gradientStart = desc.p0;
-  FloatPoint gradientEnd;
-  if (isLinearGradientValue())
-    gradientEnd = desc.p1;
-  else if (isRadialGradientValue())
-    gradientEnd = gradientStart + FloatSize(desc.r1, 0);
-  float gradientLength =
-      FloatSize(gradientStart - gradientEnd).diagonalLength();
+  switch (getClassType()) {

[fs, 2017/03/30 22:09:11]

(The old code here had me scratch my head...)

+    case LinearGradientClass:
+      gradientLength = FloatSize(desc.p1 - desc.p0).diagonalLength();
+      break;
+    case RadialGradientClass:
+      gradientLength = desc.r1;
+      break;
+    case ConicGradientClass:
+      gradientLength = 1;
+      break;
+    default:
+      NOTREACHED();
+      gradientLength = 0;
+  }
 
   for (size_t i = 0; i < numStops; ++i) {
     const CSSGradientColorStop& stop = m_stops[i];
@@ -566,28 +573,37 @@ void CSSGradientValue::addStops(CSSGradientValue::GradientDesc& desc,
 
   // At this point we have a fully resolved set of stops. Time to perform
   // adjustments for repeat gradients and degenerate values if needed.
-  if (requiresStopsNormalization(stops, desc)) {
-    // Negative offsets are only an issue for non-repeating radial gradients:
-    // linear gradient points can be repositioned arbitrarily, and for repeating
-    // radial gradients we shift the radii into equivalent positive values.
-    if (isRadialGradientValue() && !m_repeating)
-      clampNegativeOffsets(stops);
-
-    if (normalizeAndAddStops(stops, desc)) {
-      if (isLinearGradientValue()) {
+  // Note: the normalization trick doesn't work for conic gradients, because
+  // the underlying Skia implementation doesn't support tiling.
+  if (isConicGradientValue() || !requiresStopsNormalization(stops, desc)) {
+    // No normalization required, just add the current stops.
+    for (const auto& stop : stops)
+      desc.stops.emplace_back(stop.offset, stop.color);
+    return;
+  }
+
+  switch (getClassType()) {
+    case LinearGradientClass:
+      if (normalizeAndAddStops(stops, desc)) {
         adjustGradientPointsForOffsetRange(desc, stops.front().offset,
                                            stops.back().offset);
-      } else {
+      }
+      break;
+    case RadialGradientClass:
+      // Negative offsets are only an issue for non-repeating radial gradients:
+      // linear gradient points can be repositioned arbitrarily, and for
+      // repeating radial gradients we shift the radii into equivalent positive
+      // values.
+      if (!m_repeating)
+        clampNegativeOffsets(stops);
+
+      if (normalizeAndAddStops(stops, desc)) {
         adjustGradientRadiiForOffsetRange(desc, stops.front().offset,
                                           stops.back().offset);
-      }
-    } else {
-      // Normalization failed because the stop set is coincident.
     }

[fs, 2017/03/30 22:09:11]

Strange indent (here and next line)? (Or late hour?)

[f(malita), 2017/03/31 13:56:23]

Done.

-  } else {
-    // No normalization required, just add the current stops.
-    for (const auto& stop : stops)
-      desc.stops.emplace_back(stop.offset, stop.color);
+    break;
+    default:
+      NOTREACHED();
   }
 }
 
@@ -1368,8 +1384,22 @@ PassRefPtr<Gradient> CSSConicGradientValue::createGradient(
     const LayoutObject& object) {
   DCHECK(!size.isEmpty());
 
-  // TODO(fmalita): implement
-  return Gradient::createLinear(FloatPoint(), FloatPoint());
+  const float angle = m_fromAngle ? m_fromAngle->computeDegrees() : 0;
+
+  const FloatPoint position(
+      m_firstX ? positionFromValue(m_firstX, conversionData, size, true)
+               : size.width() / 2,
+      m_firstY ? positionFromValue(m_firstY, conversionData, size, false)
+               : size.height() / 2);
+
+  GradientDesc desc(position, position,
+                    m_repeating ? SpreadMethodRepeat : SpreadMethodPad);
+  addStops(desc, conversionData, object);
+
+  RefPtr<Gradient> gradient = Gradient::createConic(position, angle);

[fs, 2017/03/30 22:09:11]

position, angle, ..., Gradient::ColorInterpolation::Premultiplied

right? Whether ... is desc.spreadMethod or not I guess doesn't matter much for
now, but maybe best to use that.

(Maybe this also means we should have a test for stops with alpha? or
'transparent' stops)

[f(malita), 2017/03/31 13:56:23]

Good catch, done.
The last conic-gradient.html example uses some translucent colors and does catch
this diff.

[fs, 2017/03/31 14:05:43]

Yeah, I saw that... it's very subtle though. So maybe a more "plain" test would
be good to have - like "conic-gradient(blue, transparent)", where the effect
ought to be fairly noticable.

+  gradient->addColorStops(desc.stops);
+
+  return gradient.release();
 }
 
 bool CSSConicGradientValue::equals(const CSSConicGradientValue& other) const {