Bug 70048 – Uncomplicate CSSRuleList.

• Status: RESOLVED
• Resolution: INVALID
• Component: CSS

[Andreas Kling]

CSSRuleList is currently a bit schizophrenic due to the needs of CSSMediaRule and WebKitCSSKeyframesRule.
We should move this complexity out of CSSRuleLIst and make it look more like the CSSOM's CSSRuleList.

[Andreas Kling]

Created attachment 110891 [details]
Proposed patch

[Andreas Kling]

Comment on attachment 110891 [details]
Proposed patch

Woops, just caught one of the assertions firing in the inspector. Clearing for investigation.

[Andreas Kling]

Created attachment 110892 [details]
Proposed patch v2

Fixed assertion in the charset rule filtering by calling StyleList::append() instead of CSSRuleList::append().

[Darin Adler]

Comment on attachment 110892 [details]
Proposed patch v2

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

Seems great to eliminate the confusing dual mode for CSSRuleList, but also seems like a possibly-inefficient way to implement things to use a class designed for exposure to the DOM just to hold a list of rules.

> Source/WebCore/css/CSSRuleList.cpp:69
> +    StyleBase* rule = m_list->item(index);
> +    ASSERT(!rule || rule->isRule());
> +    return static_cast<CSSRule*>(rule);

What guarantees that other kinds of objects won’t get into the StyleList?

> Source/WebCore/css/CSSRuleList.cpp:72
>  void CSSRuleList::append(CSSRule* rule)

Why do we still need this function? Can’t the clients just work with the style list directly?

> Source/WebCore/css/CSSRuleList.cpp:77
>          // FIXME: Should we throw an exception?

This comment should go away. This is not a DOM function, so the question of throwing an exception doesn’t arise.

The relevant question here is whether we should just assert that rule is non-zero instead of silently doing nothing.

> Source/WebCore/css/StyleList.h:37
> +            return adoptRef(new StyleList);

Why not pass 0 here, instead of making the parent default to 0?

[Andreas Kling]

(In reply to comment #4)
> (From update of attachment 110892 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110892&action=review
> 
> Seems great to eliminate the confusing dual mode for CSSRuleList, but also seems like a possibly-inefficient way to implement things to use a class designed for exposure to the DOM just to hold a list of rules.

Possibly indeed, though it's quite convenient in the current state of things. I considered simply keeping a list of sub-rules on CSSMediaRule/WebKitCSSKeyframesRule , but that would make exposing a live .cssRules object much trickier. If you have suggestions I'm all ears of course. :)

> > Source/WebCore/css/CSSRuleList.cpp:69
> > +    StyleBase* rule = m_list->item(index);
> > +    ASSERT(!rule || rule->isRule());
> > +    return static_cast<CSSRule*>(rule);
> 
> What guarantees that other kinds of objects won’t get into the StyleList?

I haven't investigated that fully yet. Given this assertion, you'd think the isRule() check in the constructor's charset rule filtering code is unnecessary. I'll continue on this, it doesn't make sense for CSSRuleList to manage anything but CSSRules AFAIK.

> > Source/WebCore/css/CSSRuleList.cpp:72
> >  void CSSRuleList::append(CSSRule* rule)
> 
> Why do we still need this function? Can’t the clients just work with the style list directly?

True. I'll revise.

> > Source/WebCore/css/CSSRuleList.cpp:77
> >          // FIXME: Should we throw an exception?
> 
> This comment should go away. This is not a DOM function, so the question of throwing an exception doesn’t arise.

Ok great, I wasn't sure.

> The relevant question here is whether we should just assert that rule is non-zero instead of silently doing nothing.
> 
> > Source/WebCore/css/StyleList.h:37
> > +            return adoptRef(new StyleList);
> 
> Why not pass 0 here, instead of making the parent default to 0?

I couldn't decide which looked nicer. On second thought, I agree it's better to avoid adding ways to construct a StyleList.

[Andreas Kling]

When revising I realized this approach causes a subtle behavior change, StyleBase::insertedIntoParent() will now be called on rules as they are added to the CSSRuleList's backing StyleList. This virtual is only implemented by CSSImportRule, where it [may] fire a ResourceRequest for the sheet-to-be-imported.

I'm not sure this is desirable, will look into it tomorrow.

[Andreas Kling]

Comment on attachment 110892 [details]
Proposed patch v2

This patch needs to be redone.

[Andreas Kling]

CSSRuleList has changed significantly and this doesn't apply anymore.