Closed Bug 895182 Opened 11 years ago Closed 11 years ago

[CSS Filters] Implement parsing for blur, brightness, contrast, grayscale, invert, opacity, saturate, sepia

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla25

People

(Reporter: mvujovic, Assigned: mvujovic)

References

Details

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

Attachments

(1 file, 4 obsolete files)

Patch v5
40.56 KB, patch
Details | Diff | Splinter Review 
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.
Attached patch Patch (obsolete) — — Splinter Review 
Here is a patch that Dirk Schulze and I have authored.
Attachment #777461 - Flags: review?(robert)
Did you test that the mochitests in layout/style pass with the default value of the layout.css.filters.enabled pref flipped to true?
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.
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.
Flags: needinfo?(dbaron)
Minor drive-by comment: can you make the nsStyleFilter::Type values begin with "e" rather than "k"?  The latter should only be for consts.
(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
Attached patch Patch v2 (obsolete) — — Splinter Review 
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.
Attachment #777461 - Attachment is obsolete: true
Attachment #777461 - Flags: review?(robert)
Attachment #777954 - Flags: review?
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.
Attachment #777954 - Flags: review? → review?(cam)
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".
Attachment #777954 - Flags: review?(cam)
Attached patch Patch v3 (obsolete) — — Splinter Review 
Patch to address review feedback.
Attachment #777954 - Attachment is obsolete: true
Attachment #778695 - Flags: review?
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".
Attachment #777954 - Attachment is obsolete: false
Removing the needinfo flag I set for David, since Cameron addressed my question.
Flags: needinfo?(dbaron)
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 "(".
Attachment #778695 - Flags: review? → review+
Attached patch Patch v4 (obsolete) — — Splinter Review 
Addressed review feedback and rebased patch.
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.
(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.
(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
Keywords: checkin-needed
Attachment #777954 - Attachment is obsolete: true
Attachment #778695 - Attachment is obsolete: true
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
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.
Attached patch Patch v5 — — Splinter Review 
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?
Attachment #779439 - Attachment is obsolete: true
Flags: needinfo?(cam)
(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.
(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?
Flags: needinfo?(cam)
(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.
... and I forgot blur() that already has a length value and not number | percentage.
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.
Blocks: 897392
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
Attachment #779887 - Flags: checkin?
Comment on attachment 779887 [details] [diff] [review]
Patch v5

Just use checkin-needed please
Attachment #779887 - Flags: checkin?
(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!
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?
(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.
https://hg.mozilla.org/mozilla-central/rev/0f7620a5047a
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(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.
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
(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.
Thanks :-)
Blocks: 878883
Whiteboard: [DocArea=CSS]
This has been documented when it was activated by default.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: