Skip to content

Add configuration option to allow moving window to background when window.close is called#1115

Merged
magomez merged 1 commit intoWebPlatformForEmbedded:wpe-2.38from
LibertyGlobal:jgocol/ONEM-31242-upstream
Jul 10, 2023
Merged

Add configuration option to allow moving window to background when window.close is called#1115
magomez merged 1 commit intoWebPlatformForEmbedded:wpe-2.38from
LibertyGlobal:jgocol/ONEM-31242-upstream

Conversation

@jakub-gocol-red
Copy link
Copy Markdown

When allow_scripts_to_close_windows property is set to true, scripts can close windows that are not opened by them. But after window.close API is called, page is in "is closing" and browser expects window to be closed. When integrator wants to hide browser window/suspend browser, instead of closing it, page will be stuck in "is closing" state, where some APIs don't work anymore.

This change introduces new property allow_move_to_suspend_on_window_close (disabled by default) which removes setting "is closing" state and just sends notification to browser integration, without any expectations on how it's handled.

…ndow.close is called

When allow_scripts_to_close_windows property is set to true, scripts can
close windows that are not opened by them. But after window.close API is
called, page is in "is closing" and browser expects window to be closed.
When integrator wants to hide browser window/suspend browser, instead of
closing it, page will be stuck in "is closing" state, where some APIs
don't work anymore.

This change introduces new property allow_move_to_suspend_on_window_close
(disabled by default) which removes setting "is closing" state and just
sends notification to browser integration, without any expectations on
how it's handled.
@magomez
Copy link
Copy Markdown

magomez commented Jul 7, 2023

I don't fully get what this wants to achieve.
It seems that if the new setting is set to true, the value of allow_scripts_to_close_windows is ignored, and scripts can't close windows at all.
What's exactly your use case?
Without checking the possible side effects of it, have you checked whether making the page->setIsClosing() conditional is enough for what you want to do?

@jakub-gocol-red
Copy link
Copy Markdown
Author

jakub-gocol-red commented Jul 7, 2023

I don't fully get what this wants to achieve.
What's exactly your use case?

Right now, with allow_scripts_to_close_windows property set to true, when application calls window.close(), page is switched to "is closing" state and page->chrome().closeWindow() is called, which sends event to integration that it should close window. The only correct behaviour at this point is to really close window.
In my project we don't want to close window in this case. We want to close opened page by switching url to let's say about:blank and hide browser window to the background. But then browser still is in "is closing" state and some apis don't work.

It seems that if the new setting is set to true, the value of allow_scripts_to_close_windows is ignored, and scripts can't close windows at all.

When allow_move_to_suspend_on_window_close is true, value of allow_scripts_to_close_windows is ignored and scripts can close window. I assumed there's no point in checking allow_scripts_to_close_windows value since anybody who'll use it will want to close window in all cases (not only those created by scripts). If you disagree, I might modify that check. It doesn't matter that much to me.

Without checking the possible side effects of it, have you checked whether making the page->setIsClosing() conditional is enough for what you want to do?

yes, I checked and modifying that whole change to look like this also works:
if (!frame->settings().allowMoveToSuspendOnWindowClose()) { page->setIsClosing(); }

Would such solution be acceptable?

@magomez
Copy link
Copy Markdown

magomez commented Jul 10, 2023

yes, I checked and modifying that whole change to look like this also works: if (!frame->settings().allowMoveToSuspendOnWindowClose()) { page->setIsClosing(); }

Would such solution be acceptable?

mmmm I'm a bit worried about leaving something in an inconsistent state if we do so.
Ok, let's merge this as it is now, and we'll fix it later if it creates any issue.

@magomez magomez merged commit 19e0c1e into WebPlatformForEmbedded:wpe-2.38 Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants