[webkit-reviews] review denied: [Bug 104616] Switch the gradient drawing code to use bearing angles : [Attachment 179096] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has denied Tab Atkins <tabatkins at google.com>'s
request for review:
Bug 104616: Switch the gradient drawing code to use bearing angles
https://bugs.webkit.org/show_bug.cgi?id=104616

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

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=179096&action=review


> Source/WebCore/ChangeLog:13
> +	   * css/CSSGradientValue.cpp:
> +	   (WebCore::CSSLinearGradientValue::customCssText):

Might as well as per-function comments here since it is such an easy change to
describe.

> Source/WebCore/css/CSSGradientValue.cpp:586
> +    // angleDeg is a "bearing angle" (0deg = N, 90deg = E),
> +    // but tan expects 0deg = E, 90deg = N

Nit: comments end with "."

> Source/WebCore/css/CSSGradientValue.cpp:593
> +    // Compute start corner relative to center, in cartesian space (+y = up)


Ditto. Extra bonus nit: Cartesian starts with uppercase.

> Source/WebCore/css/CSSGradientValue.cpp:612
> +    // We computed the end point, so set the second point, 
> +    // taking into account the moved origin and the fact that we're in
drawing space (+y = down)

Ditto.

> Source/WebCore/css/CSSGradientValue.h:136
>      void setAngle(PassRefPtr<CSSPrimitiveValue> val) { m_angle = val; }
> +    void setPrefixedAngle(PassRefPtr<CSSPrimitiveValue> val) {
m_prefixedAngle = val; }

I'm concerned that this exposes a way for the two angles to get out of sync. I
guess the long term plan is that we always accept prefixed angles, but why not
have setAngle automatically call setPrefixedAngle? I realise this means your
helper function would have to move.

> Source/WebCore/css/CSSParser.cpp:7163
> +	   convertBetweenBearingAngleAndPolarCoordinateAngle(a);

I don't like how this modifies in place, even for a temporary variable. I think
changing setAngle to do both values would be the best.