Bug 128156

Summary: [EFL][WK2] Stop using COMPILE_ASSERT_MATCHING_ENUM macros in EFL WebKit2
Product: WebKit Reporter: Jinwoo Song <jinwoo7.song>
Component: WebKit EFLAssignee: 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 Flags
Patch
none
Patch none

Description Jinwoo Song 2014-02-03 20:30:39 PST
SSIA.
Comment 1 Jinwoo Song 2014-02-03 20:33:00 PST
Created attachment 223067 [details]
Patch
Comment 2 Ryuan Choi 2014-02-03 20:46:56 PST
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 3 Ryuan Choi 2014-02-03 20:51:52 PST
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 4 Anders Carlsson 2014-02-03 22:20:35 PST
Comment on attachment 223067 [details]
Patch

Patch looks good. Feel free to land as is, or address Ryan's comments.
Comment 5 Jinwoo Song 2014-02-03 23:24:37 PST
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.
Comment 6 Jinwoo Song 2014-02-03 23:25:42 PST
(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.
Comment 7 Jinwoo Song 2014-02-03 23:31:10 PST
Created attachment 223075 [details]
Patch

Patch for landing after applying Ryuan's comments.
Comment 8 Ryuan Choi 2014-02-03 23:35:51 PST
(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 9 WebKit Commit Bot 2014-02-04 00:15:27 PST
Comment on attachment 223075 [details]
Patch

Clearing flags on attachment: 223075

Committed r163367: <http://trac.webkit.org/changeset/163367>
Comment 10 WebKit Commit Bot 2014-02-04 00:15:30 PST
All reviewed patches have been landed.  Closing bug.