[Webkit-unassigned] [Bug 141877] Web Inspector: split out CSSAgent wrapper classes to fix protocol layering violation and cruft

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=141877

--- Comment #6 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 247264
  --> https://bugs.webkit.org/attachment.cgi?id=247264
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().

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150224/c3db4614/attachment-0002.html>