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

VERIFIED FIXED in Firefox 38

Status

()

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: Mardak, Assigned: Mardak)

Tracking

Trunk
Firefox 39
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox38 verified, firefox39 verified)

Details

(Whiteboard: .004)

Attachments

(2 attachments, 5 obsolete attachments)

Assignee

Description

4 years ago
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.
Flags: needinfo?(kghim)
Assignee

Comment 1

4 years ago
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.
Flags: needinfo?(kghim)

Comment 2

4 years ago
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.
Assignee

Comment 3

4 years ago
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.
Summary: Only show a suggested tile for some number of times → Only show a suggested tile url for some number of times or until clicked
Assignee

Comment 4

4 years ago
For initial 38 implementation, we'll do an in-memory cap (i.e., capped for each Firefox launch) on a per tile basis.
Assignee: nobody → edilee
Assignee

Comment 5

4 years ago
Posted patch v1 (obsolete) — Splinter Review
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.
Attachment #8581249 - Flags: feedback?(msamuel)
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
Attachment #8581249 - Flags: feedback?(msamuel) → feedback+
Assignee

Comment 7

4 years ago
Posted patch v1.1 (obsolete) — Splinter Review
Added tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=650fefa34cfd
Attachment #8581249 - Attachment is obsolete: true
Attachment #8582945 - Flags: review?(adw)
Assignee

Updated

4 years ago
Iteration: --- → 39.3 - 30 Mar
Points: --- → 5
Whiteboard: .? → .004
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.
Assignee

Comment 9

4 years ago
Posted patch v2 (obsolete) — Splinter Review
(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.
Attachment #8582945 - Attachment is obsolete: true
Attachment #8582945 - Flags: review?(adw)
Attachment #8583676 - Flags: review?(adw)
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()
Assignee

Comment 11

4 years ago
(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.
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.
Assignee

Comment 13

4 years ago
Posted patch v3 (obsolete) — Splinter Review
Thanks emtwo for catching that. The new tests also pass with your changes without the odd maxNumLinks--/++.
Attachment #8583676 - Attachment is obsolete: true
Attachment #8583949 - Attachment is obsolete: true
Attachment #8583676 - Flags: review?(adw)
Attachment #8584010 - Flags: review?(adw)
Assignee

Comment 14

4 years ago
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?
Assignee

Comment 15

4 years ago
(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 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);
Attachment #8584010 - Flags: review?(adw) → review+
(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!))
Assignee

Comment 18

4 years ago
Posted patch for checkinSplinter Review
Thanks. I see your es6-...-fanciness and raise with more es6-method-fanciness!

_callObservers(methodName, ...args) {
Attachment #8584010 - Attachment is obsolete: true
Assignee

Comment 19

4 years ago
(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"
https://hg.mozilla.org/mozilla-central/rev/ac13bb5f374c
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Assignee

Updated

4 years ago
Flags: firefox-backlog+
Assignee

Comment 21

4 years ago
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
Assignee

Updated

4 years ago
Blocks: 1148859
Flags: qe-verify+
QA Contact: cornel.ionce
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?
Flags: needinfo?(edilee)
Assignee

Comment 24

4 years ago
(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?
Flags: needinfo?(edilee)
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.
Blocks: 1152352
This issue seems to be only appearing when e10s is enabled. I've filed a follow up: bug 1152352
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.
Status: RESOLVED → VERIFIED

Updated

4 years ago
Blocks: 1155443

Updated

4 years ago
No longer blocks: 1155443
Assignee

Updated

4 years ago
Blocks: 1159571
You need to log in before you can comment on or make changes to this bug.