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.
Created attachment 178836 [details] Patch
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
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
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
Created attachment 179096 [details] Patch
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.
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?
(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.
(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?
(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.
(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.
Created attachment 179173 [details] Patch
(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. ^_^
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.
Created attachment 179177 [details] Patch
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
Created attachment 179344 [details] Patch
Comment on attachment 179344 [details] Patch Clearing flags on attachment: 179344 Committed r137669: <http://trac.webkit.org/changeset/137669>
All reviewed patches have been landed. Closing bug.