[webkit-reviews] review granted: [Bug 79092] Remove stylesheet pointer from StylePropertySet : [Attachment 127945] patch

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 79092: Remove stylesheet pointer from StylePropertySet
https://bugs.webkit.org/show_bug.cgi?id=79092

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

------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=127945&action=review


r=me with some glorious exposition:

> Source/WebCore/css/CSSParser.cpp:349
> +    Document* document = contextStyleSheet->findDocument();

Are we sure it's safe to always dereference 'contextStyleSheet' here? An
assertion might be nice.

> Source/WebCore/css/CSSParser.cpp:473
> +    Document* document = contextStyleSheet->findDocument();

Same story.

> Source/WebCore/css/WebKitCSSMatrix.cpp:56
> -    if (CSSParser::parseValue(styleDeclaration.get(),
CSSPropertyWebkitTransform, string, true, true)) {
> +    if (CSSParser::parseValue(styleDeclaration.get(),
CSSPropertyWebkitTransform, string, true, true, 0)) {

Here's a call site where parseValue() would have a null contextStyleSheet.
Though it won't be a problem for CSSPropertyWebkitTransform, it sets a bad
precedent.

> Source/WebCore/dom/StyledElement.h:72
> +    void applyPresentationAttributeToStyle(StylePropertySet*, int
propertyID, int value);
> +    void applyPresentationAttributeToStyle(StylePropertySet*, int
propertyID, double value, CSSPrimitiveValue::UnitTypes);
> +    void applyPresentationAttributeToStyle(StylePropertySet*, int
propertyID, const String& value);

This name is not entirely fabulous, as the function is only concerned with a
single property at a time. addPropertyToAttributeStyle() perhaps?