Bug 125182 – [GTK][WK2] Fix WebKit2 build after API::Client changes

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: New Bugs

[Nick Diego Yamane (diegoyam)]

[GTK][EFL][WK2] Fix WebKit2 build after r160028

[Nick Diego Yamane (diegoyam)]

Created attachment 218338 [details]
Patch

[Nick Diego Yamane (diegoyam)]

It's still failing due to changes from bug 125178.

../../Source/WebKit2/Shared/API/c/WKDeclarationSpecifiers.h:49:48: error: missing binary operator before token "("
 #if defined(__has_extension) && __has_extension(enumerator_attributes) && __has_extension(attribute_unavailable_with_message)
                                                ^
../../Source/WebKit2/Shared/API/c/WKDeclarationSpecifiers.h:55:48: error: missing binary operator before token "("
 #if defined(__has_extension) && __has_extension(enumerator_attributes) && __has_extension(attribute_unavailable_with_message)
                                                ^

[Anders Carlsson]

You can probably try something like

#ifndef __has_extension
#define __has_extension(x) 0
#endif

[Nick Diego Yamane (diegoyam)]

(In reply to comment #3)
> You can probably try something like
> 
> #ifndef __has_extension
> #define __has_extension(x) 0
> #endif

From bug 116352, it looks like we should use "__has_feature" instead of "__has_extension". So should we add #ifndef to the WKDeclarationSpecifiers.h or to use "__has_feature" which already is properly handled in Compiler.h ?

[Anders Carlsson]

(In reply to comment #4)
> (In reply to comment #3)
> > You can probably try something like
> > 
> > #ifndef __has_extension
> > #define __has_extension(x) 0
> > #endif
> 
> From bug 116352, it looks like we should use "__has_feature" instead of "__has_extension". So should we add #ifndef to the WKDeclarationSpecifiers.h or to use "__has_feature" which already is properly handled in Compiler.h ?

This is in the API headers so we can’t use ifdefs from Compiler.h

[Nick Diego Yamane (diegoyam)]

Created attachment 218349 [details]
Patch

Also adding fix after r160036

[Nick Diego Yamane (diegoyam)]

Created attachment 218354 [details]
Patch

Renaming APIClient to API::Client

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

(In reply to comment #7)
> Created an attachment (id=218354) [details]
> Patch
> 
> Renaming APIClient to API::Client

EFL build is still broken with this patch, some error log:

/home/oszi/WebKit/Source/WebKit2/UIProcess/efl/WebUIPopupMenuClient.h:41:112: error: wrong number of template arguments (2, should be 1)
/home/oszi/WebKit/Source/WebKit2/Shared/APIClient.h:36:42: error: provided for 'template<class ClientInterface> class API::Client'
...

[Csaba Osztrogonác]

It seems you will need similar changes to http://trac.webkit.org/changeset/159988

[Thiago de Barros Lacerda]

Comment on attachment 218354 [details]
Patch

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

> Source/WebKit2/Shared/API/c/WKDeclarationSpecifiers.h:49
> +#ifndef __has_extension

Hey nick, maybe would be cleaner to do some workaround like I did in https://bugs.webkit.org/show_bug.cgi?id=125187, instead of adding some code that does nothing.
What do you think? Ossy?

[Nick Diego Yamane (diegoyam)]

(In reply to comment #9)
> (In reply to comment #7)
> > Created an attachment (id=218354) [details] [details]
> > Patch
> > 
> > Renaming APIClient to API::Client
> 
> EFL build is still broken with this patch, some error log:
> 
> /home/oszi/WebKit/Source/WebKit2/UIProcess/efl/WebUIPopupMenuClient.h:41:112: error: wrong number of template arguments (2, should be 1)
> /home/oszi/WebKit/Source/WebKit2/Shared/APIClient.h:36:42: error: provided for 'template<class ClientInterface> class API::Client'
> ...

Yeah, the changes needed are less trivial I thought initially. ClientTraits was reimplemented..

[Nick Diego Yamane (diegoyam)]

(In reply to comment #10)
> It seems you will need similar changes to http://trac.webkit.org/changeset/159988

Hmm, I'll take a look at it. thanks.

[Nick Diego Yamane (diegoyam)]

(In reply to comment #11)
> (From update of attachment 218354 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218354&action=review
> 
> > Source/WebKit2/Shared/API/c/WKDeclarationSpecifiers.h:49
> > +#ifndef __has_extension
> 
> Hey nick, maybe would be cleaner to do some workaround like I did in https://bugs.webkit.org/show_bug.cgi?id=125187, instead of adding some code that does nothing.
> What do you think? Ossy?

Hmm, good. But that's still not enough to solve the build issues :)

[Nick Diego Yamane (diegoyam)]

(In reply to comment #13)
> (In reply to comment #10)
> > It seems you will need similar changes to http://trac.webkit.org/changeset/159988
> 
> Hmm, I'll take a look at it. thanks.

Hey Ossy, you're right. We'll have to do similar things to that patch to get it working!

So we'll need lots of changes to get the following classes working according to the new API::Client implementation:

* WebBatteryProvider
* WebNetworkInfoProvider
* WebTextCheckerClient
* WebVibrationProvider
* efl/WebUIPopupMenuClient
* gtk/WebFullScreenClientGtk
* gtk/WebInspectorClientGtk
* soup/WebSoupRequestManagerClient

[Thiago de Barros Lacerda]

(In reply to comment #14)
> (In reply to comment #11)
> > (From update of attachment 218354 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=218354&action=review
> > 
> > > Source/WebKit2/Shared/API/c/WKDeclarationSpecifiers.h:49
> > > +#ifndef __has_extension
> > 
> > Hey nick, maybe would be cleaner to do some workaround like I did in https://bugs.webkit.org/show_bug.cgi?id=125187, instead of adding some code that does nothing.
> > What do you think? Ossy?
> 
> Hmm, good. But that's still not enough to solve the build issues :)

I know, it is just for that specific part of the code

[Nick Diego Yamane (diegoyam)]

Created attachment 218373 [details]
Patch

Moving to the new API::Client

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Nick Diego Yamane (diegoyam)]

FYI, EFL will be addressed in bug 125206.

[Nick Diego Yamane (diegoyam)]

Comment on attachment 218373 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitFullscreenClient.cpp:48
> +    WKViewSetFullScreenClientGtk(toAPI(WEBKIT_WEB_VIEW_BASE(webView)), reinterpret_cast<WKFullScreenClientGtkBase*>(&wkFullScreenClient));

I'm not sure if that's the right thing to do here :(

@andersca, could you help us on this?

> Source/WebKit2/UIProcess/API/gtk/WebKitTextChecker.cpp:110
> +    WKTextCheckerSetClient(reinterpret_cast<WKTextCheckerClientBase*>(&wkTextCheckerClient));

ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebInspector.cpp:332
> +    WKInspectorSetInspectorClientGtk(toAPI(webInspector), reinterpret_cast<WKInspectorClientGtkBase*>(&wkInspectorClientGtk));

ditto.

[Csaba Osztrogonác]

Comment on attachment 218373 [details]
Patch

The build is still broken with this patch :(

[Csaba Osztrogonác]

(In reply to comment #19)
> FYI, EFL will be addressed in bug 125206.

What happened? This bug was to fix EFL and GTK too originally.
Why do we have two separated bug report now? And there isn't
any patch with r? in the other bug report. :(

[Gustavo Noronha (kov)]

This seems to have been made obsolete by the patch on the other bug.

*** This bug has been marked as a duplicate of bug 125206 ***

[Anders Carlsson]

Comment on attachment 218373 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/gtk/WebKitFullscreenClient.cpp:48
>> +    WKViewSetFullScreenClientGtk(toAPI(WEBKIT_WEB_VIEW_BASE(webView)), reinterpret_cast<WKFullScreenClientGtkBase*>(&wkFullScreenClient));
> 
> I'm not sure if that's the right thing to do here :(
> 
> @andersca, could you help us on this?

This should just be &wkFullScreenClient.base.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebInspector.cpp:332
>> +    WKInspectorSetInspectorClientGtk(toAPI(webInspector), reinterpret_cast<WKInspectorClientGtkBase*>(&wkInspectorClientGtk));
> 
> ditto.

Same thing here.

[Nick Diego Yamane (diegoyam)]

(In reply to comment #24)
> (From update of attachment 218373 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218373&action=review
> 
> >> Source/WebKit2/UIProcess/API/gtk/WebKitFullscreenClient.cpp:48
> >> +    WKViewSetFullScreenClientGtk(toAPI(WEBKIT_WEB_VIEW_BASE(webView)), reinterpret_cast<WKFullScreenClientGtkBase*>(&wkFullScreenClient));
> > 
> > I'm not sure if that's the right thing to do here :(
> > 
> > @andersca, could you help us on this?
> 
> This should just be &wkFullScreenClient.base.
> 
> >> Source/WebKit2/UIProcess/API/gtk/WebKitWebInspector.cpp:332
> >> +    WKInspectorSetInspectorClientGtk(toAPI(webInspector), reinterpret_cast<WKInspectorClientGtkBase*>(&wkInspectorClientGtk));
> > 
> > ditto.
> 
> Same thing here.

Ok, thanks.
It will be fixed in bug 125231.