Bug 144905 – Purge PassRefPtr create() factory functions in WebCore/css

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

[Joonghun Park]

Return Ref instead of PassRefPtr in create() factory functions in html, because the factory can't return null.

[Joonghun Park]

Created attachment 252962 [details]
<WIP>

[Joonghun Park]

Created attachment 253013 [details]
Patch

[Joonghun Park]

Created attachment 253015 [details]
Patch

[Joonghun Park]

Created attachment 253016 [details]
Patch

[Darin Adler]

Comment on attachment 253016 [details]
Patch

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

> Source/WebCore/css/CSSPrimitiveValue.h:394
> +    CSSPrimitiveValue::CSSPrimitiveValue(DashboardRegion*)

Need to remove the CSSPrimitiveValue:: prefix here.

[Joonghun Park]

(In reply to comment #5)
> Comment on attachment 253016 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253016&action=review
> 
> > Source/WebCore/css/CSSPrimitiveValue.h:394
> > +    CSSPrimitiveValue::CSSPrimitiveValue(DashboardRegion*)
> 
> Need to remove the CSSPrimitiveValue:: prefix here.

Oh, I missed it. Thank you for your comment :)

[Joonghun Park]

Created attachment 253019 [details]
Patch

[Joonghun Park]

Created attachment 253075 [details]
<WIP>

[Joonghun Park]

Created attachment 253090 [details]
Patch

[Joonghun Park]

Created attachment 253092 [details]
Patch

[Darin Adler]

Comment on attachment 253092 [details]
Patch

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

review- because the CSSPrimitiveValue part seems wrong; otherwise all looks fine

> Source/WebCore/css/BasicShapeFunctions.cpp:52
> +static RefPtr<CSSPrimitiveValue> basicShapeRadiusToCSSValue(const RenderStyle* style, CSSValuePool& pool, const BasicShapeRadius& radius)

Seems like this should return Ref. The failure case that currently returns null could and should be changed to return another value. Callers can’t handle null! Also seems that the style argument should be changed to a reference rather than a pointer.

> Source/WebCore/css/CSSPrimitiveValue.h:42
> +#if ENABLE(DASHBOARD_SUPPORT)
>  class DashboardRegion;
> +#endif

Please don’t do this. It’s OK to forward declare this even if it’s not used. If you did want to make it conditional we’d move it to a separate paragraph.

> Source/WebCore/css/CSSPrimitiveValue.h:393
> +        init(Ref<T>(*val));

What guarantees the pointer is non-null here? We can’t just do a * on a pointer that might be null.

> Source/WebCore/css/CSSPrimitiveValue.h:400
> +    CSSPrimitiveValue(CSSBasicShape*);
> +    CSSPrimitiveValue(CSSCalcValue*);
> +#if ENABLE(DASHBOARD_SUPPORT)
> +    CSSPrimitiveValue(DashboardRegion*);
> +#endif

Why are these needed? I think this is a bad idea.

> Source/WebCore/css/CSSPrimitiveValue.h:428
> +    void init(RefPtr<DashboardRegion>&&); // FIXME: Dashboard region should not be a primitive value.

Why RefPtr instead of Ref? Do callers really need to do this with null?

> Source/WebCore/css/CSSPrimitiveValue.h:431
> +    void init(RefPtr<CSSBasicShape>&&);
> +    void init(RefPtr<CSSCalcValue>&&);

Why RefPtr instead of Ref? Do callers really need to do this with null?

> Source/WebCore/css/CSSPrimitiveValue.h:457
> +#if ENABLE(DASHBOARD_SUPPORT)
>          DashboardRegion* region;
> +#endif

I don’t think this is an improvement. Lets go a little lighter on the #if here, unless you are sure we should do it. I think it does no harm to compile this on platforms that don’t support it.

> Source/WebCore/css/FontLoader.cpp:53
> +        return adoptRef<LoadFontCallback>(*new LoadFontCallback(numLoading, fontLoader, loadCallback, errorCallback));

I don’t understand the use of adoptRef<LoadFontCallback> here. Should just be adoptRef with no <> after it.

[Joonghun Park]

Created attachment 253822 [details]
<WIP> only first and last comment have been addressed in this patch

[Joonghun Park]

(In reply to comment #11)
> Comment on attachment 253092 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253092&action=review
> 
> review- because the CSSPrimitiveValue part seems wrong; otherwise all looks
> fine
> 
> > Source/WebCore/css/BasicShapeFunctions.cpp:52
> > +static RefPtr<CSSPrimitiveValue> basicShapeRadiusToCSSValue(const RenderStyle* style, CSSValuePool& pool, const BasicShapeRadius& radius)
> 
> Seems like this should return Ref. The failure case that currently returns
> null could and should be changed to return another value. Callers can’t
> handle null! Also seems that the style argument should be changed to a
> reference rather than a pointer.
> 
I changed this to Ref. And let default return be pool.createIdentifierValue(CSSValueClosestSide).
I'm not sure this is right decision.

> > Source/WebCore/css/CSSPrimitiveValue.h:42
> > +#if ENABLE(DASHBOARD_SUPPORT)
> >  class DashboardRegion;
> > +#endif
> 
> Please don’t do this. It’s OK to forward declare this even if it’s not used.
> If you did want to make it conditional we’d move it to a separate paragraph.
> 
I removed #if directive as you said because it is not necessary.

> > Source/WebCore/css/CSSPrimitiveValue.h:393
> > +        init(Ref<T>(*val));
> 
> What guarantees the pointer is non-null here? We can’t just do a * on a
> pointer that might be null.
> 
About this, I don't have a good idea. I just added a null check to this,
but it doesn't seem right. Do you have any advice?

> > Source/WebCore/css/CSSPrimitiveValue.h:400
> > +    CSSPrimitiveValue(CSSBasicShape*);
> > +    CSSPrimitiveValue(CSSCalcValue*);
> > +#if ENABLE(DASHBOARD_SUPPORT)
> > +    CSSPrimitiveValue(DashboardRegion*);
> > +#endif
> 
> Why are these needed? I think this is a bad idea.
> 
I removed other things except CSSPrimitveValue(CSSCalcValue*),
because without this one, 'error: call of overloaded ‘init(WTF::Ref<WebCore::CSSCalcValue>)’ is ambiguous' occurs.

> > Source/WebCore/css/CSSPrimitiveValue.h:428
> > +    void init(RefPtr<DashboardRegion>&&); // FIXME: Dashboard region should not be a primitive value.
> 
> Why RefPtr instead of Ref? Do callers really need to do this with null?
> 
I changed this one with Ref according to your comment.

> > Source/WebCore/css/CSSPrimitiveValue.h:431
> > +    void init(RefPtr<CSSBasicShape>&&);
> > +    void init(RefPtr<CSSCalcValue>&&);
> 
> Why RefPtr instead of Ref? Do callers really need to do this with null?
> 
I changed this one with Ref according to your comment.

> > Source/WebCore/css/CSSPrimitiveValue.h:457
> > +#if ENABLE(DASHBOARD_SUPPORT)
> >          DashboardRegion* region;
> > +#endif
> 
> I don’t think this is an improvement. Lets go a little lighter on the #if
> here, unless you are sure we should do it. I think it does no harm to
> compile this on platforms that don’t support it.
> 
As you said, I removed this unnecessary guard.

> > Source/WebCore/css/FontLoader.cpp:53
> > +        return adoptRef<LoadFontCallback>(*new LoadFontCallback(numLoading, fontLoader, loadCallback, errorCallback));
> 
> I don’t understand the use of adoptRef<LoadFontCallback> here. Should just
> be adoptRef with no <> after it.
I revised this one.

[Joonghun Park]

Created attachment 253839 [details]
Patch

[Joonghun Park]

Created attachment 253843 [details]
Patch

[Build Bot]

Comment on attachment 253843 [details]
Patch

Attachment 253843 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6173793483489280

New failing tests:
fast/css/large-value-csstext.html

[Build Bot]

Created attachment 253844 [details]
Archive of layout-test-results from ews100 for mac-mavericks

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

[Build Bot]

Comment on attachment 253843 [details]
Patch

Attachment 253843 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6116746251468800

New failing tests:
fast/css/large-value-csstext.html

[Build Bot]

Created attachment 253845 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

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

[Darin Adler]

Comment on attachment 253843 [details]
Patch

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

> Source/WebCore/css/BasicShapeFunctions.cpp:63
> -    ASSERT_NOT_REACHED();
> -    return 0;
> +    return pool.createIdentifierValue(CSSValueClosestSide);

Should not delete the ASSERT_NOT_REACHED.

> Source/WebCore/css/BasicShapeFunctions.cpp:74
> -        RefPtr<CSSBasicShapeCircle> circleValue = CSSBasicShapeCircle::create();
> +        Ref<CSSBasicShapeCircle> circleValue = CSSBasicShapeCircle::create();

Maybe auto instead of writing out the type?

> Source/WebCore/css/BasicShapeFunctions.cpp:84
> -        RefPtr<CSSBasicShapeEllipse> ellipseValue = CSSBasicShapeEllipse::create();
> +        Ref<CSSBasicShapeEllipse> ellipseValue = CSSBasicShapeEllipse::create();

Maybe auto instead of writing out the type?

> Source/WebCore/css/BasicShapeFunctions.cpp:95
> -        RefPtr<CSSBasicShapePolygon> polygonValue = CSSBasicShapePolygon::create();
> +        Ref<CSSBasicShapePolygon> polygonValue = CSSBasicShapePolygon::create();

Maybe auto instead of writing out the type?

> Source/WebCore/css/BasicShapeFunctions.cpp:126
> +    return pool.createValue(basicShapeValue.releaseNonNull());

This is wrong. If we hit the default case above then basicShapeValue is null. Really need to fix that.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1956
> +                return fillSizeToCSSValue(layers->size(), *style.get());

No need for the ".get()" here. Same for all the other sites below!

> Source/WebCore/css/CSSParser.cpp:5604
>  

Blank line here seems out of place (between declaration and initialization of lineNames).

> Source/WebCore/css/CSSParser.cpp:5605
> +    if (previousNamedAreaTrailingLineNames != nullptr)

WebKit coding style says you just do:

    if (previousNamedAreaTrailingLineNames)

> Source/WebCore/css/CSSPrimitiveValue.cpp:240
> +CSSPrimitiveValue::CSSPrimitiveValue(CSSCalcValue* val)

Please use the name "value", not "val".

> Source/WebCore/css/CSSPrimitiveValue.cpp:316
> +        RefPtr<CSSCalcValue> calcValue = CSSCalcValue::create(length.calculationValue(), style);
> +        init(WTF::move(calcValue));

This would read better without the local variable and you could remove the braces too.

> Source/WebCore/css/CSSPrimitiveValue.h:392
> +        if (!val)
> +            init(Ref<T>(*val));

This looks like it will break everything; I bet it’s why tests are failing! I think you mean if (val). But also is this behavior OK for null pointers.

> Source/WebCore/css/CSSPrimitiveValue.h:395
> +    CSSPrimitiveValue(CSSCalcValue*);

Pretty messy. Maybe we can change this at the call site to use Ref or RefPtr rather than a raw pointer.

[Joonghun Park]

(In reply to comment #20)
> Comment on attachment 253843 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253843&action=review
> 
> > Source/WebCore/css/BasicShapeFunctions.cpp:63
> > -    ASSERT_NOT_REACHED();
> > -    return 0;
> > +    return pool.createIdentifierValue(CSSValueClosestSide);
> 
> Should not delete the ASSERT_NOT_REACHED.
> 
I readded ASSERT_NOT_REACHED here.

> > Source/WebCore/css/BasicShapeFunctions.cpp:74
> > -        RefPtr<CSSBasicShapeCircle> circleValue = CSSBasicShapeCircle::create();
> > +        Ref<CSSBasicShapeCircle> circleValue = CSSBasicShapeCircle::create();
> 
> Maybe auto instead of writing out the type?
> 
I changed the type as auto instead.

> > Source/WebCore/css/BasicShapeFunctions.cpp:84
> > -        RefPtr<CSSBasicShapeEllipse> ellipseValue = CSSBasicShapeEllipse::create();
> > +        Ref<CSSBasicShapeEllipse> ellipseValue = CSSBasicShapeEllipse::create();
> 
> Maybe auto instead of writing out the type?
> 
ditto.

> > Source/WebCore/css/BasicShapeFunctions.cpp:95
> > -        RefPtr<CSSBasicShapePolygon> polygonValue = CSSBasicShapePolygon::create();
> > +        Ref<CSSBasicShapePolygon> polygonValue = CSSBasicShapePolygon::create();
> 
> Maybe auto instead of writing out the type?
> 
ditto.

> > Source/WebCore/css/BasicShapeFunctions.cpp:126
> > +    return pool.createValue(basicShapeValue.releaseNonNull());
> 
> This is wrong. If we hit the default case above then basicShapeValue is
> null. Really need to fix that.
> 
It seems that Ref<BasicShape> basicShapeForValue(); function is implemented as same as this one because CSSBasicShape is abstract class and type() is pure virtual.
This is the same for BasicShape class too.

For readability it could be an option to add ASSERT_NOT_REACHED in default in both basicShapeForValue() and valueForBasicShape().
But by releaseNonNull(), null check assert is already being done, so it doesn't
seems to be necessary.

> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1956
> > +                return fillSizeToCSSValue(layers->size(), *style.get());
> 
> No need for the ".get()" here. Same for all the other sites below!
> 
Ah, truly it is :) Thank you for your pointing out.

> > Source/WebCore/css/CSSParser.cpp:5604
> >  
> 
> Blank line here seems out of place (between declaration and initialization
> of lineNames).
> 
I removed this unnecessary blank line.

> > Source/WebCore/css/CSSParser.cpp:5605
> > +    if (previousNamedAreaTrailingLineNames != nullptr)
> 
> WebKit coding style says you just do:
> 
>     if (previousNamedAreaTrailingLineNames)
> 
I revised this one as you commented.

> > Source/WebCore/css/CSSPrimitiveValue.cpp:240
> > +CSSPrimitiveValue::CSSPrimitiveValue(CSSCalcValue* val)
> 
> Please use the name "value", not "val".
> 
I removed ctor CSSPrimitiveValue(T* value) for now,
because it is being used no more, and I was not sure just adding if (val) is right behavior, as you commented also.

> > Source/WebCore/css/CSSPrimitiveValue.cpp:316
> > +        RefPtr<CSSCalcValue> calcValue = CSSCalcValue::create(length.calculationValue(), style);
> > +        init(WTF::move(calcValue));
> 
> This would read better without the local variable and you could remove the
> braces too.
> 
I revised this one as you commented.

> > Source/WebCore/css/CSSPrimitiveValue.h:392
> > +        if (!val)
> > +            init(Ref<T>(*val));
> 
> This looks like it will break everything; I bet it’s why tests are failing!
> I think you mean if (val). But also is this behavior OK for null pointers.
> 
For now I removed this ctor as I wrote above.

> > Source/WebCore/css/CSSPrimitiveValue.h:395
> > +    CSSPrimitiveValue(CSSCalcValue*);
> 
> Pretty messy. Maybe we can change this at the call site to use Ref or RefPtr
> rather than a raw pointer.
I revised the callsites to use Ref or RefPtr instead of raw pointers.

[Joonghun Park]

Created attachment 253971 [details]
Patch

[Darin Adler]

Comment on attachment 253971 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2744
> -            return cssValuePool().createValue(firstRegion.release());
> +            return cssValuePool().createValue(firstRegion.releaseNonNull());

I don’t see a guarantee here that there is a firstRegion. Can’t count be zero?

I’m talking about a runtime problem here, not an assertion. Naturally releaseNonNull will assert, but I am talking about in production without assertions present to inform us.

> Source/WebCore/css/CSSPrimitiveValue.h:418
> +    void init(RefPtr<CSSCalcValue>&&);

Need to explain why this one is a RefPtr instead of a Ref.

[Joonghun Park]

(In reply to comment #23)
> Comment on attachment 253971 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253971&action=review
> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2744
> > -            return cssValuePool().createValue(firstRegion.release());
> > +            return cssValuePool().createValue(firstRegion.releaseNonNull());
> 
> I don’t see a guarantee here that there is a firstRegion. Can’t count be
> zero?
> 

As you pointing out, it seems that if getPropertyValue is called when dashboardRegion was not being set, count can be zero.
So I revised init parameter type to RefPtr<DashboardRegion>&&.

> I’m talking about a runtime problem here, not an assertion. Naturally
> releaseNonNull will assert, but I am talking about in production without
> assertions present to inform us.
> 
> > Source/WebCore/css/CSSPrimitiveValue.h:418
> > +    void init(RefPtr<CSSCalcValue>&&);
> 
> Need to explain why this one is a RefPtr instead of a Ref.

In CSSCalculationValue.h, the two CSSCalcValue::create()'s return type is
RefPtr because they can return nullptr, 
and init(CSSCalcValue::create(length.calculationValue(), style)); is called 
in CSSPrimitiveValue::CSSPrimitiveValue(const Length&, const RenderStyle&)
for example.
So I used RefPtr for CSSCalcValue.

[Joonghun Park]

Created attachment 254104 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 254104 [details]
Patch

Clearing flags on attachment: 254104

Committed r185238: <http://trac.webkit.org/changeset/185238>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.