Closed
Bug 1139496
Opened 9 years ago
Closed 9 years ago
Allow server provided explanation / ad group name to be displayed on Suggested Tiles
Categories
(Firefox :: New Tab Page, defect)
Firefox
New Tab Page
Tracking
()
People
(Reporter: kghim, Assigned: emtwo)
References
(Blocks 1 open bug)
Details
(Whiteboard: .004)
Attachments
(4 files, 2 obsolete files)
14.46 KB,
patch
|
benjamin
:
feedback+
|
Details | Diff | Splinter Review |
5.00 KB,
patch
|
adw
:
review+
|
Details | Diff | Splinter Review |
18.58 KB,
patch
|
Details | Diff | Splinter Review | |
18.73 KB,
patch
|
Details | Diff | Splinter Review |
Allow ad group name to be displayed on Suggested Tiles' explanation text if needed. For example, if the ad group name is "Comic Books," the explanation text would be something like "Suggested because you like Comic Books" Best practices: - Use standard Moz Categories if possible. Listed here: https://docs.google.com/a/mozilla.com/spreadsheets/d/1wlPZrU07y035hXDKcHp0KtmszTdRf7jWuz3jo1kpqyk/edit#gid=0 - If not, use short and localizable categories - Try avoiding custom categories with indistinguishable interests
Updated•9 years ago
|
Comment 1•9 years ago
|
||
Instead of just an adgroup name that gets inserted, we could have the server provide the whole localized string. This would allow us to have different types of messaging that still reuse the underlying site matching mechanism: [firefox for android tile] Because you downloaded Firefox
Summary: Allow ad group name to be displayed on Suggested Tiles → Allow server provided explanation / ad group name to be displayed on Suggested Tiles
Assignee | ||
Updated•9 years ago
|
Points: --- → 3
Reporter | ||
Updated•9 years ago
|
Whiteboard: .? → .001
Comment 2•9 years ago
|
||
Whiteboard: .? → .001 .001 makes this bug pretty much the highest priority item. I don't think this work should be done before we finish what we need to do for 38.
Reporter | ||
Updated•9 years ago
|
Whiteboard: .001 → .004
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → msamuel
Flags: needinfo?(oyiptong)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(oyiptong)
Updated•9 years ago
|
Component: Tiles: Administration Front-End → Tiles
Updated•9 years ago
|
Iteration: --- → 40.3 - 11 May
Component: Tiles → New Tab Page
Product: Content Services → Firefox
Updated•9 years ago
|
Iteration: 40.3 - 11 May → 41.1 - May 25
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8605535 -
Flags: review?(adw)
Assignee | ||
Comment 6•9 years ago
|
||
Taking out some code I was testing with.
Attachment #8605535 -
Attachment is obsolete: true
Attachment #8605535 -
Flags: review?(adw)
Attachment #8605536 -
Flags: review?(adw)
Assignee | ||
Updated•9 years ago
|
Attachment #8605536 -
Attachment is patch: true
Comment 7•9 years ago
|
||
Comment on attachment 8605536 [details] [diff] [review] v2: Allow server provided explanation / ad group name to be displayed on Suggested Tile Review of attachment 8605536 [details] [diff] [review]: ----------------------------------------------------------------- Cool. ::: browser/base/content/newtab/sites.js @@ +118,5 @@ > > + _newTabString: function(str, substrArr) { > + let regExp = /%[0-9]\$S/g; > + let matches; > + while ((matches = regExp.exec(str)) !== null) { Nit: while ((matches = regExp.exec(str))) {
Attachment #8605536 -
Flags: review?(adw) → review+
Comment 8•9 years ago
|
||
Comment on attachment 8605536 [details] [diff] [review] v2: Allow server provided explanation / ad group name to be displayed on Suggested Tile emtwo, I forgot to mention we need to update the .rst explaining the data from the server and giving an example: explanation and adgroup_name See bug 1156549 comment 16 for an example comment. And request f? :bsmedberg See https://hg.mozilla.org/mozilla-central/rev/cd19fda2d5f8 for updated rst for time_limits
Attachment #8605536 -
Flags: feedback-
Comment 9•9 years ago
|
||
f? per https://wiki.mozilla.org/Firefox/Data_Collection There's no additional data being collected, but the server provides two extra strings relating to the explanation below the suggested tile.
Attachment #8606109 -
Flags: feedback?(benjamin)
Updated•9 years ago
|
Attachment #8605536 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8606109 -
Attachment description: v2.1 → Part 1: v2.1: Allow server provided explanation / ad group name to be displayed on Suggested Tile
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8606754 -
Flags: review?(adw)
Comment 11•9 years ago
|
||
Comment on attachment 8606754 [details] [diff] [review] Part 2: v1: Sanitize explanation & adgroup name sent by server Review of attachment 8606754 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/modules/DirectoryLinksProvider.jsm @@ +617,5 @@ > if (name == undefined) { > return; > } > + let sanitizeFlags = > + ParserUtils.SanitizerCidEmbedsOnly|ParserUtils.SanitizerDropForms|ParserUtils.SanitizerDropNonCSSPresentation; Style nit: please put each of these flags on its own line.
Attachment #8606754 -
Flags: review?(adw) → review+
Updated•9 years ago
|
Attachment #8606109 -
Flags: feedback?(benjamin) → feedback+
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bafcb7152768
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Comment 14•9 years ago
|
||
Comment 15•9 years ago
|
||
Comment 16•9 years ago
|
||
Verified on Nightly - 41.0a1 (2015-05-26). verification steps: 1. created fresh profile 2. browsed to achieve 8 history sites showing up in newtab 3. one of history sites is cmm.com (which is in the "News" list of targeted sites) 4. observed "News App" suggested tile from Firefox Marketplace targeted to News sites 5. Text snippet under the tile says "Suggested for News visitors"
Updated•9 years ago
|
Status: RESOLVED → VERIFIED
Comment 17•9 years ago
|
||
Verified on Nightly - 41.0a1 (2015-05-26). Note explanation text in json (full version: https://people.mozilla.org/~mzhilyaev/suggested1.json) { "adgroup_name": "Mozilla enthusiast", .... "explanation": "A %1$S suggestion for those who visit %2$S (I'm so wordy!)", .... } And the corresponding display of explanation text in the attached screen shot: https://people.mozilla.org/~mzhilyaev/1138817.sh2.png The explanation string shows properly replaced '%1$S' with adgroup_name and '%2$S' with targeted site.
Comment 18•9 years ago
|
||
The verification step for comment 17 requires going to about:config and setting browser.newtabpage.directory.source to https://people.mozilla.org/~mzhilyaev/suggested1.json
Comment 19•9 years ago
|
||
Comment on attachment 8610021 [details] [diff] [review] for aurora (Mardak will land) Approval Request Comment: See bug 1140185 comment 11
Attachment #8610021 -
Flags: approval-mozilla-aurora?
Comment 21•9 years ago
|
||
Comment on attachment 8610021 [details] [diff] [review] for aurora (Mardak will land) approval-mozilla-aurora+ granted in bug 1140185 comment 14
Attachment #8610021 -
Flags: approval-mozilla-aurora?
Comment 22•9 years ago
|
||
Confirming the fix on latest Aurora, build ID: 20150528004000. Testing was performed on Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit.
QA Contact: cornel.ionce
You need to log in
before you can comment on or make changes to this bug.
Description
•