Closed Bug 1156549 Opened 5 years ago Closed 5 years ago

Allow ramp up time for campaigns with strict start/stop times

Categories

(Firefox :: New Tab Page, defect)

defect
Not set
Points:
8

Tracking

()

VERIFIED FIXED
Firefox 40
Iteration:
40.3 - 11 May
Tracking Status
firefox40 --- verified
firefox41 --- verified

People

(Reporter: kghim, Assigned: mzhilyaev)

References

(Blocks 1 open bug)

Details

(Whiteboard: .002)

Attachments

(3 files, 5 obsolete files)

For shorter campaigns, ramping up impressions before the actual start date/time would be beneficial. Firefox takes time to distribute tiles and we need to figure out a way to allow for ramp up time for time sensitive campaigns. The criteria are:
1) Allow for a mechanism to ramp up with out effecting paid impression during live campaign dates/times
2) Don't allow users to click on this tile before start date/time. A click using the client's URL could be a breach of contract

This may work by:
1) Storing and serving start/stop times
2) By Firefox using local machine to show/hide tiles
Whiteboard: .? → .002
maksik, one potential solution is to change _updateSuggestedTile to skip the suggested link when iterating through suggestedLinksMap.forEach((suggestedLink, url) => {

We don't need to immediately show a suggestion at the time. We just need to not show a suggestion too early.
Assignee: nobody → mzhilyaev
Iteration: --- → 40.3 - 11 May
Component: Tiles → New Tab Page
Product: Content Services → Firefox
Blocks: 1140185
Attached patch v1 (obsolete) — Splinter Review
Reattaching from bug 1156542
Attachment #8600165 - Flags: feedback?(msamuel)
Attachment #8600165 - Attachment is patch: true
Summary: Allow ramp up time for campaigns → Allow ramp up time for campaigns with strict start/stop times
Duplicate of this bug: 1156555
Blocks: 1160372
copying Mardak's comment on newtab preloading and potential danger of showing tile passed end time:

a potentially simple fix is when a new tab page is being shown, it checks if a suggested tile should be hidden because it's expired. We could try to be smart and find a new suggested tile to show, but missing one opportunity should be fine.

One thing to be careful of is the new tab page is preloaded, so even if something didn't expire yet when preloading, it could be expired when the user opens up a new tab. That's why I suggest just hiding a suggestion as it's being shown.
Comment on attachment 8600165 [details] [diff] [review]
v1

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

::: browser/modules/DirectoryLinksProvider.jsm
@@ +351,5 @@
> +  _setupStartEndTime: function DirectoryLinksProvider_setupStartEndTime(link) {
> +    // set start/end limits if they exist. If dates are unparsable
> +    // Date.parse() returns NaN, which would fail existence check
> +    // when we try to use it - badly formatted date acts as empty date
> +    if (link.time_limits) {

nit: I would do if (!link.time_limits) { return; } to avoid nested if statements.. but it's pretty much just a style difference so up to you.

@@ +362,5 @@
> +    }
> +  },
> +
> +  /*
> +   * Handles capmpaign timeout

nit: typo: capmpaign -> campaign

@@ +366,5 @@
> +   * Handles capmpaign timeout
> +   */
> +
> +  _onCampaignTimeout: function DirectoryLinksProvider_onCampaignTimeout() {
> +    this.campaignTimeoutID = null;

Why not call _clearCampaignTimeout() here?

@@ +735,5 @@
>          if (this._frequencyCaps.get(url) <= 0) {
>            return;
>          }
>  
> +        // as we iterate suggestedLinks, check for capmaign start/end

nit: capmaign -> campaign

@@ +737,5 @@
>          }
>  
> +        // as we iterate suggestedLinks, check for capmaign start/end
> +        // time limits, and set nextTimeout to the closest timestamp
> +        if (suggestedLink.startTime && suggestedLink.startTime > currentTime) {

Can we factor out these time checks into a separate function, just for readability?
Attachment #8600165 - Flags: feedback?(msamuel) → feedback+
Blocks: 1145428
Attached patch v2. campaign time limits (obsolete) — Splinter Review
all comments are fixed, but the one below - made a code comment to why it's better to just set _campaignTimeoutID to null

    // _campaignTimeoutID is invalid here, so just set it to null
    this._campaignTimeoutID = null;
Attachment #8600165 - Attachment is obsolete: true
Attachment #8600431 - Flags: review?(msamuel)
Attachment #8600431 - Flags: review?(msamuel) → feedback+
Comment on attachment 8600431 [details] [diff] [review]
v2.  campaign time limits

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

Requesting adw's final approval
Attachment #8600431 - Flags: review?(adw)
Attachment #8600431 - Flags: feedback?
Comment on attachment 8600431 [details] [diff] [review]
v2.  campaign time limits

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

::: browser/modules/DirectoryLinksProvider.jsm
@@ +346,5 @@
>    /**
> +   * Translates link.time_limits to UTC miliseconds and sets
> +   * link.startTime and link.endTime properties in link object
> +   */
> +

Please remove this blank line.

@@ +348,5 @@
> +   * link.startTime and link.endTime properties in link object
> +   */
> +
> +  _setupStartEndTime: function DirectoryLinksProvider_setupStartEndTime(link) {
> +    // set start/end limits if they exist. If dates are unparsable

In the comment, please mention what format time_limits.{start,end} are expected to be in.

@@ +351,5 @@
> +  _setupStartEndTime: function DirectoryLinksProvider_setupStartEndTime(link) {
> +    // set start/end limits if they exist. If dates are unparsable
> +    // Date.parse() returns NaN, which would fail existence check
> +    // when we try to use it - badly formatted date acts as empty date
> +    if (!link.time_limits) { return; }

Please write like:

if (!link.time_limits) {
  return;
}

@@ +357,5 @@
> +    if (link.time_limits.start) {
> +      link.startTime = Date.parse(link.time_limits.start);
> +    }
> +    if (link.time_limits.end) {
> +      link.endTime = Date.parse(link.time_limits.end);

You should probably handle malformed dates or unexpected formats.  Like:

link.startTime = Date.parse(...);
if (isNaN(link.startTime)) {
  delete link.startTime;
}
link.endTime = Date.parse(...);
if (isNaN(link.endTime)) {
  delete link.endTime;
}

@@ +364,5 @@
> +
> +  /*
> +   * Handles campaign timeout
> +   */
> +

Please remove this blank line.

@@ +386,5 @@
> +   * Setup capmpaign timeout to recompute suggested tiles upon
> +   * reaching soonest start or end time for the campaign
> +   * @param timeout in milliseconds
> +   */
> +

Please remove this blank line.

@@ +395,5 @@
> +    }
> +    this._clearCampaignTimeout();
> +    // setup next timeout
> +    this._campaignTimeoutID = setTimeout(this._onCampaignTimeout.bind(this), timeout);
> +    return;

No return needed.

@@ +401,5 @@
> +
> +  /**
> +   * Test link for campaign time limits
> +   * @param link
> +   * @return object {use: true/false, timeout: milliseconds/null}

Please call `timeout` `timeoutDate` instead.  And please expand the comment describing what the method does and also what the returned object means.

@@ +403,5 @@
> +   * Test link for campaign time limits
> +   * @param link
> +   * @return object {use: true/false, timeout: milliseconds/null}
> +   */
> +

Please remove this blank line.

@@ +769,5 @@
> +        if (timeout && (!nextTimeout || nextTimeout > timeout)) {
> +          nextTimeout = timeout;
> +        }
> +        // Skip link if it falls outside campaign time limits
> +        if (!use) { return; }

if (!use) {
  return;
}
Attachment #8600431 - Flags: review?(adw) → review+
We're also documenting the data formats with in-tree docs:

http://mxr.mozilla.org/mozilla-central/source/browser/docs/DirectoryLinksProvider.rst#176

So we'll need to update those. We should get confirmation from oyiptong/tspurway about the tiles data format. E.g., can it be nested as you have or should it be flat.
Attached patch v3. campaign time limits (obsolete) — Splinter Review
final patch with all comments resolved, rebased on origin/master and  browser/modules/test/xpcshell/test_DirectoryLinksProvider.js passing
Attachment #8600431 - Attachment is obsolete: true
Attachment #8600431 - Flags: feedback?
Depends on: 1161197
Attached patch v4. updated documentation (obsolete) — Splinter Review
updated documentation at browser/docs/DirectoryLinksProvider.rst
Attachment #8600731 - Attachment is obsolete: true
Attached patch v5. minor documentation tweak (obsolete) — Splinter Review
added time_limits example to suggested tile example
Attachment #8601747 - Attachment is obsolete: true
Attachment #8602240 - Attachment is patch: true
Attachment #8602240 - Attachment mime type: text/x-patch → text/plain
refreshed hg patch
Attachment #8602240 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/cd19fda2d5f8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
No longer blocks: 1160372
ni? per https://wiki.mozilla.org/Firefox/Data_Collection

(In reply to Carsten Book [:Tomcat] from comment #15)
> https://hg.mozilla.org/mozilla-central/rev/cd19fda2d5f8
There's no additional data being collected, but the server provides an extra object containing start/end times.
Flags: needinfo?(benjamin)
Points: --- → 8
Comment on attachment 8602265 [details] [diff] [review]
v6.  HG changeset patch with commit message

Since these are patches, let's use feedback? for approval.
Flags: needinfo?(benjamin)
Attachment #8602265 - Flags: feedback+
Verified with osx nightly 2015-05-26

1) new profile
2) about:config pref browser.newtabpage.directory.source = https://people.mozilla.org/~elee/suggested.1156549.json (for later verification, need to download the file and manually edit the start/end dates from the current:
"time_limits": {
  "end": "2015-05-26T17:35",
  "start": "2015-05-27T00:30:00.000Z"

This is testing both absolute UTC time as well as relative time (start 12:30am utc, end 5:35pm local [pacific for me])

3) visit enough sites (at least 8) to allow for suggestions
4) see that the clock is before 5:30pm and no suggestion is made
5) wait until 5:30pm and open new tab seeing suggestion [attached]
6) wait until 5:35pm and open new tab seeing no suggestion
Status: RESOLVED → VERIFIED
Attached image verify screenshot
Verified fixed across platforms using latest Aurora, build ID: 20150528004000.
QA Contact: cornel.ionce
You need to log in before you can comment on or make changes to this bug.