Bug 69108 - Parse drop-shadow() filter syntax
Summary: Parse drop-shadow() filter syntax
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks: 68469
  Show dependency treegraph
 
Reported: 2011-09-29 16:14 PDT by Dean Jackson
Modified: 2011-11-10 11:29 PST (History)
5 users (show)

See Also:


Attachments
Patch (32.04 KB, patch)
2011-11-09 20:37 PST, Dean Jackson
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2011-09-29 16:14:01 PDT
drop-shadow is more complicated than the other effects because it takes shadow syntax (with some restrictions). I'll need to extract the shadow parsing code from CSSParser into something that can be used for this as well.
Comment 1 Radar WebKit Bug Importer 2011-09-29 16:14:45 PDT
<rdar://problem/10211535>
Comment 2 Dean Jackson 2011-11-09 20:37:59 PST
Created attachment 114425 [details]
Patch
Comment 3 Simon Fraser (smfr) 2011-11-09 22:31:45 PST
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.
Comment 4 Dean Jackson 2011-11-10 02:04:24 PST
(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.
Comment 5 Dean Jackson 2011-11-10 11:29:06 PST
http://trac.webkit.org/changeset/99883