See testcase. It should be a linear gradient from gray to red but instead the whole declaration is dropped. Gecko gets this right.
Created attachment 93609 [details] Patch
Comment on attachment 93609 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93609&action=review > LayoutTests/fast/css/linear-gradient-currentcolor.html:5 > + color:red; Please don't have visible red in a passing test. It would be better to use green, and maybe put two color stops next to eachother to get a sharp edge that's less likely to suffer from platform differences. Spaces after colons please. > Source/WebCore/ChangeLog:8 > + https://bugs.webkit.org/show_bug.cgi?id=58730 > + > + Test: fast/css/linear-gradient-currentcolor.html This needs some text saying how you fixed the issue. > Source/WebCore/css/CSSGradientValue.cpp:159 > + stops[i].color = renderer->document()->styleSelector()->getColorFromPrimitiveValue(color); I don't get it. getColorFromPrimitiveValue() already looks for CSSValueCurrentcolor.
Created attachment 93714 [details] Patch
Comment on attachment 93714 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93714&action=review > Source/WebCore/css/CSSGradientValue.cpp:159 > - stops[i].color = renderer->document()->styleSelector()->getColorFromPrimitiveValue(stop.m_color.get()); > + CSSPrimitiveValue* color = stop.m_color.get(); > + if (color->getIdent() == CSSValueCurrentcolor) > + stops[i].color = style->visitedDependentColor(CSSPropertyColor); > + else > + stops[i].color = renderer->document()->styleSelector()->getColorFromPrimitiveValue(color); I still don't get this part. getColorFromPrimitiveValue() handles currentCOlor already.
Comment on attachment 93714 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93714&action=review >> Source/WebCore/css/CSSGradientValue.cpp:159 >> + stops[i].color = renderer->document()->styleSelector()->getColorFromPrimitiveValue(color); > > I still don't get this part. getColorFromPrimitiveValue() handles currentCOlor already. In this context, CSSStyleSelector::m_style is NULL rather than equal to the local var 'style'. This change works around that issue pending a change in the path to getColorFromPrimitiveValue(). Maybe a FIXME is warranted? This feature addition exposes a lurking NULL-deref bug.
Created attachment 96546 [details] Revised patch
Comment on attachment 96546 [details] Revised patch Revised patch - force initialization of CSSStyleSelector rather than work around the lack thereof
Comment on attachment 96546 [details] Revised patch View in context: https://bugs.webkit.org/attachment.cgi?id=96546&action=review > Source/WebCore/css/CSSGradientValue.cpp:153 > + // FIXME Force initialization to support CSSValueCurrentcolor - should this happen earlier? > + renderer->document()->styleSelector()->setStyle(style); It seems like the style selector needs "start using for style" and "stop using for style" methods. Are there other places that need to do the same thing?
I've just discovered that this is the tip of the iceberg - there are a issues with caching the computed image too. New patch soon to come. smfr: My feeling is that the proposed methods ought to be called in WebCore::StyleGeneratedImage::image() before and after the call to GeneratedImage::image(). I'm don't know about other callers.
Created attachment 96557 [details] Patch
Comment on attachment 96557 [details] Patch Ok, more an ice-cube than iceberg - moved initialization of CSSStyleSelector to WebCore::StyleGeneratedImage::image() and added additional constraint to CSSGradientValue::isCacheable().
Just noticed that the file summary in the WebCore ChangeLog is wrong, sorry. I'm surprised that webkit-patch doesn't warn about that.
Created attachment 96672 [details] Patch
Comment on attachment 96672 [details] Patch Re-generated ChangeLog entries to capture the methods affected.
Comment on attachment 96672 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96672&action=review > Source/WebCore/ChangeLog:8 > + currentColor not supported in CSS gradients > + https://bugs.webkit.org/show_bug.cgi?id=58730 > + > + Test: fast/css/linear-gradient-currentcolor.html This needs to describe what was broken, and what you did to fix it.
The primary outstanding concern with this change is clarifying the contract with CSSStyleSelector. The patch implements the parsing and style logic for currentColor support in CSS gradient color stops.
Created attachment 109122 [details] Patch
Comment on attachment 109122 [details] Patch Updated changelog with active description.
Comment on attachment 109122 [details] Patch Rejecting attachment 109122 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: saving rejects to file LayoutTests/platform/chromium/test_expectations.txt.rej patching file Source/WebCore/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Source/WebCore/css/CSSGradientValue.cpp patching file Source/WebCore/css/CSSParser.cpp Hunk #1 succeeded at 5903 (offset -2 lines). patching file Source/WebCore/rendering/style/StyleGeneratedImage.cpp Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Simon Fraser', u'--for..." exit_code: 1 Full output: http://queues.webkit.org/results/9888418
Created attachment 109132 [details] Patch
I added some detail to the ChangeLog and resolved the conflict encountered by the commit queue. However, it is likely to conflict again due to the high rate of appends to LayoutTests/platform/chromium/test_expectations.txt. Is it simply a matter of rebasing repeatedly until it lands? Maybe we could use a merge driver for test_expectations.txt.
Created attachment 109233 [details] Patch
Comment on attachment 109233 [details] Patch Updated test_expectations.txt change so that conflict is less likely to occur on the commit queue.
Comment on attachment 109233 [details] Patch Forwarding simon.fraser's r+
Comment on attachment 109233 [details] Patch Clearing flags on attachment: 109233 Committed r96449: <http://trac.webkit.org/changeset/96449>
All reviewed patches have been landed. Closing bug.