Bug 89879
| Summary: | META: StyleResolver does too much and needs refactoring | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> |
| Component: | Layout and Rendering | Assignee: | 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 | ||
Eric Seidel (no email)
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.
| Attachments | ||
|---|---|---|
| Add attachment proposed patch, testcase, etc. |
Antti Koivisto
What are you trying to achieve concretely? Refactoring for the sake of refactoring usually ends up in tears.
Antti Koivisto
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.
Eric Seidel (no email)
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. :)
Ahmad Saleem
All dependent bugs are resolved now, I am going to mark this as "RESOLVED CONFIGURATION CHANGED". Thanks!