http://crbug.com/48280
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).
dglazkov, you reviewed the patch that this patch is fixing. Can you take a look?
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?
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- :-)
Also, `ack webkit-gradient LayoutTests/fst` shows that there's no existing test case for this.
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.
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)
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. :)
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?
(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.
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.
> 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.
Comment on attachment 60580 [details] patch ok.
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.
Comment on attachment 60580 [details] patch Clearing flags on attachment: 60580 Committed r62753: <http://trac.webkit.org/changeset/62753>
All reviewed patches have been landed. Closing bug.
I'm adding mac results, but the test is not good; the pixel results are chopped off. See attachment.
Created attachment 60977 [details] Pixel result, showing truncation
Created attachment 60979 [details] better test
Comment on attachment 60979 [details] better test Missing pixel results
Created attachment 60984 [details] Patch
Reopening, so that cq picks up patch.
Comment on attachment 60984 [details] Patch Clearing flags on attachment: 60984 Committed r62981: <http://trac.webkit.org/changeset/62981>