Bug 116601 – [EFL][WK2] Fix test_ewk2_context_menu

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

[Michal Pakula vel Rutka]

test_ewk2_context_menu is always passing, it does not test any context menu feature as it ends before context menu is called.

[Michal Pakula vel Rutka]

Created attachment 202988 [details]
proposed patch

[Michal Pakula vel Rutka]

Created attachment 204013 [details]
patch

[Grzegorz Czajkowski]

Comment on attachment 204013 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204013&action=review

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:227
> +bool EWK2UnitTestBase::waitUntilTrue(bool &flag, double timeoutSeconds)

flag doesn't need to be passed as a reference, the method doesn't change it.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:53
> +    bool waitUntilTrue(bool &flag, double timeoutSeconds = defaultTimeoutSeconds);

timeoutSeconds -> timeoutInSeconds ?
defaultTimeoutSeconds -> defaultTimeoutInSeconds ? ( I don't think the reviewers will be against of changing it even if this patch doesn't introduce it)

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context_menu.cpp:33
> +static const int customItemTag = EWK_CONTEXT_MENU_ITEM_BASE_APPLICATION_TAG;

It seems that it's always converted to Ewk_Context_Menu_Item_Action. Wouldn't be better to define it as Ewk_Context_Menu_Item_Action?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context_menu.cpp:76
> +    ewk_context_menu_item_title_set(item, "Copy Link");

It'd be nice to define all those item titles somewhere.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context_menu.cpp:88
> +

Could you add short note what the callback tests?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context_menu.cpp:120
> +

Ditto.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context_menu.cpp:161
>      return true;

return testFinished;

[Grzegorz Czajkowski]

Comment on attachment 204013 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204013&action=review

>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:227
>> +bool EWK2UnitTestBase::waitUntilTrue(bool &flag, double timeoutSeconds)
> 
> flag doesn't need to be passed as a reference, the method doesn't change it.

Please skip this comment. I missed that it's changed somewhere else.

[Michal Pakula vel Rutka]

Comment on attachment 204013 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204013&action=review

>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:53
>> +    bool waitUntilTrue(bool &flag, double timeoutSeconds = defaultTimeoutSeconds);
> 
> timeoutSeconds -> timeoutInSeconds ?
> defaultTimeoutSeconds -> defaultTimeoutInSeconds ? ( I don't think the reviewers will be against of changing it even if this patch doesn't introduce it)

defaultTimeoutSeconds and timeoutSeconds are used in several places in this file, I would rather change the names in separate patch

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context_menu.cpp:33
>> +static const int customItemTag = EWK_CONTEXT_MENU_ITEM_BASE_APPLICATION_TAG;
> 
> It seems that it's always converted to Ewk_Context_Menu_Item_Action. Wouldn't be better to define it as Ewk_Context_Menu_Item_Action?

done

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context_menu.cpp:76
>> +    ewk_context_menu_item_title_set(item, "Copy Link");
> 
> It'd be nice to define all those item titles somewhere.

done

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context_menu.cpp:88
>> +
> 
> Could you add short note what the callback tests?

done

[Michal Pakula vel Rutka]

Created attachment 204034 [details]
fixes after review

[Chris Dumez]

Comment on attachment 204034 [details]
fixes after review

View in context: https://bugs.webkit.org/attachment.cgi?id=204034&action=review

LGTM. I'll let Gyuyoung take a look.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:227
> +bool EWK2UnitTestBase::waitUntilTrue(bool &flag, double timeoutSeconds)

Nice addition.

[Gyuyoung Kim]

Comment on attachment 204034 [details]
fixes after review

Looks nice as well.

[WebKit Commit Bot]

Comment on attachment 204034 [details]
fixes after review

Clearing flags on attachment: 204034

Committed r151428: <http://trac.webkit.org/changeset/151428>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.