[webkit-reviews] review granted: [Bug 221294] conic-gradient with stops starting after 1turn is blank : [Attachment 424251] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Tim Nguyen <tim_nguyen2 at apple.com>'s
request for review:
Bug 221294: conic-gradient with stops starting after 1turn is blank
https://bugs.webkit.org/show_bug.cgi?id=221294

Attachment 424251: Patch

https://bugs.webkit.org/attachment.cgi?id=424251&action=review




--- Comment #26 from Darin Adler <darin at apple.com> ---
Comment on attachment 424251
  --> https://bugs.webkit.org/attachment.cgi?id=424251
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;