1140496 - Only show a suggested tile url for some number of times or until clicked

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect)

Description

[Ed Lee :Mardak]

Given that we don't have many suggested tiles to pick from, we want to avoid show the same suggested tile over and over again to the same user especially if the user doesn't interact with it (so arguably we're now taking away space that could be showing a more useful tile).

One way is to limit how many times we show a given suggested tile.

A simple initial approach is to have Firefox only show a suggested tile X total times then stop showing it. Firefox would need a counter to remember how many times it showed a tile (probably by id).

Partially to avoid keeping state in Firefox forever for all suggested tiles, we could have Firefox reset its counters every day. This effectively makes the frequency cap a daily cap, which also matches up with the most common usage of frequency capping.

I'm not sure if we need to support both a total cap and a per/day cap. kghim?

Additionally, we could hard-code the cap for all suggested tiles in Firefox to simplify some server design initially. We'll want to support server-provided frequency caps eventually, but we could keep things simple for now.

Comment 1

[Ed Lee :Mardak]

Just chatted with kghim. We're leaning more towards a total cap for now. The number of views would be roughly 2-3x the average number of new tabs people open a day. We don't have the actual numbers of new tabs, so we're estimating a cap of 30-45.

Re: the caching-forever concern I raised, we won't have many tiles right now, so we don't need to think too hard about it now. Even without a clear-each-day approach, we could do stuff like "last shown on this date" and remove entries that are 30 days old.

Comment 2

[Kevin Ghim]

I'm in favor of setting a frequency cap of 30 per campaign (clickthrough URL for now).

Couple of additional items to consider:

1) For impression capping: there will be scenarios where there will be more than one ad group running for the same campaign. For example, Fennec targeting a) Android fans and b) Technology news readers. In this case, we'll create buckets where the sites won't overlap, so the frequency capping should be ideally per user. Since this might be complicated for the affiliate release, we should consider capping per tile clickthrough URL. This would ensure a user not seeing the same advertiser 60 times, if the frequency cap is set at 30 per tile.

2) For click capping: once the user clicks on the suggested tile, we should stop showing the tile. I believe this should also be set on the clickthrough URL and not per tile ID.

Comment 3

[Ed Lee :Mardak]

Updating bug title to cover capping by url instead of tile id as well as adding in the new functionality of capping when the user clicks.

I guess we could stop showing after the user clicked by increasing the internal frequency cap counter of the url to the max or infinity?


Potentially instead of immediately hiding the tile after a click, we could treat the click as "10 views" so the user could click 3 times to hit the "30 view" cap. Or alternatively, after a click, make the view count 1 less than the cap, so the user could have one more chance to see it if the user opens a new tab to then pin it.

Comment 4

[Ed Lee :Mardak]

For initial 38 implementation, we'll do an in-memory cap (i.e., capped for each Firefox launch) on a per tile basis.

Comment 5

[Ed Lee :Mardak]

Attachment: v1

Does this seem reasonable? Decided to count down as it would be similar to reading out a frequency cap value from the json. This is as opposed to counting up and needing to reference the current count as well as the total cap.

Comment 6

[Marina Samuel [:emtwo]]

Comment on attachment 8581249 [details] [diff] [review]
v1

Review of attachment 8581249 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me

::: browser/modules/DirectoryLinksProvider.jsm
@@ +628,2 @@
>      let flattenedLinks = [...possibleLinks.values()];
> +    if (flattenedLinks.length == 0) {

nit: I think you can check for possibleLinks.size one line earlier before creating flattenedLinks

Comment 7

[Ed Lee :Mardak]

Attachment: v1.1

Added tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=650fefa34cfd

Comment 8

[Drew Willcoxon :adw]

Comment on attachment 8582945 [details] [diff] [review]
v1.1

Review of attachment 8582945 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine, just one question before I stamp it:

::: browser/modules/DirectoryLinksProvider.jsm
@@ +584,5 @@
>      if (initialLength) {
>        let mostFrecentLink = sortedLinks[0];
>        if ("related" == mostFrecentLink.type) {
> +        // Force the maximum to be the same so the 0-frecency item is pushed out
> +        this.maxNumLinks = initialLength;

There's one other place we call onLinkChanged, below in this same method.  Links.onLinkChanged expects provider.maxNumLinks to be defined, at least in a couple of paths.  So only setting it here seems like a problem.  Or do you end up returning early, before the onLinkChanged call below, if sortedLinks is empty or the first link's type isn't related?  If that's true, a comment explaining the possible danger would be helpful, but even then, to avoid bugs in the future I think it would be a good idea to always set maxNumLinks, if it makes sense to do that.

Comment 9

[Ed Lee :Mardak]

Attachment: v2

(In reply to Drew Willcoxon :adw from comment #8)
> Links.onLinkChanged expects provider.maxNumLinks to be defined
> I think it would be a good idea to always set maxNumLinks
Indeed. We only allow one spot for suggested right now, and onLinkChanged is only used for adding/replacing the suggested link.

Turns out the suggested link wasn't getting pushed out as expected in the first place! So I've added checks in the test to make sure it's removed.

Comment 10

[Marina Samuel [:emtwo]]

Comment on attachment 8583676 [details] [diff] [review]
v2

Review of attachment 8583676 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/DirectoryLinksProvider.jsm
@@ +594,5 @@
>            url: mostFrecentLink.url,
>            frecency: 0,
>            lastVisitDate: mostFrecentLink.lastVisitDate,
>            type: "related",
>          }, 0, true);

We set the parameter as 'true' here for aDeleted. This should be triggering the delete rather than setting maxNumLinks to a smaller size.

I think the bug might be in the delete functionality in NewTabUtils.onLinkChanged()

Comment 11

[Ed Lee :Mardak]

(In reply to Marina Samuel [:emtwo] from comment #10)
> We set the parameter as 'true' here for aDeleted.
Hrm. Indeed, but if I remove the maxNumLinks--/++, the new test fails:

PASS [test_frequencyCappedSites_views : 468] "related" == "related"
PASS [test_frequencyCappedSites_views : 469] 2 == 2
PASS [test_frequencyCappedSites_views : 468] "organic" == "organic"
FAIL [test_frequencyCappedSites_views : 469] 2 == 1

Where that fail is for getLinks returning an organic tile plus related tile with frecency 0 as the second tile.

Comment 12

[Marina Samuel [:emtwo]]

Attachment: Bug with DirectoryLinksProvider._callObservers()

This kind of blew my mind because test_NewTabUtils had a notifyLinkDelete() test that was passing ....so I had to look at it (hope you don't mind, Ed) ... 

Looks like _callObservers was broken and not sending the extra index and delete params. Additionally, when attempting to delete a link with frecency '0' it didn't exist because it was stored with frecency 'Infinity'.

I just attached the changes I had which also make tests pass.

Comment 13

[Ed Lee :Mardak]

Attachment: v3

Thanks emtwo for catching that. The new tests also pass with your changes without the odd maxNumLinks--/++.

Comment 14

[Ed Lee :Mardak]

Comment on attachment 8583949 [details] [diff] [review]
Bug with DirectoryLinksProvider._callObservers()

>-  _callObservers: function DirectoryLinksProvider__callObservers(aMethodName, aArg) {
>+  _callObservers: function DirectoryLinksProvider__callObservers() {
>+    let observerMethodName = arguments[0];
>+    let args = Array.prototype.slice.call(arguments, 1);
>+    args.unshift(this);
>     for (let obs of this._observers) {
>-      if (typeof(obs[aMethodName]) == "function") {
>+      if (typeof(obs[observerMethodName]) == "function") {
>         try {
>-          obs[aMethodName](this, aArg);
>+          obs[observerMethodName].apply(NewTabUtils.links, args);
Actually... isn't there some new fancy ... splat splay or something?

Comment 15

[Ed Lee :Mardak]

(In reply to Ed Lee :Mardak from comment #14)
> Actually... isn't there some new fancy ... splat splay or something?
Aha! I was thinking of rest parameters, so we could do aMethodName, ...aArgs). Then I think we can do method.apply(links, aArgs) or method.call(links, ...aArgs)

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/rest_parameters
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_operator

Probably don't need both.

Comment 16

[Drew Willcoxon :adw]

Comment on attachment 8584010 [details] [diff] [review]
v3

Review of attachment 8584010 [details] [diff] [review]:
-----------------------------------------------------------------

Great!

::: browser/modules/DirectoryLinksProvider.jsm
@@ +672,5 @@
>    removeObserver: function DirectoryLinksProvider_removeObserver(aObserver) {
>      this._observers.delete(aObserver);
>    },
>  
> +  _callObservers: function DirectoryLinksProvider__callObservers() {

function DirectoryLinksProvider__callObservers(methodName, ...args) {

@@ +682,2 @@
>          try {
> +          obs[observerMethodName].apply(NewTabUtils.links, args);

obs[observerMethodName](this, ...args);

Comment 17

[Drew Willcoxon :adw]

(The thisArg of the obs call should be obs.  (Not to be confused with passing `this`, the DirectoryLinksProvider, as the first arg in the call!))

Comment 18

[Ed Lee :Mardak]

Attachment: for checkin

Thanks. I see your es6-...-fanciness and raise with more es6-method-fanciness!

_callObservers(methodName, ...args) {

Comment 19

[Ed Lee :Mardak]

(In reply to Kevin Ghim from email)
> - From the code it seems frequency capping will be available for not
> only Suggested but also Directory. Is this true?
Frequency cap is only for suggested tiles.

> - Can we set frequency caps per Tile (unique click through URL), per
> ad group, and per tile type (affiliate, sponsored, etc)?
It's capping impressions at a suggested tile url level.

> - How is the frequency cap set? Can it be adjusted periodically?
It's a hardcoded cap of 5 views per instance of Firefox. So if the
user restarts, the cap resets to 5 more views.

> - Can you verify that capping is done on both impression or clicks?
It stops showing after 5 views or until the first click.

> So, if I restart Firefox and load 5 tabs at once using session restore, I'd
> only see it once?
I'm not sure why it would only show once. Restoring 5 tabs is not opening 5 new tab pages.

> If I click on the suggested tile, will it get removed immediately or on the
> next instance? or next new tab?
Most people click a tile in a way that the loads the url in the current tab. I'm not sure what you're getting at next instance vs next new tab.

> I assume blocking will supersede memory cache. The next instance won't bring
> up a blocked suggested tile, right?
Blocking a tile work as usual.

"I understand this is a temporary solution - we're going to need to move quickly towards granular control of capping. Those can include:

    total capping, not per instance
    being able to set different impression or click capping per
        capping per campaign (this table needs to be created first)
        capping per ad group
        and further along would be per user per campaign/ad group/tile across different channels"

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ac13bb5f374c

Comment 21

[Ed Lee :Mardak]

For qe-verify, now that bug 1143745 is fixed, the instructions are similar to bug 1126188 comment 48 except with a different pref value:

1) change about:config pref "browser.newtabpage.directory.source" to..
https://people.mozilla.org/~msamuel/suggested2.json

2) load "irs.gov" from location bar

testing views:
3) open new tab and see the suggested tile
4) open up several more new tabs (at least 5) until the suggested tile is removed

testing click:
3) open new tab and click the suggested tile
4) open up another new tab and suggested tile should be gone

Comment 22

[Ed Lee :Mardak]

https://hg.mozilla.org/releases/mozilla-beta/rev/4afccec73fb9

Comment 23

[Cornel Ionce [:noni] [Hubs QA]]

While testing this on latest Nightly (build ID: 20150405030238) using the STR from comment 21, I've encountered the following:

1. testing views:
a. The suggested tile was removed if I've opened 5 or more new tabs
b. Force refreshing the page (ctrl+F5) will bring the suggested tile back -- not sure if this is expected.

2. testing click:
a. clicking the suggested tile, it removed it only after:
- a force refresh
- a browser restart
- opening newtab pages 5 times

Ed, would you mind having a look over these?

Comment 24

[Ed Lee :Mardak]

(In reply to Cornel Ionce [QA] from comment #23)
> b. Force refreshing the page (ctrl+F5) will bring the suggested tile back --
> not sure if this is expected.
This isn't expected, but I can't seem to reproduce on osx. Does this happen if you click on the reload icon in the location bar?

> a. clicking the suggested tile, it removed it only after:
> - a force refresh
How are you clicking on the tile? Are you opening the suggested tile in another tab so that the new tab page stays open? If so, how about if you click on the tile so that the current new tab page navigates away and opening a new tab?

Comment 25

[Cornel Ionce [:noni] [Hubs QA]]

Attachment: suggested_tile_behavior.gif

I've created an attachment with the suggested tile behavior using a clean profile on Windows 7 64-bit.

As it can be seen: 
- the suggested tile is not dismissed after clicking on it; opening a new tab after the page was completely loaded will still display it.
- the tile will be removed after several new tab pages opened.
- after a browser restart, opening a new tab will cause the suggested tile to be displayed again. If not, a reload/force refresh of the page will do.

Comment 26

[Marina Samuel [:emtwo]]

This issue seems to be only appearing when e10s is enabled. I've filed a follow up: bug 1152352

Comment 27

[Cornel Ionce [:noni] [Hubs QA]]

Thank you Marina!

I can confirm that everything works as expected on Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit using:
- Firefox 38 beta 4, build ID: 20150413143743;
- latest Firefox 39.0a2, build ID: 20150414004003
- Latest Nightly (e10s disabled), build ID: 20150414130911.