Skip to content
This repository has been archived by the owner. It is now read-only.

Bug 1476309 - use RemoteSettings for Tippy Top #4241

Merged
merged 1 commit into from Jul 19, 2018

Conversation

@rlr
Copy link
Contributor

@rlr rlr commented Jul 13, 2018

@ncloudioj to test it out, you need to change the prefs for RS. I just run the following in the browser console:

const SERVER_STAGE = "https://settings.stage.mozaws.net/v1";
const HASH_STAGE = "DB:74:CE:58:E4:F9:D0:9E:E0:42:36:BE:6C:C5:C4:F6:6A:E7:74:7D:C0:21:42:7A:03:BC:2F:57:0C:8B:9B:90";
Services.prefs.setCharPref("services.settings.server", SERVER_STAGE);
Services.prefs.setCharPref("security.content.signature.root_hash", HASH_STAGE);
@rlr rlr requested a review from ncloudioj Jul 13, 2018
@ncloudioj
Copy link
Member

@ncloudioj ncloudioj commented Jul 13, 2018

LGTM!

@leplatrem, could you also take a look at this please (the client side change of bug 1437671)?

The key change of this patch is that we use RemoteSettings to replace the old JSON Tippytop manifest (manually sync'd to its origin on S3).

Note that since the Tippytop functionality is not sensitive to the data latency (add/update/delete) in Activity Stream, we completely rely on the default syncing strategy of RS. We've also removed the memory cache for the Tippytop manifest, and implemented the lookup directly on top of RemoteSettings.get(). That at least considerably simplified the code : ).

Would love to hear your thoughts on those changes.

Copy link
Contributor

@leplatrem leplatrem left a comment

It looks good ! Nice refactor indeed :)

@@ -246,21 +152,27 @@ this.FaviconFeed = class FaviconFeed {
}
}

async getSite(domain) {
const sites = await this.tippyTop.get({filters: {"domain": domain}});

This comment has been minimized.

@leplatrem

leplatrem Jul 16, 2018
Contributor

nitpick: could also be shortened as this.tippyTop.get({filters: {domain}});

let iconUri = Services.io.newURI(sitesByDomain[domain].image_url);
const site = await this.getSite(getDomain(url));

if (site) {

This comment has been minimized.

@leplatrem

leplatrem Jul 16, 2018
Contributor

nitpick: since the rest of the function is useless when site is null, maybe:

if (!site) {
  return;
}
it("should lazy init _tippyTop", async () => {
assert.isUndefined(feed._tippyTop);
await feed.getSite("example.com");
assert.ok(feed._tippyTop);
});

This comment has been minimized.

@leplatrem

leplatrem Jul 16, 2018
Contributor

An alternative to this would be to store records in the local db:

const collection = await RemoteSettings("tippytop").openCollection();
await collection.create({
  domain,
  image_url: `${url}/icon.png`
});

But this may require to setup IndexedDB in your tests suite, which may not be obvious

@rlr rlr force-pushed the rlr:tippytop-remotesettings branch from 304888d to a3f4f1f Jul 17, 2018
@rlr rlr changed the title [WIP] Bug xxxxxx - use RemoteSettings for Tippy Top Bug 1476309 - use RemoteSettings for Tippy Top Jul 17, 2018
@rlr rlr force-pushed the rlr:tippytop-remotesettings branch from a3f4f1f to 92157c9 Jul 17, 2018
@rlr rlr merged commit b44d9a5 into mozilla:master Jul 19, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rlr rlr deleted the rlr:tippytop-remotesettings branch Aug 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants