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

feat (experiment): #2065 screenshots in highlights experiment #2222

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 30 additions & 4 deletions addon/Feeds/HighlightsFeed.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const {Recommender} = require("common/recommender/Recommender");
const Feed = require("addon/lib/Feed");
const {TOP_SITES_LENGTH, HIGHLIGHTS_LENGTH} = require("common/constants");
const am = require("common/action-manager");
const getScreenshots = require("addon/lib/getScreenshots");

const UPDATE_TIME = 15 * 60 * 1000; // 15 minutes

Expand Down Expand Up @@ -44,7 +45,10 @@ module.exports = class HighlightsFeed extends Feed {
initializeRecommender(reason) {
return PlacesProvider.links.getAllHistoryItems().then(links => {
let highlightsCoefficients = this.getCoefficientsFromPrefs();
this.baselineRecommender = new Recommender(links, {highlightsCoefficients});
this.baselineRecommender = new Recommender(links, {
experiments: this.store.getState().Experiments.values,
highlightsCoefficients
});
}).then(() => this.refresh(reason));
}

Expand All @@ -54,13 +58,27 @@ module.exports = class HighlightsFeed extends Feed {
* @return Promise A promise that resolves with the "HIGHLIGHTS_RESPONSE" action
*/
getData() {
const experiments = this.store.getState().Experiments.values;
if (!this.baselineRecommender) {
return Promise.reject(new Error("Tried to get weighted highlights but there was no baselineRecommender"));
}
return PlacesProvider.links.getRecentlyVisited()
let highlights = PlacesProvider.links.getRecentlyVisited()
.then(links => this.options.getCachedMetadata(links, "HIGHLIGHTS_RESPONSE"))
.then(links => this.baselineRecommender.scoreEntries(links))
.then(links => am.actions.Response("HIGHLIGHTS_RESPONSE", links));
.then(links => this.baselineRecommender.scoreEntries(links));

if (experiments.bookmarkScreenshots) {
highlights = highlights.then(links => {
let lessLinks = links.slice(0, 18);
return getScreenshots(lessLinks, site => {
if (!site.images || site.images.length === 0) {
return true;
}
return false;
});
});
}

return highlights.then(links => am.actions.Response("HIGHLIGHTS_RESPONSE", links));
}

onAction(state, action) {
Expand Down Expand Up @@ -91,6 +109,14 @@ module.exports = class HighlightsFeed extends Feed {
this.baselineRecommender.updateOptions({highlightsCoefficients});
this.refresh("coefficients were changed");
}
if (action.data.name === "experiments.bookmarkScreenshots" && this.baselineRecommender) {
let highlightsCoefficients = this.getCoefficientsFromPrefs();
this.baselineRecommender.updateOptions({
experiments: this.store.getState().Experiments.values,
highlightsCoefficients
});
this.refresh("bookmarkScreenshots experiment changed");
}
break;
case am.type("SYNC_COMPLETE"):
// We always want new synced tabs.
Expand Down
10 changes: 6 additions & 4 deletions common/recommender/Baseline.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,12 @@ class Baseline {
newScore *= 0.2;
}

// Boost boomarks even if they have low score or no images giving a
// just-bookmarked page a near-infinite boost
if (features.bookmarkAge) {
newScore += BOOKMARK_AGE_DIVIDEND / features.bookmarkAge;
if (this.options.experiments && this.options.experiments.bookmarkScreenshots) {
// Boost boomarks even if they have low score or no images giving a
// just-bookmarked page a near-infinite boost
if (features.bookmarkAge) {
newScore += BOOKMARK_AGE_DIVIDEND / features.bookmarkAge;
}
}

return newScore;
Expand Down
4 changes: 3 additions & 1 deletion content-src/components/Spotlight/Spotlight.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,16 @@ const SpotlightItem = React.createClass({

if (imageUrl) {
style.backgroundImage = `url(${imageUrl})`;
} else if (site.screenshot) {
style.backgroundImage = `url(${site.screenshot})`;
} else {
style.backgroundColor = site.backgroundColor;
this.props.dispatch(actions.NotifyUndesiredEvent({
event: "MISSING_IMAGE",
source: "HIGHLIGHTS"
}));
}
return (<li className={classNames("spotlight-item", {active: this.state.showContextMenu})}>
return (<li className={classNames("spotlight-item", {active: this.state.showContextMenu, screenshot: site.screenshot})}>
<a onClick={this.props.onClick} className="spotlight-inner" href={site.url} ref="link">
<div className={classNames("spotlight-image", {portrait: isPortrait})} style={style} ref="image">
<SiteIcon className="spotlight-icon" height={40} width={40} site={site} ref="icon" showBackground={true} border={false} faviconSize={32} />
Expand Down
6 changes: 6 additions & 0 deletions content-src/components/Spotlight/Spotlight.scss
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,12 @@
font-size: $spotlight-font-size;
margin: 0;
}

&.screenshot {
.spotlight-image {
background-position: top;
}
}
}

// Overrides for Top Sites styling experiment.
Expand Down
47 changes: 45 additions & 2 deletions content-test/addon/Feeds/HighlightsFeed.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,18 +25,24 @@ const createStoreWithState = state => ({
}
});

const getScreenshots = sinon.spy(() => Promise.resolve(["foo"]));

describe("HighlightsFeed", () => {
let HighlightsFeed;
let instance;
let reduxState;
beforeEach(() => {
HighlightsFeed = require("inject!addon/Feeds/HighlightsFeed")({
"addon/PlacesProvider": {PlacesProvider},
"sdk/simple-prefs": simplePrefs,
"common/recommender/Recommender": {Recommender}
"common/recommender/Recommender": {Recommender},
"addon/lib/getScreenshots": getScreenshots
});
Object.keys(PlacesProvider.links).forEach(k => PlacesProvider.links[k].reset());
instance = new HighlightsFeed({getCachedMetadata});
instance.refresh = sinon.spy();
reduxState = {Experiments: {values: {}}};
instance.store = {getState() {return reduxState;}};
sinon.spy(instance.options, "getCachedMetadata");
});
it("should create a HighlightsFeed", () => {
Expand Down Expand Up @@ -73,7 +79,10 @@ describe("HighlightsFeed", () => {
simplePrefs.prefs.weightedHighlightsCoefficients = "[0, 1, 2]";
return instance.initializeRecommender().then(() => {
assert.equal(instance.baselineRecommender.args[0], testLinks);
assert.deepEqual(instance.baselineRecommender.args[1], {highlightsCoefficients: [0, 1, 2]});
assert.deepEqual(instance.baselineRecommender.args[1], {
experiments: reduxState.Experiments.values,
highlightsCoefficients: [0, 1, 2]
});
});
});
it("should call refresh with the reason", () => {
Expand Down Expand Up @@ -114,6 +123,40 @@ describe("HighlightsFeed", () => {
assert.calledWithExactly(instance.baselineRecommender.scoreEntries, getCachedMetadata(testLinks))
));
});
it("should call getScreenshots when in the bookmarkScreenshots experiment", () => {
instance.baselineRecommender = {scoreEntries: sinon.spy(links => links)};
reduxState.Experiments.values.bookmarkScreenshots = true;
return instance.getData().then(result => {
assert.calledOnce(getScreenshots);
// Note: our fake getScreenshots function resolves with ["foo"]
assert.deepEqual(result.data, ["foo"]);
});
});
it("should add screenshots when no other images are available", () => {
instance.baselineRecommender = {scoreEntries: sinon.spy(links => links)};
reduxState.Experiments.values.bookmarkScreenshots = true;
return instance.getData().then(result => {
// The function that determines whether a screenshot should be captured
// is passed into getScreenshots() as the second argument.
const shouldGetScreenshot = getScreenshots.getCall(0).args[1];
assert.equal(
true,
shouldGetScreenshot({images: []})
);
assert.equal(
true,
shouldGetScreenshot({images: null})
);
assert.equal(
true,
shouldGetScreenshot({})
);
assert.equal(
false,
shouldGetScreenshot({images: ["foo"]})
);
});
});
});
describe("#onAction", () => {
let clock;
Expand Down
9 changes: 7 additions & 2 deletions content-test/common/Baseline.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,13 @@ describe("Baseline", () => {

beforeEach(() => {
sandbox = sinon.sandbox.create();
baseline = new Baseline(fakeHistory,
{highlightsCoefficients: [-0.1, -0.1, -0.1, 0.4, 0.2]});
baseline = new Baseline(
fakeHistory,
{
experiments: {bookmarkScreenshots: true},
highlightsCoefficients: [-0.1, -0.1, -0.1, 0.4, 0.2]
}
);
});

afterEach(() => {
Expand Down
15 changes: 15 additions & 0 deletions experiments.json
Original file line number Diff line number Diff line change
Expand Up @@ -148,5 +148,20 @@
"threshold": 0.2,
"description": "Fetch page content locally"
}
},
"bookmarkScreenshots": {
"name": "Screenshots for Highlights",
"active": true,
"description": "Show a screenshot for highlights that otherwise don't have an image",
"control": {
"value": false,
"description": "Don't show screenshots"
},
"variant": {
"id": "exp-011-bookmark-screenshots",
"value": true,
"threshold": 0.2,
"description": "Show screenshots"
}
}
}