Commit 86e20797 authored by Robert Knight's avatar Robert Knight

Send content banner data to all guests

Even though this data is only used by (or at least, we expect it to only be used
by) the main guest, we can get it there faster if we send the data to all
guests. This avoids the need to wait for the `documentInfoChanged` event to be
sent by the guest to the sidebar, in order for the sidebar to know whether a
guest is the main one or not. In the context of PDFs, this event is only sent
once the full PDF has been loaded.

If in future we do need to distinguish between main and non-main guests when
sending this event, it would be better to rework the guest <-> sidebar
connection protocol so that the sidebar can know immediately whether a guest is
the main one or a sub-frame, without having to wait for `documentInfoChanged`.

As a bonus, sending the data to all guests simplifies some tests.
parent fa912dcb
...@@ -243,8 +243,11 @@ export class FrameSyncService { ...@@ -243,8 +243,11 @@ export class FrameSyncService {
this._store.subscribe, this._store.subscribe,
() => this._store.getContentInfo(), () => this._store.getContentInfo(),
contentInfo => { contentInfo => {
const mainGuest = this._guestRPC.get(null); // We send the content info to all guests, even though it is only needed
mainGuest?.call('showContentInfo', contentInfo); // by the main one. See notes in `_connectGuest`.
this._guestRPC.forEach(guest => {
guest.call('showContentInfo', contentInfo);
});
} }
); );
} }
...@@ -282,16 +285,6 @@ export class FrameSyncService { ...@@ -282,16 +285,6 @@ export class FrameSyncService {
frameIdentifier = info.frameIdentifier; frameIdentifier = info.frameIdentifier;
this._guestRPC.set(frameIdentifier, guestRPC); this._guestRPC.set(frameIdentifier, guestRPC);
// Show the content info banner, if this is the main guest frame and
// if we already have the data for it.
//
// We send this only after "documentInfoChanged" is received, because
// we don't know before then if this is the main guest or not.
const contentInfo = this._store.getContentInfo();
if (frameIdentifier === null && contentInfo) {
guestRPC.call('showContentInfo', contentInfo);
}
this._store.connectFrame({ this._store.connectFrame({
id: info.frameIdentifier, id: info.frameIdentifier,
metadata: info.metadata, metadata: info.metadata,
...@@ -406,6 +399,17 @@ export class FrameSyncService { ...@@ -406,6 +399,17 @@ export class FrameSyncService {
// Synchronize highlight visibility in this guest with the sidebar's controls. // Synchronize highlight visibility in this guest with the sidebar's controls.
guestRPC.call('setHighlightsVisible', this._highlightsVisible); guestRPC.call('setHighlightsVisible', this._highlightsVisible);
guestRPC.call('featureFlagsUpdated', this._store.profile().features); guestRPC.call('featureFlagsUpdated', this._store.profile().features);
// If we have content banner data, send it to the guest. If there are
// multiple guests the banner is likely only appropriate for the main one.
// Current contexts that use the banner only have one guest, so we can get
// the data to the guest faster by sending it immediately, rather than
// waiting for the `documentInfoChanged` event to tell us which is the main
// guest.
const contentInfo = this._store.getContentInfo();
if (contentInfo) {
guestRPC.call('showContentInfo', contentInfo);
}
} }
/** /**
......
...@@ -605,21 +605,8 @@ describe('FrameSyncService', () => { ...@@ -605,21 +605,8 @@ describe('FrameSyncService', () => {
assert.calledWith(channel.call, 'setHighlightsVisible', false); assert.calledWith(channel.call, 'setHighlightsVisible', false);
}); });
[ [true, false].forEach(haveContentInfo => {
{ it('sends content info to guest if available', async () => {
haveContentInfo: false,
isMainGuest: true,
},
{
haveContentInfo: true,
isMainGuest: false,
},
{
haveContentInfo: true,
isMainGuest: true,
},
].forEach(({ haveContentInfo, isMainGuest }) => {
it('sends content info to main guest if available', async () => {
let channel; let channel;
setupPortRPC = rpc => { setupPortRPC = rpc => {
channel = rpc; channel = rpc;
...@@ -632,12 +619,12 @@ describe('FrameSyncService', () => { ...@@ -632,12 +619,12 @@ describe('FrameSyncService', () => {
await connectGuest(); await connectGuest();
emitGuestEvent('documentInfoChanged', { emitGuestEvent('documentInfoChanged', {
uri: 'https://publisher.org/article.pdf', uri: 'https://publisher.org/article.pdf',
frameIdentifier: isMainGuest ? null : 'sub-frame', frameIdentifier: null,
}); });
assert.equal( assert.equal(
channel.call.calledWith('showContentInfo', contentInfo), channel.call.calledWith('showContentInfo', contentInfo),
haveContentInfo && isMainGuest haveContentInfo
); );
}); });
}); });
...@@ -829,7 +816,7 @@ describe('FrameSyncService', () => { ...@@ -829,7 +816,7 @@ describe('FrameSyncService', () => {
context('when content info in store changes', () => { context('when content info in store changes', () => {
const contentInfo = { item: { title: 'Some article' } }; const contentInfo = { item: { title: 'Some article' } };
it('sends content info to main guest', async () => { it('sends new content info to guests', async () => {
await frameSync.connect(); await frameSync.connect();
await connectGuest(); await connectGuest();
emitGuestEvent('documentInfoChanged', { emitGuestEvent('documentInfoChanged', {
...@@ -841,18 +828,5 @@ describe('FrameSyncService', () => { ...@@ -841,18 +828,5 @@ describe('FrameSyncService', () => {
assert.calledWith(guestRPC().call, 'showContentInfo', contentInfo); assert.calledWith(guestRPC().call, 'showContentInfo', contentInfo);
}); });
it("doesn't send content info to non-main guests", async () => {
await frameSync.connect();
await connectGuest();
emitGuestEvent('documentInfoChanged', {
uri: 'https://publisher.org/article.pdf',
frameIdentifier: 'sub-frame',
});
fakeStore.setContentInfo(contentInfo);
assert.isFalse(guestRPC().call.calledWith('showContentInfo'));
});
}); });
}); });
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