Bug 184379 – [WPE] Move libWPEWebInspectorResources.so to pkglibdir

• Component: WPE WebKit
• Status: RESOLVED FIXED

[Michael Catanzaro]

libWPEWebInspectorResources.so is installed into libdir, which is probably OK, but:

 * It's not referenced in any pkg-config file, so it would be inappropriate for applications to link to it directly
 * The only use inside WebKit is in RemoteInspectorUtils.cpp, where it is dlopened

If it's only ever dlopened, it should probably go into pkglibdir instead, to get it out of the linker search path. But this makes me question how the web inspector is intended to work by default. How can it work if it's not linked to its resources? Is only remote inspector supported? Why is this so different from WebKitGTK+?

One way or another, we need to ensure it is API-versioned, so if we do not move it to pkglibdir, we should append the API version to the library name.

[Adrian Perez]

(In reply to Michael Catanzaro from comment #0)
> libWPEWebInspectorResources.so is installed into libdir, which is probably
> OK, but:
> 
>  * It's not referenced in any pkg-config file, so it would be inappropriate
> for applications to link to it directly
>  * The only use inside WebKit is in RemoteInspectorUtils.cpp, where it is
> dlopened
> 
> If it's only ever dlopened, it should probably go into pkglibdir instead, to
> get it out of the linker search path. But this makes me question how the web
> inspector is intended to work by default. How can it work if it's not linked
> to its resources? Is only remote inspector supported? Why is this so
> different from WebKitGTK+?

Yup, pkglibdir is probably a good option. If it's going to be installed
alongside “WPEWebProcess” and friends, I would also make it possible to
override the path where it is searched for using “WEBKIT_EXEC_PATH”.

The inspector only ever needs reading resources when it's in use. While
not in use, it does not matter that the library is not loaded. This saves
around 2,5 MiB of RAM, and in very constrained environments it is possible
to skip shipping it — the inspector just won't work. I could argue that
WebKitGTK+ should follow suit and also ship a separate resources file.

What I don't understand is why using a shared object instead of a
“.gresource” file, and loading it with “g_resource_load()” followed
by a call to “g_resources_register()” — which uses “mmap()” under the
hood.

> One way or another, we need to ensure it is API-versioned, so if we do not
> move it to pkglibdir, we should append the API version to the library name.

Agreed. I would move it into pkglibdir.

[Michael Catanzaro]

(In reply to Adrian Perez from comment #1)
> Yup, pkglibdir is probably a good option. If it's going to be installed
> alongside “WPEWebProcess” and friends, I would also make it possible to
> override the path where it is searched for using “WEBKIT_EXEC_PATH”.

No, this is a library, not an executable. I suggested moving the executables into pkglibexecdir, not pkglibdir. (On Debian, pkglibexecdir uses non-multiarch /usr/lib. And Fedora has separate /usr/libexecdir.)

> The inspector only ever needs reading resources when it's in use. While
> not in use, it does not matter that the library is not loaded. This saves
> around 2,5 MiB of RAM, and in very constrained environments it is possible
> to skip shipping it — the inspector just won't work. I could argue that
> WebKitGTK+ should follow suit and also ship a separate resources file.

Seems like really small beans to me, but OK.

> What I don't understand is why using a shared object instead of a
> “.gresource” file, and loading it with “g_resource_load()” followed
> by a call to “g_resources_register()” — which uses “mmap()” under the
> hood.

It's an implementation detail, we can change it later.

> > One way or another, we need to ensure it is API-versioned, so if we do not
> > move it to pkglibdir, we should append the API version to the library name.
> 
> Agreed. I would move it into pkglibdir.

Yeah, let's move it to pkglibdir. It will be happy there.

[Michael Catanzaro]

Well, I guess my other concern here is that normal web inspector is probably broken in WPE, because there's no mechanism to load libWPEWebInspectorResources.so except in RemoteInspectorUtils.cpp. So I guess the usual case of local inspector must be broken. Then again, without support for context menus, I guess there's probably no way to open local inspector anyway, so whatever.

[Michael Catanzaro]

Created attachment 337667 [details]
Patch

[Michael Catanzaro]

This is not tested at all. Could someone who knows how to use remote inspector check it, please?

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #2)
> (In reply to Adrian Perez from comment #1)
> > Yup, pkglibdir is probably a good option. If it's going to be installed
> > alongside “WPEWebProcess” and friends, I would also make it possible to
> > override the path where it is searched for using “WEBKIT_EXEC_PATH”.
> 
> No, this is a library, not an executable. I suggested moving the executables
> into pkglibexecdir, not pkglibdir. (On Debian, pkglibexecdir uses
> non-multiarch /usr/lib. And Fedora has separate /usr/libexecdir.)
> 
> > The inspector only ever needs reading resources when it's in use. While
> > not in use, it does not matter that the library is not loaded. This saves
> > around 2,5 MiB of RAM, and in very constrained environments it is possible
> > to skip shipping it — the inspector just won't work. I could argue that
> > WebKitGTK+ should follow suit and also ship a separate resources file.
> 
> Seems like really small beans to me, but OK.

It's super-convenient to know that resources are always available, no matter if your ran make install or not. But, I agree it could be useful or even required in some embedded systems, so we could add it to WebKitGTK+ as a build option disabled by default, for example.

> > What I don't understand is why using a shared object instead of a
> > “.gresource” file, and loading it with “g_resource_load()” followed
> > by a call to “g_resources_register()” — which uses “mmap()” under the
> > hood.
> 
> It's an implementation detail, we can change it later.

I also think this is a good opportunity to change that, I never understood why this was a shared lib either.

> > > One way or another, we need to ensure it is API-versioned, so if we do not
> > > move it to pkglibdir, we should append the API version to the library name.
> > 
> > Agreed. I would move it into pkglibdir.
> 
> Yeah, let's move it to pkglibdir. It will be happy there.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #6)
> It's super-convenient to know that resources are always available, no matter
> if your ran make install or not. But, I agree it could be useful or even
> required in some embedded systems, so we could add it to WebKitGTK+ as a
> build option disabled by default, for example.

Let's not, unless we really need it.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #6)
> > > What I don't understand is why using a shared object instead of a
> > > “.gresource” file, and loading it with “g_resource_load()” followed
> > > by a call to “g_resources_register()” — which uses “mmap()” under the
> > > hood.
> > 
> > It's an implementation detail, we can change it later.
> 
> I also think this is a good opportunity to change that, I never understood
> why this was a shared lib either.

I'm kinda backlogged now, can you handle this if you want to change it now? Otherwise, let's just move it to pkglibdir for now, and change it later.

[Zan Dobersek]

Comment on attachment 337667 [details]
Patch

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

> Source/JavaScriptCore/PlatformWPE.cmake:36
> +add_definitions(-DPKGLIBDIR="${CMAKE_INSTALL_FULL_LIBDIR}/wpe-webkit-${WPE_API_VERSION})

Missing the ending " here.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #5)
> This is not tested at all. Could someone who knows how to use remote
> inspector check it, please?

Wasn't kidding :P

[Michael Catanzaro]

Created attachment 337798 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 337798 [details]
Patch

Clearing flags on attachment: 337798

Committed r230585: <https://trac.webkit.org/changeset/230585>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.