Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Description • 19 years ago

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

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 1 • 19 years ago

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)

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 2 • 19 years ago

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

	David Baron :dbaron:
	Comment 3 • 19 years ago

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?

	David Baron :dbaron:
	Comment 4 • 19 years ago

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

	David Baron :dbaron:
	Comment 5 • 19 years ago

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.

	David Baron :dbaron:
	Comment 6 • 19 years ago

+    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.]

	David Baron :dbaron:
	Comment 7 • 19 years ago

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" ?)

	David Baron :dbaron:
	Comment 8 • 19 years ago

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.

	David Baron :dbaron:
	Comment 9 • 19 years ago

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");

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 10 • 19 years ago

> 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.

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 11 • 19 years ago

(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?

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 12 • 19 years ago

(even with the double space)

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 13 • 19 years ago

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

	David Baron :dbaron:
	Comment 14 • 19 years ago

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.

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 15 • 19 years ago

OK, how about this?

	David Baron :dbaron:
	Comment 16 • 19 years ago

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(',')).

	David Baron :dbaron:
	Comment 17 • 19 years ago

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

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

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 18 • 19 years ago

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

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 19 • 19 years ago

this one should be right.

	David Baron :dbaron:
	Comment 20 • 19 years ago

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().

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 21 • 19 years ago

(transferring r+sr=dbaron)

also removes the now-unused expectEOP variable

	David Baron :dbaron:
	Comment 22 • 19 years ago

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

	David Baron :dbaron:
	Comment 23 • 19 years ago

Er, never mind, I was thinking of atEOP.

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 24 • 19 years ago

fixed on trunk.