Bug 61601 – Implement CSSPropertyOutlineStyle handler in CSSStyleApplyProperty

• Component: New Bugs
• Status: RESOLVED FIXED

[Luke Macpherson]

Implement CSSPropertyOutlineStyle handler in CSSStyleApplyProperty

[Luke Macpherson]

Created attachment 95108 [details]
Patch

[Eric Seidel (no email)]

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]

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]

Ping.

[Eric Seidel (no email)]

Comment on attachment 95108 [details]
Patch

OK.

[WebKit Review Bot]

Comment on attachment 95108 [details]
Patch

Clearing flags on attachment: 95108

Committed r88525: <http://trac.webkit.org/changeset/88525>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.