Bug 147821 – Reduce use of PassRefPtr in WebCore/css

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Gyuyoung Kim]

SSIA.

[Gyuyoung Kim]

Created attachment 258609 [details]
Patch

[Gyuyoung Kim]

Created attachment 258610 [details]
Patch

[Daniel Bates]

Comment on attachment 258610 [details]
Patch

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

> Source/WebCore/css/CSSGridLineNamesValue.cpp:47
> +RefPtr<CSSGridLineNamesValue> CSSGridLineNamesValue::cloneForCSSOM() const

How did you come to the decision to change from Ref to RefPtr?

> Source/WebCore/css/RGBColor.cpp:43
> +    return WTF::move(result);

Is this WTF::move() necessary?

> Source/WebCore/css/RGBColor.cpp:51
> +    return WTF::move(result);

Ditto.

> Source/WebCore/css/RGBColor.cpp:59
> +    return WTF::move(result);

Ditto.

> Source/WebCore/css/RGBColor.cpp:67
> +    return WTF::move(result);

Ditto.

[Gyuyoung Kim]

Comment on attachment 258610 [details]
Patch

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

>> Source/WebCore/css/CSSGridLineNamesValue.cpp:47
>> +RefPtr<CSSGridLineNamesValue> CSSGridLineNamesValue::cloneForCSSOM() const
> 
> How did you come to the decision to change from Ref to RefPtr?

Oops, I confused this change with other cloneForCSSOM(). Fixed.

>> Source/WebCore/css/RGBColor.cpp:43
>> +    return WTF::move(result);
> 
> Is this WTF::move() necessary?

Yes, it is unnecessary. nice catch !

[Gyuyoung Kim]

Created attachment 258693 [details]
Patch

[Daniel Bates]

Comment on attachment 258693 [details]
Patch

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

> Source/WebCore/css/CSSCalculationValue.cpp:351
> +    static RefPtr<CSSCalcBinaryOperation> create(CalcOperator op, PassRefPtr<CSSCalcExpressionNode> leftSide, PassRefPtr<CSSCalcExpressionNode> rightSide)

I'm assuming you plan to remove the use of PassRefPtr in the arguments in a separate bug/patch.

> Source/WebCore/css/CSSCalculationValue.cpp:364
> +    static RefPtr<CSSCalcExpressionNode> createSimplified(CalcOperator op, PassRefPtr<CSSCalcExpressionNode> leftSide, PassRefPtr<CSSCalcExpressionNode> rightSide)

Ditto.

> Source/WebCore/css/CSSCalculationValue.cpp:574
> +    RefPtr<CSSCalcExpressionNode> parseCalc(CSSParserValueList* tokens)

This is OK as-is. The methods of this class, including this method, assume that argument tokens is a non-null pointer. So, we should change the data type of the argument tokens from CSSParserValueList* to CSSParserValueList& in all applicable methods of this class. This can be done in a separate bug.

> Source/WebCore/css/CSSComputedStyleDeclaration.h:71
> +    RefPtr<SVGPaint> adjustSVGPaintForCurrentColor(PassRefPtr<SVGPaint>, RenderStyle*) const;

I'm assuming you plan to remove the use of PassRefPtr in the first argument in a separate bug/patch.

> Source/WebCore/css/CSSParser.h:357
> +    RefPtr<StyleRuleBase> createMediaRule(PassRefPtr<MediaQuerySet>, RuleList*);

I'm assuming you plan to remove the use of PassRefPtr in the first argument in a separate bug/patch.

> Source/WebCore/css/CSSParser.h:561
> +    RefPtr<CSSBasicShape> parseInsetRoundedCorners(PassRefPtr<CSSBasicShapeInset>, CSSParserValueList&);

Ditto.

[Gyuyoung Kim]

Comment on attachment 258693 [details]
Patch

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

>> Source/WebCore/css/CSSCalculationValue.cpp:351
>> +    static RefPtr<CSSCalcBinaryOperation> create(CalcOperator op, PassRefPtr<CSSCalcExpressionNode> leftSide, PassRefPtr<CSSCalcExpressionNode> rightSide)
> 
> I'm assuming you plan to remove the use of PassRefPtr in the arguments in a separate bug/patch.

Yes, right. Now I focus on removing PassRefPtr in return type first. Then I will plan to reduce use of PassRefPtr in the arguments too.

>> Source/WebCore/css/CSSCalculationValue.cpp:574
>> +    RefPtr<CSSCalcExpressionNode> parseCalc(CSSParserValueList* tokens)
> 
> This is OK as-is. The methods of this class, including this method, assume that argument tokens is a non-null pointer. So, we should change the data type of the argument tokens from CSSParserValueList* to CSSParserValueList& in all applicable methods of this class. This can be done in a separate bug.

Ok, let me file a new bug for it.

>> Source/WebCore/css/CSSComputedStyleDeclaration.h:71
>> +    RefPtr<SVGPaint> adjustSVGPaintForCurrentColor(PassRefPtr<SVGPaint>, RenderStyle*) const;
> 
> I'm assuming you plan to remove the use of PassRefPtr in the first argument in a separate bug/patch.

ditto.

[WebKit Commit Bot]

Comment on attachment 258693 [details]
Patch

Clearing flags on attachment: 258693

Committed r188315: <http://trac.webkit.org/changeset/188315>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.