Bug 191906 – [WPE] Use new view state API from libwpe

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

[Carlos Garcia Campos]

[WPE] Use new view state API from libwpe

[Carlos Garcia Campos]

Created attachment 355469 [details]
Patch

[Michael Catanzaro]

Comment on attachment 355469 [details]
Patch

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

> Source/WebKit/UIProcess/API/wpe/WPEView.cpp:48
> +#if !defined(WPE_BACKEND_CHECK_VERSION) || !WPE_BACKEND_CHECK_VERSION(1, 1, 0)

It should be renamed to LIBWPE_CHECK_VERSION.

[Carlos Garcia Campos]

Created attachment 355719 [details]
Patch

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #2)
> Comment on attachment 355469 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355469&action=review
> 
> > Source/WebKit/UIProcess/API/wpe/WPEView.cpp:48
> > +#if !defined(WPE_BACKEND_CHECK_VERSION) || !WPE_BACKEND_CHECK_VERSION(1, 1, 0)
> 
> It should be renamed to LIBWPE_CHECK_VERSION.

https://github.com/WebPlatformForEmbedded/libwpe/pull/38

[Michael Catanzaro]

Comment on attachment 355719 [details]
Patch

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

> Source/WebKit/UIProcess/API/C/wpe/WKView.h:49
> +// Deprecated: Use wpe_view_backend_add_activity_state instead.
>  WK_EXPORT void WKViewSetViewState(WKViewRef, WKViewState);

I don't understand this. This API is not public, so it should be removed, not deprecated, right? And I can see it's not used anywhere, so that should be trivial.

> Source/WebKit/UIProcess/API/wpe/WPEView.cpp:48
> +#if !defined(WPE_BACKEND_CHECK_VERSION) || !WPE_BACKEND_CHECK_VERSION(1, 1, 0)

Again, it should be renamed to WPE_CHECK_VERSION (or LIBWPE_CHECK_VERSION) and instead of checking whether it's defined or not, we should just require that version to build in order to avoid the extra check. It's not appropriate for the macro to be named WPE_BACKEND_CHECK_VERSION because it checks the version of libwpe, not the version of the WPE backend.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #5)
> Comment on attachment 355719 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355719&action=review
> 
> > Source/WebKit/UIProcess/API/C/wpe/WKView.h:49
> > +// Deprecated: Use wpe_view_backend_add_activity_state instead.
> >  WK_EXPORT void WKViewSetViewState(WKViewRef, WKViewState);
> 
> I don't understand this. This API is not public, so it should be removed,
> not deprecated, right? And I can see it's not used anywhere, so that should
> be trivial.

It's public C API. It's not used inside WebKit itself, but used by applications. I agree we can just remove it, but once we bump the libwpe requirement.

> > Source/WebKit/UIProcess/API/wpe/WPEView.cpp:48
> > +#if !defined(WPE_BACKEND_CHECK_VERSION) || !WPE_BACKEND_CHECK_VERSION(1, 1, 0)
> 
> Again, it should be renamed to WPE_CHECK_VERSION (or LIBWPE_CHECK_VERSION)
> and instead of checking whether it's defined or not, we should just require
> that version to build in order to avoid the extra check. It's not
> appropriate for the macro to be named WPE_BACKEND_CHECK_VERSION because it
> checks the version of libwpe, not the version of the WPE backend.

wpe backend was renamed as libwpe, is the same thing. I find it weird the lib prefix, the same way there's no LIBGTK_CHECK_VERSION nor LIBSOUP_CHECK_VERSION. The prefix used in all public APIs in libwpe is wpe, not libwpe.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #6) 
> It's public C API. It's not used inside WebKit itself, but used by
> applications. I agree we can just remove it, but once we bump the libwpe
> requirement.

No it's not. That header is not installed, and the symbols are not exported, so it's impossible for applications to use it. I know it's still available in the downstream port, but this is upstream.

> wpe backend was renamed as libwpe, is the same thing. I find it weird the
> lib prefix, the same way there's no LIBGTK_CHECK_VERSION nor
> LIBSOUP_CHECK_VERSION. The prefix used in all public APIs in libwpe is wpe,
> not libwpe.

So then rename it to WPE_CHECK_VERSION. WPE_BACKEND_CHECK_VERSION is too legacy, too confusing. I know it requires another libwpe API version bump, but the cost of that is very low right now. It will only get harder in the future.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #7)
> (In reply to Carlos Garcia Campos from comment #6) 
> > It's public C API. It's not used inside WebKit itself, but used by
> > applications. I agree we can just remove it, but once we bump the libwpe
> > requirement.
> 
> No it's not. That header is not installed, and the symbols are not exported,
> so it's impossible for applications to use it. I know it's still available
> in the downstream port, but this is upstream.

Ok, I'll remove it then.

> > wpe backend was renamed as libwpe, is the same thing. I find it weird the
> > lib prefix, the same way there's no LIBGTK_CHECK_VERSION nor
> > LIBSOUP_CHECK_VERSION. The prefix used in all public APIs in libwpe is wpe,
> > not libwpe.
> 
> So then rename it to WPE_CHECK_VERSION. WPE_BACKEND_CHECK_VERSION is too
> legacy, too confusing. I know it requires another libwpe API version bump,
> but the cost of that is very low right now. It will only get harder in the
> future.

That's exactly what I did, I guess you didn't click the link in comment #4

[Carlos Garcia Campos]

Created attachment 355859 [details]
Patch

[Michael Catanzaro]

OK, looks fine to me. I guess Zan will probably want to review it too.

[Carlos Garcia Campos]

Created attachment 357307 [details]
Patch for landing

[Carlos Garcia Campos]

Committed r239203: <https://trac.webkit.org/changeset/239203>