Bug 108798

Created attachment 186337 [details] Patch

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?

Created attachment 186339 [details] Patch Take Kenneth's feedback into consideration.

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

Created attachment 186344 [details] Patch Attempt to fix mac-wk2 ews.

Created attachment 186355 [details] Patch Rebase on master as the XCode project change started conflicting.

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

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

Created attachment 186599 [details] Patch Refactor the implementation so that we do not require any C API anymore.

Comment on attachment 186599 [details] Patch Great changelog, looks good

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.

Created attachment 189016 [details] Patch

Comment on attachment 189016 [details] Patch LGTM

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.

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

Comment on attachment 189016 [details] Patch r=me as signed of by ben

Comment on attachment 189016 [details] Patch Clearing flags on attachment: 189016 Committed r143444: <http://trac.webkit.org/changeset/143444>

All reviewed patches have been landed. Closing bug.