Skip to content
This repository has been archived by the owner on Feb 29, 2020. It is now read-only.

Commit

Permalink
fix(systemaddon): Dedupe Topsites. Closes #2933.
Browse files Browse the repository at this point in the history
  • Loading branch information
piatra committed Aug 1, 2017
1 parent 956a8fb commit d782ac9
Show file tree
Hide file tree
Showing 13 changed files with 215 additions and 49 deletions.
40 changes: 40 additions & 0 deletions system-addon/common/Dedupe.jsm
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
this.Dedupe = class Dedupe {
constructor(createKey, compare) {
this.createKey = createKey || this.defaultCreateKey;
this.compare = compare || this.defaultCompare;
}

defaultCreateKey(item) {
return item;
}

defaultCompare() {
return false;
}

/**
* Dedupe an array containing groups of elements.
* Duplicate removal favors earlier groups.
*
* @param {Array} groups Contains an arbitrary number of arrays of elements.
* @returns {Array}
*/
group(groups) {
const globalKeys = new Set();
const result = [];
for (const values of groups) {
const valueMap = new Map();
for (const value of values) {
const key = this.createKey(value);
if (!globalKeys.has(key) && (!valueMap.has(key) || this.compare(valueMap.get(key), value))) {
valueMap.set(key, value);
}
}
result.push(valueMap);
valueMap.forEach((value, key) => globalKeys.add(key));
}
return result.map(m => Array.from(m.values()));
}
};

this.EXPORTED_SYMBOLS = ["Dedupe"];
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
Components.utils.importGlobalProperties(["URL"]);

/**
* shortURL - Creates a short version of a link's url, used for display purposes
* e.g. {url: http://www.foosite.com, eTLD: "com"} => "foosite"
Expand All @@ -13,7 +15,7 @@
* {str} link.title (optional) - The title of the link
* @return {str} A short url
*/
module.exports = function shortURL(link) {
this.shortURL = function shortURL(link) {
if (!link.url && !link.hostname) {
return "";
}
Expand All @@ -26,3 +28,5 @@ module.exports = function shortURL(link) {
// If URL and hostname are not present fallback to page title.
return hostname.slice(0, eTLDExtra).toLowerCase() || hostname || link.title || link.url;
};

this.EXPORTED_SYMBOLS = ["shortURL"];
4 changes: 1 addition & 3 deletions system-addon/content-src/components/Card/Card.jsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
const React = require("react");
const LinkMenu = require("content-src/components/LinkMenu/LinkMenu");
const shortURL = require("content-src/lib/short-url");
const {FormattedMessage} = require("react-intl");
const cardContextTypes = require("./types");
const {actionCreators: ac, actionTypes: at} = require("common/Actions.jsm");
Expand Down Expand Up @@ -48,15 +47,14 @@ class Card extends React.Component {
render() {
const {index, link, dispatch, contextMenuOptions, eventSource} = this.props;
const isContextMenuOpen = this.state.showContextMenu && this.state.activeCard === index;
const hostname = shortURL(link);
const {icon, intlID} = cardContextTypes[link.type];

return (<li className={`card-outer${isContextMenuOpen ? " active" : ""}`}>
<a href={link.url} onClick={this.onLinkClick}>
<div className="card">
{link.image && <div className="card-preview-image" style={{backgroundImage: `url(${link.image})`}} />}
<div className="card-details">
<div className="card-host-name"> {hostname} </div>
<div className="card-host-name"> {link.hostname} </div>
<div className={`card-text${link.image ? "" : " full-height"}`}>
<h4 className="card-title" dir="auto"> {link.title} </h4>
<p className="card-description" dir="auto"> {link.description} </p>
Expand Down
3 changes: 1 addition & 2 deletions system-addon/content-src/components/TopSites/TopSites.jsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
const React = require("react");
const {connect} = require("react-redux");
const {FormattedMessage} = require("react-intl");
const shortURL = require("content-src/lib/short-url");
const LinkMenu = require("content-src/components/LinkMenu/LinkMenu");
const {actionCreators: ac, actionTypes: at} = require("common/Actions.jsm");
const {perfService: perfSvc} = require("common/PerfService.jsm");
Expand Down Expand Up @@ -40,7 +39,7 @@ class TopSite extends React.Component {
render() {
const {link, index, dispatch} = this.props;
const isContextMenuOpen = this.state.showContextMenu && this.state.activeTile === index;
const title = link.pinTitle || shortURL(link);
const title = link.pinTitle || link.hostname;
const topSiteOuterClassName = `top-site-outer${isContextMenuOpen ? " active" : ""}`;
const {tippyTopIcon} = link;
let imageClassName;
Expand Down
3 changes: 1 addition & 2 deletions system-addon/content-src/lib/link-menu-options.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
const {actionTypes: at, actionCreators: ac} = require("common/Actions.jsm");
const shortURL = require("content-src/lib/short-url");

/**
* List of functions that return items that can be included as menu options in a
Expand Down Expand Up @@ -74,7 +73,7 @@ module.exports = {
icon: "pin",
action: ac.SendToMain({
type: at.TOP_SITES_PIN,
data: {site: {url: site.url, title: shortURL(site)}, index}
data: {site: {url: site.url, title: site.hostname}, index}
}),
userEvent: "PIN"
}),
Expand Down
24 changes: 17 additions & 7 deletions system-addon/lib/TopSitesFeed.jsm
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ Cu.import("resource://gre/modules/XPCOMUtils.jsm");
const {actionCreators: ac, actionTypes: at} = Cu.import("resource://activity-stream/common/Actions.jsm", {});
const {TippyTopProvider} = Cu.import("resource://activity-stream/lib/TippyTopProvider.jsm", {});
const {insertPinned} = Cu.import("resource://activity-stream/common/Reducers.jsm", {});
const {Dedupe} = Cu.import("resource://activity-stream/common/Dedupe.jsm", {});
const {shortURL} = Cu.import("resource://activity-stream/common/ShortURL.jsm", {});

XPCOMUtils.defineLazyModuleGetter(this, "NewTabUtils",
"resource://gre/modules/NewTabUtils.jsm");
Expand All @@ -25,6 +27,10 @@ this.TopSitesFeed = class TopSitesFeed {
this.lastUpdated = 0;
this._tippyTopProvider = new TippyTopProvider();
this._tippyTopProvider.init();
this.dedupe = new Dedupe(this._dedupeKey);
}
_dedupeKey(site) {
return site && site.hostname;
}
refreshDefaults(sites) {
// Clear out the array of any previous defaults
Expand Down Expand Up @@ -57,7 +63,17 @@ this.TopSitesFeed = class TopSitesFeed {
frecent = frecent.filter(link => link && link.type !== "affiliate");
}

return insertPinned([...frecent, ...DEFAULT_TOP_SITES], pinned).slice(0, TOP_SITES_SHOWMORE_LENGTH);
// Group together websites that require deduping.
let topsitesGroup = [];
for (const group of [pinned, frecent, DEFAULT_TOP_SITES]) {
topsitesGroup.push(group.filter(site => site).map(site => Object.assign({}, site, {hostname: shortURL(site)})));
}

const dedupedGroups = this.dedupe.group(topsitesGroup);
// Insert original pinned websites in the result of the dedupe operation.
pinned = insertPinned([...dedupedGroups[1], ...dedupedGroups[2]], pinned);

return pinned.slice(0, TOP_SITES_SHOWMORE_LENGTH);
}
async refresh(target = null) {
const links = await this.getLinksWithDefaults();
Expand Down Expand Up @@ -119,15 +135,9 @@ this.TopSitesFeed = class TopSitesFeed {
}));
}
onAction(action) {
let realRows;
switch (action.type) {
case at.NEW_TAB_LOAD:
// Only check against real rows returned from history, not default ones.
realRows = this.store.getState().TopSites.rows.filter(row => !row.isDefault);
if (
// When a new tab is opened, if we don't have enough top sites yet, refresh the data.
(realRows.length < TOP_SITES_SHOWMORE_LENGTH) ||

// When a new tab is opened, if the last time we refreshed the data
// is greater than 15 minutes, refresh the data.
(Date.now() - this.lastUpdated >= UPDATE_TIME)
Expand Down
2 changes: 2 additions & 0 deletions system-addon/lib/TopStoriesFeed.jsm
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ Cu.importGlobalProperties(["fetch"]);

const {actionCreators: ac, actionTypes: at} = Cu.import("resource://activity-stream/common/Actions.jsm", {});
const {Prefs} = Cu.import("resource://activity-stream/lib/ActivityStreamPrefs.jsm", {});
const {shortURL} = Cu.import("resource://activity-stream/common/ShortURL.jsm", {});

const STORIES_UPDATE_TIME = 30 * 60 * 1000; // 30 minutes
const TOPICS_UPDATE_TIME = 3 * 60 * 60 * 1000; // 3 hours
Expand Down Expand Up @@ -85,6 +86,7 @@ this.TopStoriesFeed = class TopStoriesFeed {
.filter(s => !NewTabUtils.blockedLinks.isBlocked({"url": s.dedupe_url}))
.map(s => ({
"guid": s.id,
"hostname": shortURL(Object.assign({}, s, {url: s.dedupe_url})),
"type": (Date.now() - (s.published_timestamp * 1000)) <= STORIES_NOW_THRESHOLD ? "now" : "trending",
"title": s.title,
"description": s.excerpt,
Expand Down
42 changes: 42 additions & 0 deletions system-addon/test/unit/common/Dedupe.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
const {Dedupe} = require("common/Dedupe.jsm");

describe("Dedupe", () => {
let instance;
beforeEach(() => {
instance = new Dedupe();
});
describe("group", () => {
it("should remove duplicates inside the groups", () => {
const beforeItems = [[1, 1, 1], [2, 2, 2], [3, 3, 3]];
const afterItems = [[1], [2], [3]];
assert.deepEqual(instance.group(beforeItems), afterItems);
});
it("should remove duplicates between groups, favouring earlier groups", () => {
const beforeItems = [[1, 2, 3], [2, 3, 4], [3, 4, 5]];
const afterItems = [[1, 2, 3], [4], [5]];
assert.deepEqual(instance.group(beforeItems), afterItems);
});
it("should remove duplicates from groups of objects", () => {
instance = new Dedupe(item => item.id);
const beforeItems = [[{id: 1}, {id: 1}, {id: 2}], [{id: 1}, {id: 3}, {id: 2}], [{id: 1}, {id: 2}, {id: 5}]];
const afterItems = [[{id: 1}, {id: 2}], [{id: 3}], [{id: 5}]];
assert.deepEqual(instance.group(beforeItems), afterItems);
});
it("should take a custom comparison function", () => {
function compare(previous, current) {
return current.amount > previous.amount;
}
instance = new Dedupe(item => item.id, compare);
const beforeItems = [
[{id: 1, amount: 50}, {id: 1, amount: 100}],
[{id: 1, amount: 200}, {id: 2, amount: 0}, {id: 2, amount: 100}]
];
const afterItems = [
[{id: 1, amount: 100}],
[{id: 2, amount: 100}]
];

assert.deepEqual(instance.group(beforeItems), afterItems);
});
});
});
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const shortURL = require("content-src/lib/short-url");
const {shortURL} = require("common/ShortURL.jsm");

describe("shortURL", () => {
it("should return a blank string if url and hostname is falsey", () => {
Expand Down Expand Up @@ -30,6 +30,15 @@ describe("shortURL", () => {
assert.equal(shortURL({url: "http://localhost:8000/", eTLD: "localhost"}), "localhost");
});

it("should fallback to link title if it exists", () => {
const link = {
url: "file:///Users/voprea/Work/activity-stream/logs/coverage/system-addon/report-html/index.html",
title: "Code coverage report"
};

assert.equal(shortURL(link), link.title);
});

it("should return the url if no hostname or title is provided", () => {
const url = "file://foo/bar.txt";
assert.equal(shortURL({url, eTLD: "foo"}), url);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ const {_unconnected: LinkMenu} = require("content-src/components/LinkMenu/LinkMe
const ContextMenu = require("content-src/components/ContextMenu/ContextMenu");
const {IntlProvider} = require("react-intl");
const messages = require("data/locales.json")["en-US"];
const shortURL = require("content-src/lib/short-url");

describe("<LinkMenu>", () => {
let wrapper;
Expand Down Expand Up @@ -91,7 +90,7 @@ describe("<LinkMenu>", () => {
describe(".onClick", () => {
const FAKE_INDEX = 3;
const FAKE_SOURCE = "TOP_SITES";
const FAKE_SITE = {url: "https://foo.com", referrer: "https://foo.com/ref", title: "bar", bookmarkGuid: 1234};
const FAKE_SITE = {url: "https://foo.com", referrer: "https://foo.com/ref", title: "bar", bookmarkGuid: 1234, hostname: "foo"};
const dispatch = sinon.stub();
const propOptions = ["Separator", "RemoveBookmark", "AddBookmark", "OpenInNewWindow", "OpenInPrivateWindow", "BlockUrl", "DeleteUrl", "PinTopSite", "UnpinTopSite", "SaveToPocket"];
const expectedActionData = {
Expand All @@ -101,7 +100,7 @@ describe("<LinkMenu>", () => {
menu_action_open_private_window: {url: FAKE_SITE.url, referrer: FAKE_SITE.referrer},
menu_action_dismiss: FAKE_SITE.url,
menu_action_delete: FAKE_SITE.url,
menu_action_pin: {site: {url: FAKE_SITE.url, title: shortURL(FAKE_SITE)}, index: FAKE_INDEX},
menu_action_pin: {site: {url: FAKE_SITE.url, title: FAKE_SITE.hostname}, index: FAKE_INDEX},
menu_action_unpin: {site: {url: FAKE_SITE.url}},
menu_action_save_to_pocket: {site: {url: FAKE_SITE.url, title: FAKE_SITE.title}}
};
Expand Down
11 changes: 2 additions & 9 deletions system-addon/test/unit/content-src/components/TopSites.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ describe("<TopSites>", () => {
describe("<TopSite>", () => {
let link;
beforeEach(() => {
link = {url: "https://foo.com", screenshot: "foo.jpg"};
link = {url: "https://foo.com", screenshot: "foo.jpg", hostname: "foo"};
});

it("should render a TopSite", () => {
Expand All @@ -196,20 +196,13 @@ describe("<TopSite>", () => {
});
it("should render a shortened title based off the url", () => {
link.url = "https://www.foobar.org";
link.hostname = "foobar";
link.eTLD = "org";
const wrapper = shallow(<TopSite link={link} />);
const titleEl = wrapper.find(".title");

assert.equal(titleEl.text(), "foobar");
});
it("should fallback to link title for file:// protocol", () => {
link.url = "file:///Users/voprea/Work/activity-stream/logs/coverage/system-addon/report-html/index.html";
link.title = "Code coverage report";
const wrapper = shallow(<TopSite link={link} />);
const titleEl = wrapper.find(".title");

assert.equal(titleEl.text(), link.title);
});
it("should render the pinTitle if set", () => {
link.isPinned = true;
link.pinnedIndex = 7;
Expand Down
Loading

0 comments on commit d782ac9

Please sign in to comment.