Bug 51240 - [Chromium][Skia] Border with a color with alpha != 1 breaks webkit gradient on skia
Summary: [Chromium][Skia] Border with a color with alpha != 1 breaks webkit gradient o...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-17 02:22 PST by Hajime Morrita
Modified: 2010-12-20 20:55 PST (History)
8 users (show)

See Also:


Attachments
Patch (7.53 KB, patch)
2010-12-17 03:13 PST, Hajime Morrita
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2010-12-17 02:22:05 PST
From http://code.google.com/p/chromium/issues/detail?id=48278
Comment 1 Hajime Morrita 2010-12-17 03:13:28 PST
Created attachment 76862 [details]
Patch
Comment 2 Adam Langley 2010-12-17 09:32:33 PST
(Note: I am not a WebKit reviewer. You also need an r+ from a real reviewer.)

LGTM
Comment 3 Nico Weber 2010-12-17 09:41:50 PST
I believe senorblanco wrote this code.

I don't quite understand why the patch works (it looks like setFillColor() still sets the gradient to NULL), but since the layout test passes and agl says it's ok, this is my fault and not the patch's :-)
Comment 4 Adam Langley 2010-12-17 10:32:54 PST
(In reply to comment #3)
> since the layout test passes

Yes, I largely defer to the reality of layout tests when evaluating correctness!
Comment 5 Hajime Morrita 2010-12-17 19:16:43 PST
So could any kind reviewer give r+ to this ?

(In reply to comment #3)
> I don't quite understand why the patch works (it looks like setFillColor() still sets the gradient to NULL), but since the layout test passes and agl says it's ok, this is my fault and not the patch's :-)
What wrong here is, in my understanding, not the fill code but the gradient code.
The gradient rendering refers color value which is set by setFillColor(), 
and uses the alpha component of that color to make things transparent.
So setFillColor() and setFillGradient() weren't mutually exclusive, regardless they should be.

Actually there are some confusions around how the transparency of each pixel is determined:
we have State::m_alpha and State::m_fillColor on the chromium land,
and have layers, blitters and shaders in the Skia land...
Comment 6 James Robinson 2010-12-20 16:56:31 PST
Comment on attachment 76862 [details]
Patch

OK
Comment 7 Hajime Morrita 2010-12-20 20:32:56 PST
Committed r74386: <http://trac.webkit.org/changeset/74386>
Comment 8 Hajime Morrita 2010-12-20 20:37:34 PST
(In reply to comment #6)
> (From update of attachment 76862 [details])
> OK
Landed. thank you for your r+!
Comment 9 WebKit Review Bot 2010-12-20 20:55:52 PST
http://trac.webkit.org/changeset/74386 might have broken Qt Linux Release
The following tests are not passing:
fast/gradients/gradient-after-transparent-border.html