Summary: | [EFL][WK2] Stop using COMPILE_ASSERT_MATCHING_ENUM macros in EFL WebKit2 | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jinwoo Song <jinwoo7.song> | ||||||
Component: | WebKit EFL | Assignee: | Jinwoo Song <jinwoo7.song> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bunhere, cdumez, commit-queue, gyuyoung.kim, lucas.de.marchi, ryuan.choi | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Jinwoo Song
2014-02-03 20:30:39 PST
Created attachment 223067 [details]
Patch
Comment on attachment 223067 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=223067&action=review Thanks. I commented bit. > Source/WebKit2/UIProcess/API/efl/ewk_navigation_policy_decision_private.h:69 > + Ewk_Navigation_Type toEwkNavigationType(WKFrameNavigationType navigationType) const; > + Event_Mouse_Button toEventMouseButton(WKEventMouseButton mouseButton) const; > + Event_Modifier_Keys toEventModifierKeys(WKEventModifiers modifiers) const; I am not sure why we should add these as methods? Can we make them as static inline function? At least, they can be static methods. > Source/WebKit2/UIProcess/API/efl/ewk_private.h:29 > #include <wtf/Assertions.h> Does we need to keep this? > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:399 > +inline WKFindOptions toWKFindOptions(Ewk_Find_Options options) > +{ > + unsigned wkFindOptions = 0; > + > + if (options & EWK_FIND_OPTIONS_CASE_INSENSITIVE) > + wkFindOptions |= kWKFindOptionsCaseInsensitive; > + if (options & EWK_FIND_OPTIONS_AT_WORD_STARTS) > + wkFindOptions |= kWKFindOptionsAtWordStarts; > + if (options & EWK_FIND_OPTIONS_TREAT_MEDIAL_CAPITAL_AS_WORD_START) > + wkFindOptions |= kWKFindOptionsTreatMedialCapitalAsWordStart; > + if (options & EWK_FIND_OPTIONS_BACKWARDS) > + wkFindOptions |= kWKFindOptionsBackwards; > + if (options & EWK_FIND_OPTIONS_WRAP_AROUND) > + wkFindOptions |= kWKFindOptionsWrapAround; > + if (options & EWK_FIND_OPTIONS_SHOW_OVERLAY) > + wkFindOptions |= kWKFindOptionsShowOverlay; > + if (options & EWK_FIND_OPTIONS_SHOW_FIND_INDICATOR) > + wkFindOptions |= kWKFindOptionsShowFindIndicator; > + if (options & EWK_FIND_OPTIONS_SHOW_HIGHLIGHT) > + wkFindOptions |= kWKFindOptionsShowHighlight; > + > + return static_cast<WKFindOptions>(wkFindOptions); > +} I didn't check whether it is correct. but it looks changed the logic, right? > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:529 > + switch (mode) { > + case EWK_PAGINATION_MODE_INVALID: > + break; Should we keep EWK_PAGINATION_MODE_INVALID if it is not in kWK's? Comment on attachment 223067 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=223067&action=review > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:215 > + return kWKCacheModelPrimaryWebBrowser; > + } > + ASSERT_NOT_REACHED(); > + > + return kWKCacheModelDocumentViewer; > +} I don't know whether we have rules. But, you did it as two ways in a patch. any reason? > Source/WebKit2/UIProcess/API/efl/ewk_popup_menu_item.cpp:47 > + default: > + ASSERT_NOT_REACHED(); > + return EWK_POPUP_MENU_ITEM; > + } here Comment on attachment 223067 [details]
Patch
Patch looks good. Feel free to land as is, or address Ryan's comments.
Comment on attachment 223067 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=223067&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:215 >> +} > > I don't know whether we have rules. > > But, you did it as two ways in a patch. any reason? As there is no concrete rules as I know, I added functions when there are more than three or four enum values. >> Source/WebKit2/UIProcess/API/efl/ewk_navigation_policy_decision_private.h:69 >> + Event_Modifier_Keys toEventModifierKeys(WKEventModifiers modifiers) const; > > I am not sure why we should add these as methods? > > Can we make them as static inline function? > > At least, they can be static methods. Exactly. I'll fix it. >> Source/WebKit2/UIProcess/API/efl/ewk_popup_menu_item.cpp:47 >> + } > > here I'll fix it. >> Source/WebKit2/UIProcess/API/efl/ewk_private.h:29 >> #include <wtf/Assertions.h> > > Does we need to keep this? No, I'll remove this line. >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:399 >> +} > > I didn't check whether it is correct. but it looks changed the logic, right? As several find options may be mixed, we have to do '|' operation. In previous code, static_cast converts the Ewk_Find_Options to WKFindOptions and in WKSahredAPICast, find options are mixed similarly like this. >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:529 >> + break; > > Should we keep EWK_PAGINATION_MODE_INVALID if it is not in kWK's? Because of [-Werror=switch] error, we have to keep this value in switch. (In reply to comment #4) > (From update of attachment 223067 [details]) > Patch looks good. Feel free to land as is, or address Ryan's comments. Thanks for review, Andersca! I'll land a patch with Ryuan's comment. Created attachment 223075 [details]
Patch
Patch for landing after applying Ryuan's comments.
(In reply to comment #7) > Created an attachment (id=223075) [details] > Patch > > Patch for landing after applying Ryuan's comments. Looks fine to me. :) Comment on attachment 223075 [details] Patch Clearing flags on attachment: 223075 Committed r163367: <http://trac.webkit.org/changeset/163367> All reviewed patches have been landed. Closing bug. |