[webkit-reviews] review granted: [Bug 229218] Web Inspector: Style rules declared after a rule whose selector has over 8192 components are not shown correctly : [Attachment 435934] Patch v1.4 - Refine new member names in StyleRule

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Patrick Angle <pangle at apple.com>'s
request for review:
Bug 229218: Web Inspector: Style rules declared after a rule whose selector has
over 8192 components are not shown correctly
https://bugs.webkit.org/show_bug.cgi?id=229218

Attachment 435934: Patch v1.4 - Refine new member names in StyleRule

https://bugs.webkit.org/attachment.cgi?id=435934&action=review




--- Comment #11 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 435934
  --> https://bugs.webkit.org/attachment.cgi?id=435934
Patch v1.4 - Refine new member names in StyleRule

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

r=me, nice work! :)

> Source/WebCore/ChangeLog:31
> +	   mark the last rule split from a given rule
(`m_isLastRuleInSplitRule`). The second member prevents situations
> +	   where we might otherwise accidentally continue iterating into a list
of `StyleRule` where two large authored
> +	   rules that had to be split are present next to each other in an
ordered collection of `StyleRule`.

Can we add a test for this scenario too?  e.g. create another rule set that
flips the a-yA-Y order as z-bZ-B instead =P

> Source/WebCore/css/StyleRule.h:127
> +    bool m_isSplitRule { false };
> +    bool m_isLastRuleInSplitRule { false };

NIT: I'd put these at the end for (potentially) better packing

> Source/WebCore/inspector/InspectorStyleSheet.cpp:1116
> +	   auto rule = m_flatRules.at(i);

You're assuming that items in `m_flatRules` exist above.  Can/Should we assume
that it exists here too?  Or do we need to add checks above?

> Source/WebCore/inspector/InspectorStyleSheet.cpp:1123
> +	   rules.append(*rule.get());

Does `*rule` not work?

> Source/WebCore/inspector/InspectorStyleSheet.cpp:1301
> +    const auto treatSplitRulesAsSingleRule = true;

`constexpr auto`

> Source/WebCore/inspector/InspectorStyleSheet.cpp:1312
> +unsigned InspectorStyleSheet::ruleIndexByStyle(CSSStyleDeclaration*
pageStyle, bool treatSplitRulesAsSingleRule) const

NIT: How about `combineSplitRules`?

> LayoutTests/inspector/css/getMatchedStylesForNodeLargeSelectors.html:61
> +	       }

Style: unnecessary `{` `}`

> LayoutTests/inspector/css/getMatchedStylesForNodeLargeSelectors.html:93
> +    /* Intentional preceding selector comment */ #test1 /* Intentional
following selector comment. */ {

Are the comments actually needed for the test?	If so, why?

> LayoutTests/inspector/css/getMatchedStylesForNodeLargeSelectors.html:271
> +    #unusedRule {
> +	   /* The rule intentionally left blank. */
> +    }

Is this actually needed for the test?  If so, why?