RESOLVED WONTFIX 240052
[WPE] Enable process swap on navigation
https://bugs.webkit.org/show_bug.cgi?id=240052
Summary [WPE] Enable process swap on navigation
Yury Semikhatsky
Reported 2022-05-03 20:17:28 PDT
It is currently available only in GTK but we'd like to use it in WPE port too.
Attachments
Patch (6.99 KB, patch)
2022-05-03 20:26 PDT, Yury Semikhatsky
no flags
Patch (6.50 KB, patch)
2022-05-04 09:28 PDT, Yury Semikhatsky
no flags
Patch (1.72 KB, patch)
2022-05-04 10:29 PDT, Yury Semikhatsky
cgarcia: review-
Yury Semikhatsky
Comment 1 2022-05-03 20:26:34 PDT
Michael Catanzaro
Comment 2 2022-05-04 08:07:22 PDT
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.
Michael Catanzaro
Comment 3 2022-05-04 09:17:29 PDT
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. ;)
Yury Semikhatsky
Comment 4 2022-05-04 09:26:44 PDT
(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.
Yury Semikhatsky
Comment 5 2022-05-04 09:28:40 PDT
Yury Semikhatsky
Comment 6 2022-05-04 09:29:08 PDT
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.
Michael Catanzaro
Comment 7 2022-05-04 10:02:33 PDT
(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?
Yury Semikhatsky
Comment 8 2022-05-04 10:20:29 PDT
(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.
Yury Semikhatsky
Comment 9 2022-05-04 10:29:05 PDT
Michael Catanzaro
Comment 10 2022-05-04 11:42:53 PDT
Looks good. We need an opinion from Adrian on whether this justifies bumping the WPE API version. I would say it does.
Michael Catanzaro
Comment 11 2022-05-04 13:32:48 PDT
(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.
Yury Semikhatsky
Comment 12 2022-05-04 14:59:42 PDT
(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)?
Michael Catanzaro
Comment 13 2022-05-04 15:34:37 PDT
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.
Carlos Garcia Campos
Comment 14 2022-05-05 01:15:02 PDT
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.
Yury Semikhatsky
Comment 15 2022-05-05 10:12:32 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.