Bug 129853 - Move to using std::unique_ptr for EFL objects.
Summary: Move to using std::unique_ptr for EFL objects.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hyowon Kim
URL:
Keywords:
Depends on: 129676
Blocks: 128007
  Show dependency treegraph
 
Reported: 2014-03-06 17:37 PST by Hyowon Kim
Modified: 2014-03-20 21:15 PDT (History)
11 users (show)

See Also:


Attachments
Patch (35.21 KB, patch)
2014-03-19 06:52 PDT, Hyowon Kim
no flags Details | Formatted Diff | Diff
Patch (35.28 KB, patch)
2014-03-19 08:41 PDT, Hyowon Kim
no flags Details | Formatted Diff | Diff
Patch (35.72 KB, patch)
2014-03-19 15:09 PDT, Hyowon Kim
no flags Details | Formatted Diff | Diff
Patch (36.06 KB, patch)
2014-03-20 08:33 PDT, Hyowon 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 Hyowon Kim 2014-03-06 17:37:42 PST
EFL objects are still using OwnedPtr.
See OwnPtrCommon.h and OwnPtrEfl.cpp.
Comment 1 Hyowon Kim 2014-03-19 06:52:53 PDT
Created attachment 227179 [details]
Patch
Comment 2 Anders Carlsson 2014-03-19 07:44:23 PDT
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.
Comment 3 Hyowon Kim 2014-03-19 08:41:52 PDT
Created attachment 227185 [details]
Patch
Comment 4 Hyowon Kim 2014-03-19 15:09:09 PDT
(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.
Comment 5 Hyowon Kim 2014-03-19 15:09:40 PDT
Created attachment 227225 [details]
Patch
Comment 6 Ryuan Choi 2014-03-19 18:29:57 PDT
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.
Comment 7 Hyowon Kim 2014-03-19 23:19:32 PDT
(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?
Comment 8 Ryuan Choi 2014-03-20 00:43:05 PDT
(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.
Comment 9 Hyowon Kim 2014-03-20 08:31:17 PDT
(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.
Comment 10 Hyowon Kim 2014-03-20 08:33:33 PDT
Created attachment 227292 [details]
Patch
Comment 11 Ryuan Choi 2014-03-20 16:29:54 PDT
(In reply to comment #10)
> Created an attachment (id=227292) [details]
> Patch

Looks fine to me.
Comment 12 Gyuyoung Kim 2014-03-20 17:02:44 PDT
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 ?
Comment 13 Ryuan Choi 2014-03-20 21:14:18 PDT
Committed r166039: <http://trac.webkit.org/changeset/166039>
Comment 14 Ryuan Choi 2014-03-20 21:15:00 PDT
Comment on attachment 227292 [details]
Patch

clearing flags.
I landed after fixed boilerplate.