Bug 114508

Summary: [meta] Re-engineer StyleBuilder into something maintainable
Product: WebKit Reporter: Dirk Schulze <krit>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: benjamin, bjonesbe, hugo.lima, kling, koivisto, mmaxfield, psolanki, rniwa, vivekg
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 114532, 114715    
Bug Blocks: 54707, 102844    

Description Dirk Schulze 2013-04-12 06:49:28 PDT
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
Comment 1 Antti Koivisto 2013-04-12 13:59:54 PDT
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.
Comment 2 Ryosuke Niwa 2013-04-12 14:01:30 PDT
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.
Comment 3 Dirk Schulze 2013-04-12 14:12:07 PDT
(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?
Comment 4 Dirk Schulze 2013-04-13 08:02:35 PDT
(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?
Comment 5 Antti Koivisto 2013-04-13 08:55:10 PDT
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.