Implement the syntax specified in the Filter Effects spec: https://dvcs.w3.org/hg/FXTF/raw-file/default/filters/index.html#FilterProperty hue-rotate(<angle>)

Here is a patch that Max Vujovic and I have authored.

Comment on attachment 780273 [details] [diff] [review] Patch Review of attachment 780273 [details] [diff] [review]: ----------------------------------------------------------------- This looks good overall. Upon reflection, I don't like the name of nsStyleFilter::mCoord; it's too generic. Can you make it mFilterParameter or something like that? Also can you add a comment on the line that declares it indicating which nsStyleCoord units it might have? (You can see examples of that throughout nsStyleStruct.h.) ::: layout/style/nsCSSParser.cpp @@ +10076,4 @@ > // VARIANT_NONNEGATIVE_DIMENSION will already reject negative lengths. > rejectNegativeArgument = false; > break; > + case eCSSKeyword_hue_rotate: Could you sort this clump of keywords alphabetically while you're here. @@ +10076,5 @@ > // VARIANT_NONNEGATIVE_DIMENSION will already reject negative lengths. > rejectNegativeArgument = false; > break; > + case eCSSKeyword_hue_rotate: > + variantMask = VARIANT_ANGLE | VARIANT_ZERO_ANGLE; There is a VARIANT_ANGLE_OR_ZERO you can use. ::: layout/style/nsROCSSPrimitiveValue.h @@ +19,4 @@ > class nsDOMCSSRect; > class nsDOMCSSRGBColor; > > +#define CSS_TURN 30U Can you move this into the cpp file, since it's not going to be exposed from this class. Also, can you add a comment above it explaining why we're defining it rather than extending the constants on the interface. @@ +46,4 @@ > // CSSPrimitiveValue > uint16_t PrimitiveType() > { > + if (mType > CSS_RGBCOLOR) { Add a brief comment above this line too mentioning why we do this. ::: layout/style/nsRuleNode.cpp @@ +775,5 @@ > + case eCSSUnit_Radian: unit = eStyleUnit_Radian; break; > + case eCSSUnit_Turn: unit = eStyleUnit_Turn; break; > + default: NS_NOTREACHED("unrecognized angular unit"); > + unit = eStyleUnit_Degree; > + } While moving this code, could you indent the cases one level? @@ +7766,3 @@ > mask = SETCOORD_LENGTH | SETCOORD_STORE_CALC; > + } else if (aStyleFilter->mType == nsStyleFilter::Type::eHueRotate) { > + mask = SETCOORD_ANGLE; Trailing white space.

One other thing: is it correct to accept "0" as an angle here? I thought in general, unlike lengths, zero angles always had to have a unit. (SVG properties are the exception.)

(In reply to Cameron McCormack (:heycam) from comment #2) > Comment on attachment 780273 [details] [diff] [review] > Patch > > Review of attachment 780273 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks good overall. > > Upon reflection, I don't like the name of nsStyleFilter::mCoord; it's too > generic. Can you make it mFilterParameter or something like that? Done. > Also can > you add a comment on the line that declares it indicating which nsStyleCoord > units it might have? (You can see examples of that throughout > nsStyleStruct.h.) Done. > > ::: layout/style/nsCSSParser.cpp > @@ +10076,4 @@ > > // VARIANT_NONNEGATIVE_DIMENSION will already reject negative lengths. > > rejectNegativeArgument = false; > > break; > > + case eCSSKeyword_hue_rotate: > > Could you sort this clump of keywords alphabetically while you're here. They are sorted but also grouped to avoid code duplication. Is that ok for you? > > @@ +10076,5 @@ > > // VARIANT_NONNEGATIVE_DIMENSION will already reject negative lengths. > > rejectNegativeArgument = false; > > break; > > + case eCSSKeyword_hue_rotate: > > + variantMask = VARIANT_ANGLE | VARIANT_ZERO_ANGLE; > > There is a VARIANT_ANGLE_OR_ZERO you can use. I removed ZERO after your last comment. > > ::: layout/style/nsROCSSPrimitiveValue.h > @@ +19,4 @@ > > class nsDOMCSSRect; > > class nsDOMCSSRGBColor; > > > > +#define CSS_TURN 30U > > Can you move this into the cpp file, since it's not going to be exposed from > this class. Also, can you add a comment above it explaining why we're > defining it rather than extending the constants on the interface. Done. > > @@ +46,4 @@ > > // CSSPrimitiveValue > > uint16_t PrimitiveType() > > { > > + if (mType > CSS_RGBCOLOR) { > > Add a brief comment above this line too mentioning why we do this. Done. > > ::: layout/style/nsRuleNode.cpp > @@ +775,5 @@ > > + case eCSSUnit_Radian: unit = eStyleUnit_Radian; break; > > + case eCSSUnit_Turn: unit = eStyleUnit_Turn; break; > > + default: NS_NOTREACHED("unrecognized angular unit"); > > + unit = eStyleUnit_Degree; > > + } > > While moving this code, could you indent the cases one level? Done. > > @@ +7766,3 @@ > > mask = SETCOORD_LENGTH | SETCOORD_STORE_CALC; > > + } else if (aStyleFilter->mType == nsStyleFilter::Type::eHueRotate) { > > + mask = SETCOORD_ANGLE; > > Trailing white space. Done.

(In reply to Dirk Schulze from comment #4) > > ::: layout/style/nsCSSParser.cpp > > @@ +10076,4 @@ > > > // VARIANT_NONNEGATIVE_DIMENSION will already reject negative lengths. > > > rejectNegativeArgument = false; > > > break; > > > + case eCSSKeyword_hue_rotate: > > > > Could you sort this clump of keywords alphabetically while you're here. > > They are sorted but also grouped to avoid code duplication. Is that ok for > you? Yes, I just meant to sort that group.

Addressed reviewer comments.

Comment on attachment 780844 [details] [diff] [review] Patch v2 Review of attachment 780844 [details] [diff] [review]: ----------------------------------------------------------------- r=me with these couple of changes. ::: layout/style/nsROCSSPrimitiveValue.cpp @@ +18,4 @@ > > using namespace mozilla; > > +// CSS_TURN is not part of CSSPrimitiveValue.idl yet. Return CSS_UNKOWN for CSS OM. Saying that it is not part of that file yet invites the reader to wonder why we haven't just added it to that file. So instead I think we should talk about how it's not likely that that interface is going to be updated in specifications. Can I suggest: There is no CSS_TURN constant on the CSSPrimitiveValue interface, since that unit is newer than DOM Level 2 Style, and CSS OM will probably expose CSS values in some other way in the future. We use this value in mType for "turn"-unit angles, but we define it here to avoid exposing it to content. ::: layout/style/nsROCSSPrimitiveValue.h @@ +44,5 @@ > // CSSPrimitiveValue > uint16_t PrimitiveType() > { > + // New value types were introduced but not added to CSS OM. > + // Return CSS_UNKNOWN until CSSPrimitiveValue.idl was updated. Instead of the second line, how about: Return CSS_UNKNOWN to avoid exposing CSS_TURN to content. ::: layout/style/nsRuleNode.cpp @@ +7772,5 @@ > "all filter functions except drop-shadow should have " > "exactly one argument"); > > nsCSSValue& arg = filterFunction->Item(1); > + DebugOnly<bool> success = SetCoord(arg, aStyleFilter->mFilterParameter, nsStyleCoord(), Rewrap this to avoid going over 80 columns.

Updated patch v3.

Doc updated: https://developer.mozilla.org/en-US/docs/Web/CSS/filter#hue-rotate%28%29 https://developer.mozilla.org/en-US/Firefox/Releases/25

(In reply to Jean-Yves Perrier [:teoli] from comment #12) > Doc updated: > https://developer.mozilla.org/en-US/docs/Web/CSS/filter#hue-rotate%28%29 > https://developer.mozilla.org/en-US/Firefox/Releases/25 Thanks for the doc work! One nit- the doc says "Maximum value is 360deg", but we actually allow values over 360, and the effect wraps around. The spec says, "Implementations should not normalize this value in order to allow animations beyond 360deg." For example, an animation from 0 to 720deg should go through two hue-rotation cycles.

(In reply to Max Vujovic from comment #13) > Thanks for the doc work! I will not take this pride: the page was written long ago by others (thanks to them!). I only checked it was okayish + updated the compat data value :-) > One nit- the doc says "Maximum value is 360deg", > but we actually allow values over 360, and the effect wraps around. I fixed the entry. Thanks.