Closed Bug 562753 Opened 10 years ago Closed 10 years ago

On upgrade, old win7 taskbar entries should have their app modal id upgraded, based on install path

Categories

(Firefox :: Installer, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jimm, Assigned: jimm)

References

Details

Attachments

(3 files, 3 obsolete files)

After a discussion with rstrong on IRC, it was decided that we should try and update the app modal id of pinned win7 taskbar and startmenu links, if we can, after an upgrade. This would need to be done in fx proper, to account installs shared by multiple users.

Filed in installer for now, although technically this will probably involve Fx / Widget C++ code.
Summary: On upgrade,old win7 taskbar entries should have their app modal id upgraded, based on install path → On upgrade, old win7 taskbar entries should have their app modal id upgraded, based on install path
I suggest that the version of the app is stored in a pref like is done for the EM and nsBrowserContentHandler.js. When this changes launch the NSIS helper.exe (will need a separate app at some point for MSI) and let it do the work async. Just enum the shortcut files, check the path to exe is our path using shelllink, and update the app id using the app id plugin from bug 521141
(In reply to comment #1)
> I suggest that the version of the app is stored in a pref like is done for the
> EM and nsBrowserContentHandler.js. When this changes launch the NSIS helper.exe
> (will need a separate app at some point for MSI) and let it do the work async.
> Just enum the shortcut files, check the path to exe is our path using
> shelllink, and update the app id using the app id plugin from bug 521141

I should be checking for all users though correct, so get the user's appdata dir, step up one, step into every user on the system, and iterate across the two shortcut dirs. Maybe there's a shell folder constant for the User's directory I can use.
(In reply to comment #2)
> (In reply to comment #1)
> > I suggest that the version of the app is stored in a pref like is done for the
> > EM and nsBrowserContentHandler.js. When this changes launch the NSIS helper.exe
> > (will need a separate app at some point for MSI) and let it do the work async.
> > Just enum the shortcut files, check the path to exe is our path using
> > shelllink, and update the app id using the app id plugin from bug 521141
> 
> I should be checking for all users though correct, so get the user's appdata
> dir, step up one, step into every user on the system, and iterate across the
> two shortcut dirs. Maybe there's a shell folder constant for the User's
> directory I can use.
As stated on IRC, you won't have permissions to do this. You will have to do this on a per user basis so only do the current user.
btw: for taskbar you can use $QUICKLAUNCH to get the user's quick launch dir and then go to the User Pinned\TaskBar sub dir.
Attached patch patch v.1 (obsolete) — Splinter Review
Assignee: nobody → jmathies
Attachment #443363 - Flags: review?(robert.bugzilla)
Comment on attachment 443363 [details] [diff] [review]
patch v.1

Talked with jim over irc about doing this from WindowsJumpLists.jsm to avoid the TS hit on other versions of Windows.

>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
>@@ -4666,17 +4666,25 @@
>       MessageBox MB_YESNO|MB_ICONEXCLAMATION "$(WARN_RESTART_REQUIRED_UPGRADE)" IDNO +2
>       Reboot
>       Quit ; Nothing initialized so no need to call OnEndCommon
> 
>       ${GetParameters} $R0
> 
>       StrCmp "$R0" "" continue +1
> 
>+      ; Update this user's shortcuts with the latest app user model id.
>+      ClearErrors
>+      ${GetOptions} "$R0" "/UpdateShortcutAppUserModelIds" $R2
>+      IfErrors hideshortcuts +1
>+      ${UpdateShortcutAppModelIDs}  "$INSTDIR\${FileMainEXE}" "${AppUserModelID}"
>+      GoTo finish
>+
>       ; Require elevation if the user can elevate
>+      hideshortcuts:
>       ClearErrors
>       ${GetOptions} "$R0" "/HideShortcuts" $R2
>       IfErrors showshortcuts +1
> !ifndef NONADMIN_ELEVATE
>       ${ElevateUAC}
> !endif
>       ${HideShortcuts}
>       GoTo finish
>@@ -5771,8 +5779,102 @@
> 
> /**
>  * Deletes the shortcuts log ini file.
>  */
> !macro DeleteShortcutsLogFile
>   ${DeleteFile} "$INSTDIR\uninstall\${SHORTCUTS_LOG}"
> !macroend
> !define DeleteShortcutsLogFile "!insertmacro DeleteShortcutsLogFile"
>+
>+################################################################################
>+# Macros for managing Win7 install features
>+
>+/**
>+ * Update Start Menu and Taskbar lnk files that point to the current install
>+ * with the current application user model ID. Requires ApplicationID.
>+ *
>+ * @param   _INSTALL_PATH
>+ *          The install path of the app
>+ * @param   _APP_ID
>+ *          The application user model ID for the current install
>+ */
>+!macro UpdateShortcutAppModelIDs
>+
>+  !ifndef UpdateShortcutAppModelIDs
>+    !verbose push
>+    !verbose ${_MOZFUNC_VERBOSE}
>+    !define UpdateShortcutAppModelIDs "!insertmacro UpdateShortcutAppModelIDsCall"
>+
>+    Function UpdateShortcutAppModelIDs
>+      ClearErrors
>+
>+      ; stack: path, appid
>+      Exch $R0 ; stack: $R0, appid | $R0: path 
>+      Exch 1   ; stack: appid, $R0
>+      Exch $R1 ; stack: $R1, $R0 | $R1: appid
>+      Push $R2 ; stack: $R2, $R1, $R0
>+      Push $R3
>+      Push $R4
>+      Push $R5 ; stack: $R5, $R4, $R3, $R2, $R1, $R0
>+
>+      StrCpy $R2 "$QUICKLAUNCH\User Pinned"
>+
>+      ClearErrors
>+
>+      ; Taskbar links
>+      FindFirst $R3 $R4 "$R2\TaskBar\*.lnk"
>+      LoopTaskBar:
>+      ${If} ${FileExists} "$R2\TaskBar\$R4"
>+        ShellLink::GetShortCutTarget "$R2\TaskBar\$R4"
>+        Pop $R5
>+        ${If} "$R5" == "$R0" ; link path == InstallPath
>+          MessageBox MB_OK "$R2\TaskBar\$R4"
Remove the MessageBox here and below
Attachment #443363 - Flags: review?(robert.bugzilla) → review-
Attached patch patch v.2 (obsolete) — Splinter Review
updated to comments.
Attachment #443363 - Attachment is obsolete: true
Attachment #443449 - Flags: review?(robert.bugzilla)
Attached image registers screenshot
I added 
${debugSetRegisters}
${UpdateShortcutAppModelIDs} "$INSTDIR\${FileMainEXE}" "${AppUserModelID}"
${debugDisplayRegisters}

to the beginning of .onInit and find that your new macro is overwriting $R5. There is info in the beginning of common.nsh on how to avoid this.
Comment on attachment 443449 [details] [diff] [review]
patch v.2

>diff --git a/browser/components/shell/src/nsWindowsShellService.cpp b/browser/components/shell/src/nsWindowsShellService.cpp
>--- a/browser/components/shell/src/nsWindowsShellService.cpp
>+++ b/browser/components/shell/src/nsWindowsShellService.cpp
>@@ -58,16 +58,18 @@
> #include "nsIProcess.h"
> #include "nsICategoryManager.h"
> #include "nsBrowserCompsCID.h"
> #include "nsDirectoryServiceUtils.h"
> #include "nsAppDirectoryServiceDefs.h"
> #include "nsDirectoryServiceDefs.h"
> #include "nsIWindowsRegKey.h"
> #include "nsUnicharUtils.h"
>+#include "nsIWinTaskbar.h"
>+#include "nsISupportsPrimitives.h"
> 
> #include "windows.h"
> #include "shellapi.h"
> 
> #ifdef _WIN32_WINNT
> #undef _WIN32_WINNT
> #endif
> #define _WIN32_WINNT 0x0600
>@@ -81,16 +83,18 @@
> #endif
> 
> #define REG_SUCCEEDED(val) \
>   (val == ERROR_SUCCESS)
> 
> #define REG_FAILED(val) \
>   (val != ERROR_SUCCESS)
> 
>+#define NS_TASKBARINFO_CONTRACTID "@mozilla.org/windows-taskbar-info;1"
>+
> #ifndef WINCE
> NS_IMPL_ISUPPORTS2(nsWindowsShellService, nsIWindowsShellService, nsIShellService)
> #else
> NS_IMPL_ISUPPORTS1(nsWindowsShellService, nsIShellService)
> #endif
> 
> static nsresult
> OpenKeyForReading(HKEY aKeyRoot, const nsAString& aKeyName, HKEY* aKey)
>@@ -263,16 +267,130 @@ static SETTING gSettings[] = {
> 
>   // Protocol Handlers
>   { MAKE_KEY_NAME1("HTTP", DI),    "", VAL_FILE_ICON },
>   { MAKE_KEY_NAME1("HTTP", SOP),   "", VAL_OPEN },
>   { MAKE_KEY_NAME1("HTTPS", DI),   "", VAL_FILE_ICON },
>   { MAKE_KEY_NAME1("HTTPS", SOP),  "", VAL_OPEN }
> };
> 
>+nsresult
>+GetHelperPath(nsAutoString& aPath)
How about #ifndef WINCE for these?

minusing due to overwriting the $R5 register
Attachment #443449 - Flags: review?(robert.bugzilla) → review-
That must be due to this:

+      ; stack: path, appid
+      Exch $R0 ; stack: $R0, appid | $R0: path 
+      Exch 1   ; stack: appid, $R0
+      Exch $R1 ; stack: $R1, $R0 | $R1: appid
+      Push $R2 ; stack: $R2, $R1, $R0
+      Push $R3
+      Push $R4
+      Push $R5 ; stack: $R5, $R4, $R3, $R2, $R1, $R0


+      ${If} ${FileExists} "$R2\TaskBar\$R4"
+        ShellLink::GetShortCutTarget "$R2\TaskBar\$R4"
+        Pop $R5


+      Pop $R5
+      Pop $R4
+      Pop $R3
+      Pop $R2
+      Pop $R1
+      Pop $R0

Grabbed the return result the wrong way maybe? Oddly enough this worked just fine and $R5 had the right ShellLink call result.
> How about #ifndef WINCE for these?

Ok. Thought we dropped ce support though.
(In reply to comment #10)
> That must be due to this:
> 
> +      ; stack: path, appid
> +      Exch $R0 ; stack: $R0, appid | $R0: path 
> +      Exch 1   ; stack: appid, $R0
> +      Exch $R1 ; stack: $R1, $R0 | $R1: appid
> +      Push $R2 ; stack: $R2, $R1, $R0
> +      Push $R3
> +      Push $R4
> +      Push $R5 ; stack: $R5, $R4, $R3, $R2, $R1, $R0
> 
> 
> +      ${If} ${FileExists} "$R2\TaskBar\$R4"
> +        ShellLink::GetShortCutTarget "$R2\TaskBar\$R4"
> +        Pop $R5
> 
> 
> +      Pop $R5
> +      Pop $R4
> +      Pop $R3
> +      Pop $R2
> +      Pop $R1
> +      Pop $R0
If you follow the style of the existing macros you won't run into this problem.

> Grabbed the return result the wrong way maybe? Oddly enough this worked just
> fine and $R5 had the right ShellLink call result.
It will work just fine... the problem is that a caller might have set $R5 and then when they call this macro it will get overwritten.
(In reply to comment #11)
> > How about #ifndef WINCE for these?
> 
> Ok. Thought we dropped ce support though.
We did but until the code is removed I don't want to break compiling on WinCE in case it is added back (e.g. think of Tegra netbooks).
I forget if you added a new macros in one of the other patches I reviewed recently... if so, please verify that it also doesn't overwrite registers since I likely forgot to check with all of the patches I've reviewed this week. Thanks
(In reply to comment #12)
> (In reply to comment #10)
> > That must be due to this:
> > 
> > +      ; stack: path, appid
> > +      Exch $R0 ; stack: $R0, appid | $R0: path 
> > +      Exch 1   ; stack: appid, $R0
> > +      Exch $R1 ; stack: $R1, $R0 | $R1: appid
> > +      Push $R2 ; stack: $R2, $R1, $R0
> > +      Push $R3
> > +      Push $R4
> > +      Push $R5 ; stack: $R5, $R4, $R3, $R2, $R1, $R0
> > 
> > 
> > +      ${If} ${FileExists} "$R2\TaskBar\$R4"
> > +        ShellLink::GetShortCutTarget "$R2\TaskBar\$R4"
> > +        Pop $R5
> > 
> > 
> > +      Pop $R5
> > +      Pop $R4
> > +      Pop $R3
> > +      Pop $R2
> > +      Pop $R1
> > +      Pop $R0
> If you follow the style of the existing macros you won't run into this problem.
> 
> > Grabbed the return result the wrong way maybe? Oddly enough this worked just
> > fine and $R5 had the right ShellLink call result.
> It will work just fine... the problem is that a caller might have set $R5 and
> then when they call this macro it will get overwritten.

Hmm, but I pushed $R5 on the stack at the beginning. When I called ShellLink::GetShortCutTarget, that should have pushed a value on the stack, which I then popped into $R5, leaving the old R5 on the stack to be popped at the end?? Not sure what's going on here, but I'll figure it out.
+          ApplicationID::Set "$R2\StartMenu\$R4" "$R1"

ahh, maybe I need to pop the result from Set!
If you prefer I can rewrite the macro in the same style as the existing macros... it would also be nice to keep them consistent.
btw: you'll notice that I don't use macros inside other macros unless I absolutely have to in common.nsh since some of them overwrite registers as well which is why I use IntCmp, StrCmp, etc. almost everywhere in common.nsh.
Attached patch patch v.3 (obsolete) — Splinter Review
updated to comments. This also passes the registers check test for the macro.
Attachment #443449 - Attachment is obsolete: true
Attachment #444014 - Flags: review?(robert.bugzilla)
Comment on attachment 444014 [details] [diff] [review]
patch v.3

wrong patch
Attachment #444014 - Attachment is obsolete: true
Attachment #444014 - Flags: review?(robert.bugzilla)
Attached patch patch v.3Splinter Review
Ok, this one has the updates in it.
Attachment #444015 - Flags: review?(robert.bugzilla)
Comment on attachment 444015 [details] [diff] [review]
patch v.3

>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
>@@ -117,16 +117,17 @@ VIAddVersionKey "OriginalFilename" "setu
> !insertmacro ManualCloseAppPrompt
> !insertmacro RegCleanAppHandler
> !insertmacro RegCleanMain
> !insertmacro RegCleanUninstall
> !insertmacro SetBrandNameVars
> !insertmacro UnloadUAC
> !insertmacro WriteRegStr2
> !insertmacro WriteRegDWORD2
>+!insertmacro UpdateShortcutAppModelIDs
nit: these are sorted alphabetically so please put UpdateShortcutAppModelIDs after UnloadUAC

>diff --git a/browser/installer/windows/nsis/uninstaller.nsi b/browser/installer/windows/nsis/uninstaller.nsi
>--- a/browser/installer/windows/nsis/uninstaller.nsi
>+++ b/browser/installer/windows/nsis/uninstaller.nsi
>@@ -99,16 +99,17 @@ VIAddVersionKey "OriginalFilename" "help
> !insertmacro IsHandlerForInstallDir
> !insertmacro RegCleanAppHandler
> !insertmacro RegCleanMain
> !insertmacro RegCleanUninstall
> !insertmacro SetBrandNameVars
> !insertmacro UnloadUAC
> !insertmacro WriteRegDWORD2
> !insertmacro WriteRegStr2
>+!insertmacro UpdateShortcutAppModelIDs
same here
Attachment #444015 - Flags: review?(robert.bugzilla) → review+
http://hg.mozilla.org/mozilla-central/rev/a24884c8140a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 566125
You need to log in before you can comment on or make changes to this bug.