Summary: | Parse drop-shadow() filter syntax | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dean Jackson <dino> | ||||
Component: | CSS | Assignee: | Dean Jackson <dino> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | cmarrin, macpherson, simon.fraser, webkit-bug-importer, webkit.review.bot | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 68469 | ||||||
Attachments: |
|
Description
Dean Jackson
2011-09-29 16:14:01 PDT
Created attachment 114425 [details]
Patch
Comment on attachment 114425 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=114425&action=review r+ but please remove the FIXME > Source/WebCore/css/CSSParser.cpp:6507 > + maximumArgumentCount = 5; // FIXME: is this right? Is it? > Source/WebCore/css/CSSParser.cpp:6750 > + RefPtr<CSSValueList> shadowValueList = parseShadow(args, CSSPropertyWebkitFilter); I think it's a bit odd to re-use parseShadow for this filter. (In reply to comment #3) > (From update of attachment 114425 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=114425&action=review > > r+ but please remove the FIXME > > > Source/WebCore/css/CSSParser.cpp:6507 > > + maximumArgumentCount = 5; // FIXME: is this right? > > Is it? Oops!! I need to test that. > > > Source/WebCore/css/CSSParser.cpp:6750 > > + RefPtr<CSSValueList> shadowValueList = parseShadow(args, CSSPropertyWebkitFilter); > > I think it's a bit odd to re-use parseShadow for this filter. That was the majority of the work in this patch. We share exactly the same syntax as text shadow and it has all the smarts to parse the (quite complex) shadow syntax. It would be a lot of duplicated code otherwise. The fact that I can pass in CSSPropertyWebkitFilter is something I should consider changing. The parseShadow code doesn't expect that and happens to fall into a mode where it disallows spread and inset style - just as we want. |