1229626 - Firefox is not pinned to the taskbar when using the stub installer and not making it the default browser

Status: VERIFIED FIXED

(Firefox :: Installer, defect, P3)

Description

[Verdi [:verdi] Best to slack me]

On Windows 7 and 8.1, if you install Firefox with the Stub Installer, leaving the option for pinning Firefox to the taskbar checked AND:
 * Uncheck make Firefox my default browser on Windows 7
 * Dismiss modal default browser prompt or fail to set Firefox as default in Windows 8.1
THEN, Firefox will not be pinned to the taskbar.

More info in Bug 1190351

Comment 1

[:Gijs (out for now; he/him)]

Isn't this a dupe of bug 900635?

Comment 2

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #1)
> Isn't this a dupe of bug 900635?

Re-needinfo'ing as I think this got cleared accidentally.

Comment 3

[Verdi [:verdi] Best to slack me]

I don't think so? In https://bugzilla.mozilla.org/show_bug.cgi?id=900635#c0 the reporter seems to say Firefox is always pinned to the taskbar. I'm saying that in the case where you don't make Firefox the default browser, your preference to have it pinned to the taskbar is ignored.

Comment 4

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

I'm quite certain it is the same root cause.

Comment 5

[:Gijs (out for now; he/him)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #4)
> I'm quite certain it is the same root cause.

Is this just happening because this section of the nsis code:

https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/browser/installer/windows/nsis/shared.nsh#1124-1144

checks that we're the default browser and no-ops otherwise?

Likewise, if I'm reading https://dxr.mozilla.org/mozilla-central/source/browser/installer/windows/nsis/stub.nsi right, we're using "CheckboxShortcutOnBar" for the checkbox. We do two things with that value:

1) This code turns off the full installer pinning something if you untick the checkbox:

https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/browser/installer/windows/nsis/stub.nsi#1593-1596

2) This code installs a quick launch icon:

https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/browser/installer/windows/nsis/stub.nsi#1731-1744


so if you don't make Firefox the default, and keep the icon checked, we "fall back" on the real installer and, if we're not the default browser, we don't pin to taskbar (see first link).

Is that right / am I making sense of this correctly?

Likewise, it then seems that this section of code:

https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/browser/installer/windows/nsis/shared.nsh#1438-1490

pins to taskbar if you've made Firefox the default, even if you untick the box in the stub installer, because it runs PinToTaskbar directly, rather than going through MigrateTaskBarShortcut, thus bypassing (1) mentioned above.

Comment 6

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

iirc it is happening because we use the full installer to pin instead of the stub in part to keep the stub small and in part due to the option to pin being part of setting the default browser in the full installer (e.g. is it already default, are we setting as default, etc.).

Comment 7

[:Gijs (out for now; he/him)]

Attachment: strawman patch, f?rstrong

Comment 8

[:Gijs (out for now; he/him)]

FWIW, I don't know how to test this. If I run the stub installer, how does it determine where to get the full installer and/or how do I ensure it runs the one I compiled?

Comment 9

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

Comment on attachment 8715064 [details] [diff] [review]
strawman patch, f?rstrong

The simplest way to change the url for the download is to change ${URLStubDownload} to point to an url for the modified full installer.

Instead of using shortcuts.ini this should use the configuration ini specified on the command line... examples

http://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/nsis/installer.nsi#207

and

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/windows/nsis/common.nsh#5170

Comment 10

[:Gijs (out for now; he/him)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #9)
> Comment on attachment 8715064 [details] [diff] [review]
> strawman patch, f?rstrong
> 
> The simplest way to change the url for the download is to change
> ${URLStubDownload} to point to an url for the modified full installer.

I have tried to do this but failed. I'm assuming I just want the full URL to "firefox-47.0a1.en-US.win32.installer.exe". Does the server need to support range requests or some other fancy thing, or something? I tried to use file:/// URLs initially but that failed. But using Mongoose and http URLs also fails ("Your download was interrupted. Please click the OK button to continue."). Is there a different way I can test this?

Comment 11

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #10)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #9)
> > Comment on attachment 8715064 [details] [diff] [review]
> > strawman patch, f?rstrong
> > 
> > The simplest way to change the url for the download is to change
> > ${URLStubDownload} to point to an url for the modified full installer.
> 
> I have tried to do this but failed. I'm assuming I just want the full URL to
> "firefox-47.0a1.en-US.win32.installer.exe". Does the server need to support
> range requests or some other fancy thing, or something? I tried to use
> file:/// URLs initially but that failed. But using Mongoose and http URLs
> also fails ("Your download was interrupted. Please click the OK button to
> continue."). Is there a different way I can test this?

Just tried with apache on my mac, seeing the same thing. Looks like this might be the cert verification step that is failing. Just going to comment that out and see if that helps... :-\

Comment 12

[:Gijs (out for now; he/him)]

Attachment: MozReview Request: Bug 1229626 - fix pinning to taskbar to reflect user preference, r?rstrong

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

Comment 13

[:Gijs (out for now; he/him)]

https://reviewboard.mozilla.org/r/33749/#review30443

I tested all of this on Win7, running:

the stub with default browser + taskbar
the stub without default browser and without taskbar
the stub with default browser and without taskbar
the stub without default browser and with taskbar

the full installer with default browser
the full installer without default browser

and verified that the options were respected.

::: browser/installer/windows/nsis/installer.nsi
(Diff revision 1)
> -    ; Adds a pinned Task Bar shortcut (see MigrateTaskBarShortcut for details).
> -    ${MigrateTaskBarShortcut}
>    ${EndUnless}
>  
> -  ${GetShortcutsLogPath} $0
> -  WriteIniStr "$0" "TASKBAR" "Migrated" "true"
> +  ; Adds a pinned Task Bar shortcut (see MigrateTaskBarShortcut for details).
> +  ${MigrateTaskBarShortcut}

Note that this code wasn't currently being run at all in the stub installer case, because it invokes the full installer with a config file, for which we `SetSilent silent`. So I moved it out of the 'Unless' block.

Comment 14

[:Gijs (out for now; he/him)]

Benjamin, can you suggest an alternative reviewer given that rstrong is on leave?

Comment 15

[Benjamin Smedberg]

Comment on attachment 8716243 [details]
MozReview Request: Bug 1229626 - fix pinning to taskbar to reflect user preference, r?rstrong

Matt, do you feel comfortable reviewing this?

Comment 16

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

Yeah, looks like I can handle this review. Can't promise I can get to it today, but I should be able to tomorrow if not.

Comment 17

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

https://reviewboard.mozilla.org/r/33749/#review30443

Would you be able to test on Windows 8.1 as well? It's also mentioned in the bug.

Comment 18

[:Gijs (out for now; he/him)]

(In reply to Matt Howell [:mhowell] from comment #17)
> https://reviewboard.mozilla.org/r/33749/#review30443
> 
> Would you be able to test on Windows 8.1 as well? It's also mentioned in the
> bug.

I can, from Friday on, when I have access to a secondary machine running 8.1. Removing all Firefox installs from my main machine (which runs 8.1) for testing isn't very practical.

Did you mean to finish a review?

Comment 19

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

(In reply to :Gijs Kruitbosch from comment #18)

Don't worry about it then, I'll do it; I have a VM I can use.

I'll finish the review once that's done.

Comment 20

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

https://reviewboard.mozilla.org/r/33749/#review30443

I tested the changes on Windows 8.1 using the same scenarios, except that the set as default option is never offered on 8.1, and observed the expected behavior in all cases.

Comment 21

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

Comment on attachment 8716243 [details]
MozReview Request: Bug 1229626 - fix pinning to taskbar to reflect user preference, r?rstrong

https://reviewboard.mozilla.org/r/33749/#review31947

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9ceeaac229de

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9ceeaac229de

Comment 24

[Cornel Ionce [:noni] [Hubs QA]]

Verified that Firefox correctly pinned to the taskbar on Windows 7 x64 and Windows 8.1 x64 using the scenarios from comment 13.
Testing was performed using Fx 47.0b4, build ID: 20160509171155.