Bug 128156 – [EFL][WK2] Stop using COMPILE_ASSERT_MATCHING_ENUM macros in EFL WebKit2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Jinwoo Song]

SSIA.

[Jinwoo Song]

Created attachment 223067 [details]
Patch

[Ryuan Choi]

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?

[Ryuan Choi]

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

[Anders Carlsson]

Comment on attachment 223067 [details]
Patch

Patch looks good. Feel free to land as is, or address Ryan's comments.

[Jinwoo Song]

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.

[Jinwoo Song]

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

[Jinwoo Song]

Created attachment 223075 [details]
Patch

Patch for landing after applying Ryuan's comments.

[Ryuan Choi]

(In reply to comment #7)
> Created an attachment (id=223075) [details]
> Patch
> 
> Patch for landing after applying Ryuan's comments.

Looks fine to me. :)

[WebKit Commit Bot]

Comment on attachment 223075 [details]
Patch

Clearing flags on attachment: 223075

Committed r163367: <http://trac.webkit.org/changeset/163367>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.