Closed
Bug 995436
Opened 11 years ago
Closed 10 years ago
Use different sponsored panel text for Release and non-Release
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
VERIFIED
FIXED
Firefox 31
People
(Reporter: Mardak, Assigned: mzhilyaev)
References
()
Details
(Whiteboard: p=3 s=it-31c-30a-29b.3 [qa-])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
adw
:
review+
|
Details | Diff | Splinter Review |
Seems like we'll want the non-Release sponsored panel text to say something about testing while the Release text will mention sponsoring/supporting Mozilla.
clarkbw will provide the desired text.
To implement this, I believe the pattern is to insert strings into the appropriate branding properties file, so we'll need to initialize the panel with a string instead of relying on the dtd/entity:
http://mxr.mozilla.org/mozilla-central/source/browser/branding/
http://mxr.mozilla.org/mozilla-central/source/browser/branding/unofficial/locales/en-US/brand.properties
http://mxr.mozilla.org/mozilla-central/source/browser/components/migration/content/migration.js#251
Flags: firefox-backlog+
Updated•11 years ago
|
Assignee: nobody → clarkbw
Status: NEW → ASSIGNED
Whiteboard: p=3 → p=3 s=it-31c-30a-29b.3 [qa?]
Probably something we should test but it's not something we can do in this iteration. Until we have a "release" build to test (ie. Beta/RC) we won't be able to confirm this works as designed. We can revisit testing this at that time if desired. Tagging as [qa-] for this iteration though.
Please let me know if something more is needed from QA here.
Whiteboard: p=3 s=it-31c-30a-29b.3 [qa?] → p=3 s=it-31c-30a-29b.3 [qa-]
Comment 2•10 years ago
|
||
Seems like this should have been split into "provide the text" and "implement the text differences" bug, since I don't think Bryan will be doing both.
Comment 3•10 years ago
|
||
Our initial experiment does not have actual sponsors, yet we want to test out the experience of having sponsors so we've enlisted a couple "Trial Sponsors". Trial Sponsors may be interested in having Sponsored Tiles if we go down that path but a revenue agreement doesn't currently exist.
Trial Sponsor Text:
"This Trial Sponsor site was suggested because we hoped you’d find it interesting and because it supports Mozilla’s mission. Learn more.."
A Sponsored version would drop the “Trial” word:
"This Sponsor site was suggested because we hoped you’d find it interesting and because it supports Mozilla’s mission. Learn more.."
Comment 4•10 years ago
|
||
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #2)
> Seems like this should have been split into "provide the text" and
> "implement the text differences" bug, since I don't think Bryan will be
> doing both.
Indeed, string finalized. I believe maksik is taking over now.
Assignee: clarkbw → nobody
Updated•10 years ago
|
Status: ASSIGNED → NEW
Whiteboard: p=3 s=it-31c-30a-29b.3 [qa-] → p=3 [qa-]
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → mzhilyaev
Reporter | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Whiteboard: p=3 [qa-] → p=3 s=it-31c-30a-29b.3 [qa-]
Reporter | ||
Comment 5•10 years ago
|
||
Not exactly sure what the format should be for brand.properties in that should the key be "newtab." prefixed? All the other ones don't have "."s.
Attachment #8412322 -
Flags: review?(adw)
Reporter | ||
Comment 6•10 years ago
|
||
Actually, looking at this patch, I see there's nightly, aurora, unofficial, official.
clarkbw, should Beta have the Release text or not-Release text?
Flags: needinfo?(clarkbw)
Comment 7•10 years ago
|
||
Comment on attachment 8412322 [details] [diff] [review]
v1
Review of attachment 8412322 [details] [diff] [review]:
-----------------------------------------------------------------
It's not clear to me reading this bug that official branding maps to non-trial sponsors and all other branding maps to trial sponsors. Bryan's comments here so far don't really make those connections. But I might be missing context.
r+ assuming those are the right mappings.
(In reply to Ed Lee :Mardak from comment #5)
> Not exactly sure what the format should be for brand.properties in that
> should the key be "newtab." prefixed? All the other ones don't have "."s.
I'm not sure, either. Maybe it's better not to use a newtab prefix in case we show these some other place in the future. I don't think it's a big deal either way.
sponsoredPanelBrandMessage is a little vague. You can imagine other sponsored things in the app. I'd suggest sponsoredTileMessage or sponsoredSiteMessage. Only suggestions, up to you.
Attachment #8412322 -
Flags: review?(adw) → review+
Reporter | ||
Updated•10 years ago
|
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(clarkbw)
Reporter | ||
Comment 8•10 years ago
|
||
maksik updated the patch to only show the non-Trial message for release-type channels, so that means Beta will show Trial.
Attachment #8413060 -
Flags: review?(adw)
Comment 9•10 years ago
|
||
Comment on attachment 8413060 [details] [diff] [review]
v2
Review of attachment 8413060 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/base/content/newtab/page.js
@@ +24,5 @@
> button.addEventListener("click", this, false);
>
> // Initialize sponsored panel
> this._sponsoredPanel = document.getElementById("sponsored-panel");
> + if (UpdateChannel.get().startsWith("release")) {
For posterity, I had a question about this part, but Gavin says:
> adw: the patch there does UpdateChannel.get().startsWith("release") to see if it's a "release-type" channel
> gavin: that shouldn't be based on the update channel
> gavin: #ifdef RELEASE_BUILD is probably better
> gavin: (that is also defined in beta builds)
> adw: Mardak says clarkbw said we want the "trial" language for betas too
> gavin: well I think I disagree with them
> ...
> gavin: we can sort it out later, the patch is OK temporarily
> gavin: (well, that part of the patch)
Attachment #8413060 -
Flags: review?(adw) → review+
Reporter | ||
Updated•10 years ago
|
Attachment #8412322 -
Attachment is obsolete: true
Reporter | ||
Comment 10•10 years ago
|
||
Comment 11•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Updated•10 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•