Clean up CSS code
Created attachment 279158 [details] Patch
Created attachment 279171 [details] Patch
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.
Created attachment 279177 [details] Patch
Created attachment 279182 [details] Patch
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.
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
Created attachment 279220 [details] Patch
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.
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
Created attachment 279227 [details] Patch
Created attachment 279257 [details] Patch
Created attachment 279260 [details] Patch
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<>?
Created attachment 279279 [details] Patch
Created attachment 279281 [details] Patch
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.
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
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.
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
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.
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
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.
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
Created attachment 279303 [details] Patch
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<>
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.
(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<>&&.
(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.