577867 - No way to un-group separate instances on Windows 7 TaskBar

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[Rod Vagg]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.2.4) Gecko/20100611 Firefox/3.6.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:2.0b1) Gecko/20100630 Firefox/4.0b1

Background: I run separate instances of FF with different configurations. My GMail and Google Reader instances don't have any Add-ons, the UI is minimal and the themes make them look distinct. My standard browsing FF has all my Add-ons. My passwords, bookmarks and history are all synced with Firefox Sync. My shortcuts look like this: [ "C:\Program Files (x86)\Mozilla Firefox\firefox.exe" -no-remote -P <profile name> ] with "-no-remote" missing from my standard instance.

Until FF4b1 I could pin these shortcuts to the Windows 7 TaskBar and run them as if they were separate applications. Windows 7 didn't link them together and didn't group them together when I ran out of space, they were treated as if they were completely distinct applications. Once the pinned shortcut is launched FF runs ontop of that shortcut. This all makes switching between them and seeing which is which very easy.

With the new Win7 integration in FF4 (which is generally excellent btw), Windows 7 recognises them as the same application so they get joined and grouped. When I run out of space on my TaskBar they combine and it's difficult to switch between the windows; this is compounded when there are multiple tabs open across the windows because clicking on the grouped icon brings up all the tabs and it's difficult to find the one that I want to switch to. Additionally, I can pin my shortcuts to the TaskBar but once launched they group together ontop of one of the shortcuts and the others remain as if they are unlaunched.

While I understand the logic behind what's happening and the Windows 7 integration is a good step forward, perhaps there is cause for making instances run with different -P values run as separate instances? Or maybe there is scope for a new command line argument to run them separately?

Reproducible: Always

Comment 1

[Will]

This is actually a big usability issue for me. My wife and I each have our own separate Fx profile. With Fx 3.x, each of us can have a separate Fx icon on the Windows 7 taskbar, making it very simple to launch a particular profile. Because of the way that Fx 4 behaves, we can now only have 1 icon on the taskbar.

Currently my wife owns the taskbar icon as it's the easiest to launch. For me to launch my profile, I either have to go through the Start menu or find an icon on the desktop. If I launch my profile while my wife's is already running, them mine gets grouped with hers on the taskbar. In the case that I launch my profile before she starts hers, then it takes over her icon on the taskbar. Now, for my wife to launch her own profile, she can't simply click the icon anymore... she has to either go through the start menu or a desktop shortcut.

So, again this is a very big usability issue for me that will affect me on  a daily basis. Is there any way we can have the old Fx 3.x behaviour back in some way?

Comment 2

[Will]

I would also like to confirm that this still occurs on Fx 4 RC1.

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Here's what I've found. Maybe it'll work for you.

Pin Firefox to the taskbar normally. Now, open the Firefox folder in Program Files. Drag firefox.exe to the Desktop as a shortcut. Drag that shortcut to the taskbar, and you should get the two separate instances. Right-click each shortcut, go to Properties -> Shortcut, and set the "-P PROFNAME" flag in the Target section (and -no-remote, probably). You can now start both profiles up, although the window shortcuts will probably collapse onto one of the taskbar icons.


Pretty sure this is fallout from bug 505925. Looks like Seamonkey had something land around the same time, though I'm not sure what it did (bug 560846).

Once it lands, bug 597911 would be one way to get around this, assuming you're running a 64-bit version of Windows 7. (One of you could use a 32-bit shortcut, while the other could use the 64-bit one.)

CCing Jimm, because he's been present in every app model id bug I've seen. Jimm, what was it that landed in bug 560846 for Seamonkey?

Comment 4

[Will]

Tried your workaround, but the result was buggy and unpredictable at best. I was eventually able to get 2 separate taskbar icons, but with both profiles open, the windows would ultimately end up being grouped under the same icon.

This illustrates another problem with the current model. If my wife and I have both our profiles running simultaneously, because the windows are grouped under the same icon, the likelihood of me accidentally closing her windows or vice versa is very high. As it is this bug is severely hampering the practical usefulness of separate profiles on a shared computer.

Comment 5

[MagoBone]

Will, i also hope this gets fixed on final version of Firefox 4 (i had same issues when 3.5 first came out). In the meanwhile i would suggest you and your wife to use two different personas styles, so that it's clear which profile a window belongs to. Just a workaround of course...but better than nothing :)

Comment 6

[Will]

I just wanted to also point out that this bug also extends to the Start menu. Even with a different shorcut for each profile, apparently you can only pin one to the Windows 7 Start menu.

Having two different personas does help a little with the accidentally closing each other's windows, but obviously the launching issue is still there.

Comment 8

[Jim Mathies [:jimm]]

This may be tough to fix. The id used is embedded in the application.ini file
and in the installer/updater code header. (It's of the form
|vendor.application.version|.) Every time the version revs all shortcuts get
updated so they continue to group correctly. Appending a profile name to the id
in the main fx app is easy, but propagating that to the installer/updater might
be complex. cc'ing rstrong, he might have some ideas. 

For now, you can always alter you application.ini settings to get profiles to
group apart. But those changes aren't exactly user friendly.

Comment 9

[Jim Mathies [:jimm]]

I guess one option would be to have the updater continue to update shortcuts with the standard 'vender.app.version' string. Then maybe add a pref like 

"taskbar.grouping.useprofilename" 

which would force the running instance of fx to use something like:

"vender.application.profile"

The updater would continue to do it's normal maintenance of shortcut ids, but before updating a shortcut it would check for a non-standard group id before doing so. If it found one of these profile tagged shortcuts it would leave it alone.

Comment 10

[Pascal]

(In reply to comment #8)
> For now, you can always alter you application.ini settings to get profiles to
> group apart. But those changes aren't exactly user friendly.

How would one do this? I only see one ID in the application.ini file. If I were to change it, wouldn't all Firefox instances use the new ID and have the same problem?

Comment 11

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

(In reply to comment #9)
Then every time the app version changes we'd have to parse out the version for each shortcut that points to the updated install location and update just that one field.

This would be a lot simpler if we were able to come up with a way to use the install location to create the app model id... then we wouldn't have to ever update the shortcuts since they wouldn't change with each version change and multiple installs of the same version wouldn't get grouped together.

Comment 12

[Jim Mathies [:jimm]]

(In reply to comment #11)
> (In reply to comment #9)
> Then every time the app version changes we'd have to parse out the version for
> each shortcut that points to the updated install location and update just that
> one field.
> 
> This would be a lot simpler if we were able to come up with a way to use the
> install location to create the app model id... then we wouldn't have to ever
> update the shortcuts since they wouldn't change with each version change and
> multiple installs of the same version wouldn't get grouped together.

We could do that by replacing the version string with an MD5 hash of the install path. (The id has a 128 character limit.) Looks like there's an nsis crypt module we could leverage:

http://nsis.sourceforge.net/Crypto_plug-in

Then also add the profile on the end using the pref switch I suggested. Sound workable Rob?

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

If you go with the hash option, bug 597911 should probably be wontfixed, since this change would make that one unnecessary.

Comment 14

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

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #9)
> > Then every time the app version changes we'd have to parse out the version for
> > each shortcut that points to the updated install location and update just that
> > one field.
> > 
> > This would be a lot simpler if we were able to come up with a way to use the
> > install location to create the app model id... then we wouldn't have to ever
> > update the shortcuts since they wouldn't change with each version change and
> > multiple installs of the same version wouldn't get grouped together.
> 
> We could do that by replacing the version string with an MD5 hash of the
> install path. (The id has a 128 character limit.) Looks like there's an nsis
> crypt module we could leverage:
> 
> http://nsis.sourceforge.net/Crypto_plug-in
> 
> Then also add the profile on the end using the pref switch I suggested. Sound
> workable Rob?
Sounds good to me. We might want to store the hash in a file or possibly a reg key instead of generating it during every startup.

Comment 15

[Ivan Dobrianov]

(In reply to comment #8)
> This may be tough to fix. The id used is embedded in the application.ini file
> and in the installer/updater code header. (It's of the form
> |vendor.application.version|.) Every time the version revs all shortcuts get
> updated so they continue to group correctly. Appending a profile name to the id
> in the main fx app is easy, but propagating that to the installer/updater might
> be complex. cc'ing rstrong, he might have some ideas. 
> 
> For now, you can always alter you application.ini settings to get profiles to
> group apart. But those changes aren't exactly user friendly.

I'd like to second Pascal's question #10: How do you do these "not exactly user friendly" changes to application.ini? This is just one .ini file - how do you make different FF instances/profiles use different IDs? Can you clarify? For the immediate future, I'd settle for _anything_ that works! -- Many thanks!

Comment 16

[Jim Mathies [:jimm]]

You would have to install two separate copies if fx, and alter the ini file so that the id's are different, then associate each with a different profile. Kind of a pain. Plus I think during updates the ini file might get over written.

Comment 17

[Paul Alexandrescu]

I concur with Jim's post that the best solution would be to append the profile name to the Application Id.  
I don't see why a user would have firefox installed in 2 locations and use the same profile, and launch both apps at the same time. 
Most likely usage ( mine included ) is using 2 profiles at the same time of the same install.  Right now I'm using Jim's other suggestion of changing the application.ini and using 2 firefoxes installed, but that's just a temp workaround I hope.

Comment 18

[Jim Mathies [:jimm]]

Attachment: md5 licence

Rob, I'm thinking of using this add-in:

http://nsis.sourceforge.net/MD5_plugin

The crypt plugin apparently has issues. I'm concerned about the license though, should I run this past someone in legal before I commit to using the code?

Comment 19

[Jim Mathies [:jimm]]

Comment on attachment 524457 [details]
md5 licence

Gerv,

This is the license for a piece of code in a new nsis install plugin we want to use. Curious if this is compatible, can be used out right without any changes to our licensing, about info, etc..

Comment 20

[Gervase Markham [:gerv]]

Comment on attachment 524457 [details]
md5 licence

Hi Jim,

Unfortunately, that software has a "BSD Advertising Clause":
http://www.gnu.org/philosophy/bsd.html

Software with such a clause is considered a pain in the ass and, more importantly, is incompatible with the GPL, one of the licenses we use for Mozilla code:
http://www.gnu.org/licenses/license-list.html (grep for the section titled "Original BSD license").

There are other implementations of MD5 listed here, including ones in C specifically listed as "RSA-free":
http://userpages.umbc.edu/~mabzug1/cs/md5/md5.html

Could you use one of those?

Also, I hope you are not using MD5 for anything cryptographically strong?

Gerv

Comment 21

[Jim Mathies [:jimm]]

(In reply to comment #20)
> Comment on attachment 524457 [details]
> md5 licence
> 
> Hi Jim,
> 
> Unfortunately, that software has a "BSD Advertising Clause":
> http://www.gnu.org/philosophy/bsd.html
> 
> Software with such a clause is considered a pain in the ass and, more
> importantly, is incompatible with the GPL, one of the licenses we use for
> Mozilla code:
> http://www.gnu.org/licenses/license-list.html (grep for the section titled
> "Original BSD license").
> 
> There are other implementations of MD5 listed here, including ones in C
> specifically listed as "RSA-free":
> http://userpages.umbc.edu/~mabzug1/cs/md5/md5.html
> 
> Could you use one of those?
> 
> Also, I hope you are not using MD5 for anything cryptographically strong?
> 
> Gerv

Thanks for the feedback. Nothing crypto related here, we just need a short hash of an install path string for some windows 7 taskbar registration work. I'll look around at other options, we might just roll our own extension for this, the code required is pretty straight forward.

Comment 22

[Will]

I don't know much about hashing or licenses, but I stumbled across this today and thought it could potentially useful.
http://google-opensource.blogspot.com/2011/04/introducing-cityhash.html

Comment 23

[Jim Mathies [:jimm]]

(In reply to comment #22)
> I don't know much about hashing or licenses, but I stumbled across this today
> and thought it could potentially useful.
> http://google-opensource.blogspot.com/2011/04/introducing-cityhash.html

Hey thanks, that CityHash64 looks perfect for what we're doing here.

Comment 24

[Jim Mathies [:jimm]]

Attachment: google cityhash header

Here's another prospect Gerv, w/a MIT license:

http://code.google.com/p/cityhash/

Comment 25

[Gervase Markham [:gerv]]

This looks great. It doesn't even need adding to about:licence.

Gerv

Comment 26

[Jim Mathies [:jimm]]

Attachment: cityhash 64 plugin

Comment 27

[Jim Mathies [:jimm]]

Attachment: cityhash64 plugin src

Comment 28

[Jim Mathies [:jimm]]

Attachment: cityhash64 plugin src

updates...

Comment 29

[Jim Mathies [:jimm]]

Attachment: release dll build

Note, this was built using vs 2008 and is linked to the 2008 SP1 runtime. Win7 has this pre-installed, so this should be ok.

Comment 30

[Jim Mathies [:jimm]]

Attachment: installer changes v.1

Comment 31

[Jim Mathies [:jimm]]

Attachment: cityhash64 plugin src

- updated to unicode
- fixed sprintf, now uses I64 precision for output

Comment 32

[Jim Mathies [:jimm]]

Attachment: release dll build

Comment 33

[Jim Mathies [:jimm]]

Attachment: installer changes v.2

- removed debug gunk
- fixed a few bugs after some testing

Comment 34

[Jim Mathies [:jimm]]

Attachment: widget changes v.1

Comment 35

[Jim Mathies [:jimm]]

Attachment: installer changes v.3

more cleanup

Comment 36

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

Comment on attachment 526819 [details] [diff] [review]
cityhash64 plugin src

Gerv, what (if any) are the additional restrictions when using the MPL license on some files along with other files with a MIT license and software that is not copyrighted in the context of an NSIS plugin?

Comment 37

[Gervase Markham [:gerv]]

Rob: there's pretty much no such thing as "software which is not copyrighted"; what do you mean by that part of what you said?

The license Google is fully compatible with ours, so using files licensed like that and tri-licensed files in the same software is OK. All we need to note is that it has used has to be put in about:licence if we ship the code as part of Firefox. If it's going in the installer, that's technically not part of Firefox itself, but that seems like the easiest place to put it anyway.

Gerv

Comment 38

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

(In reply to comment #36)
> Comment on attachment 526819 [details] [diff] [review]
> cityhash64 plugin src
> 
> Gerv, what (if any) are the additional restrictions when using the MPL license
> on some files along with other files with a MIT license and software that is
> not copyrighted in the context of an NSIS plugin?

(In reply to comment #37)
> Rob: there's pretty much no such thing as "software which is not copyrighted";
> what do you mean by that part of what you said?
From attachment #526819 [details] [diff] [review]
stdint.h uses

>+/* ISO C9x  7.18  Integer types <stdint.h>
>+ * Based on ISO/IEC SC22/WG14 9899 Committee draft (SC22 N2794)
>+ *
>+ *  THIS SOFTWARE IS NOT COPYRIGHTED
>+ *
>+ *  Contributor: Danny Smith <[e-mail]>
>+ *
>+ *  This source code is offered for use in the public domain. You may
>+ *  use, modify or distribute it freely.
>+ *
>+ *  This code is distributed in the hope that it will be useful but
>+ *  WITHOUT ANY WARRANTY. ALL WARRANTIES, EXPRESS OR IMPLIED ARE HEREBY
>+ *  DISCLAIMED. This includes but is not limited to warranties of
>+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>+ *
>+ *  Date: 2000-12-02
>+ */

> The license Google is fully compatible with ours, so using files licensed like
> that and tri-licensed files in the same software is OK. All we need to note is
> that it has used has to be put in about:licence if we ship the code as part of
> Firefox. If it's going in the installer, that's technically not part of Firefox
> itself, but that seems like the easiest place to put it anyway.
My concern is regarding additional restrictions. I know that it is fine for us to use the NSIS plugin. I am concerned that it won't be fine for all others to use this NSIS plugin.

Comment 39

[Jim Mathies [:jimm]]

If that Windows compliant stdint.h is a problem, I can roll just the defines cityhash uses into the plugin header.

Comment 40

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

Comment on attachment 526828 [details] [diff] [review]
installer changes v.3

Partial review since I had a few minutes

>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
>@@ -1134,19 +1134,19 @@
> !macro CheckIfRegistryKeyExists
>   !ifndef CheckIfRegistryKeyExists
>     !verbose push
>     !verbose ${_MOZFUNC_VERBOSE}
>     !define CheckIfRegistryKeyExists "!insertmacro CheckIfRegistryKeyExistsCall"
> 
>     Function CheckIfRegistryKeyExists
>       ; stack: main key, key
>-      Exch $R9 ; main key
>-      Exch 1
>-      Exch $R8 ; key
>+      Exch $R9 ; main key, stack: OR9, key
>+      Exch 1   ; stack: key, OR9
>+      Exch $R8 ; key, stack: OR8, OR9
What does "OR" stand for in the case of OR8 and OR9 above and below?

>       Push $R7
>       Push $R6
>       Push $R5
> 
>       StrCpy $R5 "false"
>       StrCpy $R7 "0" # loop index
>       ${Do}
>         EnumRegKey $R6 SHCTX "$R9" "$R7"
>@@ -1157,20 +1157,19 @@
>         IntOp $R7 $R7 + 1
>       ${LoopWhile} $R6 != ""
>       ClearErrors
> 
>       StrCpy $R9 $R5
> 
>       Pop $R5
>       Pop $R6
>-      Pop $R7
>-      Exch $R8
>-      Exch 1
>-      Exch $R9
>+      Pop $R7 ; stack: OR8, OR9 
>+      Pop $R8 ; stack: OR9
>+      Exch $R9 ; stack: result
>     FunctionEnd
> 
>     !verbose pop
>   !endif
> !macroend
> 
> !macro CheckIfRegistryKeyExistsCall _MAIN_KEY _KEY _RESULT
>   !verbose push
>@@ -4826,17 +4825,17 @@
>       ${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}" $R2
>+      ${UpdateShortcutAppModelIDs}  "$INSTDIR\${FileMainEXE}" "$AppUserModelID" $R2
You are going to break other apps again since they don't have AppUserModelID declared as you did when you first added UpdateShortcutAppModelIDs.

Just add Var AppUserModelID to the UpdateShortcutAppModelIDs macro as is done in other macros.

Also, file or remove the AppUserModelID define for other apps.

>       StrCmp "$R2" "true" finish +1 ; true indicates that shortcuts have been updated
>       Quit ; Nothing initialized so no need to call OnEndCommon
> 
>       ; Require elevation if the user can elevate
>       hideshortcuts:
>       ClearErrors
>       ${GetOptions} "$R0" "/HideShortcuts" $R2
>       IfErrors showshortcuts +1
>@@ -6670,8 +6669,59 @@
>   !verbose push
>   !verbose ${_MOZFUNC_VERBOSE}
>   Push "${_APP_ID}"
>   Push "${_EXE_PATH}"
>   Call UpdateShortcutAppModelIDs
>   Pop ${_RESULT}
>   !verbose pop
> !macroend
>+
>+!macro GetHashAppModelId
>+  !ifndef GetHashAppModelId
>+    !insertmacro GetLongPath
>+    !verbose push
>+    !verbose ${_MOZFUNC_VERBOSE}
>+    !define GetHashAppModelId "!insertmacro GetHashAppModelIdCall"
>+
>+    Function GetHashAppModelId
>+      ${debugSetRegisters}
>+      Exch $R9 ; exchange R9 w/path on stack
>+      Push $R8 ; stack: old $R8, old $R9
>+      
>+      ${If} ${AtLeastWin7}
>+        ${GetLongPath} "$R9" $R9
>+        ClearErrors
>+        ReadRegStr $R8 SHCTX "Software\Mozilla\Taskbar" "$R9"
>+        IfErrors +1 end
Please use ${If} ${Errors}... LogicLib can now be used in common.nsh

>+        ; If it doesn't exist, create a new one and store it
>+        CityHash::NSISCityHash64 "$R9"
Redundant naming - NSISCityHash64

>+        Pop $R8
>+        ${If} $R8 == "error"
>+          GoTo end
>+        ${EndIf}
>+        ClearErrors
>+        WriteRegStr SHCTX "Software\Mozilla\Taskbar"  "$R9" "$R8"
Please put these in an app specific registry key so apps don't stomp on each other. Better still, pass the key to store it into the function.

>diff --git a/toolkit/mozapps/installer/windows/nsis/makensis.mk b/toolkit/mozapps/installer/windows/nsis/makensis.mk
>--- a/toolkit/mozapps/installer/windows/nsis/makensis.mk
>+++ b/toolkit/mozapps/installer/windows/nsis/makensis.mk
>@@ -55,16 +55,17 @@ TOOLKIT_NSIS_FILES = \
> 
> CUSTOM_NSIS_PLUGINS = \
> 	AccessControl.dll \
> 	AppAssocReg.dll \
> 	ApplicationID.dll \
> 	InvokeShellVerb.dll \
> 	ShellLink.dll \
> 	UAC.dll \
>+	CityHash.dll \
Since this is in alphabetical order please keep it that way.

Comment 41

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

To ease the transition to this new method I *think* you can just change the pref check logic to if the pref exists clear it and call the helper. The pref check, etc. can be removed after a couple of releases with this change so a new bug should be filed so that doesn't get lost.

Comment 42

[Jim Mathies [:jimm]]

(In reply to comment #41)
> To ease the transition to this new method I *think* you can just change the
> pref check logic to if the pref exists clear it and call the helper. The pref
> check, etc. can be removed after a couple of releases with this change so a new
> bug should be filed so that doesn't get lost.

Yeah I was thinking we'd rip it all out about a year from now. I'll add the changes to the helper call.

Comment 43

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

Comment on attachment 526828 [details] [diff] [review]
installer changes v.3

>+!macro GetHashAppModelId
>+  !ifndef GetHashAppModelId
>+    !insertmacro GetLongPath
>+    !verbose push
>+    !verbose ${_MOZFUNC_VERBOSE}
>+    !define GetHashAppModelId "!insertmacro GetHashAppModelIdCall"
>+
>+    Function GetHashAppModelId
>+      ${debugSetRegisters}
>+      Exch $R9 ; exchange R9 w/path on stack
>+      Push $R8 ; stack: old $R8, old $R9
>+      
>+      ${If} ${AtLeastWin7}
>+        ${GetLongPath} "$R9" $R9
>+        ClearErrors
>+        ReadRegStr $R8 SHCTX "Software\Mozilla\Taskbar" "$R9"
I don't think the logic here is quite right since it could exist in HKLM or HKCU while SHCTX will only be one or the other. I think you should check both and then try to write to HKLM and if that fails write to HKCU.

Comment 44

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

Comment on attachment 526828 [details] [diff] [review]
installer changes 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
>...
>@@ -301,28 +303,37 @@ Section "-Application" APP_IDX
>   ${RegCleanMain} "Software\Mozilla"
>   ${RegCleanUninstall}
>   ${UpdateProtocolHandlers}
> 
>   ClearErrors
>   WriteRegStr HKLM "Software\Mozilla" "${BrandShortName}InstallerTest" "Write Test"
>   ${If} ${Errors}
>     StrCpy $TmpVal "HKCU" ; used primarily for logging
>+
>+    SetShellVarContext current
>+    Call GetShortcutAppModelID
>+    SetShellVarContext all
This is just wrong... SetShellVarContext is already set to current and this will break registry writes from this point forward since the user doesn't have access to HKLM and you leave SetShellVarContext set to all. You should be able to just use one call and not concern yourself with the SHCTX if you implement the previous comment.

There are enough review comments that will require changing the current approach taken so minusing now instead of reviewing code that will need to change.

Comment 45

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

From irc:
<gerv>	MIT is fine for commercial use,
<gerv>	but MIT and MPL is a strange combo.
<gerv>	Which bug is this?
<jimm>	gerv: bug 577867
<jimm>	that Google City Hash code
<gerv>	Oh, that.
<gerv>	You leave it as MIT,
<gerv>	you don't add the tri-license on top.
<rs>	gerv: thanks and ignore the email I just sent... that was my concern
<gerv>	OK.
<gerv>	If it's third party code,
<gerv>	you leave the original licence on it.

Comment 46

[Jim Mathies [:jimm]]

(In reply to comment #43)
> Comment on attachment 526828 [details] [diff] [review] [review]
> >+        ReadRegStr $R8 SHCTX "Software\Mozilla\Taskbar" "$R9"
> I don't think the logic here is quite right since it could exist in HKLM or
> HKCU while SHCTX will only be one or the other. I think you should check
> both and then try to write to HKLM and if that fails write to HKCU.

added.

(In reply to comment #40)
> Comment on attachment 526828 [details] [diff] [review] [review]
> >+      Exch 1   ; stack: key, OR9
> >+      Exch $R8 ; key, stack: OR8, OR9
> What does "OR" stand for in the case of OR8 and OR9 above and below?

"original". I've updated this though.

> >-      ${UpdateShortcutAppModelIDs}  "$INSTDIR\${FileMainEXE}" "${AppUserModelID}" $R2
> >+      ${UpdateShortcutAppModelIDs}  "$INSTDIR\${FileMainEXE}" "$AppUserModelID" $R2
> You are going to break other apps again since they don't have AppUserModelID
> declared as you did when you first added UpdateShortcutAppModelIDs.
> 
> Just add Var AppUserModelID to the UpdateShortcutAppModelIDs macro as is
> done in other macros.

I didn't understand this suggestion. Adding a Var to the macro (and removing them from the install and uninstall scripts) resulted in broken builds for the install and uninstall. What I ended up doing was !ifdef'ing to check for ${AppUserModelID} in common. If you can clarify what the Var trick is with the macros I can update again. 

> Also, file or remove the AppUserModelID define for other apps.

I'm going to do that myself here after I finish fx up.

> >+        ReadRegStr $R8 SHCTX "Software\Mozilla\Taskbar" "$R9"
> >+        IfErrors +1 end
> Please use ${If} ${Errors}... LogicLib can now be used in common.nsh

updated.

> 
> >+        ; If it doesn't exist, create a new one and store it
> >+        CityHash::NSISCityHash64 "$R9"
> Redundant naming - NSISCityHash64

Not sure what you meant here. The c function that city hash exposes is 'CityHash64', so the entry point for the plugin needed to be a variant of that. I could make this "GetCityHash64" or similar if you want. Note the '64' is due to there being two entry points 'CityHash64' & 'CityHash128'.)
> 
> >+        Pop $R8
> >+        ${If} $R8 == "error"
> >+          GoTo end
> >+        ${EndIf}
> >+        ClearErrors
> >+        WriteRegStr SHCTX "Software\Mozilla\Taskbar"  "$R9" "$R8"
> Please put these in an app specific registry key so apps don't stomp on each
> other. Better still, pass the key to store it into the function.

Updated per our irc discussion.

> > CUSTOM_NSIS_PLUGINS = \
> > 	AccessControl.dll \
> > 	AppAssocReg.dll \
> > 	ApplicationID.dll \
> > 	InvokeShellVerb.dll \
> > 	ShellLink.dll \
> > 	UAC.dll \
> >+	CityHash.dll \
> Since this is in alphabetical order please keep it that way.

updated.

Comment 47

[Jim Mathies [:jimm]]

Attachment: installer changes v.4

Comment 48

[Jim Mathies [:jimm]]

Attachment: widget changes v.2

Comment 49

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

(In reply to comment #46)
> (In reply to comment #43)
> > >-      ${UpdateShortcutAppModelIDs}  "$INSTDIR\${FileMainEXE}" "${AppUserModelID}" $R2
> > >+      ${UpdateShortcutAppModelIDs}  "$INSTDIR\${FileMainEXE}" "$AppUserModelID" $R2
> > You are going to break other apps again since they don't have AppUserModelID
> > declared as you did when you first added UpdateShortcutAppModelIDs.
> > 
> > Just add Var AppUserModelID to the UpdateShortcutAppModelIDs macro as is
> > done in other macros.
> 
> I didn't understand this suggestion. Adding a Var to the macro (and removing
> them from the install and uninstall scripts) resulted in broken builds for
> the install and uninstall. What I ended up doing was !ifdef'ing to check for
> ${AppUserModelID} in common. If you can clarify what the Var trick is with
> the macros I can update again. 
See
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/windows/nsis/common.nsh#4332
and
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/windows/nsis/common.nsh#5530

What this does is make it so each app that inserts the macro will automatically have the var declared so each app doesn't need to add it.

> > >+        ReadRegStr $R8 SHCTX "Software\Mozilla\Taskbar" "$R9"
> > >+        IfErrors +1 end
> > Please use ${If} ${Errors}... LogicLib can now be used in common.nsh
> 
> updated.
> 
> > 
> > >+        ; If it doesn't exist, create a new one and store it
> > >+        CityHash::NSISCityHash64 "$R9"
> > Redundant naming - NSISCityHash64
> 
> Not sure what you meant here. The c function that city hash exposes is
> 'CityHash64', so the entry point for the plugin needed to be a variant of
> that. I could make this "GetCityHash64" or similar if you want. Note the
> '64' is due to there being two entry points 'CityHash64' & 'CityHash128'.)
GetCityHash64 sounds better

Comment 50

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

There are compiler warning when compiling the uninstaller. Specifically, you never call GetShortcutAppModelID and I believe you want to call it in shared.nsi before any use of $AppUserModelID.

I am fairly certain you can remove the Var declarations in installer.nsi and uninstaller.nsi and just add the following
 !macro UpdateShortcutAppModelIDs

   !ifndef UpdateShortcutAppModelIDs
     !insertmacro GetLongPath

+    Var AppUserModelID

Comment 51

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

Comment on attachment 531164 [details] [diff] [review]
installer changes v.4

Jim, it is going to be a couple of days until I can look at this again. WOuld you mind updating the patch to the current comments prior to? I also noticed that you probably have a patch in your queue that modifies installer.nsi that adds the comment near the top of the file regarding CityHash

Comment 52

[Jim Mathies [:jimm]]

Attachment: cityhash64 plugin src

- updated plugin src since we changed the entry point

Comment 53

[Jim Mathies [:jimm]]

Attachment: release dll build

- new dll

Comment 54

[Jim Mathies [:jimm]]

Attachment: installer changes v.5

- installer updates

Comment 55

[Jim Mathies [:jimm]]

Attachment: installer changes v.5

- nix a testing message box

Comment 56

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

Comment on attachment 531409 [details] [diff] [review]
cityhash64 plugin src

>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
>@@ -34,16 +34,17 @@
> #
> # ***** END LICENSE BLOCK *****
> 
> # Required Plugins:
> # AppAssocReg   http://nsis.sourceforge.net/Application_Association_Registration_plug-in
> # ApplicationID http://nsis.sourceforge.net/ApplicationID_plug-in
> # ShellLink     http://nsis.sourceforge.net/ShellLink_plug-in
> # UAC           http://nsis.sourceforge.net/UAC_plug-in
>+# CityHash
> 
> ; Set verbosity to 3 (e.g. no script) to lessen the noise in the build logs
> !verbose 3
> 
> ; 7-Zip provides better compression than the lzma from NSIS so we add the files
> ; uncompressed and use 7-Zip to create a SFX archive of it
> SetDatablockOptimize on
> SetCompress off
Please remove this from the patch. Thanks

Comment 57

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

Comment on attachment 531419 [details] [diff] [review]
installer changes v.5

>diff --git a/browser/installer/windows/nsis/defines.nsi.in b/browser/installer/windows/nsis/defines.nsi.in
>--- a/browser/installer/windows/nsis/defines.nsi.in
>+++ b/browser/installer/windows/nsis/defines.nsi.in
>@@ -1,24 +1,17 @@
> #filter substitution
> 
>-# Win7: AppVendor, AppName, and AppVersion must match the application.ini values
>-# of Vendor, Name, and Version. These values are used in registering shortcuts
>+# Win7: AppVendor and AppName values may be used in registering shortcuts
> # with the taskbar. ExplicitAppUserModelID registration when the app launches is
> # handled in widget/src/windows/WinTaskbar.cpp.
This comment should live with the GetShortcutAppModelID function and it should state that it will use a hash of the path and fallback to using Vendor, Name, and Version if that fails.

>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
>@@ -103,16 +103,17 @@ VIAddVersionKey "OriginalFilename" "help
> !insertmacro RegCleanUninstall
> !insertmacro SetAppLSPCategories
> !insertmacro SetBrandNameVars
> !insertmacro UpdateShortcutAppModelIDs
> !insertmacro UnloadUAC
> !insertmacro WriteRegDWORD2
> !insertmacro WriteRegStr2
> !insertmacro CheckIfRegistryKeyExists
>+!insertmacro GetHashAppModelId
> 
> !insertmacro un.ChangeMUIHeaderImage
> !insertmacro un.CheckForFilesInUse
> !insertmacro un.CleanUpdatesDir
> !insertmacro un.CleanVirtualStore
> !insertmacro un.DeleteRelativeProfiles
> !insertmacro un.DeleteShortcuts
> !insertmacro un.GetLongPath
>@@ -230,17 +231,21 @@ Section "Uninstall"
>   ${EndIf}
> 
>   SetShellVarContext current  ; Set SHCTX to HKCU
>   ${un.RegCleanMain} "Software\Mozilla"
>   ${un.RegCleanUninstall}
>   ${un.DeleteShortcuts}
> 
>   ; Unregister resources associated with Win7 taskbar jump lists.
>-  ApplicationID::UninstallJumpLists "${AppUserModelID}"
>+  ApplicationID::UninstallJumpLists "$AppUserModelID"
$AppUserModelID hasn't been set!

You'll need to set it in at least ShowShortcuts in shared.nsi as well... perhaps elsewhere. Basically, whenever you use $AppUserModelID you have to call GetShortcutAppModelID. You might want to just name it GetAppModelID, make it into a macro in common.nsh, make it support both install and uninstall, and pass the base path for the registry key so other apps can use it with a different path.

What are the changes to CheckIfRegistryKeyExists for? Was it overwritting the registers?

Comment 58

[Jim Mathies [:jimm]]

(In reply to comment #57)
> > 
> >   ; Unregister resources associated with Win7 taskbar jump lists.
> >-  ApplicationID::UninstallJumpLists "${AppUserModelID}"
> >+  ApplicationID::UninstallJumpLists "$AppUserModelID"
> $AppUserModelID hasn't been set!
> 
> You'll need to set it in at least ShowShortcuts in shared.nsi as well...
> perhaps elsewhere. Basically, whenever you use $AppUserModelID you have to
> call GetShortcutAppModelID. You might want to just name it GetAppModelID,
> make it into a macro in common.nsh, make it support both install and
> uninstall, and pass the base path for the registry key so other apps can use
> it with a different path.

Can't I just add a 

Call GetShortcutAppModelID

above this to fix the problem?

> What are the changes to CheckIfRegistryKeyExists for? Was it overwritting
> the registers?

The registers weren't getting reset right on exit.

Comment 59

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

You also need it for ShowShortcuts and for this you would need un.GetShortcutAppModelID since it is in the uninstaller. Also, adding it to common.nsh would be cleaner for all apps and it would make it easier to maintain / make changes.

Comment 60

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

Should have said, that I haven't looked at all of the places that use $AppUserModelID so there may be others than ShowShortcuts

Comment 61

[Jim Mathies [:jimm]]

Actually, wouldn't it be better to move that Var declaration out of UpdateShortcutAppModelIDs and place it in GetHashAppModelId? I fail to see how the uninstall gets this defined correctly without a call to UpdateShortcutAppModelIDs, which I don't think it makes.

Then we just add the Call GetHashAppModelId before any use of the variable.

Comment 62

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

Makes sense and sounds good.

Comment 63

[Jim Mathies [:jimm]]

(In reply to comment #62)
> Makes sense and sounds good.

(In reply to comment #59)
> You also need it for ShowShortcuts and for this you would need
> un.GetShortcutAppModelID since it is in the uninstaller. Also, adding it to
> common.nsh would be cleaner for all apps and it would make it easier to
> maintain / make changes.

I can move GetShortcutAppModelID to common as well, in which case I would move the Var there vs. UpdateShortcutAppModelIDs.

Comment 64

[Jim Mathies [:jimm]]

(In reply to comment #63)
> (In reply to comment #62)
> > Makes sense and sounds good.
> 
> (In reply to comment #59)
> > You also need it for ShowShortcuts and for this you would need
> > un.GetShortcutAppModelID since it is in the uninstaller. Also, adding it to
> > common.nsh would be cleaner for all apps and it would make it easier to
> > maintain / make changes.
> 
> I can move GetShortcutAppModelID to common as well, in which case I would
> move the Var there vs. UpdateShortcutAppModelIDs.

Actually, to simplify this I'm just going to combine GetShortcutAppModelID & GetHashAppModelId into a single macro called InitHashAppModelId.

Comment 65

[Jim Mathies [:jimm]]

Attachment: installer changes v.6

Comment 66

[Jim Mathies [:jimm]]

Attachment: add profile option

Comment 67

[Jim Mathies [:jimm]]

Note since thunderbird doesn't define an AppVendor, I've modified the fallback model id ifdef in the common script to:

!ifdef AppVendor
!ifdef AppName
; Firefox
        StrCpy $AppUserModelID "${AppVendor}.${AppName}"
!endif
!else
; Thunderbird
!ifdef AppVersion
!ifdef AppName
        StrCpy $AppUserModelID "${AppName}.${AppVersion}"
!endif
!endif
!endif

Comment 68

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

(In reply to comment #67)
> Note since thunderbird doesn't define an AppVendor, I've modified the
> fallback model id ifdef in the common script to:
> 
> !ifdef AppVendor
> !ifdef AppName
> ; Firefox
>         StrCpy $AppUserModelID "${AppVendor}.${AppName}"
should be
${AppVendor}.${AppName}.${AppVersion}.Win64
#ifdef HAVE_64BIT_OS
; differentiate 64-bit builds, we do the same in widget.
!define AppUserModelID        "${AppVendor}.${AppName}.${AppVersion}.Win64"
#else
!define AppUserModelID        "${AppVendor}.${AppName}.${AppVersion}"
#endif

but since this file isn't preprocessed perhaps just leave the values in defines.nsi.in and use ${AppUserModelID} for the fallback?

> !endif
> !else
> ; Thunderbird
> !ifdef AppVersion
> !ifdef AppName
>         StrCpy $AppUserModelID "${AppName}.${AppVersion}"
> !endif
> !endif
> !endif
You can do the same for Thunderbird.

Comment 69

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

Might be good to change $AppUserModelID to something more distinguishable from ${AppUserModelID} though.

Comment 70

[Jim Mathies [:jimm]]

(In reply to comment #69)
> Might be good to change $AppUserModelID to something more distinguishable
> from ${AppUserModelID} though.

Hmm, do we really want AppUserModelID defined in defines when it's basically never used at all? I suppose I could change the defines value to ModelIdFallback or something but it just seems simpler to bury this in some ifdef'ing down in the common script that uses it.

Comment 71

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

I also don't like this file using ${AppName}.${AppVersion} directly and using ${AppUserModelID} instead is no worse as I see it. I'd really like to not have any of those used by common.nsh since it sets up a dependency on the application files when it should just be passed in by the application itself... let me finish reviewing the patch and I'll figure it out.

Comment 72

[Jim Mathies [:jimm]]

(In reply to comment #67)
> Note since thunderbird doesn't define an AppVendor, I've modified the
> fallback model id ifdef in the common script to:
> 
> !ifdef AppVendor
> !ifdef AppName
> ; Firefox
>         StrCpy $AppUserModelID "${AppVendor}.${AppName}"
> !endif
> !else
> ; Thunderbird
> !ifdef AppVersion
> !ifdef AppName
>         StrCpy $AppUserModelID "${AppName}.${AppVersion}"
> !endif
> !endif
> !endif

Just an FYI, I've been testing the widget code with thunderbird and I want to drop the overhead of getting the version, so in the rare case this fails, for tb I'll be using just the AppName.

Comment 73

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

Out of curiosity, what is the overhead? I ask because you get the app name from XULAppInfo which also has the version.

Comment 74

[Jim Mathies [:jimm]]

(In reply to comment #73)
> Out of curiosity, what is the overhead? I ask because you get the app name
> from XULAppInfo which also has the version.

Basically this is just a bunch of code in widget that never executes.

Comment 75

[Jim Mathies [:jimm]]

(In reply to comment #74)
> (In reply to comment #73)
> > Out of curiosity, what is the overhead? I ask because you get the app name
> > from XULAppInfo which also has the version.
> 
> Basically this is just a bunch of code in widget that never executes.

The code block as it stands now looks like this:

http://pastebin.mozilla.org/1229873

Comment 76

[Jim Mathies [:jimm]]

Attachment: installer changes v.7

per our discussion on irc, this ditches the fallback and instead skips off the registration calls when there's no id set in the var. I'll post an updated widget patch as well.

Comment 77

[Jim Mathies [:jimm]]

Attachment: widget changes v.3

Comment 78

[Jim Mathies [:jimm]]

Attachment: seamonkey patch

Comment 79

[Jim Mathies [:jimm]]

Attachment: thunderbird patch

Comment 80

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

Comment on attachment 533739 [details] [diff] [review]
installer changes v.7

>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
>@@ -224,23 +226,32 @@ Section "Uninstall"
>   ${MUI_INSTALLOPTIONS_READ} $0 "unconfirm.ini" "Field 3" "State"
>   ${If} "$0" == "1"
>     ${un.DeleteRelativeProfiles} "Mozilla\Firefox"
>     RmDir "$APPDATA\Mozilla\Extensions\{ec8030f7-c20a-464f-9b0e-13a3a9e97384}"
>     RmDir "$APPDATA\Mozilla\Extensions"
>     RmDir "$APPDATA\Mozilla"
>   ${EndIf}
> 
>+  ; setup the application model id registration value
>+  ${un.InitHashAppModelId} "$INSTDIR" "Software\Mozilla\${AppName}\TaskBarIDs"
>+
>   SetShellVarContext current  ; Set SHCTX to HKCU
>   ${un.RegCleanMain} "Software\Mozilla"
>   ${un.RegCleanUninstall}
>   ${un.DeleteShortcuts}
> 
>   ; Unregister resources associated with Win7 taskbar jump lists.
>-  ApplicationID::UninstallJumpLists "${AppUserModelID}"
>+  ${If} "$AppUserModelID" != ""
>+    ApplicationID::UninstallJumpLists "$AppUserModelID"
>+  ${EndIf}
Please add a ${If} ${AtLeastWin7} check here too.
 
>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
>@@ -4769,16 +4768,17 @@
> !macro UninstallOnInitCommon
> 
>   !ifndef UninstallOnInitCommon
>     !insertmacro ElevateUAC
>     !insertmacro GetLongPath
>     !insertmacro GetOptions
>     !insertmacro GetParameters
>     !insertmacro GetParent
>+    !insertmacro InitHashAppModelId
>     !insertmacro UnloadUAC
>     !insertmacro UpdateShortcutAppModelIDs
>     !insertmacro UpdateUninstallLog
> 
>     !verbose push
>     !verbose ${_MOZFUNC_VERBOSE}
>     !define UninstallOnInitCommon "!insertmacro UninstallOnInitCommonCall"
> 
>@@ -4822,21 +4822,30 @@
>       !ifdef HAVE_64BIT_OS
>         SetRegView 64
>       !endif
> 
>       ${GetParameters} $R0
> 
>       StrCmp "$R0" "" continue +1
> 
>+      ; We need this set up for most of the helper.exe operations below.
>+      !ifdef AppName
>+      ${InitHashAppModelId} "$INSTDIR" "Software\Mozilla\${AppName}\TaskBarIDs"
>+      !else
>+      ${InitHashAppModelId} "$INSTDIR" "Software\Mozilla\Generic\TaskBarIDs"
>+      !endif
Please move this to Function .onInit in uninstaller.nsi and get rid of the ifdef and Software\Mozilla\Generic\TaskBarIDs. I don't want the common code creating generic registry entries for applications especially since the application won't know that it should remove it on uninstall.

Also, just remove the !insertmacro InitHashAppModelId since it is being moved to uninstaller.nsi and it is already added there

>+
>       ; Update this user's shortcuts with the latest app user model id.
>       ClearErrors
>       ${GetOptions} "$R0" "/UpdateShortcutAppUserModelIds" $R2
>       IfErrors hideshortcuts +1
>-      ${UpdateShortcutAppModelIDs}  "$INSTDIR\${FileMainEXE}" "${AppUserModelID}" $R2
>+      ${If} "$AppUserModelID" != ""
>+        ${UpdateShortcutAppModelIDs}  "$INSTDIR\${FileMainEXE}" "$AppUserModelID" $R2
>+      ${EndIf}
>       StrCmp "$R2" "true" finish +1 ; true indicates that shortcuts have been updated
Since you don't call UpdateShortcutAppModelIDs unless $AppUserModelID is not an empty string you need to flip the logic here as follows:
StrCmp "$R2" "false" +1 finish

btw: is there any reason CityHash wouldn't work on Win2K and above? I'm thinking about using it to create unique uninstall registry key names.

Looks pretty good so far... just need to finish up reviewing the InitHashAppModelId macro

Comment 81

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

>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
>...
>       ${GetOptions} "$R0" "/UpdateShortcutAppUserModelIds" $R2
>       IfErrors hideshortcuts +1
>-      ${UpdateShortcutAppModelIDs}  "$INSTDIR\${FileMainEXE}" "${AppUserModelID}" $R2
>+      ${If} "$AppUserModelID" != ""
>+        ${UpdateShortcutAppModelIDs}  "$INSTDIR\${FileMainEXE}" "$AppUserModelID" $R2
>+      ${EndIf}
So, this can be changed to
!ifmacrodef InitHashAppModelId
  ${If} "$AppUserModelID" != ""
    ${UpdateShortcutAppModelIDs}  "$INSTDIR\${FileMainEXE}" "$AppUserModelID" $R2
  ${EndIf}
!endif

You will need to have !insertmacro InitHashAppModelId before !insertmacro UninstallOnInitCommon in uninstaller.nsi for this to work so please add a comment in uninstaller.nsi

Comment 82

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

Comment on attachment 533739 [details] [diff] [review]
installer changes v.7

>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
>...
>@@ -6670,8 +6679,121 @@
>   !verbose push
>   !verbose ${_MOZFUNC_VERBOSE}
>   Push "${_APP_ID}"
>   Push "${_EXE_PATH}"
>   Call UpdateShortcutAppModelIDs
>   Pop ${_RESULT}
>   !verbose pop
> !macroend
>+
>+/**
>+ * Retrieve if present or generate and store a 64 bit hash of an install path
>+ * using the City Hash algorithm.  On return the resulting id is saved in the
>+ * $AppUserModelID variable declared by inserting this macro. InitHashAppModelId
>+ * will attempt to load from HKLM/_REG_PATH first, then HKCU/_REG_PATH. If found
>+ * in either it will return the hash it finds. If not found it will generate a
>+ * new hash and attempt to store the hash in HKLM/_REG_PATH, then HKCU/_REG_PATH.
>+ * Subsequent calls will then retreive the stored hash value. On any failure,
>+ * $AppUserModelID will be set to "${AppVendor}.${AppName}" if these defines are
>+ * present or an empty string if they are not.
This last sentence appears to be bogus now

>+ *
>+ * Registry format: root/_REG_PATH/"_EXE_PATH" = "hash"
>+ *
>+ * @param   _EXE_PATH
>+ *          The main application executable path
>+ * @param   _REG_PATH
>+ *          The HKLM/HKCU agnostic registry path where the key hash should
>+ *          be stored. ex: "Software\Mozilla\Firefox\TaskBarIDs"
>+ * @result  (Var) $AppUserModelID contains the app model id.
>+ */
>+!macro InitHashAppModelId
>+  !ifndef ${_MOZFUNC_UN}InitHashAppModelId
>+    !define _MOZFUNC_UN_TMP ${_MOZFUNC_UN}
>+    !insertmacro ${_MOZFUNC_UN_TMP}GetLongPath
>+    !undef _MOZFUNC_UN
>+    !define _MOZFUNC_UN ${_MOZFUNC_UN_TMP}
>+    !undef _MOZFUNC_UN_TMP
>+    !ifndef InitHashAppModelId
>+    Var AppUserModelID
please indent... would also prefer if there were a newline before the !ifndef and after the !endif so it is easier to see (see other macros for examples)

>+    !endif
>+    !verbose push
>+    !verbose ${_MOZFUNC_VERBOSE}
>+    !define ${_MOZFUNC_UN}InitHashAppModelId "!insertmacro ${_MOZFUNC_UN}InitHashAppModelIdCall"
>+
>+    Function ${_MOZFUNC_UN}InitHashAppModelId
>+      ; stack: apppath, regpath
>+      Exch $R9 ; stack: $R9, regpath | $R9 = apppath
>+      Exch 1   ; stack: regpath, $R9
>+      Exch $R8 ; stack: $R8, $R9   | $R8 = regpath
You also need to push $R7 since you use it below

>+
>+      ${If} ${AtLeastWin7}
>+        ${${_MOZFUNC_UN}GetLongPath} "$R9" $R9
>+        ClearErrors
>+        ReadRegStr $R7 HKLM "$R8" "$R9"
>+        ${If} ${Errors}
>+          ClearErrors
>+          ReadRegStr $R7 HKCU "$R8" "$R9"
>+          ${If} ${Errors}
>+            ; If it doesn't exist, create a new one and store it
>+            CityHash::GetCityHash64 "$R9"
>+            Pop $AppUserModelID
>+            ${If} $AppUserModelID == "error"
>+              GoTo end
>+            ${EndIf}
>+            ClearErrors
>+            WriteRegStr HKLM "$R8" "$R9" "$AppUserModelID"
>+            ${If} ${Errors}
>+              WriteRegStr HKLM "$R8" "$R9" "$AppUserModelID"
>+              ${If} ${Errors}
>+                StrCpy $AppUserModelID "error"
>+              ${EndIf}
>+            ${EndIf}
>+          ${EndIf}
>+        ${EndIf}
>+      ${EndIf}
>+
>+      end:
>+      ${If} "$AppUserModelID" == "error"
>+        StrCpy $AppUserModelID ""
>+      ${EndIf}
>+
>+      ClearErrors
You also need to Pop $R7 since you use it above

>+      Pop $R8
>+      Pop $R9
This should be
Exch $R8
Exch 1
Exch $R9

Comment 83

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

Comment on attachment 533739 [details] [diff] [review]
installer changes v.7

>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
>...
>@@ -6670,8 +6679,121 @@
>...
>+
>+      ${If} ${AtLeastWin7}
>+        ${${_MOZFUNC_UN}GetLongPath} "$R9" $R9
I like that you are erring on the side of caution here :)

>+        ClearErrors
>+        ReadRegStr $R7 HKLM "$R8" "$R9"
>+        ${If} ${Errors}
>+          ClearErrors
>+          ReadRegStr $R7 HKCU "$R8" "$R9"
>+          ${If} ${Errors}
>+            ; If it doesn't exist, create a new one and store it
>+            CityHash::GetCityHash64 "$R9"
>+            Pop $AppUserModelID
>+            ${If} $AppUserModelID == "error"
>+              GoTo end
>+            ${EndIf}
>+            ClearErrors
>+            WriteRegStr HKLM "$R8" "$R9" "$AppUserModelID"
>+            ${If} ${Errors}
You need a ClearErrors here!

>+              WriteRegStr HKLM "$R8" "$R9" "$AppUserModelID"
>+              ${If} ${Errors}
>+                StrCpy $AppUserModelID "error"
>+              ${EndIf}
>+            ${EndIf}
>+          ${EndIf}
>+        ${EndIf}
>+      ${EndIf}

I'm fine with r+'ing this with the comments addressed. If the comments can't be addressed please request another review.

Comment 84

[Jim Mathies [:jimm]]

(In reply to comment #80)
> btw: is there any reason CityHash wouldn't work on Win2K and above? I'm
> thinking about using it to create unique uninstall registry key names.

I doubt anyone has tried to compile it on 2k, so maybe, maybe not. :)

Comment 85

[Jim Mathies [:jimm]]

Attachment: installer changes v.8

Updated per review comments. Testing so far shows everything working as expected.

Comment 86

[Jim Mathies [:jimm]]

Comment on attachment 533635 [details] [diff] [review]
add profile option

This adds a hash of the profile directory to the app user model id based on a hidden pref, and moves registration into widget so that user prefs are available at the time of the call.

Comment 87

[Jim Mathies [:jimm]]

(In reply to comment #86)
> Comment on attachment 533635 [details] [diff] [review] [review]
> add profile option
> 
> This adds a hash of the profile directory to the app user model id based on
> a hidden pref, and moves registration into widget so that user prefs are
> available at the time of the call.

Note that patch sits on top of "widget changes v.3".

Comment 88

[Jim Mathies [:jimm]]

Comment on attachment 533741 [details] [diff] [review]
widget changes v.3

Neil, this adds widget support for the installer changes made here. The installer will now be generating a taskbar id (a hash based on the install path) which it stores in the registry. This bit of code in widget looks for that id and if it finds it uses it to register with the windows taskbar. Prior to these changes we were using a version string when registering.

Comment 89

[[e-mail]]

Comment on attachment 533741 [details] [diff] [review]
widget changes v.3

Patch didn't apply locally; given the age of the patch, I'm assuming bitrot.

>-    AppendASCIItoUTF16(val, aDefaultGroupId);
>+      aDefaultGroupId.Append(buf);
Looks like you got lucky, and all of your callers pass in a new string. (Normally XPCOM methods should not assume that.)

>+  regKey.Append(NS_ConvertUTF8toUTF16(appName));
AppendUTF8toUTF16

>+        path[idx] = NULL; // no trailing slash
Did you mean '\0'?

>+  if (GetModuleFileNameW(NULL, path, MAX_PATH)) {
>+    for (int idx = wcslen(path); idx > 0; idx--) {
GetModuleFileNameW returns the length of the path.
Or you could use wcsrchr(path, L'\\')

Comment 90

[Rob Arnold [:robarnold]]

Comment on attachment 533635 [details] [diff] [review]
add profile option

Why did you move the call site for RegisterAppUserModelID? I thought this patch is still keeping all the windows in a process in the same group.

Also I've heard that there's a native Pref service cache somewhere so you don't need to do the do_GetService song & dance anymore.

Comment 91

[Jim Mathies [:jimm]]

(In reply to comment #90)
> Comment on attachment 533635 [details] [diff] [review] [review]
> add profile option
> 
> Why did you move the call site for RegisterAppUserModelID? I thought this
> patch is still keeping all the windows in a process in the same group.

User prefs weren't available at the earlier point in startup, so in adding the profile requirement I had to move it out such that prefs were loaded but no ui had been shown. 

> Also I've heard that there's a native Pref service cache somewhere so you
> don't need to do the do_GetService song & dance anymore.

Yep, your right. This was written before that landed. I'll look into using it.

Comment 92

[Jim Mathies [:jimm]]

Attachment: widget changes v.3

updated per comments.

Comment 93

[Jim Mathies [:jimm]]

Attachment: add profile option v.2

updated per comments.

Comment 94

[Jim Mathies [:jimm]]

Comment on attachment 544244 [details] [diff] [review]
widget changes v.3

rats sorry, got some mixed cruft in these.

Comment 95

[Jim Mathies [:jimm]]

Attachment: widget changes v.3

sorry for the bug spam.. cleaned up patch.

Comment 96

[Jim Mathies [:jimm]]

Attachment: add profile option v.2

Comment 97

[[e-mail]]

Comment on attachment 544250 [details] [diff] [review]
widget changes v.3

>+  PRUint32 length;
>+  if (length = GetModuleFileNameW(NULL, path, MAX_PATH)) {
>+    for (int idx = length; idx >= 0; idx--) {
Confusing mixture of types here. r=me with that fixed.

>+  if (length = GetModuleFileNameW(NULL, path, MAX_PATH)) {
>+    for (int idx = length; idx >= 0; idx--) {
>+      if (!idx) {
>+        return PR_FALSE;
>+      } else if (path[idx] == '\\') {
>+        path[idx] = '\0'; // no trailing slash
>+        break;
>+      }
>+    }
This looks confusing. (The else after return doesn't help.) Maybe you would be better off using wcsrchr; the best loop I could come up with was this:
int idx = GetModuleFileNameW(NULL, path, MAX_PATH);
do {
  if (!idx--)
    return PR_FALSE;
} while (path[idx] != '\\');
path[idx] = '\0'; // no trailing slash

Comment 98

[Jim Mathies [:jimm]]

final try builds with updates for test:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4067b216bcf8

these should be up in a few hours.

Comment 99

[Brian R. Bondy [:bbondy]]

What's remaining for this task to be pushed? Jim mentioned it might auto fix bug 691757.   The build link in comment 98 is down.

Comment 100

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #99)
> What's remaining for this task to be pushed? Jim mentioned it might auto fix
> bug 691757.   The build link in comment 98 is down.

I should have this landed this week or next. I need to update the patches for sm & tb and test everything.

Comment 101

[Jim Mathies [:jimm]]

Attachment: tbird install changes v.1

Pretty much a carbon copy of the changes made to fx's scripts.

Comment 102

[Jim Mathies [:jimm]]

Just noticed, looks like I should also add the AtLeastWin7 check to the function FixShortcutAppModelIDs. Since it's not called prior to calling that function. I'll add that to the set.

Comment 103

[Jim Mathies [:jimm]]

rstrong: review ping. (FWIW, the changes on the sm and tb side are pretty straight forward)

Comment 104

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

Comment on attachment 565309 [details] [diff] [review]
tbird install changes v.1

Not sure if I'm a peer for Thunderbird's Installer so verify with Mark Banner that my review is good enough.

Comment 105

[Jim Mathies [:jimm]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #104)
> Comment on attachment 565309 [details] [diff] [review] [diff] [details] [review]
> tbird install changes v.1
> 
> Not sure if I'm a peer for Thunderbird's Installer so verify with Mark
> Banner that my review is good enough.

Ok. I've got a duplicate patch for seamonkey too which I need to get approved by someone. Once I've got that I'll land these all at once.

Thanks!

Comment 106

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #105)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #104)
> > Comment on attachment 565309 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > tbird install changes v.1
> > 
> > Not sure if I'm a peer for Thunderbird's Installer so verify with Mark
> > Banner that my review is good enough.
> 
> Ok. I've got a duplicate patch for seamonkey too which I need to get
> approved by someone. Once I've got that I'll land these all at once.
> 
> Thanks!

Er, sorry, not sm, this patch is for suite.

Comment 107

[Jim Mathies [:jimm]]

Attachment: suite patch

Comment 108

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

callek or mcsmurf (might ask both to see which one gets to it first) would be a better choice for review.

Comment 109

[Robert Kaiser]

Comment on attachment 567911 [details] [diff] [review]
suite patch

I'm not a good reviewer for this, but either mcsmurf or Callek should be able to review this, thanks for the patch!

Comment 111

[Jim Mathies [:jimm]]

FYI - i'm going to go ahead and land this set up to and including the tbird patch today.

Comment 112

[Jim Mathies [:jimm]]

mozilla-inbound landings:

https://hg.mozilla.org/integration/mozilla-inbound/rev/52b9e8718829
https://hg.mozilla.org/integration/mozilla-inbound/rev/85f633edac87
https://hg.mozilla.org/integration/mozilla-inbound/rev/97082ded20b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcd87b30b18b
https://hg.mozilla.org/integration/mozilla-inbound/rev/edbec2d92d14

once this flips over to mc, I'll land the tb patch.

Comment 113

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/edbec2d92d14
https://hg.mozilla.org/mozilla-central/rev/dcd87b30b18b
https://hg.mozilla.org/mozilla-central/rev/97082ded20b8
https://hg.mozilla.org/mozilla-central/rev/85f633edac87
https://hg.mozilla.org/mozilla-central/rev/52b9e8718829

Comment 114

[Siddhartha Dugar [:sdrocking]]

When I open two profiles in Nightly (11/5), they still appear together. Is this expected ?

Comment 115

[Patrick Westerhoff]

Looking at the landed revisions, I’m unsure how this is supposed to work. Now Firefox looks into the registry to determine what AppUserModelID it should work, but to get the correct ID, it only uses the process path. How is that supposed to help when launching different profiles? Those would be still executed using the same executable, just with different command line options. But both the executable’s file name and especially the command line options (or profile selectors) are not respected when selecting the ID.

Oh, and btw. it seems that one of those `InitHashAppModelId` calls in rev 52b9e8718829 happens too early. I got a `C:\Program Files (x86)\Nightly` entry into my registry although that is not the path I installed Nightly into. I guess the macro runs once before the setup finds out where Nightly was previously installed.

Comment 116

[Jim Mathies [:jimm]]

For profile based separation, add a new bool pref - "taskbar.grouping.useprofile"(In reply to Patrick W. from comment #115)
> Looking at the landed revisions, I’m unsure how this is supposed to work.
> Now Firefox looks into the registry to determine what AppUserModelID it
> should work, but to get the correct ID, it only uses the process path. How
> is that supposed to help when launching different profiles? Those would be
> still executed using the same executable, just with different command line
> options. But both the executable’s file name and especially the command line
> options (or profile selectors) are not respected when selecting the ID.

For profile based separation, add a new bool pref - "taskbar.grouping.useprofile", and set it to true.

> Oh, and btw. it seems that one of those `InitHashAppModelId` calls in rev
> 52b9e8718829 happens too early. I got a `C:\Program Files (x86)\Nightly`
> entry into my registry although that is not the path I installed Nightly
> into. I guess the macro runs once before the setup finds out where Nightly
> was previously installed.

Hmm, sounds like a bug, I'll investigate this next week. Thanks for reporting.

Comment 117

[Jim Mathies [:jimm]]

comm-central push w/thunderbird patch:
http://hg.mozilla.org/comm-central/rev/7ec79765fce9

Comment 118

[Patrick Westerhoff]

(In reply to Jim Mathies [:jimm] from comment #116)
> For profile based separation, add a new bool pref -
> "taskbar.grouping.useprofile", and set it to true.

I’ve tried both `taskbar.grouping.userprofile` and `browser.taskbar.grouping.useprofile` (because all ohter taskbar preferences start with `browser` too) now, but it doesn’t seem to work. Are you sure this is how it is supposed to work?

Comment 119

[Jim Mathies [:jimm]]

(In reply to Patrick W. from comment #118)
> (In reply to Jim Mathies [:jimm] from comment #116)
> > For profile based separation, add a new bool pref -
> > "taskbar.grouping.useprofile", and set it to true.
> 
> I’ve tried both `taskbar.grouping.userprofile` and
> `browser.taskbar.grouping.useprofile` (because all ohter taskbar preferences
> start with `browser` too) now, but it doesn’t seem to work. Are you sure
> this is how it is supposed to work?

OOPS! I didn't get that patch in with my checkin. :) I'll land it today. Sorry about that.

Comment 120

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #119)
> (In reply to Patrick W. from comment #118)
> > (In reply to Jim Mathies [:jimm] from comment #116)
> > > For profile based separation, add a new bool pref -
> > > "taskbar.grouping.useprofile", and set it to true.
> > 
> > I’ve tried both `taskbar.grouping.userprofile` and
> > `browser.taskbar.grouping.useprofile` (because all ohter taskbar preferences
> > start with `browser` too) now, but it doesn’t seem to work. Are you sure
> > this is how it is supposed to work?
> 
> OOPS! I didn't get that patch in with my checkin. :) I'll land it today.
> Sorry about that.

Actually I did, but apparently there was a merge conflict I missed when updating. the Win taskbar chunk didn't make it.

Comment 121

[Jim Mathies [:jimm]]

Attachment: missing chunk

Need to test, then I'll land it directly on mc.

Comment 122

[Patrick Westerhoff]

Haha, okay, and I thought I did some mistake ;)

Might be a good moment then to change the preferences name to `browser.taskbar.grouping.useprofile` (i.e. with `browser` prefix) to match the other related preferences, or not?

Comment 123

[Rain [:rain1] (inactive)]

No, the other prefs are specific to Firefox. This one isn't.

Comment 124

[Patrick Westerhoff]

Ah I see, that makes sense. Thanks!

Different suggestion: Wouldn’t it be better to include the registry mechanism for the profile too? Maybe by having a registry key that is made up of both the executable path and the profile name? That way a user could still modify the appId, and we wouldn’t have to hash the profile path for every launch.

With an activated `taskbar.grouping.useprofile` setting, we could first look for a `PATH:PROFILENAME` registry key and if that doesn’t exist fall back to default and only use `PATH`. Or without a fallback to the current behaviour, simply create a `PATH:PROFILENAME` key when it doesn’t exist (like lazy hashing, to create it once). And then a user could easily change them to their liking (for example to have two profiles with the same ID, for whatever reason).

Comment 125

[Jim Mathies [:jimm]]

https://hg.mozilla.org/mozilla-central/rev/8200dc82b5ed

Comment 126

[Jim Mathies [:jimm]]

(In reply to Patrick W. from comment #124)
> Ah I see, that makes sense. Thanks!
> 
> Different suggestion: Wouldn’t it be better to include the registry
> mechanism for the profile too? Maybe by having a registry key that is made
> up of both the executable path and the profile name? That way a user could
> still modify the appId, and we wouldn’t have to hash the profile path for
> every launch.
> 
> With an activated `taskbar.grouping.useprofile` setting, we could first look
> for a `PATH:PROFILENAME` registry key and if that doesn’t exist fall back to
> default and only use `PATH`. Or without a fallback to the current behaviour,
> simply create a `PATH:PROFILENAME` key when it doesn’t exist (like lazy
> hashing, to create it once). And then a user could easily change them to
> their liking (for example to have two profiles with the same ID, for
> whatever reason).

I don't believe the installer has access to profile data, since on new installs the profile hasn't been created yet.

Feel free to file a bug though with your suggestions and we can take a look at it.

Comment 127

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

(In reply to Jim Mathies [:jimm] from comment #126)
> (In reply to Patrick W. from comment #124)
> > Ah I see, that makes sense. Thanks!
> > 
> > Different suggestion: Wouldn’t it be better to include the registry
> > mechanism for the profile too? Maybe by having a registry key that is made
> > up of both the executable path and the profile name? That way a user could
> > still modify the appId, and we wouldn’t have to hash the profile path for
> > every launch.
> > 
> > With an activated `taskbar.grouping.useprofile` setting, we could first look
> > for a `PATH:PROFILENAME` registry key and if that doesn’t exist fall back to
> > default and only use `PATH`. Or without a fallback to the current behaviour,
> > simply create a `PATH:PROFILENAME` key when it doesn’t exist (like lazy
> > hashing, to create it once). And then a user could easily change them to
> > their liking (for example to have two profiles with the same ID, for
> > whatever reason).
> 
> I don't believe the installer has access to profile data, since on new
> installs the profile hasn't been created yet.
Also, for new profiles after installation such as a second user on the system or a new / second profile for the user that performs the install. The installer wouldn't be able to do this and while the application could I doubt the extra complexity would be allowed into Firefox for this use case.

Comment 128

[Justin Wood (:Callek)]

Comment on attachment 567911 [details] [diff] [review]
suite patch

Robert Strong is a peer of the Mozilla Installer code, even if we don't call on him too often. Since he (seems to) have background on this bug he can review in place of me or frank as well. (I might be a week or two before I can get to it. But if it passes review I'll say here and now that it can have a+=me for aurora [10])

Comment 129

[Patrick Westerhoff]

Okay, current Nightly is built from rev 80010, and the change landed in 79888, so it should be available now, right?

And when I set `taskbar.grouping.useprofile` to true in one (or both) profiles, Firefox will build an appId from the profile path and Windows 7 should no longer group the windows, right? Well, if my thinking is correct, then it doesn’t work yet. I still end up with a combined icon for both profiles.

Comment 130

[Jim Mathies [:jimm]]

(In reply to Patrick W. from comment #129)
> Okay, current Nightly is built from rev 80010, and the change landed in
> 79888, so it should be available now, right?
> 
> And when I set `taskbar.grouping.useprofile` to true in one (or both)
> profiles, Firefox will build an appId from the profile path and Windows 7
> should no longer group the windows, right? Well, if my thinking is correct,
> then it doesn’t work yet. I still end up with a combined icon for both
> profiles.

Yes, this is something unexpected. I've filed bug 701098 to investigate.

Comment 131

[Patrick Westerhoff]

Thanks, guess I can leave this bug alone then (at least for now). I’ll think about filing an extra bug about my suggestion from above later. Anyway, thanks a lot for all the work on this ticket!

Comment 132

[Justin Wood (:Callek)]

Comment on attachment 567911 [details] [diff] [review]
suite patch

Its been a while, and I'm far behind. if you can get someone with at least win7 and also a reviewer of some sort for m-c or c-c to f+ I'll rs+ and review after-the-fact.

Comment 133

[Justin Wood (:Callek)]

Comment on attachment 567911 [details] [diff] [review]
suite patch

I have not tested, and my caveat on landing this is that *someone* does, even you is fine.

Would still love one of the roberts to review, but won't block on that.

Comment 134

[Will]

So has this fix been incorporated into any of the releases yet? Beta? Aurora? Nightly?

Comment 135

[Ed Morley [:emorley]]

The target milestone is Firefox 10, so this is already on the release channel.

Comment 136

[Will]

I just tried to have 2 separate Firefox icons pinned to the taskbar in Windows 7. No dice with Fx 10.0.2.

Comment 137

[david.pate]

I've now been able to pin 2 different Thunderbird 10 icons to the Windows 7 taskbar, but that was not easy at all.
So, I've first set to true`taskbar.grouping.useprofile`. 
Doing this, I was still not able to pin 2 icons from the shorcuts I have on my desktop to launch 2 different TB profiles. In that case, I had the option to attach the icon on the taskbar with the first profile, but detach the icon with the second profile.
To pin the 2 icons, I had to launch the 2 TB profiles, then I was allowed to attach the 2 different icons from the running applications.
But I also had to modify the shortcuts attached to the taskbar to change their names and the shortcut to add the parameters specifying which profile should be launched.

Comment 138

[Jim Mathies [:jimm]]

(In reply to david.pate from comment #137)
> Doing this, I was still not able to pin 2 icons from the shorcuts I have on
> my desktop to launch 2 different TB profiles.

See bug 633728 - we disabled setting app model id on desktop shortcuts a while ago due to desktop refresh issues. You have to pin running tasks to get separate icons.

Comment 139

[Will]

Ah, got it working now, thanks. I must have missed a few posts since I wasn't pinning the running tasks, nor did I create the new about:config boolean taskbar.grouping.useprofile. 

Finally I don't have to have the Beta version installed alongside the Release version to get separate pinned icons. For the most part it worked fine, but every time there was a new Release version it got complicated because there seemed to be a problem having a Release and a Beta installation using the same version number. Thanks everybody!

Comment 140

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

Comment on attachment 567911 [details] [diff] [review]
suite patch

Should be fine

Comment 141

[Ryan VanderMeulen [:RyanVM]]

Suite patch:
https://hg.mozilla.org/comm-central/rev/b2e89794570a

Comment 142

[Frank Wein [:mcsmurf]]

Comment on attachment 567911 [details] [diff] [review]
suite patch

I guess my review is not required anymore :(... sorry, I'll do better in the future!