Last Comment Bug 739793 - allow passing previous version to post-upgrade homepage override URL
: allow passing previous version to post-upgrade homepage override URL
Status: RESOLVED FIXED
[qa-]
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 14
Assigned To: :Gavin Sharp [email: gavin@gavinsharp.com]
:
Mentors:
Depends on: 743846
Blocks: 738407
  Show dependency treegraph
 
Reported: 2012-03-27 14:42 PDT by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2012-05-10 12:17 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed


Attachments
patch (4.25 KB, patch)
2012-03-27 15:04 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
mak77: review+
Details | Diff | Splinter Review
checked-in patch (4.43 KB, patch)
2012-04-06 16:09 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-27 14:42:17 PDT
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: gavin@gavinsharp.com] 2012-03-27 15:04:06 PDT
Created attachment 609904 [details] [diff] [review]
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] 2012-03-27 15:34:29 PDT
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: gavin@gavinsharp.com] 2012-03-27 15:43:39 PDT
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: gavin@gavinsharp.com] 2012-03-27 15:46:03 PDT
Oh, the _path_ (and host). No, that was entirely unintentional. I'll fix it before landing!
Comment 5 Marco Bonardo [::mak] 2012-03-27 17:01:51 PDT
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: gavin@gavinsharp.com] 2012-03-27 17:04:00 PDT
(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: gavin@gavinsharp.com] 2012-03-27 17:04:23 PDT
(but that's not obvious, so you're right that it's better to be clear)
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-28 10:15:48 PDT
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] 2012-03-29 08:48:59 PDT
https://hg.mozilla.org/mozilla-central/rev/ec441303e32e
Comment 10 Alex Keybl [:akeybl] 2012-03-30 09:30:00 PDT
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] 2012-04-02 13:54:57 PDT
(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: gavin@gavinsharp.com] 2012-04-02 15:34:41 PDT
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: gavin@gavinsharp.com] 2012-04-06 16:09:02 PDT
Created attachment 613014 [details] [diff] [review]
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: gavin@gavinsharp.com] 2012-04-06 16:10:50 PDT
Bug 742998 covers the server-side work to have this change take effect.

Note You need to log in before you can comment on or make changes to this bug.