Add pdfjs switch

[msdemlei]

This is an attempt to address bug #1052; I'm (well: my machine is) tired of rebuilding luakit to patch out pdf.js.

What this does now is follow Michael's recipe on https://bugs.webkit.org/show_bug.cgi?id=260019#c4 (comment 4) to unconditionally turn off pdfjs using some ugly custom code in widgets/webview.c; that works on a PoC level.

However, assuming that not everyone is as abhorred by pdf.js as I am, this would now need to be made configurable. I think we will still need C code for this, as webkit_settings_get_all_features does not seem to be exposed to lua (and I don't think it should). But I have not found a way to trick that C code into settings.lua; actually, I have not even worked out how settings.lua talks to webkit in "normal" cases.

Does anyone here still understand settings.lua? Well enough to figure out whether we can have a setting that simply calls a C function on toggling, even?

[c0dev0id]

You don't need to touch settings.lua. You can register a toggle like I have done it when adding dark-mode support here: ea082a5

Except your are registering your toggle in webview.lua and read it from webview.c.
There are already settings being registered in webview.lua. You can add a toggle there.

[msdemlei]

On Tue, Jan 23, 2024 at 02:46:19PM -0800, Stefan Hagen wrote: You don't need to touch settings.lua. You can register a toggle like I have done it when adding dark-mode support here: ea082a5

Thanks for the hints -- that's really helpful, although of course I'm basically blindly prodding along, wondering if there is a place I can read up on all that (well: the lua-C interface I know where to read up on, and I'll do that now). But I've reached a place where I'll need someone with actual knowledge about the interface to webkitgtk: It seems I can't just invent configuration keys in webview.lua, and I wonder if I'm just being dense or whether I really need to do this some other way. You see, webview.lua has a signal handler for init which executes set_all. That, in turn, iterates over webview_settings, drops the constant prefix "webview" and then runs wv[config-key] = somevalue. With my new setting webview.enable_pdfjs, that fails with: Lua error: cannot set unknown webview property 'enable_pdfjs' So...

There are already settings being registered in webview.lua. You can add a toggle there.

Is it actually right to just add something in webview_settings or do these have to reflect properties of the C-side object? And if I can bother you to have a quick look at the last commit: Does the rest look as if it could work?

[msdemlei]

So, I'm now creating an application.enable_pdfjs setting in webview.lua, which is the first application setting in there. To keep that out of libwebkitgtk's view, I'm now doing prefix checking in the init signal handler.

I hope this is ready for review now. One thing someone knowledgeable should definitely look at: do I need to free any of what comes back from the webkit calls in luaH_webview_set_pdfjs except for the result of webkit_settings_get_all_features (where the docs say I own it)? And is it ok to use g_autoptr in luakit? Glib docs say it's only available in gcc and clang – is this something we want to avoid?

[c0dev0id]

I'm a bit torn weather we should enable or disable it per default.

Without your PR, webkit renders PDFs itself. So your PR changes this behavior.

But before webkit started display PDFs, a download dialog was shown and we had the viewpdf module to handle it.

I tend to enabling pdfjs per default and adding a sentence to the viewpdf documentation that it requires this setting to be toggled off to work.

[c0dev0id]

So... it looks like this is the first setting that's retrieved from the webview, but it's actually an application setting. My first thought was that this should go into window.lua/window.c. But then we need to include webview there, which is wrong. Therefore I think this is ok.

[c0dev0id]

do I need to free any of what comes back from the webkit calls in luaH_webview_set_pdfjs except for the result of webkit_settings_get_all_features

I'm not sure. The memory leak unit test does not complain, so I assume it's okay as it is.

g_autoptr

We can try it. And we change it in case it creates trouble.

[msdemlei]

On Tue, Feb 06, 2024 at 02:11:40AM -0800, Stefan Hagen wrote: I tend to enabling pdfjs per default and adding a sentence to the viewpdf documentation that it requires this setting to be toggled off to work.

Ah... well. I *would* have bet money that the clientele that uses luakit would howl about their PDFs not opening in their beloved zathura (or whatever), but since nobody but me gnashed their teeth I'm willing to accept that I may be slightly odd. So, in aca1ee5 I'm turning pdfjs on by default. I'm also making a brief note in the changelog in that commit. For now I can't think of anything else to do here, so from where I stand this looks ready to go. Thanks for the review!