Bug 240052

Created attachment 458770 [details]
Patch

Hmmm, please see bug #220116. I wonder: are you trying to *enable* PSON, or *disable* it? I thought it was always enabled for WPE, but could be wrong.

Comment on attachment 458770 [details]
Patch

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

I suppose I'm OK with these changes, but I'll ask Carlos Garcia and Adrian for opinions in bug #220116. My worry is that if PSON becomes mandatory in the future, the option to disable it now could set apps up for breakage later.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:-322
> -#if ENABLE(DEVELOPER_MODE)
>      const char* bundleDirectory = g_getenv("WEBKIT_INJECTED_BUNDLE_PATH");
>      if (bundleDirectory && g_file_test(bundleDirectory, G_FILE_TEST_IS_DIR))
>          return bundleDirectory;
> -#endif

I'm OK with removing this, but definitely not in this patch. ;)

(In reply to Michael Catanzaro from comment #2)
> Hmmm, please see bug #220116. I wonder: are you trying to *enable* PSON, or
> *disable* it? I thought it was always enabled for WPE, but could be wrong.

We need it to *enable* PSON, I believe previously it was off by default, perhaps it has changed since then.

Created attachment 458804 [details]
Patch

Comment on attachment 458770 [details]
Patch

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

>> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:-322
>> -#endif
> 
> I'm OK with removing this, but definitely not in this patch. ;)

Oops, reverted.

(In reply to Yury Semikhatsky from comment #4)
> We need it to *enable* PSON, I believe previously it was off by default,
> perhaps it has changed since then.

PSON ought to be enabled by default for WPE in order to match our future default for GTK. This setting should provide an off switch, not an on switch.

Since you don't need to disable it, maybe it would be better to not provide the setting at all. If nobody asks for it, then we don't have to support it or worrying about it breaking in the future. How about a patch to enable PSON unconditionally for WPE?

(In reply to Michael Catanzaro from comment #7)
> (In reply to Yury Semikhatsky from comment #4)
> > We need it to *enable* PSON, I believe previously it was off by default,
> > perhaps it has changed since then.
> 
> PSON ought to be enabled by default for WPE in order to match our future
> default for GTK. This setting should provide an off switch, not an on switch.
> 
> Since you don't need to disable it, maybe it would be better to not provide
> the setting at all. If nobody asks for it, then we don't have to support it
> or worrying about it breaking in the future. How about a patch to enable
> PSON unconditionally for WPE?

That will work for us. I've just checked that processPool.configuration().processSwapsOnNavigation() returns false in WPE by default. Let me prepare a patch that updates that behavior.

Created attachment 458807 [details]
Patch

Looks good.

We need an opinion from Adrian on whether this justifies bumping the WPE API version. I would say it does.

(In reply to Michael Catanzaro from comment #10)
> We need an opinion from Adrian on whether this justifies bumping the WPE API
> version. I would say it does.

After some discussion, Adrian and I agreed on this: bump WPE API version 1.1 (libsoup 3) -> 1.2 and enable PSON only there. PSON is a big API break, and this will be the first of many API breaks, so we need a new API version.

Keep it off in the WPE API version 1.0 (libsoup 2) for now, to avoid breaking anything that depends on PSON being off. This is a conservative choice for now. I think we should be able to quickly remove the 1.0 API, but no agreement to do that yet.

TODO for me: I will provide a patch in another bug to bump the API version, and figure out what guards to use for this. Ideally we'd have just one build guard to be shared by GTK 4 and WPE 1.2, which would toggle between the new and old version.

(In reply to Michael Catanzaro from comment #11)
> (In reply to Michael Catanzaro from comment #10)
> > We need an opinion from Adrian on whether this justifies bumping the WPE API
> > version. I would say it does.
> 
> Keep it off in the WPE API version 1.0 (libsoup 2) for now, to avoid
> breaking anything that depends on PSON being off. This is a conservative
> choice for now. I think we should be able to quickly remove the 1.0 API, but
> no agreement to do that yet.
> 

This versioning logic makes sense. Does it mean that Ubuntu 18.04 and 20.04 (the two linux distros we currently run WPE on) builds will not be able to turn PSON on and will only switch to PSON=on starting Ubuntu 22.04 (which comes with both libsoup 2 and 3)?

Hmm, yes that would probably be a no-go, because you'd have to make sure none of your dependencies use libsoup2.

I suppose that creates a need for this setting after all:

(In reply to Michael Catanzaro from comment #7)
> Since you don't need to disable it, maybe it would be better to not provide
> the setting at all. If nobody asks for it, then we don't have to support it
> or worrying about it breaking in the future. How about a patch to enable
> PSON unconditionally for WPE?

Let's wait a bit longer and see what other devs think.

Comment on attachment 458807 [details]
Patch

PSON is already enabled by default in WPE, we decided not to add the setting because we didn't notice any break in apps using WPE, and we added it to GTK because it broke several apps, to give some more time to adapt them. PSON is enabled by default for WPE and GTK in WebPreferencesExperimental.yaml and that value is used when client doesn't explicitly enable/disable it. So, we don't need to set it to true explicilty.

(In reply to Carlos Garcia Campos from comment #14)
> Comment on attachment 458807 [details]
> Patch
> 
> PSON is already enabled by default in WPE, we decided not to add the setting
> because we didn't notice any break in apps using WPE, and we added it to GTK
> because it broke several apps, to give some more time to adapt them. PSON is
> enabled by default for WPE and GTK in WebPreferencesExperimental.yaml and
> that value is used when client doesn't explicitly enable/disable it. So, we
> don't need to set it to true explicilty.

Makes sense, thanks for explaining. This works for us, closing this bug.