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

VERIFIED FIXED in Firefox 51

Status

()

defect
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: Gijs, Assigned: dao)

Tracking

(Depends on 1 bug, Blocks 1 bug)

unspecified
Firefox 53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 verified, firefox52 fixed, firefox53 fixed)

Details

(Whiteboard: pref: browser.shell.skipDefaultBrowserCheckOnFirstRun = true)

Attachments

(1 attachment)

This miiight just be flipping a pref to get the nightly behaviour (which IIRC doesn't show it on first startup).
We should also make sure we keep showing it if the default browser changes after the user has made Firefox the default.
Summary: Don't show the default browser prompt on first startup for the feb 2017 funnelcake → Show the default browser prompt on *second* startup for the feb 2017 funnelcake
Assignee: nobody → dao+bmo
Posted patch patchSplinter Review
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
Attachment #8820539 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8820539 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8820539 - Flags: review?(jaws)
(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 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.
Attachment #8820539 - Flags: review?(jaws) → review+
(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.
Pushed by dgottwald@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/36ab9273387b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Whiteboard: pref: browser.shell.skipDefaultBrowserCheckOnFirstRun = true
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]:
Attachment #8820539 - Flags: approval-mozilla-beta?
Attachment #8820539 - Flags: approval-mozilla-aurora?
Comment on attachment 8820539 [details] [diff] [review]
patch

Onboarding funnelcake changes, OK for aurora and beta.
Attachment #8820539 - Flags: approval-mozilla-beta?
Attachment #8820539 - Flags: approval-mozilla-beta+
Attachment #8820539 - Flags: approval-mozilla-aurora?
Attachment #8820539 - Flags: approval-mozilla-aurora+
Setting the flag for verification, description available in Comment 0 and Comment 1.
Flags: qe-verify+
Flags: needinfo?(gwimberly)
Flags: needinfo?(gwimberly)
Status: RESOLVED → VERIFIED
Flags: qe-verify+
(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
Blocks: 1367073
You need to log in before you can comment on or make changes to this bug.