Bug 129853

Created attachment 227179 [details] Patch

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.

Created attachment 227185 [details] Patch

(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.

Created attachment 227225 [details] Patch

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.

(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?

(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.

(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.

Created attachment 227292 [details] Patch

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

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 ?

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

Comment on attachment 227292 [details] Patch clearing flags. I landed after fixed boilerplate.