Testcase URL: data:text/html,<div style="width: 200px;height: 200px; background: conic-gradient(red 1.1turn, orange 1.2turn);"/> Associated WPT: https://searchfox.org/mozilla-central/source/testing/web-platform/tests/css/css-images/normalization-conic-2.html I think Chrome and Firefox behavior of rendering the first stop color is reasonable since it's consistent with how linear or radial gradients are handled (in all browsers): data:text/html,<div style="width: 200px;height: 200px; background: linear-gradient(red 300px, orange 310px);"/> data:text/html,<div style="width: 200px;height: 200px; background: radial-gradient(red 300px, orange 310px);"/>
<rdar://problem/74157218>
Created attachment 423887 [details] Patch
Comment on attachment 423887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423887&action=review > Source/WebCore/ChangeLog:9 > + Reviewed by NOBODY (OOPS!). > + > + Tests: web-platform-tests/css/css-images/normalization-conic-2.html Please add a description here of the bug and what you changed to fix it.
Comment on attachment 423887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423887&action=review >> Source/WebCore/ChangeLog:9 >> + Tests: web-platform-tests/css/css-images/normalization-conic-2.html > > Please add a description here of the bug and what you changed to fix it. The test is currently marked failing in LayoutTests/TestExpectations. The patch should remove that line so the test runs. > Source/WebCore/css/CSSGradientValue.cpp:243 > + while (firstZeroOrGreaterIndex < numStops && * stops[firstZeroOrGreaterIndex].offset < 0) { WebKit coding style is '*stops' without a space. See https://webkit.org/code-style-guidelines/ for more. > Source/WebCore/css/CSSGradientValue.cpp:245 > + firstZeroOrGreaterIndex++; > } "One-line control clauses should not use braces unless comments are included or a single statement spans multiple lines" > Source/WebCore/css/CSSGradientValue.cpp:271 > + while (lastOneOrLessIndex > 0 && *stops[lastOneOrLessIndex].offset > 1) { > + lastOneOrLessIndex--; > } same here > Source/WebCore/css/CSSGradientValue.cpp:290 > - stops[i].offset = 1; > + *stops[i].offset = 1; I suppose this saves a branch if we know stop Optionals are already initialized. Not sure if that is meaningful, at least it should be done consistently in other offset assignments in this function too.
Created attachment 423928 [details] Patch v2 There were some unintended changes in the previous patch, undid them.
Created attachment 423931 [details] Patch v2.1 I submitted the wrong patch earlier, sorry.
Comment on attachment 423931 [details] Patch v2.1 View in context: https://bugs.webkit.org/attachment.cgi?id=423931&action=review > LayoutTests/TestExpectations:-4467 > -webkit.org/b/214456 imported/w3c/web-platform-tests/css/css-images/normalization-conic-2.html [ ImageOnlyFailure ] You should re-run prepare-ChangeLog to update LayoutTests/ChangeLog too. > Source/WebCore/ChangeLog:9 > + 1. Notably fixes the case where all stops have an offset above 1. You should explain what the bug was and what you changed to fix it. > Source/WebCore/ChangeLog:10 > + 2. Remove trailing whitespace from CSSGradientValue.cpp No need to mention this sort of things. > Source/WebCore/ChangeLog:11 > + 3. Adjust test expectations. This is for LayoutTests/ChangeLog
Created attachment 423943 [details] Patch v2.2 Adjusted changelogs according to feedback, thanks Antti!
Comment on attachment 423943 [details] Patch v2.2 I keep forgetting to use `-a` before amending patches (I'm used to working with mercurial which does that automatically), sorry. I'll upload the updated patch soon.
> I keep forgetting to use `-a` before amending patches (I'm used to working > with mercurial which does that automatically), sorry. Tools/Scripts/webkit-patch upload without '-g' also works and will take uncommitted changes to generate the patch.
Created attachment 424006 [details] Patch v3
Comment on attachment 424006 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=424006&action=review > LayoutTests/ChangeLog:7 > + Reviewed by Antti Koivisto. It should probably be still Reviewed by nobody (OOPS) since Antti did not yet r+ed the patch from what I see. Once the patch is r+ed, the commit queue will update the changelogs automatically. Or, if you need to reupload a patch, you might need to do it yourself. > LayoutTests/ChangeLog:9 > + * imported/w3c/web-platform-tests/css/css-images/normalization-conic-2.html: Passed. s/imported/w3c/web-platform-tests/css/css-images/normalization-conic-2.html: Passed./TestExpectations: enabled imported/w3c/web-platform-tests/css/css-images/normalization-conic-2.html. > Source/WebCore/css/CSSGradientValue.cpp:243 > + while (firstZeroOrGreaterIndex < (numStops - 1) && *stops[firstZeroOrGreaterIndex].offset < 0) Is it safe to do (numStops - 1) in case numStops = 0?
Created attachment 424009 [details] Patch v3.1 Address webkit-style issue.
Comment on attachment 424009 [details] Patch v3.1 Will address Youenn's feedback (thanks!), and look at the repeating-conic-gradient failure.
Created attachment 424018 [details] Patch v4 This should hopefully fix all the issues. :)
(In reply to youenn fablet from comment #12) > > Source/WebCore/css/CSSGradientValue.cpp:243 > > + while (firstZeroOrGreaterIndex < (numStops - 1) && *stops[firstZeroOrGreaterIndex].offset < 0) > > Is it safe to do (numStops - 1) in case numStops = 0? conic-gradient() with 0 stops is illegal CSS syntax, so I think that would probably be sanitized earlier. In any case, there seems to be an assert here catching this: https://webkit-search.igalia.com/webkit/rev/49548842a98b68dfe318ce29f901962b1318e593/Source/WebCore/css/CSSGradientValue.cpp#386
Comment on attachment 424018 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=424018&action=review This patch does fix the bug, but I think it makes the code more confusing, so not thrilled with it the way it is. > Source/WebCore/css/CSSGradientValue.cpp:240 > auto numStops = stops.size(); I think this code might be easier to read if we defined a lastStopIndex local variable containing numStops - 1. > Source/WebCore/css/CSSGradientValue.cpp:244 > + size_t firstZeroOrGreaterIndex = 0; > + while (firstZeroOrGreaterIndex < (numStops - 1) && *stops[firstZeroOrGreaterIndex].offset < 0) > + firstZeroOrGreaterIndex++; This is not an improvement. I find this while loop harder to read than the for loop we had before. The old one was a straightforward "look through all the stops" idiomatic loop, so we only had to read the body of the loop. The new loop is less idiomatic and thus harder to read, for me at least. The old loop would set firstZeroOrGreaterIndex past the end of the set of stops if there was no stop >= 0. But the new code instead points at the last stop, so the index does not point to a zero-or-greater index. The old logic is better and more straightforward than the new. The new logic instead relies a case where firstZeroOrGreaterIndex points to an index which contains a negative number stop value. If we think that "numStops" is a confusing sentinel value, we could use Optional<size_t> instead to clarify the logic. I think that would be a better direction than what this patch does. > Source/WebCore/css/CSSGradientValue.cpp:263 > // All stops are below 0; just clamp them. > - for (size_t i = 0; i < firstZeroOrGreaterIndex; ++i) > + for (size_t i = 0; i <= firstZeroOrGreaterIndex; ++i) > stops[i].offset = 0; If the comment here was correct, then the loop should go through all the stops, and use numStops, not firstZeroOrGreaterIndex, as the loop limit. If the comment is not correct, then the comment should be rewritten to be correct. > Source/WebCore/css/CSSGradientValue.cpp:269 > + size_t lastOneOrLessIndex = numStops - 1; > + while (lastOneOrLessIndex > 0 && *stops[lastOneOrLessIndex].offset > 1) > + lastOneOrLessIndex--; This has the same problem as the change above. The loop is harder to read, but also lastOneOrLessIndex can be left set to 0 both in the case where it's one or less, and in the case where it's not. Changing the loop makes the variable name wrong. I’m not sure this is a good change. As above, if we think that "numStops" is a confusing sentinel value, we could use Optional<size_t> instead to clarify the logic. I think that would be a better direction than what this patch does.
Comment on attachment 424018 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=424018&action=review > LayoutTests/ChangeLog:1 > +2021-03-22 Tim Nguyen <ntim.bugs@gmail.com> Please switch to your apple.com e-mail address. > Source/WebCore/ChangeLog:1 > +2021-03-22 Tim Nguyen <ntim.bugs@gmail.com> Ditto.
Created attachment 424238 [details] Patch
Comment on attachment 424238 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424238&action=review Much better! > Source/WebCore/css/CSSGradientValue.cpp:250 > + if (firstZeroOrGreaterIndex != WTF::nullopt) { you can just do if (firstZeroOrGreaterIndex) { > Source/WebCore/css/CSSGradientValue.cpp:251 > + size_t index = firstZeroOrGreaterIndex.value(); you could do auto index = *firstZeroOrGreaterIndex; > Source/WebCore/css/CSSGradientValue.cpp:254 > + float prevOffset = *stops[index - 1].offset; We usually use full words 'previousOffset' (I know the existing code used prevOffset but there is no need to stick to it). > Source/WebCore/css/CSSGradientValue.cpp:281 > + if (lastOneOrLessIndex != WTF::nullopt) { Here too > Source/WebCore/css/CSSGradientValue.cpp:282 > + size_t index = lastOneOrLessIndex.value(); Here too > Source/WebCore/css/CSSGradientValue.cpp:285 > + float prevOffset = *stops[index].offset; Here too
Created attachment 424240 [details] Patch
Created attachment 424248 [details] Patch
Created attachment 424249 [details] Patch
Looks like you'll also need to skip the tests on windows (by adding them to LayoutTests/platform/win/TestExpectations). I think the feature is not enabled there (it is behind ENABLE(CSS_CONIC_GRADIENTS) flag).
Created attachment 424251 [details] Patch
Comment on attachment 424251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424251&action=review This looks OK. Still have some suggestions. Also, I am setting review+ but please only land if all tests pass. > Source/WebCore/css/CSSGradientValue.cpp:240 > + size_t lastStopIndex = stops.size() - 1; I had meant to suggest that we would have *both* numStops and lastStopIndex, not that we would drop numStops. It’s idiomatic to have "< size" or "< numStops" and replacing them all with "<= lastStopIndex" is not an improvement. > Source/WebCore/css/CSSGradientValue.cpp:269 > + for (size_t i = 0; i <= lastStopIndex; ++i) > + stops[i].offset = 0; This is a loop through the entire vector. Should use a modern range-based for loop, which is more obviously correct than other types of loops: for (auto& stop : stops) stop.offset = 0; > Source/WebCore/css/CSSGradientValue.cpp:282 > + if (index <= lastStopIndex - 1) { "< lastStopIndex" makes more sense than "<= lastStopIndex - 1" to me, bit it could just be personal preference. > Source/WebCore/css/CSSGradientValue.cpp:299 > + for (size_t i = 0; i <= lastStopIndex; ++i) > + stops[i].offset = 1; Same: for (auto& stop : stops) stop.offset = 1;
Created attachment 424268 [details] Patch
Created attachment 424270 [details] Patch
tim_nguyen2@apple.com does not have committer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/Tools/Scripts/webkitpy/common/config/contributors.json. Rejecting attachment 424270 [details] from commit queue.
Committed r275055: <https://commits.webkit.org/r275055> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424270 [details].