Use different sponsored panel text for Release and non-Release

VERIFIED FIXED in Firefox 31

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Mardak, Assigned: mzhilyaev)

Tracking

Trunk
Firefox 31
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: p=3 s=it-31c-30a-29b.3 [qa-], )

Attachments

(1 attachment, 1 obsolete attachment)

v2
4.29 KB, patch
adw
: review+
Details | Diff | Splinter Review
Reporter

Description

5 years ago
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+
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-]
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.
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.."
(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
Status: ASSIGNED → NEW
Whiteboard: p=3 s=it-31c-30a-29b.3 [qa-] → p=3 [qa-]
Reporter

Updated

5 years ago
Assignee: nobody → mzhilyaev
Reporter

Updated

5 years ago
Status: NEW → ASSIGNED
Whiteboard: p=3 [qa-] → p=3 s=it-31c-30a-29b.3 [qa-]
Reporter

Comment 5

5 years ago
Posted patch v1 (obsolete) — Splinter Review
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

5 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 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

5 years ago
Flags: needinfo?(clarkbw)
Reporter

Comment 8

5 years ago
Posted patch v2Splinter Review
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 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

5 years ago
Attachment #8412322 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/2ddc109368cc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.