Closed Bug 577867 Opened 14 years ago Closed 13 years ago

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

Categories

(Firefox :: Shell Integration, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 10

People

(Reporter: rod, Assigned: jimm)

References

(Blocks 1 open bug)

Details

Attachments

(10 files, 22 obsolete files)

34.09 KB, patch
Details | Diff | Splinter Review
7.08 KB, patch
Details | Diff | Splinter Review
22.75 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
11.61 KB, patch
Details | Diff | Splinter Review
22.59 KB, patch
Details | Diff | Splinter Review
7.77 KB, patch
neil
: review+
Details | Diff | Splinter Review
4.16 KB, patch
robarnold
: review+
Details | Diff | Splinter Review
14.76 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
13.19 KB, patch
Callek
: review+
robert.strong.bugs
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
2.13 KB, patch
Details | Diff | Splinter Review
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
Version: unspecified → Trunk
Blocks: win7support
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?
I would also like to confirm that this still occurs on Fx 4 RC1.
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?
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.
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 :)
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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
(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?
(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.
(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?
If you go with the hash option, bug 597911 should probably be wontfixed, since this change would make that one unnecessary.
(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.
(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!
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.
Blocks: 597911
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.
Attached file md5 licence (obsolete) —
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?
Assignee: nobody → jmathies
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..
Attachment #524457 - Flags: review?(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
Attachment #524457 - Flags: review?(gerv) → review-
(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.
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
(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.
Attached file google cityhash header (obsolete) —
Here's another prospect Gerv, w/a MIT license:

http://code.google.com/p/cityhash/
Attachment #524457 - Attachment is obsolete: true
Attachment #525423 - Flags: review?(gerv)
This looks great. It doesn't even need adding to about:licence.

Gerv
Attached patch cityhash 64 plugin (obsolete) — Splinter Review
Attachment #525423 - Attachment is obsolete: true
Attachment #525423 - Flags: review?(gerv)
Attached patch cityhash64 plugin src (obsolete) — Splinter Review
Attachment #525496 - Attachment is obsolete: true
Attached patch cityhash64 plugin src (obsolete) — Splinter Review
updates...
Attachment #525498 - Attachment is obsolete: true
Attached patch release dll build (obsolete) — Splinter Review
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.
Attached patch installer changes v.1 (obsolete) — Splinter Review
Attached patch cityhash64 plugin src (obsolete) — Splinter Review
- updated to unicode
- fixed sprintf, now uses I64 precision for output
Attachment #526741 - Attachment is obsolete: true
Attached patch release dll build (obsolete) — Splinter Review
Attachment #526743 - Attachment is obsolete: true
Attached patch installer changes v.2 (obsolete) — Splinter Review
- removed debug gunk
- fixed a few bugs after some testing
Attachment #526745 - Attachment is obsolete: true
Attached patch widget changes v.1 (obsolete) — Splinter Review
Attached patch installer changes v.3 (obsolete) — Splinter Review
more cleanup
Attachment #526822 - Attachment is obsolete: true
Attachment #526828 - Flags: review?(robert.bugzilla)
Attachment #526819 - Flags: review?(robert.bugzilla)
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?
Attachment #526819 - Flags: review?(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
(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 <danny_r_smith_2001@yahoo.co.nz>
>+ *
>+ *  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.
If that Windows compliant stdint.h is a problem, I can roll just the defines cityhash uses into the plugin header.
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.
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.
(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 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 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.
Attachment #526828 - Flags: review?(robert.bugzilla) → review-
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.
Attachment #526819 - Flags: review?(gerv)
(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.
Attached patch installer changes v.4 (obsolete) — Splinter Review
Attachment #526828 - Attachment is obsolete: true
Attachment #531164 - Flags: review?(robert.bugzilla)
Attached patch widget changes v.2 (obsolete) — Splinter Review
Attachment #526824 - Attachment is obsolete: true
(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
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 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
Attachment #531164 - Flags: review?(robert.bugzilla) → review-
- updated plugin src since we changed the entry point
Attachment #526819 - Attachment is obsolete: true
Attachment #526819 - Flags: review?(robert.bugzilla)
- new dll
Attachment #526821 - Attachment is obsolete: true
Attached patch installer changes v.5 (obsolete) — Splinter Review
- installer updates
Attachment #531164 - Attachment is obsolete: true
Attached patch installer changes v.5 (obsolete) — Splinter Review
- nix a testing message box
Attachment #531412 - Attachment is obsolete: true
Attachment #531419 - Flags: review?
Attachment #531419 - Flags: review? → review?(robert.bugzilla)
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 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?
Attachment #531419 - Flags: review?(robert.bugzilla) → review-
(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.
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.
Should have said, that I haven't looked at all of the places that use $AppUserModelID so there may be others than ShowShortcuts
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.
Makes sense and sounds good.
(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.
(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.
Attached patch installer changes v.6 (obsolete) — Splinter Review
Attachment #531419 - Attachment is obsolete: true
Attachment #533286 - Flags: review?(robert.bugzilla)
Attached patch add profile option (obsolete) — Splinter Review
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
(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.
Might be good to change $AppUserModelID to something more distinguishable from ${AppUserModelID} though.
(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.
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.
(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.
Out of curiosity, what is the overhead? I ask because you get the app name from XULAppInfo which also has the version.
(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.
(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
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.
Attachment #533286 - Attachment is obsolete: true
Attachment #533739 - Flags: review?(robert.bugzilla)
Attachment #533286 - Flags: review?(robert.bugzilla)
Attached patch widget changes v.3 (obsolete) — Splinter Review
Attachment #531166 - Attachment is obsolete: true
Attached patch seamonkey patchSplinter Review
Attached patch thunderbird patch (obsolete) — Splinter Review
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
>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 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 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.
Attachment #533739 - Flags: review?(robert.bugzilla) → review+
(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. :)
Updated per review comments. Testing so far shows everything working as expected.
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.
Attachment #533635 - Flags: review?(tellrob)
(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 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.
Attachment #533741 - Flags: review?(neil)
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'\\')
Attachment #533741 - Flags: review?(neil)
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.
(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.
Attachment #533635 - Flags: review?(tellrob) → review-
Attached patch widget changes v.3 (obsolete) — Splinter Review
updated per comments.
Attachment #533741 - Attachment is obsolete: true
Attachment #544244 - Flags: review?(neil)
Attached patch add profile option v.2 (obsolete) — Splinter Review
updated per comments.
Attachment #544245 - Flags: review?(tellrob)
Attachment #533635 - Attachment is obsolete: true
Comment on attachment 544244 [details] [diff] [review]
widget changes v.3

rats sorry, got some mixed cruft in these.
Attachment #544244 - Attachment is obsolete: true
Attachment #544244 - Flags: review?(neil)
Attachment #544245 - Attachment is obsolete: true
Attachment #544245 - Flags: review?(tellrob)
sorry for the bug spam.. cleaned up patch.
Attachment #544250 - Flags: review?(neil)
Attachment #544251 - Flags: review?(tellrob)
Attachment #544251 - Flags: review?(tellrob) → review+
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
Attachment #544250 - Flags: review?(neil) → review+
final try builds with updates for test:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jmathies@mozilla.com-4067b216bcf8

these should be up in a few hours.
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.
(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.
Blocks: 692295
Pretty much a carbon copy of the changes made to fx's scripts.
Attachment #533789 - Attachment is obsolete: true
Attachment #565309 - Flags: review?(robert.bugzilla)
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.
rstrong: review ping. (FWIW, the changes on the sm and tb side are pretty straight forward)
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.
Attachment #565309 - Flags: review?(robert.bugzilla) → review+
(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!
(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.
Attached patch suite patchSplinter Review
Attachment #567911 - Flags: review?(kairo)
callek or mcsmurf (might ask both to see which one gets to it first) would be a better choice for review.
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!
Attachment #567911 - Flags: review?(kairo)
Attachment #567911 - Flags: review?(bugzilla)
Attachment #567911 - Flags: review?(bugspam.Callek)
FYI - i'm going to go ahead and land this set up to and including the tbird patch today.
When I open two profiles in Nightly (11/5), they still appear together. Is this expected ?
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.
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.
Summary: No way to un-group separate instances on Windows 7 TaskBar (FF4b1) → No way to un-group separate instances on Windows 7 TaskBar
Depends on: 700267
comm-central push w/thunderbird patch:
http://hg.mozilla.org/comm-central/rev/7ec79765fce9
(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?
(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.
(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.
Attached patch missing chunkSplinter Review
Need to test, then I'll land it directly on mc.
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?
No, the other prefs are specific to Firefox. This one isn't.
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).
(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.
(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 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])
Attachment #567911 - Flags: review?(robert.bugzilla)
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.
Depends on: 701098
(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.
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 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 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.
Attachment #567911 - Flags: review?(bugspam.Callek) → review+
Depends on: 714800
No longer depends on: 714800
So has this fix been incorporated into any of the releases yet? Beta? Aurora? Nightly?
The target milestone is Firefox 10, so this is already on the release channel.
I just tried to have 2 separate Firefox icons pinned to the taskbar in Windows 7. No dice with Fx 10.0.2.
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.
(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.
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!
Depends on: 734628
Comment on attachment 567911 [details] [diff] [review]
suite patch

Should be fine
Attachment #567911 - Flags: review?(robert.bugzilla) → review+
Keywords: checkin-needed
Whiteboard: [c-n for suite patch to comm-central]
Attachment #567911 - Flags: checkin+
Suite patch:
https://hg.mozilla.org/comm-central/rev/b2e89794570a
No longer blocks: win7support, 692295, 597911, 653633
No longer depends on: 700267, 734628, 701098
Keywords: checkin-needed
Whiteboard: [c-n for suite patch to comm-central]
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!
Attachment #567911 - Flags: review?(bugzilla)
Blocks: 1017856
You need to log in before you can comment on or make changes to this bug.