Bug 41580 – [chromium] Fix radial gradients

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Nico Weber]

http://crbug.com/48280

[Nico Weber]

Created attachment 60473 [details]
patch

In theory, we could also check if r0 > 0 and r1 == 0, then swap the order of the colors, and create a one-point radial, but that seems like overkill (and should be done in skia, not here, anyway).

[Nico Weber]

dglazkov, you reviewed the patch that this patch is fixing. Can you take a look?

[Dimitri Glazkov (Google)]

Comment on attachment 60473 [details]
patch

The change makes sense to me, but I have nits:

* please make sure not to add any svn props 
* Please don't forget to check in test expectations as well as the new test. Are you sure there isn't a test that we're failing already?

[Nico Weber]

Is it documented somwhere why svn props are not wanted? The first hit for "webkit svn props" gives me https://trac.macports.org/ticket/12594 , which suggests that the eol svn prop is wanted (?).

As for test expectations, that's why I set cq- :-)

[Nico Weber]

Also, `ack webkit-gradient LayoutTests/fst` shows that there's no existing test case for this.

[Stephen White]

This looks wrong to me.

As the comment says, the two-point radial gradient should only be invoked if the focal points are different (it really is much slower).  Maybe I'm reading this wrong, but it seems like we'll be invoking it now for any m0 > 0, simply because it handles reversed radii correctly.

I think the other solution you suggested, of swapping the colours and radii is preferable.

[Nico Weber]

But the single-point radial gradient can't handle the case where both radii are > 0.

(Also, this is an optimization that should be done in skia's SkGradientShader::CreateTwoPointRadial() function :-P)

[Stephen White]

Hmm.  I guess it's only the (r0, 0) radii case that this will really punish, performance-wise -- the others (where r1 > 0 also) weren't working anyway.  Is that correct?  OK.  I'd still rather it swap and do the fast case, though.  But feel free to submit that optimization to Skia instead.  :)

[Nico Weber]

How about we check this in as-is, then I submit a patch to skia that does the swapping and all these checks in SkGradientShader::CreateTwoPointRadial(), and then I optimization from here?

[Stephen White]

(In reply to comment #9)
> How about we check this in as-is, then I submit a patch to skia that does the swapping and all these checks in SkGradientShader::CreateTwoPointRadial(), and then I optimization from here?

Sounds great.

[Nico Weber]

Created attachment 60580 [details]
patch

Removed the svn:eol-style, and I think test expectations should be fine.

I would still appreciate answers to my question about eol-style above.

[Adam Barth]

> Is it documented somwhere why svn props are not wanted?

I think it's been discussed on webkit-dev, but I don't remember the details.  Sorry that's not a very helpful answer.

[Dimitri Glazkov (Google)]

Comment on attachment 60580 [details]
patch

ok.

[Peter Kasting]

For the record, Darin Adler has explicitly told me we want svn:eol-style on all files, and that he's gone through a couple times and added it in the past.  So I'm not sure where this "don't add svn props" is coming from, and I think the patch with the props should be the committed one.

[WebKit Commit Bot]

Comment on attachment 60580 [details]
patch

Clearing flags on attachment: 60580

Committed r62753: <http://trac.webkit.org/changeset/62753>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

I'm adding mac results, but the test is not good; the pixel results are chopped off. See attachment.

[Simon Fraser (smfr)]

Created attachment 60977 [details]
Pixel result, showing truncation

[Nico Weber]

Created attachment 60979 [details]
better test

[Nico Weber]

Comment on attachment 60979 [details]
better test

Missing pixel results

[Nico Weber]

Created attachment 60984 [details]
Patch

[Nico Weber]

Reopening, so that cq picks up patch.

[WebKit Commit Bot]

Comment on attachment 60984 [details]
Patch

Clearing flags on attachment: 60984

Committed r62981: <http://trac.webkit.org/changeset/62981>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.