1322723 - Show the default browser prompt on *second* startup for the feb 2017 funnelcake

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[:Gijs (he/him)]

This miiight just be flipping a pref to get the nightly behaviour (which IIRC doesn't show it on first startup).

Comment 1

[:Gijs (he/him)]

We should also make sure we keep showing it if the default browser changes after the user has made Firefox the default.

Comment 2

[Dão Gottwald [:dao]]

Attachment: patch

This code is incredibly ugly and unpleasant to work with. I hope I got this right.

New pref to be set for the funnelcake build:
browser.shell.skipDefaultBrowserCheckOnFirstRun

Comment 3

[:Gijs (he/him)]

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

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

Why can't we reuse the skipDefaultBrowserCheck mechanism which AFAICT already does this?

I suspect Jared will be a better reviewer here.

Comment 4

[Dão Gottwald [:dao]]

browser.shell.skipDefaultBrowserCheck is one of those confusing parts of this code. Apparently it's just there for keeping record of whether we're performing the first run. That's why it's already set to 'true' for all builds.

Comment 5

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #4)
> browser.shell.skipDefaultBrowserCheck is one of those confusing parts of
> this code. Apparently it's just there for keeping record of whether we're
> performing the first run. That's why it's already set to 'true' for all
> builds.

So we could reuse it rather than adding another pref, no? We could just edit the code so only the count stuff is behind the !BETA_OR_RELEASE check, and then flip the default value for the pref to false on !BETA_OR_RELEASE, flipping it to true on the funnelcake. That seems cleaner to me than adding yet another pref here.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

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

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

Yeah, this code is hairy and ugly. I'm OK with the patch here. Making it simpler is easier said than done.

Comment 7

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Dão Gottwald [:dao] from comment #4)
> > browser.shell.skipDefaultBrowserCheck is one of those confusing parts of
> > this code. Apparently it's just there for keeping record of whether we're
> > performing the first run. That's why it's already set to 'true' for all
> > builds.
> 
> So we could reuse it rather than adding another pref, no? We could just edit
> the code so only the count stuff is behind the !BETA_OR_RELEASE check, and
> then flip the default value for the pref to false on !BETA_OR_RELEASE,
> flipping it to true on the funnelcake. That seems cleaner to me than adding
> yet another pref here.

It's not clear to me how you'd do that while preserving the current purpose of that pref. Maybe it's possible, my head still hurts a bit from looking at this code. In any case we should play it safe since we'll want to uplift. If it's possible to get rid of the new pref again, we can do that in a followup.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/36ab9273387b
Add pref to allow release builds to show the default browser prompt on second startup. r=jaws

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/36ab9273387b

Comment 10

[Dão Gottwald [:dao]]

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

Approval Request Comment
[Feature/Bug causing the regression]: bug 1322718 comment 1

[User impact if declined]: standard release behavior in the funnelcake build

[Is this code covered by automated tests?]: no

[Has the fix been verified in Nightly?]: can't be verified in Nightly since the new behavior is disabled by default

[Needs manual test from QE? If yes, steps to reproduce]: I assume QA will be part of the funnelcake process. This change will need to be part of that.

[List of other uplifts needed for the feature/fix]: 

[Is the change risky?]: somewhat risky

[Why is the change risky/not risky?]: the code is messy. Easy for QA to test though, given prepared builds.

[String changes made/needed]:

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Onboarding funnelcake changes, OK for aurora and beta.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/87a48d902eb2

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/aa6ce690f3d8

Comment 14

[Andrei Vaida [:avaida]]

Setting the flag for verification, description available in Comment 0 and Comment 1.

Comment 15

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #4)
> browser.shell.skipDefaultBrowserCheck is one of those confusing parts of
> this code. Apparently it's just there for keeping record of whether we're
> performing the first run. That's why it's already set to 'true' for all
> builds.

filed bug 1363118