Summary: | Early return in CSSPrimitiveValue::getDoubleValueInternal() omits additional invalid enums | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||
Component: | CSS | Assignee: | Alexander Pavlov (apavlov) <apavlov> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | apavlov, macpherson, simon.fraser | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
David Kilzer (:ddkilzer)
2011-08-29 10:37:13 PDT
(In reply to comment #0) > * SUMMARY > The early return code in CSSPrimitiveValue::getDoubleValueInternal() in CSSPrimitiveValue.cpp ignores the newly added CSS_COUNTER_NAME enum because it checks for currently-known illegal values instead of currently-known legal values. As well as CSS_FROM_FLOW and CSS_SHAPE now. Created attachment 105643 [details]
[PATCH] Suggested fix
Thanks for the tip David.
Comment on attachment 105643 [details] [PATCH] Suggested fix View in context: https://bugs.webkit.org/attachment.cgi?id=105643&action=review r=me even though I think we can make some small improvements > Source/WebCore/css/CSSPrimitiveValue.cpp:53 > + switch (unitType) { Using a switch is a great technique to make sure we make a conscious decision for each enum value. But I’d suggest just having one block for false and one for true, sorted alphabetically. The order now may have some logic to it, but it’s not obvious and I think it would be better to not have that. > Source/WebCore/css/CSSPrimitiveValue.cpp:86 > + case CSSPrimitiveValue:: CSS_UNICODE_RANGE: > + > + case CSSPrimitiveValue:: CSS_PARSER_OPERATOR: This blank line looks like a mistake. (In reply to comment #3) > (From update of attachment 105643 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=105643&action=review > > r=me even though I think we can make some small improvements > > > Source/WebCore/css/CSSPrimitiveValue.cpp:53 > > + switch (unitType) { > > Using a switch is a great technique to make sure we make a conscious decision for each enum value. But I’d suggest just having one block for false and one for true, sorted alphabetically. The order now may have some logic to it, but it’s not obvious and I think it would be better to not have that. > > > Source/WebCore/css/CSSPrimitiveValue.cpp:86 > > + case CSSPrimitiveValue:: CSS_UNICODE_RANGE: > > + > > + case CSSPrimitiveValue:: CSS_PARSER_OPERATOR: > > This blank line looks like a mistake. Thanks Darin, I will land the patch tomorrow with these nits fixed. Committed r94169: <http://trac.webkit.org/changeset/94169> FWIW, I think a test could have been written for this using the fact that certain CSS properties would return a value instead of throwing an exception. |