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

protocol: fix registerStandardSchemes api #5406

Merged
merged 4 commits into from May 9, 2016

Conversation

Projects
None yet
2 participants
@deepak1556
Copy link
Member

deepak1556 commented May 5, 2016

Fixes #5303

Depends on electron/libchromiumcontent#201

@@ -394,7 +396,7 @@ describe('protocol module', function () {
var handler = function (request, callback) {
callback(fakeFilePath)
}
protocol.registerBufferProtocol(protocolName, handler, function (error) {
protocol.registerFileProtocol(protocolName, handler, function (error) {

This comment has been minimized.

Copy link
@zcbenz

zcbenz May 6, 2016

Member

Is there a reason behind this change?

This comment has been minimized.

Copy link
@deepak1556

deepak1556 May 6, 2016

Author Member

not related to the PR, the specs were inside protocol.registerFileProtocol, so thought they are supposed to test it ?

This comment has been minimized.

Copy link
@zcbenz

zcbenz May 6, 2016

Member

That makes sense.

@zcbenz

This comment has been minimized.

Copy link
Member

zcbenz commented May 6, 2016

It seems that the specs have passed even though we don't have electron/libchromiumcontent#201 yet.

@deepak1556

This comment has been minimized.

Copy link
Member Author

deepak1556 commented May 6, 2016

It seems that the specs have passed

Yeah, I am not sure what to do. Should we patch ?

@zcbenz

This comment has been minimized.

Copy link
Member

zcbenz commented May 6, 2016

Yeah, I am not sure what to do. Should we patch ?

The test case just always pass, so it can not test out whether protocol.registerStandardSchemes is working. A test case should fail when there is no fix, and pass after the fix gets merged.

@deepak1556 deepak1556 force-pushed the deepak1556:protocol_standard_scheme_patch branch from 7bf8b76 to e757e95 May 6, 2016

@deepak1556

This comment has been minimized.

Copy link
Member Author

deepak1556 commented May 6, 2016

Have made a separate PR to show the failing spec.

@zcbenz

This comment has been minimized.

Copy link
Member

zcbenz commented May 7, 2016

So we don't actually need electron/libchromiumcontent#201 to fix #5303?

@deepak1556 deepak1556 force-pushed the deepak1556:protocol_standard_scheme_patch branch from 55a1546 to 2f02f45 May 7, 2016

deepak1556 added some commits May 5, 2016

allow protocol module initialization before app ready.
 * ensure registerStandardSchemes can only be called before app ready
 * ensure other protocol methods can only be used after app ready

@deepak1556 deepak1556 force-pushed the deepak1556:protocol_standard_scheme_patch branch from ec1c0b4 to 9c71c9f May 7, 2016

@deepak1556

This comment has been minimized.

Copy link
Member Author

deepak1556 commented May 7, 2016

Have made the changes, thanks! Now

  • Protocol module can be initialized before the ready event.
  • registerStandardSchemes can only be used before ready event and other methods only after the ready event.

A possible problem that might arise, is when making protocol module work with partitions. If we hook protocol as a property of session, then it can be used only after ready event which will conflict with registerStandardSchemes. So how are we going make protocol module respect sessions ?

  1. We could provide a fromPartition method and keep current behavior (or)
  2. Move the registerStandardSchemes api to app module and proide a behavior similar to #4551 .
@zcbenz

This comment has been minimized.

Copy link
Member

zcbenz commented May 8, 2016

I prefer to use a less-intrusive way like this:

const {createProtocolObject, registerStandardSchemes} = process.atomBinding('protocol')
exports.registerStandardSchemes = function (schemes) {
  if (isReady) {
    throw new Error()
  }
  registerStandardSchemes(schemes)
}

app.once('ready', function () {
  protocol = createProtocolObject();
  // Be compatible with old APIs.
  for (let method in protocol) {
    exports[method] = protocol[method].bind(protocol)
  }
})

So registerStandardSchemes is moved away from api::Protocol as an independent API, and the creation of protocol object is delayed in createProtocolObject.

After we have multi-session support for protocol, the createProtocolObject() will become session.defaultSession.protocol.

A possible problem that might arise, is when making protocol module work with partitions. If we hook protocol as a property of session, then it can be used only after ready event which will conflict with registerStandardSchemes.

After moving registerStandardSchemes out of api::Protocol, it will be solved automatically.

@deepak1556

This comment has been minimized.

Copy link
Member Author

deepak1556 commented May 8, 2016

Thanks for the explanation! have made the changes.

@zcbenz

This comment has been minimized.

Copy link
Member

zcbenz commented May 9, 2016

👍

@zcbenz zcbenz merged commit af0afec into electron:master May 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
You can’t perform that action at this time.