Bug 157808 – Clean up CSS code

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Clean up CSS code

[Alex Christensen]

Created attachment 279158 [details]
Patch

[Alex Christensen]

Created attachment 279171 [details]
Patch

[Darin Adler]

Comment on attachment 279171 [details]
Patch

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

> Source/WebCore/css/CSSProperty.h:66
>          , m_value(value)

Need WTFMove here.

> Source/WebCore/css/StyleProperties.cpp:755
> +void MutableStyleProperties::setProperty(CSSPropertyID propertyID, RefPtr<CSSValue>&& prpValue, bool important)

Should rename prpValue to value.

> Source/WebCore/css/StylePropertyShorthand.h:32
> +        : m_properties(nullptr)

Should initialize data members where they are defined rather than in the constructor.

> Source/WebCore/css/StyleResolver.cpp:303
>      m_keyframesRuleMap.set(s.impl(), rule);

Missing WTFMove here.

[Alex Christensen]

Created attachment 279177 [details]
Patch

[Alex Christensen]

Created attachment 279182 [details]
Patch

[Build Bot]

Comment on attachment 279182 [details]
Patch

Attachment 279182 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1338834

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 279189 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Alex Christensen]

Created attachment 279220 [details]
Patch

[Build Bot]

Comment on attachment 279220 [details]
Patch

Attachment 279220 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1341189

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 279223 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Alex Christensen]

Created attachment 279227 [details]
Patch

[Alex Christensen]

Created attachment 279257 [details]
Patch

[Alex Christensen]

Created attachment 279260 [details]
Patch

[Chris Dumez]

Comment on attachment 279260 [details]
Patch

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

> Source/WebCore/css/BasicShapeFunctions.cpp:191
> +static BasicShapeRadius cssValueToBasicShapeRadius(const CSSToLengthConversionData& conversionData, RefPtr<CSSPrimitiveValue>&& radius)

We don't technically transfer ownership, I think this should be a CSSPrimitiveValue*

> Source/WebCore/css/CSSAnimationTriggerScrollValue.h:36
> +    static Ref<CSSAnimationTriggerScrollValue> create(RefPtr<CSSValue>&& startValue, RefPtr<CSSValue>&& endValue = nullptr)

Can start be null? If not, it should be a Ref<>

> Source/WebCore/css/CSSAnimationTriggerScrollValue.h:52
>          , m_startValue(startValue)

Missing WTFMove()

> Source/WebCore/css/CSSAnimationTriggerScrollValue.h:53
>          , m_endValue(endValue)

Missing WTFMove()

> Source/WebCore/css/CSSBasicShapes.h:79
> +    void setTop(RefPtr<CSSPrimitiveValue>&& top) { m_top = top; }

Missing WTFMove()?

> Source/WebCore/css/CSSBasicShapes.h:80
> +    void setRight(RefPtr<CSSPrimitiveValue>&& right) { m_right = right; }

ditto

> Source/WebCore/css/CSSBasicShapes.h:81
> +    void setBottom(RefPtr<CSSPrimitiveValue>&& bottom) { m_bottom = bottom; }

ditto

> Source/WebCore/css/CSSBasicShapes.h:82
> +    void setLeft(RefPtr<CSSPrimitiveValue>&& left) { m_left = left; }

ditto

> Source/WebCore/css/CSSBasicShapes.h:107
> +    void setTopLeftRadius(RefPtr<CSSPrimitiveValue>&& radius) { m_topLeftRadius = radius; }

Missing WTFMove()?

> Source/WebCore/css/CSSBasicShapes.h:108
> +    void setTopRightRadius(RefPtr<CSSPrimitiveValue>&& radius) { m_topRightRadius = radius; }

ditto

> Source/WebCore/css/CSSBasicShapes.h:109
> +    void setBottomRightRadius(RefPtr<CSSPrimitiveValue>&& radius) { m_bottomRightRadius = radius; }

ditto

> Source/WebCore/css/CSSBasicShapes.h:110
> +    void setBottomLeftRadius(RefPtr<CSSPrimitiveValue>&& radius) { m_bottomLeftRadius = radius; }

ditto

> Source/WebCore/css/CSSBasicShapes.h:138
> +    void setCenterX(RefPtr<CSSPrimitiveValue>&& centerX) { m_centerX = centerX; }

Missing WTFMove()?

> Source/WebCore/css/CSSBasicShapes.h:139
> +    void setCenterY(RefPtr<CSSPrimitiveValue>&& centerY) { m_centerY = centerY; }

ditto

> Source/WebCore/css/CSSBasicShapes.h:140
> +    void setRadius(RefPtr<CSSPrimitiveValue>&& radius) { m_radius = radius; }

ditto

> Source/WebCore/css/CSSBorderImage.cpp:30
>      if (image)

You want to use some releaseNonNull() below instead of *imageSlice (etc..) CSSValueList::append() takes in a Ref<>&&.

> Source/WebCore/css/CSSBorderImageSliceValue.h:39
> +        return adoptRef(*new CSSBorderImageSliceValue(WTFMove(slices), fill));

The constructor is still using a PassRefPtr, please update it.

> Source/WebCore/css/CSSCanvasValue.h:57
> +        , m_element(nullptr)

We probably want to initialize the member inline instead:
HTMLCanvasElement* m_element { nullptr };

below.

> Source/WebCore/css/SVGCSSComputedStyleDeclaration.cpp:98
>      RefPtr<SVGPaint> paint = newPaint;

Useless local variable?

Also, if we drop the local, I think we'll need to WTFMove() in the return statement.

> Source/WebCore/css/StyleResolver.cpp:1690
> +RefPtr<StyleImage> StyleResolver::generatedOrPendingFromValue(CSSPropertyID property, CSSImageGeneratorValue& value)

Cannot this return a Ref<>?

[Alex Christensen]

Created attachment 279279 [details]
Patch

[Alex Christensen]

Created attachment 279281 [details]
Patch

[Build Bot]

Comment on attachment 279281 [details]
Patch

Attachment 279281 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1344515

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 279289 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 279281 [details]
Patch

Attachment 279281 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1344510

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 279290 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 279281 [details]
Patch

Attachment 279281 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1344524

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 279292 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 279281 [details]
Patch

Attachment 279281 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1344518

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 279295 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Alex Christensen]

Created attachment 279303 [details]
Patch

[Chris Dumez]

Comment on attachment 279303 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:8
> +        No new tests.  Just cleaning up and modernizing code.

nit: extra space in there

> Source/WebCore/css/CSSAnimationTriggerScrollValue.h:41
>      const CSSValue* startValue() const { return m_startValue.get(); }

This should probably return a reference? And m_startValue should probably be a Ref<>?

> Source/WebCore/css/SVGCSSComputedStyleDeclaration.cpp:100
>      return paint;

Are you sure we're not supposed to WTFMove() here? We are not returning a local variable anymore.

> Source/WebCore/css/StylePropertyShorthand.h:31
>      StylePropertyShorthand()

= default; ?

> Source/WebCore/css/StyleResolver.cpp:306
> +void StyleResolver::addKeyframeStyle(RefPtr<StyleRuleKeyframes>&& rule)

Looks like this should be a Ref<>

[Alex Christensen]

This patch is getting too big.  I committed http://trac.webkit.org/changeset/201113
(In reply to comment #26)
> > Source/WebCore/css/CSSAnimationTriggerScrollValue.h:41
> >      const CSSValue* startValue() const { return m_startValue.get(); }
> 
> This should probably return a reference? And m_startValue should probably be
> a Ref<>?
Maybe in another patch.
> 
> > Source/WebCore/css/SVGCSSComputedStyleDeclaration.cpp:100
> >      return paint;
> 
> Are you sure we're not supposed to WTFMove() here? We are not returning a
> local variable anymore.
I was under the impression that c++11 made it so we never need to return WTFMove(anything), but I could be wrong.  I put it in.
> > Source/WebCore/css/StyleResolver.cpp:306
> > +void StyleResolver::addKeyframeStyle(RefPtr<StyleRuleKeyframes>&& rule)
> 
> Looks like this should be a Ref<>
What???  I don't think so.

[Chris Dumez]

(In reply to comment #27)
> This patch is getting too big.  I committed
> http://trac.webkit.org/changeset/201113
> (In reply to comment #26)
> > > Source/WebCore/css/CSSAnimationTriggerScrollValue.h:41
> > >      const CSSValue* startValue() const { return m_startValue.get(); }
> > 
> > This should probably return a reference? And m_startValue should probably be
> > a Ref<>?
> Maybe in another patch.
> > 
> > > Source/WebCore/css/SVGCSSComputedStyleDeclaration.cpp:100
> > >      return paint;
> > 
> > Are you sure we're not supposed to WTFMove() here? We are not returning a
> > local variable anymore.
> I was under the impression that c++11 made it so we never need to return
> WTFMove(anything), but I could be wrong.  I put it in.
> > > Source/WebCore/css/StyleResolver.cpp:306
> > > +void StyleResolver::addKeyframeStyle(RefPtr<StyleRuleKeyframes>&& rule)
> > 
> > Looks like this should be a Ref<>
> What???  I don't think so.

Could you elaborate? The first thing the method does is dereference 'rule' so how could it ever be null? As such, I maintain this should be a Ref<>&& not a RefPtr<>&&.

[Chris Dumez]

(In reply to comment #28)
> (In reply to comment #27)
> > This patch is getting too big.  I committed
> > http://trac.webkit.org/changeset/201113
> > (In reply to comment #26)
> > > > Source/WebCore/css/CSSAnimationTriggerScrollValue.h:41
> > > >      const CSSValue* startValue() const { return m_startValue.get(); }
> > > 
> > > This should probably return a reference? And m_startValue should probably be
> > > a Ref<>?
> > Maybe in another patch.
> > > 
> > > > Source/WebCore/css/SVGCSSComputedStyleDeclaration.cpp:100
> > > >      return paint;
> > > 
> > > Are you sure we're not supposed to WTFMove() here? We are not returning a
> > > local variable anymore.
> > I was under the impression that c++11 made it so we never need to return
> > WTFMove(anything), but I could be wrong.  I put it in.
> > > > Source/WebCore/css/StyleResolver.cpp:306
> > > > +void StyleResolver::addKeyframeStyle(RefPtr<StyleRuleKeyframes>&& rule)
> > > 
> > > Looks like this should be a Ref<>
> > What???  I don't think so.
> 
> Could you elaborate? The first thing the method does is dereference 'rule'
> so how could it ever be null? As such, I maintain this should be a Ref<>&&
> not a RefPtr<>&&.

I guess I'll just do in in Bug 157961 then.