114508 2013-04-12 06:49:28 -0700 [meta] Re-engineer StyleBuilder into something maintainable 2017-07-18 08:27:32 -0700 1 1 1 Unclassified WebKit CSS 528+ (Nightly build) Unspecified Unspecified NEW P2 Normal --- 114532 114715 54707 102844 1 krit webkit-unassigned benjamin bjonesbe hugo.lima kling koivisto mmaxfield psolanki rniwa vivekg oldest_to_newest 874626 0 krit 2013-04-12 06:49:28 -0700 Join StyleBuilder with StyleResolver again. * In a first step we should move all properties back into a giant switch statement. * We should rename StyleResolver to StyleBuilder * Experiment with better ways to build style in branch after this move 874938 1 koivisto 2013-04-12 13:59:54 -0700 As I mentioned in webkit-dev, I don't think joining them is a good idea. The problem is that the current style building implementation sucks, not that separating it is was a bad idea itself. I think this one should be WONTFIX. 874939 2 rniwa 2013-04-12 14:01:30 -0700 Yeah, I think we can instead move the entire switch statement out of the StyleResolver and move it to StyleBuilder. In fact, we can just repurpose this bug to do that. 874946 3 krit 2013-04-12 14:12:07 -0700 (In reply to comment #1) > As I mentioned in webkit-dev, I don't think joining them is a good idea. The problem is that the current style building implementation sucks, not that separating it is was a bad idea itself. I think this one should be WONTFIX. Oh, maybe I misunderstood you. (In reply to comment #2) > Yeah, I think we can instead move the entire switch statement out of the StyleResolver and move it to StyleBuilder. In fact, we can just repurpose this bug to do that. Yes, that would work as well. Antti, what do you think about that? 875156 4 krit 2013-04-13 08:02:35 -0700 (In reply to comment #2) > Yeah, I think we can instead move the entire switch statement out of the StyleResolver and move it to StyleBuilder. In fact, we can just repurpose this bug to do that. I looked a bit in moving applyProperty(CSSPropertyID id, CSSValue* value) from StyleResolver to StyleBuilder. Doing that would require to * move some inline and static functions from StyleResolver to StyleBuilder * make the shared StyleBuilder to a not constant resource (atm: const StaticBuilder& in StyleResolver) * solve some circular dependencies All together, it is a quite complex move with a lot of refactoring. Therefore, we should be sure that this is the way to go. Further more, when the template and the switch are in one place, what is the next step? 875166 5 koivisto 2013-04-13 08:55:10 -0700 If it was an easy refactoring it would have been done long time ago. :) I think this should be done in steps, something like this: 1) rename StyleBuilder -> DeprecatedStyleBuilder 2) create new StyleBuilder (it forwards to DeprecatedStyleBuilder for properties that are still implemented there) 3) move the giant switch and the related functions from StyleResolver to StyleBuilder 4) move individual properties from DeprecatedStyleBuilder to StyleBuilder until nothing remains 5) delete DeprecatedStyleBuilder We'll have DeprecatedStyleBuilder and the new StyleBuilder side by side for a while and can verify it is actually better.