Closed Bug 621873 Opened 13 years ago Closed 13 years ago

Pin to taskbar when setting as default browser on Windows 7

Categories

(Firefox :: Installer, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 4.0b12
Tracking Status
blocking2.0 --- -

People

(Reporter: sdrocking, Assigned: robert.strong.bugs)

References

(Blocks 1 open bug, )

Details

Attachments

(4 files, 4 obsolete files)

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
Component: Shell Integration → Installer
QA Contact: shell.integration → installer
Pinning to taskbar by default would promote usage.
blocking2.0: --- → ?
Summary: Remove/disable "Shortcut in Quick Launch bar" when installing on Windows 7 → Replace "Shortcut in Quick Launch bar" by "Pin to taskbar" when installing on Windows 7
blocking2.0: ? → -
Blocks: 603789
This functionality has nothing to do with panorama... removing dependency
No longer blocks: 603789
(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
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
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.
Blocks: 630821
Summary: Replace "Shortcut in Quick Launch bar" by "Pin to taskbar" when installing on Windows 7 → Pin to taskbar when setting as default browser on Windows 7
Severity: minor → normal
Attached file source for NSIS invoke shell plugin (obsolete) —
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.
Attachment #509258 - Flags: review?(jmathies)
Attached patch patch in progress - almost there (obsolete) — Splinter Review
Found and fixed a few bugs while writing the patch
Attachment #509258 - Attachment is obsolete: true
Attachment #509258 - Flags: review?(jmathies)
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.
Attachment #509846 - Flags: review?(vladimir)
Still haven't finished the plugin documentation... after I do I'll upload this to the NSIS site.
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.
Attachment #509846 - Flags: review?(vladimir) → review+
Attached patch 2. main patch rev1 (obsolete) — Splinter Review
Attachment #509730 - Attachment is obsolete: true
Attachment #509859 - Flags: review?(jmathies)
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 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
Attachment #509843 - Flags: review?(jmathies)
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.
Attachment #509859 - Attachment description: patch rev1 → 2. main patch rev1
Attached patch 3. additional cleanup (obsolete) — Splinter Review
Attachment #509994 - Flags: review?(jmathies)
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?
(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.
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?
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
(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.
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.
Attached patch 2. patch rev2Splinter Review
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.
Attachment #509859 - Attachment is obsolete: true
Attachment #509994 - Attachment is obsolete: true
Attachment #510096 - Flags: review?(jmathies)
Attachment #509859 - Flags: review?(jmathies)
Attachment #509994 - Flags: review?(jmathies)
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
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.
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.
Attachment #509843 - Flags: review?(jmathies) → review+
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?
Attachment #510096 - Flags: review?(jmathies) → review+
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=?
Attachment #509843 - Flags: approval2.0?
Attachment #510096 - Flags: approval2.0?
Attachment #509843 - Flags: approval2.0? → approval2.0+
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.
Attachment #510096 - Flags: approval2.0? → approval2.0+
The hook and I are great friends! ;)

and thanks for the approval... tis' the best!
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.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite-
Flags: in-litmus?
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b12
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.
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/
btw: I should have just stated that further discussion should be in the dev-apps-firefox newsgroup / maillist.
Blocks: 633111
Depends on: 633728
Depends on: 732078
No longer depends on: 732078
See Also: → 1229626
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.