Switch out jschannel for frame-rpc

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.

Author: tilgovi

h/static/scripts/annotation-sync.coffee

@@ -58,49 +58,47 @@ module.exports = class AnnotationSync
   getAnnotationForTag: (tag) ->
     @cache[tag] or null
 
-  sync: (annotations, cb) ->
+  sync: (annotations) ->
     annotations = (this._format a for a in annotations)
-    @bridge.call
-      method: 'sync'
-      params: annotations
-      callback: cb
+    @bridge.call 'sync', annotations, (err, annotations = []) =>
+      for a in annotations
+        this._parse(a)
     this
 
   # Handlers for messages arriving through a channel
   _channelListeners:
-    'beforeCreateAnnotation': (txn, body) ->
+    'beforeCreateAnnotation': (body, cb) ->
       annotation = this._parse(body)
       delete @cache[annotation.$$tag]
       @_emit 'beforeAnnotationCreated', annotation
       @cache[annotation.$$tag] = annotation
-      this._format annotation
+      cb(null, this._format(annotation))
 
-    'createAnnotation': (txn, body) ->
+    'createAnnotation': (body, cb) ->
       annotation = this._parse(body)
       delete @cache[annotation.$$tag]
       @_emit 'annotationCreated', annotation
       @cache[annotation.$$tag] = annotation
-      this._format annotation
+      cb(null, this._format(annotation))
 
-    'updateAnnotation': (txn, body) ->
+    'updateAnnotation': (body, cb) ->
       annotation = this._parse(body)
       delete @cache[annotation.$$tag]
       @_emit('beforeAnnotationUpdated', annotation)
       @_emit('annotationUpdated', annotation)
       @cache[annotation.$$tag] = annotation
-      this._format annotation
+      cb(null, this._format(annotation))
 
-    'deleteAnnotation': (txn, body) ->
+    'deleteAnnotation': (body, cb) ->
       annotation = this._parse(body)
       delete @cache[annotation.$$tag]
       @_emit('annotationDeleted', annotation)
-      res = this._format(annotation)
-      res
+      cb(null, this._format(annotation))
 
-    'sync': (ctx, bodies) ->
-      (this._format(this._parse(b)) for b in bodies)
+    'sync': (bodies, cb) ->
+      cb(null, (this._format(this._parse(b)) for b in bodies))
 
-    'loadAnnotations': (txn, bodies) ->
+    'loadAnnotations': (bodies) ->
       annotations = (this._parse(a) for a in bodies)
       @_emit('loadAnnotations', annotations)
 
@@ -127,17 +125,13 @@ module.exports = class AnnotationSync
     'annotationsLoaded': (annotations) ->
       bodies = (this._format a for a in annotations when not a.$$tag)
       return unless bodies.length
-      @bridge.notify
-        method: 'loadAnnotations'
-        params: bodies
+      @bridge.call('loadAnnotations', bodies)
 
   _syncCache: (channel) ->
     # Synchronise (here to there) the items in our cache
     annotations = (this._format a for t, a of @cache)
     if annotations.length
-      channel.notify
-        method: 'loadAnnotations'
-        params: annotations
+      channel.call('loadAnnotations', annotations)
 
   _mkCallRemotelyAndParseResults: (method, callBack) ->
     (annotation) =>
@@ -148,11 +142,7 @@ module.exports = class AnnotationSync
         callBack? failure, results
 
       # Call the remote method
-      options =
-        method: method
-        callback: wrappedCallback
-        params: this._format(annotation)
-      @bridge.call(options)
+      @bridge.call(method, this._format(annotation), wrappedCallback)
 
   # Parse returned message bodies to update cache with any changes made remotely
   _parseResults: (results) ->

h/static/scripts/annotation-ui-sync.coffee

@@ -18,18 +18,18 @@ module.exports = class AnnotationUISync
       tags.map(annotationSync.getAnnotationForTag, annotationSync)
 
     channelListeners =
-      showAnnotations: (ctx, tags=[]) ->
+      showAnnotations: (tags=[]) ->
         annotations = getAnnotationsByTags(tags)
         annotationUI.selectAnnotations(annotations)
-      focusAnnotations: (ctx, tags=[]) ->
+      focusAnnotations: (tags=[]) ->
         annotations = getAnnotationsByTags(tags)
         annotationUI.focusAnnotations(annotations)
-      toggleAnnotationSelection: (ctx, tags=[]) ->
+      toggleAnnotationSelection: (tags=[]) ->
         annotations = getAnnotationsByTags(tags)
         annotationUI.xorSelectedAnnotations(annotations)
-      setVisibleHighlights: (ctx, state) ->
+      setVisibleHighlights: (state) ->
         annotationUI.visibleHighlights = Boolean(state)
-        bridge.notify(method: 'setVisibleHighlights', params: state)
+        bridge.call('setVisibleHighlights', state)
 
     # Because the channel events are all outside of the angular framework we
     # need to inform Angular that it needs to re-check it's state and re-draw
@@ -45,8 +45,6 @@ module.exports = class AnnotationUISync
     onConnect = (channel, source) ->
       # Allow the host to define its own state
       unless source is $window.parent
-        channel.notify
-          method: 'setVisibleHighlights'
-          params: annotationUI.visibleHighlights
+        channel.call('setVisibleHighlights', annotationUI.visibleHighlights)
 
     bridge.onConnect(onConnect)

h/static/scripts/annotator/guest.coffee

@@ -72,10 +72,6 @@ module.exports = class Guest extends Annotator
         formatted = {}
         for k, v of annotation when k isnt 'anchors'
           formatted[k] = v
-        # Work around issue in jschannel where a repeated object is considered
-        # recursive, even if it is not its own ancestor.
-        if formatted.document?.title
-          formatted.document.title = formatted.document.title.slice()
         formatted
 
     this.addPlugin('CrossFrame', cfOptions)
@@ -115,21 +111,20 @@ module.exports = class Guest extends Annotator
     crossframe.onConnect(=> this.publish('panelReady'))
     crossframe.on('onEditorHide', this.onEditorHide)
     crossframe.on('onEditorSubmit', this.onEditorSubmit)
-    crossframe.on 'focusAnnotations', (ctx, tags=[]) =>
+    crossframe.on 'focusAnnotations', (tags=[]) =>
       for anchor in @anchors when anchor.highlights?
         toggle = anchor.annotation.$$tag in tags
         $(anchor.highlights).toggleClass('annotator-hl-focused', toggle)
-    crossframe.on 'scrollToAnnotation', (ctx, tag) =>
+    crossframe.on 'scrollToAnnotation', (tag) =>
       for anchor in @anchors when anchor.highlights?
         if anchor.annotation.$$tag is tag
           $(anchor.highlights).scrollintoview()
           return
-    crossframe.on 'getDocumentInfo', (trans) =>
-      trans.delayReturn(true)
+    crossframe.on 'getDocumentInfo', (cb) =>
       this.getDocumentInfo()
-      .then((info) -> trans.complete(info))
-      .catch((reason) -> trans.error(reason))
-    crossframe.on 'setVisibleHighlights', (ctx, state) =>
+      .then((info) -> cb(null, info))
+      .catch((reason) -> cb(reason))
+    crossframe.on 'setVisibleHighlights', (state) =>
       this.publish 'setVisibleHighlights', state
 
   _setupWrapper: ->
@@ -315,24 +310,20 @@ module.exports = class Guest extends Annotator
     return annotation
 
   showAnnotations: (annotations) =>
-    @crossframe?.notify
-      method: "showAnnotations"
-      params: (a.$$tag for a in annotations)
+    tags = (a.$$tag for a in annotations)
+    @crossframe?.call('showAnnotations', tags)
 
   toggleAnnotationSelection: (annotations) =>
-    @crossframe?.notify
-      method: "toggleAnnotationSelection"
-      params: (a.$$tag for a in annotations)
+    tags = (a.$$tag for a in annotations)
+    @crossframe?.call('toggleAnnotationSelection', tags)
 
   updateAnnotations: (annotations) =>
-    @crossframe?.notify
-      method: "updateAnnotations"
-      params: (a.$$tag for a in annotations)
+    tags = (a.$$tag for a in annotations)
+    @crossframe?.call('updateAnnotations', tags)
 
   focusAnnotations: (annotations) =>
-    @crossframe?.notify
-      method: "focusAnnotations"
-      params: (a.$$tag for a in annotations)
+    tags = (a.$$tag for a in annotations)
+    @crossframe?.call('focusAnnotations', tags)
 
   onSuccessfulSelection: (event, immediate) ->
     unless event?
@@ -396,11 +387,7 @@ module.exports = class Guest extends Annotator
   # Pass true to show the highlights in the frame or false to disable.
   setVisibleHighlights: (shouldShowHighlights) ->
     return if @visibleHighlights == shouldShowHighlights
-
-    @crossframe?.notify
-      method: 'setVisibleHighlights'
-      params: shouldShowHighlights
-
+    @crossframe?.call('setVisibleHighlights', shouldShowHighlights)
     this.toggleHighlightClass(shouldShowHighlights)
 
   toggleHighlightClass: (shouldShowHighlights) ->
@@ -413,11 +400,11 @@ module.exports = class Guest extends Annotator
 
   # Open the sidebar
   showFrame: ->
-    @crossframe?.notify method: 'open'
+    @crossframe?.call('open')
 
   # Close the sidebar
   hideFrame: ->
-    @crossframe?.notify method: 'back'
+    @crossframe?.call('back')
 
   onAdderMouseup: (event) ->
     event.preventDefault()

h/static/scripts/annotator/plugin/cross-frame.coffee

@@ -12,7 +12,7 @@ extract = extract = (obj, keys...) ->
 # as keeping the annotation state in sync with the sidebar application, this
 # frame acts as the bridge client, the sidebar is the server. This plugin
 # can also be used to send messages through to the sidebar using the
-# notify method.
+# call method.
 module.exports = class CrossFrame extends Annotator.Plugin
   constructor: (elem, options) ->
     super
@@ -41,8 +41,8 @@ module.exports = class CrossFrame extends Annotator.Plugin
     this.on = (event, fn) ->
       bridge.on(event, fn)
 
-    this.notify = (message) ->
-      bridge.notify(message)
+    this.call = (message, args...) ->
+      bridge.call(message, args...)
 
     this.onConnect = (fn) ->
       bridge.onConnect(fn)

h/static/scripts/annotator/plugin/test/cross-frame-test.coffee

@@ -21,7 +21,7 @@ describe 'Annotator.Plugin.CrossFrame', ->
     fakeBridge =
       createChannel: sandbox.stub()
       onConnect: sandbox.stub()
-      notify: sandbox.stub()
+      call: sandbox.stub()
       on: sandbox.stub()
 
     fakeAnnotationSync =
@@ -91,11 +91,11 @@ describe 'Annotator.Plugin.CrossFrame', ->
       bridge.on('event', 'arg')
       assert.calledWith(fakeBridge.on, 'event', 'arg')
 
-  describe '.notify', ->
+  describe '.call', ->
     it 'proxies the call to the bridge', ->
       bridge = createCrossFrame()
-      bridge.notify(method: 'method')
-      assert.calledWith(fakeBridge.notify, method: 'method')
+      bridge.call('method', 'arg1', 'arg2')
+      assert.calledWith(fakeBridge.call, 'method', 'arg1', 'arg2')
 
   describe '.onConnect', ->
     it 'proxies the call to the bridge', ->

h/static/scripts/annotator/test/guest-test.coffee

@@ -144,18 +144,11 @@ describe 'Guest', ->
         formatted = options.formatter(ann)
         assert.equal(formatted.$$tag, 'tag1')
 
-      it 'strips the "anchors" property', ->
+      it 'strips properties that are not whitelisted', ->
         ann = {$$tag: 'tag1', anchors: []}
         formatted = options.formatter(ann)
         assert.notProperty(formatted, 'anchors')
 
-      it 'clones the document.title array if present', ->
-        title = ['Page Title']
-        ann = {$$tag: 'tag1', document: {title: title}}
-        formatted = options.formatter(ann)
-        assert.notStrictEqual(title, formatted.document.title)
-        assert.deepEqual(title, formatted.document.title)
-
   describe 'annotation UI events', ->
     emitGuestEvent = (event, args...) ->
       fn(args...) for [evt, fn] in fakeCrossFrame.on.args when event == evt
@@ -183,7 +176,7 @@ describe 'Guest', ->
           {annotation: {$$tag: 'tag1'}, highlights: highlight0.toArray()}
           {annotation: {$$tag: 'tag2'}, highlights: highlight1.toArray()}
         ]
-        emitGuestEvent('focusAnnotations', 'ctx', ['tag1'])
+        emitGuestEvent('focusAnnotations', ['tag1'])
         assert.isTrue(highlight0.hasClass('annotator-hl-focused'))
 
       it 'unfocuses any annotations without a matching tag', ->
@@ -210,7 +203,7 @@ describe 'Guest', ->
         guest.anchors = [
           {annotation: {$$tag: 'tag1'}, highlights: highlight.toArray()}
         ]
-        emitGuestEvent('scrollToAnnotation', 'ctx', 'tag1')
+        emitGuestEvent('scrollToAnnotation', 'tag1')
         assert.calledOn($.fn.scrollintoview, sinon.match(highlight))
 
     describe 'on "getDocumentInfo" event', ->
@@ -227,44 +220,34 @@ describe 'Guest', ->
         sandbox.restore()
 
       it 'calls the callback with the href and pdf metadata', (done) ->
-        assertComplete = (payload) ->
+        assertComplete = (err, payload) ->
           try
             assert.equal(payload.uri, document.location.href)
             assert.equal(payload.metadata, metadata)
             done()
           catch e
             done(e)
 
-        ctx = {complete: assertComplete, delayReturn: sandbox.stub()}
         metadata = {title: 'hi'}
         promise = Promise.resolve(metadata)
         guest.plugins.PDF.getMetadata.returns(promise)
 
-        emitGuestEvent('getDocumentInfo', ctx)
+        emitGuestEvent('getDocumentInfo', assertComplete)
 
       it 'calls the callback with the href and basic metadata if pdf fails', (done) ->
-        assertComplete = (payload) ->
+        assertComplete = (err, payload) ->
           try
             assert.equal(payload.uri, window.location.href)
             assert.deepEqual(payload.metadata, metadata)
             done()
           catch e
             done(e)
 
-        ctx = {complete: assertComplete, delayReturn: sandbox.stub()}
         metadata = {title: 'hi', link: [{href: window.location.href}]}
-
         promise = Promise.reject(new Error('Not a PDF document'))
         guest.plugins.PDF.getMetadata.returns(promise)
 
-        emitGuestEvent('getDocumentInfo', ctx)
-
-      it 'notifies the channel that the return value is async', ->
-        delete guest.plugins.PDF
-
-        ctx = {complete: sandbox.stub(), delayReturn: sandbox.stub()}
-        emitGuestEvent('getDocumentInfo', ctx)
-        assert.calledWith(ctx.delayReturn, true)
+        emitGuestEvent('getDocumentInfo', assertComplete)
 
   describe 'onAdderMouseUp', ->
     it 'it prevents the default browser action when triggered', () ->

h/static/scripts/annotator/test/host-test.coffee

@@ -16,7 +16,7 @@ describe 'Host', ->
     fakeCrossFrame = {}
     fakeCrossFrame.onConnect = sandbox.stub().returns(fakeCrossFrame)
     fakeCrossFrame.on = sandbox.stub().returns(fakeCrossFrame)
-    fakeCrossFrame.notify = sandbox.stub().returns(fakeCrossFrame)
+    fakeCrossFrame.call = sandbox.spy()
 
     Annotator.Plugin.CrossFrame = -> fakeCrossFrame