Last Comment Bug 897392 - [CSS Filters] Implement parsing for hue-rotate
: [CSS Filters] Implement parsing for hue-rotate
Status: RESOLVED FIXED
[DocArea=CSS]
: dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla25
Assigned To: Dirk Schulze
:
: Jet Villegas (:jet)
Mentors:
Depends on: 895182
Blocks: 869828
  Show dependency treegraph
 
Reported: 2013-07-24 01:29 PDT by Dirk Schulze
Modified: 2014-09-15 13:48 PDT (History)
10 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (10.40 KB, patch)
2013-07-24 01:52 PDT, Dirk Schulze
no flags Details | Diff | Splinter Review
Patch v2 (12.64 KB, patch)
2013-07-25 00:34 PDT, Dirk Schulze
cam: review+
Details | Diff | Splinter Review
Patch v3 (19.67 KB, patch)
2013-07-25 13:42 PDT, Dirk Schulze
no flags Details | Diff | Splinter Review

Description Dirk Schulze 2013-07-24 01:29:05 PDT
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>)
Comment 1 Dirk Schulze 2013-07-24 01:52:39 PDT
Created attachment 780273 [details] [diff] [review]
Patch

Here is a patch that Max Vujovic and I have authored.
Comment 2 Cameron McCormack (:heycam) 2013-07-24 16:59:49 PDT
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.
Comment 3 Cameron McCormack (:heycam) 2013-07-24 17:11:28 PDT
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.)
Comment 4 Dirk Schulze 2013-07-24 23:38:13 PDT
(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.
Comment 5 Cameron McCormack (:heycam) 2013-07-24 23:41:14 PDT
(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.
Comment 6 Dirk Schulze 2013-07-25 00:34:36 PDT
Created attachment 780844 [details] [diff] [review]
Patch v2

Addressed reviewer comments.
Comment 7 Cameron McCormack (:heycam) 2013-07-25 00:56:32 PDT
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.
Comment 8 Dirk Schulze 2013-07-25 13:42:46 PDT
Created attachment 781202 [details] [diff] [review]
Patch v3

Updated patch v3.
Comment 9 Cameron McCormack (:heycam) 2013-07-25 14:44:12 PDT
https://tbpl.mozilla.org/?tree=Try&rev=d6d001512515
Comment 10 Cameron McCormack (:heycam) 2013-07-25 23:03:23 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c30d62aa552
Comment 11 Ryan VanderMeulen [:RyanVM] 2013-07-26 19:44:02 PDT
https://hg.mozilla.org/mozilla-central/rev/6c30d62aa552
Comment 13 Max Vujovic 2014-09-15 11:36:28 PDT
(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.
Comment 14 Jean-Yves Perrier [:teoli] 2014-09-15 13:48:01 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.