621873 - Pin to taskbar when setting as default browser on Windows 7

Status: RESOLVED FIXED

(Firefox :: Installer, defect)

Description

[Siddhartha Dugar [:sdrocking]]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0b9pre) Gecko/20101228 Firefox/4.0b9pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0b9pre) Gecko/20101228 Firefox/4.0b9pre

By default there is no quick launch bar in windows 7. And few users care to get it back. So i suggest this option be unselected when installing on Windows 7.

Reproducible: Always

Comment 1

[Siddhartha Dugar [:sdrocking]]

Pinning to taskbar by default would promote usage.

Comment 2

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

This functionality has nothing to do with panorama... removing dependency

Comment 3

[Siddhartha Dugar [:sdrocking]]

(In reply to comment #2)
> This functionality has nothing to do with panorama... removing dependency

I did this after approval:
https://bugzilla.mozilla.org/show_bug.cgi?id=603789#c7

Comment 4

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

I highly suspect that he assumed you had read the summary for bug 603789 which is "[Meta] Panorama Features and Fixes We Want "Soon" after Firefox 4". What ever the case, non Panorama bugs do not have "approval" to be added as a dependency of that bug unless Panorama requires them for some reason and this is definitely not needed by Panorama.

We don't use meta bugs for requesting blocking and haven't added the blocking flags yet. Be patient and wait until the blocking flags have been added at which time you can request blocking for this bug. The flags may very well not be added until after we ship Firefox 4.0 so we can focus on shipping Firefox 4.0.

Comment 5

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

Changing summary to something I might be able to implement for Firefox 4 since we are way past string changes. Filed bug 630821 to provide an option to pin to taskbar during install.

Comment 6

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

Attachment: source for NSIS invoke shell plugin

Jim, this is the source for a new NSIS plugin that invokes a shell verb and uses the localized verb name to find the verb to invoke. I'll upload the source to the NSIS site after I finish documenting it.

Comment 7

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

Attachment: patch in progress - almost there

Found and fixed a few bugs while writing the patch

Comment 8

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

Attachment: 1. patch binary plugin

Comment 9

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

Attachment: InvokeShellVerb.cpp from plugin

This is the NSIS plugin c++ and isn't to be checked in. I'll attach all of the files for the plugin after this.

Comment 10

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

Attachment: Zipped NSIS Plugin files

Still haven't finished the plugin documentation... after I do I'll upload this to the NSIS site.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 509846 [details] [diff] [review]
InvokeShellVerb.cpp from plugin

Looks fine to me, though I'm not a fan of the error handling; it seems like all of these:

      if (FAILED(hr))
      {
        pushstring(OUT_ERR_CALL_FAILED);
        pItem->Release();
        pSD->Release();
        CoFreeUnusedLibraries();
        CoUninitialize();
        return;
      }

could be replaced with a 

      if (FAILED(hr)) {
        err = OUT_ERR_CALL_FAILED;
        goto bail;
      }
..
bail:
      if (err)
        pushstring(err);
      if (pVerbs) pVerbs->Release();
      if (pItem) pItem->Release();
      if (pFolder) pFolder->Release();
      if (pSD) pSD->Release();
      CoFreeUnusedLibraries();
      CoUninitialize();
      return;

But up to you whether you want to fix it, ideally we'll never need to touch this again.

Comment 12

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

Attachment: 2. main patch rev1

Comment 13

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

Comment on attachment 509859 [details] [diff] [review]
2. main patch rev1

Some notes to help with the review

>diff --git a/browser/installer/windows/nsis/installer.nsi b/browser/installer/windows/nsis/installer.nsi
>--- a/browser/installer/windows/nsis/installer.nsi
>+++ b/browser/installer/windows/nsis/installer.nsi
>@@ -387,32 +394,53 @@ Section "-Application" APP_IDX
> 
>   ; Always add the application's shortcuts to the shortcuts log ini file. The
>   ; DeleteShortcuts macro will do the right thing on uninstall if the
>   ; shortcuts don't exist.
>   ${LogStartMenuShortcut} "${BrandFullName}.lnk"
>   ${LogQuickLaunchShortcut} "${BrandFullName}.lnk"
>   ${LogDesktopShortcut} "${BrandFullName}.lnk"
> 
>+  ; Best effort to update the Win7 taskbar and start menu shortcut app model
>+  ; id's. The possible contexts are current user / system and the user that
>+  ; elevated the installer.
>+  Call FixShortcutAppModelIDs
>+  ; If the current context is all also perform Win7 taskbar and start menu link
>+  ; maintenance for the current user context.
>+  ${If} $TmpVal == "HKLM"
>+    SetShellVarContext current  ; Set SHCTX to HKCU
>+    Call FixShortcutAppModelIDs
>+    SetShellVarContext all  ; Set SHCTX to HKLM
>+  ${EndIf}
>+
>+  ; If running elevated also perform Win7 taskbar and start menu link
>+  ; maintenance for the unelevated user context in case that is different than
>+  ; the current user.
>+  ClearErrors
>+  ${GetParameters} $0
>+  ${GetOptions} "$0" "/UAC:" $0
>+  ${Unless} ${Errors}
>+    GetFunctionAddress $0 FixShortcutAppModelIDs
>+    UAC::ExecCodeSegment $0
>+  ${EndIf}
Changed so that we update shortcuts app model ID in both the user and the system locations as well as the original user's location in case the user installed with a different user account


>   ; UAC only allows elevating to an Admin account so there is no need to add
>   ; the Start Menu or Desktop shortcuts from the original unelevated process
>   ; since this will either add it for the user if unelevated or All Users if
>   ; elevated.
>   ${If} $AddStartMenuSC == 1
>-    CreateShortCut "$SMPROGRAMS\${BrandFullName}.lnk" "$INSTDIR\${FileMainEXE}" \
>-                   "" "$INSTDIR\${FileMainEXE}" 0
>+    CreateShortCut "$SMPROGRAMS\${BrandFullName}.lnk" "$INSTDIR\${FileMainEXE}"
Turns out that specifying the icon breaks "Change Icon" in that it is unable to find the executable... this will just default to the first icon which is what we want anyways.


>   ; Refresh desktop icons
>-  System::Call "shell32::SHChangeNotify(i, i, i, i) v (0x08000000, 0, 0, 0)"
>+  System::Call "shell32::SHChangeNotify(i 0x08000000, i 0, i 0, i 0)"
Updated to what the latest NSIS uses.

>diff --git a/browser/installer/windows/nsis/shared.nsh b/browser/installer/windows/nsis/shared.nsh
>--- a/browser/installer/windows/nsis/shared.nsh
>+++ b/browser/installer/windows/nsis/shared.nsh
>@@ -39,31 +39,36 @@
> 
>   ; Remove registry entries for non-existent apps and for apps that point to our
>   ; install location in the Software\Mozilla key and uninstall registry entries
>   ; that point to our install location for both HKCU and HKLM.
>   SetShellVarContext current  ; Set SHCTX to the current user (e.g. HKCU)
>   ${RegCleanMain} "Software\Mozilla"
>   ${RegCleanUninstall}
>   ${UpdateProtocolHandlers}
>+  ; Win7 taskbar and start menu link maintenance
>+  Call FixShortcutAppModelIDs
> 
>   ClearErrors
>   WriteRegStr HKLM "Software\Mozilla" "${BrandShortName}InstallerTest" "Write Test"
>   ${If} ${Errors}
>     StrCpy $TmpVal "HKCU" ; used primarily for logging
>   ${Else}
>     SetShellVarContext all    ; Set SHCTX to all users (e.g. HKLM)
>     DeleteRegValue HKLM "Software\Mozilla" "${BrandShortName}InstallerTest"
>     StrCpy $TmpVal "HKLM" ; used primarily for logging
>     ${RegCleanMain} "Software\Mozilla"
>     ${RegCleanUninstall}
>     ${UpdateProtocolHandlers}
>     ${FixShellIconHandler}
>     ${SetAppLSPCategories} ${LSP_CATEGORIES}
> 
>+    ; Win7 taskbar and start menu link maintenance
>+    Call FixShortcutAppModelIDs
Changed so that we update shortcuts app model ID in both the user and the system locations.

>@@ -175,58 +181,71 @@
> ; to Open With for the file types the application handles (bug 370480).
> !macro ShowShortcuts
>   ${StrFilter} "${FileMainEXE}" "+" "" "" $0
>   StrCpy $R1 "Software\Clients\StartMenuInternet\$0\InstallInfo"
>   WriteRegDWORD HKLM "$R1" "IconsVisible" 1
> 
>   SetShellVarContext all  ; Set $DESKTOP to All Users
>   ${Unless} ${FileExists} "$DESKTOP\${BrandFullName}.lnk"
>-    CreateShortCut "$DESKTOP\${BrandFullName}.lnk" "$INSTDIR\${FileMainEXE}" \
>-                   "" "$INSTDIR\${FileMainEXE}" 0
>-    ShellLink::SetShortCutWorkingDirectory "$DESKTOP\${BrandFullName}.lnk" "$INSTDIR"
>-    ApplicationID::Set "$DESKTOP\${BrandFullName}.lnk" "${AppUserModelID}"
>-    ${Unless} ${FileExists} "$DESKTOP\${BrandFullName}.lnk"
>+    CreateShortCut "$DESKTOP\${BrandFullName}.lnk" "$INSTDIR\${FileMainEXE}"
>+    ${If} ${FileExists} "$DESKTOP\${BrandFullName}.lnk"
>+      ShellLink::SetShortCutWorkingDirectory "$DESKTOP\${BrandFullName}.lnk" "$INSTDIR"
>+      ${If} ${AtLeastWin7}
>+        ApplicationID::Set "$DESKTOP\${BrandFullName}.lnk" "${AppUserModelID}"
>+      ${EndIf}
>+    ${Else}
Changed so it doesn't try to perform the additional operations on shortcuts when creating the shortcut fails.

>diff --git a/toolkit/mozapps/installer/windows/nsis/common.nsh b/toolkit/mozapps/installer/windows/nsis/common.nsh
>--- a/toolkit/mozapps/installer/windows/nsis/common.nsh
>+++ b/toolkit/mozapps/installer/windows/nsis/common.nsh

>...
> !macro DeleteShortcuts
DeleteShortcuts assumed that shortcuts were available to unpin and now it just uses the pinned shortcuts to unpin. It also was using $STARTMENU when it should have been using $SMPROGRAMS for shortcuts in the root of the start menu programs directory.

>...
> !macro UpdateShortcutAppModelIDs
Updated to use the shortcuts_log.ini instead of hardcoding the Start Menu Programs shortcuts and updates all shortcuts in the log

Comment 14

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

Comment on attachment 509843 [details] [diff] [review]
1. patch binary plugin

Nothing to be reviewed here since it is a binary but I'll need r+ to check this in

Comment 15

[Johnathan Nightingale [:johnath]]

Talked this through in triage, we still don't think it's a blocker. I want to give approval, but can't until we have a reviewed patch. Jump up and down when it's ready, but it doesn't block.

Comment 16

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

Attachment: 3. additional cleanup

Comment 17

[Reed Loden [:reed]]

For some reason, I want to say that for licensing reasons, we need to include the source for the .dll file in the tree somewhere (even if it doesn't get directly compiled). Gerv, is that correct?

Also, what license is the .dll under? There's not a license header in the .cpp file. If this is being submitted upstream, shouldn't the author be listed as Mozilla Foundation rather than you personally?

Comment 18

[Reed Loden [:reed]]

(In reply to comment #17)
> For some reason, I want to say that for licensing reasons, we need to include
> the source for the .dll file in the tree somewhere (even if it doesn't get
> directly compiled). Gerv, is that correct?

other-licenses/nsis/Contrib/ has the source code to some (but not all) of the other included .dlls in other-licenses/nsis/Plugins/. Should just put the .cpp under Contrib/, as well as get another bug filed on getting the source added for all the other .dlls.

Comment 19

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

We already went through the process of getting approval from Gerv for 3rd party dll's in the tree for the installer... as a matter of fact we did this before Firefox 1.0 in order to use the 7-zip self extracting archive. Previously I created the Application Association Registration NSIS plugin (AppAssocReg.dll) with a compatible license and published the source on the NSIS site as the other dll's have done as well which is what I am going to do in this instance. When we don't have to make changes and can use the precompiled dll we do so without adding the source as in this instance. In the instances where we need to modify the source we then check in the source to the tree along with info as to what had to be changed and the associated bug numbers.

Cool?

Comment 20

[Gervase Markham [:gerv]]

If you are submitting this upstream under an open source license, and they are maintaining it, I agree there's no need to have the source in our tree too.

All of this NSIS stuff is open source _somewhere_, right? :-)

Gerv

Comment 21

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

(In reply to comment #17)
>...
> Also, what license is the .dll under? There's not a license header in the .cpp
> file. If this is being submitted upstream, shouldn't the author be listed as
> Mozilla Foundation rather than you personally?
It will be licensed as zlib/libpng.
http://mxr.mozilla.org/mozilla-central/source/other-licenses/nsis/Contrib/README?force=1#7

NSIS doesn't do things the way we do things hence why it isn't in the .cpp.

I've taken care that the work I've done on NSIS plugins is outside of my work on Mozilla even though this work benefits Mozilla. This way this work can be licensed as zlib/libpng and all consumers of NSIS don't have to worry about whether the license on the plugins I create which would be different than the vast majority of NSIS plugins will be a problem for them.

(In reply to comment #20)
> If you are submitting this upstream under an open source license, and they are
> maintaining it, I agree there's no need to have the source in our tree too.
It will be available on the NSIS site in the same manner as all non-standard NSIS plugins. I sent links to some of these plugins before to you to get your approval the first time we added them and here is one example:
http://nsis.sourceforge.net/Application_Association_Registration_plug-in

> All of this NSIS stuff is open source _somewhere_, right? :-)
I suspect this is a rhetorical but in case it isn't the answer is yes.

Comment 22

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

One case this doesn't handle is adding the taskbar shortcut when installing on top of an existing install that is already the default browser. I can either do this in this bug or a followup bug.

Comment 23

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

Attachment: 2. patch rev2

Fixed so that the pinned taskbar shortcut is added when installing on top of an existing install that is already the default browser along with fixing the detection (it was checking at the system level vs. the user level). Also added the additional cleanup to this patch.

Comment 24

[Gervase Markham [:gerv]]

rstrong: it was a rhetorical question :-) However, you shouldn't need to keep some sort of Chinese wall between your work for Mozilla and your work on NSIS. Mozilla standard policy is to contribute to upstream projects under the license those projects use (unless it conflicts with ours, which zlib/libpng doesn't).

Gerv

Comment 25

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

Sounds good and I was likely just being extra careful just to avoid having to spend time discussing these types of issues when I could be spending that time on other things.

Comment 26

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

Jim, in case it isn't clear it only adds the pinned taskbar shortcut on install / app update for one user and only one time which is intentional. This prevents us from accidentally adding it back when a user chooses to remove the pinned taskbar shortcut. When Firefox is set as the default browser it will always try to pin Firefox to the taskbar.

Comment 27

[Jim Mathies [:jimm]]

Comment on attachment 510096 [details] [diff] [review]
2. patch rev2

This is working better. Would you mind firing off a try build so we can do some final testing before this lands?

Comment 28

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

Submitted to try
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-24c8aefca55f

Comment 29

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

Builds are available at 
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-24c8aefca55f/tryserver-win32/

Comment 30

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

Comment on attachment 509843 [details] [diff] [review]
1. patch binary plugin

I'm confident that the patches as they are written is a good thing to get landed so requesting a=?

Comment 31

[Johnathan Nightingale [:johnath]]

Comment on attachment 510096 [details] [diff] [review]
2. patch rev2

Talked this over with rel-drivers, got a chorus of yes to amplify my own sense.

You're so so so on the hook if this breaks, but *thank you* for doing it.

Comment 32

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

The hook and I are great friends! ;)

and thanks for the approval... tis' the best!

Comment 33

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

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/57cb22e845d4
http://hg.mozilla.org/mozilla-central/rev/9bdc5192bd76

The plugin is available at
http://nsis.sourceforge.net/Invoke_Shell_Verb_plugin

I recompiled it with VC6 to lessen the dependencies / file size and tested it on Win7 / Vista / Win2K.

Comment 34

[Dan]

If I'm understanding this correctly, when the user has Firefox 4 as their default browser, they get an auto-pinned shortcut on their taskbar... and it's separate from the option most installers give for a Quick Launch shortcut (that is, it's merged with the "default browser" option).

I think this is a bad idea, and Raymond Chen says some stuff about user preferences and applications better than I could:

http://blogs.msdn.com/b/oldnewthing/archive/2009/02/02/9388941.aspx
http://blogs.msdn.com/b/oldnewthing/archive/2006/06/19/636823.aspx
http://blogs.msdn.com/b/oldnewthing/archive/2006/11/01/922449.aspx

I wouldn't mind if auto-pinning was linked to the "Quick Launch" checkbox (or a renamed variant) so it acted similarly to classic "Quick Launch" checkboxes in installers.  But it seems to me we shouldn't pin Firefox for the user without telling them; they can always do it themselves if they want to.

Just my 2c.

Comment 35

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

I agree wholeheartedly that it is a bad idea but I also believe wholeheartedly that it is a worse idea to expect users to figure out how to pin their default browser (mail as well) and I know many people that haven't bothered to learn how to especially since they didn't need to learn how to when it was in the top left of the Start Menu. Fact of the matter is that it was decided that the OS provided browser should be pinned by default and I do think it is correct to pin a browser but it should be the browser the user has chosen with an OS provided option not to show the default browser.

https://blog.mozilla.com/rstrong/2011/02/09/firefox-auto-pinned-to-the-win7-task-bar/

Comment 36

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

btw: I should have just stated that further discussion should be in the dev-apps-firefox newsgroup / maillist.