| Summary: | [WPE] Move libWPEWebInspectorResources.so to pkglibdir | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||
| Component: | WPE WebKit | Assignee: | Michael Catanzaro <mcatanzaro> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | aperez, bugs-noreply, cgarcia, commit-queue, mcatanzaro, zan | ||||||
| Priority: | P2 | ||||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | PC | ||||||||
| OS: | Linux | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 178894 | ||||||||
| Attachments: |
|
||||||||
|
Description
Michael Catanzaro
2018-04-06 18:17:07 PDT
(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. (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. 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. Created attachment 337667 [details]
Patch
This is not tested at all. Could someone who knows how to use remote inspector check it, please? (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. (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. (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. 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. (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 Created attachment 337798 [details]
Patch
Comment on attachment 337798 [details] Patch Clearing flags on attachment: 337798 Committed r230585: <https://trac.webkit.org/changeset/230585> All reviewed patches have been landed. Closing bug. |