Bug 84383 – Remove unnecessary null check for styleSelector on RuleSet::addRulesFromSheet

• Component: CSS
• Status: RESOLVED WONTFIX

[Rafael Brandao]

Remove unnecessary check on CSSStyleSelector::addRulesFromSheet

[Rafael Brandao]

Created attachment 137975 [details]
Patch

[Alexis Menard (darktears)]

Comment on attachment 137975 [details]
Patch

Changelog should be more explicit why it is unnecessary and also why you don't change line 2527, 2535?

[Alexey Proskuryakov]

Comment on attachment 137975 [details]
Patch

> Changelog should be more explicit why it is unnecessary and also why you don't change line 2527, 2535?

I strongly agree. Let's make this r- until there full is clarity.

[Alexey Proskuryakov]

...there is full clarity.

[Rafael Brandao]

Sure. The thing is inside that if we don't make use of styleSelector, like on the previous ifs (for example rule->isStyleRule() or rule->isPageRule()). Differently from lines 2527 and 2535 where we use styleSelector inside, so it's reasonable to check that pointer.

[Luke Macpherson]

Comment on attachment 137975 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Remove unnecessary check on CSSStyleSelector::addRulesFromSheet

Bug title could be improved by using the word null somewhere.

> Source/WebCore/ChangeLog:8
> +        No new tests needed, just a cleanup.

I think you should just update the ChangeLog to say that the styleSelector null check is not needed in this case because styleSelector is not dereferenced or passed to anything else here.

> Source/WebCore/css/CSSStyleSelector.cpp:2540
> +        } else if (rule->isRegionRule()) {

FWIW the actual code change seems safe, though since it is always checked right now, it might be even better to remove all the individual styleSelector checks and instead have a single check earlier in the function.

[Rafael Brandao]

StyleSelector became StyleResolver, should I also rename the variable for styleResolver? I won't do it unless it's necessary.

> > Source/WebCore/css/CSSStyleSelector.cpp:2540
> > +        } else if (rule->isRegionRule()) {
> 
> FWIW the actual code change seems safe, though since it is always checked right now, it might be even better to remove all the individual styleSelector checks and instead have a single check earlier in the function.

You mean to add a null check on the beginning of addRulesFromSheet? If so, that would change the behavior here, some rules doesn't depend on that styleSelector.

Maybe grouping rules that depend on it would be better? I will submit a patch trying this approach.

[Rafael Brandao]

Created attachment 140337 [details]
Patch

[Brent Fulgham]

Comment on attachment 140337 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:-2545
> -        } else if (rule->isRegionRule() && styleSelector) {

Can rules have multiple categories (e.g,, a rule is both a key frame style *and* a region rule? If so, this change will subtly modify the behavior of this code.

If they can never be overlapping, this change looks fine (but that might imply that the rules should be unique objects, rather than flags on a generic rule object.)

[Antti Koivisto]

Comment on attachment 140337 [details]
Patch

This patch is almost pure noise.