Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move shared process to a browser window #21879

Merged
merged 25 commits into from
Mar 7, 2017
Merged

Conversation

joaomoreno
Copy link
Member

Moving the shared process to a browser window will enable it to use Chrome's network stack. This will affect mostly the extension viewlet.

Related to #18103

@joaomoreno joaomoreno added the proxy Issues regarding network proxies label Mar 3, 2017
@joaomoreno joaomoreno added this to the March 2017 milestone Mar 3, 2017
@joaomoreno joaomoreno self-assigned this Mar 3, 2017
@@ -174,6 +175,12 @@ function main(accessor: ServicesAccessor, mainIpcServer: Server, userEnv: platfo
// TODO@Joao: unfold this
windowsMainService = accessor.get(IWindowsMainService);

windowsMainService.onWindowClose(() => {
if (windowsMainService.getWindowCount() === 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joaomoreno as I said before, this should have a check for Windows/Linux, because only there we close the app when closing the last window!

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall. Upon first looks I am not sure why the native crash on Windows happens. Would be interesting if the crash reproduces if the contents of that window are super simple (that would indicate it's an issue with the way the lifecycle of the window is handled).

<script>
(function () {
try {
window.location.search.substring(1).split('&').forEach(p => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theming here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah good catch, removing this.

@@ -0,0 +1,102 @@
/*---------------------------------------------------------------------------------------------
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a lot of overlap with https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/electron-browser/bootstrap/index.js#L48, maybe better to have a common base that both can reuse?

@joaomoreno joaomoreno merged commit a3ad921 into master Mar 7, 2017
@joaomoreno joaomoreno deleted the joao/shared-process branch March 7, 2017 10:34
@joaomoreno
Copy link
Member Author

Wow, that took a lot of commits. Thanks for the help, @bpasero !

@FloMaetschke
Copy link

@joaomoreno is the merge already in the latest insider build?
If yes, i can't confirm working proxy yet.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
proxy Issues regarding network proxies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants