Bug 134722 - Support radial-gradients with relative offsets
Summary: Support radial-gradients with relative offsets
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-07-08 04:55 PDT by Martin Hodovan
Modified: 2017-07-18 08:29 PDT (History)
13 users (show)

See Also:


Attachments
Proposed patch (8.78 KB, patch)
2014-07-08 05:03 PDT, Martin Hodovan
simon.fraser: review-
simon.fraser: commit-queue-
Details | Formatted Diff | Diff
Proposed patch (8.20 KB, patch)
2014-07-10 01:54 PDT, Martin Hodovan
simon.fraser: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (544.90 KB, application/zip)
2014-07-10 05:02 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Hodovan 2014-07-08 04:55:22 PDT
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.
Comment 1 Martin Hodovan 2014-07-08 05:03:02 PDT
Created attachment 234558 [details]
Proposed patch

Patch by Renata Hodovan, backported from Blink #177082.
https://codereview.chromium.org/356683005/
Comment 2 Sam Weinig 2014-07-08 06:36:13 PDT
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.
Comment 3 zalan 2014-07-08 07:16:09 PDT
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.
Comment 4 Simon Fraser (smfr) 2014-07-08 08:29:59 PDT
Comment on attachment 234558 [details]
Proposed patch

r- based on comments.
Comment 5 Martin Hodovan 2014-07-10 01:54:29 PDT
Created attachment 234696 [details]
Proposed patch
Comment 6 Martin Hodovan 2014-07-10 02:00:42 PDT
(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.)
Comment 7 Build Bot 2014-07-10 05:02:48 PDT
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
Comment 8 Build Bot 2014-07-10 05:02:54 PDT
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
Comment 9 Martin Hodovan 2014-07-15 04:03:32 PDT
(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?
Comment 10 Martin Hodovan 2014-07-24 04:04:53 PDT
Could someone take a look at this?
Comment 11 Csaba Osztrogonác 2015-05-11 03:35:57 PDT
ping?
Comment 12 Simon Fraser (smfr) 2015-05-11 08:43:47 PDT
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.