When there are no stops, ToT uses a black fill, when something equivalent to fill="none" should be used. Tot also sorts stop offsets, but should not, and should take for each offset value the max between the offset value and all previous offset values. This breaks the W3C test pservers-grad-16-b Patch following.
Created attachment 12279 [details] Handle stops corner cases better Use a transparent fill when there are no stops. Don't sort stop offsets, and always take the max between current offset value and previous offset values.
Hey Remi, patch looks great! Two tiny comments: m_stops = stops; - std::sort(m_stops.begin(), m_stops.end(), compareStopOffset); you can remove the static compareStopOffset helper completelty. + CGFloat currOffset = max(stops[i].first,previousOffset); missing space between first and previousOffset. Keep up the good work. Niko
Created attachment 12287 [details] Addressed WildFox's comments
This is covered in the spec, it seems: Some notes on gradients: Gradient offset values less than 0 (or less than 0%) are rounded up to 0%. Gradient offset values greater than 1 (or greater than 100%) are rounded down to 100%. It is necessary that at least two stops defined to have a gradient effect. If no stops are defined, then painting shall occur as if 'none' were specified as the paint style. If one stop is defined, then paint with the solid color fill using the color defined for that gradient stop. Each gradient offset value is required to be equal to or greater than the previous gradient stop's offset value. If a given gradient stop's offset value is not equal to or greater than all previous offset values, then the offset value is adjusted to be equal to the largest of all previous offset values. If two gradient stops have the same offset value, then the latter gradient stop controls the color value at the overlap point. In particular: <stop offset="0" stop-color="white"/> <stop offset=".2" stop-color="red"/> <stop offset=".2" stop-color="blue"/> <stop offset="1" stop-color="black"/> will have approximately the same effect as: <stop offset="0" stop-color="white"/> <stop offset=".1999999999" stop-color="red"/> <stop offset=".2" stop-color="blue"/> <stop offset="1" stop-color="black"/> which is a gradient that goes smoothly from white to red, then abruptly shifts from red to blue, and then goes smoothly from blue to black.
Comment on attachment 12287 [details] Addressed WildFox's comments Looks good. r=me Is there also a maximum value for stops of 1.0 that should be enforced?
Created attachment 12309 [details] updated patch, with new test case and handling of offset>1.0
Comment on attachment 12309 [details] updated patch, with new test case and handling of offset>1.0 Looks good to me. Thanks for the great patch!
Landed in r18681.
Comment on attachment 12309 [details] updated patch, with new test case and handling of offset>1.0 memset is not a correct way to set floating point values to 0 -- the memset part of this patch is incorrect
Reopened per Comment #9.
Comment on attachment 12287 [details] Addressed WildFox's comments Clearing darin's r+ as this patch was never landed.
Comment on attachment 12309 [details] updated patch, with new test case and handling of offset>1.0 Clearing macdome's r+ so this bug doesn't show up in the commit queue. We still need to address the issue in Comment #9.
Comment on attachment 12309 [details] updated patch, with new test case and handling of offset>1.0 (In reply to comment #12) > Clearing macdome's r+ so this bug doesn't show up in the commit queue. > > We still need to address the issue in Comment #9. REALLY clearing the flag this time.
Created attachment 12335 [details] Don't use memset to set a CGFloat to 0 Do not use memset. Patch in attachment #12309 [details] was committed as r18681
Comment on attachment 12335 [details] Don't use memset to set a CGFloat to 0 I'm no reviewer; correct the flag
Comment on attachment 12335 [details] Don't use memset to set a CGFloat to 0 r=me, sorry for leading you astray.
Comment on attachment 12335 [details] Don't use memset to set a CGFloat to 0 r=me
Comment on attachment 12335 [details] Don't use memset to set a CGFloat to 0 >+ outColor[0] = 0; >+ outColor[1] = 0; >+ outColor[2] = 0; >+ outColor[3] = 0; I think "0" should be "0.0f" here.
(In reply to comment #18) > >+ outColor[0] = 0; > >+ outColor[1] = 0; > >+ outColor[2] = 0; > >+ outColor[3] = 0; > > I think "0" should be "0.0f" here. > The original code used "0"
Comment on attachment 12335 [details] Don't use memset to set a CGFloat to 0 This version of Bugzilla apparently doesn't have collision detection for review flags. Please see Comment #16 or #17 for a "real" reviewer to credit.
Comment on attachment 12335 [details] Don't use memset to set a CGFloat to 0 0.0 or 0.0f would be OK (CGFloat is double under 64-bit). But in general this does not matter, because the conversion works fine. In expressions (not simple constants) the type is more likely to matter because of the intermediate values.
(In reply to comment #20) > This version of Bugzilla apparently doesn't have collision detection for review > flags. In case anyone is interested in collision detection for flags in Bugzilla: https://bugzilla.mozilla.org/show_bug.cgi?id=99215
(In reply to comment #14) > Created an attachment (id=12335) [edit] > Don't use memset to set a CGFloat to 0 Committed revision 18731.