convert ParseTextShadow to nsCSSValue::Array and nsCSSValueList

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
CSS Parsing and Computation
P3
normal
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: Biesinger, Assigned: Biesinger)

Tracking

Trunk
mozilla1.9alpha1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

this allows eliminating nsCSSShadow and a lot of code related to it.
Created attachment 195996 [details]
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)
Created attachment 195997 [details] [diff] [review]
patch

This rewrites ParseTextShadow, it is now pretty similar to ParseCursor.
Attachment #195997 - Flags: superreview?(dbaron)
Attachment #195997 - Flags: review?(dbaron)
Status: NEW → ASSIGNED
OS: Linux → All
Priority: -- → P3
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha
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 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
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.
+    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 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" ?)
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.
Attachment #195997 - Flags: superreview?(dbaron)
Attachment #195997 - Flags: superreview-
Attachment #195997 - Flags: review?(dbaron)
Attachment #195997 - Flags: review-
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");
> 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.
(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?
(even with the double space)
Created attachment 196151 [details]
valid testcase

turns out the previous testcase was invalid, it was missing the units...
Attachment #195996 - Attachment is obsolete: true
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.
Created attachment 196220 [details] [diff] [review]
patch v2

OK, how about this?
Attachment #195997 - Attachment is obsolete: true
Attachment #196220 - Flags: superreview?(dbaron)
Attachment #196220 - Flags: review?(dbaron)
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 on attachment 196220 [details] [diff] [review]
patch v2

Didn't review this thoroughly, but it still needs more work (see previous
comment).
Attachment #196220 - Flags: superreview?(dbaron)
Attachment #196220 - Flags: superreview-
Attachment #196220 - Flags: review?(dbaron)
Attachment #196220 - Flags: review-
note to self: the patch uses the wrong order in serialization, color should be last.
Created attachment 196261 [details] [diff] [review]
patch

this one should be right.
Attachment #196220 - Attachment is obsolete: true
Attachment #196261 - Flags: superreview?(dbaron)
Attachment #196261 - Flags: review?(dbaron)
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().
Attachment #196261 - Flags: superreview?(dbaron)
Attachment #196261 - Flags: superreview+
Attachment #196261 - Flags: review?(dbaron)
Attachment #196261 - Flags: review+
Created attachment 196715 [details] [diff] [review]
final patch

(transferring r+sr=dbaron)

also removes the now-unused expectEOP variable
Attachment #196261 - Attachment is obsolete: true
Attachment #196715 - Flags: superreview+
Attachment #196715 - Flags: review+
No, you still need the second use of expectEOP, no?
Er, never mind, I was thinking of atEOP.
fixed on trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Blocks: 10713

Updated

12 years ago
Depends on: 345620
You need to log in before you can comment on or make changes to this bug.