Bug 141877 – Web Inspector: split out CSSAgent wrapper classes to fix protocol layering violation and cruft

• Component: Web Inspector
• Status: NEW

[Brian Burg]

Yuck!

[Radar WebKit Bug Importer]

<rdar://problem/19915490>

[Brian Burg]

Created attachment 247080 [details]
WIP

[Brian Burg]

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)

[Timothy Hatcher]

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.

[Brian Burg]

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.

[Joseph Pecoraro]

Comment on attachment 247264 [details]
WIP 2

View in context: https://bugs.webkit.org/attachment.cgi?id=247264&action=review

I like the functional aspect of this. I'm not totally familiar with the code, but I really like that it is separating out into discrete classes / files that can be more easily understood on their own. I mostly just made style nits as I looked over it.

> Source/JavaScriptCore/inspector/protocol/CSS.json:9
>              "id": "StyleSheetId",
> -            "type": "string"
> +            "type": "integer"
>          },

We need to make sure the frontend handles this properly with legacy backends.

> Source/JavaScriptCore/inspector/protocol/CSS.json:-180
> -            "enum": ["active", "inactive", "disabled", "style"],

Good research! I just found this out myself.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:216
> -        return String::format("SetStyleSheetText %s", m_styleSheet->id().utf8().data());
> +        return String::format("SetStyleSheetText %" PRIu64, m_ruleSet->identifier());

Nit: %llu

> Source/WebCore/inspector/InspectorCSSAgent.cpp:256
> +        if (is<StylesheetRuleSet>(m_ruleSet))
> +            downcast<StylesheetRuleSet>(m_ruleSet.get()).withStyleForId(m_cssId, handler);
> +        if (is<InlineStyleRuleSet>(m_ruleSet))
> +            downcast<InlineStyleRuleSet>(m_ruleSet.get()).withInlineStyle(handler);

Nit: else-if since I believe these are mutually exclusive. Same in redo as well.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:718
> +    if (is<InlineStyleRuleSet>(ruleSet)) {

Ditto.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:1070
> +        return Inspector::Protocol::CSS::CSSMedia::Source::ImportRule;
> +        break;

You can remove all these break after returns. They would be "-Wunreachable-code" warnings.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:1096
> +        auto selectorObject = Inspector::Protocol::CSS::CSSSelector::create()
> +            .setText(selector.selectorText())
> +            .release();

While you are here. I've considered moving "matches" into CSSSelector. Instead of CSS.getMatchedStylesForNode's awkward matchedCSSRules out parameter array.

This information is already only computable when there is a context element (m_element) so it only gets good values when created in a getMatchedStylesForNode response.

Doesn't need to be added, just something to keep in mind we might want to do.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:1125
> +    CollectRuleSelectorsFunctor collectSelector(element);

Could be inlined, it might read better! I thought collectorSelector was going to be used elsewhere, or had some destruction semantics, but it doesn't.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:1143
> +    CollectStylesheetRulesFunctor()

Stylesheet => StyleSheet? Seems we do the latter more often.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:1223
> +            sourceURL = "";

Nit: String() instead of "" when possible.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:1241
> +                        sourceURL = "";

Ditto

> Source/WebCore/inspector/InspectorCSSAgent.cpp:1436
> +    CollectPropertyShorthandsFunctor()
> +    : m_shorthandsArray(Inspector::Protocol::Array<Inspector::Protocol::CSS::ShorthandEntry>::create())
> +    {
> +    }

Style: There have been a few of these Collect class with broken indentation which you will want to clean up.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:1481
> +        propertyObject->setParsedOk(property.parsedOk());
> +        propertyObject->setImplicit(property.isImplicit());

If these are always set, maybe they should not be optional.

> Source/WebCore/inspector/InspectorDOMTracingAgent.h:130
> -    
> +

lol.

> Source/WebCore/inspector/InspectorInlineStyleRuleSet.cpp:59
> +        m_element->setAttribute("style", text, ec);

Nit: ASCIILiteral, here and a few other places.

> Source/WebCore/inspector/InspectorInlineStyleRuleSet.h:77
> +    StyleDeclaration wrapper(CSSId(identifier(), 0), *inlineStyle(), this);

These seem like they need a special constructor instead of passing 0, unless the 0 is really needed. Style attributes don't have a "rule ordinal" do they?

> Source/WebCore/inspector/InspectorRuleSelector.h:42
> +    RuleSelector(RuleSelector&&) = default;

That explains the tweet.

> Source/WebCore/inspector/InspectorStyleDeclaration.cpp:73
> +    HashSet<String> foundProperties;
> +
> +    if (RefPtr<WebCore::CSSRuleSourceData> sourceData = styleSourceData()) {
> +        Vector<WebCore::CSSPropertySourceData>& sourcePropertyData = sourceData->styleSourceData->propertyData;
> +
> +        for (auto& data : sourcePropertyData) {
> +            foundProperties.add(data.name.lower());
> +            result.append(StyleProperty(*this, data, true));
> +        }
> +    }
> +
> +    for (int i = 0, size = m_declaration.length(); i < size; ++i) {
> +        String name = m_declaration.item(i);
> +        if (foundProperties.contains(name.lower()))
> +            continue;
> +
> +        foundProperties.add(name.lower());
> +        result.append(StyleProperty(*this, WebCore::CSSPropertySourceData(name, m_declaration.getPropertyValue(name), !m_declaration.getPropertyPriority(name).isEmpty(), true, WebCore::SourceRange()), false));
> +    }

Lots of name.lower()s. I wonder if the case folding HashSet would be better:

  HashSet<String, CaseFoldingHash> foundProperties;

Or at the very least, eliminating some of the duplicate lower() calls.

> Source/WebCore/inspector/InspectorStyleRule.h:48
> +class StyleRule final {

Is the final needed, this doesn't subclass anything, right?

> Source/WebCore/inspector/InspectorStyleRule.h:60
> +    bool hasSelectors();

Not used. Might be removable.

> Source/WebCore/inspector/InspectorStyleRuleSet.cpp:114
> +std::unique_ptr<ParsedRuleSetData> ParsedRuleSetData::createEmpty()
> +    {
> +        return std::make_unique<ParsedRuleSetData>(emptyString(), std::make_unique<RuleSourceDataList>(), std::make_unique<StyleRuleList>());
> +    }

More weird whitespace.

Also, maybe String() instead of emptyString().