Closed Bug 731271 Opened 12 years ago Closed 10 years ago

Make "Authored" color values available through style system

Categories

(Core :: DOM: CSS Object Model, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: miker, Assigned: heycam)

References

Details

Attachments

(7 files, 7 obsolete files)

10.04 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
17.04 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
16.19 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
59.50 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
5.71 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
7.68 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
2.69 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
In the developer tools style panels we would like to display authored styles by default. These authored styles are the actual values that are in the users stylesheet e.g. #f00 instead of rgb(255, 0, 0), which is what most users would like to see.

Our problem is that it does not appear that we have any access to the property values from the author's stylesheet.
Gecko does not store the original string the color was specified as.  Parsing a color production in our CSS parser outputs a single 32-bit integer representing an RGBA color specification (unless it's a named color, in which case it outputs the string).  At that point, we no longer know whether the original color was in the #nnn form, or #nnnnnn or rgb() or rgba() or hsl() or hsla().
Summary: Make "Authored" styles available through getComputedStyle() or similar → Make "Authored" styles available through style system
Michael, do you need the exact string that was used, or just something closer to the syntactic form that was used?  I'm a bit worried about having to store the exact text of every CSS property value, since it's going to use a bunch of memory, even if you weren't going to query the authored styles.

We could limit that if we just stored whether, for colours, say, it was #nnn, #nnnnnn, rgb(), etc.  And normalise the capitalisation of the hex letters, the rgb() values to integers, and so on.
Flags: needinfo?(mratcliffe)
(In reply to Cameron McCormack (:heycam) from comment #2)
> Michael, do you need the exact string that was used, or just something
> closer to the syntactic form that was used?  I'm a bit worried about having
> to store the exact text of every CSS property value, since it's going to use
> a bunch of memory, even if you weren't going to query the authored styles.
> 
> We could limit that if we just stored whether, for colours, say, it was
> #nnn, #nnnnnn, rgb(), etc.  And normalise the capitalisation of the hex
> letters, the rgb() values to integers, and so on.

That would actually be perfect.
Flags: needinfo?(mratcliffe)
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #752583 - Flags: review?(dbaron)
Attached patch Part 5: Test. (obsolete) — Splinter Review
Attachment #752588 - Flags: review?(dbaron)
So this adds CSSDeclaration.getAuthoredPropertyValue(), which only returns values different from getPropertyValue() for things which have colours in them.
(CSSStyleDeclaration, sorry.)
Attached patch Part 5: Test. (v1.1) (obsolete) — Splinter Review
Test some other properties too.
Attachment #752588 - Attachment is obsolete: true
Attachment #752588 - Flags: review?(dbaron)
Attachment #752596 - Flags: review?(dbaron)
For the authored property value, I have two suggestions:

1)  Don't bother to add it to the XPCOM interface, unless we actually have non-WebIDL
    implementations where we want to use it (which we shouldn't).
2)  Put the Mozilla-specific thing on a separate partial interface documented to hold
    Mozilla-specific things.
dbaron: Review ping for Cameron's patches. Thanks!
Comment on attachment 752583 [details] [diff] [review]
Part 1: Add function to convert RGB to HSL.

r=dbaron, though I didn't review the math thoroughly.

Though it seems like you can drop these three " + (delta / 2)":

>+    float delta_r = (((max - r) / 6.0f) + (delta / 2)) / delta;
>+    float delta_g = (((max - g) / 6.0f) + (delta / 2)) / delta;
>+    float delta_b = (((max - b) / 6.0f) + (delta / 2)) / delta;

given that they'll all cancel out:

>+    if (r == max) {
>+      *h = delta_b - delta_g;
>+    } else if (g == max) {
>+      *h = 1.0f / 3.0f + delta_r - delta_b;
>+    } else {
>+      *h = 2.0f / 3.0f + delta_g - delta_r;
>+    }
Attachment #752583 - Flags: review?(dbaron) → review+
Comment on attachment 752584 [details] [diff] [review]
Part 2: Store on nsCSSValue the syntactic form of color values.

I'm not crazy about the _Default behavior for the HTML cases (which are
either hex or keyword), but it's probably easier for now than adding
another case (since 'transparent' can't be hex).

I'm not happy with the eColorSyntax_Component*_Integer stuff.  Can you
just drop it for hsl(), or are there hsl values that can't be expressed
as integers?  And then can you reuse nsStyleUtil::ColorComponentToFloat
for rgb() with percentages?  That seems preferable to spitting out
six-decimal-place floats whenever 50.0% is used.  Or is there a reason
I'm missing to have this?  (You can also remove the new third param
to ParseColorComponent... which you also don't use correctly!  And also
the change to ScanNumber and the comment in nsCSSScanner.h.)

r=dbaron with that

I think this patch would have been nice split into more than one patch;
the type addition and the nsCSSParser refactorings could have been one
patch, and then the SetColorValue change and the storage another (or
even the actual storage of the value in a third).

And would you mind filing (and maybe fixing) a followup bug to remove
support for -moz-rgba() and -mos-hsla()?
Attachment #752584 - Flags: review?(dbaron) → review+
Comment on attachment 752585 [details] [diff] [review]
Part 3: Add ability for nsCSSValues to serialize themselves closer to their authored form, beginning with color values.

Ugly, but I guess it's ok.  Though given how much code this took, I'm
not sure I'd have been ok with doing this if it hadn't already been
written.

Can you remove the default param to nsCSSValue::AppendToString,
nsCSSValueList::AppendToString, etc.?  How many more changes would
that require?  Removing it would drastically lower the risk of failing
to pass it through.

>+    switch (aValueSerialization == eAuthorSpecified ?
>+              mFlags & eColorSyntax_TypeMask :
>+              0) {
>+      case eColorSyntax_Default:

I don't understand how the "0" as the argument to this switch works.
Why isn't this eColorSyntax_Default instead of 0?  And also maybe a case
for both Unknown (0) and Default?

See also comments on previous patch re Component*_Integer.

And fix the 80th column violations you introduced.

r=dbaron with that.
Attachment #752585 - Flags: review?(dbaron) → review+
Comment on attachment 752586 [details] [diff] [review]
Part 4: Add a chrome-only CSSDeclaration.getAuthoredPropertyValue() method to expose a property value closer to its authored form.

The duplication here (multiple pairs of near-identical methods on multiple classes) is a bit ugly, and if it weren't for FontFaceStyleDecl, I'd make you fix it.

So r=dbaron with comment 12 addressed.
Attachment #752586 - Flags: review?(dbaron) → review+
Comment on attachment 752596 [details] [diff] [review]
Part 5: Test. (v1.1)

I can see this being mochitest-chrome, but it shouldn't be in mochitest-browser-chrome.  And it should live in layout/style/test/ (which will need a MOCHITEST_CHROME_FILES line).
Attachment #752596 - Flags: review?(dbaron) → review-
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #15)
> I'm not happy with the eColorSyntax_Component*_Integer stuff.  Can you
> just drop it for hsl(), or are there hsl values that can't be expressed
> as integers?  And then can you reuse nsStyleUtil::ColorComponentToFloat
> for rgb() with percentages?  That seems preferable to spitting out
> six-decimal-place floats whenever 50.0% is used.  Or is there a reason
> I'm missing to have this?  (You can also remove the new third param
> to ParseColorComponent... which you also don't use correctly!  And also
> the change to ScanNumber and the comment in nsCSSScanner.h.)

Out of the 16M RGB colours that can be stored in an nscolor, only around 2M of them can be represented with an hsl() colour using integer components.

I had thought that taking the nscolor, converting to back to HSL, and then rounding the components to an integer would result in an HSL colour that would convert to the same RGB colour, but that's not right at all.  (I did realise that hsl -> rgb -> hsl would not get you back the same value in most cases.)

So I'm not really sure what to do with hsl() colours now.  I was trying to avoid having to store them in something other than an nscolor, but I guess we can't do that if we want to preserve whether the hsl components were integers.  Any suggestions?  Unless we're happy to just write out float hsl components all the time.

> r=dbaron with that
> 
> I think this patch would have been nice split into more than one patch;
> the type addition and the nsCSSParser refactorings could have been one
> patch, and then the SetColorValue change and the storage another (or
> even the actual storage of the value in a third).

Sorry about that.

> And would you mind filing (and maybe fixing) a followup bug to remove
> support for -moz-rgba() and -mos-hsla()?

Bug 893319.
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #16)
> Can you remove the default param to nsCSSValue::AppendToString,
> nsCSSValueList::AppendToString, etc.?  How many more changes would
> that require?  Removing it would drastically lower the risk of failing
> to pass it through.

There are 43 call sites that need to have the eNormalized argument added.

> >+    switch (aValueSerialization == eAuthorSpecified ?
> >+              mFlags & eColorSyntax_TypeMask :
> >+              0) {
> >+      case eColorSyntax_Default:
> 
> I don't understand how the "0" as the argument to this switch works.
> Why isn't this eColorSyntax_Default instead of 0?  And also maybe a case
> for both Unknown (0) and Default?

Yeah, that should be eColorSyntax_Default.
needinfo? for comment 19.
Flags: needinfo?(dbaron)
Can you map every rgb() color with hsl() colors with one decimal point in them?  Or two?

Either way, it seems like we should be able to do something like what we do for rgb() with percentages, and only use decimal points when we need them, and when we do need them, only go to one or two places.


(Also, with 43 callsites, I'd be fine going either way.)
Flags: needinfo?(dbaron)
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #22)
> Can you map every rgb() color with hsl() colors with one decimal point in
> them?  Or two?
> 
> Either way, it seems like we should be able to do something like what we do
> for rgb() with percentages, and only use decimal points when we need them,
> and when we do need them, only go to one or two places.

I will look into that.
Firebug's need for this is described in bug 911906, which also contains some test cases.

Sebastian
Hey Cameron, what the status of this bug?
Flags: needinfo?(cam)
Thanks for the ping Paul.  I'll come back to this bug soon -- hopefully later this week.  Last I was working on it, I was stuck finding a way to get an HSL colour value that would roundtrip to the same value when stored as an sRGB colour (which is how HSL colours are stored internally at the moment).  I'm thinking just to avoid the problem and store HSL colours as specified.  That'll take slightly more memory, but HSL colours are uncommon enough that it should be OK.  It also would mean we can report the actual authored values, and not something close but different.
Flags: needinfo?(cam)
That sounds perfect for me.

Sebastian
David, I'm considering doing something significantly simpler than the previous patches:

* Add separate nsCSSValue units to represent the different specified
  syntax values:

    1. rgb(int,int,int)
    2. rgba(int,int,int,float) (but with alpha still stored as 0..255)
    3. #rrggbb
    4. #rgb
    5. rgb(float%,float%,float%)
    6. rgba(float%,float%,float%)
    7. hsl(float,float%,float%)
    8. hsla(float,float%,float%,float%)

* Store 1-4 as an nscolor in the nsCSSValue union, and 5-8 as an
  nsCSSValueFloatColor object that stores four floats.

* Make these color values always serialize using the syntax indicated by
  the nsCSSValue unit, rather than adding a new
  CSSDeclaration.getAuthoredPropertyValue() method.

How does that sound?
Flags: needinfo?(dbaron)
Attached patch alternative WIP (obsolete) — Splinter Review
So something like this.
Attachment #804851 - Flags: feedback?(dbaron)
(In reply to Cameron McCormack (:heycam) from comment #29)
> * Make these color values always serialize using the syntax indicated by
>   the nsCSSValue unit, rather than adding a new
>   CSSDeclaration.getAuthoredPropertyValue() method.

I guess sites might be relying on the way specified colour values are serialised, though.
Attachment #752583 - Attachment is obsolete: true
Attachment #752584 - Attachment is obsolete: true
Attachment #752585 - Attachment is obsolete: true
Attachment #752586 - Attachment is obsolete: true
Attachment #752596 - Attachment is obsolete: true
Attachment #804851 - Attachment is obsolete: true
Attachment #804851 - Flags: feedback?(dbaron)
Attachment #805077 - Flags: review?(dbaron)
Not carrying over any r+s, due to the changes I made per comment 29.  The previous review comments that still apply should all be addressed, though.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=2bbf986c0af3
I have a question about CSSDeclaration.getAuthoredPropertyValue().  Will this allow us to retrieve invalid property values as authored?  What about invalid property names?

div {
   background: orang;
   word-wrap: 1px;
   bckground: red;
}

Given this CSS, it is very useful for DevTools to show the user these properties with feedback that they were are not being applied.
No it won't.  All it will do, with these patches, is store colour values in their original syntactic form (so one of the eight forms listed in comment 29).  It still doesn't return the exact text that was used in the style sheet.

Do you want to file a separate bug for that?
(In reply to Cameron McCormack (:heycam) from comment #41)
> No it won't.  All it will do, with these patches, is store colour values in
> their original syntactic form (so one of the eight forms listed in comment
> 29).  It still doesn't return the exact text that was used in the style
> sheet.
> 
> Do you want to file a separate bug for that?

I'd be happy to if there isn't already one.  Paul/Mike, is there already a bug you are tracking for the 'exact text as authored' feature?
To be clear, I was referring to the ability to (a) store the invalid value (like 'background: orang' and 'word-wrap: 1px') and (b) store an unrecognised property and its value (like 'bckground: red').  I don't think we should be storing the exact text as authored for properties that are valid.
(In reply to Cameron McCormack (:heycam) from comment #43)
> To be clear, I was referring to the ability to (a) store the invalid value
> (like 'background: orang' and 'word-wrap: 1px') and (b) store an
> unrecognised property and its value (like 'bckground: red').  I don't think
> we should be storing the exact text as authored for properties that are
> valid.

Are you saying that (a) and (b) are part of these patches, or that we should open another bug to address them?
My "to be clear" should have been clearer. ;)

These patches only allow you to get at the original syntactic form of colour values used in properties.  (a) and (b) above aren't supported by these patches, but we could probably do something to support that in a separate bug.  But we'll need to drill down into the details of what's wanted.  Storing the exact original text for a valid property declaration, e.g. having "color: gReEn" return " gReEn" rather than "green", is not really feasible.
To be clear what is expected from the Firebug Working Group side, we expect to also be able to get incorrect property names and property values, so we can display the author that they are wrong.
Also we want to get equally named properties even when they are both valid, so the user can see the originally authored properties.
And we want to show shorthand property values in the order they were authored.
For the former two things mentioned above there are already issues for the built-in dev tools integration.[1][2] Though AFAICS there aren't enhancement requests for the APIs behind them yet. So let me know if I should create them and whether they should be split into separate requests or merged into one. Personally I think one API for that would be reasonable.

Anyway, all that shouldn't be part of this issue. This is really just about allowing to show color values in their originally authored form. Maybe the summary should be changed to reflect that.

Sebastian

[1] Bug 770357
[2] Bug 913661
(In reply to Sebastian Zartner from comment #46)
> To be clear what is expected from the Firebug Working Group side, we expect
> to also be able to get incorrect property names and property values, so we
> can display the author that they are wrong.
> Also we want to get equally named properties even when they are both valid,
> so the user can see the originally authored properties.
> And we want to show shorthand property values in the order they were
> authored.

I see.  Let's discuss those in separate bugs.

> For the former two things mentioned above there are already issues for the
> built-in dev tools integration.[1][2] Though AFAICS there aren't enhancement
> requests for the APIs behind them yet. So let me know if I should create
> them and whether they should be split into separate requests or merged into
> one. Personally I think one API for that would be reasonable.

I expect that we'd write [ChromeOnly] APIs for them that both devtools and FireBug could use.
Summary: Make "Authored" styles available through style system → Make "Authored" color values available through style system
(In reply to Sebastian Zartner from comment #46)
> So let me know if I should create
> them and whether they should be split into separate requests or merged into
> one.

And yes please file a bug for the API that bug 770357 and/or bug 913661 could then depend on.
> I expect that we'd write [ChromeOnly] APIs for them that both devtools and FireBug could use.
Yes, they should be chrome-only.

> And yes please file a bug for the API that bug 770357 and/or bug 913661 could then depend on.
Created bug 916977.

Sebastian
Comment on attachment 805069 [details] [diff] [review]
Part 1: Rename eCSSUnit_Color to eCSSUnit_RGBAColor.

>     case eCSSUnit_Enumerated:   break;
>     case eCSSUnit_EnumColor:    break;
>-    case eCSSUnit_Color:        break;
>+    case eCSSUnit_RGBAColor:        break;

Please fix the indentation here.

r=dbaron
Attachment #805069 - Flags: review?(dbaron) → review+
Comment on attachment 805070 [details] [diff] [review]
Part 2: Add new nsCSSValue units to store colors in their original syntactic form.

>+  bool IsIntegerColorUnit() const { return IsIntegerColorUnit(mUnit); }
>+  bool IsFloatColorUnit() const { return IsFloatColorUnit(mUnit); }
>+  bool IsNumericColorUnit() const { return IsNumericColorUnit(mUnit); }
>+  static bool IsIntegerColorUnit(nsCSSUnit aUnit)
>+  { return eCSSUnit_RGBColor <= aUnit && aUnit <= eCSSUnit_ShortHexColor; }
>+  static bool IsFloatColorUnit(nsCSSUnit aUnit)
>+  { return eCSSUnit_FloatRGBColor <= aUnit &&
>+           aUnit <= eCSSUnit_FloatHSLAColor; }
>+  static bool IsNumericColorUnit(nsCSSUnit aUnit)
>+  { return IsIntegerColorUnit(aUnit) || IsFloatColorUnit(aUnit); }

Put a brief comment explaining what these all mean, and pointing
out that keyword and system colors are eCSSUnit_Ident.

>+class nsCSSValueFloatColor {

Could you add this higher in the file, above Array, so that the class
ordering matches up with the ordering within nsCSSValue?

>+  float mComponent1;  // 0..1 for RGB, 0..360 for HSL
>+  float mComponent2;  // 0..1
>+  float mComponent3;  // 0..1
>+  float mAlpha;       // 0..1

The RGB components actually aren't supposed to be clamped to 0..1.
So maybe put a FIXME comment here?

(We should defer that to another bug.)


> void nsCSSValue::DoReset()
> {
>   if (UnitHasStringValue()) {
>     mValue.mString->Release();
>   } else if (UnitHasArrayValue()) {
>     mValue.mArray->Release();
>+  } else if (IsFloatColorUnit()) {
>+    mValue.mFloatColor->Release();

Put this before array, like the rest?

r=dbaron with that
Attachment #805070 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-5) from comment #51)
> Comment on attachment 805070 [details] [diff] [review]
> Part 2: Add new nsCSSValue units to store colors in their original syntactic
> form.
> Put a brief comment explaining what these all mean, and pointing
> out that keyword and system colors are eCSSUnit_Ident.

er, and eCSSUnit_EnumColor.
Flags: needinfo?(dbaron)
Comment on attachment 805071 [details] [diff] [review]
Part 3: Store parsed CSS color values in their original syntactic form. (v1.1)

Please put the CanvasRenderingContext2D.cpp and nsRuleNode.cpp changes,
and the change to CSSParserImpl::ParseShadowItem, in patch 2 instead of
patch 3.

Oh, and one other thought on the previous patch:  Color level 4 will
probably drop various integer restrictions, so you should probably
rename things like FloatRGBColor to PercentRGBColor, since we may need
to add a third (FloatRGBColor) soon.

>       // #xxyyzz
>       if (NS_HexToRGB(tk->mIdent, &rgba)) {
>-        aValue.SetColorValue(rgba);
>+        nsCSSUnit unit = tk->mIdent.Length() == 3 ?
>+                           eCSSUnit_ShortHexColor :
>+                           eCSSUnit_HexColor;
>+        aValue.SetIntegerColorValue(rgba, unit);
>         return true;
>       }

Please assert that tk->mIdent.Length() is either 3 or 6.  (This
double-checks that it's not, say, 4 or 7.)

>+          MOZ_ASSERT(mToken.mType == eCSSToken_Percentage);

This assertion will fail on syntax errors; remove it.

>+  bool ParseColorComponent(uint8_t& aComponent, char aStop);
>+  bool ParseColorComponent(float& aComponent, char aStop);

Please call these ParseNumberColorComponent and
ParsePercentColorComponent (in line with my comment about level 4 of
color).

r=dbaron
Attachment #805071 - Flags: review?(dbaron) → review+
Comment on attachment 805072 [details] [diff] [review]
Part 4: Add ability for nsCSSValues to serialize themselves in their original syntactic form. (v1.1)

The nsCSSParser.cpp change belongs in the previous patch (and was
one of my review comments on it).

The first nsStyleAnimation.cpp change belongs in patch 2 rather than 
this patch.

r=dbaron with that
Attachment #805072 - Flags: review?(dbaron) → review+
Comment on attachment 805074 [details] [diff] [review]
Part 5: Add support for serializing colors in nsCSSValue in their original syntactic form.

r=dbaron
Attachment #805074 - Flags: review?(dbaron) → review+
Comment on attachment 805075 [details] [diff] [review]
Part 6: Add a chrome-only CSSDeclaration.getAuthoredPropertyValue() method to expose a property value in its original syntactic form. (v1.1)

>@@ -155,16 +155,35 @@ nsDOMCSSDeclaration::GetPropertyValue(co
>     aReturn.Truncate();
>     return NS_OK;
>   }
> 
>   return GetPropertyValue(propID, aReturn);
> }
> 
> NS_IMETHODIMP
>+nsDOMCSSDeclaration::GetAuthoredPropertyValue(const nsAString& aPropertyName,
>+                                              nsAString& aReturn)
>+{
>+  aReturn.Truncate();
>+
>+  const nsCSSProperty propID = nsCSSProps::LookupProperty(aPropertyName,
>+                                                          nsCSSProps::eEnabled);
>+  if (propID == eCSSProperty_UNKNOWN) {
>+    return NS_OK;
>+  }

You'll need to adjust this bit to merge with the variables landing, probably by just duplicating the insertion of variables-related code in nsDOMCSSDeclaration::GetPropertyValue.

r=dbaron with that
Attachment #805075 - Flags: review?(dbaron) → review+
Comment on attachment 805077 [details] [diff] [review]
Part 7: Test. (v1.2)

The Makefile.in change should now be a chrome.ini change.

r=dbaron, and sorry for the delay
Attachment #805077 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #51)
> Comment on attachment 805070 [details] [diff] [review]
> Part 2: Add new nsCSSValue units to store colors in their original syntactic
> form.
> 
> >+class nsCSSValueFloatColor {
> 
> Could you add this higher in the file, above Array, so that the class
> ordering matches up with the ordering within nsCSSValue?

The ordering of the members in nsCSSValue's union?  It should go just below nsCSSValuePairList in that case shouldn't it?

> > void nsCSSValue::DoReset()
> > {
> >   if (UnitHasStringValue()) {
> >     mValue.mString->Release();
> >   } else if (UnitHasArrayValue()) {
> >     mValue.mArray->Release();
> >+  } else if (IsFloatColorUnit()) {
> >+    mValue.mFloatColor->Release();
> 
> Put this before array, like the rest?

Shouldn't it be at the end of the if-then chain, just after eCSSUnit_PairList?
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #53)
> Comment on attachment 805071 [details] [diff] [review]
> Part 3: Store parsed CSS color values in their original syntactic form.
> (v1.1)
> 
> Oh, and one other thought on the previous patch:  Color level 4 will
> probably drop various integer restrictions, so you should probably
> rename things like FloatRGBColor to PercentRGBColor, since we may need
> to add a third (FloatRGBColor) soon.

I'll rename the units to:

  eCSSUnit_PercentageRGBColor
  eCSSUnit_PercentageRGBAColor
  eCSSUnit_HSLColor
  eCSSUnit_HSLAColor

(dropping the prefix from the HSL* ones since I don't think it's needed in retrospect) but I'll keep the class named nsCSSValueFloatColor as that's talking about the storage rather than the syntax.
(In reply to Cameron McCormack (:heycam) from comment #58)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #51)
> > Comment on attachment 805070 [details] [diff] [review]
> > Part 2: Add new nsCSSValue units to store colors in their original syntactic
> > form.
> > 
> > >+class nsCSSValueFloatColor {
> > 
> > Could you add this higher in the file, above Array, so that the class
> > ordering matches up with the ordering within nsCSSValue?
> 
> The ordering of the members in nsCSSValue's union?  It should go just below
> nsCSSValuePairList in that case shouldn't it?

Hmmm.  I meant the ordering in nsCSSValue.cpp, I think.  (In other words, keeping the .h and the .cpp in the same order.)

> > > void nsCSSValue::DoReset()
> > > {
> > >   if (UnitHasStringValue()) {
> > >     mValue.mString->Release();
> > >   } else if (UnitHasArrayValue()) {
> > >     mValue.mArray->Release();
> > >+  } else if (IsFloatColorUnit()) {
> > >+    mValue.mFloatColor->Release();
> > 
> > Put this before array, like the rest?
> 
> Shouldn't it be at the end of the if-then chain, just after
> eCSSUnit_PairList?

If you have a good reason for a particular order, then feel free to use that throughout.  It just seems odd for one method to use a different order (of cases) from the other methods.
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #60)
> Hmmm.  I meant the ordering in nsCSSValue.cpp, I think.  (In other words,
> keeping the .h and the .cpp in the same order.)

I'm still confused, as there aren't any nsCSSValue::Array functions in nsCSSValue.cpp (apart from nsCSSValue::Array::SizeOfIncludingThis, which is placed next to some other SizeOfIncludingThis functions from other classes).  The ordering of the nsCSSValue::mValue union members is consistent neither with the set of forward declarations just above nsCSSValue, nor with the order of the class definitions later in the .h file.  The .h and .cpp files do match up, though.  Anyway, I'll move up nsCSSValueFloatColor just one spot to above nsCSSCornerSizes, to keep it next to other classes used by the union.

> > > > void nsCSSValue::DoReset()
> > > > {
> > > >   if (UnitHasStringValue()) {
> > > >     mValue.mString->Release();
> > > >   } else if (UnitHasArrayValue()) {
> > > >     mValue.mArray->Release();
> > > >+  } else if (IsFloatColorUnit()) {
> > > >+    mValue.mFloatColor->Release();
> > > 
> > > Put this before array, like the rest?
> > 
> > Shouldn't it be at the end of the if-then chain, just after
> > eCSSUnit_PairList?
> 
> If you have a good reason for a particular order, then feel free to use that
> throughout.  It just seems odd for one method to use a different order (of
> cases) from the other methods.

Oh indeed, I didn't notice it didn't match up with operator== etc.
(In reply to Cameron McCormack (:heycam) from comment #61)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #60)
> > Hmmm.  I meant the ordering in nsCSSValue.cpp, I think.  (In other words,
> > keeping the .h and the .cpp in the same order.)
> 
> I'm still confused, as there aren't any nsCSSValue::Array functions in
> nsCSSValue.cpp (apart from nsCSSValue::Array::SizeOfIncludingThis, which is
> placed next to some other SizeOfIncludingThis functions from other classes).
> The ordering of the nsCSSValue::mValue union members is consistent neither
> with the set of forward declarations just above nsCSSValue, nor with the
> order of the class definitions later in the .h file.  The .h and .cpp files
> do match up, though.  Anyway, I'll move up nsCSSValueFloatColor just one
> spot to above nsCSSCornerSizes, to keep it next to other classes used by the
> union.

Yeah, I'm not sure what I meant either.  But if you move it up, please keep the .h and .cpp in parallel order.
Flags: in-testsuite?
Does this need more automation coverage?
Flags: needinfo?(cam)
I think the test I added is sufficient.  Thanks.
Flags: needinfo?(cam)
Flags: in-testsuite?
Flags: in-testsuite+
Depends on: 1302513
The cssparser crate doesn't need to be changed AFAIK given we don't actually use its ToCss trait, and we already store authored values in our Color type, AFAICT.

Many types that use colours can be covered by the existing derive(ToCss) machinery in components/style_derive/to_css.rs, but not gradients, which were unfortunately SimonSapin's example.

I'm not sure SimonSapin's idea with the (Write, bool) wrapper is a good idea, given the derive code uses some stuff that pass wrappers of Write itself (grep for SequenceWriter), and I think wrappers of wrappers is just going to make everything more complex, but I could be wrong.

What about a method that takes an additional enum SerialisationMode { Authored, NotAuthored }, and changing the to_css() one to just call the new one with SerialisationMode::NotAuthored?
You need to log in before you can comment on or make changes to this bug.