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

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Justin Dolske [:Dolske]]

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).

Comment 1

[:Gijs (he/him)]

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? :-)

Comment 2

[:Gijs (he/him)]

(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?

Comment 3

[:Gavin Sharp [email: [e-mail]]]

(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.

Comment 4

[Holly Habstritt Gaal [:Habber]]

(In reply to :Gavin Sharp [email: [e-mail]] 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.

Comment 5

[Chris More [:cmore]]

There's some 33.1 vs 33.0.1 chatter in these comments: https://bugzilla.mozilla.org/show_bug.cgi?id=1077517#c2

Comment 6

[Chris More [:cmore]]

Erin: can you confirm the version question in comment 5 on this bug and the bug I mentioned?

Comment 7

[Justin Dolske [:Dolske]]

The actual version number really isn't relevant to this bug.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-1077643-Enable-WhatsNew-page-in-product-for-33.X.patch

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 :)

Comment 9

[Matthew N. [:MattN]]

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

Comment 10

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 11

[:Gijs (he/him)]

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. :-(

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-1077643-Enable-WhatsNew-page-in-product-for-33.X.patch

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.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 14

[Matthew N. [:MattN]]

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.

Comment 15

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-1077643-Enable-WhatsNew-page-in-product-for-33.X.patch

(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)

Comment 16

[Matthew N. [:MattN]]

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

Comment 17

[Chris More [:cmore]]

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

Comment 18

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-1077643-Enable-WhatsNew-page-in-product-for-33.X.patch

(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 19

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8510698 [details] [diff] [review]
0001-Bug-1077643-Enable-WhatsNew-page-in-product-for-33.X.patch

oops - double-press of <enter> :)

Comment 20

[:Gavin Sharp [email: [e-mail]]]

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)

Comment 21

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-1077643-Enable-WhatsNew-page-in-product-for-33.X.patch

Removed the 'XXX' comment and carry r=gavin forward.

Dolske, are you happy for me to just land this on alder?

Comment 22

[Justin Dolske [:Dolske]]

Yeah, please go ahead and land on alder.

Comment 23

[Justin Dolske [: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.

Comment 24

[Justin Dolske [:Dolske]]

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

Comment 25

[Justin Dolske [:Dolske]]

Disabled the test that's failing, per sheriff request.

https://hg.mozilla.org/projects/alder/rev/3e492d503e8d

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/1d7405a097b1
https://hg.mozilla.org/releases/mozilla-release/rev/3e492d503e8d

Comment 27

[Catalin Varga [QA][:VarCat]]

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.

Comment 28

[Justin Dolske [:Dolske]]

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)

Comment 29

[:Gavin Sharp [email: [e-mail]]]

(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?

Comment 30

[Catalin Varga [QA][:VarCat]]

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.

Comment 31

[:Gavin Sharp [email: [e-mail]]]

Catalin, can you check the values of browser.startup.homepage_override.mstone in about:config before and after your update and list them here?

Comment 32

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Chris, can you check the snippets configuration for the scenario from comment 30, particularly re: their "showURL" or "silent" actions?

Comment 34

[Catalin Varga [QA][:VarCat]]

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

Comment 35

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Gavin Sharp [email: [e-mail]] 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.

Comment 36

[:Gavin Sharp [email: [e-mail]]]

Should be fixed per bug 1093187.

Comment 37

[Nick Thomas [:nthomas] (UTC+12)]

Sorry for the confusion caused by incorrect snippets.

Comment 38

[Catalin Varga [QA][:VarCat]]

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.

Comment 39

[Catalin Varga [QA][:VarCat]]

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?

Comment 40

[:Gavin Sharp [email: [e-mail]]]

Jennifer would know the answer to that.

Comment 41

[Jennifer Bertsch [: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

Comment 42

[Holly Habstritt Gaal [:Habber]]

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.

Comment 43

[Nick Thomas [:nthomas] (UTC+12)]

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'.

Comment 44

[Alex Gibson [:agibson]]

(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

Comment 45

[Alex Gibson [:agibson]]

(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.

Comment 46

[Mark Hammond [:markh] [:mhammond]]

(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.

Comment 47

[Alex Gibson [:agibson]]

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

Comment 48

[Nick Thomas [:nthomas] (UTC+12)]

Sorry, I keep writing 31.1.1 when I mean 33.1.1.   E_TOO_MANY_RELEASES_LATELY

Comment 49

[Holly Habstritt Gaal [:Habber]]

Great! Thanks for confirming. Sounds like everyone is on the same page.