208729 - [FIXr]consider creating an nsCSSValuePair type

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

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

If we create an nsCSSValuePair type (as bz suggested in bug 125246 comment 41),
we might be able to get rid of some shorthand hacks:

  Looking over this code, could we introduce a nsCSSValuePair type of data and
  then eliminate the -x-* properties for 'border-spacing', 'size', 'play-during',
  'background-position'?  That could simplify some more code, and arguably make
  some more correct (see the XXX comment you added in nsCSSPropList.h).  Probably
  do this as a separate patch after this one lands...

This would be a seventh type in the eCSSType enumeration, I think.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase for background-position stuff that we should keep on passing...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Current state of things

This doesn't convert background-position because I've not yet been able to get
the shorthand code to play nice with this setup... The problem, of course, is
that background-position can be a shorthand of its own _or_ part of the
background shorthand.  Yay.

Comment 3

[Boris Zbarsky [:bzbarsky]]

By "shorthand code" I mean the code that outputs shorthands when
nsCSSDeclaration is converted into a string.

Comment 4

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

So I wouldn't mind using this for something new (bug 72747).  Is it reviewable
(in its 'background-position'-not-yet-converted state)?

I note the XXXldb where you added the #ifdef DEBUG could probably be removed, as
could the "debugging methods only".

Comment 5

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

Er, well, now that I think about it for 10 seconds, I actually don't want to use
this.  So never mind the dependency, but it could still be worth landing.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yeah, the patch should be reviewable as it is, with background-position to be
fixed later.

Let me post a diff against current trunk for review.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Diff against tip

Comment 8

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

Comment on attachment 156555 [details] [diff] [review]
Diff against tip

>+#ifdef DEBUG
>+void nsCSSValuePair::AppendToString(nsAString& aString,
>+                                    nsCSSProperty aPropName) const
>+{
>+  if (mXValue.GetUnit() != eCSSUnit_Null) {
>+    AppendUTF8toUTF16(nsCSSProps::GetStringValue(aPropName), aString);
>+    aString.Append(NS_LITERAL_STRING(": "));
>+    mXValue.AppendToString(aString);
>+    NS_ASSERTION(mYValue.GetUnit() != eCSSUnit_Null,
>+                 nsPrintfCString("Parsed half of a %s?",
>+                                 nsCSSProps::GetStringValue(aPropName).get()).get());
>+    mYValue.AppendToString(aString);

Appending a space might be good, I think, unless something else does it.


A little more context in this diff might have been nice, but r+sr=dbaron.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Appending a space might be good, I think, unless something else does it.

Yeah, I need to do that.  Good catch.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to review comments.

Note that while getting play-during directly will work with this patch, cssText
for it will still be broken.  I'll file a bug on that, as well on the
background-position issue.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Filed bug 258080 on the background-position issue and bug 258079 on a related
play-during issue I noticed.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Fix checked in for 1.8a4