The stub installer currently contains branding download links that point to x86 builds for both x86 and x64 builds. We should have different links for x86 and x64 depending on !ifdef HAVE_64BIT_OS$ Example: /browser/branding/aurora/branding.nsi 1.13 +!define URLStubDownload "http://download.mozilla.org/?product=firefox-latest&os=win&lang=${AB_CD}" 1.14 +!define URLManualDownload "http://download.mozilla.org/?product=firefox-latest&os=win&lang=${AB_CD}"

Agreed. We are going to punt on this until much later so we can get the support for release completed first and finish any other work that is a higher priority.

Note: we had discussed automatically downloading the 64 vs 32 bit build depending on the OS instead of supplying one stub for 32 bit and another for 64 bit. This would only happen after our 64 bit build of Firefox is supported.

Ya I thought of automatically detecting in the stub too, but posted this for now since it isn't supported yet. I just wanted to have this on file, and yup it is a very low priority.

rstrong recommends that the Win64-aware stub installer place new 64-bit installs in the 64-bit "C:\Program Files" directory, allowing 64-bit side-by-side with existing 32-bit installs in "C:\Program Files (x86)". For the 32-to-64-bit upgrade case (bug 1274659), he recommends upgrading in place ("pave over" install) in the 32-bit "C:\Program Files (x86)" directory instead of moving files to the 64-bit "C:\Program Files" directory. This would avoid breaking any existing user shortcuts that point to the original "C:\Program Files (x86)" directory.

The following doc summarizes our current proposal for the 64-bit Firefox install experience: https://docs.google.com/document/d/1MlhbdeBCHzgM4_BYnnSEI1WNSHhv_nJp1Kr_lfJ8XsI/ The stub installer will silently choose 32-bit or 64-bit Firefox for the user. If the user is running a 64-bit version of Windows 7/8/8.1/10 and has 4 (TBD: or 3.8?) or more GiB RAM, then the stub installer will download the 64-bit Firefox package. Otherwise, it will download the 32-bit Firefox package. We will more clearly highlight the download page's links to the 32-bit and 64-bit full installers for users who want to install a Firefox version other than the 32-bit or 64-bit version automatically selected by the stub installer.

Matt will be adding 64-bit support to the stub installer. The current UX plan is described in this Google Doc: https://docs.google.com/document/d/1MlhbdeBCHzgM4_BYnnSEI1WNSHhv_nJp1Kr_lfJ8XsI/ 1. The stub installer will silently choose 32-bit or 64-bit Firefox for the user: 1a. If the user is running a 64-bit version of Windows 7/8/8.1/10 and has 3,800 MiB or more RAM, then the stub installer will download the 64-bit Firefox package and install it to the 64-bit Program Files directory/etc. We will measure available physical memory using Windows’ GlobalMemoryStatusEx() API, which excludes space taken up by the BIOS memory/etc: https://dxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/nsExceptionHandler.cpp#774-801 1b. If the user is running XP, Vista, or 32-bit Windows 7/8/8.1/10 or has less than 3,800 MiB RAM, then the stub installer will download the 32-bit Firefox package and install it to the 32-bit Program Files directory/etc. 2. The stub installer will offer the user a choice in the Options page to override the 32/64-bit default.

This patch is preliminary because it does not implement the final UI. I just added a new checkbox and moved the shortcut options up a bit to keep the middle from looking alarmingly cramped. The real design should be much nicer. But all the behaviors spelled out in comment 6 are there, and seem to be working.

Comment on attachment 8785470 [details] [diff] [review] Preliminary Patch Review of attachment 8785470 [details] [diff] [review]: ----------------------------------------------------------------- > keep the middle from looking alarmingly cramped. The real design should be much nicer. I'm sure Michael will design something that looks good :) but if you have UX or implementation concerns from what you've seen with your prototype, it might be worth giving him a heads up. ::: browser/installer/windows/nsis/stub.nsi @@ +390,5 @@ > > + ${If} $DefaultIs64Bit == "1" > + StrCpy $INSTDIR "$PROGRAMFILES64\${BrandFullName}\" > + ${Else} > + StrCpy $INSTDIR "$PROGRAMFILES32\${BrandFullName}\" If 64-bit is selected as the default but the user unchecks the 64-bit option, will the $INSTDIR directory switch to $PROGRAMFILES32? @@ +404,5 @@ > System::Call 'user32::SetProcessDPIAware()' > ${EndIf} > !endif > > + ; If we have any existing installation, use it's location as the default s/it's location/its location/ typo :)

(In reply to Chris Peterson [:cpeterson] from comment #8) > I'm sure Michael will design something that looks good :) but if you have UX > or implementation concerns from what you've seen with your prototype, it > might be worth giving him a heads up. Michael had just sent out his design right before I wrote that comment, and he had specifically addressed this, so it's all good. :) > If 64-bit is selected as the default but the user unchecks the 64-bit > option, will the $INSTDIR directory switch to $PROGRAMFILES32? It will not. I thought about that, and I didn't want the directory to change right in the user's face. But it probably does make sense to have it change if the user hasen't already selected something non-default, and if we didn't get our default from an existing installation. > s/it's location/its location/ typo :) Good catch; thanks.

This patch now implements verdi's UI design, as well as the tweaks that cpeterson pointed out.

So that nobody has to build the installer to see what the revised options page looks like.

Wow... the new UI now makes it so if a user doesn't want a single shortcut they only have the option to create all or create no shortcuts. This is a footgun for anyone that unchecks that since there won't be any shortcuts.

Comment on attachment 8786847 [details] [diff] [review] Patch Review of attachment 8786847 [details] [diff] [review]: ----------------------------------------------------------------- The UI LGTM! It might be nice to split 64-bit support and the unrelated UI redesign into separate patches, but that's your call. :) ::: browser/installer/windows/nsis/stub.nsi @@ +1918,5 @@ > + ${If} $FoundPreviousInstall == "0" > + ${AndIf} $INSTDIR == $InitialInstallDir > + ${NSD_GetText} $Droplist64Bit $0 > + ${If} $0 == "${DroplistOption32Bit}" > + StrCpy $InitialInstallDir "${DefaultInstDir32bit}" Is this code switching the install directory between "Program Files" and "Program Files (x86)" when the user changes the 32/64-bit option? Maybe add a comment describing what we're doing here?

Please verify that the dropdown is both rtl and positioned correctly when using an rtl locale. You can simulate this by adding en-US_rtl to https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/windows/nsis/locales.nsi

(In reply to Chris Peterson [:cpeterson] from comment #13) > Is this code switching the install directory between "Program Files" and > "Program Files (x86)" when the user changes the 32/64-bit option? Maybe add > a comment describing what we're doing here? That's exactly what it's doing, but only if the user hasn't changed it themselves, and if we didn't find an existing installation and set our default to its location. I'll add a comment. (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #14) > Please verify that the dropdown is both rtl and positioned correctly when > using an rtl locale. So the dropdown doesn't change position, but neither do the checkboxes? I'm not sure what the correct behavior is.

(In reply to Chris Peterson [:cpeterson] from comment #16) > Michael Verdi said in email that the shortcut option should: > > - Use "Create Desktop and Task Bar Shortcuts for Firefox" for Windows XP and > Vista > - Use "Create Desktop and Quick Launch Shortcuts for Firefox" for Windows 7+ > I think the "quick launch" wording is for XP and Vista and the "task bar" wording is for 7+.

oops. Yes, you are correct! I copy and pasted the wrong email.. :\ - Use "Create Desktop and Quick Launch Shortcuts for Firefox" for Windows XP and Vista - Use "Create Desktop and Task Bar Shortcuts for Firefox" for Windows 7+

Implemented the changes described above, and some minor refactoring.

Comment on attachment 8789003 [details] [diff] [review] Patch revision 2 When changing strings you have to use a new name so l10n will pick up the new string.

Specifically ADD_SC_TASKBAR ADD_SC_QUICKLAUNCHBAR need new names

(In reply to Matt Howell [:mhowell] from comment #15) <snip> > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > comment #14) > > Please verify that the dropdown is both rtl and positioned correctly when > > using an rtl locale. > So the dropdown doesn't change position, but neither do the checkboxes? I'm > not sure what the correct behavior is. Did you run mach build before running mach build installer? I just did after adding !define en-US_rtl to locales.nsi and it showed up as rtl with the en-US build.

The only change in this revision is to rename the two changed strings, as mentioned in comment 21.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #23) > (In reply to Matt Howell [:mhowell] from comment #15) > <snip> > > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > > comment #14) > > > Please verify that the dropdown is both rtl and positioned correctly when > > > using an rtl locale. > > So the dropdown doesn't change position, but neither do the checkboxes? I'm > > not sure what the correct behavior is. > Did you run mach build before running mach build installer? I just did after > adding > !define en-US_rtl > > to locales.nsi and it showed up as rtl with the en-US build. I have no idea what I did before, but I just tried again, and it does show up as RTL for me now.

Comment on attachment 8789921 [details] [diff] [review] Patch revision 3 Windows alt key shortcuts should be as close to the control (checkbox in this case) as possible so it is easily scanned as associated with the control. Please use C for the alt key shortcut. +ADD_SC_DESKTOP_TASKBAR=Create desktop and task bar &shortcuts for $BrandShortName +ADD_SC_DESKTOP_QUICKLAUNCHBAR=Create desktop and quick launch &shortcuts for $BrandShortName Still working on the code review.

At least on RTL it looks like the dropdown is not sized properly for the text it contains. The text should be verified using an actual RTL locale.

Same goes for ltr.

It would be a good thing to check with Axel whether or not the text in the dropdown needs to be localized. There is precedence for not localizing some text such as brand names and install requirements such as "Microsoft Windows 7 x64" and "Microsoft Windows XP SP2".

Axel, I suspect the text in the dropdown doesn't need to be localized and if there are concerns about localizing it that those concerns could be addressed so it doesn't have to be localized. The strings are: 32-bit $BrandShortName 64-bit $BrandShortName What is your take on this?

Comment on attachment 8789954 [details] screenshot Note that when the maintenance service is already installed or if the user cannot install it the checkbox is not displayed. In this case the control following it is typically re-positioned accordingly.

Comment on attachment 8789954 [details] screenshot Note: there appears to be enough space for the addition of the control by slightly lessening the space between the controls in the old ui without the unrelated to 64 bit stub installer change to the shortcuts. I'm not against this change but it does add risk.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #30) > Axel, I suspect the text in the dropdown doesn't need to be localized and if > there are concerns about localizing it that those concerns could be > addressed so it doesn't have to be localized. > The strings are: > 32-bit $BrandShortName > 64-bit $BrandShortName > > What is your take on this? https://transvision.mozfr.org/string/?entity=mail/chrome/messenger/messenger.properties:carbonFailurePluginsMessage.restartButton.label&repo=aurora is an existing localization of "Restart in 32-bit mode". That shows quite a variety in how 32-bit is shown across our languages, so I think the better UX would be to get this string localized. Up to the point that the arabic literal "32" doesn't show up in the translation for Khmer.

Comment on attachment 8789921 [details] [diff] [review] Patch revision 3 This should also include in the ping whether the Firefox 32 bit or 64 bit was installed. It would be a good thing to also send whether or not the system met the minimum requirements for defaulting to 64 bit. Be sure to change !define StubURLVersion "v7" to !define StubURLVersion "v8"

Looks like the ping does contain whether the 32 bit or 64 bit was installed. The rest of comment #34 stands.

Comment on attachment 8789921 [details] [diff] [review] Patch revision 3 The following are all fairly minor but minusing due to comment #26, comment #27, and comment #28. The dropdown text will need to be verified in a ltr locale which can be done using en-US using a string along with brandShortName in the link Axel provided, and verifying the strings appear correctly in the dropdown. Also as Axel mention in comment #33, make sure the arabic literal "32" doesn't show up in the translation for Khmer. diff --git a/browser/installer/windows/nsis/stub.nsi b/browser/installer/windows/nsis/stub.nsi --- a/browser/installer/windows/nsis/stub.nsi +++ b/browser/installer/windows/nsis/stub.nsi @@ -1168,20 +1204,20 @@ Function leaveOptions Pop $0 ${GetSecondsElapsed} "$StartOptionsPhaseTickCount" "$0" $OptionsPhaseSeconds ; It is possible for this value to be 0 if the user clicks fast enough so ; increment the value by 1 if it is 0. ${If} $OptionsPhaseSeconds == 0 IntOp $OptionsPhaseSeconds $OptionsPhaseSeconds + 1 ${EndIf} - ${NSD_GetState} $CheckboxShortcutOnBar $CheckboxShortcutOnBar - ${NSD_GetState} $CheckboxShortcutInStartMenu $CheckboxShortcutInStartMenu - ${NSD_GetState} $CheckboxShortcutOnDesktop $CheckboxShortcutOnDesktop + ${NSD_GetState} $CheckboxShortcuts $CheckboxShortcuts + ${NSD_GetText} $DroplistArch $DroplistArch ${NSD_GetState} $CheckboxSendPing $CheckboxSendPing + nit: I prefer not adding the extra newline since this is related to the other get states for the install. !ifdef MOZ_MAINTENANCE_SERVICE ${NSD_GetState} $CheckboxInstallMaintSvc $CheckboxInstallMaintSvc !endif @@ -1890,16 +1928,33 @@ Function OnClick_ButtonBrowse ${EndIf} ${If} $0 != "" StrCpy $INSTDIR "$0" System::Call 'user32::SetWindowTextW(i $DirRequest, w "$INSTDIR")' ${EndIf} FunctionEnd +Function OnChange_DroplistArch + ; When the user changes the 32/64-bit setting, + ; change the default install path to use the correct version of Program Files. + ; But only do that if the user hasn't selected their own install path yet, + ; and if we didn't select our default as the location of an existing install. nit: go up to 80 on each line + ; When the user changes the 32/64-bit setting, change the default install path + ; to use the correct version of Program Files. But only do that if the user + ; hasn't selected their own install path yet, and if we didn't select our + ; default as the location of an existing install. + ${If} $FoundPreviousInstall == "0" + ${AndIf} $INSTDIR == $InitialInstallDir This doesn't seem right. If someone has a 32 bit installation, they have support for 64 bit, and they switch back to a 32 bit installation $InitialInstallDir will be for the 64 bit install, etc. + ${NSD_GetText} $DroplistArch $0 + ${If} $0 == "$(VERSION_32BIT)" + StrCpy $InitialInstallDir "${DefaultInstDir32bit}" + ${Else} + StrCpy $InitialInstallDir "${DefaultInstDir64bit}" + ${EndIf} + ${NSD_SetText} $DirRequest $InitialInstallDir + ${EndIf} +FunctionEnd +

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #36) > + ; When the user changes the 32/64-bit setting, change the default install path > + ; to use the correct version of Program Files. But only do that if the user > + ; hasn't selected their own install path yet, and if we didn't select our > + ; default as the location of an existing install. > > + ${If} $FoundPreviousInstall == "0" > + ${AndIf} $INSTDIR == $InitialInstallDir > This doesn't seem right. If someone has a 32 bit installation, they have > support for 64 bit, and they switch back to a 32 bit installation > $InitialInstallDir will be for the 64 bit install, etc. I don't think so; if a 32-bit installation already existed, then $FoundPreviousInstall is set and this logic never runs, so the original $InitialInstallDir, which is for the current install, is used. Still working on the other comments.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #27) > Created attachment 8789953 [details] > rtl screenshot > > At least on RTL it looks like the dropdown is not sized properly for the > text it contains. The text should be verified using an actual RTL locale. This seems to be caused by the size of the dropdown button increasing with the display scaling setting; I can reproduce it by increasing that regardless of the text direction. Unfortunately there doesn't seem to be a way to set the width of just the "text area" of the dropdown, you have to include the button also, which means you have to know how wide it's going to need to be. I'll increase the width so that it works with at least 200% scaling, if I can't find a way to get the actual correct width (haven't so far).

(In reply to Matt Howell [:mhowell] from comment #37) > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > comment #36) > > + ; When the user changes the 32/64-bit setting, change the default install path > > + ; to use the correct version of Program Files. But only do that if the user > > + ; hasn't selected their own install path yet, and if we didn't select our > > + ; default as the location of an existing install. > > > > + ${If} $FoundPreviousInstall == "0" > > + ${AndIf} $INSTDIR == $InitialInstallDir > > This doesn't seem right. If someone has a 32 bit installation, they have > > support for 64 bit, and they switch back to a 32 bit installation > > $InitialInstallDir will be for the 64 bit install, etc. > > I don't think so; if a 32-bit installation already existed, then > $FoundPreviousInstall is set and this logic never runs, so the original > $InitialInstallDir, which is for the current install, is used. This should be simple to verify. Install both 32 and a 64 bit Firefox into non-default directories and check if the correct install location is shown when switching between 32 and 64 bit.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #39) > This should be simple to verify. Install both 32 and a 64 bit Firefox into > non-default directories and check if the correct install location is shown > when switching between 32 and 64 bit. Done; whichever version is installed does get its directory selected, and it remains selected when toggling the dropdown.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #36) > The dropdown text will need to be verified in a ltr > locale which can be done using en-US using a string along with > brandShortName in the link Axel provided, and verifying the strings appear > correctly in the dropdown. I tried using the string from [https://transvision.mozfr.org/string/?entity=browser/installer/nsisstrings.properties:MAKE_DEFAULT&repo=release#ar] as the dropdown options and setting the en-US_rtl define. I checked that the string was rendered identically as on that web page, that the box and its label correctly swapped positions, and that $BrandShortName was substituted correctly. The default directory switching logic also continued to work. I'm not sure what else should be verified for this. > Also as Axel mention in comment #33, make sure > the arabic literal "32" doesn't show up in the translation for Khmer. I think Axel brought this up just to demonstrate the amount of variability in how strings containing "32-bit" get translated, not to express a specific requirement about the Khmer version.

Verdi, what are your thoughts on comment 32? I tried it that way before you sent out your design, and I thought it looked a bit crowded, but my opinion on design questions is probably, well, questionable.

This revision addresses all comments except comment 32 and a few that I answered above.

(In reply to Matt Howell [:mhowell] from comment #42) > Verdi, what are your thoughts on comment 32? I tried it that way before you > sent out your design, and I thought it looked a bit crowded, but my opinion > on design questions is probably, well, questionable. That was the first thing I tried and both Stephen Horlander and I agreed that it didn't work.

In the English US translation there are no letters that have a descender (e.g. y) above the dropdown. I would be surprised if that is the case for all locales though and the spacing between the bottom of a letter with a descender and the top of the dropdown looks like it should be increased to the same amount as there is with the text above the textbox for the install path. The text in the dropdown doesn't have the same baseline as the text beside it. It seems to me that the amount of space between "Version to install" and the dropdown is rather large and the amount should be the same as between the checkbox and the text associated with the checkbox.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #45) > Created attachment 8791684 [details] > screenshot <snip> > The text in the dropdown doesn't have the same baseline as the text beside > it. Note: this might be due to this being a hiDPI system but it would be a good thing to see if it can be aligned better.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #45) > Created attachment 8791684 [details] > screenshot This defaulted to my custom 32 bit install directory for installing 64 bit. Is that intentional and if so why?

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #45) > Created attachment 8791684 [details] > screenshot > > In the English US translation there are no letters that have a descender > (e.g. y) above the dropdown. I would be surprised if that is the case for > all locales though and the spacing between the bottom of a letter with a > descender and the top of the dropdown looks like it should be increased to > the same amount as there is with the text above the textbox for the install > path. The dimensions of the rendered localized option strings are what the size the box is set from, so I don't think this would be a problem. > The text in the dropdown doesn't have the same baseline as the text beside > it. I agree. I did the best I could, and it is less egregious when scaling isn't on. But to do it properly I'd have to find the baseline of the dropdown, then position the label with the same baseline, and I have no idea how to do either of those things. And I haven't succeeded in looking them up. Any advice would be appreciated. > It seems to me that the amount of space between "Version to install" and the > dropdown is rather large and the amount should be the same as between the > checkbox and the text associated with the checkbox. Agreed. Not sure what I was doing there, I'll cut that down. (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #47) > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > comment #45) > > Created attachment 8791684 [details] > > screenshot > This defaulted to my custom 32 bit install directory for installing 64 bit. > Is that intentional and if so why? It's intentional. Since we decided on leaving the install dir the same when we start doing updates to 64-bit, I thought it made sense here as well. Also I thought detecting the architecture of the existing install sounded hard, but now I think I could get it out of the registry without too much trouble.

(In reply to Matt Howell [:mhowell] from comment #48) > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > comment #45) > > Created attachment 8791684 [details] > > screenshot > > > > In the English US translation there are no letters that have a descender > > (e.g. y) above the dropdown. I would be surprised if that is the case for > > all locales though and the spacing between the bottom of a letter with a > > descender and the top of the dropdown looks like it should be increased to > > the same amount as there is with the text above the textbox for the install > > path. > The dimensions of the rendered localized option strings are what the size > the box is set from, so I don't think this would be a problem. > > > The text in the dropdown doesn't have the same baseline as the text beside > > it. > I agree. I did the best I could, and it is less egregious when scaling isn't > on. But to do it properly I'd have to find the baseline of the dropdown, > then position the label with the same baseline, and I have no idea how to do > either of those things. And I haven't succeeded in looking them up. Any > advice would be appreciated. > > > It seems to me that the amount of space between "Version to install" and the > > dropdown is rather large and the amount should be the same as between the > > checkbox and the text associated with the checkbox. > Agreed. Not sure what I was doing there, I'll cut that down. > > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > comment #47) > > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > > comment #45) > > > Created attachment 8791684 [details] > > > screenshot > > This defaulted to my custom 32 bit install directory for installing 64 bit. > > Is that intentional and if so why? > It's intentional. Since we decided on leaving the install dir the same when > we start doing updates to 64-bit, I thought it made sense here as well. Also > I thought detecting the architecture of the existing install sounded hard, > but now I think I could get it out of the registry without too much trouble. The reason we did that for updates is because there is no way to reasonably move the install which is why this was called out specifically for updates. For installs I think we want to install into the 64 bit location.

In other words we would if we reasonably could and I think we should for the installer since it doesn't have the same restrictions as we have with updating.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #49) > The reason we did that for updates is because there is no way to reasonably > move the install which is why this was called out specifically for updates. > For installs I think we want to install into the 64 bit location. So new 64-bit installs should always default to the 64-bit "C:\Program Files" directory instead of trying to pave over an existing 32-bit Firefox directory? My understanding was that we did want new 64-bit installs to pave over existing 32-bit installs by default. That said, I don't have a strong opinion and either approach (overwrite or side-by-side install) seems OK for new installs.

I've only stated that with updating we don't have the ability to move the install directory and that it should just update the 32 bit install location. I would prefer not having it pave over 32 bit installs but I haven't put much thought into it so I'll leave this up to UX and Product.

Two changes in this patch: 1) The excessive padding between the dropdown and the label is gone. 2) If an existing installation is found, its architecture is detected and its location is only used as the default when the same architecture is selected. The directory for an existing 32-bit installation won't be used as the default for a new 64-bit installation.

This version addresses (hopefully) the last two remaining issues, which I had previously missed/misunderstood: 1) Added a bit of additional space above the dropdown, so that large descenders in the checkbox label above it won't run into it. 2) Fixed the excessive padding between the dropdown and its label by more accurately computing the correct width for the label.

Comment on attachment 8794424 [details] [diff] [review] Patch revision 6 Review of attachment 8794424 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/installer/windows/nsis/stub.nsi @@ +324,5 @@ > ; Restrict x64 builds from being installed on x86 and pre Win7 > + StrCpy $SystemSupports64Bit "0" > + StrCpy $DefaultIs64Bit "0" > + ${If} ${RunningX64} > + ${AndIf} ${AtLeastWin7} We want to temporarily omit 64-bit stub installs on Windows 10 until the following Windows 10 bugs are fixed: bug 1240848, bug 1240977, bug 1253261. So we should restrict 64-bit to Windows 7/8/8.1 with logic like ${AtLeastWin7) ${AndIf} ${AtMostWin8.1}. In today's 64-bit meeting, I said I would file a follow-up bug, but it's probably better to omit Windows 10 in this initial patch. Otherwise there is a window of opportunity for some Windows 10 installs to hit those unfixed crash bugs.

Comment on attachment 8794424 [details] [diff] [review] Patch revision 6 This looks much much better. Thank you! The taskbar is typically referred to as 'taskbar' or 'Taskbar' by Microsoft and other sources and not 'task bar'. It would be a good think to get UX to sign off on the positioning changes and the text change. I'll finish the review of the current patch and there is no need to update the patch for this at this time.

Comment on attachment 8794424 [details] [diff] [review] Patch revision 6 Discussed this over irc. New version of the patch coming with the disallow Win10 and other fixes so clearing review. diff --git a/browser/installer/windows/nsis/stub.nsi b/browser/installer/windows/nsis/stub.nsi --- a/browser/installer/windows/nsis/stub.nsi +++ b/browser/installer/windows/nsis/stub.nsi @@ -1365,18 +1450,25 @@ Function createInstall ${NSD_FreeImage} $0 ${NSD_FreeImage} $HwndBitmapBlurb1 ${NSD_FreeImage} $HwndBitmapBlurb2 ${NSD_FreeImage} $HWndBitmapBlurb3 FunctionEnd Function StartDownload ${NSD_KillTimer} StartDownload - InetBgDL::Get "${URLStubDownload}${URLStubDownloadAppend}" "$PLUGINSDIR\download.exe" \ - /CONNECTTIMEOUT 120 /RECEIVETIMEOUT 120 /END + ${If} $DroplistArch == "$(VERSION_64BIT)" $DroplistArch is only set if the options page is shown.

Fixes the above, and excludes Windows 10 from the 64-bit option.

Verdi, per comment 56, can I get your feedback on the slightly modified spacing and the string change ("task bar" => "taskbar")?

Comment on attachment 8794952 [details] [diff] [review] bug797208_r7.diff >@@ -1077,40 +1102,108 @@ Function createOptions > ; we don't ever want to install it on XP without at least SP3 installed. > ${If} $0 == "true" > ${AndIf} ${IsWinXP} > ${AndIf} ${AtMostServicePack} 2 > StrCpy $0 "false" > ${EndIf} > > ; Only show the maintenance service checkbox if we have write access to HKLM >+ StrCpy $CheckboxInstallMaintSvc "0" > ClearErrors > WriteRegStr HKLM "Software\Mozilla" "${BrandShortName}InstallerTest" \ > "Write Test" >- ${If} ${Errors} >- ${OrIf} $0 != "true" >- StrCpy $CheckboxInstallMaintSvc "0" >- ${Else} >+ ${IfNot} ${Errors} >+ ${AndIf} $0 == "true" > DeleteRegValue HKLM "Software\Mozilla" "${BrandShortName}InstallerTest" This makes it so the test regsitry value is no longer removed when $0 == "true" While you are here please quote $0. For string I try to keep it consistent by always quoting so it is obvious that it is performing a string comparison check. One more pass of that change and I will r+ this.

Actually, I may be wrong about the above... taking a closer look

That looks like it should be cleaned up... how about just going with ${IfNot} ${Errors} DeleteRegValue HKLM "Software\Mozilla" "${BrandShortName}InstallerTest" ${If} $0 == "true" etc.

Or if you prefer just delete it after the block and call ClearErrors afterwards.

Minor change: don't create the maintenance service write test registry entry if we already know we can't install the service, and always delete it if it was created.

Comment on attachment 8795013 [details] [diff] [review] Patch revision 8 Looks good. As a safeguard please add the following before the call to ClearErrors in case it already exists in the registry. DeleteRegValue HKLM "Software\Mozilla" "${BrandShortName}InstallerTest"

Comment on attachment 8794953 [details] Options screen from revision 7 patch (In reply to Matt Howell [:mhowell] from comment #59) > Created attachment 8794953 [details] > Options screen from revision 7 patch > > Verdi, per comment 56, can I get your feedback on the slightly modified > spacing and the string change ("task bar" => "taskbar")? Matt this looks great. "taskbar" is definitely the word we want. Bonus points if you can move the text in the select box up, literally 1px, so it's baseline matches the label exactly.

(In reply to Verdi [:verdi] from comment #66) > Matt this looks great. "taskbar" is definitely the word we want. Bonus > points if you can move the text in the select box up, literally 1px, so it's > baseline matches the label exactly. That's the closest I could get unfortunately; it's bugging me too, but doing anything with baselines in this layout proved surprisingly difficult.

Comment on attachment 8795013 [details] [diff] [review] Patch revision 8 Review of attachment 8795013 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/installer/windows/nsis/stub.nsi @@ +339,4 @@ > > + ; Make 64-bit the default if we have 3,800 MB of usable physical memory. > + ; 3800 MB * 1024 MB/KB * 1024 KB/B = 3984588800 Bytes > + ${If} $1 L>= 3984588800 After further discussion, Nick Nethercote now recommends we reduce the default memory requirement from 3,800 MB to 1,900 MB. This will allow nearly all 64-bit Windows users (95+%) to get 64-bit Firefox and reduce OOM crashes.

Adjusted the soft RAM requirement down to 1900 MB. Carrying over r+ because the change is small. Note that 1900 MB is less than Microsoft's requirement for installing 64-bit Windows 7, which is 2 GB. We might be justified in removing our RAM check entirely if it's going to be that low.

(In reply to Matt Howell [:mhowell] from comment #69) > Note that 1900 MB is less than Microsoft's requirement for installing 64-bit > Windows 7, which is 2 GB. We might be justified in removing our RAM check > entirely if it's going to be that low. Interesting. Good catch! It also looks like 2 GB is now the recommendation for both 32- and 64-bit Windows 10. Our telemetry shows that almost 2% of 64-bit Windows 7-10 users have less than 2 GB so, for now, including the 1900 MB is probably a reasonable check. https://www.microsoft.com/en-US/windows/windows-10-specifications#sysreqs

(In reply to Chris Peterson [:cpeterson] from comment #70) > (In reply to Matt Howell [:mhowell] from comment #69) > > Note that 1900 MB is less than Microsoft's requirement for installing 64-bit > > Windows 7, which is 2 GB. We might be justified in removing our RAM check > > entirely if it's going to be that low. > > Interesting. Good catch! It also looks like 2 GB is now the recommendation > for both 32- and 64-bit Windows 10. Our telemetry shows that almost 2% of > 64-bit Windows 7-10 users have less than 2 GB so, for now, including the > 1900 MB is probably a reasonable check. Matt, after further discussion with Nick Nethercote, he thinks we should probably just remove the minimum memory requirement. As you originally pointed out, 64-bit Windows already has a 2 GB memory requirement. We would just be adding special case code to the stub installer to handle a use case unsupported (by Microsoft) for less than 2% of our users. Sorry for the changing recommendations! I imagine removing the memory checks will be a welcome simplification. :)

Removed RAM check, per comment 71. You were right, that was a nice simplification. We now simply default to 64-bit on all supported 64-bit Windows versions (which still excludes Win10). That means there's no difference anymore between the states "supports 64-bit" and "defaults to 64-bit," which meant I could remove a bit of code that dealt with that, and that I could roll back the ping to v7 because it no longer had a new variable it needed to send.

[Test Plan]: https://wiki.mozilla.org/QA/x64StubInstaller

aklotz has fixed Windows 10 bug 1240848 and bug 1240977 so Matt will remove the Windows 10 exception from the stub installer.

As advertised, this revision removes the Windows 10 exception. No other changes.

Release Note Request (optional, but appreciated) [Why is this notable]: Big change [Suggested wording]: 64 bit Firefox installed by default in the stub installer for Windows 7+ users on 64 bit OS (Romain proposed it) [Links (documentation, blog post, etc)]:

This is a stub installer built using patch 11 (attachment 8801161 [details] [diff] [review]) on top of the current mozilla-central (https://hg.mozilla.org/mozilla-central/rev/9888f1a23001).

SV requested this build of the stub installer with release branding instead of the default nightly branding for their testing.

QA on the patch is taking longer than expected and it will end up needing to be uplifted to aurora. But there are new strings being introduced, and we don't want to drop those on L10N in an uplift. Therefore, this patch does nothing but add in the strings without using them anywhere or making any other changes. Requesting review even though the strings themselves already have it just because this is kind of an odd thing to be doing.

Comment on attachment 8809502 [details] [diff] [review] "Part 0" (strings only) patch Not a problem... I've had to do this several times in the past. Please give Axel a heads up about this just in case.

-> flod, who's handling these now. Testing installer l10n is hard enough as it is, do we really need to blend this with pre-landed strings?

(In reply to Axel Hecht [:Pike] from comment #81) > -> flod, who's handling these now. > > Testing installer l10n is hard enough as it is, do we really need to blend > this with pre-landed strings? comment #79 provides details from the developer perspective. Project managers should be asked regarding the schedule which is driving this. From my perspective, there are several locales seem to only get stub installer testing due to it landing and riding the trains which btw I'm fine with.

To mitigate l10n issues, either Matt or I can go through the strings and verify that they are at least present and appear to be translated as I have done in the past. https://bugzilla.mozilla.org/buglist.cgi?list_id=13304658&status_whiteboard_type=anywords&query_format=advanced&status_whiteboard=%5Bstublocale%5D%20%5Bstublocalev3%5D

We don't plan to enable this 64-bit stub installer change in Release channel users until 53, but we would like to run a limited Funnelcake experiment in Release 52. Would limiting the Funnelcake experiment to English locales for 52 make l10n easier? Alternately, we could land all the 64-bit stub installer changes in Nightly 52. Matt's patches have been code-complete and r+'d for three weeks; we've just been blocked waiting for Softvision test sign-off and they ran into an issue with their corporate firewall that affects all Firefox pre-release stub installers, not just Matt's private build. What do you all recommend? I'm leaning towards just landing all the code now and letting Softvision to sign-off post-landing. We're balancing the process overhead/headaches for the l10n team (and Matt and Robert) versus the risk of Nightly users running into a stub installer regression. Uplifting the install code changes later also means we get less bake time on Nightly and Aurora.

I would go with landing it all now. We often do that with new features on Nightly especially since the dev will have verified that it works as expected though there may be edge cases that the dev didn't catch. I think it would be a good thing to make sure the strings have been added to l10n when it reaches the beta cycle since I have found several stub installer strings previously that weren't translated.

I'm reading the bug but I'm a bit confused. Is attachment 8809502 [details] [diff] [review] supposed to land in mozilla-central before Merge Day, as pre-landand strings for Firefox 52, and ride the trains? If that's the case, given how long the cycle is going to be, I don't think it's a problem, and I prefer it to uplifting strings in aurora later in the cycle. Do we have any estimate of when the code changes would land in aurora, to make testing possible?

Looks like we have an issue with a particular firewall configuration for the 64-bit stub installer. Andrei, can you file a new dependent bug with the details? Thanks!

Tracking for 52 so we can keep an eye on the issues.

(In reply to Francesco Lodolo [:flod] from comment #86) > I'm reading the bug but I'm a bit confused. > > Is attachment 8809502 [details] [diff] [review] supposed to land in > mozilla-central before Merge Day, as pre-landand strings for Firefox 52, and > ride the trains? > If that's the case, given how long the cycle is going to be, I don't think > it's a problem, and I prefer it to uplifting strings in aurora later in the > cycle. Yes, that's the idea. > Do we have any estimate of when the code changes would land in aurora, to > make testing possible? Unfortunately nothing solid; it's dependent on getting QA sign-off, and there are some outstanding issues. This is a high priority for me, though.

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #87) > Looks like we have an issue with a particular firewall configuration for the > 64-bit stub installer. Andrei, can you file a new dependent bug with the > details? Thanks! Done, filed as 1317281.

Comment on attachment 8809502 [details] [diff] [review] "Part 0" (strings only) patch If I understand correctly, we're no longer planning on landing this work in 52; the testing that needs to be done of 64-bit support in general can use full installers. That means we can push the whole deal back to the 53 train without having to uplift or preload anything. Sorry for the false alarm, everyone.

This revision fixes issues found in QA with the feature of detecting an existing install and using its location as the default for the new install; the detection was working, but the logic for when to actually use the existing path was busted.

New testing build made from the above revised patch.

Rebased on top of bug 1305453. That allowed for a minor simplification, because now the OS requirement for 64-bit is the same as for 32-bit (at least Win7). No other changes in this revision. QA testing has concluded with the only issues found being the ones fixed in revision 12, and a set of non-blocking cosmetic issues filed as bug 1319192, bug 1319202, bug 1319479, and bug 1319482. This, then, should be the revision that finally lands.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8638691b8a7a641b6d40eb4f59c95920c38b188a Bug 797208 - Allow the stub installer to install either 32-bit or 64-bit builds, and make 64-bit the default when supported; r=rstrong

It turns out we don't actually need this for 52. Funnelcake testing of 64-bit builds is planned for 52, but that will use full installers.

Doesn't sound like we need a release note here, though we should note when we ship this turned on by default in 54 or 55.

the firefox 53 stub installer doesn't seem to automatically select 64-bit -- tried in 64-bit Windows 10 ... when i clicked the "Options" button, it showed 32-bit selected by default...?

We're intentionally holding off on making 64-bit the default until 55 because of a couple of outstanding issues. See bug 1342347.

thanks i really wish reversions were noted in the original bug -- it all gets hard to keep track of

Hello everyone, it is a pit nitpicking but I do not see the request being solved Stub installer should >automatically< select 32-bit or 64-bit Firefox at install-time. I am happy to see that the Stub Installer now got the ability to either install the 32bit or 64bit version of Firefox, however, at the same time I don't understand why you don't just do the following, to meet the description of the submitted case, so to let the stub installer automatically choose the correct installation. all that is now missing to comply with this request is: - determine whether the OS is 64bit OS - recommended: determine previous installations of Firefox 32bit and and offer the user to change to 64bit Firefox if he accepts this will uninstall FF 32bit, and cause the stub installer to download the 64bit installer. If the user declines to change to 64bit Firefox, the stub installer shall continue to download a 32bit installer. At the same time I would like to reprise this request: https://bugzilla.mozilla.org/show_bug.cgi?id=1272745 as it is quite related this original idea here, to increase ease of use. Thank you very much for all your efforts so far and for considering this change request.

This question has been asked and answered already, including in the immediately preceding comments on this very bug. The option exists to manually select 64-bit as of Firefox 53. The *automatic* selection of 64-bit was delayed until Firefox 55 *after* the code was written and landed (in this bug) because of issues involving Flash that were found later. The current implementation deliberately avoids disturbing an existing installation of a different architecture. Your suggestion of offering to replace an existing installation would have to be filed and discussed as a separate bug. I don't think anyone following this bug would be involved in fixing bug 1272745. I'll comment over there though, because I think it still isn't in the correct component.

Thanks for your quick response. The irritating part on this is that FF got some press coverage where they reported that the stub installer would automatically select the correct installer, while it does not at the moment but will by default install a 32bit Firefox. I have double-checked that this morning. As you say it was delayed for FF 55, I wonder why the press reports differently.

I'm not sure either; the release notes [https://www.mozilla.org/en-US/firefox/53.0/releasenotes/] are correct.