[EFL][WK2] Implement WebPopupMenuProxyEfl to support <select>

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Ryuan Choi]

Patch will be added.

[Ryuan Choi]

Created attachment 146474 [details]
work in progress #1

[Chris Dumez]

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

[Gyuyoung Kim]

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.

[Ryuan Choi]

Created attachment 158030 [details]
Patch

[Ryuan Choi]

(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.

[Gyuyoung Kim]

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.

[Ryuan Choi]

Created attachment 158034 [details]
Patch

[Ryuan Choi]

Created attachment 158035 [details]
Patch

[Ryuan Choi]

(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.

[Gyuyoung Kim]

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.

[Ryuan Choi]

Created attachment 158206 [details]
Patch

[Ryuan Choi]

(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

[Gyuyoung Kim]

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.

[Ryuan Choi]

Created attachment 158217 [details]
Patch

[Ryuan Choi]

(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.

[Gyuyoung Kim]

Comment on attachment 158217 [details]
Patch

Looks good to me now. By the way, are there any unskip test by this patch ?

[Ryuan Choi]

(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.

[Chris Dumez]

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.

[Gyuyoung Kim]

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

[Ryuan Choi]

Created attachment 158253 [details]
Patch

[Ryuan Choi]

Created attachment 158254 [details]
Patch

[Ryuan Choi]

(In reply to comment #21)
> Created an attachment (id=158254) [details]
> Patch

Tried to introduce ewk_popup_menu_item.{h|cpp}

[Chris Dumez]

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.

[Ryuan Choi]

Created attachment 158261 [details]
Patch

[Ryuan Choi]

(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.

[Gyuyoung Kim]

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.

[Chris Dumez]

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.

[Chris Dumez]

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).

[Ryuan Choi]

Created attachment 158295 [details]
Patch

[Ryuan Choi]

(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.

[Chris Dumez]

Comment on attachment 158295 [details]
Patch

LGTM. Thanks!

[Chris Dumez]

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.

[Gyuyoung Kim]

(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.

[Ryuan Choi]

(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.

[Chris Dumez]

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.

[Ryuan Choi]

Created attachment 158305 [details]
Patch

[Chris Dumez]

Comment on attachment 158305 [details]
Patch

LGTM.

[Kangil Han]

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. :-)

[Kangil Han]

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. :-)

[Ryuan Choi]

(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.

[Ryuan Choi]

Created attachment 158983 [details]
Patch

[Ryuan Choi]

(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.

[Gyuyoung Kim]

(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.

[Ryuan Choi]

Created attachment 160623 [details]
patch with unit tests

[Gyuyoung Kim]

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.

[Ryuan Choi]

Created attachment 160626 [details]
Patch

[Ryuan Choi]

(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.

[Ryuan Choi]

Created attachment 160633 [details]
Patch

[Gyuyoung Kim]

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().

[Ryuan Choi]

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.

[Ryuan Choi]

Created attachment 160635 [details]
Patch

[Ryuan Choi]

Created attachment 160636 [details]
Patch

[Ryuan Choi]

(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.

[Gyuyoung Kim]

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.

[Ryuan Choi]

Created attachment 160644 [details]
Patch

[Ryuan Choi]

Created attachment 160647 [details]
Patch

[Ryuan Choi]

(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.

[Gyuyoung Kim]

Comment on attachment 160647 [details]
Patch

Please land this patch after getting LGTM from other committers(maybe Intel?).

[Thiago Marcos P. Santos]

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.

[Ryuan Choi]

Created attachment 160945 [details]
Patch

[Ryuan Choi]

(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.

[Thiago Marcos P. Santos]

Comment on attachment 160945 [details]
Patch

LGTM.

[WebKit Review Bot]

Comment on attachment 160945 [details]
Patch

Clearing flags on attachment: 160945

Committed r126866: <http://trac.webkit.org/changeset/126866>