Last Comment Bug 895182 - [CSS Filters] Implement parsing for blur, brightness, contrast, grayscale, invert, opacity, saturate, sepia
: [CSS Filters] Implement parsing for blur, brightness, contrast, grayscale, in...
Status: RESOLVED FIXED
[DocArea=CSS]
: dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla25
Assigned To: Max Vujovic
:
Mentors:
Depends on:
Blocks: 869828 878883 897392
  Show dependency treegraph
 
Reported: 2013-07-17 16:21 PDT by Max Vujovic
Modified: 2014-09-12 04:45 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (27.39 KB, patch)
2013-07-17 16:27 PDT, Max Vujovic
no flags Details | Diff | Review
Patch v2 (28.54 KB, patch)
2013-07-18 11:25 PDT, Max Vujovic
no flags Details | Diff | Review
Patch v3 (30.18 KB, patch)
2013-07-19 15:26 PDT, Max Vujovic
cam: review+
Details | Diff | Review
Patch v4 (40.47 KB, patch)
2013-07-22 15:19 PDT, Max Vujovic
no flags Details | Diff | Review
Patch v5 (40.56 KB, patch)
2013-07-23 11:12 PDT, Max Vujovic
no flags Details | Diff | Review

Description Max Vujovic 2013-07-17 16:21:22 PDT
Implement the syntax specified in the Filter Effects spec:
https://dvcs.w3.org/hg/FXTF/raw-file/default/filters/index.html#FilterProperty

As defined in the current editor's draft:
- blur accepts a nonnegative <length>.
- brightness, contrast, grayscale, invert, opacity, saturate, and sepia accept a nonnegative <number>|<percentage>.
- Values above 1.0 (or 100%) for brightness, contrast, and saturate are meaningful and useful.
- Conversely, values above 1.0 (or 100%) are clamped to 1.0 (or 100%) for grayscale, invert, opacity, and sepia clamp.

The remaining two built-in filters, hue-rotate and drop-shadow, are a little different than the rest, and I think it is more convenient to implement them in separate patches. Hue-rotate introduces some opportunities for factoring out and sharing some <angle> parsing code used in CSS Gradients.

Until the filter rendering backend is complete, I think we should put the parsing of the new CSS filter property syntax behind a "layout.css.filters.enabled" preference, which is disabled by default. This way, web authors won't get false positives in their CSS Filters feature detection.
Comment 1 Max Vujovic 2013-07-17 16:27:06 PDT
Created attachment 777461 [details] [diff] [review]
Patch

Here is a patch that Dirk Schulze and I have authored.
Comment 2 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-07-17 18:12:17 PDT
Did you test that the mochitests in layout/style pass with the default value of the layout.css.filters.enabled pref flipped to true?
Comment 3 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-07-17 18:17:01 PDT
Also, you should definitely use Preferences::AddBoolVarCache rather than Preferences::GetBool.  (If there's no more-obvious place, probably use a static in nsLayoutUtils.)

I'm also not crazy about the DeprecatedFilter() name, unless it's something you're planning to remove entirely later in the patch series.

Other than that, I'll wait to look more closely until I see an answer to comment 2.
Comment 4 Max Vujovic 2013-07-17 20:38:28 PDT
Thanks for taking a look!

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #2)
> Did you test that the mochitests in layout/style pass with the default value
> of the layout.css.filters.enabled pref flipped to true?

Yes, they all pass with the pref on or off.

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #3)
> Also, you should definitely use Preferences::AddBoolVarCache rather than
> Preferences::GetBool.  (If there's no more-obvious place, probably use a
> static in nsLayoutUtils.)

Will do!

> 
> I'm also not crazy about the DeprecatedFilter() name, unless it's something
> you're planning to remove entirely later in the patch series.

Maybe we should call it SingleFilterUrl() or SingleReferenceFilter()? This function won't be removed in the parsing patches, but it will be removed eventually when we connect the new filter rendering backend. The new backend will be able to handle a chain of filters instead of a single SVG reference filter. It will access mFilters directly.

On a related note, I also considered using the name nsStyleFilter::Type::kReference instead of nsStyleFilter::Type::kURL for SVG reference filters, in case you have a preference.

> 
> Other than that, I'll wait to look more closely until I see an answer to
> comment 2.
Comment 5 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2013-07-17 21:39:28 PDT
Minor drive-by comment: can you make the nsStyleFilter::Type values begin with "e" rather than "k"?  The latter should only be for consts.
Comment 6 Max Vujovic 2013-07-18 09:53:59 PDT
(In reply to Cameron McCormack (:heycam) from comment #5)
> Minor drive-by comment: can you make the nsStyleFilter::Type values begin
> with "e" rather than "k"?  The latter should only be for consts.

Absolutely. Should we add "e" to the style guide under "Variable prefixes"?
https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style
Comment 7 Max Vujovic 2013-07-18 11:25:49 PDT
Created attachment 777954 [details] [diff] [review]
Patch v2

New patch to address the feedback so far, and some small issues I noticed.

Changes:
- Add nsLayoutUtils::CSSFiltersEnabled, which uses Preferences::AddBoolVarCache.
- Change the "k" prefix to "e" for the nsFilterStyle::Type enums.
- Remove the SETCOORD_STORE_CALC from the flag for factors and percentages in nsRuleNode.cpp::SetStyleFilterToCSSValue, since we aren't accepting calc for those arguments in CSSParserImpl::ParseSingleFilter.
- Change SETCOORD_LP to SETCOORD_LENGTH in nsRuleNode.cpp::SetStyleFilterToCSSValue, since blur doesn't accept percentages, and we aren't accepting them in CSSParserImpl::ParseSingleFilter.
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-07-18 20:29:26 PDT
Comment on attachment 777954 [details] [diff] [review]
Patch v2

Review of attachment 777954 [details] [diff] [review]:
-----------------------------------------------------------------

Cameron's a good reviewer for all this. Looks good to me in a quick skim.

::: layout/style/nsStyleStruct.cpp
@@ +1016,5 @@
> +
> +  if (mType == eURL)
> +    mUrl = aSource.mUrl;
> +  else if (mType != eNull)
> +    mCoord = aSource.mCoord;

{} around the bodies of if/else statements.
Comment 9 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2013-07-18 23:15:24 PDT
Comment on attachment 777954 [details] [diff] [review]
Patch v2

Review of attachment 777954 [details] [diff] [review]:
-----------------------------------------------------------------

This mostly looks good to me.  A few comments.

::: layout/style/nsCSSParser.cpp
@@ +10120,4 @@
>    return true;
>  }
>  
> +/* Reads a single url or filter function from the tokenizer stream, reporting an

"/**" in case we actually run doc generation on this file.

@@ +10131,5 @@
> +  }
> +
> +  if (!nsLayoutUtils::CSSFiltersEnabled()) {
> +    // With CSS Filters disabled, we should only accept an SVG reference filter.
> +    return false;

Before returning false here, you should report a CSS parsing error.  You can use PARSE_UNEXPECTED_TOKEN.  See dom/locales/en-US/chrome/layout/css.properties for where the messages are defined; you can make one up if there isn't one appropriate.

@@ +10139,5 @@
> +    return false;
> +  }
> +
> +  if (mToken.mType != eCSSToken_Function) {
> +    return false;

PARSE_UNEXPECTED_TOKEN before this.

@@ +10143,5 @@
> +    return false;
> +  }
> +
> +  // Set up the parsing rules based on the filter function.
> +  int32_t variant = VARIANT_PN;

Can you call this "variantMask" to be consistent with ParseSingleTransform.

@@ +10147,5 @@
> +  int32_t variant = VARIANT_PN;
> +  bool rejectNegativeArgument = true;
> +  bool clampArgumentToOne = false;
> +  nsCSSKeyword functionName = nsCSSKeywords::LookupKeyword(mToken.mIdent);
> +  switch(functionName) {

Space before "(".

@@ +10148,5 @@
> +  bool rejectNegativeArgument = true;
> +  bool clampArgumentToOne = false;
> +  nsCSSKeyword functionName = nsCSSKeywords::LookupKeyword(mToken.mIdent);
> +  switch(functionName) {
> +  case eCSSKeyword_blur:

Local style indents the cases one level, so please follow that.

@@ +10165,5 @@
> +  case eCSSKeyword_saturate:
> +    break;
> +  default:
> +    // Unrecognized filter function.
> +    return false;

REPORT_UNEXPECTED_TOKEN.  You'll also need to call SkipUntil(')') before returning to follow the forwards compatible parsing.

@@ +10188,5 @@
> +  nsCSSValue& arg = aValue->GetArrayValue()->Item(1);
> +
> +  if (rejectNegativeArgument) {
> +    if (arg.GetUnit() == eCSSUnit_Number && arg.GetFloatValue() < 0.0f) {
> +      return false;

My first thought is that it would be better to handle this checking in ParseVariant, so that error messages could be made to point to the exact argument that used the negative value, but we've only got a couple of variant bits left.  So I think this is OK, but add a REPORT_UNEXPECTED to report the invalid value.

@@ +10200,5 @@
> +    if (arg.GetUnit() == eCSSUnit_Number &&
> +        arg.GetFloatValue() > 1.0f) {
> +      arg.SetFloatValue(1.0f, arg.GetUnit());
> +    }
> +    else if (arg.GetUnit() == eCSSUnit_Percent &&

"else if" on previous line.

@@ +10209,5 @@
> +
> +  return true;
> +}
> +
> +/* Parses a filter property value by continuously reading in urls and/or filter

"/**".

::: layout/style/nsCSSPropList.h
@@ +3333,3 @@
>      nullptr,
>      CSS_PROP_NO_OFFSET,
>      eStyleAnimType_None)

We'll want to be able to animate this property, but we can do that in a separate bug.

::: layout/style/nsComputedDOMStyle.cpp
@@ +4468,5 @@
>    return val;
>  }
>  
> +void
> +nsComputedDOMStyle::SetCssTextToCoord(nsAutoString* aCssText,

Make this nsAString&.

@@ +4479,5 @@
> +  delete value;
> +}
> +
> +static void
> +FilterFunctionName(nsAutoString* aString, nsStyleFilter::Type mType)

Call this GetFilterFunctionName, and make the argument nsAString&.

@@ +4547,2 @@
>  
> +  if (!filters.Length()) {

filters.IsEmpty()

::: layout/style/nsRuleNode.cpp
@@ +7707,4 @@
>    COMPUTE_END_INHERITED(SVG, svg)
>  }
>  
> +static nsStyleFilter::Type StyleFilterTypeForFunctionName(

Newline after "Type".  (It's a bit inconsistent in this file, but I believe this is the style.)

@@ +7710,5 @@
> +static nsStyleFilter::Type StyleFilterTypeForFunctionName(
> +  nsCSSKeyword functionName)
> +{
> +  switch(functionName) {
> +  case eCSSKeyword_blur:

Space after "switch" and indent cases.

@@ +7763,5 @@
> +
> +  nsCSSValue& arg = filterFunction->Item(1);
> +  const nsStyleCoord dummyParentCoord;
> +  DebugOnly<bool> success = SetCoord(arg, aStyleFilter->mCoord,
> +                                     dummyParentCoord, mask, aStyleContext,

Pass in nsStyleCoord() instead of dummpyParentCoord, as some other functions in this file do.

@@ +7765,5 @@
> +  const nsStyleCoord dummyParentCoord;
> +  DebugOnly<bool> success = SetCoord(arg, aStyleFilter->mCoord,
> +                                     dummyParentCoord, mask, aStyleContext,
> +                                     aPresContext, aCanStoreInRuleTree);
> +  NS_ABORT_IF_FALSE(success, "could not resolve filter function argument");

Make this error message "unexpected unit", like others in this file.

@@ +7845,5 @@
>  
>    // filter: url, none, inherit
>    const nsCSSValue* filterValue = aRuleData->ValueForFilter();
> +  switch(filterValue->GetUnit()) {
> +  case eCSSUnit_Null:

Space after "switch" and indent cases.

@@ +7871,5 @@
> +    }
> +    break;
> +  }
> +  default:
> +    NS_NOTREACHED("unexpected css value unit");

"unexpected unit" or "unexpected value unit"

::: layout/style/nsStyleStruct.cpp
@@ +1069,4 @@
>    nsChangeHint hint = nsChangeHint(0);
>  
>    if (!EqualURIs(mClipPath, aOther.mClipPath) ||
> +      !EqualURIs(DeprecatedFilter(), aOther.DeprecatedFilter()) ||

You'll need to change this to compare the two arrays.

::: layout/style/nsStyleStruct.h
@@ +2277,5 @@
> +  };
> +
> +  Type mType;
> +  union {
> +    nsIURI* mUrl;

mURL

@@ +2304,5 @@
>  
> +  // The backend only supports one SVG reference right now.
> +  // Eventually, it will support multiple chained SVG reference filters and CSS
> +  // filter functions.
> +  nsIURI* DeprecatedFilter() const {

I agree with David; we should call this something other than "Deprecated".  I like "SingleFilterURL" or "SingleFilter".
Comment 10 Max Vujovic 2013-07-19 15:26:03 PDT
Created attachment 778695 [details] [diff] [review]
Patch v3

Patch to address review feedback.
Comment 11 Max Vujovic 2013-07-19 15:28:33 PDT
Comment on attachment 777954 [details] [diff] [review]
Patch v2

Review of attachment 777954 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks very much for the detailed review!

::: layout/style/nsCSSParser.cpp
@@ +10120,4 @@
>    return true;
>  }
>  
> +/* Reads a single url or filter function from the tokenizer stream, reporting an

Done.

@@ +10131,5 @@
> +  }
> +
> +  if (!nsLayoutUtils::CSSFiltersEnabled()) {
> +    // With CSS Filters disabled, we should only accept an SVG reference filter.
> +    return false;

Done. I've added PARSE_UNEXPECTED... calls to each return false case in ParseSingleFilter and one return false case in ParseFilter. The other return false cases in ParseFilter rely on PARSE_UNEXPECTED... calls from other functions (e.g. ExpectEndProperty).

@@ +10139,5 @@
> +    return false;
> +  }
> +
> +  if (mToken.mType != eCSSToken_Function) {
> +    return false;

Done.

@@ +10143,5 @@
> +    return false;
> +  }
> +
> +  // Set up the parsing rules based on the filter function.
> +  int32_t variant = VARIANT_PN;

Done.

@@ +10147,5 @@
> +  int32_t variant = VARIANT_PN;
> +  bool rejectNegativeArgument = true;
> +  bool clampArgumentToOne = false;
> +  nsCSSKeyword functionName = nsCSSKeywords::LookupKeyword(mToken.mIdent);
> +  switch(functionName) {

Done.

@@ +10148,5 @@
> +  bool rejectNegativeArgument = true;
> +  bool clampArgumentToOne = false;
> +  nsCSSKeyword functionName = nsCSSKeywords::LookupKeyword(mToken.mIdent);
> +  switch(functionName) {
> +  case eCSSKeyword_blur:

Done.

@@ +10165,5 @@
> +  case eCSSKeyword_saturate:
> +    break;
> +  default:
> +    // Unrecognized filter function.
> +    return false;

Done and done.

I don't totally understand the need for SkipUntil(')') though. An invalid filter function or url makes the entire filter property invalid, and ParseFilter doesn't proceed if one ParseSingleFilter call fails. Besides making ParseSingleFilter more robust if called from somewhere else, is there another reason to call SkipUntil(')')?

@@ +10188,5 @@
> +  nsCSSValue& arg = aValue->GetArrayValue()->Item(1);
> +
> +  if (rejectNegativeArgument) {
> +    if (arg.GetUnit() == eCSSUnit_Number && arg.GetFloatValue() < 0.0f) {
> +      return false;

Done. I've also combined these 3 if statements into one, since the if statement body is more than just "return false;" now.

I considered turning the VARIANT_NONNEGATIVE_DIMENSION flag into just VARIANT_NONNEGATIVE and making it capable of handling percentages and numbers. I refrained because I was afraid of slowing down ParseVariant with more checks. Also, it might be better done in a separate patch since it could affect other code paths. What do you think?

@@ +10200,5 @@
> +    if (arg.GetUnit() == eCSSUnit_Number &&
> +        arg.GetFloatValue() > 1.0f) {
> +      arg.SetFloatValue(1.0f, arg.GetUnit());
> +    }
> +    else if (arg.GetUnit() == eCSSUnit_Percent &&

Done.

@@ +10209,5 @@
> +
> +  return true;
> +}
> +
> +/* Parses a filter property value by continuously reading in urls and/or filter

Done.

::: layout/style/nsCSSPropList.h
@@ +3333,3 @@
>      nullptr,
>      CSS_PROP_NO_OFFSET,
>      eStyleAnimType_None)

Yes! Added bug 896050.

::: layout/style/nsComputedDOMStyle.cpp
@@ +4468,5 @@
>    return val;
>  }
>  
> +void
> +nsComputedDOMStyle::SetCssTextToCoord(nsAutoString* aCssText,

Done.

The style guide says to use pointers for function out parameters [1]. In this file, we mostly use references. In general, would you use references, pointers, or stick with local style?

FYI, I use pointers for function out parameters in two other places:
- ParseSingleFilter(nsCSSValue* aValue)
- SetStyleFilterToCSSValue(nsStyleFilter* aStyleFilter, ...)

[1]: "Use pointers instead of references for function out parameters, even for primitive types." http://www.mozilla.org/en-US/styleguide/

@@ +4479,5 @@
> +  delete value;
> +}
> +
> +static void
> +FilterFunctionName(nsAutoString* aString, nsStyleFilter::Type mType)

Done.

@@ +4547,2 @@
>  
> +  if (!filters.Length()) {

Done. Much better!

::: layout/style/nsRuleNode.cpp
@@ +7707,4 @@
>    COMPUTE_END_INHERITED(SVG, svg)
>  }
>  
> +static nsStyleFilter::Type StyleFilterTypeForFunctionName(

Done. I've also done the same for the new function below, SetStyleFilterToCSSValue.

@@ +7710,5 @@
> +static nsStyleFilter::Type StyleFilterTypeForFunctionName(
> +  nsCSSKeyword functionName)
> +{
> +  switch(functionName) {
> +  case eCSSKeyword_blur:

Done and done.

@@ +7763,5 @@
> +
> +  nsCSSValue& arg = filterFunction->Item(1);
> +  const nsStyleCoord dummyParentCoord;
> +  DebugOnly<bool> success = SetCoord(arg, aStyleFilter->mCoord,
> +                                     dummyParentCoord, mask, aStyleContext,

Done.

@@ +7765,5 @@
> +  const nsStyleCoord dummyParentCoord;
> +  DebugOnly<bool> success = SetCoord(arg, aStyleFilter->mCoord,
> +                                     dummyParentCoord, mask, aStyleContext,
> +                                     aPresContext, aCanStoreInRuleTree);
> +  NS_ABORT_IF_FALSE(success, "could not resolve filter function argument");

Done.

@@ +7845,5 @@
>  
>    // filter: url, none, inherit
>    const nsCSSValue* filterValue = aRuleData->ValueForFilter();
> +  switch(filterValue->GetUnit()) {
> +  case eCSSUnit_Null:

Done.

@@ +7871,5 @@
> +    }
> +    break;
> +  }
> +  default:
> +    NS_NOTREACHED("unexpected css value unit");

Done. I went with "unexpected unit".

::: layout/style/nsStyleStruct.cpp
@@ +1016,5 @@
> +
> +  if (mType == eURL)
> +    mUrl = aSource.mUrl;
> +  else if (mType != eNull)
> +    mCoord = aSource.mCoord;

Done.

@@ +1069,4 @@
>    nsChangeHint hint = nsChangeHint(0);
>  
>    if (!EqualURIs(mClipPath, aOther.mClipPath) ||
> +      !EqualURIs(DeprecatedFilter(), aOther.DeprecatedFilter()) ||

Done. I added an equals operator to nsStyleFilter and I'm using the TArray<nsStyleFilter> not equals operator to compare the filter chains.

I'm doing the comparison after mClipPath and mMask- Please let me know if there is a preferred order.

::: layout/style/nsStyleStruct.h
@@ +2277,5 @@
> +  };
> +
> +  Type mType;
> +  union {
> +    nsIURI* mUrl;

Done.

@@ +2304,5 @@
>  
> +  // The backend only supports one SVG reference right now.
> +  // Eventually, it will support multiple chained SVG reference filters and CSS
> +  // filter functions.
> +  nsIURI* DeprecatedFilter() const {

Done. I went with "SingleFilter".
Comment 12 Max Vujovic 2013-07-19 15:30:20 PDT
Removing the needinfo flag I set for David, since Cameron addressed my question.
Comment 13 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2013-07-19 21:16:57 PDT
Comment on attachment 778695 [details] [diff] [review]
Patch v3

Review of attachment 778695 [details] [diff] [review]:
-----------------------------------------------------------------

(I should have pointed out that without the specific error messages you report, you will still get a generic "Error in parsing value for 'filter'." message, but it's good to have the additional information for authors.)

r=me with a few more changes.

> I don't totally understand the need for SkipUntil(')') though.
> An invalid filter function or url makes the entire filter property
> invalid, and ParseFilter doesn't proceed if one ParseSingleFilter
> call fails. Besides making ParseSingleFilter more robust if called
> from somewhere else, is there another reason to call SkipUntil(')')?

SkipUntil handles balanced parentheses, brackets and braces in the stuff that it skips over.  So if you had:

  <style>p { filter: invalid(; color: green; }</style>

then CSS parsing rules require all of the "; color: green; }" to be skipped while looking for a closing parenthesis to match the "invalid(" token.  Without the SkipUntil(')') call, the remainder of CSSParserImpl::ParseDeclaration would just search for the ';' to find the end of the declarations without knowing that there is a '(' still open.

> I considered turning the VARIANT_NONNEGATIVE_DIMENSION flag
> into just VARIANT_NONNEGATIVE and making it capable of
> handling percentages and numbers. I refrained because I was
> afraid of slowing down ParseVariant with more checks. Also,
> it might be better done in a separate patch since it could
> affect other code paths. What do you think?

I wondered that too, but I wasn't sure that it would be correct. I don't know if there are properties that take, say, percentages and dimensions, where the dimension must not be negative but where the percentage can be.  David might know.  If it is possible to do this, I would do it in a separate bug.

> The style guide says to use pointers for function out parameters
> [1]. In this file, we mostly use references. In general, would
> you use references, pointers, or stick with local style?

(I don't think your [1] URL is right.)

For strings I think I nearly always see references used.  I'm pretty sure it is the prevailing style.  To be honest, I'm not sure what the state of our written-down style guides are.  I think I saw a message on dev-platform recently saying there were three different ones. :)  So I don't think I have any guidance on how to get that written down anyway, sorry.  Thanks though for following what style guide information you do find.

> I'm doing the comparison after mClipPath and mMask- Please let
> me know if there is a preferred order.

I don't think it matters, unless we knew that authors overwhelmingly changed one property rather than another.

::: dom/locales/en-US/chrome/layout/css.properties
@@ +138,5 @@
>  PESupportsConditionExpectedNot=Expected 'not' while parsing supports condition but found '%1$S'.
>  PESupportsGroupRuleStart=Expected '{' to begin @supports rule but found '%1$S'.
> +PEFilterEOF=filter
> +PEExpectedURL=Expected URL but found '%1$S'.
> +PEExpectedURLOrFilterFunction=Expected URL or filter function but found '%1$S'.

I would mention "none" in these two messages.

@@ +139,5 @@
>  PESupportsGroupRuleStart=Expected '{' to begin @supports rule but found '%1$S'.
> +PEFilterEOF=filter
> +PEExpectedURL=Expected URL but found '%1$S'.
> +PEExpectedURLOrFilterFunction=Expected URL or filter function but found '%1$S'.
> +PEExpectedNonnegativeNP=Expected nonnegative number or percentage.

"non-negative"

@@ +140,5 @@
> +PEFilterEOF=filter
> +PEExpectedURL=Expected URL but found '%1$S'.
> +PEExpectedURLOrFilterFunction=Expected URL or filter function but found '%1$S'.
> +PEExpectedNonnegativeNP=Expected nonnegative number or percentage.
> +PEUnexpectedFunctionArguments=Unexpected arguments provided to function.

"Unexpected" to me sounds like it found something it did not expect.  Since we use this same message when there are too few arguments (or none at all), can we change it to "Error parsing filter function arguments"?  (I'd suggest "Error in parsing ..." for consistency with PEValueParsingError but I think that sounds awkward.)

::: layout/style/nsCSSParser.cpp
@@ +10196,5 @@
> +
> +  if (rejectNegativeArgument &&
> +      ((arg.GetUnit() == eCSSUnit_Percent && arg.GetPercentValue() < 0.0f) ||
> +       (arg.GetUnit() == eCSSUnit_Number && arg.GetFloatValue() < 0.0f))) {
> +    REPORT_UNEXPECTED(PEExpectedNonnegativePN);

s/PN/NP/

@@ +10205,5 @@
> +    if (arg.GetUnit() == eCSSUnit_Number &&
> +        arg.GetFloatValue() > 1.0f) {
> +      arg.SetFloatValue(1.0f, arg.GetUnit());
> +    } else if (arg.GetUnit() == eCSSUnit_Percent &&
> +             arg.GetPercentValue() > 1.0f) {

Fix indenting.

::: layout/style/nsComputedDOMStyle.cpp
@@ +4481,5 @@
> +
> +static void
> +GetFilterFunctionName(nsAString& aString, nsStyleFilter::Type mType)
> +{
> +  switch(mType) {

I missed this one before; space before "(".
Comment 14 Max Vujovic 2013-07-22 15:19:07 PDT
Created attachment 779439 [details] [diff] [review]
Patch v4

Addressed review feedback and rebased patch.
Comment 15 Max Vujovic 2013-07-22 15:19:35 PDT
Comment on attachment 778695 [details] [diff] [review]
Patch v3

Review of attachment 778695 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks again!

> (I should have pointed out that without the specific error messages you
> report, you will still get a generic "Error in parsing value for 'filter'."
> message, but it's good to have the additional information for authors.)

That's pretty cool. I've managed to produce each of the new error messages in the console.

> 
> r=me with a few more changes.
> 
> > I don't totally understand the need for SkipUntil(')') though.
> > An invalid filter function or url makes the entire filter property
> > invalid, and ParseFilter doesn't proceed if one ParseSingleFilter
> > call fails. Besides making ParseSingleFilter more robust if called
> > from somewhere else, is there another reason to call SkipUntil(')')?
> 
> SkipUntil handles balanced parentheses, brackets and braces in the stuff
> that it skips over.  So if you had:
> 
>   <style>p { filter: invalid(; color: green; }</style>
> 
> then CSS parsing rules require all of the "; color: green; }" to be skipped
> while looking for a closing parenthesis to match the "invalid(" token. 
> Without the SkipUntil(')') call, the remainder of
> CSSParserImpl::ParseDeclaration would just search for the ';' to find the
> end of the declarations without knowing that there is a '(' still open.

Thanks for the example! That makes sense.

I took a closer look at ParseTransform, and I think we need to add a SkipUntil(")") to nsCSSParser::ParseFunctionInternals. 

Transforms fail the following test I cooked up:

<style>
div {
    width: 100px;
    height: 100px;

    background-color: green;
    transform: translateX(10px;
    background-color: red;
    /* The above property should not parse because of a mismatched paren. */
}
</style>
<div></div>

Regarding filters in the current patch, the following case will correctly perform the SkipUntil(")") we added:
filter: invalid(10px;

However, the following will not, just like transforms:
filter: blur(10px;

ParseFunctionInternals/ParseFunction is used by the CSS properties "font-variant-alternates", "transform", and now "filter". Adding SkipUntil(")") will probably require factoring it a little differently and adding some new tests. Would you like me to file a separate bug for this or try and fix it in this one?

> 
> > I considered turning the VARIANT_NONNEGATIVE_DIMENSION flag
> > into just VARIANT_NONNEGATIVE and making it capable of
> > handling percentages and numbers. I refrained because I was
> > afraid of slowing down ParseVariant with more checks. Also,
> > it might be better done in a separate patch since it could
> > affect other code paths. What do you think?
> 
> I wondered that too, but I wasn't sure that it would be correct. I don't
> know if there are properties that take, say, percentages and dimensions,
> where the dimension must not be negative but where the percentage can be. 
> David might know.  If it is possible to do this, I would do it in a separate
> bug.

Right, these could be some interesting cases. I won't touch it for now.

> 
> > The style guide says to use pointers for function out parameters
> > [1]. In this file, we mostly use references. In general, would
> > you use references, pointers, or stick with local style?
> 
> (I don't think your [1] URL is right.)
> 
> For strings I think I nearly always see references used.  I'm pretty sure it
> is the prevailing style.  To be honest, I'm not sure what the state of our
> written-down style guides are.  I think I saw a message on dev-platform
> recently saying there were three different ones. :)  So I don't think I have
> any guidance on how to get that written down anyway, sorry.  Thanks though
> for following what style guide information you do find.

Haha all right. No worries! It's easy enough to follow the local style.

> 
> > I'm doing the comparison after mClipPath and mMask- Please let
> > me know if there is a preferred order.
> 
> I don't think it matters, unless we knew that authors overwhelmingly changed
> one property rather than another.

Sounds good.

::: dom/locales/en-US/chrome/layout/css.properties
@@ +138,5 @@
>  PESupportsConditionExpectedNot=Expected 'not' while parsing supports condition but found '%1$S'.
>  PESupportsGroupRuleStart=Expected '{' to begin @supports rule but found '%1$S'.
> +PEFilterEOF=filter
> +PEExpectedURL=Expected URL but found '%1$S'.
> +PEExpectedURLOrFilterFunction=Expected URL or filter function but found '%1$S'.

Done. Good point.

@@ +139,5 @@
>  PESupportsGroupRuleStart=Expected '{' to begin @supports rule but found '%1$S'.
> +PEFilterEOF=filter
> +PEExpectedURL=Expected URL but found '%1$S'.
> +PEExpectedURLOrFilterFunction=Expected URL or filter function but found '%1$S'.
> +PEExpectedNonnegativeNP=Expected nonnegative number or percentage.

Done.

@@ +140,5 @@
> +PEFilterEOF=filter
> +PEExpectedURL=Expected URL but found '%1$S'.
> +PEExpectedURLOrFilterFunction=Expected URL or filter function but found '%1$S'.
> +PEExpectedNonnegativeNP=Expected nonnegative number or percentage.
> +PEUnexpectedFunctionArguments=Unexpected arguments provided to function.

Done. That makes sense. I've changed it to:
PEFilterFunctionArgumentsParsingError=Error in parsing arguments for filter function.

::: layout/style/nsCSSParser.cpp
@@ +10196,5 @@
> +
> +  if (rejectNegativeArgument &&
> +      ((arg.GetUnit() == eCSSUnit_Percent && arg.GetPercentValue() < 0.0f) ||
> +       (arg.GetUnit() == eCSSUnit_Number && arg.GetFloatValue() < 0.0f))) {
> +    REPORT_UNEXPECTED(PEExpectedNonnegativePN);

Done. Thanks!

@@ +10205,5 @@
> +    if (arg.GetUnit() == eCSSUnit_Number &&
> +        arg.GetFloatValue() > 1.0f) {
> +      arg.SetFloatValue(1.0f, arg.GetUnit());
> +    } else if (arg.GetUnit() == eCSSUnit_Percent &&
> +             arg.GetPercentValue() > 1.0f) {

Done.

::: layout/style/nsComputedDOMStyle.cpp
@@ +4481,5 @@
> +
> +static void
> +GetFilterFunctionName(nsAString& aString, nsStyleFilter::Type mType)
> +{
> +  switch(mType) {

Done.
Comment 16 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2013-07-22 17:32:30 PDT
(In reply to Max Vujovic from comment #15)
> I took a closer look at ParseTransform, and I think we need to add a
> SkipUntil(")") to nsCSSParser::ParseFunctionInternals. 
> 
> Transforms fail the following test I cooked up:
> 
> <style>
> div {
>     width: 100px;
>     height: 100px;
> 
>     background-color: green;
>     transform: translateX(10px;
>     background-color: red;
>     /* The above property should not parse because of a mismatched paren. */
> }
> </style>
> <div></div>

Good find!

> Regarding filters in the current patch, the following case will correctly
> perform the SkipUntil(")") we added:
> filter: invalid(10px;
> 
> However, the following will not, just like transforms:
> filter: blur(10px;
> 
> ParseFunctionInternals/ParseFunction is used by the CSS properties
> "font-variant-alternates", "transform", and now "filter". Adding
> SkipUntil(")") will probably require factoring it a little differently and
> adding some new tests. Would you like me to file a separate bug for this or
> try and fix it in this one?

A separate bug would be good, I think.

You can land this patch first, or if you don't have commit access yet, add the checkin-needed keyword to the bug and someone will do it on your behalf.
Comment 17 Dirk Schulze 2013-07-23 05:35:41 PDT
(In reply to Cameron McCormack (:heycam) from comment #16)
> You can land this patch first, or if you don't have commit access yet, add
> the checkin-needed keyword to the bug and someone will do it on your behalf.

Please let me set the keyword for Max, so that there is some activity from me on the bug report itself as well :D
Comment 18 Ryan VanderMeulen [:RyanVM] 2013-07-23 07:58:29 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f24d81b85929
Comment 19 Ed Morley [:emorley] 2013-07-23 08:40:03 PDT
Backed out for:
https://tbpl.mozilla.org/php/getParsedLog.php?id=25611220&tree=Mozilla-Inbound
{
e:\builds\moz2_slave\m-in-w32-d-0000000000000000000\build\layout\style\nsStyleStruct.h(2294) : error C2621: member 'nsStyleFilter::mCoord' of union 'nsStyleFilter::<unnamed-tag>' has copy constructor 
}

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0bc271ee1749
Comment 21 Max Vujovic 2013-07-23 09:49:37 PDT
Thanks Ryan for checking in and Ed for backing out :)

Looks like we are using a C++11 unrestricted union in the patch. I suppose we shouldn't do that. I'll upload a new patch.

I'm working on getting level 1 commit access (bug 896779), so we can run this through the try bots before attempting checkin.
Comment 22 Max Vujovic 2013-07-23 11:12:16 PDT
Created attachment 779887 [details] [diff] [review]
Patch v5

Remove the union in nsStyleFilter in order to be pre-C++11 compatible. The union members, mURL and mCoord, and now direct members of nsStyleFilter.

I considered using a pointer for mCoord so that it could stay in the union, but I decided it wasn't worth the complexity.

heycam - Does this seem ok?
Comment 23 Max Vujovic 2013-07-23 11:23:32 PDT
(In reply to Cameron McCormack (:heycam) from comment #16)
> (In reply to Max Vujovic from comment #15)
> > I took a closer look at ParseTransform, and I think we need to add a
> > SkipUntil(")") to nsCSSParser::ParseFunctionInternals. 
> > 
> > Transforms fail the following test I cooked up:
> > 
> > <style>
> > div {
> >     width: 100px;
> >     height: 100px;
> > 
> >     background-color: green;
> >     transform: translateX(10px;
> >     background-color: red;
> >     /* The above property should not parse because of a mismatched paren. */
> > }
> > </style>
> > <div></div>
> 
> Good find!

Thanks!

> 
> > Regarding filters in the current patch, the following case will correctly
> > perform the SkipUntil(")") we added:
> > filter: invalid(10px;
> > 
> > However, the following will not, just like transforms:
> > filter: blur(10px;
> > 
> > ParseFunctionInternals/ParseFunction is used by the CSS properties
> > "font-variant-alternates", "transform", and now "filter". Adding
> > SkipUntil(")") will probably require factoring it a little differently and
> > adding some new tests. Would you like me to file a separate bug for this or
> > try and fix it in this one?
> 
> A separate bug would be good, I think.
> 
> You can land this patch first, or if you don't have commit access yet, add
> the checkin-needed keyword to the bug and someone will do it on your behalf.

Sounds good. I've added bug 897094 for SkipUntil(")") bug.
Comment 24 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2013-07-23 17:33:19 PDT
(In reply to Max Vujovic from comment #22)
> Remove the union in nsStyleFilter in order to be pre-C++11 compatible. The
> union members, mURL and mCoord, and now direct members of nsStyleFilter.
> 
> I considered using a pointer for mCoord so that it could stay in the union,
> but I decided it wasn't worth the complexity.
> 
> heycam - Does this seem ok?

Actually I wonder whether we can avoid using nsStyleCoord, and instead just store a float.  Since all of the parameters we support so far are numbers or percentages, and I assume the intention is for the spec to treat them similarly (0..1 meaning the same as 0%..100%, although it doesn't actually say!), we might not have to store the unit-ed value.

One issue is that the spec says that the computed value of this is "as specified", which means strictly we'd need to preserve whether a value was a percentage of a number.  I don't think this is useful, though, so I'll send in a comment to public-fx asking for that change.

So can you keep the union and change nsStyleCoord to float?
Comment 25 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2013-07-23 17:38:03 PDT
http://lists.w3.org/Archives/Public/public-fx/2013JulSep/0021.html
Comment 26 Dirk Schulze 2013-07-23 22:47:48 PDT
(In reply to Cameron McCormack (:heycam) from comment #24)
> Actually I wonder whether we can avoid using nsStyleCoord, and instead just
> store a float.  Since all of the parameters we support so far are numbers or
> percentages, and I assume the intention is for the spec to treat them
> similarly (0..1 meaning the same as 0%..100%, although it doesn't actually
> say!), we might not have to store the unit-ed value.
> 
> One issue is that the spec says that the computed value of this is "as
> specified", which means strictly we'd need to preserve whether a value was a
> percentage of a number.  I don't think this is useful, though, so I'll send
> in a comment to public-fx asking for that change.
> 
> So can you keep the union and change nsStyleCoord to float?

The spec says as specified. Therefore you can not omit percentage value. (Would be inconsistent with all other properties anyway.) Furthermore, we started with the simple functions. The next function is hue-rotate which has an angle. You want to treat angle for hue-rotate the same way as on gradients and transforms: with unit! This is again consistent with all other properties.

I believe we should keep the nsStyleCoord.
Comment 27 Dirk Schulze 2013-07-23 23:05:03 PDT
... and I forgot blur() that already has a length value and not number | percentage.
Comment 28 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2013-07-23 23:20:44 PDT
OK, given we'll need nsStyleCoords for other arguments, let's just go with the simple solution for now and not use a union.  If we want to later, we can do something clever to keep it in the union.
Comment 29 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2013-07-23 23:45:48 PDT
I pushed the v5 patch to try: https://tbpl.mozilla.org/?tree=Try&rev=e579134f15c2
Comment 30 Max Vujovic 2013-07-24 09:36:36 PDT
Comment on attachment 779887 [details] [diff] [review]
Patch v5

Setting checkin?.

Thanks for pushing the patch to try. As far as I can tell, the results look good- one seemingly unrelated failure for "browser_inspector_bug_831693_searchbox_panel_navigation.js".
https://tbpl.mozilla.org/?tree=Try&rev=e579134f15c2
Comment 31 Ryan VanderMeulen [:RyanVM] 2013-07-24 13:41:09 PDT
Comment on attachment 779887 [details] [diff] [review]
Patch v5

Just use checkin-needed please
Comment 32 Max Vujovic 2013-07-24 13:46:00 PDT
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #31)
> Comment on attachment 779887 [details] [diff] [review]
> Patch v5
> 
> Just use checkin-needed please

Oops! Didn't realize that was a bug keyword, not a patch flag. Thanks!
Comment 33 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-07-24 14:23:55 PDT
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.

Shouldn't prevent landing this, but maybe fix as a followup?
Comment 34 Max Vujovic 2013-07-24 14:38:42 PDT
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #33)
> 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.
> 
> Shouldn't prevent landing this, but maybe fix as a followup?

Yes, that's a nice mechanism- thanks for pointing it out. I will create a follow-up patch after this one lands.
Comment 35 Ryan VanderMeulen [:RyanVM] 2013-07-24 14:50:58 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f7620a5047a
Comment 36 Wes Kocher (:KWierso) 2013-07-24 19:56:52 PDT
https://hg.mozilla.org/mozilla-central/rev/0f7620a5047a
Comment 37 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-07-26 03:12:34 PDT
(In reply to Max Vujovic from comment #34)
> (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from
> comment #33)
> > 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.
> > 
> > Shouldn't prevent landing this, but maybe fix as a followup?
> 
> Yes, that's a nice mechanism- thanks for pointing it out. I will create a
> follow-up patch after this one lands.

This is bug 898175.
Comment 38 Francesco Lodolo [:flod] - OFFLINE Jun 26-Jul 3 2013-07-27 07:58:10 PDT
Can you please explain how this string is used?
PEFilterEOF=filter

Is it a noun, a verb? Is it used alone or with other strings? I checked the code but I couldn't really understand it
Comment 39 Max Vujovic 2013-07-29 10:29:05 PDT
(In reply to Francesco Lodolo [:flod] from comment #38)
> Can you please explain how this string is used?
> PEFilterEOF=filter
> 
> Is it a noun, a verb? Is it used alone or with other strings? I checked the
> code but I couldn't really understand it

It is a noun that is placed inside another string "PEUnexpEOF2".

If you look at "dom/locales/en-US/chrome/layout/css.properties" and search for "EOF", you will see similar strings like "PEColorEOF" or "PEColorHueEOF" that are just a word or a phrase.

These are used in various places in nsCSSParser.cpp like this:
REPORT_UNEXPECTED_EOF(PEFilterEOF);
REPORT_UNEXPECTED_EOF(PEColorEOF);
REPORT_UNEXPECTED_EOF(PEColorHueEOF);

The macro expands to:
#define REPORT_UNEXPECTED_EOF(lf_) \
  mReporter->ReportUnexpectedEOF(#lf_)

Thus, the macro calls "ErrorReporter::ReportUnexpectedEOF(const char *aMessage)".

Inside ErrorReporter::ReportUnexpectedEOF, there is a call:
  sStringBundle->FormatStringFromName(NS_LITERAL_STRING("PEUnexpEOF2").get(),
                                      params, ArrayLength(params),
                                      getter_Copies(str));

This call uses "PEUnexpEOF2", which is defined in the css.properties file as:
PEUnexpEOF2=Unexpected end of file while searching for %1$S.

So, our PEFilterEOF, PEColorEOF, or PEColorHueEOF strings get inserted near the end of the PEUnexpEOF2 sentence.
Comment 40 Francesco Lodolo [:flod] - OFFLINE Jun 26-Jul 3 2013-07-29 11:00:59 PDT
Thanks :-)
Comment 41 Jean-Yves Perrier [:teoli] 2014-09-12 04:45:02 PDT
This has been documented when it was activated by default.

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