Commit 28ac128e authored by Nick Stenning's avatar Nick Stenning

Only make one search query per frame on page load

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.
parent 0b2a6109
# 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. # the interface remains in sync.
module.exports = class AnnotationUISync module.exports = class AnnotationUISync
###* ###*
......
# Instantiates all objects used for cross frame discovery and communication. # Instantiates all objects used for cross frame discovery and communication.
module.exports = class CrossFrame module.exports = class CrossFrame
providers: null
this.inject = [ this.inject = [
'$rootScope', '$document', '$window', 'store', 'annotationUI' '$rootScope', '$document', '$window', 'store', 'annotationUI'
'Discovery', 'bridge', 'Discovery', 'bridge',
...@@ -12,7 +10,7 @@ module.exports = class CrossFrame ...@@ -12,7 +10,7 @@ module.exports = class CrossFrame
Discovery, bridge, Discovery, bridge,
AnnotationSync, AnnotationUISync AnnotationSync, AnnotationUISync
) -> ) ->
@providers = [] @frames = []
createDiscovery = -> createDiscovery = ->
options = options =
...@@ -46,20 +44,17 @@ module.exports = class CrossFrame ...@@ -46,20 +44,17 @@ module.exports = class CrossFrame
createAnnotationUISync = (annotationSync) -> createAnnotationUISync = (annotationSync) ->
new AnnotationUISync($rootScope, $window, bridge, annotationSync, annotationUI) new AnnotationUISync($rootScope, $window, bridge, annotationSync, annotationUI)
addProvider = (channel) => addFrame = (channel) =>
provider = {channel: channel, entities: []}
channel.call channel.call
method: 'getDocumentInfo' method: 'getDocumentInfo'
success: (info) => success: (info) =>
$rootScope.$apply => $rootScope.$apply =>
provider.entities = (link.href for link in info.metadata.link) @frames.push({channel: channel, uri: info.uri})
@providers.push(provider)
this.connect = -> this.connect = ->
discovery = createDiscovery() discovery = createDiscovery()
bridge.onConnect(addProvider) bridge.onConnect(addFrame)
annotationSync = createAnnotationSync() annotationSync = createAnnotationSync()
annotationUISync = createAnnotationUISync(annotationSync) annotationUISync = createAnnotationUISync(annotationSync)
......
...@@ -15,13 +15,11 @@ module.exports = ['crossframe', (crossframe) -> ...@@ -15,13 +15,11 @@ module.exports = ['crossframe', (crossframe) ->
if visible if visible
scope.$evalAsync(-> elem.find('#via').focus().select()) scope.$evalAsync(-> elem.find('#via').focus().select())
scope.$watchCollection (-> crossframe.providers), -> scope.$watchCollection (-> crossframe.frames), (frames) ->
if crossframe.providers?.length if not frames.length
# XXX: Consider multiple providers in the future return
p = crossframe.providers[0] # XXX: Consider sharing multiple frames in the future?
if p.entities?.length scope.viaPageLink = 'https://via.hypothes.is/' + frames[0].uri
e = p.entities[0]
scope.viaPageLink = 'https://via.hypothes.is/' + e
restrict: 'A' restrict: 'A'
templateUrl: 'share_dialog.html' templateUrl: 'share_dialog.html'
......
...@@ -15,7 +15,7 @@ describe 'share-dialog', -> ...@@ -15,7 +15,7 @@ describe 'share-dialog', ->
beforeEach module('h.templates') beforeEach module('h.templates')
beforeEach module ($provide) -> beforeEach module ($provide) ->
fakeCrossFrame = {providers: []} fakeCrossFrame = {frames: []}
$provide.value 'crossframe', fakeCrossFrame $provide.value 'crossframe', fakeCrossFrame
return return
...@@ -26,7 +26,7 @@ describe 'share-dialog', -> ...@@ -26,7 +26,7 @@ describe 'share-dialog', ->
it 'generates new via link', -> it 'generates new via link', ->
$element = $compile('<div share-dialog="">')($scope) $element = $compile('<div share-dialog="">')($scope)
fakeCrossFrame.providers.push {entities: ['http://example.com']} fakeCrossFrame.frames.push({uri: 'http://example.com'})
$scope.$digest() $scope.$digest()
assert.equal($scope.viaPageLink, assert.equal($scope.viaPageLink,
'https://via.hypothes.is/http://example.com') 'https://via.hypothes.is/http://example.com')
...@@ -64,8 +64,8 @@ describe 'CrossFrame', -> ...@@ -64,8 +64,8 @@ describe 'CrossFrame', ->
'source', 'origin', 'token') 'source', 'origin', 'token')
it 'queries discovered frames for metadata', -> it 'queries discovered frames for metadata', ->
info = {metadata: link: [{href: 'http://example.com'}]} uri = 'http://example.com'
channel = {call: sandbox.stub().yieldsTo('success', info)} channel = {call: sandbox.stub().yieldsTo('success', {uri: uri})}
fakeBridge.onConnect.yields(channel) fakeBridge.onConnect.yields(channel)
crossframe.connect() crossframe.connect()
assert.calledWith(channel.call, { assert.calledWith(channel.call, {
...@@ -73,13 +73,13 @@ describe 'CrossFrame', -> ...@@ -73,13 +73,13 @@ describe 'CrossFrame', ->
success: sinon.match.func success: sinon.match.func
}) })
it 'updates the providers array', -> it 'updates the frames array', ->
info = {metadata: link: [{href: 'http://example.com'}]} uri = 'http://example.com'
channel = {call: sandbox.stub().yieldsTo('success', info)} channel = {call: sandbox.stub().yieldsTo('success', {uri: uri})}
fakeBridge.onConnect.yields(channel) fakeBridge.onConnect.yields(channel)
crossframe.connect() crossframe.connect()
assert.deepEqual(crossframe.providers, [ assert.deepEqual(crossframe.frames, [
{channel: channel, entities: ['http://example.com']} {channel: channel, uri: uri}
]) ])
......
...@@ -31,7 +31,7 @@ describe 'WidgetController', -> ...@@ -31,7 +31,7 @@ describe 'WidgetController', ->
clearSelectedAnnotations: sandbox.spy() clearSelectedAnnotations: sandbox.spy()
} }
fakeAuth = {user: null} fakeAuth = {user: null}
fakeCrossFrame = {providers: []} fakeCrossFrame = {frames: []}
fakeStore = { fakeStore = {
SearchResource: SearchResource:
...@@ -73,9 +73,9 @@ describe 'WidgetController', -> ...@@ -73,9 +73,9 @@ describe 'WidgetController', ->
sandbox.restore() sandbox.restore()
describe 'loadAnnotations', -> describe 'loadAnnotations', ->
it 'loads all annotation for a provider', -> it 'loads all annotations for a frame', ->
viewer.chunkSize = 20 viewer.chunkSize = 20
fakeCrossFrame.providers.push {entities: ['http://example.com']} fakeCrossFrame.frames.push({uri: 'http://example.com'})
$scope.$digest() $scope.$digest()
loadSpy = fakeAnnotationMapper.loadAnnotations loadSpy = fakeAnnotationMapper.loadAnnotations
assert.callCount(loadSpy, 5) assert.callCount(loadSpy, 5)
......
...@@ -33,20 +33,17 @@ module.exports = class WidgetController ...@@ -33,20 +33,17 @@ module.exports = class WidgetController
annotationMapper.loadAnnotations(results.rows) annotationMapper.loadAnnotations(results.rows)
loadAnnotations = -> loadAnnotations = (frames) ->
query = {} for f in frames
if f.uri in loaded
for p in crossframe.providers continue
for e in p.entities when e not in loaded loaded.push(f.uri)
loaded.push e _loadAnnotationsFrom({uri: f.uri}, 0)
q = angular.extend(uri: e, query)
_loadAnnotationsFrom q, 0
streamFilter.resetFilter().addClause('/uri', 'one_of', loaded) streamFilter.resetFilter().addClause('/uri', 'one_of', loaded)
streamer.send({filter: streamFilter.getFilter()}) streamer.send({filter: streamFilter.getFilter()})
$scope.$watchCollection (-> crossframe.providers), loadAnnotations $scope.$watchCollection (-> crossframe.frames), loadAnnotations
$scope.focus = (annotation) -> $scope.focus = (annotation) ->
if angular.isObject annotation if angular.isObject annotation
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment