898175 - [CSS Filters] Refactor parsing to use a keyword lookup table for filter function names

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Max Vujovic]

In bug 895182, dbaron suggested:

"""
One minor comment:  I tend to dislike storing data as code rather than storing data as data; I think the two functions GetFilterFunctionName and StyleFilterTypeForFunctionName could have been replaced by a keyword table in nsCSSProps plus calls to nsCSSProps::FindKeyword and nsCSSProps::ValueToKeyword.
"""

Comment 1

[Max Vujovic]

Attachment: 898175-1-git.patch

This patch passes Mochitests locally.

Small issue: I wish I could use "uint8_t mType" instead of "int32_t mType" in nsStyleFilter, but the keyword lookup APIs like FindKeyword want to assign to int32_t. I could use a cast, but I thought that was uglier.

Comment 2

[Max Vujovic]

Adding heycam because he's also been reviewing this code.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 781302 [details] [diff] [review]
898175-1-git.patch

>-  Type mType;
>-  nsIURI* mURL;
>-  nsStyleCoord mCoord;
>+  int32_t mType;       // [reset] see nsStyleConsts.h
>+  nsIURI* mURL;        // [reset]
>+  nsStyleCoord mCoord; // [reset]

I would prefer not to have these [reset] comments, which are for properties (noting whether they're inherited or non-inherited (reset)). These are parts of a value, not properties.

On the other hand, the see comment should say "see NS_STYLE_FILTER_* constants in nsStyleConsts.h" or similar.

r=dbaron with that

Comment 4

[Dirk Schulze]

The patch should be updated and landed after the patch for hue-rotate and drop-shadow landed (both uploaded and read ready for review/landing).

Comment 5

[Max Vujovic]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #3)
> Comment on attachment 781302 [details] [diff] [review]
> 898175-1-git.patch
> 
> >-  Type mType;
> >-  nsIURI* mURL;
> >-  nsStyleCoord mCoord;
> >+  int32_t mType;       // [reset] see nsStyleConsts.h
> >+  nsIURI* mURL;        // [reset]
> >+  nsStyleCoord mCoord; // [reset]
> 
> I would prefer not to have these [reset] comments, which are for properties
> (noting whether they're inherited or non-inherited (reset)). These are parts
> of a value, not properties.
> 
> On the other hand, the see comment should say "see NS_STYLE_FILTER_*
> constants in nsStyleConsts.h" or similar.
> 
> r=dbaron with that

Makes sense. I'll do that. Thanks for reviewing!

Comment 6

[Max Vujovic]

(In reply to Dirk Schulze from comment #4)
> The patch should be updated and landed after the patch for hue-rotate and
> drop-shadow landed (both uploaded and read ready for review/landing).

Yes, I will rebase after hue-rotate and drop-shadow land and add constants for them. Should be a trivial change.

Comment 7

[Dirk Schulze]

Attachment: style-cleanup-moz.patch

patch for landing

Comment 8

[Cameron McCormack (:heycam)]

New patch looks good.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c45e9d8a9d

Comment 9

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/d7c45e9d8a9d