Summary: | [EFL] Introduce AssertMatchingEnums.cpp. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryuan Choi <ryuan.choi> | ||||||||||||
Component: | WebKit EFL | Assignee: | 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
Ryuan Choi
2011-07-27 01:24:09 PDT
Created attachment 102105 [details]
Patch
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 ? (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. OK, LGTM. Created attachment 102125 [details]
typo_fixed
> 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. Created attachment 102218 [details]
Patch
(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. > > > 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.
Created attachment 102349 [details]
Patch
Comment on attachment 102349 [details]
Patch
Sorry, It's mistake.
Created attachment 113821 [details]
Patch
(In reply to comment #12) > Created an attachment (id=113821) [details] > Patch Rebased and remove more switch statements which is recently added. 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. (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 on attachment 113821 [details]
Patch
Looks good, r=me
Comment on attachment 113821 [details] Patch Clearing flags on attachment: 113821 Committed r102297: <http://trac.webkit.org/changeset/102297> All reviewed patches have been landed. Closing bug. |