Bug 191906 - [WPE] Use new view state API from libwpe
Summary: [WPE] Use new view state API from libwpe
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WPE WebKit (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 192224
  Show dependency treegraph
 
Reported: 2018-11-22 05:58 PST by Carlos Garcia Campos
Modified: 2018-12-14 04:57 PST (History)
6 users (show)

See Also:


Attachments
Patch (12.85 KB, patch)
2018-11-22 06:06 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (13.66 KB, patch)
2018-11-27 03:05 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (16.44 KB, patch)
2018-11-28 02:13 PST, Carlos Garcia Campos
zan: review+
Details | Formatted Diff | Diff
Patch for landing (17.22 KB, patch)
2018-12-14 01:18 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2018-11-22 05:58:58 PST
[WPE] Use new view state API from libwpe
Comment 1 Carlos Garcia Campos 2018-11-22 06:06:33 PST
Created attachment 355469 [details]
Patch
Comment 2 Michael Catanzaro 2018-11-23 09:01:57 PST
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.
Comment 3 Carlos Garcia Campos 2018-11-27 03:05:27 PST
Created attachment 355719 [details]
Patch
Comment 4 Carlos Garcia Campos 2018-11-27 03:19:55 PST
(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
Comment 5 Michael Catanzaro 2018-11-27 07:14:29 PST
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.
Comment 6 Carlos Garcia Campos 2018-11-27 08:00:00 PST
(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.
Comment 7 Michael Catanzaro 2018-11-27 09:04:30 PST
(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.
Comment 8 Carlos Garcia Campos 2018-11-28 00:21:50 PST
(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
Comment 9 Carlos Garcia Campos 2018-11-28 02:13:42 PST
Created attachment 355859 [details]
Patch
Comment 10 Michael Catanzaro 2018-11-28 10:19:34 PST
OK, looks fine to me. I guess Zan will probably want to review it too.
Comment 11 Carlos Garcia Campos 2018-12-14 01:18:49 PST
Created attachment 357307 [details]
Patch for landing
Comment 12 Carlos Garcia Campos 2018-12-14 04:57:40 PST
Committed r239203: <https://trac.webkit.org/changeset/239203>