Summary: | [WPE] Use new view state API from libwpe | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||||||
Component: | WPE WebKit | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | aperez, bugs-noreply, clopez, ews-watchlist, mcatanzaro, zan | ||||||||||
Priority: | P2 | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
See Also: | https://github.com/WebPlatformForEmbedded/libwpe/pull/36 | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 192224 | ||||||||||||
Attachments: |
|
Description
Carlos Garcia Campos
2018-11-22 05:58:58 PST
Created attachment 355469 [details]
Patch
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. Created attachment 355719 [details]
Patch
(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 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. (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. (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. (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 Created attachment 355859 [details]
Patch
OK, looks fine to me. I guess Zan will probably want to review it too. Created attachment 357307 [details]
Patch for landing
Committed r239203: <https://trac.webkit.org/changeset/239203> |