Closed
Bug 1156549
Opened 9 years ago
Closed 9 years ago
Allow ramp up time for campaigns with strict start/stop times
Categories
(Firefox :: New Tab Page, defect)
Firefox
New Tab Page
Tracking
()
People
(Reporter: kghim, Assigned: mzhilyaev)
References
Details
(Whiteboard: .002)
Attachments
(3 files, 5 obsolete files)
14.19 KB,
patch
|
benjamin
:
feedback+
|
Details | Diff | Splinter Review |
17.11 KB,
patch
|
Details | Diff | Splinter Review | |
254.80 KB,
image/png
|
Details |
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
Reporter | ||
Updated•9 years ago
|
Whiteboard: .? → .002
Comment 1•9 years ago
|
||
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
Updated•9 years ago
|
Iteration: --- → 40.3 - 11 May
Updated•9 years ago
|
Component: Tiles → New Tab Page
Product: Content Services → Firefox
Comment 2•9 years ago
|
||
Reattaching from bug 1156542
Attachment #8600165 -
Flags: feedback?(msamuel)
Updated•9 years ago
|
Attachment #8600165 -
Attachment is patch: true
Updated•9 years ago
|
Summary: Allow ramp up time for campaigns → Allow ramp up time for campaigns with strict start/stop times
Assignee | ||
Comment 4•9 years ago
|
||
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 5•9 years ago
|
||
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+
Assignee | ||
Comment 6•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8600431 -
Flags: review?(msamuel) → feedback+
Assignee | ||
Comment 7•9 years ago
|
||
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 8•9 years ago
|
||
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+
Comment 9•9 years ago
|
||
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.
Assignee | ||
Comment 10•9 years ago
|
||
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?
Assignee | ||
Comment 11•9 years ago
|
||
updated documentation at browser/docs/DirectoryLinksProvider.rst
Attachment #8600731 -
Attachment is obsolete: true
Assignee | ||
Comment 12•9 years ago
|
||
added time_limits example to suggested tile example
Attachment #8601747 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8602240 -
Attachment is patch: true
Attachment #8602240 -
Attachment mime type: text/x-patch → text/plain
Assignee | ||
Comment 13•9 years ago
|
||
refreshed hg patch
Attachment #8602240 -
Attachment is obsolete: true
Comment 15•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cd19fda2d5f8
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Comment 16•9 years ago
|
||
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)
Updated•9 years ago
|
Points: --- → 8
Comment 17•9 years ago
|
||
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+
Comment 18•9 years ago
|
||
Comment 19•9 years ago
|
||
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
status-firefox41:
--- → verified
Comment 20•9 years ago
|
||
Comment 21•9 years ago
|
||
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.
Description
•