test_ewk2_context_menu is always passing, it does not test any context menu feature as it ends before context menu is called.
Created attachment 202988 [details] proposed patch
Created attachment 204013 [details] patch
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;
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.
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
Created attachment 204034 [details] fixes after review
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.
Comment on attachment 204034 [details] fixes after review Looks nice as well.
Comment on attachment 204034 [details] fixes after review Clearing flags on attachment: 204034 Committed r151428: <http://trac.webkit.org/changeset/151428>
All reviewed patches have been landed. Closing bug.