Bug 65238

Summary: [EFL] Introduce AssertMatchingEnums.cpp.
Product: WebKit Reporter: Ryuan Choi <ryuan.choi>
Component: WebKit EFLAssignee: Ryuan Choi <ryuan.choi>
Status: RESOLVED FIXED    
Severity: Normal CC: gyuyoung.kim, gyuyoung.kim, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
typo_fixed
none
Patch
none
Patch
none
Patch none

Description Ryuan Choi 2011-07-27 01:24:09 PDT
Patch will be added.
Comment 1 Ryuan Choi 2011-07-27 01:29:46 PDT
Created attachment 102105 [details]
Patch
Comment 2 Gyuyoung Kim 2011-07-27 01:58:56 PDT
Comment on attachment 102105 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_frame.h:153
>      EWK_TOUCH_POINT_MOVED,

Is it better to sort by alphabetic order ?
Comment 3 Ryuan Choi 2011-07-27 02:09:59 PDT
(In reply to comment #2)
> (From update of attachment 102105 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102105&action=review
> 
> > Source/WebKit/efl/ewk/ewk_frame.h:153
> >      EWK_TOUCH_POINT_MOVED,
> 
> Is it better to sort by alphabetic order ?

No, This patch is to keep same order between WebCore's enum values and WebKit's enum values.

If they are different, AssertMatchingEnums will complain it in compile time.
Comment 4 Gyuyoung Kim 2011-07-27 02:28:52 PDT
OK, LGTM.
Comment 5 Ryuan Choi 2011-07-27 04:49:21 PDT
Created attachment 102125 [details]
typo_fixed
Comment 6 Raphael Kubo da Costa (:rakuco) 2011-07-27 06:41:49 PDT
> Source/WebKit/efl/WebCoreSupport/AssertMatchingEnums.cpp:23
> +

Isn't it better to explicitly include <wtf/Assertions.h> here?

> Source/WebKit/efl/WebCoreSupport/AssertMatchingEnums.cpp:33
> +    COMPILE_ASSERT(int(webkit_name) == int(WebCore::webcore_name), mismatching_enums)

I prefer a C++ cast here.

> Source/WebKit/efl/ewk/ewk_frame.h:147
>  } Ewk_Touch_Event_Type;

It's probably a good idea to add a comment here mentioning that the order of the items should match the order of the equivalent structure in WebCore.

> Source/WebKit/efl/ewk/ewk_frame.h:157
>  } Ewk_Touch_Point_Type;

Ditto.
Comment 7 Ryuan Choi 2011-07-27 18:59:55 PDT
Created attachment 102218 [details]
Patch
Comment 8 Ryuan Choi 2011-07-27 19:03:36 PDT
(In reply to comment #6)
> > Source/WebKit/efl/WebCoreSupport/AssertMatchingEnums.cpp:23
> > +
> 
> Isn't it better to explicitly include <wtf/Assertions.h> here?
> 
Done.

> > Source/WebKit/efl/WebCoreSupport/AssertMatchingEnums.cpp:33
> > +    COMPILE_ASSERT(int(webkit_name) == int(WebCore::webcore_name), mismatching_enums)
> 
> I prefer a C++ cast here.
> 
Done.

> > Source/WebKit/efl/ewk/ewk_frame.h:147
> >  } Ewk_Touch_Event_Type;
> 
> It's probably a good idea to add a comment here mentioning that the order of the items should match the order of the equivalent structure in WebCore.
> 
> > Source/WebKit/efl/ewk/ewk_frame.h:157
> >  } Ewk_Touch_Point_Type;
> 
> Ditto.

I'm not sure.
I think that this comment make developers confused.
Comment 9 Raphael Kubo da Costa (:rakuco) 2011-07-28 05:37:30 PDT
> > > Source/WebKit/efl/ewk/ewk_frame.h:147
> > >  } Ewk_Touch_Event_Type;
> > 
> > It's probably a good idea to add a comment here mentioning that the order of the items should match the order of the equivalent structure in WebCore.
> > 
> > > Source/WebKit/efl/ewk/ewk_frame.h:157
> > >  } Ewk_Touch_Point_Type;
> > 
> > Ditto.
> 
> I'm not sure.
> I think that this comment make developers confused.

Coming to think of it again, the compilation error that will happen if the enum is changed should be enough indeed. r+ from my side.
Comment 10 Ryuan Choi 2011-07-29 05:25:32 PDT
Created attachment 102349 [details]
Patch
Comment 11 Ryuan Choi 2011-07-29 05:26:29 PDT
Comment on attachment 102349 [details]
Patch

Sorry, It's mistake.
Comment 12 Ryuan Choi 2011-11-06 20:36:46 PST
Created attachment 113821 [details]
Patch
Comment 13 Ryuan Choi 2011-11-06 20:37:44 PST
(In reply to comment #12)
> Created an attachment (id=113821) [details]
> Patch

Rebased and remove more switch statements which is recently added.
Comment 14 Raphael Kubo da Costa (:rakuco) 2011-12-06 07:37:50 PST
LGTM :)

Dunno if there are currently more enums which can benefit from this change, but it's up to you whether you want to do that in this patch or fix them later.
Comment 15 Ryuan Choi 2011-12-06 15:41:57 PST
(In reply to comment #14)
> LGTM :)
> 
> Dunno if there are currently more enums which can benefit from this change, but it's up to you whether you want to do that in this patch or fix them later.

Thanks for the comment.

I briefly searched ewk/ and this patch is still enough.
Comment 16 Filip Pizlo 2011-12-07 17:53:02 PST
Comment on attachment 113821 [details]
Patch

Looks good, r=me
Comment 17 WebKit Review Bot 2011-12-07 18:11:25 PST
Comment on attachment 113821 [details]
Patch

Clearing flags on attachment: 113821

Committed r102297: <http://trac.webkit.org/changeset/102297>
Comment 18 WebKit Review Bot 2011-12-07 18:11:31 PST
All reviewed patches have been landed.  Closing bug.