Summary: | Remove dead code in SVGCSSParser | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Rob Buis <rwlbuis> | ||||||
Component: | SVG | Assignee: | Rob Buis <rwlbuis> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | macpherson, menard, webkit.review.bot, zimmermann | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Rob Buis
2012-04-06 15:40:51 PDT
Created attachment 136080 [details]
Patch
Created attachment 136120 [details]
Patch
Comment on attachment 136080 [details]
Patch
CSSPropertyColor is already handled by CSSParser I guess, so we never reach this code path?
If so, please include this in the ChangeLog ("why" change logs are better than "what" change logs). Still r=me.
Comment on attachment 136120 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136120&action=review Ah, just saw you posted a new patch, r=me for that as well, as it resolved my issue :-) > LayoutTests/svg/custom/script-tests/currentColor-on-color.js:13 > +rectElement.setAttribute("color", "currentColor"); This is not needed, is it? > LayoutTests/svg/custom/script-tests/currentColor-on-color.js:19 > +shouldBeEqualToString("document.defaultView.getComputedStyle(rectElement).color", "rgb(0, 128, 0)"); s/document.defaultView// (it's not need). Hi Niko, Thanks for the review! (In reply to comment #4) > (From update of attachment 136120 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=136120&action=review > > Ah, just saw you posted a new patch, r=me for that as well, as it resolved my issue :-) Nice :) > > LayoutTests/svg/custom/script-tests/currentColor-on-color.js:13 > > +rectElement.setAttribute("color", "currentColor"); > > This is not needed, is it? I think it is. Read the LayoutTests Changelog :) Basically we inherit the color=currentColor CSS3 behaviour from CSSParser.cpp (matches what FF does), and I agreed with Dirk to make a regression test for that since we are giving up dealing with color property in SVGCSSParser.cpp, based on his bug and closing of bug 15012. > > LayoutTests/svg/custom/script-tests/currentColor-on-color.js:19 > > +shouldBeEqualToString("document.defaultView.getComputedStyle(rectElement).color", "rgb(0, 128, 0)"); > > s/document.defaultView// (it's not need). Great! Will fix before landing. Cheers, Rob. |