110209 2013-02-19 04:22:51 -0800 [EFL][WK2] ewk_popup_menu_close() does not work as intended 2013-03-12 20:49:16 -0700 1 1 1 Unclassified WebKit WebKit EFL 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 108934 111543 1 cdumez cdumez ap benjamin gyuyoung.kim kenneth laszlo.gombos lucas.de.marchi mikhail.pozdnyakov rakuco webkit.review.bot oldest_to_newest 836221 0 cdumez 2013-02-19 04:22:51 -0800 ewk_popup_menu_close() is called by the browser to notify WebKit that the popup menu was closed. However, the current implementation calls a smart function asking the browser to hide the popup menu instead of passing the information along to the WebProcess. 836232 1 189055 cdumez 2013-02-19 04:32:58 -0800 Created attachment 189055 Patch 852782 2 cdumez 2013-03-11 15:23:25 -0700 Could I please get informal review? 853125 3 189055 gyuyoung.kim 2013-03-11 23:53:03 -0700 Comment on attachment 189055 Patch It seems to me existing implementation is weird. LGTM. BTW, should we provide ewk_popup_menu_close()? When browser lets webkit know that popup menu is closed ? Any use case ? 853128 4 cdumez 2013-03-12 00:02:37 -0700 (In reply to comment #3) > (From update of attachment 189055 [details]) > It seems to me existing implementation is weird. LGTM. BTW, should we provide ewk_popup_menu_close()? When browser lets webkit know that popup menu is closed ? Any use case ? Yes, we need to provide ewk_popup_menu_close() so that the browser lets us know when the popup menu gets hidden (so that WebCore updates its state). See MiniBrowser implementation in Bug 108934. Some other ports do not need such API because they run an event loop while displaying the menu. However, this is not appropriate for EFL port as EFL has issues with nested event loops. 853139 5 gyuyoung.kim 2013-03-12 00:25:07 -0700 (In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 189055 [details] [details]) > > It seems to me existing implementation is weird. LGTM. BTW, should we provide ewk_popup_menu_close()? When browser lets webkit know that popup menu is closed ? Any use case ? > > Yes, we need to provide ewk_popup_menu_close() so that the browser lets us know when the popup menu gets hidden (so that WebCore updates its state). See MiniBrowser implementation in Bug 108934. > > Some other ports do not need such API because they run an event loop while displaying the menu. However, this is not appropriate for EFL port as EFL has issues with nested event loops. I see. I talk with Byungwoo. Yes, we need to provide this API because of EFL nested event loop issue. Tizen WebKit also has same problem. BTW, I considered that we can add a new C API to hide popup menu. But, it looks it isn't good to add new C APIs for only EFL specific issue. LGTM. 853772 6 189055 benjamin 2013-03-12 14:16:43 -0700 Comment on attachment 189055 Patch Ok for WebKit2. 854029 7 189055 laszlo.gombos 2013-03-12 20:40:57 -0700 Comment on attachment 189055 Patch r=me. 854031 8 189055 webkit.review.bot 2013-03-12 20:49:12 -0700 Comment on attachment 189055 Patch Clearing flags on attachment: 189055 Committed r145676: <http://trac.webkit.org/changeset/145676> 854032 9 webkit.review.bot 2013-03-12 20:49:16 -0700 All reviewed patches have been landed. Closing bug. 189055 2013-02-19 04:32:58 -0800 2013-03-12 20:49:12 -0700 Patch 110209_ewk_popup_menu_close.patch text/plain 1678 cdumez ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQyL0No YW5nZUxvZwppbmRleCA2ZDFiYmYxLi4yMjYwNzgwIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 Mi9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjEg QEAKKzIwMTMtMDItMTkgIENocmlzdG9waGUgRHVtZXogIDxjaC5kdW1lekBzaXNhLnNhbXN1bmcu Y29tPgorCisgICAgICAgIFtFRkxdW1dLMl0gZXdrX3BvcHVwX21lbnVfY2xvc2UoKSBkb2VzIG5v dCB3b3JrIGFzIGludGVuZGVkCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3df YnVnLmNnaT9pZD0xMTAyMDkKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4K KworICAgICAgICBld2tfcG9wdXBfbWVudV9jbG9zZSgpIGlzIGNhbGxlZCBieSB0aGUgYnJvd3Nl ciB0byBub3RpZnkgV2ViS2l0IHRoYXQKKyAgICAgICAgdGhlIHBvcHVwIG1lbnUgd2FzIGNsb3Nl ZC4gSG93ZXZlciwgdGhlIGltcGxlbWVudGF0aW9uIHdhcyBjYWxsaW5nCisgICAgICAgIEV3a1Zp ZXc6OmNsb3NlUG9wdXBNZW51KCkgd2hpY2ggd2FzIGFza2luZyB0aGUgYnJvd3NlciB0byBjbG9z ZSB0aGUKKyAgICAgICAgcG9wdXAgbWVudSB2aWEgYW4gRXZhcyBzbWFydCBmdW5jdGlvbi4gVGhp cyBwYXRjaCBmaXhlcyB0aGUgYmVoYXZpb3IKKyAgICAgICAgYW5kIGV3a19wb3B1cF9tZW51X2Ns b3NlKCkgbm93IGNhbGxzIFdLUG9wdXBNZW51TGlzdGVuZXJTZXRTZWxlY3Rpb24oKQorICAgICAg ICBDIEFQSSBzbyB0aGF0IHRoZSBXZWJQcm9jZXNzIGlzIG5vdGlmaWVkIHRoYXQgdGhlIHBvcHVw IG1lbnUgd2FzCisgICAgICAgIGhpZGRlbi4KKworICAgICAgICAqIFVJUHJvY2Vzcy9BUEkvZWZs L2V3a19wb3B1cF9tZW51LmNwcDoKKyAgICAgICAgKEV3a1BvcHVwTWVudTo6Y2xvc2UpOgorCiAy MDEzLTAyLTE4ICBDYXJsb3MgR2FyY2lhIENhbXBvcyAgPGNnYXJjaWFAaWdhbGlhLmNvbT4KIAog ICAgICAgICBbR1RLXSBSZW1vdmUgd2Via2l0X3dlYl92aWV3X2dldF9zdWJyZXNvdXJjZXMgZnJv bSBXZWJLaXQyIEdUSysgQVBJCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3Mv QVBJL2VmbC9ld2tfcG9wdXBfbWVudS5jcHAgYi9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJ L2VmbC9ld2tfcG9wdXBfbWVudS5jcHAKaW5kZXggNjI5ZWY0Zi4uZDE2NTIyNSAxMDA2NDQKLS0t IGEvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL0FQSS9lZmwvZXdrX3BvcHVwX21lbnUuY3BwCisr KyBiL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZWZsL2V3a19wb3B1cF9tZW51LmNwcApA QCAtNjAsNyArNjAsOCBAQCBFd2tQb3B1cE1lbnU6On5Fd2tQb3B1cE1lbnUoKQogCiB2b2lkIEV3 a1BvcHVwTWVudTo6Y2xvc2UoKQogewotICAgIG1fdmlldy0+Y2xvc2VQb3B1cE1lbnUoKTsKKyAg ICAvLyBTZXR0aW5nIHNlbGVjdGVkIGl0ZW0gd2lsbCBjYXVzZSB0aGUgcG9wdXAgbWVudSB0byBj bG9zZS4KKyAgICBXS1BvcHVwTWVudUxpc3RlbmVyU2V0U2VsZWN0aW9uKG1fcG9wdXBNZW51TGlz dGVuZXIuZ2V0KCksIG1fc2VsZWN0ZWRJbmRleCk7CiB9CiAKIGNvbnN0IEVpbmFfTGlzdCogRXdr UG9wdXBNZW51OjppdGVtcygpIGNvbnN0Cg==