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

browser: set up extension protocol handler for each browser context #5904

Merged
merged 7 commits into from Jun 9, 2016

Conversation

Projects
None yet
3 participants
@deepak1556
Member

deepak1556 commented Jun 7, 2016

Fixes #5896
Fixes #5925
Fixes #4991

What is the take on extensions and partitions ? Currently extensions are loaded for all windows irrespective of partitions, should extensions be persisted for non persistent partitions ?

@@ -879,6 +880,47 @@ describe('browser-window module', function () {
})
})
})
describe('does not crash when a partition is used', function (done) {

This comment has been minimized.

@kevinsawicki

kevinsawicki Jun 7, 2016

Contributor

I don't think you need the done parameter on the describe block here.

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Jun 8, 2016

This is mostly a work around of the original bug, I prefer to fix the root problem with following steps:

  1. Fix the crash in #5896, so instead of crashing, the devtools should just be simply not working
  2. Make the protocol module work with partitions
  3. Add event for session creations for session module
  4. Create the handler for chrome-extension protocol when a new session is created
@deepak1556

This comment has been minimized.

Member

deepak1556 commented Jun 8, 2016

Have made the changes, might have to add a few more tests. Thanks!

```javascript
const electron = require('electron');
const app = electron.app;

This comment has been minimized.

@zcbenz

zcbenz Jun 9, 2016

Contributor

We can use destructuring assignment here:

const {app, session} = require('electron')

And for new code samples in docs we should use new coding style, e.g. without ;.

const session = electron.session;
const path = require('path');
app.on('ready', function() {

This comment has been minimized.

@zcbenz

zcbenz Jun 9, 2016

Contributor

There should be a space after function.

@@ -35,6 +35,7 @@ Object.defineProperty(exports, 'defaultSession', {
const wrapSession = function (session) {
// Session is an EventEmitter.
Object.setPrototypeOf(session, EventEmitter.prototype)
process.emit('session-created', session)

This comment has been minimized.

@zcbenz

zcbenz Jun 9, 2016

Contributor

How about making this event part of session module, so users can listen to it by:

const {session} = require('electron')
session.on('session-created', () => {
})
})
})
afterEach(function () {

This comment has been minimized.

@zcbenz

zcbenz Jun 9, 2016

Contributor

This seems redundant.

@deepak1556

This comment has been minimized.

Member

deepak1556 commented Jun 9, 2016

@zcbenz Fixed, thanks!

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Jun 9, 2016

👍

@zcbenz zcbenz merged commit 2ec5406 into electron:master Jun 9, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment