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

remote: track listeners on browser side #3251

Merged
merged 5 commits into from
Oct 31, 2015

Conversation

deepak1556
Copy link
Member

Fixes #3229

@@ -3,6 +3,9 @@ path = require 'path'
objectsRegistry = require './objects-registry.js'
v8Util = process.atomBinding 'v8_util'

# weak refereence to callback with their registry ID.
Copy link
Member

Choose a reason for hiding this comment

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

First character of comment line should be capitalized.

@zcbenz
Copy link
Member

zcbenz commented Oct 29, 2015

The atom::WeakMap is an RAII class used by C++ classes, if you want to expose it to JavaScript you should add an binding to it instead of making it a Wrappable directly.

@deepak1556
Copy link
Member Author

@zcbenz have made the changes.

@zcbenz
Copy link
Member

zcbenz commented Oct 29, 2015

Using WeakMap is going to conflict with ES6's WeakMap, I still prefer to use IDWeakMap, which also matches the C++ class's name.

@@ -70,6 +74,9 @@ unwrapArgs = (sender, args) ->
returnValue = metaToValue meta.value
-> returnValue
when 'function'
if rendererCallbacks.has(meta.id)
Copy link
Member

Choose a reason for hiding this comment

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

Different WebContents can send callbacks with the same ID.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, thanks!

@deepak1556 deepak1556 force-pushed the remote_callback_patch branch 2 times, most recently from a449435 to f37e0c3 Compare October 30, 2015 05:28
if not rendererCallbacks?
# Weak reference to callbacks with their ID
rendererCallbacks = new IDWeakMap()
rendererRegistry[webContentsId] = rendererCallbacks
Copy link
Member

Choose a reason for hiding this comment

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

The rendererCallbacks should be destroyed when the render view of webContents is destroyed.

@zcbenz
Copy link
Member

zcbenz commented Oct 30, 2015

Can you reimplement IDWeakMap::Add with IDWeakMap::Set? Their code is basically the same.

@deepak1556 deepak1556 force-pushed the remote_callback_patch branch 2 times, most recently from 826af98 to d047f1f Compare October 30, 2015 13:40
@zcbenz
Copy link
Member

zcbenz commented Oct 31, 2015

Thanks.

zcbenz added a commit that referenced this pull request Oct 31, 2015
remote: track listeners on browser side
@zcbenz zcbenz merged commit 323ab92 into electron:master Oct 31, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants