Created attachment 146474 [details] work in progress #1

Comment on attachment 146474 [details] work in progress #1 View in context: https://bugs.webkit.org/attachment.cgi?id=146474&action=review > Source/WebKit2/UIProcess/API/efl/ewk_enums.h:28 > + EWK_RTL, In WebKit1, we also had a EWK_TEXT_DIRECTION_DEFAULT which mapped to the "natural" direction in WebCore. Also shouldn't we use something like "EWK_TEXT_DIRECTION_RTL" instead of "EWK_RTL"? > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:22 > +#include "ewk_view_private.h" This private header should be sorted with the other below I believe. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:529 > + Shouldn't add a null check for popupMenu here? > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:536 > + for (size_t i = 0; i < items.size(); ++i) { According to coding style, items.size() should be cached. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:550 > + smartData->api->popup_menu_show(smartData, rect, static_cast<Ewk_Text_Direction>(textDirection), pageScaleFactor, popupItems, selectedIndex); This cast from TextDirection to Ewk_Text_Direction is not safe. I don't think we have a AssertMatchingEnums equivalent for WK2, do we? > Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:41 > + extra blank line

Comment on attachment 146474 [details] work in progress #1 Ryuan, if possible, please do not submit working in progress patch. It looks patch like this can make confusion. If you need to submit intermediate patch, please set cq- first.

Created attachment 158030 [details] Patch

(In reply to comment #2) > (From update of attachment 146474 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146474&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_enums.h:28 > > + EWK_RTL, > > In WebKit1, we also had a EWK_TEXT_DIRECTION_DEFAULT which mapped to the "natural" direction in WebCore. Also shouldn't we use something like "EWK_TEXT_DIRECTION_RTL" instead of "EWK_RTL"? > WebCore has TextDirection(containts RTL, LTR) and WritingDirection (contains natural, RTL, LTR). I think that this is little bit ambiguous. BTW, we need TextDirection now. > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:529 > > + > > Shouldn't add a null check for popupMenu here? I added ASSERT. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:536 > > + for (size_t i = 0; i < items.size(); ++i) { > > According to coding style, items.size() should be cached. > Done. > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:550 > > + smartData->api->popup_menu_show(smartData, rect, static_cast<Ewk_Text_Direction>(textDirection), pageScaleFactor, popupItems, selectedIndex); > > This cast from TextDirection to Ewk_Text_Direction is not safe. I don't think we have a AssertMatchingEnums equivalent for WK2, do we? > Done. > > Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:41 > > + > > extra blank line Removed.

Comment on attachment 158030 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158030&action=review > Source/WebKit2/ChangeLog:10 > + The applications should implement popup menu by overriding %s/The applications/Applications/g > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:134 > + void* itemv; IIRC, EFL port has used *items* more commonly. But, I don't mind to use this. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1524 > + } Personally, I think it is good to add an empty line to here. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1534 > + if (!priv->popupMenuProxy) As we discussed this before, how do you think to use EINA_SAFETY_ON_NULL_RETURN_VAL() ? > Source/WebKit2/UIProcess/API/efl/ewk_view.h:127 > +#define EWK_VIEW_SMART_CLASS_INIT(smart_class_init) {smart_class_init, EWK_VIEW_SMART_CLASS_VERSION, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} Don't you need to update #define EWK_VIEW_SMART_CLASS_VERSION 1UL ? > Source/WebKit2/UIProcess/API/efl/ewk_view.h:609 > +* Selects index of current popup menu. Nit : Add a space in front of *. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:620 > +* Closes current popup menu. ditto.

Created attachment 158034 [details] Patch

Created attachment 158035 [details] Patch

(In reply to comment #6) > (From update of attachment 158030 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158030&action=review > > > Source/WebKit2/ChangeLog:10 > > + The applications should implement popup menu by overriding > > %s/The applications/Applications/g > Done. > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:134 > > + void* itemv; > > IIRC, EFL port has used *items* more commonly. But, I don't mind to use this. > Sorry, I don't have good name for this, so I just copied this from webkit1/efl. In this case, items is not good because itemv is temporal item to be cast. > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1524 > > + } > > Personally, I think it is good to add an empty line to here. > Done. > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1534 > > + if (!priv->popupMenuProxy) > > As we discussed this before, how do you think to use EINA_SAFETY_ON_NULL_RETURN_VAL() ? > My mistake. Done. > > Source/WebKit2/UIProcess/API/efl/ewk_view.h:127 > > +#define EWK_VIEW_SMART_CLASS_INIT(smart_class_init) {smart_class_init, EWK_VIEW_SMART_CLASS_VERSION, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} > > Don't you need to update #define EWK_VIEW_SMART_CLASS_VERSION 1UL ? > Done. > > Source/WebKit2/UIProcess/API/efl/ewk_view.h:609 > > +* Selects index of current popup menu. > > Nit : Add a space in front of *. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.h:620 > > +* Closes current popup menu. > > ditto. Done, Thank you.

Comment on attachment 158035 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158035&action=review > Source/WebKit2/UIProcess/API/efl/ewk_enums.h:28 > + In public files case, we have added file description. > Source/WebKit2/UIProcess/API/efl/ewk_enums.h:33 > +/// Enum values containing text directionality values. Could you explain pros when we have a file for enums ? BTW, I think you need to write description in ChangeLog as well. Because, this file can be used by other patches.

Created attachment 158206 [details] Patch

(In reply to comment #10) > (From update of attachment 158035 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158035&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_enums.h:28 > > + > > In public files case, we have added file description. > > > Source/WebKit2/UIProcess/API/efl/ewk_enums.h:33 > > +/// Enum values containing text directionality values. > > Could you explain pros when we have a file for enums ? BTW, I think you need to write description in ChangeLog as well. Because, this file can be used by other patches. I decide not to add it. I thought small benefits that reduces includes from header files. But, I removed because 1) we already has some enums and I don't want to move them, 2) it is not highly related to this bug. Thank you

Comment on attachment 158206 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158206&action=review > Source/WebKit2/ChangeLog:14 > + * UIProcess/API/efl/EWebKit2.h: Remove this line. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1485 > + EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv); I would like to recommend to move this macro to below 1479 line in order to be consistent with existing ewk_view.cpp. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1515 > + EINA_SAFETY_ON_NULL_RETURN_VAL(smartData->api, false); ditto.

Created attachment 158217 [details] Patch

(In reply to comment #13) > (From update of attachment 158206 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158206&action=review > > > Source/WebKit2/ChangeLog:14 > > + * UIProcess/API/efl/EWebKit2.h: > > Remove this line. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1485 > > + EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv); > > I would like to recommend to move this macro to below 1479 line in order to be consistent with existing ewk_view.cpp. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1515 > > + EINA_SAFETY_ON_NULL_RETURN_VAL(smartData->api, false); > > ditto. OK, fixed.

Comment on attachment 158217 [details] Patch Looks good to me now. By the way, are there any unskip test by this patch ?

(In reply to comment #16) > (From update of attachment 158217 [details]) > Looks good to me now. By the way, are there any unskip test by this patch ? Thank you, this is not related to test case. But you can see the crash when we click <select> without this.

Comment on attachment 158217 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158217&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:133 > + if (popupMenuItems) { Is this check really needed? I thought NULL was a empty Eina_List. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:137 > + eina_stringshare_del(item->text); This should be part of the Ewk_Popup_Menu_Item destructor. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1493 > + Ewk_Popup_Menu_Item* item = new Ewk_Popup_Menu_Item; We should have a constructor for Ewk_Popup_Menu_Item and pass text / type as argument. This is more C++ like and more consistent with the style used in WK2-EFL. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1522 > + eina_stringshare_del(item->text); Should be in a destructor. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1537 > + if (selectedIndex > eina_list_count(priv->popupMenuItems)) >= ? > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1541 > + return true; We usually add an empty line before return statements. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:100 > + Eina_Bool (*popup_menu_show)(Ewk_View_Smart_Data *sd, Eina_Rectangle rect, Ewk_Text_Direction text_direction, double page_scale_factor, Eina_List* items, int selected_index); "Eina_List* items" <- Star on wrong side. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:196 > +struct _Ewk_Popup_Menu_Item { Shouldn't this be defined in its own ewk_popup_menu_item.cpp with getters in ewk_popup_menu_item.h, and property constructor/destructor. We have tried not to use this kind of structs in the headers for WK2 EFL. > Source/WebKit2/UIProcess/efl/WebPopupMenuProxyEfl.cpp:33 > + Why the empty line here? > Source/WebKit2/UIProcess/efl/WebPopupMenuProxyEfl.h:47 > + ~WebPopupMenuProxyEfl(); Destructor should be virtual.

Comment on attachment 158217 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158217&action=review >> Source/WebKit2/UIProcess/efl/WebPopupMenuProxyEfl.h:47 >> + ~WebPopupMenuProxyEfl(); > > Destructor should be virtual. I'm not sure if we SHOULD use virtual keyword again when super class(WebPopupMenuProxy) already defined virtual destructor. To my quick search, QT and GTK ports didn't use virtual keyword in WebPopupMenuProxyGtk | QT. In addition, I couldn't find duplicated virtual keyword both super class and child in WK2 at least. How do you think about this ? See also, WebColorChooserProxyQt.h

Created attachment 158253 [details] Patch

Created attachment 158254 [details] Patch

(In reply to comment #21) > Created an attachment (id=158254) [details] > Patch Tried to introduce ewk_popup_menu_item.{h|cpp}

Comment on attachment 158253 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158253&action=review > Source/WebKit2/UIProcess/API/efl/ewk_popup_menu_item.cpp:41 > + , text(eina_stringshare_add(_text)) Apparently, passing NULL to eina_stringshare_add() is fine, according to the doc, it will return NULL. > Source/WebKit2/UIProcess/API/efl/ewk_popup_menu_item.cpp:46 > + eina_stringshare_del(text); I believe you need a NULL-check here. The eina_stringshare_del() doc says: "Note that if the given pointer is not shared or NULL, bad things will happen, likely a segmentation fault." > Source/WebKit2/UIProcess/API/efl/ewk_popup_menu_item.cpp:65 > + delete item; We usually have an EINA_SAFETY check for item here. (even if "delete 0;" is not invalid). This is just more consistent with EFL C APIs. > Source/WebKit2/UIProcess/API/efl/ewk_popup_menu_item.cpp:71 > + return item->type; We usually put an empty line before return statements. > Source/WebKit2/UIProcess/API/efl/ewk_popup_menu_item.cpp:77 > + return item->text; Ditto. > Source/WebKit2/UIProcess/API/efl/ewk_popup_menu_item.h:57 > +EAPI Ewk_Popup_Menu_Item_Type ewk_popup_menu_item_type_get(const Ewk_Popup_Menu_Item* item); Star on wrong side. > Source/WebKit2/UIProcess/API/efl/ewk_popup_menu_item.h:64 > + * @return the text of the @a item or @c NULL in case of error. This pointer is It also returns NULL if this is a separator (which is not an error case). Maybe it should be mentioned in the doc. > Source/WebKit2/UIProcess/API/efl/ewk_popup_menu_item.h:70 > +EAPI const char* ewk_popup_menu_item_text_get(const Ewk_Popup_Menu_Item* item); Stars on wrong side. > Source/WebKit2/UIProcess/API/efl/ewk_popup_menu_item_private.h:26 > +Ewk_Popup_Menu_Item* ewk_popup_menu_item_new(Ewk_Popup_Menu_Item_Type type, const char* text); Since it is a private function, I would use the WebPopupItem::Type here, for convenience, instead of Ewk_Popup_Menu_Item_Type. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1473 > +COMPILE_ASSERT_MATCHING_ENUM(EWK_POPUP_MENU_SEPARATOR, WebPopupItem::Separator); Should be in ewk_popup_menu_item.cpp > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1474 > +COMPILE_ASSERT_MATCHING_ENUM(EWK_POPUP_MENU_ITEM, WebPopupItem::Item); Ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1492 > + Ewk_Popup_Menu_Item* item = ewk_popup_menu_item_new(static_cast<Ewk_Popup_Menu_Item_Type>(items[i].m_type), items[i].m_text.utf8().data()); I would remove this cast.

Created attachment 158261 [details] Patch

(In reply to comment #23) > (From update of attachment 158253 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158253&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_popup_menu_item.cpp:41 > > + , text(eina_stringshare_add(_text)) > > Apparently, passing NULL to eina_stringshare_add() is fine, according to the doc, it will return NULL. > > > Source/WebKit2/UIProcess/API/efl/ewk_popup_menu_item.cpp:46 > > + eina_stringshare_del(text); > > I believe you need a NULL-check here. The eina_stringshare_del() doc says: > "Note that if the given pointer is not shared or NULL, bad things will happen, likely a segmentation fault." > Well, I believe that document is wrong. Indeed, document has contradiction. In http://docs.enlightenment.org/auto/eina/group__Eina__Stringshare__Group.html#ga495411b0bc85b9c0d59ec7255a025260 "... If str is NULL, the function returns immediately. Note that if the given pointer is not shared or NULL, bad things will happen, likely a segmentation fault." I fixed other. Thank you.

Comment on attachment 158253 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158253&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_popup_menu_item.cpp:46 >> + eina_stringshare_del(text); > > I believe you need a NULL-check here. The eina_stringshare_del() doc says: > "Note that if the given pointer is not shared or NULL, bad things will happen, likely a segmentation fault." If so, we need to add a NULL-check in existing implementation. It looks many existing implementations don't have the NULL check. For example, in ewk_intent_service.cpp case, struct _Ewk_Intent_Service { unsigned int __ref; /**< the reference count of the object */ #if ENABLE(WEB_INTENTS_TAG) WKRetainPtr<WKIntentServiceInfoRef> wkService; #endif const char* action; const char* type; const char* href; const char* title; const char* disposition; ~_Ewk_Intent_Service() { ASSERT(!__ref); eina_stringshare_del(action); eina_stringshare_del(type); eina_stringshare_del(href); eina_stringshare_del(title); eina_stringshare_del(disposition); } }; >> Source/WebKit2/UIProcess/API/efl/ewk_popup_menu_item_private.h:26 >> +Ewk_Popup_Menu_Item* ewk_popup_menu_item_new(Ewk_Popup_Menu_Item_Type type, const char* text); > > Since it is a private function, I would use the WebPopupItem::Type here, for convenience, instead of Ewk_Popup_Menu_Item_Type. ewk_popup_menu_item returns new Ewk_Popup_Menu_Item() instance. And, ctor of _Ewk_Popup_Menu_Item is defined using Ewk_Popup_Menu_Item_Type as below, struct _Ewk_Popup_Menu_Item { ... _Ewk_Popup_Menu_Item(Ewk_Popup_Menu_Item_Type _type, const char* _text) In addition, Ewk_Popup_Menu_Item_Type is different from WebPopupItem::Type struct WebPopupItem { enum Type { Separator, Item }; So, I don't think it is good to use WebPopupItem::type instead of Ewk_Popup_Menu_Item_Type.

Comment on attachment 158253 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158253&action=review >>> Source/WebKit2/UIProcess/API/efl/ewk_popup_menu_item.cpp:46 >>> + eina_stringshare_del(text); >> >> I believe you need a NULL-check here. The eina_stringshare_del() doc says: >> "Note that if the given pointer is not shared or NULL, bad things will happen, likely a segmentation fault." > > If so, we need to add a NULL-check in existing implementation. It looks many existing implementations don't have the NULL check. > > For example, in ewk_intent_service.cpp case, > > struct _Ewk_Intent_Service { > unsigned int __ref; /**< the reference count of the object */ > #if ENABLE(WEB_INTENTS_TAG) > WKRetainPtr<WKIntentServiceInfoRef> wkService; > #endif > const char* action; > const char* type; > const char* href; > const char* title; > const char* disposition; > > ~_Ewk_Intent_Service() > { > ASSERT(!__ref); > eina_stringshare_del(action); > eina_stringshare_del(type); > eina_stringshare_del(href); > eina_stringshare_del(title); > eina_stringshare_del(disposition); > } > }; Ok, then fine by me. Thanks for checking. >>> Source/WebKit2/UIProcess/API/efl/ewk_popup_menu_item_private.h:26 >>> +Ewk_Popup_Menu_Item* ewk_popup_menu_item_new(Ewk_Popup_Menu_Item_Type type, const char* text); >> >> Since it is a private function, I would use the WebPopupItem::Type here, for convenience, instead of Ewk_Popup_Menu_Item_Type. > > ewk_popup_menu_item returns new Ewk_Popup_Menu_Item() instance. And, ctor of _Ewk_Popup_Menu_Item is defined using Ewk_Popup_Menu_Item_Type as below, > > struct _Ewk_Popup_Menu_Item { > ... > _Ewk_Popup_Menu_Item(Ewk_Popup_Menu_Item_Type _type, const char* _text) > > In addition, Ewk_Popup_Menu_Item_Type is different from WebPopupItem::Type > > struct WebPopupItem { > enum Type { > Separator, > Item > }; > > So, I don't think it is good to use WebPopupItem::type instead of Ewk_Popup_Menu_Item_Type. Yes, the _Ewk_Popup_Menu_Item() constructor uses Ewk_Popup_Menu_Item_Type and I think it is fine. However, I still prefer the ewk_popup_menu_item_new() to take a WebPopupItem::type. As a result, the casting hidden will be inside ewk_popup_menu_item_new() instead of the callers. This makes the API more convenient to use and we don't need to use Ewk types since it is private API. You are saying "Ewk_Popup_Menu_Item_Type is different from WebPopupItem::Type". How is it different? We have compile assertions to make sure they match. However, the compile assertions should be in ewk_popup_menu_item.cpp instead of the caller (ewk_view.cpp), and therefore, the casting would be better in ewk_popup_menu_item.cpp as well, instead of ewk_view.cpp.

Comment on attachment 158261 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158261&action=review LGTM with minor suggestion: > Source/WebKit2/UIProcess/API/efl/ewk_popup_menu_item.cpp:40 > + const char* text; You could use the new WKEinaSharedString type (c.f. webkit-efl mailing list).

Created attachment 158295 [details] Patch

(In reply to comment #28) > (From update of attachment 158261 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158261&action=review > > LGTM with minor suggestion: > > > Source/WebKit2/UIProcess/API/efl/ewk_popup_menu_item.cpp:40 > > + const char* text; > > You could use the new WKEinaSharedString type (c.f. webkit-efl mailing list). Right, I fixed.

Comment on attachment 158295 [details] Patch LGTM. Thanks!

Comment on attachment 158295 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158295&action=review > Source/WebKit2/UIProcess/efl/WebPopupMenuProxyEfl.h:47 > + ~WebPopupMenuProxyEfl(); Hmm. You apparently forgot to mark this destructor as virtual.

(In reply to comment #32) > (From update of attachment 158295 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158295&action=review > > > Source/WebKit2/UIProcess/efl/WebPopupMenuProxyEfl.h:47 > > + ~WebPopupMenuProxyEfl(); > > Hmm. You apparently forgot to mark this destructor as virtual. I already mentioned this in Comment 19.

(In reply to comment #32) > (From update of attachment 158295 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158295&action=review > > > Source/WebKit2/UIProcess/efl/WebPopupMenuProxyEfl.h:47 > > + ~WebPopupMenuProxyEfl(); > > Hmm. You apparently forgot to mark this destructor as virtual. Sorry, I missed to add comment. Because this is derived class, I think that virtual keyword is not needed. If I am right, other derived classes in webkit also does not have virtual destructor.

Comment on attachment 158295 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158295&action=review > Source/WebKit2/UIProcess/efl/WebPopupMenuProxyEfl.h:49 > + virtual void showPopupMenu(const WebCore::IntRect&, WebCore::TextDirection, double pageScaleFactor, const Vector<WebPopupItem>&, const PlatformPopupMenuData&, int32_t selectedIndex); You have methods marked as virtual here but the destructor is not marked as virtual: This is wrong. If the WebPopupMenuProxyEfl class cannot be subclassed, then remove "virtual" keyword from those 2 methods, otherwise, please mark the destructor as virtual.

Created attachment 158305 [details] Patch

Comment on attachment 158305 [details] Patch LGTM.

ryuan: It seems gyuyoung and chris both have commented on this patch as 'LGTM'. Let's push this into the trunk. I want to verify something with this patch. :-)

Sorry for itching. ;-) I think this patch would resolve some layout test cases including 'BUGWKEFL : fast/forms/select/menulist-popup-crash.html = CRASH'. Why don't you run test only in TestExpectations. :-)

(In reply to comment #39) > Sorry for itching. ;-) > I think this patch would resolve some layout test cases including 'BUGWKEFL : fast/forms/select/menulist-popup-crash.html = CRASH'. Why don't you run test only in TestExpectations. :-) Good point. I didn't catch it. I will rebase after checking. Thank you.

Created attachment 158983 [details] Patch

(In reply to comment #39) > Sorry for itching. ;-) > I think this patch would resolve some layout test cases including 'BUGWKEFL : fast/forms/select/menulist-popup-crash.html = CRASH'. Why don't you run test only in TestExpectations. :-) I removed it in TestExpectations. Thank you. And, I changed EINA_SAFETY_ON_NULL_RETURN for popup_menu_show to if( ...) not to print in WTR. Previously it is not important to me, but now I believe that we'd better not to make noise for layout test.

(In reply to comment #42) > And, I changed EINA_SAFETY_ON_NULL_RETURN for popup_menu_show to if( ...) not to print in WTR. > > Previously it is not important to me, but now I believe that we'd better not to make noise for layout test. Yes, I also think it is more important not to make noise for layout test.

Created attachment 160623 [details] patch with unit tests

Comment on attachment 160623 [details] patch with unit tests View in context: https://bugs.webkit.org/attachment.cgi?id=160623&action=review > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:192 > +Eina_Bool popup_menu_show(Ewk_View_Smart_Data *sd, Eina_Rectangle rect, Ewk_Text_Direction text_direction, double page_scale_factor, Eina_List *items, int selected_index) AFAIK, unit test also follows webkit coding style.

Created attachment 160626 [details] Patch

(In reply to comment #45) > (From update of attachment 160623 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=160623&action=review > > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:192 > > +Eina_Bool popup_menu_show(Ewk_View_Smart_Data *sd, Eina_Rectangle rect, Ewk_Text_Direction text_direction, double page_scale_factor, Eina_List *items, int selected_index) > > AFAIK, unit test also follows webkit coding style. Right, my mistake with copy&pate. :) Fixed.

Created attachment 160633 [details] Patch

Comment on attachment 160626 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160626&action=review And, I think this patch needs to get informal review as well. If there is no comment, I will set r+. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1487 > + It looks this is unneeded line. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:192 > +Eina_Bool popup_menu_show(Ewk_View_Smart_Data* smartData, Eina_Rectangle, Ewk_Text_Direction, double, Eina_List*, int selectedIndex) EFL unit test has used static keyword for internal function. Please use standard bool, showPopupMenu() instead of Eina_Bool and popup_menu_show().

Comment on attachment 160633 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160633&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1502 > + if (!priv->popupMenuProxy) > + return false; I changed from EINA_SAFETY_ON_NULL_RETURN_VAL to `if(...)`. It's because ewk_view_popup_menu_close can be called although popupMenuProxy is null. When WebPageProxy want to call popup menu more that twice, WebPageProxy calls ewk_view_popup_menu_close although user did.

Created attachment 160635 [details] Patch

Created attachment 160636 [details] Patch

(In reply to comment #49) > (From update of attachment 160626 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=160626&action=review > > And, I think this patch needs to get informal review as well. If there is no comment, I will set r+. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1487 > > + > > It looks this is unneeded line. Removed. > > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:192 > > +Eina_Bool popup_menu_show(Ewk_View_Smart_Data* smartData, Eina_Rectangle, Ewk_Text_Direction, double, Eina_List*, int selectedIndex) > > EFL unit test has used static keyword for internal function. Please use standard bool, showPopupMenu() instead of Eina_Bool and popup_menu_show(). right, fixed.

Comment on attachment 160636 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160636&action=review > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:192 > +static Eina_Bool popup_menu_show(Ewk_View_Smart_Data* smartData, Eina_Rectangle, Ewk_Text_Direction, double, Eina_List*, int selectedIndex) This is not fixed yet.

Created attachment 160644 [details] Patch

Created attachment 160647 [details] Patch

(In reply to comment #54) > (From update of attachment 160636 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=160636&action=review > > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:192 > > +static Eina_Bool popup_menu_show(Ewk_View_Smart_Data* smartData, Eina_Rectangle, Ewk_Text_Direction, double, Eina_List*, int selectedIndex) > > This is not fixed yet. Sorry for all noise. Fixed.

Comment on attachment 160647 [details] Patch Please land this patch after getting LGTM from other committers(maybe Intel?).

Comment on attachment 160647 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160647&action=review > Source/WebKit2/UIProcess/API/efl/ewk_popup_menu_item.cpp:50 > + > + ~_Ewk_Popup_Menu_Item() > + { > + } Not needed. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:213 > + waitUntilTitleChangedTo("first"); > + ASSERT_STREQ(ewk_view_title_get(webView()), "first"); This assert is not really necessary since waitUntilTitleChangedTo ensures that for you. If something goes wrong and the title does not change to "first", the test will timeout and never reach the ASSERT anyway. > Source/WebKit2/UIProcess/efl/WebPopupMenuProxyEfl.cpp:47 > + > +WebPopupMenuProxyEfl::~WebPopupMenuProxyEfl() > +{ > +} Not needed. > Source/WebKit2/UIProcess/efl/WebPopupMenuProxyEfl.h:47 > + ~WebPopupMenuProxyEfl(); Ditto.

Created attachment 160945 [details] Patch

(In reply to comment #59) > (From update of attachment 160647 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=160647&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_popup_menu_item.cpp:50 > > + > > + ~_Ewk_Popup_Menu_Item() > > + { > > + } > > Not needed. > > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:213 > > + waitUntilTitleChangedTo("first"); > > + ASSERT_STREQ(ewk_view_title_get(webView()), "first"); > > This assert is not really necessary since waitUntilTitleChangedTo ensures that for you. If something goes wrong and the title does not change to "first", the test will timeout and never reach the ASSERT anyway. > > > Source/WebKit2/UIProcess/efl/WebPopupMenuProxyEfl.cpp:47 > > + > > +WebPopupMenuProxyEfl::~WebPopupMenuProxyEfl() > > +{ > > +} > > Not needed. > > > Source/WebKit2/UIProcess/efl/WebPopupMenuProxyEfl.h:47 > > + ~WebPopupMenuProxyEfl(); > > Ditto. Thank you. I fixed what you addressed.

Comment on attachment 160945 [details] Patch LGTM.

Comment on attachment 160945 [details] Patch Clearing flags on attachment: 160945 Committed r126866: <http://trac.webkit.org/changeset/126866>