Closed Bug 1229626 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Installer, defect, P3)

x86
Windows 7
defect

Tracking

()

VERIFIED FIXED
Firefox 47
Tracking Status
firefox47 --- verified

People

(Reporter: verdi, Assigned: Gijs)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [Onboarding])

Attachments

(2 files)

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
See Also: → 900635
Whiteboard: [DUPEME] → [DUPEME][Onboarding]
Isn't this a dupe of bug 900635?
Flags: needinfo?(mverdi)
Flags: needinfo?(mverdi)
Priority: -- → P3
(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.
Flags: needinfo?(mverdi)
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.
Flags: needinfo?(mverdi)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
See Also: → 621873
I'm quite certain it is the same root cause.
Depends on: 900635
(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.
Flags: needinfo?(robert.strong.bugs)
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.).
Flags: needinfo?(robert.strong.bugs)
Attachment #8715064 - Flags: feedback?(robert.strong.bugs)
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?
Flags: needinfo?(robert.strong.bugs)
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
Flags: needinfo?(robert.strong.bugs)
Attachment #8715064 - Flags: feedback?(robert.strong.bugs) → feedback-
(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?
Flags: needinfo?(robert.strong.bugs)
(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... :-\
Flags: needinfo?(robert.strong.bugs)
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.
Benjamin, can you suggest an alternative reviewer given that rstrong is on leave?
Flags: needinfo?(benjamin)
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?
Flags: needinfo?(benjamin)
Attachment #8716243 - Flags: review?(mhowell)
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.
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.
(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?
Flags: needinfo?(mhowell)
(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.
Flags: needinfo?(mhowell)
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 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
Attachment #8716243 - Flags: review?(mhowell) → review+
Attachment #8716243 - Flags: review?(robert.strong.bugs)
https://hg.mozilla.org/mozilla-central/rev/9ceeaac229de
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Contact: cornel.ionce
Whiteboard: [DUPEME][Onboarding] → [Onboarding]
You need to log in before you can comment on or make changes to this bug.