[webkit-reviews] review granted: [Bug 70048] Uncomplicate CSSRuleList. : [Attachment 110892] Proposed patch v2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andreas Kling <kling at webkit.org>'s
request for review:
Bug 70048: Uncomplicate CSSRuleList.
https://bugs.webkit.org/show_bug.cgi?id=70048

Attachment 110892: Proposed patch v2
https://bugs.webkit.org/attachment.cgi?id=110892&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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?