Bug 108798 – [EFL][WK2] Port EwkPopupMenuItem to the C API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Chris Dumez]

As per Bug 107657, we should stop using internal C++ API in ewk_popup_menu and start using the C API instead of avoid violating API layering.

[Chris Dumez]

Created attachment 186337 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 186337 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186337&action=review

> Source/WebKit2/UIProcess/API/C/efl/WKPopupMenuEfl.cpp:34
> +void WKPopupMenuEflSetSelectedIndex(WKPopupMenuRef menuRef, int selectedIndex)

Efl would be wrong style. Please use EFL. Maybe we don't even need it, it is just an extension for our platform.

What about multi select?

[Chris Dumez]

Created attachment 186339 [details]
Patch

Take Kenneth's feedback into consideration.

[Build Bot]

Comment on attachment 186339 [details]
Patch

Attachment 186339 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16371168

[Chris Dumez]

Created attachment 186344 [details]
Patch

Attempt to fix mac-wk2 ews.

[Chris Dumez]

Created attachment 186355 [details]
Patch

Rebase on master as the XCode project change started conflicting.

[Kenneth Rohde Christiansen]

Comment on attachment 186355 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186355&action=review

> Source/WebKit2/UIProcess/efl/WebPopupMenuProxyEfl.cpp:55
> +    // requestPopupMenu() takes ownership of the menu items.
> +    m_view->requestPopupMenu(toAPI(static_cast<WebPopupMenuProxy*>(this)), rect, textDirection, pageScaleFactor, popupMenuItems, selectedIndex);

So still this class knows about our view implementation (actually depends on it). I guess it could depend on our WebView (WKView) in the near future

[Chris Dumez]

(In reply to comment #7)
> (From update of attachment 186355 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186355&action=review
> 
> > Source/WebKit2/UIProcess/efl/WebPopupMenuProxyEfl.cpp:55
> > +    // requestPopupMenu() takes ownership of the menu items.
> > +    m_view->requestPopupMenu(toAPI(static_cast<WebPopupMenuProxy*>(this)), rect, textDirection, pageScaleFactor, popupMenuItems, selectedIndex);
> 
> So still this class knows about our view implementation (actually depends on it). I guess it could depend on our WebView (WKView) in the near future

Yes. Likely, we'll need to use the new WebView class once it is introduced (that patch has not landed yet). Until then, this will do fine :)

[Chris Dumez]

Created attachment 186599 [details]
Patch

Refactor the implementation so that we do not require any C API anymore.

[Kenneth Rohde Christiansen]

Comment on attachment 186599 [details]
Patch

Great changelog, looks good

[Mikhail Pozdnyakov]

Comment on attachment 186599 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186599&action=review

looks good.

> Source/WebKit2/UIProcess/efl/WebPopupMenuProxyEfl.cpp:78
> +    const size_t size = items.size();

const local variables are not due to coding style.

[Chris Dumez]

Created attachment 189016 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 189016 [details]
Patch

LGTM

[Benjamin Poulain]

Comment on attachment 189016 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189016&action=review

Looks great. I sign off on this.

> Source/WebKit2/UIProcess/API/efl/ewk_popup_menu_item.cpp:38
> +COMPILE_ASSERT_MATCHING_ENUM(EWK_POPUP_MENU_SEPARATOR, kWKPopupItemTypeSeparator);
> +COMPILE_ASSERT_MATCHING_ENUM(EWK_POPUP_MENU_ITEM, kWKPopupItemTypeItem);

Just a comment.

I know this style is spread in EFL already, but you can write conversion function from one enum type to an other to have the same flexibility without using tricks.
With a switch()-case() and a good compiler, there should be no additional cost.

> Source/WebKit2/UIProcess/API/efl/ewk_popup_menu_item.cpp:70
> +    if (!m_text)
> +        m_text = WKEinaSharedString(AdoptWK, WKPopupItemCopyText(m_wkItem.get()));
> +
>      return m_text;
>  }
>  
>  const char* EwkPopupMenuItem::tooltipText() const
>  {
> +    if (!m_tooltipText)
> +        m_tooltipText = WKEinaSharedString(AdoptWK, WKPopupItemCopyToolTipText(m_wkItem.get()));
> +
>      return m_tooltipText;
>  }
>  
>  const char* EwkPopupMenuItem::accessibilityText() const
>  {
> +    if (!m_accessibilityText)
> +        m_accessibilityText = WKEinaSharedString(AdoptWK, WKPopupItemCopyAccessibilityText(m_wkItem.get()));
> +

m_test, m_tooltipText and m_accessibilityText should be initialized to 0 in the constructor.

[Chris Dumez]

(In reply to comment #14)
> (From update of attachment 189016 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189016&action=review
> 
> Looks great. I sign off on this.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_popup_menu_item.cpp:38
> > +COMPILE_ASSERT_MATCHING_ENUM(EWK_POPUP_MENU_SEPARATOR, kWKPopupItemTypeSeparator);
> > +COMPILE_ASSERT_MATCHING_ENUM(EWK_POPUP_MENU_ITEM, kWKPopupItemTypeItem);
> 
> Just a comment.
> 
> I know this style is spread in EFL already, but you can write conversion function from one enum type to an other to have the same flexibility without using tricks.
> With a switch()-case() and a good compiler, there should be no additional cost.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_popup_menu_item.cpp:70
> > +    if (!m_text)
> > +        m_text = WKEinaSharedString(AdoptWK, WKPopupItemCopyText(m_wkItem.get()));
> > +
> >      return m_text;
> >  }
> >  
> >  const char* EwkPopupMenuItem::tooltipText() const
> >  {
> > +    if (!m_tooltipText)
> > +        m_tooltipText = WKEinaSharedString(AdoptWK, WKPopupItemCopyToolTipText(m_wkItem.get()));
> > +
> >      return m_tooltipText;
> >  }
> >  
> >  const char* EwkPopupMenuItem::accessibilityText() const
> >  {
> > +    if (!m_accessibilityText)
> > +        m_accessibilityText = WKEinaSharedString(AdoptWK, WKPopupItemCopyAccessibilityText(m_wkItem.get()));
> > +
> 
> m_test, m_tooltipText and m_accessibilityText should be initialized to 0 in the constructor.

Those are WKEinaSharedString objects so there is no need for initializing to 0. The default constructor takes care of initializing the internal const char* to 0.

[Kenneth Rohde Christiansen]

Comment on attachment 189016 [details]
Patch

r=me as signed of by ben

[WebKit Review Bot]

Comment on attachment 189016 [details]
Patch

Clearing flags on attachment: 189016

Committed r143444: <http://trac.webkit.org/changeset/143444>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.