Remove crossframe service and simplify communication with connected frames #116

robertknight · 2016-09-16T15:42:53Z

This PR is an effort to make the flow of events that lead to an annotation loaded in the sidebar app via the API being anchored in the page easier to understand and change. It should also unblock future optimizations (such as batching certain inter-window messages together) and simplifications to other code.

Communication between the sidebar app and the frame, in master, consists of several pieces:

CrossFrame - A service that sets up listeners from the "local" event bus (the annotator event bus in the frame, Angular's $rootScope in the sidebar app) and invokes handlers in AnnotationSync
AnnotationSync - A service that listens for annotation events on the "local" event bus, strips out most of the annotation properties except for the minimal information needed to identify and anchor them in the page, and sends them to the sidebar/host frame via the bridge service
Bridge - A wrapper around the frame-rpc npm package for making RPC calls between windows using the window.postMessage API
Discovery - A service that runs in either client or server mode and connects to Discovery services running in the opposite mode. When the "server" (the sidebar app) finds a "client" (the code running in the page), a channel is created using the Bridge service which enables the two to communicate

The biggest problem with the above architecture is that the same code was used for the AnnotationSync service in both the sidebar app and the host frame, despite the fact that each side has different responsibilities and a significantly different environment.

This PR removes the CrossFrame and AnnotationSync services from the sidebar app, and replaces it with a minimal frameSync service which listens to only the events/sends only the messages that the sidebar app needs to send. The AnnotationSync code meanwhile has been gutted of everything that was not relevant to host frame and moved to the annotator/ folder.

The Bridge and Discovery services could likely be simplified in future, but I have continued to re-use them in the same way for the moment.

robertknight · 2016-09-16T15:49:12Z

h/static/scripts/reducers/annotations.js

@@ -105,7 +108,8 @@ var update = {
          updatedTags[existing.$$tag] = true;
        }
      } else {
-        added.push(initializeAnnot(annot));
+        added.push(initializeAnnot(annot, 't' + nextTag));


In master, annotations on both sides of the channel are given local tags generated with btoa(Math.random()). The important thing about the tags is that they may be exposed to third-party JS, so shouldn't be connected to the original annotation ID at all.

Here I've gone for the simpler option of using t1,t2 ... tN for annotations loaded via the API where N is just a serial number that is incremented every time a new annotation is loaded via the API, on the basis that 3rd-party JS code in the page could observe the order in which annotations are loaded if it wanted to, so there isn't much point trying to hide that.

Annotations originating from the page still use the old scheme.

judell · 2016-09-17T02:33:43Z

Thank you. Debugging my way through cross-frame communication has been a brain-melter, sounds like this will help, look forward to finding out.

seanh

Clicking on a highlight in the document doesn't seem to do anything for me?

seanh · 2016-09-21T12:18:20Z

h/static/scripts/frame-sync.js

+      events_.forEach(function (event) {
+        inFrame.add(event.tag);
+        annotationUI.updateAnchorStatus(null, event.tag, event.msg.$orphan);
+        $rootScope.$broadcast(events.ANNOTATIONS_SYNCED, [event.tag]);


Would AFTER_ANNOTATION_ANCHORED be a better name for this event?

Yes, but this is an existing event which is referenced elsewhere in the code which I didn't want to touch as part of this PR. There is quite a bit of additional cleanup that can be done once this is merged.

Previously the communication between the sidebar and host frame was implemented by a shared AnnotationSync class, with a 'crossframe' service which abstracted the event bus on each side. This design however assumed that both sides wanted to listen to the same messages and react to them in similar ways. This is not the case, especially given the change to use Redux for state management in the sidebar app. This commit replaces the crossframe service and AnnotationSync class with a new 'frameSync' service which implements only the event listeners and sidebar -> page RPC calls that are actually needed. As a result, local annotation tags for annotations loaded via the API can now be assigned by the reducer in the Redux store, making this easier to test and getting us another step closer to making Annotation objects immutable in the sidebar app.

Now that AnnotationSync is no longer used in the sidebar app, we can remove all of the handlers for events and messages arriving through the channel which are not needed by the page itself.

robertknight · 2016-09-21T14:15:16Z

Clicking on a highlight in the document doesn't seem to do anything for me?

Good catch. I missed the fact that listeners for selection/focus events in the frame are handled by the AnnotationUISync class, which used to be initialized in the CrossFrame code. I have modified FrameSync to create a new instance of that class in the setupSyncFromFrame function.

The entire AnnotationUISync class can be replaced by something much simpler, but I'll save that for a separate refactoring.

seanh · 2016-09-21T14:31:41Z

Fantastic to see this part of the code base finally getting untangled. The new code is very easy to understand. Looking forward to further simplification that will now be possible.

One thing I think we should watch out for is terminology. The terrible names given to everything in this part of the codebase is one of the things that makes it so hard to understand. There's quite a few different things to try and give good, distinct names to: the sidebar app itself, the sidebar app's iframe, the document that contains the sidebar app's iframe, the code of ours that runs inside this document, other iframes in the document and instances of our code that run inside them, and the various objects that are used to communicate between frames (bridge, crossframe, various things with "sync" in their name...).

I think this PR makes a good start on that with frame-sync and with partially getting rid of cross-frame and annotation-sync, but there's more to be done.

robertknight commented Sep 16, 2016

View reviewed changes

robertknight mentioned this pull request Sep 19, 2016

Count deletions as pending updates #114

Merged

robertknight force-pushed the frame-sync-refactor branch from e2851d9 to 7d306bb Compare September 19, 2016 13:07

seanh self-assigned this Sep 21, 2016

seanh requested changes Sep 21, 2016

View reviewed changes

robertknight added 3 commits September 21, 2016 15:11

Move AnnotationSync class to annotator/ dir and remove unused code

e126b97

Now that AnnotationSync is no longer used in the sidebar app, we can remove all of the handlers for events and messages arriving through the channel which are not needed by the page itself.

Add a comment explaining what AnnotationSync is responsible for

b08aafe

robertknight force-pushed the frame-sync-refactor branch from 7d306bb to b08aafe Compare September 21, 2016 14:11

seanh approved these changes Sep 21, 2016

View reviewed changes

seanh merged commit f588f1e into master Sep 21, 2016

seanh deleted the frame-sync-refactor branch September 21, 2016 14:27

This was referenced Sep 23, 2016

Rename the "$$tag" annotation property and remove Object.assign() hacks #126

Merged

Direct links to Page Notes not working #129

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove crossframe service and simplify communication with connected frames #116

Remove crossframe service and simplify communication with connected frames #116

robertknight commented Sep 16, 2016 •

edited

Loading

robertknight Sep 16, 2016

judell commented Sep 17, 2016

seanh left a comment

seanh Sep 21, 2016 •

edited

Loading

robertknight Sep 21, 2016

robertknight commented Sep 21, 2016

seanh commented Sep 21, 2016

Remove crossframe service and simplify communication with connected frames #116

Remove crossframe service and simplify communication with connected frames #116

Conversation

robertknight commented Sep 16, 2016 • edited Loading

robertknight Sep 16, 2016

Choose a reason for hiding this comment

judell commented Sep 17, 2016

seanh left a comment

Choose a reason for hiding this comment

seanh Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

robertknight Sep 21, 2016

Choose a reason for hiding this comment

robertknight commented Sep 21, 2016

seanh commented Sep 21, 2016

robertknight commented Sep 16, 2016 •

edited

Loading

seanh Sep 21, 2016 •

edited

Loading