RESOLVED FIXED 61601
Implement CSSPropertyOutlineStyle handler in CSSStyleApplyProperty 
https://bugs.webkit.org/show_bug.cgi?id=61601
Summary Implement CSSPropertyOutlineStyle handler in CSSStyleApplyProperty
Luke Macpherson
Reported 2011-05-26 21:37:05 PDT
Implement CSSPropertyOutlineStyle handler in CSSStyleApplyProperty
Attachments
Patch (8.91 KB, patch)
2011-05-26 21:50 PDT, Luke Macpherson 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Luke Macpherson
Comment 1 2011-05-26 21:50:15 PDT
Created attachment 95108 [details] Patch
Eric Seidel (no email)
Comment 2 2011-05-26 21:56:37 PDT
Comment on attachment 95108 [details] Patch I worry I've pushed a little too hard on the bool -> enum thing. Bools can make sense when answering isFoo() quesitons or setIsFoo(). But they don't make sense when calling bar(foo, true). In the latter case, a enum is *strongly* preferred over a bool as it adds clarity. I don't see this bool being used in any bar(foo, bool) cases, and I wonder if we want to go through the trouble of making it an enum? I guess I'm mostly confused and don't fully understand what this patch is doing.
Luke Macpherson
Comment 3 2011-05-26 22:08:24 PDT
Yeah, it's an odd patch. Let me explain. The motivation was not to eliminate the bool. It's an odd situation because of the way outline style is defined as an outline of border style. This leads to a situation where RenderStyle::setOutlineStyle is the only setter on RenderStyle to take two parameters, which means that it doesn't fit the ApplyPropertyDefault pattern in CSSStyleApplyProperty very well. Splitting it up into two separate setters makes it fit that pattern a lot more cleanly, and avoids defining a new ApplyProperty class just for outline-style. So that's the rationale behing the change. I'm also not completely convinced it's the right way to go - it seems like a special case either way. Another alternative might be to stop defining OutLineValue in terms of BorderValue and EBorderStyle. Let me know what you think, I'd love a second opinion, especially if there is an obviously better approach that I haven't considered.
Luke Macpherson
Comment 4 2011-06-09 17:15:58 PDT
Ping.
Eric Seidel (no email)
Comment 5 2011-06-09 22:40:39 PDT
Comment on attachment 95108 [details] Patch OK.
WebKit Review Bot
Comment 6 2011-06-09 23:19:34 PDT
Comment on attachment 95108 [details] Patch Clearing flags on attachment: 95108 Committed r88525: <http://trac.webkit.org/changeset/88525>
WebKit Review Bot
Comment 7 2011-06-09 23:19:38 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.