Closed Bug 1077643 Opened 5 years ago Closed 5 years ago

Enable WhatsNew page in-product for 33.X anniversary release

Categories

(Firefox :: General, defect)

defect
Not set
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 33
Iteration:
36.2
Tracking Status
firefox33 --- verified

People

(Reporter: Dolske, Assigned: markh)

References

Details

Attachments

(2 files, 3 obsolete files)

We want to enable the whatsnew page in Firefox 33.0.1 for the anniversary stuff. Instead of controlling this via snippets, we can do it in-product and restrict it to only the locales which have the new Forget button available (see also bug 1073607).
Flags: qe-verify+
Flags: firefox-backlog+
I'm not sure I understand what needs to happen here. Can the update service not control this per locale?

If not, what exactly needs to happen in-product? Do we want to just #ifdef the tour URL to be different on the locales that have l10n ready, and keep it as-is for the ones that don't, or something else? :-)
Flags: needinfo?(nthomas)
Flags: needinfo?(gavin.sharp)
(In reply to :Gijs Kruitbosch from comment #1)
> I'm not sure I understand what needs to happen here. Can the update service
> not control this per locale?
> 
> If not, what exactly needs to happen in-product? Do we want to just #ifdef
> the tour URL to be different on the locales that have l10n ready, and keep
> it as-is for the ones that don't, or something else? :-)

Speaking of which: which exact pref, and does/should that include the Help > Firefox Tour item? Do we have a url for the new page?
(In reply to :Gijs Kruitbosch from comment #1)
> I'm not sure I understand what needs to happen here. Can the update service
> not control this per locale?

It can, and bhearsum said "not that hard" when I asked him, but the problem with that is that it only affects updates (and not "pave over"/new installs).

> If not, what exactly needs to happen in-product? Do we want to just #ifdef
> the tour URL to be different on the locales that have l10n ready, and keep
> it as-is for the ones that don't, or something else? :-)

We only want the whatsnew page to appear for locales in privacy.panicButton.enabledLocales. This means adjusting logic in nsBrowserContentHandler.js's defaultArgs getter, at the very least (probably better to just hardcode the URL in the code rather than setting the pref and having it not take effect in certain locales, but I haven't thought that through fully).

I don't know what we've decided about the "firstrun" page, I will followup on that.
Flags: needinfo?(nthomas)
Flags: needinfo?(gavin.sharp)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #3)
> (In reply to :Gijs Kruitbosch from comment #1)
> > I'm not sure I understand what needs to happen here. Can the update service
> > not control this per locale?
> 
> It can, and bhearsum said "not that hard" when I asked him, but the problem
> with that is that it only affects updates (and not "pave over"/new installs).
> 
> > If not, what exactly needs to happen in-product? Do we want to just #ifdef
> > the tour URL to be different on the locales that have l10n ready, and keep
> > it as-is for the ones that don't, or something else? :-)
> 
> We only want the whatsnew page to appear for locales in
> privacy.panicButton.enabledLocales. This means adjusting logic in
> nsBrowserContentHandler.js's defaultArgs getter, at the very least (probably
> better to just hardcode the URL in the code rather than setting the pref and
> having it not take effect in certain locales, but I haven't thought that
> through fully).
> 
> I don't know what we've decided about the "firstrun" page, I will followup
> on that.

We are not modifying the current /firstrun tour content for this campaign. The existing /firstrun tour will be shown to users who download Firefox for the first time.
Points: --- → 2
There's some 33.1 vs 33.0.1 chatter in these comments: https://bugzilla.mozilla.org/show_bug.cgi?id=1077517#c2
Erin: can you confirm the version question in comment 5 on this bug and the bug I mentioned?
Flags: needinfo?(elancaster)
The actual version number really isn't relevant to this bug.
Summary: Enable WhatsNew page in-product for 33.0.1 release → Enable WhatsNew page in-product for 33.X anniversary release
Flags: needinfo?(elancaster)
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
Iteration: --- → 36.1
Is this the sort of thing we are looking for?  Untested, but against beta.

Assuming it's not too far off:

(In reply to :Gijs Kruitbosch from comment #2)
> Speaking of which: which exact pref, and does/should that include the Help >
> Firefox Tour item? Do we have a url for the new page?

I'm not sure what the "Tour Item" reference is and the URL in the patch is marked as being a guess :)
Attachment #8505971 - Flags: feedback?(gijskruitbosch+bugs)
Attachment #8505971 - Flags: feedback?(gavin.sharp)
Comment on attachment 8505971 [details] [diff] [review]
0001-Bug-1077643-Enable-WhatsNew-page-in-product-for-33.X.patch

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

::: browser/components/nsBrowserContentHandler.js
@@ +619,5 @@
> +              break;
> +            }
> +            Services.prefs.setBoolPref(prefHaveShown, true);
> +            // XXX - correct URL?
> +            overridePage = "https://www.mozilla.org/en-US/firefox/33.1/whatsnew/";

Nit: for analytics and consistency it would be good to include the ?oldversion=%OLD_VERSION% parameter
Attachment #8505971 - Flags: feedback+
Comment on attachment 8505971 [details] [diff] [review]
0001-Bug-1077643-Enable-WhatsNew-page-in-product-for-33.X.patch

Close, but a few differences between this and the other bug:
- this should go in the existing OVERRIDE_NEW_MSTONE branch, not a new OVERRIDE_NEW_BUILD_ID branch. It should behave exactly as it would if startup.homepage_override_url was set, except we want this to be limited to certain locales
- given that, the "haveShown" pref stuff is also unnecessary

Looks good with those addressed.
Attachment #8505971 - Flags: feedback?(gavin.sharp) → feedback+
Comment on attachment 8505971 [details] [diff] [review]
0001-Bug-1077643-Enable-WhatsNew-page-in-product-for-33.X.patch

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

What gavin said. But also, when I tried that on beta it didn't work for me. I didn't manage to figure out why before I saw that the bug was assigned to you at which point I stopped investigating and /dev/null'd my patch. :-(
Attachment #8505971 - Flags: feedback?(gijskruitbosch+bugs)
This version incorporates the suggestions from Gavin, and automagically includes the oldversion= part of the URL suggested by Matt.  Note that the URL https://www.mozilla.org/en-US/firefox/33.1/whatsnew/ does actually exist, but with somewhat less content than I imagine the "real" version will have, but I've left the "// XXX - correct URL?" comment there until it is explicitly confirmed as being correct.

Note this patch still allows an updater to override this page, which is probably safe and reasonable, but maybe we want to remove this too?

This is against the current release branch.  To test, it is necessary to add new privacy.panicButton.* prefs - I assume they are going to be merged into release some time later?  Also, as expected, browser.startup.homepage_override.mstone will need to be changed to something other than "33.0" to see this in action.
Attachment #8505971 - Attachment is obsolete: true
Attachment #8506563 - Flags: feedback?(gavin.sharp)
Attachment #8506563 - Flags: feedback?(MattN+bmo)
Comment on attachment 8506563 [details] [diff] [review]
0001-Bug-1077643-Enable-WhatsNew-page-in-product-for-33.X.patch

Yep, this looks good (though the indentation looks to be off?).

We'll confirm the final URI shortly prior to landing this.

We can leave the update override in place, we'll just need to coordinate with nthomas to make sure the snippets are configured correctly.
Attachment #8506563 - Flags: feedback?(gavin.sharp) → feedback+
Comment on attachment 8506563 [details] [diff] [review]
0001-Bug-1077643-Enable-WhatsNew-page-in-product-for-33.X.patch

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

::: browser/components/nsBrowserContentHandler.js
@@ +600,5 @@
> +              let featureEnabled;
> +              let enabledLocales = [];
> +              try {
> +                featureEnabled = Services.prefs.getBoolPref("privacy.panicButton.enabled");
> +                enabledLocales = Services.prefs.getCharPref("privacy.panicButton.enabledLocales").split(' ');

Nit: use double quotes

@@ +604,5 @@
> +                enabledLocales = Services.prefs.getCharPref("privacy.panicButton.enabledLocales").split(' ');
> +              } catch (ex) {}
> +              if (featureEnabled && enabledLocales.indexOf(locale) != -1) {
> +                // XXX - correct URL?
> +                overridePage = "https://www.mozilla.org/en-US/firefox/33.1/whatsnew/?oldversion=%OLD_VERSION%";

I'm pretty sure we don't want to hard-code "en-US" here. I think we can probably use the same URL from startup.homepage_override_url and the server will look for 33.1 in the path and serve different content.
Attachment #8506563 - Flags: feedback?(MattN+bmo) → feedback+
(In reply to Matthew N. [:MattN] (behind on reviews - focused on Loop) from comment #14)
> Nit: use double quotes

Done (and I fixed the indentation)

> I think we can
> probably use the same URL from startup.homepage_override_url and the server
> will look for 33.1 in the path and serve different content.

The only occurrence of a "real" value for this override URL is for Nightly, and is defined as:

https://www.mozilla.org/projects/firefox/%VERSION%/whatsnew/?oldversion=%OLD_VERSION%

Note how this URL is quite different than I expect (eg, there's no "locale" part at all, which presumably is just a reflection of that it's nightly.  So what I've done here is:

                overridePage = Services.urlFormatter.formatURL(
                    "https://www.mozilla.org/%LOCALE%/firefox/%VERSION%/whatsnew/"
                ) + "?oldversion=%OLD_VERSION%";

On release, this currently results in https://www.mozilla.org/en-US/firefox/33.0/whatsnew/?oldversion=34.0

Obviously the oldversion part just reflects my test profile, and the "33.0" part will change to whatever version the new build has (ie, 33.1 IIUC).

Jen and Chris: Gavin asked me to needinfo you to verify this URL is correct.  Once this is verified or corrected I'll remove the '// XXX - correct URL?' comment and request review from Gavin (pending f+ from Matt obviously)
Attachment #8506563 - Attachment is obsolete: true
Flags: needinfo?(jbertsch)
Flags: needinfo?(chrismore.bugzilla)
Attachment #8510001 - Flags: feedback?(MattN+bmo)
Comment on attachment 8510001 [details] [diff] [review]
0001-Bug-1077643-Enable-WhatsNew-page-in-product-for-33.X.patch

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

::: browser/components/nsBrowserContentHandler.js
@@ +609,5 @@
> +              // not replace it with anything, it will complain about the
> +              // unknown value.
> +              overridePage = Services.urlFormatter.formatURL(
> +                  "https://www.mozilla.org/%LOCALE%/firefox/%VERSION%/whatsnew/"
> +              ) + "?oldversion=%OLD_VERSION%";

Nit: indentation
Attachment #8510001 - Flags: feedback?(MattN+bmo) → feedback+
This is correct:

 overridePage = Services.urlFormatter.formatURL(
                    "https://www.mozilla.org/%LOCALE%/firefox/%VERSION%/whatsnew/"
                ) + "?oldversion=%OLD_VERSION%";

The /projects/firefox/ url variation in comment 15 is incorrect even though it will redirect to the correct URL that excludes the /projects legacy sub-directory.

Nightly has had the hard-coded old URL for a while:

http://dxr.mozilla.org/mozilla-central/source/browser/branding/nightly/pref/firefox-branding.js
Flags: needinfo?(chrismore.bugzilla)
(In reply to Matthew N. [:MattN] (behind on reviews - focused on Loop) from comment #16)
> > +              overridePage = Services.urlFormatter.formatURL(
> > +                  "https://www.mozilla.org/%LOCALE%/firefox/%VERSION%/whatsnew/"
> > +              ) + "?oldversion=%OLD_VERSION%";
> 
> Nit: indentation

I hope/assume you mean the line starting with "https://" used 4 spaces instead of 2, 'cos that's that I fixed ;)

(In reply to Chris More [:cmore] from comment #17)
> This is correct:
> 
>  overridePage = Services.urlFormatter.formatURL(
>                    
> "https://www.mozilla.org/%LOCALE%/firefox/%VERSION%/whatsnew/"
>                 ) + "?oldversion=%OLD_VERSION%";

Great, thanks - the URL in this patch is the same as the previous one.

So I think this is ready.  Given this is for release, I'm requesting review from Gavin.
Comment on attachment 8510698 [details] [diff] [review]
0001-Bug-1077643-Enable-WhatsNew-page-in-product-for-33.X.patch

oops - double-press of <enter> :)
Flags: needinfo?(jbertsch)
Attachment #8510698 - Flags: review?(gavin.sharp)
Attachment #8510001 - Attachment is obsolete: true
Comment on attachment 8510698 [details] [diff] [review]
0001-Bug-1077643-Enable-WhatsNew-page-in-product-for-33.X.patch

Thanks. We'll need good QA testing on this for 33.1.

(you can remove the XXX comment)
Attachment #8510698 - Flags: review?(gavin.sharp) → review+
Removed the 'XXX' comment and carry r=gavin forward.

Dolske, are you happy for me to just land this on alder?
Flags: needinfo?(dolske)
Attachment #8510750 - Flags: review+
Yeah, please go ahead and land on alder.
Flags: needinfo?(dolske)
https://hg.mozilla.org/projects/alder/rev/1d7405a097b1

Blech, some garbage got included with the commit message, but it didn't show in my |hg out|. Oh well.
Whiteboard: [fixed-alder]
Hmm, looks like this causes some test failures, but I'm not sure we care? Maybe just disable the test if we don't want distracting orange on m-r?

53725 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/test/browser_bug538331.js | correct value returned by defaultArgs - Got https://www.mozilla.org/en-US/firefox/33.0.2/whatsnew/?oldversion=PreviousMilestone|about:home, expected http://pref.example.com/|about:home

53732 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/test/browser_bug538331.js | correct value returned by defaultArgs - Got https://www.mozilla.org/en-US/firefox/33.0.2/whatsnew/?oldversion=PreviousMilestone|about:home, expected http://pref.example.com/|about:home

53737 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/test/browser_bug538331.js | correct value returned by defaultArgs - Got https://www.mozilla.org/en-US/firefox/33.0.2/whatsnew/?oldversion=PreviousMilestone|about:home, expected http://pref.example.com/|about:home

53745 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/test/browser_bug538331.js | correct value returned by defaultArgs - Got https://www.mozilla.org/en-US/firefox/33.0.2/whatsnew/?oldversion=PreviousMilestone|about:home, expected http://pref.example.com/|about:home

53753 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/test/browser_bug538331.js | correct value returned by defaultArgs - Got https://www.mozilla.org/en-US/firefox/33.0.2/whatsnew/?oldversion=PreviousMilestone|about:home, expected http://pref.example.com/|about:home

53761 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/test/browser_bug538331.js | correct value returned by defaultArgs - Got https://www.mozilla.org/en-US/firefox/33.0.2/whatsnew/?oldversion=PreviousMilestone|about:home, expected http://pref.example.com/|about:home
Iteration: 36.1 → 36.2
Disabled the test that's failing, per sheriff request.

https://hg.mozilla.org/projects/alder/rev/3e492d503e8d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
QA Contact: catalin.varga
I've tested this fix using:

FF 33.0.2
beta-test channel
OS: Win 7 x64

I've updated from FF 33.0.2 to FF 33.1 using beta-test channel(build id: 20141030112145)but after restarting the previous session is restored instead of opening https://www.mozilla.org/en-US/firefox/33.1/whatsnew/?oldversion=33.0.2 this fix is not working for me.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Hmm. WFM on OS X after creating a new profile in 33.0.2, then running 33.1 with the same profile.

I get a new tab with https://www.mozilla.org/en-US/firefox/33.1/whatsnew/?oldversion=33.0.2 in it.

Does this also need to be enabled in snippets for testing updates that way? (see comment 3)
(In reply to Justin Dolske [:Dolske] from comment #28)
> Does this also need to be enabled in snippets for testing updates that way?
> (see comment 3)

No, it shouldn't be. The in-product fix should cover all cases.

Catalin, can you elaborate on your STR in comment 27, so that I can try to replicate? How do I test updates on beta-test?
Flags: needinfo?(catalin.varga)
STR:

1. Install FF 33.0.2
2. On the Firefox folder modify c:\Program Files (x86)\Mozilla Firefox\defaults\pref\channel-prefs.js by changing the channel from "release" to "betatest"
3. Start Firefox with a new Profile.
4. Go to Help-> About Firefox to check for update.
5. After the Update is performed press the Restart button from About Firefox window.
Flags: needinfo?(catalin.varga)
Catalin, can you check the values of browser.startup.homepage_override.mstone in about:config before and after your update and list them here?
Flags: needinfo?(catalin.varga)
Also, the values of:
privacy.panicButton.enabledLocales
privacy.panicButton.enabled

in the "after" build (that doesn't get the whatsnew). And what is the locale of the build you are updating to?
Chris, can you check the snippets configuration for the scenario from comment 30, particularly re: their "showURL" or "silent" actions?
Flags: needinfo?(catlee)
FF 33.0.2
browser.startup.homepage_override.mstone = 33.0.2

FF 33.1
browser.startup.homepage_override.mstone = 33.1
privacy.panicButton.enabledLocales = ast da de en-GB en-US es-AR es-CL es-ES es-MX fi fr fy-NL he hu it ja ja-JP-mac ko lv nb-NO nn-NO pa-IN pl pt-BR rm ru sk sl zh-TW
privacy.panicButton.enabled = true

I've used en-Us build for FF 33.0.2
Flags: needinfo?(catalin.varga)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #33)
> Chris, can you check the snippets configuration for the scenario from
> comment 30, particularly re: their "showURL" or "silent" actions?

The snippet here is:
https://aus3.mozilla.org/update/1/Firefox/33.0.2/20141027150301/WINNT_x86-msvc/en-US/betatest/update.xml

It has actions="silent", which is suppressing the in-product whatsnew behavior. I'll file a release engineering bug.
Flags: needinfo?(catlee)
Should be fixed per bug 1093187.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Sorry for the confusion caused by incorrect snippets.
I verified the bug after 1093187 was fixed, using the following environment:

FF 33.0.2
FF 33.1 
OS: Mac OS X 10.7.5 , Win 7 x64, Ubuntu 12.04 x86

I've used several localizations ones that are supporting panic button and ones that are not supporting. After the upgrade for the localizations that are supporting panic button the required url is opened(https://www.mozilla.org/%Locale%/firefox/33.1/whatsnew/?oldversion=33.0.2 )and for the ones that are not supporting panic button the url is not opened. I've used both methods of update(through update channels and pave over) and it's working correctly for both of them.
From the QA point of view this fix is working properly now and the bug is Verified.
Status: RESOLVED → VERIFIED
I've noticed that what's new page url for FF 33.1 eg(https://www.mozilla.org/%Locale%/firefox/33.1/whatsnew/?oldversion=33.0.2)is still displaying the old what's new page. When do we expect the new page to be public?
Flags: needinfo?(gavin.sharp)
Jennifer would know the answer to that.
Flags: needinfo?(gavin.sharp) → needinfo?(jbertsch)
(In reply to Catalin Varga [QA][:VarCat] from comment #39)
> I've noticed that what's new page url for FF 33.1
> eg(https://www.mozilla.org/%Locale%/firefox/33.1/whatsnew/?oldversion=33.0.
> 2)is still displaying the old what's new page. When do we expect the new
> page to be public?

Hi Caitlin-

The new page will be public 6am Monday Nov 10 with the 33.1 build.

Thx,
Jen
Flags: needinfo?(jbertsch)
I'd like to confirm what logic is in place for the 33.1 /whatsnew tour and who will see it moving forward. 

Is this the current logic? 

- Users updating from 33.1 to 33.1.x+ will not see the /whatsnew tour since they have already seen it. The current plan is to leave it off until the 35 Hello campaign. 

- Any users updating from versions prior to 33.1 should see /whatsnew.
Flags: needinfo?(nthomas)
Flags: needinfo?(agibson)
Sort of. I had a Firefox 22 that I updated to 31.1.1 and it got a /whatsnew with the tour for the privacy features + DDG. After updating 33.1 I got a /whatsnew with the Declare your independence content, but no tour. The patches on this bug show /whatsnew whenever the version changes, but mozilla.org isn't adding the tour content.

A firstrun page for 31.1.1 has the tour and the 'Welcome to firefox'.
Flags: needinfo?(nthomas)
(In reply to Nick Thomas [:nthomas] from comment #43)
> Sort of. I had a Firefox 22 that I updated to 31.1.1 and it got a /whatsnew
> with the tour for the privacy features + DDG. After updating 33.1 I got a
> /whatsnew with the Declare your independence content, but no tour. The
> patches on this bug show /whatsnew whenever the version changes, but
> mozilla.org isn't adding the tour content.
> 
> A firstrun page for 31.1.1 has the tour and the 'Welcome to firefox'.

I'm not sure if this sounds quite right. Only versions => 33.1 can see the privacy tour according to our server-side logic.

For example, 31.1.1 gets this page when updating from 22.0

https://www.mozilla.org/en-US/firefox/31.1.1/whatsnew/?oldversion=22.0

The privacy tour only gets shown if the URL contains 33.1 or greater, with an oldversion param value that is less than 33.1, e.g.

https://www.mozilla.org/en-US/firefox/33.1/whatsnew/?oldversion=22.0
(In reply to Holly Habstritt Gaal [:Habber] from comment #42)
> I'd like to confirm what logic is in place for the 33.1 /whatsnew tour and
> who will see it moving forward. 
> 
> Is this the current logic? 
> 
> - Users updating from 33.1 to 33.1.x+ will not see the /whatsnew tour since
> they have already seen it. The current plan is to leave it off until the 35
> Hello campaign. 
> 
> - Any users updating from versions prior to 33.1 should see /whatsnew.

Holly, this is correct as per our current logic for Firefox 33. Currently we're assuming /whatsnew will not be turned on in Firefox 34. If this changes, we would need to file a bug to update our logic on bedrock.
Flags: needinfo?(agibson)
(In reply to Alex Gibson [:agibson] from comment #45)
> Holly, this is correct as per our current logic for Firefox 33. Currently
> we're assuming /whatsnew will not be turned on in Firefox 34. If this
> changes, we would need to file a bug to update our logic on bedrock.

The code for this whatsnew page landed only in 33, so isn't going to appear in 34+ - although obviously a decision may be made to reintroduce it in later versions for some other feature we want to bring user's attention to.
I just manually checked and updating from a 22.0 build goes straight to 33.1.1 and shows the privacy tour as expected: 

https://www.mozilla.org/en-US/firefox/33.1.1/whatsnew/?oldversion=22.0
Sorry, I keep writing 31.1.1 when I mean 33.1.1.   E_TOO_MANY_RELEASES_LATELY
Great! Thanks for confirming. Sounds like everyone is on the same page.
See Also: → 1547830
You need to log in before you can comment on or make changes to this bug.