Bug 83693 – [EFL][WK2] Fix WebKit2-EFL build

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Mariusz Grzegorczyk]

After applying all patches related with master bug "[EFL][WK2] Support for EFL port of WebKit2": https://bugs.webkit.org/show_bug.cgi?id=61838 build break occurs because of missing headers path, and changed directory for MiniBrowser from Programs to bin.

[Mariusz Grzegorczyk]

Created attachment 136675 [details]
Patch fixing webkit2 efl build

[Grzegorz Czajkowski]

Please rebase this patch.

[Mariusz Grzegorczyk]

(In reply to comment #2)
> Please rebase this patch.

This patch doesn't need rebase because depends on bug 61850. Dependency/Blocks fields added.

[Ryuan Choi]

Created attachment 144546 [details]
Patch

[Ryuan Choi]

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

Updated patch to catch latest changes and removed dependency of Bug 61850 from patch.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 144546 [details]
Patch

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

Since the EWS does not test this, the most I can do is comment on the build system changes.

> Source/WebKit2/CMakeLists.txt:497
> +SET(WebProcess_NAME ../bin/WebProcess)

It's better to use SET_TARGET_PROPERTIES(... RUNTIME_OUTPUT_DIRECTORY ...) like other parts of the build system do.

> Source/WebKit2/CMakeLists.txt:498
> +SET(WebProcess_SOURCES "")

Is this empty definition needed?

> Source/WebKit2/CMakeLists.txt:507
> +SET(ForwardingHeaders_NAME )
> +SET(ForwardingNetworkHeaders_NAME )

Are these empty definitions needed?

> Source/cmake/OptionsEfl.cmake:42
> +ADD_DEFINITIONS(-DBUILDING_SOUP__=1)

I see that this is checked in WK2, but I couldn't find any part of gtk's build system defining that. Is that expected?

[Ryuan Choi]

Created attachment 144572 [details]
Patch

[Ryuan Choi]

(In reply to comment #6)
> (From update of attachment 144546 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144546&action=review
> 
> Since the EWS does not test this, the most I can do is comment on the build system changes.
> 
> > Source/WebKit2/CMakeLists.txt:497
> > +SET(WebProcess_NAME ../bin/WebProcess)
> 
> It's better to use SET_TARGET_PROPERTIES(... RUNTIME_OUTPUT_DIRECTORY ...) like other parts of the build system do.
>
Thanks for the comment.
I fixed.

> > Source/WebKit2/CMakeLists.txt:498
> > +SET(WebProcess_SOURCES "")
> 
> Is this empty definition needed?
> 
> > Source/WebKit2/CMakeLists.txt:507
> > +SET(ForwardingHeaders_NAME )
> > +SET(ForwardingNetworkHeaders_NAME )
> 
> Are these empty definitions needed?
> 
Removed them.

> > Source/cmake/OptionsEfl.cmake:42
> > +ADD_DEFINITIONS(-DBUILDING_SOUP__=1)
> 
> I see that this is checked in WK2, but I couldn't find any part of gtk's build system defining that. Is that expected?

yes, GNUmakefile.am also defines it.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 144572 [details]
Patch

The build system part looks fine, the rest should supposedly be OK.

[Ryuan Choi]

CCing mrobinson because I touched WebKit2/Gtk+ related files.

[Martin Robinson]

The libsoup changes look fine to me, but I'm CCing Carlos as he's the original author of that code.

[Carlos Garcia Campos]

Looks good to me too, I shoulnd't have used Gtk headers in soup file, sorry.

[Gyuyoung Kim]

Comment on attachment 144572 [details]
Patch

Though I don't like to mix multiple patches personally, informal rs+ on my side in order to build EFL WebKit2 now. But, I think this patch should be reviewed by GTK or WK2 reviewers.

[Carlos Garcia Campos]

Comment on attachment 144572 [details]
Patch

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

Looks good, just a couple of things you can change before landing.

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:66
> +#if ENABLE(INPUT_TYPE_COLOR)
> +#include "ColorChooser.h"
> +#endif
> +

Use #include <WebCore/ColorChooser.h> like all other webcore headers in this file. You don't need the #if ENABLE(INPUT_TYPE_COLOR) because the header is already protected by that check. Remember to move the header to keep all headers sorted alpahbetically

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:78
> +#if ENABLE(WEB_INTENTS)
> +#include "IntentRequest.h"
> +#endif

And same here for IntentRequest.h

[Ryuan Choi]

Created attachment 144749 [details]
patch for landing

[Ryuan Choi]

Comment on attachment 144749 [details]
patch for landing

Thank you so much.

[Build Bot]

Comment on attachment 144749 [details]
patch for landing

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

[Build Bot]

Comment on attachment 144749 [details]
patch for landing

Attachment 144749 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12845820

[Ryuan Choi]

Created attachment 144765 [details]
patch for landing2

[WebKit Review Bot]

Comment on attachment 144765 [details]
patch for landing2

Clearing flags on attachment: 144765

Committed r118914: <http://trac.webkit.org/changeset/118914>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.