308409 - convert ParseTextShadow to nsCSSValue::Array and nsCSSValueList

Status: RESOLVED FIXED

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

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

this allows eliminating nsCSSShadow and a lot of code related to it.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: testcase for serialization

this shows that serialization still works (see the style attribute in DOMI. the
<style> is really only there for a debugging printf in my code)

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

This rewrites ParseTextShadow, it is now pretty similar to ParseCursor.

Comment 3

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

Comment on attachment 195997 [details] [diff] [review]
patch

>- * nsCSSCompressedDataBlock holds property-value pairs corresponding to
>- * CSS declaration blocks.  The value is stored in one of the six CSS
>- * data types.  These six types are nsCSSValue, nsCSSRect,
>- * nsCSSValueList, nsCSSCounterData, nsCSSQuotes, and nsCSSShadow, and
>- * each correspond to a value of the nsCSSType enumeration.
>+ * nsCSSCompressedDataBlock holds property-value pairs corresponding
>+ * to CSS declaration blocks.  The value is stored in one of the five
>+ * CSS data types.  These five types are nsCSSValue, nsCSSRect,
>+ * nsCSSValueList, nsCSSCounterData, and nsCSSQuotes, and each
>+ * correspond to a value of the nsCSSType enumeration.
>+ * XXX what about ValuePair?

You should mention ValuePair, and keep the number at six.  And do you mind
wrapping at 72 characters so you don't rewrap the whole thing?

Comment 4

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

Comment on attachment 195997 [details] [diff] [review]
patch

>@@ -269,28 +228,19 @@ void nsCSSText::List(FILE* out, PRInt32 

>+
>+  nsCSSValueList*  shadow = mTextShadow;
>+  while (nsnull != shadow) {
>+    shadow->mValue.AppendToString(buffer, eCSSProperty_cursor);
>+    shadow = shadow->mNext;
>   }
>+

You should:
 * use a for loop, i.e.,
     for (nsCSSValueList *shadow = mTextShadow; shadow; shadow = shadow->mNext)
 * not use two spaces
 * use eCSSProperty_text_shadow

Comment 5

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

Don't you need to add to the code in nsCSSDeclaration::AppendValueToString that
does:

            if (aProperty == eCSSProperty_cursor
#ifdef MOZ_SVG
                || aProperty == eCSSProperty_stroke_dasharray
#endif
               )
              aResult.Append(PRUnichar(','));
            aResult.Append(PRUnichar(' '));

so that you get commas?

Your serialization testcase doesn't contain any value lists of length greater
than one.  It probably should.

Comment 6

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

+    if (unit != eCSSUnit_None && unit != eCSSUnit_Inherit) {

This should also check for eCSSUnit_Initial.  (VARIANT_INHERIT implies 'inherit'
and 'initial'; perhaps it should be renamed.)

[Also cc:ing bz; better late than never.]

Comment 7

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

Comment on attachment 195997 [details] [diff] [review]
patch

>+        // Must be a color

This should be an NS_ASSERTION, not a comment.

>+      // Optional radius
>+      ParseVariant(aErrorCode, val->Item(IndexRadius), VARIANT_LENGTH, nsnull);
>+
>+      // If we have no color yet, we expect it now (but it's optional)
>+      if (!haveColor) {
>+        ParseVariant(aErrorCode, val->Item(IndexColor), VARIANT_COLOR, nsnull);
>+      }

This looks like you're trying to be the only caller that doesn't check the
result of ParseVariant.  I'm not sure if it supports that for VARIANT_LENGTH,
and I doubt it supports it for VARIANT_COLOR.  (For example, do you incorrectly
accept "text-shadow: 1px 1px 1px rgb(255,255" ?)

Comment 8

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

You also need to call SetParsingCompoundProperty(PR_TRUE) and PR_FALSE around
most of the body of the function (maybe even have a little class that does it,
and fix the one other user), since in quirks mode we accept unitless numbers as
both colors and lengths, and we really don't want either of this quirks here.

Comment 9

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

In fact, you should probably add an assertion to the beginning ParseVariant that 
NS_ASSERTION(
  IsParsingCompoundProperty() || 
  ((~aVariantMask) & (VARIANT_LENGTH|VARIANT_COLOR)),
  "cannot distinguish lengths and colors in quirks mode");

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

> This looks like you're trying to be the only caller that doesn't check the
> result of ParseVariant.  I'm not sure if it supports that for VARIANT_LENGTH,
> and I doubt it supports it for VARIANT_COLOR.  (For example, do you incorrectly
> accept "text-shadow: 1px 1px 1px rgb(255,255" ?)

Hm... So, I was assuming that a false return leaves the token stream unchanged,
so that ExpectEndProperty later fails. What else would you suggest for this case?

Oh... the parser can only push back one token?

I realized today that a testcase for length > 1 was missing, will fix...

> * use a for loop, i.e.,
>     for (nsCSSValueList *shadow = mTextShadow; shadow; shadow = shadow->mNext)

OK, I'll change the cursor one too.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #10)
> OK, I'll change the cursor one too.

Actually, this pattern occurs a lot of times in this file. Should I change all
of them?

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

(even with the double space)

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: valid testcase

turns out the previous testcase was invalid, it was missing the units...

Comment 14

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

The for loop thing was just a minor style nit -- not worth fixing everywhere.

Also, I probably should have been clearer that the ParseVariant return value
issue should probably be fixed by peeking ahead one token at the caller (which
allows you to tell which calls to make), not by changing ParseVariant.

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

OK, how about this?

Comment 16

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

This has a bunch of cases where it doesn't accept EOF but it should (would break
being the last property in a style attribute without a terminiting semicolon). 
You also probably don't need TokenCanBeColor, but you should use
ExpectEndProperty a bit more (but you'd need to combine it with ExpectSymbol(',')).

Comment 17

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

Comment on attachment 196220 [details] [diff] [review]
patch v2

Didn't review this thoroughly, but it still needs more work (see previous
comment).

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

note to self: the patch uses the wrong order in serialization, color should be last.

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

this one should be right.

Comment 20

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

Comment on attachment 196261 [details] [diff] [review]
patch

>+    if (!atEOP && !expectEOP && ExpectSymbol(aErrorCode, ',', PR_TRUE)) {
>+      // Parse next value
>+      continue;
>+    }

This chunk could move in three blocks (i.e., indent 6 characters and move
before three closing braces), and then you can drop the first two parts of the
if-expression and just call ExpectSymbol.

Also, in the case where ExpectEndProperty fails and you call ParseVariant, you
should probably CLEAR_ERROR().

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: final patch

(transferring r+sr=dbaron)

also removes the now-unused expectEOP variable

Comment 22

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

No, you still need the second use of expectEOP, no?

Comment 23

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

Er, never mind, I was thinking of atEOP.

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

fixed on trunk.