Set 'noopener' when opening windows to avoid sharing event loops

[DanTup]

Prevents the web UI pausing when an app opened with window.open is paused in the browsers debugger.

Fixes #5857.

Note: I'm not sure how to test this from source. I tested it on GitPod by replacing window.open with a wrapper using the dev tools like this:

var oldWO = window.open; window.open = function(url) {console.log(url); oldWO(url, undefined, 'noopener');}

This resolved the issue of the GitPod UI hanging when my web app stopped at a breakpoint. If this is something I should be able to test locally, please provide some pointers (and ofc, I'm happy to re-test on staging once it gets there).

[akosyakov]

@DanTup thank you, it is very good change!

I will merge if the build is green and there is no objections, i.e. someone actually expect an instance of Window.

[kittaakos]

@DanTup, please update the changelog, it is a breaking change. Thank you!

[kittaakos]

The electron implementation does not return with the new BrowserWindow instance either. Can we change the API to void if we break it anyway?

[DanTup]

I'm not sure how best to describe this in the changelog, since I'm not really sure how this is exposed. Is something like this correct?

• [core] new browser windows spawned through opener-service have noopener set, preventing them from accessing window.opener and giveing them their own event loop. openNewWindow will no longer return a Window as a result.

[kittaakos]

Is something like this correct?

Perfect, we just need some info to trace back who did the breaking change ;)

[vince-fugnitto]

@DanTup in general we ask that every commit be signed-off (recent reason for the failed ECA check).
It is enough to squash the commits and preserve your initial commit message :)

[DanTup]

Heh, I was just checking why that went red. I'll fix!

[kittaakos]

I propose

• leaving the API as : Window | undefined so that subclasses can override the behavior if they want to. If you leave as is, no changelog modification is required.
• Or switching to : void return value.

The former approach feels the more flexible one, but : undefined does not make much sense to me.

[DanTup]

I thought leaving Window might be misleading to include it but always return undefined, but I didn't consider others overriding it. I'm happy to change either way, just let me know what your preferences.

[kittaakos]

As far as I know, none of the downstream projects expect a Window object, looks good to me 👍

Thank you for your contribution, @DanTup

[DanTup]

No problem! I'm not sure if you want anything changing or if it's ok as-is. Feel free to change after merging if that's easier than going back and forth. My need is only the noopener part to free us from sharing a thread :-)