Summary: | [EFL][WK2] Fix test_ewk2_context_menu | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michal Pakula vel Rutka <mpakulavelrutka> | ||||||||
Component: | WebKit EFL | Assignee: | Michal Pakula vel Rutka <mpakulavelrutka> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cdumez, commit-queue, gyuyoung.kim, lucas.de.marchi, rakuco | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 116830, 117298 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Michal Pakula vel Rutka
2013-05-22 02:33:29 PDT
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. |