Bug 65238 – [EFL] Introduce AssertMatchingEnums.cpp.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Ryuan Choi]

Patch will be added.

[Ryuan Choi]

Created attachment 102105 [details]
Patch

[Gyuyoung Kim]

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 ?

[Ryuan Choi]

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

[Gyuyoung Kim]

OK, LGTM.

[Ryuan Choi]

Created attachment 102125 [details]
typo_fixed

[Raphael Kubo da Costa (:rakuco)]

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

[Ryuan Choi]

Created attachment 102218 [details]
Patch

[Ryuan Choi]

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

[Raphael Kubo da Costa (:rakuco)]

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

[Ryuan Choi]

Created attachment 102349 [details]
Patch

[Ryuan Choi]

Comment on attachment 102349 [details]
Patch

Sorry, It's mistake.

[Ryuan Choi]

Created attachment 113821 [details]
Patch

[Ryuan Choi]

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

Rebased and remove more switch statements which is recently added.

[Raphael Kubo da Costa (:rakuco)]

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.

[Ryuan Choi]

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

[Filip Pizlo]

Comment on attachment 113821 [details]
Patch

Looks good, r=me

[WebKit Review Bot]

Comment on attachment 113821 [details]
Patch

Clearing flags on attachment: 113821

Committed r102297: <http://trac.webkit.org/changeset/102297>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.