Bug 129853 – Move to using std::unique_ptr for EFL objects.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Hyowon Kim]

EFL objects are still using OwnedPtr.
See OwnPtrCommon.h and OwnPtrEfl.cpp.

[Hyowon Kim]

Created attachment 227179 [details]
Patch

[Anders Carlsson]

Comment on attachment 227179 [details]
Patch

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

> Source/WTF/wtf/efl/UniquePtrEfl.h:35
> +    void operator()(T* ptr) const { ASSERT_NOT_REACHED(); }

I think  you can mark this = delete; instead to get a compile error instead of a runtime error.

> Source/WebCore/platform/graphics/efl/EvasGLContext.h:44
> +        return std::unique_ptr<EvasGLContext>(new EvasGLContext(evasGL, context));

This can use std::make_unique.

> Source/WebCore/platform/graphics/efl/EvasGLSurface.h:53
> +        return std::unique_ptr<EvasGLSurface>(new EvasGLSurface(evasGL, surface));

std::make_unique.

> Tools/DumpRenderTree/efl/DumpRenderTree.cpp:474
> +    ecoreEvas.reset();

= nullptr;

> Tools/ImageDiff/efl/ImageDiff.cpp:363
> +    gEcoreEvas.reset(); // Make sure ecore_evas_free is called before the EFL are shut down

= nullptr.

[Hyowon Kim]

Created attachment 227185 [details]
Patch

[Hyowon Kim]

(In reply to comment #2)
> (From update of attachment 227179 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227179&action=review
> 
> > Source/WTF/wtf/efl/UniquePtrEfl.h:35
> > +    void operator()(T* ptr) const { ASSERT_NOT_REACHED(); }
> 
> I think  you can mark this = delete; instead to get a compile error instead of a runtime error.

Done.

> > Source/WebCore/platform/graphics/efl/EvasGLContext.h:44
> > +        return std::unique_ptr<EvasGLContext>(new EvasGLContext(evasGL, context));
> 
> This can use std::make_unique.

Done.

> > Source/WebCore/platform/graphics/efl/EvasGLSurface.h:53
> > +        return std::unique_ptr<EvasGLSurface>(new EvasGLSurface(evasGL, surface));
> 
> std::make_unique.

Done.

> > Tools/DumpRenderTree/efl/DumpRenderTree.cpp:474
> > +    ecoreEvas.reset();
> 
> = nullptr;

Done.
 
> > Tools/ImageDiff/efl/ImageDiff.cpp:363
> > +    gEcoreEvas.reset(); // Make sure ecore_evas_free is called before the EFL are shut down
> 
> = nullptr.

Done.

Thanks for the review.

[Hyowon Kim]

Created attachment 227225 [details]
Patch

[Ryuan Choi]

Comment on attachment 227225 [details]
Patch

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

thanks.
I commented little for EFL side.

> Source/WebCore/platform/graphics/Icon.h:41
> +#elif PLATFORM(EFL)
> +typedef struct _Evas_Object Evas_Object;

Pleased add like below for EFL 1.8 and latest versions.

#if USE(EO)
typedef struct _Eo_Opaque Evas_Object;
#else
typedef struct _Evas_Object Evas_Object;
#endif

> Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.h:32
> +typedef struct _Evas_Object Evas_Object;

Let's just include <Evas.h> here.

> Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.h:38
> +typedef struct _Evas_Object Evas_Object;

Let's just include <Evas.h> here.

> Source/WebKit/efl/WebCoreSupport/EditorClientEfl.h:45
> +typedef struct _Evas_Object Evas_Object;

Let's just include <Evas.h> here.

> Source/WebKit/efl/WebCoreSupport/FrameNetworkingContextEfl.h:34
> +typedef struct _Evas_Object Evas_Object;

Let's just include <Evas.h> here.

> Source/WebKit/efl/WebCoreSupport/NavigatorContentUtilsClientEfl.h:36
> +typedef struct _Evas_Object Evas_Object;

Let's just include <Evas.h> here.

[Hyowon Kim]

(In reply to comment #6)
> (From update of attachment 227225 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227225&action=review
> 
> thanks.
> I commented little for EFL side.
> 
> > Source/WebCore/platform/graphics/Icon.h:41
> > +#elif PLATFORM(EFL)
> > +typedef struct _Evas_Object Evas_Object;
> 
> Pleased add like below for EFL 1.8 and latest versions.
> 
> #if USE(EO)
> typedef struct _Eo_Opaque Evas_Object;
> #else
> typedef struct _Evas_Object Evas_Object;
> #endif
> 
> > Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.h:32
> > +typedef struct _Evas_Object Evas_Object;
> 
> Let's just include <Evas.h> here.

I guess the reason you suggest replacing the forward declartion of Evas_Object with the Evas.h inclusion is because the forward declaration of Evas_Object for EFL 1.8 or later version is too long. So it looks not cool. right?

[Ryuan Choi]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 227225 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=227225&action=review
> > 
> > thanks.
> > I commented little for EFL side.
> > 
> > > Source/WebCore/platform/graphics/Icon.h:41
> > > +#elif PLATFORM(EFL)
> > > +typedef struct _Evas_Object Evas_Object;
> > 
> > Pleased add like below for EFL 1.8 and latest versions.
> > 
> > #if USE(EO)
> > typedef struct _Eo_Opaque Evas_Object;
> > #else
> > typedef struct _Evas_Object Evas_Object;
> > #endif
> > 
> > > Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.h:32
> > > +typedef struct _Evas_Object Evas_Object;
> > 
> > Let's just include <Evas.h> here.
> 
> I guess the reason you suggest replacing the forward declartion of Evas_Object with the Evas.h inclusion is because the forward declaration of Evas_Object for EFL 1.8 or later version is too long. So it looks not cool. right?

Yes, because EFL changes declaration since 1.8, we should include Evas.h or use some tricks which I mentioned above.

In fact, I don't mind among the options.
Just I grepped that some header files in efl/ already include Evas.h instead of declarations.

[Hyowon Kim]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (From update of attachment 227225 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=227225&action=review
> > > 
> > > thanks.
> > > I commented little for EFL side.
> > > 
> > > > Source/WebCore/platform/graphics/Icon.h:41
> > > > +#elif PLATFORM(EFL)
> > > > +typedef struct _Evas_Object Evas_Object;
> > > 
> > > Pleased add like below for EFL 1.8 and latest versions.
> > > 
> > > #if USE(EO)
> > > typedef struct _Eo_Opaque Evas_Object;
> > > #else
> > > typedef struct _Evas_Object Evas_Object;
> > > #endif
> > > 
> > > > Source/WebKit/efl/WebCoreSupport/BatteryClientEfl.h:32
> > > > +typedef struct _Evas_Object Evas_Object;
> > > 
> > > Let's just include <Evas.h> here.
> > 
> > I guess the reason you suggest replacing the forward declartion of Evas_Object with the Evas.h inclusion is because the forward declaration of Evas_Object for EFL 1.8 or later version is too long. So it looks not cool. right?
> 
> Yes, because EFL changes declaration since 1.8, we should include Evas.h or use some tricks which I mentioned above.
> 
> In fact, I don't mind among the options.
> Just I grepped that some header files in efl/ already include Evas.h instead of declarations.

I think we should avoid unnecessary header file inclusion.
So I've just modified typedefs on this patch.
But, I've also filed a new bug 130511 to clean up messy typedefs.
I will do that work after this bug is closed.

[Hyowon Kim]

Created attachment 227292 [details]
Patch

[Ryuan Choi]

(In reply to comment #10)
> Created an attachment (id=227292) [details]
> Patch

Looks fine to me.

[Gyuyoung Kim]

Comment on attachment 227292 [details]
Patch

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

Andersca already set r+ed. Anyway, LGTM. r+ again.

> Source/WTF/wtf/efl/UniquePtrEfl.h:2
> +    Copyright (C) 2014 Samsung Electronics

We prefer to use BSD rather than LGPL. Can't we use BSD ?

[Ryuan Choi]

Committed r166039: <http://trac.webkit.org/changeset/166039>

[Ryuan Choi]

Comment on attachment 227292 [details]
Patch

clearing flags.
I landed after fixed boilerplate.