Duplicate setup.exe into an identical executable and point Active Setup to that executable instead.

Duplicate setup.exe into an identical executable and point Active Setup to that executable instead.

Active Setup is a Windows "Run-once-for-each-user-at-next-login" mechanism which we use to install per-user shortcuts on system-level installs.

It currently points to setup.exe --configure-user-settings --...

Windows' clever heuristics detect that this is an installer solely based on the process name and tries to elevate (this is mitigated on Vista since we set requestedExecutionLevel="asInvoker" in setup's manifest; on XP however this results in a blocking "Install Program as Other User" dialog at log in...).

BUG=166473
TEST=
On Windows XP:
0) Have User A (admin) and User B (non-admin, logged out)
1) Install system-level Chrome from User A
2) Log in to User B
3) See that the per-user Quick Launch shortcut has been installed (and that you weren't prompted by the "Install Program as Other User" dialog).

More Active Setup testing can be done as per the TEST block on https://codereview.chromium.org/11465025/.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176101

[gab, 2013-01-08 21:36:26]

Sirs, can you both take a look?

Thanks!
Gab

[grt (UTC plus 2), 2013-01-09 14:13:15]

https://codereview.chromium.org/11733007/diff/1/chrome/installer/setup/instal...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/11733007/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install_worker.cc:1412:
list->AddCopyTreeWorkItem(setup_path.value(), active_setup_exe.value(),
AddActiveSetupWorkItems is called once for each product, whereas this copying of
setup.exe should only be done once for the whole install operation (only for the
binaries, I think). I think AddInstallerCopyTasks is the right place for this.

https://codereview.chromium.org/11733007/diff/1/chrome/installer/util/util_co...
File chrome/installer/util/util_constants.cc (right):

https://codereview.chromium.org/11733007/diff/1/chrome/installer/util/util_co...
chrome/installer/util/util_constants.cc:208: const wchar_t kActiveSetupExe[] =
L"as.exe";
This name is too short and obscure. How about using whatever name you'd like
setup.exe to have? chromesu.exe (8.3!)?

[gab, 2013-01-09 15:39:42]

See replies below.

https://codereview.chromium.org/11733007/diff/1/chrome/installer/setup/instal...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/11733007/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install_worker.cc:1412:
list->AddCopyTreeWorkItem(setup_path.value(), active_setup_exe.value(),
On 2013/01/09 14:13:15, grt wrote:
> AddActiveSetupWorkItems is called once for each product, whereas this copying
of
> setup.exe should only be done once for the whole install operation (only for
the
> binaries, I think). I think AddInstallerCopyTasks is the right place for this.

The early return at the top of this method takes care of this already no?

The reason I put it here is so that we only copy this if we are actually
registering Active Setup (i.e. system-level Chrome only).

https://codereview.chromium.org/11733007/diff/1/chrome/installer/util/util_co...
File chrome/installer/util/util_constants.cc (right):

https://codereview.chromium.org/11733007/diff/1/chrome/installer/util/util_co...
chrome/installer/util/util_constants.cc:208: const wchar_t kActiveSetupExe[] =
L"as.exe";
On 2013/01/09 14:13:15, grt wrote:
> This name is too short and obscure. How about using whatever name you'd like
> setup.exe to have? chromesu.exe (8.3!)?

sgtm :)!

[robertshield, 2013-01-09 15:51:59]

https://codereview.chromium.org/11733007/diff/7001/chrome/installer/setup/ins...
File chrome/installer/setup/install_worker.h (right):

https://codereview.chromium.org/11733007/diff/7001/chrome/installer/setup/ins...
chrome/installer/setup/install_worker.h:158: const FilePath& temp_path,
please document |temp_path| and |setup_path|

https://codereview.chromium.org/11733007/diff/7001/chrome/installer/util/util...
File chrome/installer/util/util_constants.cc (right):

https://codereview.chromium.org/11733007/diff/7001/chrome/installer/util/util...
chrome/installer/util/util_constants.cc:205: const wchar_t kActiveSetupExe[] =
L"chromesu.exe";
<bikeshed>That reads quickly as "chrome sux", consider renaming it to something
less likely to amuse the internets?</bikeshed>

[gab, 2013-01-09 18:47:52]

PTAL.

Thanks!
Gab

https://codereview.chromium.org/11733007/diff/7001/chrome/installer/setup/ins...
File chrome/installer/setup/install_worker.h (right):

https://codereview.chromium.org/11733007/diff/7001/chrome/installer/setup/ins...
chrome/installer/setup/install_worker.h:158: const FilePath& temp_path,
On 2013/01/09 15:51:59, robertshield wrote:
> please document |temp_path| and |setup_path|

N/A (removed new parameter).

https://codereview.chromium.org/11733007/diff/7001/chrome/installer/util/util...
File chrome/installer/util/util_constants.cc (right):

https://codereview.chromium.org/11733007/diff/7001/chrome/installer/util/util...
chrome/installer/util/util_constants.cc:205: const wchar_t kActiveSetupExe[] =
L"chromesu.exe";
On 2013/01/09 15:51:59, robertshield wrote:
> <bikeshed>That reads quickly as "chrome sux", consider renaming it to
something
> less likely to amuse the internets?</bikeshed>

Ok what about chrmstp.exe (i.e. chromesetup with no vowels....)?

[robertshield, 2013-01-09 18:53:14]

lg, one more nit

https://codereview.chromium.org/11733007/diff/7001/chrome/installer/util/util...
File chrome/installer/util/util_constants.cc (right):

https://codereview.chromium.org/11733007/diff/7001/chrome/installer/util/util...
chrome/installer/util/util_constants.cc:205: const wchar_t kActiveSetupExe[] =
L"chromesu.exe";
On 2013/01/09 18:47:52, gab wrote:
> On 2013/01/09 15:51:59, robertshield wrote:
> > <bikeshed>That reads quickly as "chrome sux", consider renaming it to
> something
> > less likely to amuse the internets?</bikeshed>
> 
> Ok what about chrmstp.exe (i.e. chromesetup with no vowels....)?

SGTM :)

https://codereview.chromium.org/11733007/diff/10001/chrome/installer/setup/in...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/11733007/diff/10001/chrome/installer/setup/in...
chrome/installer/setup/install_worker.cc:1417:
DCHECK(RequiresActiveSetup(installer_state));
Doesn't this check duplicate that done in the if statement immediately above?

[gab, 2013-01-09 18:56:35]

https://codereview.chromium.org/11733007/diff/10001/chrome/installer/setup/in...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/11733007/diff/10001/chrome/installer/setup/in...
chrome/installer/setup/install_worker.cc:1417:
DCHECK(RequiresActiveSetup(installer_state));
On 2013/01/09 18:53:14, robertshield wrote:
> Doesn't this check duplicate that done in the if statement immediately above?

Right, but as discussed with Greg, putting it here so that anyone eventually
calling this for another product would hit this if they fail to also modify
RequiresActiveSetup().

RequiresActiveSetup() itself though can't replace the above condition as
RequiresActiveSetup() is generic for the entire installer_state and
AddActiveSetupWorkItems() is called per-product.

[grt (UTC plus 2), 2013-01-09 18:57:45]

https://codereview.chromium.org/11733007/diff/10001/chrome/installer/setup/in...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/11733007/diff/10001/chrome/installer/setup/in...
chrome/installer/setup/install_worker.cc:113: bool RequiresActiveSetup(const
InstallerState& installer_state) {
did you consider making this a method on InstallerState?

https://codereview.chromium.org/11733007/diff/10001/chrome/installer/setup/in...
chrome/installer/setup/install_worker.cc:179: FilePath
active_setup_exe(installer_dir.Append(kActiveSetupExe));
suggested comment:
    // Make a copy of setup.exe with a different name so that Active Setup
    // doesn't require an admin on XP thanks to Application Compatibility.

[gab, 2013-01-09 19:35:10]

Done, thanks!

https://codereview.chromium.org/11733007/diff/10001/chrome/installer/setup/in...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/11733007/diff/10001/chrome/installer/setup/in...
chrome/installer/setup/install_worker.cc:113: bool RequiresActiveSetup(const
InstallerState& installer_state) {
On 2013/01/09 18:57:45, grt wrote:
> did you consider making this a method on InstallerState?

Arg...! Sorry for wasting your time, this is what I intended to do and somehow
didn't do...

Done.

https://codereview.chromium.org/11733007/diff/10001/chrome/installer/setup/in...
chrome/installer/setup/install_worker.cc:179: FilePath
active_setup_exe(installer_dir.Append(kActiveSetupExe));
On 2013/01/09 18:57:45, grt wrote:
> suggested comment:
>     // Make a copy of setup.exe with a different name so that Active Setup
>     // doesn't require an admin on XP thanks to Application Compatibility.

Done.

[grt (UTC plus 2), 2013-01-10 02:55:42]

lgtm!

[robertshield, 2013-01-10 03:12:17]

lgtm

[commit-bot: I haz the power, 2013-01-10 13:11:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/11733007/12004

[commit-bot: I haz the power, 2013-01-10 16:46:01]

Change committed as 176101