Bug 89879 - META: StyleResolver does too much and needs refactoring
Summary: META: StyleResolver does too much and needs refactoring
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 109909 112332 113292 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 109916 110038 110041 110663 110690 111252 112441 113298
Blocks: 111644
  Show dependency treegraph
 
Reported: 2012-06-25 08:13 PDT by Eric Seidel (no email)
Modified: 2013-03-26 05:16 PDT (History)
7 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
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. :)