Bug 155695 – Purge PassRefPtr from CSSParser

• Status: RESOLVED
• Resolution: INVALID
• Component: WebCore Misc.

[Joonghun Park]

Purge PassRefPtr from CSSParser.

[Joonghun Park]

Created attachment 274536 [details]
Patch

[Joonghun Park]

Created attachment 274537 [details]
Fix build break of Efl port

[Darin Adler]

Comment on attachment 274537 [details]
Fix build break of Efl port

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

> Source/WebCore/css/CSSParser.cpp:1488
> +bool CSSParser::parseDeclaration(MutableStyleProperties* declaration, const String& string, RefPtr<CSSRuleSourceData>&& aRuleSourceData, StyleSheetContents* contextStyleSheet)

The use of the "a" prefix here is peculiar and not WebKit coding style. Lets just call this ruleSourceData.

> Source/WebCore/css/CSSParser.cpp:1495
> +    CSSRuleSourceData* ruleSourceData = aRuleSourceData.get();

I suppose it’s OK to make this local copy to avoid reference count churn. It is a little dangerous, though, since we are counting on m_currentRuleDataStack to keep the object alive after this point. And tricky to figure out names for the two local variables (the one that is null because we moved out of it and the raw pointer that is not). An "a" prefix won’t do it.

> Source/WebCore/css/CSSParser.cpp:1641
> +    RefPtr<CSSValue> val = value.copyRef();
> +    addProperty(propId, WTFMove(value), important, implicit);

Not the right way to do it. If we have to do the reference count churn here then we should just do:

    addProperty(propId, value.copyRef(), important, implicit);

Then we can use "value" below instead of "val".

> Source/WebCore/css/CSSParser.cpp:7865
> +    void commitColor(Ref<CSSPrimitiveValue>&& val)

Since we are touching this it would be good to replace the abbreviation "val" with a word like "value" or multiple words like "colorValue".

> Source/WebCore/css/CSSParser.cpp:8153
>          return createBorderImageValue(m_image, m_imageSlice, m_borderSlice, m_outset, m_repeat);

These should all be moving/releasing rather than copying for the same reason that commitBorderImage below should be.

> Source/WebCore/css/CSSParser.cpp:8162
> +        commitBorderImageProperty(CSSPropertyBorderImageSource, parser, m_image.copyRef(), important);
> +        commitBorderImageProperty(CSSPropertyBorderImageSlice, parser, m_imageSlice.copyRef(), important);
> +        commitBorderImageProperty(CSSPropertyBorderImageWidth, parser, m_borderSlice.copyRef(), important);
> +        commitBorderImageProperty(CSSPropertyBorderImageOutset, parser, m_outset.copyRef(), important);
> +        commitBorderImageProperty(CSSPropertyBorderImageRepeat, parser, m_repeat.copyRef(), important);

These should all be WTFMove rather than copyRef; this function is used just before destroying a BorderImageParseContext.

> Source/WebCore/css/CSSParser.cpp:8667
> +static Ref<CSSPrimitiveValue> parseDeprecatedGradientPoint(CSSParserValue& value, bool horizontal)

This doesn’t seem right. There are lots of paths here that turn into nullptr.

> Source/WebCore/css/CSSParser.cpp:8681
> +    return result.releaseNonNull();

I see no guarantee that this is non-null.

> Source/WebCore/css/CSSParser.cpp:12776
> +RefPtr<StyleRuleBase> CSSParser::createMediaRule(RefPtr<MediaQuerySet>&& media, RuleList* rules)

I don’t see this using WTFMove when calling StyleRuleMedia::create. So the rvalue reference is doing us no good here. What’s the story?