In bug 1216843 I was going to extend PercentageRGBAColor to store values which are over 100% as intermediate values of accumulation. Daniel suggested that we should open a new bug with tests for it in bug 1216843 comment 112. From the spec[1]; Values outside these ranges are not invalid, but are clamped to the ranges defined here at computed-value time. https://drafts.csswg.org/css-color/#rgb-functions

I've started writing test cases. I know we need to fix the color parser and its serialization, but even so, the results seems weird. For example; input value output value obtained by style.color rgb(1e+39, 0, 0) empty string rgb(0e-39, 0, 0) empty string rgb(1e+39%, 0%, 0%) rgb(255, 0, 0) <= this is correct! rgba(0, 0, 0, 1e+39) empty string rgba(0%, 0%, 0%, 1e+39%) empty string rgb(-0.1, 0, 0) empty string OK, I found we consider non-integer values as invalid[1]. We need to fix it too. [1] https://hg.mozilla.org/mozilla-central/file/tip/layout/style/nsCSSParser.cpp#l6817

Oops! We don't support percentage alpha value?

Filed two bugs that I've noticed while I was writing test cases; bug 1295451 - decimal color component bug 1295456 - percentage alpha

Daniel, I need you advises. You suggested that below case we should return the color as it set, i.e. rgb(150%,0%,0%). element.style.color = "rgb(150%,0%,0%)"; But currently we return a normalized value like rgb(255, 0, 0) even if the color style is set by hsl() function. Did you mean that we should change this behavior?

Yes. ("element.style.color" [or the equivalent for a non-inline stylesheet] should return a serialization of our internal representation of the specified color. If our internal representation behaves equivalently to rgb(255, 0, 0) (e.g. for rendering & animation purposes), then that's a reasonable serialization. That's not the case for rgb(150%,0%,0%) -- it'd produce different animation behavior & different rendering during animation, once this bug is fixed. [I think] So it should serialize to something that we could faithfully re-parse to produce the same representation -- probably just rgb(150%, 0%, 0%).)

(Computed style is a different / more complex matter, perhaps. I'm not entirely sure if we need to preserve overflowed values there as well [and display them in getComputedStyle()]. I'm just talking about nsCSSValue so far.)

(In reply to Daniel Holbert [:dholbert] from comment #5) > Yes. > > ("element.style.color" [or the equivalent for a non-inline stylesheet] > should return a serialization of our internal representation of the > specified color. > > If our internal representation behaves equivalently to rgb(255, 0, 0) (e.g. > for rendering & animation purposes), then that's a reasonable serialization. > That's not the case for rgb(150%,0%,0%) -- it'd produce different animation > behavior & different rendering during animation, once this bug is fixed. [I > think] So it should serialize to something that we could faithfully > re-parse to produce the same representation -- probably just rgb(150%, 0%, > 0%).) Ah, thanks, great. It makes quite sense to me.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b4347ac454d Hmm, all tests on debug build did not run at all. Try syntax has been changed?

Looks like those tests did end up running eventually. They do have the "tc" prefix (tc-R, tc-M, etc.) to indicate that they're driven by the new TaskCluster infrastructure, though.

Comment on attachment 8781846 [details] Bug 1295107 - Part 1: Extend eCSSUnit_PercentageRGBAColor/eCSSUnit_PercentageRGBColor to store values greater than 100%. https://reviewboard.mozilla.org/r/72178/#review70834 This is nearly r+, but not quite there. ::: layout/style/nsCSSValue.h:378 (Diff revision 1) > eCSSUnit_HexColor = 83, // (nscolor) an opaque RGBA value specified as #rrggbb > eCSSUnit_ShortHexColor = 84, // (nscolor) an opaque RGBA value specified as #rgb > eCSSUnit_HexColorAlpha = 85, // (nscolor) an opaque RGBA value specified as #rrggbbaa > eCSSUnit_ShortHexColorAlpha = 86, // (nscolor) an opaque RGBA value specified as #rgba > - eCSSUnit_PercentageRGBColor = 87, // (nsCSSValueFloatColor*) > - eCSSUnit_PercentageRGBAColor = 88, // (nsCSSValueFloatColor*) > + eCSSUnit_PercentageRGBColor = 87, // (nsCSSValueFloatColor*) Each RGB > + // component stores as percent but it "stores as percent" is not quite correct, both grammatically & factually. (These values were *specified* with percentages, but they're *stored* as float values on a 0.0-1.0 scale.) Let's replace this with something like the following, to better-match the spec text that we use above for other color units: eCSSUnit_PercentageRGBColor = 87, // (nsCSSValueFloatColor*) an opaque // RGBA value specified as rgb() with // percentage components. Values over // 100% are allowed. ::: layout/style/nsCSSValue.h:381 (Diff revision 1) > eCSSUnit_ShortHexColorAlpha = 86, // (nscolor) an opaque RGBA value specified as #rgba > - eCSSUnit_PercentageRGBColor = 87, // (nsCSSValueFloatColor*) > - eCSSUnit_PercentageRGBAColor = 88, // (nsCSSValueFloatColor*) > + eCSSUnit_PercentageRGBColor = 87, // (nsCSSValueFloatColor*) Each RGB > + // component stores as percent but it > + // allows overflow values, > + // i.e. over 100%. > + eCSSUnit_PercentageRGBAColor = 88, // (nsCSSValueFloatColor*) same as And this should use the same suggested text as above, except now with "rgba()" instead of "rgb()", and without the word "opaque". ::: layout/style/nsCSSValue.h:1699 (Diff revision 1) > bool operator==(nsCSSValueFloatColor& aOther) const; > > nscolor GetColorValue(nsCSSUnit aUnit) const; > + float Red() const { return mComponent1; } > + float Green() const { return mComponent2; } > + float Blue() const { return mComponent3; } These component-names are semi-bogus, if we happen to be a HSL value. In that case, e.g. "Red()" would really return the Hue, Green() would be return the Saturation, etc. Not sure how best to address this... Maybe these should be named Comp1(), Comp2(), Comp3()? That seems like it'll make calling code consider these a bit more carefully and not accidentally misinterpret HSL values as Red/Green/Blue. ::: layout/style/nsCSSValue.h:1712 (Diff revision 1) > NS_INLINE_DECL_REFCOUNTING(nsCSSValueFloatColor) > > private: > - // FIXME: We should not be clamping specified RGB color components. > - float mComponent1; // 0..1 for RGB, 0..360 for HSL > - float mComponent2; // 0..1 > + // The range of each component is; > + // [0, 1] for RGBColor, RGBAColor > + // [0, 1], or [0, float::max()] for PercentrageRGBColor, PercentageRGBAColor "PercentrageRGBColor" is a typo. (you've got an extra "r" -- s/trage/tage/) Also, "[0, 1], or [0, float::max()]" is unclear. What does the "or" mean here? When is it one and when is it the other? It'd also be worth expressing (somehow) that in the [0, float::max()] representation, 1.0 still represents 100%. ::: layout/style/nsCSSValue.h:1713 (Diff revision 1) > > private: > - // FIXME: We should not be clamping specified RGB color components. > - float mComponent1; // 0..1 for RGB, 0..360 for HSL > - float mComponent2; // 0..1 > - float mComponent3; // 0..1 > + // The range of each component is; > + // [0, 1] for RGBColor, RGBAColor > + // [0, 1], or [0, float::max()] for PercentrageRGBColor, PercentageRGBAColor > + // [0, 360] for HSL This comment is saying that *each component* is [0, 360] for HSL, but that's not true. I think only the first one (Hue) has that 360-unit range, and the other channels all use the range [0, 1] in this internal representation (unless/until we allow them to overflow). At least, that's what the old code-comments here said. ::: layout/style/nsCSSValue.cpp:2871 (Diff revision 1) > nsCSSValueFloatColor::GetColorValue(nsCSSUnit aUnit) const > { > MOZ_ASSERT(nsCSSValue::IsFloatColorUnit(aUnit), "unexpected unit"); > > + // We should clamp each component value since eCSSUnit_PercentageRGBColor > + // or eCSSUnit_PercentageRGBAColor stores values greater than 1.0. Two nits: (1) s/or eCSSUnit_PercentageRGBAColor/and eCSSUnit_PercentageRGBAColor/ (2) The phrase "stores values greater than 1.0" is a bit confusing here -- that implies that it's *usual/expected* that these values will be greater than 1. (but really that's not the usual case) So: please replace "stores" with "may store", or with "allows".

Comment on attachment 8781847 [details] Bug 1295107 - Part 2: Change CSS Parser to no longer clamp out-of-range values in parsed rgb()/rgba() percentage expressions. https://reviewboard.mozilla.org/r/72180/#review70838 r=me with the following: ::: layout/style/nsCSSParser.cpp:1171 (Diff revision 1) > // ParseColorOpacity will enforce that the color ends with a ')' > // after the opacity > bool ParseColorOpacity(uint8_t& aOpacity); > - bool ParseColorOpacity(float& aOpacity); > + > + // FIXME: Once both of eCSSUnit_RGBAColor and eCSSUnit_HSLAColor allow > + // opacity values over 1.0, this enum class should be removed. Do we already have a bug covering the HSL opacity-over-1.0 changes? If not, please file one, and mark it as depending on this one. Either way, please mention its bug number in this comment. ::: layout/style/nsCSSParser.cpp:1173 (Diff revision 1) > bool ParseColorOpacity(uint8_t& aOpacity); > - bool ParseColorOpacity(float& aOpacity); > + > + // FIXME: Once both of eCSSUnit_RGBAColor and eCSSUnit_HSLAColor allow > + // opacity values over 1.0, this enum class should be removed. > + enum class ParseColorOpacityType { > + Clamped, // Clamp value in the range of [0, 1]. s/in the range of/to the range/ ::: layout/style/nsCSSParser.cpp:6857 (Diff revision 1) > - float value = mToken.mNumber; > - if (value < 0.0f) value = 0.0f; > - if (value > 1.0f) value = 1.0f; > - > if (ExpectSymbol(aStop, true)) { > - aComponent = value; > + aComponent = mToken.mNumber; The old code captured mToken.mNumber in a local variable, *before* calling ExpectSymbol. I'm pretty sure we still need to do that, because ExpectSymbol advances the mToken forward to the next token in the stream, i.e. off of the number. So I'd assume you might be reading a bogus mToken.mNumber value here... So, please restore this code to capturing mToken.mNumber in a local variable *before* the GetSymbol call. (ExpectSymbol's first action is to call "GetToken()", which updates the contents of |mToken|.) ::: layout/style/test/chrome/test_author_specified_style.html:17 (Diff revision 1) > "rgba(10,20,30,0.250)", "rgba(10, 20, 30, 0.25)", > "OrangeRed", "OrangeRed", > "rgb(10%,25%,99%)", "rgb(10%, 25%, 99%)", > "rgb(6.66667%,0%,0.0%)", "rgb(6.66667%, 0%, 0%)", > + "rgb(101%,0%,0%)", "rgb(101%, 0%, 0%)", > + "rgb(-1%,0%,0%)", "rgb(-1%, 0%, 0%)", Please include some values with out-of-bounds values for the green & blue color-channels, too. ::: layout/style/test/test_rgb_colors_out_of_range.html:22 (Diff revision 1) > + ], > + "rgb(0, 0, 0)": [ > + "rgb(-1, 0, 0)", > + "rgb(-1%, 0%, 0%)", > + "rgb(-0.1%, 0%, 0%)", > + "rgb(-1e+39%, 0%, 0%)", Please include a few lines that have negative values in the green & blue components, too. Those color-channels don't seem to be tested for clamping at all in this test. ::: layout/style/test/test_rgb_colors_out_of_range.html:41 (Diff revision 1) > + > +for (var property in gTestCases) { > + gTestCases[property].forEach(function(style) { > + test(function() { > + var div = document.createElement("div"); > + div.style.color = style; "property" and "style" are too generic for variable names here. (And these words are confusing here, in a CSS test, because they have different and/or broader meanings in the context of CSS.) Please rename "property" to "expectedComputedVal", and rename "style" to "colorValToTest". (or something like that) ::: layout/style/test/test_rgb_colors_out_of_range.html:43 (Diff revision 1) > + gTestCases[property].forEach(function(style) { > + test(function() { > + var div = document.createElement("div"); > + div.style.color = style; > + assert_equals(getComputedStyle(div).color, property, > + "getComputedStyle should return clamped value : " + style); The text of this message (and the one below it) seem to be saying that |style| *is* the clamped result that we're expecting to find. But, that's not what it is -- really, |style| is the input value that we're testing. Please reword to avoid that confusion -- maybe replace with: "getComputedStyle should return clamped version of " + style); (note that |style| should be renamed as well, as I noted above)

Comment on attachment 8781847 [details] Bug 1295107 - Part 2: Change CSS Parser to no longer clamp out-of-range values in parsed rgb()/rgba() percentage expressions. https://reviewboard.mozilla.org/r/72180/#review70904 ::: layout/style/nsCSSParser.cpp:1161 (Diff revision 1) > > int32_t ParseChoice(nsCSSValue aValues[], > const nsCSSProperty aPropIDs[], int32_t aNumIDs); > CSSParseResult ParseColor(nsCSSValue& aValue); > bool ParseNumberColorComponent(uint8_t& aComponent, char aStop); > bool ParsePercentageColorComponent(float& aComponent, char aStop); One more thing: the commit message on part 2 needs clarifying, too. (I'm filing this as an "issue" for the first line of the patch, simply so that it can be tracked as an "issue" somewhere -- MozReview doesn't let me create issues on the commit message itself.) So, right now, the commit message is: > Part 2: Allow setting over 1.0 values to eCSSUnit_PercentageRGBColor and eCSSUnit_PercentageRGBAColor. ...but that actually sounds more like a description of "part 1", not "part 2". This part is entirely about the CSS parser, so it should probably say that. Suggested replacement: "Part 2: Change CSS Parser to no longer clamp out-of-range values in parsed rgb()/rgba() percentage expressions."

Comment on attachment 8781848 [details] Bug 1295107 - Part 3: When serializing CSS rgb()/rgba() specified values, preserve out-of-range components (for e.g. elem.style.color queries). https://reviewboard.mozilla.org/r/72182/#review70906 r=me with nits addressed: ::: layout/style/nsCSSValue.h:626 (Diff revision 1) > uint32_t len = NS_strlen(GetBufferValue(mValue.mString)); > mValue.mString->ToString(len, aBuffer); > return aBuffer; > } > > const char16_t* GetStringBufferValue() const (as above, I'm filing a commit-message nit as an "issue" for the first line of the patch, because MozReview.) The commit message currently says: > element.style.color should return the values which are out of range RGB colors as it is. Two concerns here: (1) It's best to avoid words like "should" in commit messages. That sounds like it's stating an expectation or a desire, which isn't the sort of concept to put in a commit message. The commit message should be a direct description of the change or the new behavior. (2) The current commit-message implies that "element.style.color" is the only thing that's changing. But really, this is changing for *all* color-valued properties, for serialization of specified values in *all* types of CSS stylesheets (not just elem.style) -- so it's really much broader. If you like, though, you could include elem.style.color as an *example* in the commit message, but it should be clearer that it's only an example that represents a broader group. SO, please reword the commit message here to something like: Part 3: When serializing CSS rgb()/rgba() specified values, preserve out-of-range components (for e.g. elem.style.color queries) ::: layout/style/nsCSSValue.h:635 (Diff revision 1) > } > > nscolor GetColorValue() const; > bool IsNonTransparentColor() const; > + // Returns true if this value has eCSSUnit_PercentageRGBColor or > + // eCSSUnit_PercentageRGBAColor and one of its component has the value s/component/components/ s/has the value/has a value/ ::: layout/style/nsCSSValue.h:636 (Diff revision 1) > > nscolor GetColorValue() const; > bool IsNonTransparentColor() const; > + // Returns true if this value has eCSSUnit_PercentageRGBColor or > + // eCSSUnit_PercentageRGBAColor and one of its component has the value > + // which is out of range [0, 1]. s/out of range/outside the range/ ::: layout/style/nsCSSValue.cpp:800 (Diff revision 1) > +bool > +nsCSSValue::IsOutOfRangeRGBColor() const > +{ > + return (mUnit == eCSSUnit_PercentageRGBColor || > + mUnit == eCSSUnit_PercentageRGBAColor) && > + (mValue.mFloatColor->Red() < 0.0 || (Per my notes for "part 1", these accessors's Red()/Green()/Blue() names will probably need to change.) ::: layout/style/test/test_rgb_colors_out_of_range.html:55 (Diff revision 1) > + "rgb(101%, 0%, 0%)", > + "rgb(100.1%, 0%, 0%)", > + "rgb(1e+38%, 0%, 0%)", > + "rgb(-1%, 0%, 0%)", > + "rgb(-0.1%, 0%, 0%)", > + "rgb(-1e+38%, 0%, 0%)", Please include some values with out-of-range color values in other channels. Please also include some rgba() values here (with out-of-range alpha values) ::: layout/style/test/test_rgb_colors_out_of_range.html:56 (Diff revision 1) > + "rgb(100.1%, 0%, 0%)", > + "rgb(1e+38%, 0%, 0%)", > + "rgb(-1%, 0%, 0%)", > + "rgb(-0.1%, 0%, 0%)", > + "rgb(-1e+38%, 0%, 0%)", > +].forEach(function(style) { As noted for the other test code in "part 2", "style" is too vague of a name here. Maybe name this variable "colorValToTest" like above?

Could you file a followup on fixing the CSS parser to support out-of-range numeric color-component values, too? The spec-text you quoted in comment 0 isn't specific to percentages, I think -- it applies to the 0...255 range as well.

Filed bug 1299310. (In reply to Daniel Holbert [:dholbert] from comment #13) > ::: layout/style/nsCSSValue.h:1699 > (Diff revision 1) > > bool operator==(nsCSSValueFloatColor& aOther) const; > > > > nscolor GetColorValue(nsCSSUnit aUnit) const; > > + float Red() const { return mComponent1; } > > + float Green() const { return mComponent2; } > > + float Blue() const { return mComponent3; } > > These component-names are semi-bogus, if we happen to be a HSL value. In > that case, e.g. "Red()" would really return the Hue, Green() would be return > the Saturation, etc. > > Not sure how best to address this... Maybe these should be named Comp1(), > Comp2(), Comp3()? That seems like it'll make calling code consider these a > bit more carefully and not accidentally misinterpret HSL values as > Red/Green/Blue. I was thinking about these name but I did not come up with any nicer names. I will take Comp1(), etc. here. Thanks!

Duplicate of bug 515919? Also, is it really a good idea to use floats here? There's some risk with loss of precision. In bug 515919 I was planning to switch from 8-bit to 16-bit fixed-point.

Then again, maybe floats are OK when the colors were actually specified as percentages.

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #19) > Duplicate of bug 515919? Perhaps we should consider bug 515919 a metabug, with this bug here & bug 1299310 being its two pieces? > Also, is it really a good idea to use floats here? There's some risk with > loss of precision. [...] > Then again, maybe floats are OK when the colors were actually specified as > percentages. Yeah -- also, we already use floats for these units (eCSSUnit_Percentage***Color) -- the only thing changing here is whether we allow those floats to be outside the [0.0f, 1.0f] range.

(In reply to Daniel Holbert [:dholbert] from comment #21) > (In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain > patch) from comment #19) > > Duplicate of bug 515919? > > Perhaps we should consider bug 515919 a metabug, with this bug here & bug > 1299310 being its two pieces? OK. There are also rendering pieces, though.

That said, do other engines do this, or do they clamp at parse time like we do? Given that css-color-4 drops the feature of allowing out-of-sRGB-gamut values in rgb(), the old bug is actually mostly invalid at this point. So then the question is whether what it's specifying is actually compatible with some engines, or whether it's mandating a change for all engines for no good reason.

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #23) > That said, do other engines do this, or do they clamp at parse time like we > do? I think they clamp at parse-time like we do (or maybe at serialization time; not sure how to distinguish between those possibilities). Testcase for checking whether clamping happens at some point during parsing/serialization of "rgba(150%, -50%, 0%, 1.5)": https://jsfiddle.net/d6fh7vuy/ Chrome 54, Firefox 51 Nightly, & Safari 9.1 all serialize that as "rgb(255, 0, 0)". Edge 14 essentially agrees, though it uses "rgba(255, 0, 0, 1)". > So then the question is whether what it's specifying is actually compatible > with some engines, or whether it's mandating a change for all engines for no > good reason. Not sure if there's a good reason. I assumed there was, based on the spec and based on the fact that we have an XXX comment in our code saying that we should relax the clamping. :) I think we do need to be able to internally represent out-of-bounds color-valued nsCSSValue objects, for e.g. the scenario in bug 1216843 comment 6 (repeating additive web animations, basically -- where I assume we try to compute the final endpoint ahead of time, and the final endpoint might have an out-of-bounds color channel). But if we (and the css-color-4 spec) don't have good reasons to expose that to web authors, then we could just disregard "part 2" and "part 3" here I think, and take a version of "part 1" on its own, for the benefit of bug 1216843.

sorry, I meant to link to the XXX comment I mentioned above -- and it's actually a FIXME, from bug 731271: > class nsCSSValueFloatColor final { > [...] > // FIXME: We should not be clamping specified RGB color components. https://dxr.mozilla.org/mozilla-central/rev/26e22af660e543ebb69930f082188b69ec756185/layout/style/nsCSSValue.h#1699

(In reply to Daniel Holbert [:dholbert] from comment #24) > I think we do need to be able to internally represent out-of-bounds > color-valued nsCSSValue objects, for e.g. the scenario in bug 1216843 > comment 6 (repeating additive web animations, basically -- where I assume we > try to compute the final endpoint ahead of time, and the final endpoint > might have an out-of-bounds color channel). But if we (and the css-color-4 > spec) don't have good reasons to expose that to web authors, then we could > just disregard "part 2" and "part 3" here I think, and take a version of > "part 1" on its own, for the benefit of bug 1216843. Yes, and I think some of test cases in part 2 could be re-used whatever we do in this bug.

Comment on attachment 8781848 [details] Bug 1295107 - Part 3: When serializing CSS rgb()/rgba() specified values, preserve out-of-range components (for e.g. elem.style.color queries). https://reviewboard.mozilla.org/r/72182/#review70906 > (as above, I'm filing a commit-message nit as an "issue" for the first line of the patch, because MozReview.) > > The commit message currently says: > > element.style.color should return the values which are out of range RGB colors as it is. > > Two concerns here: > (1) It's best to avoid words like "should" in commit messages. That sounds like it's stating an expectation or a desire, which isn't the sort of concept to put in a commit message. The commit message should be a direct description of the change or the new behavior. > > (2) The current commit-message implies that "element.style.color" is the only thing that's changing. But really, this is changing for *all* color-valued properties, for serialization of specified values in *all* types of CSS stylesheets (not just elem.style) -- so it's really much broader. If you like, though, you could include elem.style.color as an *example* in the commit message, but it should be clearer that it's only an example that represents a broader group. > > > SO, please reword the commit message here to something like: > Part 3: When serializing CSS rgb()/rgba() specified values, preserve out-of-range components (for e.g. elem.style.color queries) Thanks for the correction and detail explanation! I'd been using 'should' in commit message many times! This will be really helpful to write commit messages.

ReviewBoard discarded lots of replies... So I am replying directly on bugzilla. (In reply to Daniel Holbert [:dholbert] from comment #14) > ::: layout/style/nsCSSParser.cpp:6857 > (Diff revision 1) > > - float value = mToken.mNumber; > > - if (value < 0.0f) value = 0.0f; > > - if (value > 1.0f) value = 1.0f; > > - > > if (ExpectSymbol(aStop, true)) { > > - aComponent = value; > > + aComponent = mToken.mNumber; > > The old code captured mToken.mNumber in a local variable, *before* calling > ExpectSymbol. I'm pretty sure we still need to do that, because > ExpectSymbol advances the mToken forward to the next token in the stream, > i.e. off of the number. So I'd assume you might be reading a bogus > mToken.mNumber value here... > > So, please restore this code to capturing mToken.mNumber in a local variable > *before* the GetSymbol call. Thank! That's really good to know! I did not know that at all.

If we still need a consensus about part 2 and 3, I will land only part 1 patch once it gets r+.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #27) > Thanks for the correction and detail explanation! I'd been using 'should' > in commit message many times! This will be really helpful to write commit > messages. Sure! BTW, the canonical documentation for Gecko commit message best-practices (along with a few examples) is here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Checkin_comment

Comment on attachment 8781846 [details] Bug 1295107 - Part 1: Extend eCSSUnit_PercentageRGBAColor/eCSSUnit_PercentageRGBColor to store values greater than 100%. https://reviewboard.mozilla.org/r/72178/#review73508 Several nits on the extended commit message -- quoting: > GetColorValue() for eCSSUnit_PercentageRGBAColor/eCSSUnit_PercentageRGBColor > returns a clamped value, Red/Green/Blue/Alpha() returns a raw value instead. (1) Red/Green/Blue are no longer the name of these accessors. They're named Comp1/Comp2/Comp3 now. Please update that here. (2) The functions you're mentioning here (GetColorValue vs. those ^ new per-component accessors) are methods on *different classes*. Please include the class name so it's a bit clearer which APIs you're comparing. i.e. add nsCSSValue:: before "GetColorValue()", and add "nsCSSValueFloatColor::" before the component list. ::: layout/style/nsCSSValue.h:1705 (Diff revision 2) > > nscolor GetColorValue(nsCSSUnit aUnit) const; > + float Comp1() const { return mComponent1; } > + float Comp2() const { return mComponent2; } > + float Comp3() const { return mComponent3; } > + float Alpha() const { return mAlpha; } Observation (probably no action needed): These functions (as well as GetFloatColorValue) don't have any callers that are being added in this bug -- BUT, I think we need them for bug 1216843 part 7 ("Implement color accumulation"). So they're dead code for now, but that's OK with me, assuming their client-code in bug 1216843 land in the near future. (Perhaps this should just wait to land until that bug is also ready to land, in the interests of avoiding dead code in the meantime.) ::: layout/style/nsCSSValue.h:1715 (Diff revision 2) > size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const; > > NS_INLINE_DECL_REFCOUNTING(nsCSSValueFloatColor) > > private: > - // FIXME: We should not be clamping specified RGB color components. > + // The range of each component is; nit: this line should end in a colon, not a semicolon. ::: layout/style/nsCSSValue.h:1716 (Diff revision 2) > > NS_INLINE_DECL_REFCOUNTING(nsCSSValueFloatColor) > > private: > - // FIXME: We should not be clamping specified RGB color components. > - float mComponent1; // 0..1 for RGB, 0..360 for HSL > + // The range of each component is; > + // [0, 1] for HSLColor and HSLAColor. This change isn't quite correct -- it's losing track of the fact that the "H" component has [0, 360] as its range. Please fix the documentation to mention that (and to avoid mistakenly implying that H ranges from 0 to 1)

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a5660157c34ae38c44b0c00853428ea269cebb2 Bug 1295107 - Part 1: Extend eCSSUnit_PercentageRGBAColor/eCSSUnit_PercentageRGBColor to store values greater than 100%. r=dholbert

Just landed only part 1.

The leave-open keyword is there and there is no activity for 6 months. :hiro, maybe it's time to close this bug?

Oh right, the other parts is no longer needed since we've already migrated to Stylo.