Bug 104616 – Switch the gradient drawing code to use bearing angles

• Component: CSS
• Status: RESOLVED FIXED

[Tab Atkins]

As the new linear-gradient() syntax uses bearing angles rather than polar-coordinate angles, we should switch the way the drawing code handles angles to be consistent with it, so as to avoid accidental errors later.

parseDeprecatedLinearGradient() has to be fixed up accordingly, so that it continues to accept polar-coord angles.

[Tab Atkins]

Created attachment 178836 [details]
Patch

[WebKit Review Bot]

Comment on attachment 178836 [details]
Patch

Attachment 178836 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15257777

New failing tests:
fast/gradients/css3-linear-angle-gradients.html
fast/gradients/css3-gradient-parsing.html

[WebKit Review Bot]

Comment on attachment 178836 [details]
Patch

Attachment 178836 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15242946

New failing tests:
fast/gradients/css3-linear-angle-gradients.html
fast/gradients/css3-gradient-parsing.html

[Build Bot]

Comment on attachment 178836 [details]
Patch

Attachment 178836 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15272628

New failing tests:
fast/gradients/css3-gradient-parsing.html

[Tab Atkins]

Created attachment 179096 [details]
Patch

[Dean Jackson]

Comment on attachment 179096 [details]
Patch

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.

[Simon Fraser (smfr)]

Comment on attachment 179096 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179096&action=review

> Source/WebCore/css/CSSGradientValue.h:163
>      RefPtr<CSSPrimitiveValue> m_angle; // may be null.
> +    RefPtr<CSSPrimitiveValue> m_prefixedAngle; // may be null. Used in -webkit-linear-gradient().

Gross. Why not use a flag saying that this CSSGradientValue was generated via the prefixed property?

[Tab Atkins]

(In reply to comment #7)
> (From update of attachment 179096 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179096&action=review
> 
> > Source/WebCore/css/CSSGradientValue.h:163
> >      RefPtr<CSSPrimitiveValue> m_angle; // may be null.
> > +    RefPtr<CSSPrimitiveValue> m_prefixedAngle; // may be null. Used in -webkit-linear-gradient().
> 
> Gross. Why not use a flag saying that this CSSGradientValue was generated via the prefixed property?

I tried that, but it means I have to construct a fresh CSSPrimitiveValue for the prefixed angle on every invocation of customCSS(), so I can call cssText() on it.

[Tab Atkins]

(In reply to comment #6)

Comments fixed.

> > 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.

Sure, but I'm not sure how best to do this.  If it happens in setAngle, that means I don't have access to the CSSParserValue anymore, and have to construct the CSSPrimitiveValue myself.  I think this means I'll have to pull in another file, no?

[Dean Jackson]

(In reply to comment #9)
> > > 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.
> 
> Sure, but I'm not sure how best to do this.  If it happens in setAngle, that means I don't have access to the CSSParserValue anymore, and have to construct the CSSPrimitiveValue myself.  I think this means I'll have to pull in another file, no?

Yeah, this is pretty annoying. 

Apologies if I haven't thought this through, but since this patch is sort-of doing two things at once (updating the drawing code and keeping track of the adjusted input angle), why not do it all at once and just update CSSLinearGradient to be bearing-angle everywhere? parseDeprecatedLinearGradient can do the conversion. Our cssText() output will change, I realise.

[Tab Atkins]

(In reply to comment #10)
> (In reply to comment #9)
> > > > 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.
> > 
> > Sure, but I'm not sure how best to do this.  If it happens in setAngle, that means I don't have access to the CSSParserValue anymore, and have to construct the CSSPrimitiveValue myself.  I think this means I'll have to pull in another file, no?
> 
> Yeah, this is pretty annoying. 
> 
> Apologies if I haven't thought this through, but since this patch is sort-of doing two things at once (updating the drawing code and keeping track of the adjusted input angle), why not do it all at once and just update CSSLinearGradient to be bearing-angle everywhere? parseDeprecatedLinearGradient can do the conversion. Our cssText() output will change, I realise.

My intention is that it's doing one thing (updating the angle we use, with appropriate bodges to make the legacy stuff work right).  ^_^

I can't change -webkit-linear-gradient() to use bearing angle, unfortunately - it'll break *way* too much stuff as horizontal gradients become vertical, etc.

I think I've got the necessary fixes together now, though - I'll upload momentarily.

[Tab Atkins]

Created attachment 179173 [details]
Patch

[Tab Atkins]

(In reply to comment #12)
> Created an attachment (id=179173) [details]
> Patch

Okay, took a completely different and much better approach.  I've just upgraded the m_deprecatedType boolean to an m_gradientType enum, and then used that to correct the behavior in the endPointsFromAngle() function.

This should make both of you happy, because I'm no longer keeping around a dummy angle value just for purposes of printing.  ^_^

[Dean Jackson]

Comment on attachment 179173 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179173&action=review

Yeah, I like this better.

> Source/WebCore/ChangeLog:29
> +        (CSSGradientValue):
> +        (CSSLinearGradientValue):

Remove these useless entries.

> Source/WebCore/css/CSSGradientValue.cpp:28
>  #include "CSSGradientValue.h"
> -
>  #include "CSSCalculationValue.h"

We keep that blank line in because everything after it should be sorted alphabetically.

[Tab Atkins]

Created attachment 179177 [details]
Patch

[WebKit Review Bot]

Comment on attachment 179177 [details]
Patch

Attachment 179177 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15310076

New failing tests:
fast/gradients/css3-linear-angle-gradients.html

[Tab Atkins]

Created attachment 179344 [details]
Patch

[WebKit Review Bot]

Comment on attachment 179344 [details]
Patch

Clearing flags on attachment: 179344

Committed r137669: <http://trac.webkit.org/changeset/137669>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.