Bug 147821 - Reduce use of PassRefPtr in WebCore/css
Summary: Reduce use of PassRefPtr in WebCore/css
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on:
Blocks: 144092
  Show dependency treegraph
 
Reported: 2015-08-09 22:27 PDT by Gyuyoung Kim
Modified: 2015-08-11 18:13 PDT (History)
3 users (show)

See Also:


Attachments
Patch (126.79 KB, patch)
2015-08-09 22:28 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (127.28 KB, patch)
2015-08-09 23:03 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (130.28 KB, patch)
2015-08-10 20:29 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2015-08-09 22:27:13 PDT
SSIA.
Comment 1 Gyuyoung Kim 2015-08-09 22:28:13 PDT
Created attachment 258609 [details]
Patch
Comment 2 Gyuyoung Kim 2015-08-09 23:03:05 PDT
Created attachment 258610 [details]
Patch
Comment 3 Daniel Bates 2015-08-10 18:12:56 PDT
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.
Comment 4 Gyuyoung Kim 2015-08-10 20:29:31 PDT
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 !
Comment 5 Gyuyoung Kim 2015-08-10 20:29:53 PDT
Created attachment 258693 [details]
Patch
Comment 6 Daniel Bates 2015-08-11 14:10:47 PDT
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.
Comment 7 Gyuyoung Kim 2015-08-11 17:24:40 PDT
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.
Comment 8 WebKit Commit Bot 2015-08-11 18:13:19 PDT
Comment on attachment 258693 [details]
Patch

Clearing flags on attachment: 258693

Committed r188315: <http://trac.webkit.org/changeset/188315>
Comment 9 WebKit Commit Bot 2015-08-11 18:13:23 PDT
All reviewed patches have been landed.  Closing bug.