739793 - allow passing previous version to post-upgrade homepage override URL

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

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

The goal is to make it possible to serve different content to users after upgrade, depending on which version they're upgrading from.

Comment 1

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

Attachment: patch

This should work, it adds an %OLD_VERSION% parameter to the whatsnew URL. This is technically the Gecko version that was last run with this profile, but for the majority of users that's going to be the build that they updated from (this also has the benefit of working in cases where users upgrade outside of our update system, e.g. by downloading new builds manually).

The patch makes use of the new parameter by adding an "oldversion" param to the existing whatsnew URL, but we may need to adjust that depending on the needs of whoever will end up implementing the server side parts of this.

Comment 2

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

Comment on attachment 609904 [details] [diff] [review]
patch

>diff --git a/browser/branding/nightly/pref/firefox-branding.js b/browser/branding/nightly/pref/firefox-branding.js
>--- a/browser/branding/nightly/pref/firefox-branding.js
>+++ b/browser/branding/nightly/pref/firefox-branding.js
>@@ -1,9 +1,9 @@
>-pref("startup.homepage_override_url","http://www.mozilla.org/projects/%APP%/%VERSION%/whatsnew/");
>+pref("startup.homepage_override_url","http://www.mozilla.com/%LOCALE%/%APP%/%VERSION%/whatsnew/?oldversion=%OLD_VERSION%");

Was changing the path for nightlies intended here ?

Comment 3

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

Yes, I did that intentionally so that we could easily test the behavior.

(I wasn't sure why we still set the whatsnew URL in nightly branding, it seems like we should avoid doing it for the same reason we don't do it on Aurora. But it is useful for testing the functionality, I guess.)

Comment 4

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

Oh, the _path_ (and host). No, that was entirely unintentional. I'll fix it before landing!

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 609904 [details] [diff] [review]
patch

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

apart what nthomas said:

::: browser/components/nsBrowserContentHandler.js
@@ +582,5 @@
>      var haveUpdateSession = false;
>      try {
> +      // Read the old value of homepage_override.mstone before
> +      // needHomepageOverride updates it.
> +      let old_mstone;

would be better to init this to an empty string or a special value?
As it is, this may end up asking for /whatsnew/?oldversion=undefined
is that expected by the server? Should at least be notified as a possible value to whoever manages that page.

@@ +611,5 @@
>              if (prefb.prefHasUserValue("app.update.postupdate"))
>                overridePage = getPostUpdateOverridePage(overridePage);
> +
> +            // Insert old_mstone into the URL
> +            overridePage = overridePage.replace("%OLD_VERSION%", old_mstone);

nit: the comment is not really useul, just saying what the code is.  Could either be removed or specify the reason we do this (comment 0 here is quite good).

Comment 6

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

(In reply to Marco Bonardo [:mak] from comment #5)
> would be better to init this to an empty string or a special value?
> As it is, this may end up asking for /whatsnew/?oldversion=undefined
> is that expected by the server? Should at least be notified as a possible
> value to whoever manages that page.

I'll use "", but it should be impossible for needHomePageOverride to return OVERRIDE_NEW_MSTONE if the value was empty/non-existent, so it shouldn't matter in practice.

Comment 7

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

(but that's not obvious, so you're right that it's better to be clear)

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec441303e32e

We may want to backport this to aurora/beta so that we can be smarter in our next update prompts.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/ec441303e32e

Comment 10

[Alex Keybl [:akeybl]]

Let's move ahead with backporting this to Aurora 13 and Beta 12 in preparation for the 3.6->12 upgrade.

Comment 11

[Alex Keybl [:akeybl]]

(In reply to Alex Keybl [:akeybl] from comment #10)
> Let's move ahead with backporting this to Aurora 13 and Beta 12 in
> preparation for the 3.6->12 upgrade.

We're actually waiting until after Thursday afternoon's meeting (and agreement on the URL we'll use) to land this.

One question while we wait though - what'd be the best way for QA to verify this fix before we first use it with our GA? Is creating a custom snippet to test internally our only option?

Comment 12

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

This can be tested on Nightly by installing a 13a1 build and updating it to 14a1. We can do the same on beta once this lands there.

Comment 13

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

Attachment: checked-in patch

We had the meeting on Thursday and decided that this parameter would be suitable. The snippets for update from 4+->12 should prevent the page from being opened entirely, so this may not even be technically required, but it's still useful in case that functionality ends up not working for some reason.

Comment 14

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

Bug 742998 covers the server-side work to have this change take effect.

Comment 15

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

https://hg.mozilla.org/releases/mozilla-beta/rev/2afef8c92bd8
https://hg.mozilla.org/releases/mozilla-aurora/rev/9f82dc1cb89c