Bug 134722 – Support radial-gradients with relative offsets

• Component: CSS
• Status: RESOLVED CONFIGURATION CHANGED

[Martin Hodovan]

The center of a radial-gradient can be determined four
different ways. It can be defined with lengths, percentages,
keywords or by combining keywords with lengths or percentages.

[Martin Hodovan]

Created attachment 234558 [details]
Proposed patch

Patch by Renata Hodovan, backported from Blink #177082.
https://codereview.chromium.org/356683005/

[Sam Weinig]

Comment on attachment 234558 [details]
Proposed patch

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

> Source/WebCore/css/CSSGradientValue.cpp:397
> +static float positionFromValue(CSSPrimitiveValue* value, const CSSToLengthConversionData& conversionData, const FloatSize& size, bool isHorizontal)

There shouldn't be any reason to convert this from a reference to a pointer.

[alan]

Comment on attachment 234558 [details]
Proposed patch

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

> Source/WebCore/css/CSSGradientValue.cpp:401
> +    int origin = 0;
> +    int sign = 1;
>      int edgeDistance = isHorizontal ? size.width() : size.height();

Any particular reason to truncate size.width/height to integral value? both origin and edgeDistance should be float.

[Simon Fraser (smfr)]

Comment on attachment 234558 [details]
Proposed patch

r- based on comments.

[Martin Hodovan]

Created attachment 234696 [details]
Proposed patch

[Martin Hodovan]

(In reply to comment #2)
> (From update of attachment 234558 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234558&action=review
> 
> There shouldn't be any reason to convert this from a reference to a pointer.

I converted the 'value' reference to a pointer because Pair::second() returns with a pointer,
and references are not reseatable. Why is it important to keep 'value' a reference?
I had to add a local pointer variable this way, which is confusing and could be evitable.
However, seems like the value of this reference is not used outside this function, so it won't
cause further problems.

(In reply to comment #3)
> (From update of attachment 234558 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234558&action=review
> 
> Any particular reason to truncate size.width/height to integral value? both origin and edgeDistance should be float.

Thanks, I changed them both to float. (Truncating edgeDistance wasn't part of my patch, though.)

[Build Bot]

Comment on attachment 234696 [details]
Proposed patch

Attachment 234696 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5979312347938816

New failing tests:
media/W3C/video/paused/paused_false_during_play.html

[Build Bot]

Created attachment 234703 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Martin Hodovan]

(In reply to comment #6)
> I converted the 'value' reference to a pointer because Pair::second() returns with a pointer,
> and references are not reseatable. Why is it important to keep 'value' a reference?
> I had to add a local pointer variable this way, which is confusing and could be evitable.

Sam, is there any particular reason to keep 'value' a reference?

[Martin Hodovan]

Could someone take a look at this?

[Csaba Osztrogonác]

ping?

[Simon Fraser (smfr)]

Comment on attachment 234696 [details]
Proposed patch

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

> Source/WebCore/css/CSSGradientValue.cpp:402
> +    CSSPrimitiveValue* pValue = &value;

Why isn't this a reference? Also, we don't use hungarian notation in WebKit.

[Karl Dubost]

It would be nice to have a test case for this one.

[Karl Dubost]

Created attachment 477999 [details]
testcase

The testcase extracted from the patch.

[Karl Dubost]

Created attachment 478000 [details]
rendering in safari, firefox, chrome

The rendering is the same in all browsers.