Bug 113949 – [EFL] --minimal build fails with error: invalid use of incomplete type `class WebKit::WebContextMenuItemData'

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

[Ed Bartosh]

./Tools/Scripts/build-webkit --efl --cmakeargs="-DSHARED_CORE=ON" --minimal fails with this error:

/home/ed/git/wk/WebKit/Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp: In member function `void EwkContextMenu::contextMenuItemSelected(const WebKit::WebContextMenuItemData&)':
/home/ed/git/wk/WebKit/Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:91:23: error: invalid use of incomplete type `class WebKit::WebContextMenuProxyEfl'
In file included from /home/ed/git/wk/WebKit/Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:30:0:
/home/ed/git/wk/WebKit/Source/WebKit2/UIProcess/API/efl/EwkView.h:70:7: error: forward declaration of `class WebKit::WebContextMenuProxyEfl'
/home/ed/git/wk/WebKit/Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp: In function `Eina_Bool ewk_context_menu_item_select(Ewk_Context_Menu*, Ewk_Context_Menu_Item*)':
/home/ed/git/wk/WebKit/Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:146:119: error: invalid use of incomplete type `class WebKit::WebContextMenuItemData'
In file included from /home/ed/git/wk/WebKit/Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:30:0:
/home/ed/git/wk/WebKit/Source/WebKit2/UIProcess/API/efl/EwkView.h:69:7: error: forward declaration of `class WebKit::WebContextMenuItemData'
In file included from /home/ed/git/wk/WebKit/Source/WTF/wtf/text/CString.h:31:0,
                 from /home/ed/git/wk/WebKit/Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:34,
                 from /home/ed/git/wk/WebKit/Source/WebKit2/UIProcess/API/efl/EwkView.h:26,
                 from /home/ed/git/wk/WebKit/Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:30:
/home/ed/git/wk/WebKit/Source/WTF/wtf/Vector.h: In instantiation of `const T& WTF::Vector<T, inlineCapacity>::at(size_t) const [with T = WebKit::WebContextMenuItemData; long unsigned int inlineCapacity = 0ul; size_t = long unsigned int]':
/home/ed/git/wk/WebKit/Source/WTF/wtf/Vector.h:563:58:   required from `const T& WTF::Vector<T, inlineCapacity>::operator[](size_t) const [with T = WebKit::WebContextMenuItemData; long unsigned int inlineCapacity = 0ul; size_t = long unsigned int]'
/home/ed/git/wk/WebKit/Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:46:104:   required from here
/home/ed/git/wk/WebKit/Source/WTF/wtf/Vector.h:559:39: error: invalid use of incomplete type `class WebKit::WebContextMenuItemData'
In file included from /home/ed/git/wk/WebKit/Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:30:0:
/home/ed/git/wk/WebKit/Source/WebKit2/UIProcess/API/efl/EwkView.h:69:7: error: forward declaration of `class WebKit::WebContextMenuItemData'
[ 92%] Building CXX object Source/WebKit2/CMakeFiles/ewebkit2.dir/UIProcess/API/efl/ewk_favicon_database.cpp.o
make[2]: *** [Source/WebKit2/CMakeFiles/ewebkit2.dir/UIProcess/API/efl/ewk_context_menu.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [Source/WebKit2/CMakeFiles/ewebkit2.dir/all] Error 2
make: *** [all] Error 2

[Ed Bartosh]

Created attachment 196506 [details]
Patch

[Ed Bartosh]

Created attachment 196744 [details]
Patch

Rebased. Please, review.

[Ed Bartosh]

*** Bug 114098 has been marked as a duplicate of this bug. ***

[Ed Bartosh]

Created attachment 196803 [details]
Patch

[Ed Bartosh]

Created attachment 196807 [details]
Patch

Rebased. Please review.

[EFL EWS Bot]

Comment on attachment 196807 [details]
Patch

Attachment 196807 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17517137

[Gyuyoung Kim]

Comment on attachment 196807 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.h:34
> +#if ENABLE(CONTEXT_MENUS)

Should not use ENABLE() macro in public header.

[Ed Bartosh]

Created attachment 196875 [details]
Patch

[EFL EWS Bot]

Comment on attachment 196875 [details]
Patch

Attachment 196875 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17586012

[Ed Bartosh]

Created attachment 196951 [details]
Patch

[EFL EWS Bot]

Comment on attachment 196951 [details]
Patch

Attachment 196951 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17423127

[Ed Bartosh]

Created attachment 197170 [details]
Patch

[EFL EWS Bot]

Comment on attachment 197170 [details]
Patch

Attachment 197170 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17597017

[EFL EWS Bot]

Comment on attachment 197170 [details]
Patch

Attachment 197170 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/57050

[Ed Bartosh]

Created attachment 200027 [details]
Patch

[EFL EWS Bot]

Comment on attachment 200027 [details]
Patch

Attachment 200027 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/229554

[Gyuyoung Kim]

Comment on attachment 200027 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:-166
> -static WKContextMenuItemTag getWKTagFromEwkAction(Ewk_Context_Menu_Item_Action action)

Why do you remove this function ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:117
> +#if ENABLE(CONTEXT_MENUS)

Please don't use ENABLE() in public header.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:178
> +#if ENABLE(CONTEXT_MENUS)

ditto.

[Ed Bartosh]

Gyuyoung, how can I distinguish between public and private headers? How can I avoid using use ENABLE(CONTEXT_MENUS) in Source/WebKit2/UIProcess/API/efl/ewk_view.h if m_contextMenu; is defined there?

[Chris Dumez]

(In reply to comment #18)
> Gyuyoung, how can I distinguish between public and private headers? How can I avoid using use ENABLE(CONTEXT_MENUS) in Source/WebKit2/UIProcess/API/efl/ewk_view.h if m_contextMenu; is defined there?

Headers in API/efl/ folder are public unless ending with '_private.h'.
There is no "m_contextMenu" in ewk_view.h so I don't understand your question.

Gyuyoung missed your ifdef to ewk_context_menu.h public header ;)

[Ed Bartosh]

I can see a lot of ENABLEs in Source/WebKit2/UIProcess/API/efl/EwkView.h is it also private header?

[Chris Dumez]

(In reply to comment #20)
> I can see a lot of ENABLEs in Source/WebKit2/UIProcess/API/efl/EwkView.h is it also private header?

No it is not. I thought those files were in UIProcess/efl, sorry.
Our public headers are written in C and are all lowercase. EwkView is C++ and camel case.

Public headers are also listed in Source/WebKit2/PlatformEfl.cmake

[Ed Bartosh]

Created attachment 201272 [details]
Patch

[Ed Bartosh]

Christophe, Thank you for your explanations!

[Chris Dumez]

Comment on attachment 201272 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:28
> +#if ENABLE(CONTEXT_MENUS)

This does not seem right. All the public functions in ewk_context_menu.h would be missing their implementation. You probably needs #ifdefs inside the body of each public API function.

[Chris Dumez]

Comment on attachment 201272 [details]
Patch

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

> Source/WebKit2/ChangeLog:17
> +        * UIProcess/CoordinatedGraphics/WebView.cpp:

FYI, you're touching some non-EFL WK2 files some you will probably need owner review (Source/WebKit2/Owners) unless Noam can do that now (since this is CoordinatedGraphics).

[Noam Rosenthal]

Comment on attachment 201272 [details]
Patch

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

>> Source/WebKit2/ChangeLog:17
>> +        * UIProcess/CoordinatedGraphics/WebView.cpp:
> 
> FYI, you're touching some non-EFL WK2 files some you will probably need owner review (Source/WebKit2/Owners) unless Noam can do that now (since this is CoordinatedGraphics).

Apple were OK with not needing owners to review CoordintedGraphics. Either way, I'm ok with the changes to WebView.
However, here you're touching PageClient.h as well.

[Chris Dumez]

Comment on attachment 201272 [details]
Patch

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

>>> Source/WebKit2/ChangeLog:17
>>> +        * UIProcess/CoordinatedGraphics/WebView.cpp:
>> 
>> FYI, you're touching some non-EFL WK2 files some you will probably need owner review (Source/WebKit2/Owners) unless Noam can do that now (since this is CoordinatedGraphics).
> 
> Apple were OK with not needing owners to review CoordintedGraphics. Either way, I'm ok with the changes to WebView.
> However, here you're touching PageClient.h as well.

Thanks for clarifying Noam. I missed the PageClient change indeed. Definitely needs Owner review then.

[Gyuyoung Kim]

(In reply to comment #19)
> (In reply to comment #18)

> Gyuyoung missed your ifdef to ewk_context_menu.h public header ;)

I said as below, ;-)

> Please don't use ENABLE() in public header.

[Ed Bartosh]

Created attachment 201373 [details]
Patch

[Ed Bartosh]

Updated according to reviewer's comments.

[Gyuyoung Kim]

Comment on attachment 201373 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:869
> +#endif

Please move this to a line above.

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_private.h:29
> +#if ENABLE(CONTEXT_MENUS)

Don't you need to wrap functions defined by this class up with ENABLE(CONTEXT_MENU) as well ? For instance, appendItem(), removeItem().

[Michael Catanzaro]

Closing this bug because the EFL port has been removed from trunk.

If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.