[FIX]CSS parser throws on unknown units

RESOLVED FIXED

Status

()

RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

16 years ago
The CSS parser will throw on unknown units in dimension tokens.  There's really
no reason to do that....

This began to be a problem in JS since the fix for bug 171830
(Assignee)

Comment 1

16 years ago
Created attachment 117094 [details]
testcase -- there should be no exception
(Assignee)

Comment 2

16 years ago
Created attachment 117097 [details] [diff] [review]
Proposed patch

This one is a little dicey, especially with ParseChoice.  The idea is that if
we can't figure out the unit we just push back the token and let someone else
deal.  This works in practice, since the ExpectEndProperty or looking for the
font-size or whatever that people do after ParseChoice will fail.  I _think_
this is the right way to go, but....

I took the liberty of eliminating a few compiler warnings and the vestiges of
NS_CSS_PARSER_DROP_DECLARATION while I was in there.
(Assignee)

Comment 3

16 years ago
Comment on attachment 117097 [details] [diff] [review]
Proposed patch

You know things are bad when you're tempted to quote Ms. Spears.... Hit me,
baby, one more time?  ;)
Attachment #117097 - Flags: superreview?(dbaron)
Attachment #117097 - Flags: review?(dbaron)
(Assignee)

Updated

16 years ago
Summary: CSS parser throws on unknown units → [FIX]CSS parser throws on unknown units
Comment on attachment 117097 [details] [diff] [review]
Proposed patch


>     else if ((VARIANT_TIME & aVariantMask) != 0) {
>       units = eCSSUnit_Seconds;
>       type = VARIANT_TIME;
>+    } else {
>+      NS_ERROR("Variant mask does not include dimension; why were we called?");
>+      return PR_FALSE;
>     }

Stick with surrounding style here ("}" and "else {" on separate lines) even
though neither of us likes it much, since it looks even funnier mixed.

Other than that, r+sr=dbaron.
Attachment #117097 - Flags: superreview?(dbaron)
Attachment #117097 - Flags: superreview+
Attachment #117097 - Flags: review?(dbaron)
Attachment #117097 - Flags: review+
(Assignee)

Comment 5

16 years ago
Created attachment 117435 [details] [diff] [review]
Patch I actually landed

This has the indent thing fixed and removes NS_CSS_PARSER_DROP_DECLARATION for
good.
Attachment #117097 - Attachment is obsolete: true
(Assignee)

Comment 6

16 years ago
Er, I meant "brace thing".

Fixed.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.