[webkit-reviews] review denied: [Bug 137171] Add support for midpoint to CSS gradients : [Attachment 238787] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Rik Cabanier <cabanier at adobe.com>'s
request for review:
Bug 137171: Add support for midpoint to CSS gradients
https://bugs.webkit.org/show_bug.cgi?id=137171

Attachment 238787: Patch
https://bugs.webkit.org/attachment.cgi?id=238787&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=238787&action=review


Looks fine. Needs a little work before landing.

Test coverage doesn’t seem sufficient. I could make the math that generates the
stops completely wrong and the tests would still pass, I think. How can we test
this code?

> Source/WebCore/css/CSSGradientValue.cpp:98
> +    bool isMidPoint;

Since midpoint is a word, not the two words mid and point, this should be
isMidpoint.

> Source/WebCore/css/CSSGradientValue.cpp:259
> +    // else add 6 stop to the left and 2 to the right.

6 stops

> Source/WebCore/css/CSSGradientValue.cpp:260
> +    // Stops on the lesser side start midway so color shifts are minimized

Missing period here.

This comment is long on what the code does does, but short on why. Why is it a
good idea to add 2 stops to the left and 6 to the right? Is that a standard? A
specific trick for rendering well?

> Source/WebCore/css/CSSGradientValue.cpp:263
> +    size_t x = 1;
> +    while (x < stops.size() - 1)

Even fi the incrementing is done inside the loop, I would prefer we use a for
lop here rather than a while loop. We can just leave the third expression
empty.

> Source/WebCore/css/CSSGradientValue.cpp:264
> +	   if (stops[x].isMidPoint) {

Early continue style would make this easier to read.

    if (!stops[x].isMidpoint) {
	++x;
	continue;
    }

    ...

> Source/WebCore/css/CSSGradientValue.cpp:266
> +	       // find previous color

This is not a good comment. Below there is code that sets up many variables,
and "find previous color" describes only the first line.

> Source/WebCore/css/CSSGradientValue.cpp:270
> +	       Color color1 = stops[x-1].color;
> +	       Color color2 = stops[x+1].color;
> +	       float offset1 = stops[x-1].offset;
> +	       float offset2 = stops[x+1].offset;

WebKit coding style puts spaces around the "-" and "+" operators.

> Source/WebCore/css/CSSGradientValue.cpp:273
> +	       Vector<GradientStop> newStops(9);

Something with a fixed size like this should be an array, not a Vector. There
is no need to allocate this on the heap.

    GradientStop newStops[9];

> Source/WebCore/css/CSSGradientValue.cpp:276
> +		       newStops[y-1].offset = offset1 + (offset - offset1) * (6
+ y) / 13;

WebKit coding style puts spaces around the "-" operator in "y - 1".

> Source/WebCore/css/CSSGradientValue.cpp:284
> +		   for (size_t y = 0; y < 7; y++)

The loop above uses 1-7, and this uses 0-6. I don’t understand why we made
those choices. I think 0-6 for both would be better, but maybe there's
something I am missing.

> Source/WebCore/css/CSSGradientValue.cpp:293
> +		   int red = color1.red() + pow(relativeOffset, log(.5f) /
log(midpoint)) * (color2.red() - color1.red());
> +		   int green = color1.green() + pow(relativeOffset, log(.5f) /
log(midpoint)) * (color2.green() - color1.green());
> +		   int blue = color1.blue() + pow(relativeOffset, log(.5f) /
log(midpoint)) * (color2.blue() - color1.blue());
> +		   int alpha = color1.alpha() + pow(relativeOffset, log(.5f) /
log(midpoint)) * (color2.alpha() - color1.alpha());

This code is converting from float to double. Is that intentional? If we want
this to stay float, then it needs to be powf and logf, or maybe some modern
overloaded C++ function.

I don’t think we should compute "pow(relativeOffset, log(.5f) / log(midpoint))"
four times. The compiler might be able to find the common subexpression, but I
think we should put it into a local variable for clarity.

> Source/WebCore/css/CSSGradientValue.cpp:295
> +		   newStops[y].color = Color(red, green, blue, alpha);

The code to compute the new color would read better if it was written as a
helper function. We should consider doing that.

> Source/WebCore/css/CSSGradientValue.h:51
> +    CSSGradientColorStop() : m_colorIsDerivedFromElement(false),
m_isMidPoint(false) { };

The trailing semicolon here is incorrect. Please remove it.

Better to set m_isMidpoint to false using the new modern C++ syntax rather than
in the constructor. We can just write:

    bool m_isMidpoint = false;

Below.

> Source/WebCore/css/CSSGradientValue.h:56
>      RefPtr<CSSPrimitiveValue> m_position; // percentage or length
>      RefPtr<CSSPrimitiveValue> m_color;
>      Color m_resolvedColor;
>      bool m_colorIsDerivedFromElement;
> +    bool m_isMidPoint;

Since this is a struct, these should not have the m_ prefix. Not new to this
patch, but worth fixing.

> Source/WebCore/css/CSSParser.cpp:8599
> +    bool previousStopWasMidPoint = true;

Again, midpoint is a single word, so should be WasMidpoint.

> Source/WebCore/css/CSSParser.cpp:8638
> +    // we can't end on a midpoint

Comments should start with a capital letter and end with a period in WebKit
code.