WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(6.50 KB, patch)
2022-05-04 09:28 PDT
,
Yury Semikhatsky
no flags
Details
Formatted Diff
Diff
Patch
(1.72 KB, patch)
2022-05-04 10:29 PDT
,
Yury Semikhatsky
cgarcia
: review-
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Yury Semikhatsky
Comment 1
2022-05-03 20:26:34 PDT
Created
attachment 458770
[details]
Patch
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
Created
attachment 458804
[details]
Patch
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
Created
attachment 458807
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug