1432310 - About dialog manual update URL is incorrect for beta

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P1)

Description

[Molly Howell (she/her) (mostly inactive)]

The URL that we show in the about dialog when we fall back to offering a manual update is incorrect for beta channel builds. Since the URL comes from a pref (app.update.url.manual) that's set in firefox-branding.js, the same value is used for both release and beta. It's correct for release, but clicking on that link in a beta takes you to the release download page. For beta, the URL should have "/beta" appended to it.

Comment 1

[Robert Strong (they/them - no direct email)]

That's controlled by Firefox prefs and the Firefox about UI so moving to Firefox -> General

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

This is tricky to do. We deliberately make beta almost indistinguishable from release (name, icon, configuration settings), which is why we only have build-time flags like RELEASE_OR_BETA and EARLY_BETA_OR_EARLIER. Mike, is there any way to tweak browser/branding/official/pref/firefox-branding.js to use a different value for app.update.url.manual?

Isn't this something we could do more easily on the server by parsing the UA string?

Comment 3

[Mike Hommey [:glandium]]

Why not base that on app.update.channel? That's one of the few legitimate uses of that pref.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Bug 1432310 - Fix manual update URL for beta.

Review commit: https://reviewboard.mozilla.org/r/216508/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216508/

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Thank you, that makes perfect sense.

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8946501 [details]
Bug 1432310 - Fix manual update URL for beta.

https://reviewboard.mozilla.org/r/216508/#review222266

::: browser/branding/official/pref/firefox-branding.js:14
(Diff revision 1)
>  pref("app.update.interval", 43200); // 12 hours
>  // Give the user x seconds to react before showing the big UI. default=192 hours
>  pref("app.update.promptWaitTime", 691200);
>  // URL user can browse to manually if for some reason all update installation
>  // attempts fail.
> +#ifdef MOZ_UPDATE_CHANNEL == beta

you want #if, not #ifdef.

::: browser/branding/official/pref/firefox-branding.js:21
(Diff revision 1)
> +#else
>  pref("app.update.url.manual", "https://www.mozilla.org/firefox/");
> +#endif
>  // A default value for the "More information about this update" link
>  // supplied in the "An update is available" page of the update wizard.
>  pref("app.update.url.details", "https://www.mozilla.org/%LOCALE%/firefox/notes");

How about this one?

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Mike Hommey [:glandium] from comment #6)
> ::: browser/branding/official/pref/firefox-branding.js:21
> (Diff revision 1)
> > +#else
> >  pref("app.update.url.manual", "https://www.mozilla.org/firefox/");
> > +#endif
> >  // A default value for the "More information about this update" link
> >  // supplied in the "An update is available" page of the update wizard.
> >  pref("app.update.url.details", "https://www.mozilla.org/%LOCALE%/firefox/notes");
> 
> How about this one?

Not sure, Matt?

Comment 8

[Molly Howell (she/her) (mostly inactive)]

(In reply to Panos Astithas [:past] (please ni?) from comment #7)
> (In reply to Mike Hommey [:glandium] from comment #6)
> > ::: browser/branding/official/pref/firefox-branding.js:21
> > (Diff revision 1)
> > > +#else
> > >  pref("app.update.url.manual", "https://www.mozilla.org/firefox/");
> > > +#endif
> > >  // A default value for the "More information about this update" link
> > >  // supplied in the "An update is available" page of the update wizard.
> > >  pref("app.update.url.details", "https://www.mozilla.org/%LOCALE%/firefox/notes");
> > 
> > How about this one?
> 
> Not sure, Matt?

Looks like that should also be changed, to "https://www.mozilla.org/%LOCALE%/firefox/beta/notes". That matches what we have on Nightly, and that URL does indeed redirect to the current beta release notes.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8946501 [details]
Bug 1432310 - Fix manual update URL for beta.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216508/diff/1-2/

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Mike Hommey [:glandium] from comment #6)
> you want #if, not #ifdef.

I looked through the repo and found another instance:

https://searchfox.org/mozilla-central/rev/c56f656febb1aa58c72b218699e24a76425bd284/browser/app/profile/firefox.js#1736

Shall I fix it here or in another bug?

Comment 11

[Mike Hommey [:glandium]]

Please file a separate bug, and make it block bug 1419312.

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8946501 [details]
Bug 1432310 - Fix manual update URL for beta.

https://reviewboard.mozilla.org/r/216508/#review222626

::: browser/branding/official/pref/firefox-branding.js:18
(Diff revision 2)
> -// attempts fail.
> +// all update installation attempts fail.
> +// app.update.url.details: a default value for the "More information about this
> +// update" link supplied in the "An update is available" page of the update
> +// wizard.
> +#if MOZ_UPDATE_CHANNEL == beta
> +pref("app.update.url.manual", "https://www.mozilla.org/firefox/beta");

Random thought: shouldn't those manual links also use %LOCALE% instead of relying on www.mozilla.org doing a possibly wrong redirection?

Comment 13

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #11)
> Please file a separate bug, and make it block bug 1419312.

Also, could you file a Core::Build config bug to make the preprocessor reject those constructs in the first place?

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

> Random thought: shouldn't those manual links also use %LOCALE% instead of
> relying on www.mozilla.org doing a possibly wrong redirection?

It's a valid suggestion, but I don't know if our server is properly set up to deal with every locale, so I'd rather leave it as is until I know more. I'll see if I can find someone who knows about this.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/924b8c32b9a8
Fix manual update URL for beta. r=glandium

Comment 16

[Panos Astithas (he/him) [:past] (please ni?)]

> Also, could you file a Core::Build config bug to make the preprocessor
> reject those constructs in the first place?

Filed bug 1434765.

Comment 17

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/924b8c32b9a8

Comment 18

[Osmose [:osmose, :mkelly]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=749b1b31bb0422aa8b4a78b350cdee93dd42ed11

Comment 19

[Osmose [:osmose, :mkelly]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ba2998cb6f730206ce410cefccad940bde88bcb