392137 - Retention: take over user defaults during install on Win32

Status: RESOLVED FIXED

(Firefox :: Installer, defect, P4)

Description

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

See "Alter the default browser settings path for better user choice" at http://wiki.mozilla.org/Retention

1. Is this the first launch? (pref check?)
2. Is this the only profile for this user?
3. Is the app not set as the default?
4. Take defaults.

1. Is app launched from the OS? (url, applevent, etc. - won't be 100% detectable on all OS's)
2. Show UI as shown on wiki and act accordingly
http://wiki.mozilla.org/images/4/4e/Retention_Plan_2.010.png

note: comments about whether this is the right or wrong thing to do should be added to the following discussion page.
http://wiki.mozilla.org/Talk:Retention

Comment 1

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

Benjamin, I'd like your insight as to how feasible the steps above are?

Comment 2

[(not reading, please use [e-mail] instead)]

> 4. Take defaults.

what does "Take defaults." mean?

Comment 3

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

It means to make Firefox the default handler for the protocols and files it can handle at the user level

Comment 4

[Benjamin Smedberg]

Rob, let's talk in person, I'm not sure what the question is exactly. I don't particularly like making the behavior depend on whether you have one or more profiles installed.

Comment 5

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

Benjamin, definitely... I'll buy the drinks. :P

note: the reason for the check of the existence of other profiles is to prevent doing this for the same user if they have multiple profiles which can be leveraged by the community of testers. A better option might be to set this in profiles.ini or some other user specific place that would apply to all profiles.

Comment 6

[(not reading, please use [e-mail] instead)]

> It means to make Firefox the default handler for the protocols and files it 
> can handle at the user level

Thanks for clarifying.  Comments here:  http://wiki.mozilla.org/Talk:Retention#concerns_over_taking_over_the_default

Comment 7

[JT Batson]

Can we touch base on the status of the bug this week? 

Also, I know there are some questions/concerns about this, so want to talk through that and figure out the best way to work through them.

Comment 8

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

Sure, I'll contact you probably Thursday. As for progress, no progress due to working on other bugs we need for Firefox 3.0 and 2.0.0.7

note: after discussing this with mconnor the current thought is to by default do this in the installer (win32 only) and take over the defaults when performing a default install and provide the option not to take over the defaults in an advanced install. The rationale is that installers such as media player installers already do this.

JT Batson, is that acceptable? Do we still need to display the additional ui to say we've taken over defaults?

Comment 9

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

Could you also get Retention added to the PRD since it is our working document for Firefox 3.0?

Comment 10

[JT Batson]

Retention in to PRD- sure-- how do I do that?

As for the solution you outlined above, I think that makes sense. Thanks for your help.

Comment 11

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

(In reply to comment #10)
> Retention in to PRD- sure-- how do I do that?
Touchbase with Basil or Beltzner if he wasn't on vacation

> As for the solution you outlined above, I think that makes sense.
OK, I'll start on this hopefully within the week.

Comment 12

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

Attachment: installer screenshots

Hey Mike, since this has to be done just 'right' I want your take on this approach. I can hide the controls in the instance where an installation of Firefox is already set as the default browser. I can also add more install summary info.

btw: I've been wanting to add back the Summary wizard page (it was in the xpinstall wizard) for quite some time so we can change the next button to an install button just prior to the actual installation operations starting.

Comment 13

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

note: I'm planning on keeping the check to see if the app is already default to only checking if we are the default handler for http. The app has thorough code for checking and this should suffice for this bug IMO.

Comment 14

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

Attachment: strings patch (checked in)

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 308034 [details] [diff] [review]
strings patch (checked in)

only nits:

- "Use $BrandShortName as my default web browser"

also, I think that the checkbox should probably be aligned with the "Choose your..." label.

Other than that, looks great. You can make those changes at checkin.

Comment 16

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

Comment on attachment 308034 [details] [diff] [review]
strings patch (checked in)

Strings checked in to trunk
Checking in mozilla/browser/locales/en-US/installer/custom.properties;
/cvsroot/mozilla/browser/locales/en-US/installer/custom.properties,v  <--  custom.properties
new revision: 1.15; previous revision: 1.14
done

Comment 17

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

bah... forgot to make the string change in original checkin
Checking in mozilla/browser/locales/en-US/installer/custom.properties;
/cvsroot/mozilla/browser/locales/en-US/installer/custom.properties,v  <--  custom.properties
new revision: 1.16; previous revision: 1.15
done

Comment 18

[Francesco Lodolo [:flod]]

Maybe a spell check on param names... ;-)
SUMMARY_MAKE_DEAFULT

Comment 19

[Reed Loden [:reed]]

Since only the strings have landed, I'll correct the string name.

Comment 20

[Reed Loden [:reed]]

s/SUMMARY_MAKE_DEAFULT/SUMMARY_MAKE_DEFAULT/.

Checking in browser/locales/en-US/installer/custom.properties;
/cvsroot/mozilla/browser/locales/en-US/installer/custom.properties,v  <--  custom.properties
new revision: 1.17; previous revision: 1.16
done

Comment 21

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

bah... the name isn't displayed and it is better to not make l10n have to modify it just for that. oh well

Comment 22

[Marek Stępień [:marcoos, inactive]]

(In reply to comment #19)
> Since only the strings have landed, I'll correct the string name.

Yeah, it's just localizers' time. Let's waste it. How long could it take, anyway? A minute at most, right?

But there are 45 locales, so it's really 45 minutes that could have been spent on sth slightly more important, like testing... :(

Comment 23

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

Attachment: patch rev1

Comment 24

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

Attachment: patch rev2

some cruft snuck in to the previous patch

Comment 25

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

Comment on attachment 314485 [details] [diff] [review]
patch rev2

note: I didn't bother with the checks since we ask if the app should be take its handlers before we know the install dir so we can't test against the install location

Comment 26

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

Comment on attachment 314485 [details] [diff] [review]
patch rev2

also requesting from mconnor in case he can review this

Comment 27

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

Comment on attachment 314485 [details] [diff] [review]
patch rev2

also requesting from mconnor in case he has time to review this

Comment 28

[Benjamin Smedberg]

Comment on attachment 314485 [details] [diff] [review]
patch rev2

I'm no NSIS expert, but this looks correct to me.

Comment 29

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

Comment on attachment 314485 [details] [diff] [review]
patch rev2

Requesting a1.9. This patch uses the existing code for setting Firefox as the default browser and is pretty darn safe.

Also, Seth said he should be able to review the patch this afternoon so we will likely have a third set of eyes on it.

Comment 30

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

Attachment: updated screenshots for all states

Comment 31

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

Attachment: patch as checked in

Comment 32

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

Thanks for the reviews Benjamin and Seth!

Checked in to trunk

Checking in mozilla/browser/installer/windows/nsis/installer.nsi;
/cvsroot/mozilla/browser/installer/windows/nsis/installer.nsi,v  <--  installer.nsi
new revision: 1.42; previous revision: 1.41
done
Checking in mozilla/toolkit/mozapps/installer/windows/nsis/common.nsh;
/cvsroot/mozilla/toolkit/mozapps/installer/windows/nsis/common.nsh,v  <--  common.nsh
new revision: 1.40; previous revision: 1.39
done