Closed Bug 897392 Opened 11 years ago Closed 11 years ago

[CSS Filters] Implement parsing for hue-rotate

Categories

(Core :: CSS Parsing and Computation, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: krit, Assigned: krit)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=CSS])

Attachments

(1 file, 2 obsolete files)

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>)
Blocks: 869828
Depends on: 895182
Attached patch Patch (obsolete) — Splinter Review
Here is a patch that Max Vujovic and I have authored.
Attachment #780273 - Flags: review?(cam)
Summary: [CSS Filters] Implement hue-rotate → [CSS Filters] Implement parsing for hue-rotate
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.
Attachment #780273 - Flags: review?(cam)
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.
Attached patch Patch v2 (obsolete) — Splinter Review
Addressed reviewer comments.
Attachment #780273 - Attachment is obsolete: true
Attachment #780844 - Flags: review?(cam)
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.
Attachment #780844 - Flags: review?(cam) → review+
Attached patch Patch v3Splinter Review
Updated patch v3.
Attachment #780844 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/6c30d62aa552
Assignee: nobody → krit
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Whiteboard: [DocArea=CSS]
(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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: