Bug 89879

Summary: META: StyleResolver does too much and needs refactoring
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED CONFIGURATION CHANGED    
Severity: Normal CC: ahmad.saleem792, eae, jchaffraix, kling, koivisto, macpherson, simon.fraser, tonyg
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 89881, 96421, 96423, 96425, 96431, 97953, 100582, 100655, 105660, 105752, 105863, 105864, 106413, 106860, 106879, 107095, 107358, 107460, 107777, 107779, 107780, 108563, 108595, 108890, 109653, 109909, 109916, 110038, 110041, 110663, 110690, 111252, 112332, 112441, 113292, 113298    
Bug Blocks: 111644    

Description Eric Seidel (no email) 2012-06-25 08:13:06 PDT
StyleResolver does to much and needs refactoring

In my opinion, at least the following should be split out of StyleResolver/StyleResolver.cpp
- element/styledElement (resolve lifetime - per-element/per-resolve state)
- style mapping functions (static - map*, which are mostly used to support StyleBuilder)
- Features object (document/stylesheet lifetime - probably just needs its own file)
- style sharing logic (static)
- default style-sheet statics (static)

The largest problem is the storage of both document (or at least between-document-changes), as well as during-resolve helper cache objects (very similar to how PaintInfo or LayoutState function during paint and layout phases).

I would like to rip out the "ResolveState" (during-resolve cache) object first, but that's going to be tricky, so I'm pulling off some of these other smaller objects first.

I'll file bugs for each, and I welcome your thoughts and comments.
Comment 1 Antti Koivisto 2012-06-25 14:09:11 PDT
What are you trying to achieve concretely? Refactoring for the sake of refactoring usually ends up in tears.
Comment 2 Antti Koivisto 2012-06-25 14:36:28 PDT
If you want to refactor something here I would suggest making StyleBuilder sane and moving all property applying code there. People are still adding new properties to the big switch in StyleResolver since adding anything to StyleBuilder is too difficult. Note that the current state of StyleBuilder is a result of a previous drive-by refactoring.
Comment 3 Eric Seidel (no email) 2013-03-21 17:36:42 PDT
At a high-level, my top priority with this work would be to split all the state out of StyleResolver into (at least) 3 separate objects, corresponding to the data lifetime:
1. Document lifetime
2. Resolve lifetime
3. Element-resolve lifetime

Some of this has already been done, but I wanted to re-state for posterity.

I believe StyleResolver could basically just be a state-less (or possibly only resolve-lifetime state) class, who's main purpose is to direct traffic during a style resolve.

All the document-lifetime storage left (if any), should just be moved onto a separate object on Document for clarity of lifetime.

One path forward with this (and on my todo-list) is to simply walk through all of StyleResolver's remaining members and annotate them as to their expected lifetime, similar to what Darin Adler did for Frame methods (sorting them into objects using comments) several years ago as a comment-document vision as to how to break Frame up.  I welcome someone else to post a patch to do such, or I'll eventually get around to doing so. :)
Comment 4 Ahmad Saleem 2022-08-06 14:36:05 PDT
All dependent bugs are resolved now, I am going to mark this as "RESOLVED CONFIGURATION CHANGED". Thanks!