132939 2014-05-14 20:44:23 -0700 [WebKit2-EFL] Fix safety check failed errors in API tests 2014-05-29 21:03:08 -0700 1 1 1 Unclassified WebKit WebKit EFL 528+ (Nightly build) Unspecified Unspecified RESOLVED REMIND P2 Normal --- 132813 1 shiva.jm webkit-unassigned bunhere cdumez commit-queue gyuyoung.kim gyuyoung.kim lucas.de.marchi ryuan.choi sergio oldest_to_newest 1009423 0 shiva.jm 2014-05-14 20:44:23 -0700 Fix erros in below API tests, ewk_context_additional_plugin_path_set(), ewk_view_contents_size_get(), ewk_context_menu_item_select(), ewk_view_script_execute(). [ RUN ] EWK2ContextTest.ewk_context_additional_plugin_path_set ERR<9873>: /home/shiva.jm/webkit-git/svngitmay14/WebKit/Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:490 ewk_context_additional_plugin_path_set() safety check failed: path == NULL [ OK ] EWK2ContextTest.ewk_context_additional_plugin_path_set (131 ms) [----------] 12 tests from EWK2ContextTest (11581 ms total) [ RUN ] EWK2ViewTest.ewk_view_contents_size_get ERR<11319>: /home/shiva.jm/webkit-git/svngitmay14/WebKit/Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:54 toEwkViewChecked() safety check failed: evasObject == NULL CRI<11319>: /home/shiva.jm/webkit-git/svngitmay14/WebKit/Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:763 ewk_view_contents_size_get() no private data for object (nil) [ OK ] EWK2ViewTest.ewk_view_contents_size_get (790 ms) [----------] 36 tests from EWK2ViewTest (29847 ms total) [----------] 1 test from EWK2UnitTestBase [ RUN ] EWK2UnitTestBase.ewk_view_script_execute ERR<11319>: /home/shiva.jm/webkit-git/svngitmay14/WebKit/Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:692 ewk_view_script_execute() safety check failed: script == NULL [ OK ] EWK2UnitTestBase.ewk_view_script_execute (675 ms) Expected: true ERR<11033>: /home/shiva.jm/webkit-git/svngitmay14/WebKit/Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:158 ewk_context_menu_item_select() safety check failed: item == NULL /home/shiva.jm/webkit-git/svngitmay14/WebKit/Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:167: Failure Value of: text Actual: "aa" 1009497 1 231498 shiva.jm 2014-05-15 04:30:14 -0700 Created attachment 231498 Patch ewk_view_contents_size_get unit test has tested unsupported evas object type. But ewk APIs gives critical error, when given evas object is not ewk type. So, we can't test wrong evas object type by using current test. so Removing the test line. 1009666 2 231498 gyuyoung.kim 2014-05-15 17:13:11 -0700 Comment on attachment 231498 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231498&action=review > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:-1146 > - ewk_view_contents_size_get(0, &contentsWidth, &contentsHeight); I think this is to check if width and height can be initialized with 0 when Evas_Object is null. To be honest, I don't know why we have to initialize width/height when evas object is null. Eina_Bool ewk_view_contents_size_get(const Evas_Object* ewkView, Evas_Coord* width, Evas_Coord* height) { EwkView* impl = toEwkViewChecked(ewkView); if (EINA_UNLIKELY(!impl)) { EINA_LOG_CRIT("no private data for object %p", ewkView); if (width) *width = 0; if (height) *height = 0; return false; } If we remove this test logic, I think we have to remove the initialization code together. 1009672 3 231498 ryuan.choi 2014-05-15 17:35:32 -0700 Comment on attachment 231498 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231498&action=review > Source/WebKit2/ChangeLog:9 > + ewk_view_contents_size_get unit test has tested unsupported evas object type. But ewk APIs gives critical error, when given evas object > + is not ewk type. So, we can't test wrong evas object type by using current test. so Removing the test line. I am not sure what is the problem. It is intentionally tested. >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:-1146 >> - ewk_view_contents_size_get(0, &contentsWidth, &contentsHeight); > > I think this is to check if width and height can be initialized with 0 when Evas_Object is null. To be honest, I don't know why we have to initialize width/height when evas object is null. > > Eina_Bool ewk_view_contents_size_get(const Evas_Object* ewkView, Evas_Coord* width, Evas_Coord* height) > { > EwkView* impl = toEwkViewChecked(ewkView); > if (EINA_UNLIKELY(!impl)) { > EINA_LOG_CRIT("no private data for object %p", ewkView); > if (width) > *width = 0; > if (height) > *height = 0; > > return false; > } > > If we remove this test logic, I think we have to remove the initialization code together. Maybe, I don't have objection about removing the logic that clears the value. But I still want test case although we remove this logic. This test case explains the behavior of ewebkit well when wrong ewkView is given. 1009673 4 ryuan.choi 2014-05-15 17:37:54 -0700 If you want clear log, it looks better to turn off the EINA_LOG for our test cases. Test cases should cover not only positive cases but also negative cases. 1009674 5 gyuyoung.kim 2014-05-15 17:40:55 -0700 (In reply to comment #4) > If you want clear log, it looks better to turn off the EINA_LOG for our test cases. > > Test cases should cover not only positive cases but also negative cases. How about turning on EINA_LOG only when debug mode ? Personally I hope to see the message on debug mode. ;) 1012564 6 shiva.jm 2014-05-29 21:03:08 -0700 I would like to postpone to fix this problem later, since to remove error logs for negative test, we may need to seperate negative tests or enable error logs in debug mode only 231498 2014-05-15 04:30:14 -0700 2014-05-15 17:35:32 -0700 Patch bug-132939.patch text/plain 1649 shiva.jm SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDE2ODg5NikKKysrIFNvdXJjZS9XZWJLaXQyL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDE0LTA1LTE1ICBTaGl2YWt1 bWFyIEpNICA8c2hpdmEuam1Ac2Ftc3VuZy5jb20+CisKKyAgICAgICAgW1dlYktpdDItRUZMXSBG aXggc2FmZXR5IGNoZWNrIGZhaWxlZCBlcnJvcnMgaW4gQVBJIHRlc3RzCisgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMzI5MzkKKworICAgICAgICBSZXZp ZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBld2tfdmlld19jb250ZW50c19zaXpl X2dldCB1bml0IHRlc3QgaGFzIHRlc3RlZCB1bnN1cHBvcnRlZCBldmFzIG9iamVjdCB0eXBlLiBC dXQgZXdrIEFQSXMgZ2l2ZXMgY3JpdGljYWwgZXJyb3IsIHdoZW4gZ2l2ZW4gZXZhcyBvYmplY3QK KyAgICAgICAgaXMgbm90IGV3ayB0eXBlLiBTbywgd2UgY2FuJ3QgdGVzdCB3cm9uZyBldmFzIG9i amVjdCB0eXBlIGJ5IHVzaW5nIGN1cnJlbnQgdGVzdC4gc28gUmVtb3ZpbmcgdGhlIHRlc3QgbGlu ZS4KKworICAgICAgICAqIFVJUHJvY2Vzcy9BUEkvZWZsL3Rlc3RzL3Rlc3RfZXdrMl92aWV3LmNw cDoKKyAgICAgICAgKFRFU1RfRik6CisKIDIwMTQtMDUtMTUgIFRpbSBIb3J0b24gIDx0aW1vdGh5 X2hvcnRvbkBhcHBsZS5jb20+CiAKICAgICAgICAgW2lPU10gSW5jcmVtZW50YWwgdXBkYXRlcyBk dXJpbmcgc2Nyb2xsLXRvLXRvcCBzaG91bGRu4oCZdCBjb3VudCBhcyDigJxzdGFibGXigJ0gdXBk YXRlcwpJbmRleDogU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL0FQSS9lZmwvdGVzdHMvdGVzdF9l d2syX3ZpZXcuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkv ZWZsL3Rlc3RzL3Rlc3RfZXdrMl92aWV3LmNwcAkocmV2aXNpb24gMTY4ODk2KQorKysgU291cmNl L1dlYktpdDIvVUlQcm9jZXNzL0FQSS9lZmwvdGVzdHMvdGVzdF9ld2syX3ZpZXcuY3BwCSh3b3Jr aW5nIGNvcHkpCkBAIC0xMTQzLDEwICsxMTQzLDYgQEAgVEVTVF9GKEVXSzJWaWV3VGVzdCwgZXdr X3ZpZXdfcGFnZV96b29tXwogVEVTVF9GKEVXSzJWaWV3VGVzdCwgZXdrX3ZpZXdfY29udGVudHNf c2l6ZV9nZXQpCiB7CiAgICAgaW50IGNvbnRlbnRzV2lkdGgsIGNvbnRlbnRzSGVpZ2h0OwotICAg IGV3a192aWV3X2NvbnRlbnRzX3NpemVfZ2V0KDAsICZjb250ZW50c1dpZHRoLCAmY29udGVudHNI ZWlnaHQpOwotCi0gICAgRVhQRUNUX0VRKDAsIGNvbnRlbnRzV2lkdGgpOwotICAgIEVYUEVDVF9F USgwLCBjb250ZW50c0hlaWdodCk7CiAKICAgICBBU1NFUlRfVFJVRShsb2FkVXJsU3luYyhlbnZp cm9ubWVudC0+ZGVmYXVsdFRlc3RQYWdlVXJsKCkpKTsKICAgICBld2tfdmlld19jb250ZW50c19z aXplX2dldCh3ZWJWaWV3KCksICZjb250ZW50c1dpZHRoLCAmY29udGVudHNIZWlnaHQpOwo=