Switch out jschannel for frame-rpc #2425
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Okay. This is verified manually. The tests all pass. I just re-forced with having removed the 'notify' calls. I did a grep for "method: " to make sure I hadn't missed any calls somehow. I'm pretty confident this is good to go, unless there are objections. |
|
Nice! |
The frame-rpc module is a proper CommonJS module and it's footprint is really tiny. Its protocol is simpler. It doesn't handle connection and buffering, but with our onConnect callback we don't need buffering. The connection handshake that jschannel did was so trivial it's been re-implemented here (each side tries to connect to the other once). We have to handle timeouts ourselves, but that's all hidden in the bridge code. In exchange, we get a simpler API, we get rid of the call/notify distinction in favor of just passing a callback or not and we avoid the excess overhead of the recursion guard in the serialization code that was giving us false positive issues with the document title. This is one step toward removing all the browserify-shim requiring libraries from the injected bundle, which will eventually fix #2397.
|
|
||
| # Close the sidebar | ||
| hideFrame: -> | ||
| @crossframe?.notify method: 'back' | ||
| @crossframe?.call('back') |
There was a problem hiding this comment.
(Not suggesting that's an issue with this PR. Just... o_O...)
|
LGTM. |
nickstenning
added a commit
that referenced
this pull request
Aug 3, 2015
Switch out jschannel for frame-rpc
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The frame-rpc module is a proper CommonJS module and it's footprint
is really tiny. Its protocol is simpler. It doesn't handle connection
and buffering, but with our onConnect callback we don't need buffering.
The connection handshake that jschannel did was so trivial it's been
re-implemented here (each side tries to connect to the other once).
We have to handle timeouts ourselves, but that's all hidden in the
bridge code. In exchange, we get a simpler API, we get rid of the
call/notify distinction in favor of just passing a callback or not
and we avoid the excess overhead of the recursion guard in the
serialization code that was giving us false positive issues with
the document title.
This is one step toward removing all the browserify-shim requiring
libraries from the injected bundle, which will eventually fix #2397.