Skip to content

Commit

Permalink
Only make one search query per frame on page load
Browse files Browse the repository at this point in the history
Previously, when loading Hypothesis into a page, the client would make
one search request for each and every URI found on the page, including
those found by the document metadata scanners (in the Annotator
"Document" and "PDF" plugins.)

This is wasteful of network bandwidth, and moreover should not be
necessary in most cases due to the ability to expand to equivalent URIs
on the server side.

The component responsible for coordinating communication across page
frames is the "crossframe" service. This services maintains a registry
of page frames (previously called "providers") and their associated
metadata. This commit reduces the metadata stored to simply the page
URL, or in the case of PDF documents, the fingerprint URN.

It's worth noting that this *does* change behaviour in one important
way: if a page contains document equivalence data that we've never seen
before, and one of the alternate URIs has already been annotated, then
this change will mean we do not load annotations until that page itself
has been annotated (and thus the document equivalence data updated). I
am assuming that this is an extreme edge case.
  • Loading branch information
nickstenning committed Jul 17, 2015
1 parent 47fa018 commit 4f75515
Show file tree
Hide file tree
Showing 7 changed files with 29 additions and 39 deletions.
2 changes: 1 addition & 1 deletion h/static/scripts/annotation-ui-sync.coffee
@@ -1,4 +1,4 @@
# Uses a channel between the sidebar and the attached providers to ensure
# Uses a channel between the sidebar and the attached frames to ensure
# the interface remains in sync.
module.exports = class AnnotationUISync
###*
Expand Down
13 changes: 4 additions & 9 deletions h/static/scripts/cross-frame.coffee
@@ -1,7 +1,5 @@
# Instantiates all objects used for cross frame discovery and communication.
module.exports = class CrossFrame
providers: null

this.inject = [
'$rootScope', '$document', '$window', 'store', 'annotationUI'
'Discovery', 'bridge',
Expand All @@ -12,7 +10,7 @@ module.exports = class CrossFrame
Discovery, bridge,
AnnotationSync, AnnotationUISync
) ->
@providers = []
@frames = []

createDiscovery = ->
options =
Expand Down Expand Up @@ -46,20 +44,17 @@ module.exports = class CrossFrame
createAnnotationUISync = (annotationSync) ->
new AnnotationUISync($rootScope, $window, bridge, annotationSync, annotationUI)

addProvider = (channel) =>
provider = {channel: channel, entities: []}

addFrame = (channel) =>
channel.call
method: 'getDocumentInfo'
success: (info) =>
$rootScope.$apply =>
provider.entities = (link.href for link in info.metadata.link)
@providers.push(provider)
@frames.push({channel: channel, uri: info.uri})

this.connect = ->
discovery = createDiscovery()

bridge.onConnect(addProvider)
bridge.onConnect(addFrame)
annotationSync = createAnnotationSync()
annotationUISync = createAnnotationUISync(annotationSync)

Expand Down
12 changes: 5 additions & 7 deletions h/static/scripts/directive/share-dialog.coffee
Expand Up @@ -15,13 +15,11 @@ module.exports = ['crossframe', (crossframe) ->
if visible
scope.$evalAsync(-> elem.find('#via').focus().select())

scope.$watchCollection (-> crossframe.providers), ->
if crossframe.providers?.length
# XXX: Consider multiple providers in the future
p = crossframe.providers[0]
if p.entities?.length
e = p.entities[0]
scope.viaPageLink = 'https://via.hypothes.is/' + e
scope.$watchCollection (-> crossframe.frames), (frames) ->
if not frames.length
return
# XXX: Consider sharing multiple frames in the future?
scope.viaPageLink = 'https://via.hypothes.is/' + frames[0].uri

restrict: 'A'
templateUrl: 'share_dialog.html'
Expand Down
4 changes: 2 additions & 2 deletions h/static/scripts/directive/test/share-dialog-test.coffee
Expand Up @@ -15,7 +15,7 @@ describe 'share-dialog', ->
beforeEach module('h.templates')

beforeEach module ($provide) ->
fakeCrossFrame = {providers: []}
fakeCrossFrame = {frames: []}

$provide.value 'crossframe', fakeCrossFrame
return
Expand All @@ -26,7 +26,7 @@ describe 'share-dialog', ->

it 'generates new via link', ->
$element = $compile('<div share-dialog="">')($scope)
fakeCrossFrame.providers.push {entities: ['http://example.com']}
fakeCrossFrame.frames.push({uri: 'http://example.com'})
$scope.$digest()
assert.equal($scope.viaPageLink,
'https://via.hypothes.is/http://example.com')
14 changes: 7 additions & 7 deletions h/static/scripts/test/cross-frame-test.coffee
Expand Up @@ -64,22 +64,22 @@ describe 'CrossFrame', ->
'source', 'origin', 'token')

it 'queries discovered frames for metadata', ->
info = {metadata: link: [{href: 'http://example.com'}]}
channel = {call: sandbox.stub().yieldsTo('success', info)}
uri = 'http://example.com'
channel = {call: sandbox.stub().yieldsTo('success', {uri: uri})}
fakeBridge.onConnect.yields(channel)
crossframe.connect()
assert.calledWith(channel.call, {
method: 'getDocumentInfo'
success: sinon.match.func
})

it 'updates the providers array', ->
info = {metadata: link: [{href: 'http://example.com'}]}
channel = {call: sandbox.stub().yieldsTo('success', info)}
it 'updates the frames array', ->
uri = 'http://example.com'
channel = {call: sandbox.stub().yieldsTo('success', {uri: uri})}
fakeBridge.onConnect.yields(channel)
crossframe.connect()
assert.deepEqual(crossframe.providers, [
{channel: channel, entities: ['http://example.com']}
assert.deepEqual(crossframe.frames, [
{channel: channel, uri: uri}
])


Expand Down
6 changes: 3 additions & 3 deletions h/static/scripts/test/widget-controller-test.coffee
Expand Up @@ -31,7 +31,7 @@ describe 'WidgetController', ->
clearSelectedAnnotations: sandbox.spy()
}
fakeAuth = {user: null}
fakeCrossFrame = {providers: []}
fakeCrossFrame = {frames: []}

fakeStore = {
SearchResource:
Expand Down Expand Up @@ -73,9 +73,9 @@ describe 'WidgetController', ->
sandbox.restore()

describe 'loadAnnotations', ->
it 'loads all annotation for a provider', ->
it 'loads all annotations for a frame', ->
viewer.chunkSize = 20
fakeCrossFrame.providers.push {entities: ['http://example.com']}
fakeCrossFrame.frames.push({uri: 'http://example.com'})
$scope.$digest()
loadSpy = fakeAnnotationMapper.loadAnnotations
assert.callCount(loadSpy, 5)
Expand Down
17 changes: 7 additions & 10 deletions h/static/scripts/widget-controller.coffee
Expand Up @@ -33,20 +33,17 @@ module.exports = class WidgetController

annotationMapper.loadAnnotations(results.rows)

loadAnnotations = ->
query = {}

for p in crossframe.providers
for e in p.entities when e not in loaded
loaded.push e
q = angular.extend(uri: e, query)
_loadAnnotationsFrom q, 0
loadAnnotations = (frames) ->
for f in frames
if f.uri in loaded
continue
loaded.push(f.uri)
_loadAnnotationsFrom({uri: f.uri}, 0)

streamFilter.resetFilter().addClause('/uri', 'one_of', loaded)

streamer.send({filter: streamFilter.getFilter()})

$scope.$watchCollection (-> crossframe.providers), loadAnnotations
$scope.$watchCollection (-> crossframe.frames), loadAnnotations

$scope.focus = (annotation) ->
if angular.isObject annotation
Expand Down

0 comments on commit 4f75515

Please sign in to comment.