Created attachment 348582 [details] [review]
portalHelper: Simplify our non-use of cache

Instead of using directories that we'll destroy when done, use the new
"ephemeral" data manager feature, through the JavaScript version of:
webkit_web_context_new_ephemeral()

Michael told me to use "Gi.require_version" but I don't know how to use that, and couldn't find any examples. We'd need WebKitGTK+ 2.16.

(In reply to Bastien Nocera from comment #2)
> Michael told me to use "Gi.require_version" but I don't know how to use
> that

You can only require the API/Gir version (that is, "4.0" for WebKit2). See bug 779593 for improving on that next cycle.

Ah well, something like

  AX_CHECK_GIR_SYMBOLS_GJS([WebKit2], [4.0], [WebContext.new_ephemeral])

in configure.ac should work, though we currently don't do any checks for runtime dependencies at all (which is arguably correct, as not having runtime dependencies on a build server is perfectly fine, as long as they are available on the end user system)

I'm fine with skipping this part then.

Review of attachment 348582 [details] [review]:

It should definitely be a runtime check and not a buildtime check. But:

ptomato[m]:  as Florian said in the bug, typelibs don't provide versions
ptomato[m]:  simplest solution is to just let the method call throw if it doesn't exist :-)
ptomato[m]:  but you can also check if the method exists and exit with a more coherent error message if not, which is what I'd recommend instead

so a runtime check is not simple, I would just not bother. No distributions are going to ship new GNOME with old WebKit and it's their fault if they try.

Like I said on IRC, this change isn't a problem for Ubuntu because 17.04 will ship with webkit 2.16.

I think it would be nice to distros to at least mention the increased runtime requirement in the NEWS file, especially since it sounds like you are wanting to land this in 3.24.1. That way distros will set the runtime dependency correctly.

(In reply to Michael Catanzaro from comment #6)
> so a runtime check is not simple

What Philip is suggesting isn't super-hard either, something like:

  if (!WebKit.WebContext.new_ephemeral) {
      log('WebKit is too old');
      return 1;
  }

in main() should do ...

(In reply to Florian Müllner from comment #8)
> What Philip is suggesting isn't super-hard either, something like:
> 
>   if (!WebKit.WebContext.new_ephemeral) {
>       log('WebKit is too old');
>       return 1;
>   }
> 
> in main() should do ...

I happened to look at the portal helper code recently and saw the WebKit 2.16 release notes so I thought of fixing this same thing, then found this bug.

I would even suggest doing the runtime test above when creating the WebView and keeping the fallback code for a release cycle (so until gnome 3.26). It's not like it's lots of code to maintain, otherwise users might get bitten by this if a distribution doesn't notice the new requirement. It's definitely in not very exercised code so it's not like you'll immediately notice large breakage unless you use the portal helper.

This kind of attention to detail and reasonable backwards compatibility is what makes the difference between god and great software even though it adds a bit to the developer burden.

Will tweak your patch soon to show what I mean.

Created attachment 348813 [details] [review]
Alternative take - use the new API but keep compatibility

Here's the promised patch.

There's even a developer advantage to doing it like this: it allows people building and testing the newest gnome-shell and portal helper and rely on the distro packaged WebKitGtk therefore not needing a build that takes a very long time. I've been tweaking and toying with the portal helper code and if this would be committed I would rather hack it locally to avoid the new function rather than waiting for WebKit to build.

Created attachment 348883 [details] [review]
portalHelper: Simplify our non-use of cache

Instead of using directories that we'll destroy when done, use the new
"ephemeral" data manager feature, through the JavaScript version of:
webkit_web_context_new_ephemeral()

We also throw an error on startup, in the logs, if WebKitGTK is too old.

Review of attachment 348883 [details] [review]:

LGTM

Pushed to gnome-3-24. As for the requirement for a newer WebKitGTK that might not be in distros, the intersection between people that compile their own gnome-shell, can't revert this patch, and can't install newer versions of WebKitGTK from their distributions' unstable repository is probably close to zero, so really not worth adding fallback code for.

Attachment 348883 [details] pushed as 6557ae0 - portalHelper: Simplify our non-use of cache