|Summary:||Web Inspector: split out CSSAgent wrapper classes to fix protocol layering violation and cruft|
|Product:||WebKit||Reporter:||Brian Burg <burg>|
|Component:||Web Inspector||Assignee:||Nobody <webkit-unassigned>|
|Severity:||Normal||CC:||benjamin, inspector-bugzilla-changes, jonowells, webkit-bug-importer|
|Version:||528+ (Nightly build)|
Description Brian Burg 2015-02-21 20:56:53 PST
Comment 3 Brian Burg 2015-02-21 22:10:50 PST
Yeah, I know, this will be hell to review. Maybe we can do it in person at WebKitCon. The gist of this change: be able to introspect, dump and modify CSS from other backend agents. Currently, CSSAgent heavily uses a wrapper class InspectorStyleSheet, which represents a stylesheet or an inline style (with subclass InspectorStyleSheetForInlineStyle). These classes hold source range data, raw style text, and have a basic API for mutating properties or entire stylesheets. These existing classes do not map well to what actually is shared and differs between inline styles and stylesheets. As a result, the code is convoluted and littered with null checks to tell apart the two "modes". There is also a lot of code to maintain up-to-date source data, raw text, and style rules, but each piece of data has a different lifetime. To top it all off, responsibility for protocol encoding is split across CSSAgent and the various wrapper classes. In some cases, a style rule descends through 3 protocol object builder methods to fully construct a protocol object. I'm proceeding in several steps to clean up this mess: 1. Convert to using a "StyleRuleSet" abstraction. A StyleRuleSet contains a number of "StyleRule" instances. Inline style rules are represented by InlineStyleRuleSet. Its single rule has no selector. It overrides mutation methods to modify the element's style attribute. Stylesheets are represented by the StylesheetRuleSet subclass. 2. Move all protocol builder classes to CSSAgent 3. Straighten out StyleRule, StyleProperty, etc. Protocol builder methods should walk over a wrapper hierarchy that mirrors the actual CSS stylesheet/rule/property hierarchy. 4. Remove unnecessary wrapper methods that are made obsolete by proper per-style and per-property APIs introduced in (3)
Comment 4 Timothy Hatcher 2015-02-22 07:36:50 PST
Comment on attachment 247080 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=247080&action=review Looks nice! I'll defer to others for this review, but I found two little nits up front. > Source/WebCore/inspector/InspectorCSSAgent.cpp:201 > + return String::format("SetStyleSheetText %" PRIu64, m_ruleSet->identifier()); %llu instead of PRIu64? I had to Google it. > Source/WebCore/inspector/InspectorCSSAgent.cpp:555 > + CSSStyleDeclaration* style = ruleSet ? ruleSet->inlineStyle() : nullptr; > + if (style) Nit: One line.
Comment 5 Brian Burg 2015-02-24 13:50:10 PST
Created attachment 247264 [details] WIP 2 The latest WIP patch is pretty close to the design I wanted. The main difference from ToT is that the protocol builders do less work digging for values. Instead that code is put in wrapper classes (StyleRule, StyleProperty, RuleSelector, etc) that can be used by other parts of the inspector backend. They are called 'wrappers' because they don't really own any data. At the top level, CSSAgent owns InlineStyleRuleSet's and StylesheetRuleSet's, which contain parsed data and strong refs to the CSSStyleSheet or Element. All other wrappers (styles, rules, selectors) are facades on top of the RuleSet's parsed source data and the relevant CSSOM objects. To minimize unnecessary refcounting and cycles, wrappers are noncopyable, stack allocated views only accessible through lambdas/functor interfaces. It's better to show what I mean with examples, so I'll highlight a few interesting lines.
Comment 6 Joseph Pecoraro 2015-02-24 15:11:23 PST