Implementing --app-launcher install/uninstall flow.

Implement --app-launcher to register the Chrome App Launcher in Add/Remove Programs.

"setup.exe --app-launcher --multi-install" installs the Chrome App Host "with App Launcher experience". At the moment this means a presence in Add/Remove Programs and a modified AP value for the App Host ClientState key.

Subsequent CL will also create a shortcut on the task bar.

BUG=151626
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=165478

[huangs, 2012-10-24 22:59:30]

Here is the work-in-progress.  PTAL.  The main description describes in detail
what currently work and what currently don't.

[erikwright (departed), 2012-10-25 01:58:30]

On Wed, Oct 24, 2012 at 6:59 PM, <huangs@chromium.org> wrote:

> Reviewers: erikwright, benwells,
>
> Message:
> Here is the work-in-progress.  PTAL.  The main description describes in
> detail
> what currently work and what currently don't.
>
> Description:
> Implementing --app-launcher install/uninstall flow.
>
> Define the following commands:
> (A) setup.exe --multi-install --app-launcher
> (B) setup.exe --multi-install --app-host
> (~A) setup.exe --uninstall --multi-install --app-launcher
> (~B) setup.exe --uninstall --multi-install --app-host
> (L) chrome.exe --show-app-list
>
> *** Current behaviours ***
>
> Assuming only Chrome is installed, then (A) should do the following:
>   - Installs app_host.exe
>   - Creates a App Host (= App Launcher) link on Desktop to invoks (L)
>

This should invoke app_host.exe --show-app-list, which will delegate to
chrome.exe


>   - Creates an uninstall entry for Google Chrome App Host in "Program and
> Features" that invokes (~A)
>   - Changes the Google Update ClientState ap key to have "-applauncher"
>   - Creates Google Update ClientState key {FDA71E6F-AC4C-4a00-8B70-**
> 9958A68906BF}
>
> Assuming only Chrome is installed, then (B) should do the following:
>   - Installs app_host.exe
>   - Changes the Google Update ClientState ap key to have "-apphost"
>   - Creates Google Update ClientState key {FDA71E6F-AC4C-4a00-8B70-**
> 9958A68906BF}
>
> (~A) undoes (A); and (~B) undoes (B).
>   - Exception: Google Update ClientState key
> {FDA71E6F-AC4C-4a00-8B70-**9958A68906BF} lingers.
>
> (B) followed by (A) = (A) only.
>
> (A) followed by (B) = no effect (logs error on cannot downgrade to App
> Host).
>
> *** Untested behaviours ***
>
> (B) followed by (~A) = no effect.
>
> (A) followed by (~B) = no effect.
>
> *** Additional complexity to be considered later ***
>
> - Chrome install/uninstall.
> - Location of shortcuts.
>
>
> BUG=151626
>
>
> Please review this at
https://codereview.chromium.**org/11267023/<https://codereview.chromium.org/1...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>   M chrome/installer/mini_**installer/configuration.cc
>   M chrome/installer/setup/**install.cc
>   M chrome/installer/setup/setup_**main.cc
>   M chrome/installer/setup/**uninstall.cc
>   M chrome/installer/util/channel_**info.h
>   M chrome/installer/util/channel_**info.cc
>   M chrome/installer/util/chrome_**app_host_distribution.cc
>   M chrome/installer/util/chrome_**app_host_operations.cc
>   M chrome/installer/util/**installation_validator.cc
>   M chrome/installer/util/**installer_state.h
>   M chrome/installer/util/**installer_state.cc
>   M chrome/installer/util/master_**preferences.h
>   M chrome/installer/util/master_**preferences.cc
>   M chrome/installer/util/master_**preferences_constants.h
>   M chrome/installer/util/master_**preferences_constants.cc
>   M chrome/installer/util/util_**constants.h
>   M chrome/installer/util/util_**constants.cc
>
>
>

[erikwright (departed), 2012-10-25 02:23:03]

At first glance, looks pretty good.

I think you need to set some kind of application ID on the shortcuts you create.
Ben can probably point you at the right way to get that.

If it works properly in all the different scenarios, please add grt@ for
chrome/installer OWNERS.

Off the top of my head, test:

1) Nothing -> App Launcher -> Remove Launcher
2) Nothing -> App Host -> App Launcher -> Remove Launcher
3) Chrome -> App Launcher -> Remove Launcher -> Remove Chrome
4) App Launcher -> Chrome -> Remove Launcher
5) Chrome -> App Launcher -> Remove Chrome -> Remove App Launcher
6) System-Level Chrome -> App Launcher -> Remove Launcher

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

https://codereview.chromium.org/11267023/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install.cc:386: if (product.is_chrome_app_host()) {
needs to be app_host.exe, not chrome.exe

https://codereview.chromium.org/11267023/diff/1/chrome/installer/util/chrome_...
File chrome/installer/util/chrome_app_host_distribution.cc (right):

https://codereview.chromium.org/11267023/diff/1/chrome/installer/util/chrome_...
chrome/installer/util/chrome_app_host_distribution.cc:63: return L"A standalone
platform for Chrome apps.";
Needs to become a localizable resource.

(Also below);

https://codereview.chromium.org/11267023/diff/1/chrome/installer/util/install...
File chrome/installer/util/installer_state.cc (right):

https://codereview.chromium.org/11267023/diff/1/chrome/installer/util/install...
chrome/installer/util/installer_state.cc:134: if
(prefs.install_chrome_app_launcher() || prefs.install_chrome_app_host()) {
It seems OK to me just to simplify this by treating the presence of both flags
as being the same as "--app-launcher".

If that doesn't cause problems elsewhere, I'd do it just to remove some code
complexity.

[benwells, 2012-10-25 04:50:37]

On 2012/10/25 02:23:03, erikwright wrote:
> At first glance, looks pretty good.
> 
> I think you need to set some kind of application ID on the shortcuts you
create.
> Ben can probably point you at the right way to get that.
> 
Indeed you will! Take a look at
chrome/browser/ui/views/app_list/app_list_controller_win.cc.

> If it works properly in all the different scenarios, please add grt@ for
> chrome/installer OWNERS.
> 
> Off the top of my head, test:
> 
> 1) Nothing -> App Launcher -> Remove Launcher
> 2) Nothing -> App Host -> App Launcher -> Remove Launcher
> 3) Chrome -> App Launcher -> Remove Launcher -> Remove Chrome
> 4) App Launcher -> Chrome -> Remove Launcher
> 5) Chrome -> App Launcher -> Remove Chrome -> Remove App Launcher
> 6) System-Level Chrome -> App Launcher -> Remove Launcher
> 
>
https://codereview.chromium.org/11267023/diff/1/chrome/installer/setup/instal...
> File chrome/installer/setup/install.cc (right):
> 
>
https://codereview.chromium.org/11267023/diff/1/chrome/installer/setup/instal...
> chrome/installer/setup/install.cc:386: if (product.is_chrome_app_host()) {
> needs to be app_host.exe, not chrome.exe
> 
>
https://codereview.chromium.org/11267023/diff/1/chrome/installer/util/chrome_...
> File chrome/installer/util/chrome_app_host_distribution.cc (right):
> 
>
https://codereview.chromium.org/11267023/diff/1/chrome/installer/util/chrome_...
> chrome/installer/util/chrome_app_host_distribution.cc:63: return L"A
standalone
> platform for Chrome apps.";
> Needs to become a localizable resource.
> 
> (Also below);
> 
>
https://codereview.chromium.org/11267023/diff/1/chrome/installer/util/install...
> File chrome/installer/util/installer_state.cc (right):
> 
>
https://codereview.chromium.org/11267023/diff/1/chrome/installer/util/install...
> chrome/installer/util/installer_state.cc:134: if
> (prefs.install_chrome_app_launcher() || prefs.install_chrome_app_host()) {
> It seems OK to me just to simplify this by treating the presence of both flags
> as being the same as "--app-launcher".
> 
> If that doesn't cause problems elsewhere, I'd do it just to remove some code
> complexity.

I'm not sure if I'm a relevant reviewer for this as it is all in the installer,
feel free to remove me - I don't be offended.

Is there going to be some sort of util function added to Chrome to invoke this
later on?

[benwells, 2012-10-25 04:50:51]

Oops forgot this...

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

https://codereview.chromium.org/11267023/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install.cc:388: // TODO(huangs): Append
switches::kUserDataDir, perhaps?
I don't think you can put kUserDataDir here as you're in the installer. Do you
have any concept of overriding the user data dir?

[grt (UTC plus 2), 2012-10-25 14:46:51]

the part where the app host is either an app host or an app launcher seems
confusing to me, but maybe it's just because i'm lacking context.  maybe you can
fill me in f2f.

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

https://codereview.chromium.org/11267023/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install.cc:285:
original_app_host_state->channel().IsAppLauncher();
using the channel id like this will lead to problems.  look in the
uninstall_command() for the switch you care about instead.

https://codereview.chromium.org/11267023/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install.cc:368: void CreateOrUpdateShortcuts(const
InstallerState& installer_state,
this function was written to very specifically handle Chrome's shortcuts and may
not be appropriate for your use.  please consult with gab@ about the right way
to have your shortcut(s) created.

https://codereview.chromium.org/11267023/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install.cc:390:
string16(L"--").append(ASCIIToUTF16(::switches::kShowAppList)));
i don't like this hacky prepend.  please use CommandLine instead:
    CommandLine app_host_cmd(CommandLine::NO_PROGRAM);
    app_host_cmd.AppendSwitch(switches::kShowAppList);
    base_properties.set_arguments(app_host_cmd.GetCommandLineString());

https://codereview.chromium.org/11267023/diff/1/chrome/installer/setup/setup_...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/11267023/diff/1/chrome/installer/setup/setup_...
chrome/installer/setup/setup_main.cc:462: const installer::ChannelInfo&
channel_info(app_host_state->channel());
same here: don't use the channel info for this.  check uninstall_command() for
the switch you care about.

https://codereview.chromium.org/11267023/diff/1/chrome/installer/setup/setup_...
chrome/installer/setup/setup_main.cc:466: *status =
installer::HIGHER_VERSION_EXISTS;
please introduce a new code rather than overloading an existing one.

https://codereview.chromium.org/11267023/diff/1/chrome/installer/util/chrome_...
File chrome/installer/util/chrome_app_host_distribution.cc (right):

https://codereview.chromium.org/11267023/diff/1/chrome/installer/util/chrome_...
chrome/installer/util/chrome_app_host_distribution.cc:63: return L"A standalone
platform for Chrome apps.";
On 2012/10/25 02:23:03, erikwright wrote:
> Needs to become a localizable resource.
> 
> (Also below);

Is this ever built for branding=Chromium?

https://codereview.chromium.org/11267023/diff/1/chrome/installer/util/chrome_...
File chrome/installer/util/chrome_app_host_operations.cc (right):

https://codereview.chromium.org/11267023/diff/1/chrome/installer/util/chrome_...
chrome/installer/util/chrome_app_host_operations.cc:101: modified |=
channel_info->SetAppHost(false);
since |= is a bitwise operation, this is subtle.  i think it's safer and more
clear to go with:
    if (set && channel_info->SetAppHost(false))
      modified = true;

[huangs, 2012-10-29 21:15:16]

PTAL.  We decided to reduce the scope of this CL, and not create any shortcuts
for now (will do this in a separate CL).  However, I'm keeping some
shortcut-related refactoring changes.

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

https://codereview.chromium.org/11267023/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install.cc:285:
original_app_host_state->channel().IsAppLauncher();
On 2012/10/25 14:46:53, grt wrote:
> using the channel id like this will lead to problems.  look in the
> uninstall_command() for the switch you care about instead.

Done.

https://codereview.chromium.org/11267023/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install.cc:368: void CreateOrUpdateShortcuts(const
InstallerState& installer_state,
Decided to do shortcut creation as a separate CL.

https://codereview.chromium.org/11267023/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install.cc:386: if (product.is_chrome_app_host()) {
On 2012/10/25 02:23:03, erikwright wrote:
> needs to be app_host.exe, not chrome.exe

Done.  (Deferring to different CL).

https://codereview.chromium.org/11267023/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install.cc:388: // TODO(huangs): Append
switches::kUserDataDir, perhaps?
On 2012/10/25 04:50:51, benwells wrote:
> I don't think you can put kUserDataDir here as you're in the installer. Do you
> have any concept of overriding the user data dir?

Nope.  I was looking at
AppListController::GetAppListCommandLine()
and decided to worry about this later.  I should have just asked.  Removing.   
(Deferring to different CL).

https://codereview.chromium.org/11267023/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install.cc:390:
string16(L"--").append(ASCIIToUTF16(::switches::kShowAppList)));
Done.  However, note that this leads to 2 spaces, since
app_host_cmd.GetCommandLineString() would return
" --show-app-list", and another gets added later.    (Deferring to different
CL).

https://codereview.chromium.org/11267023/diff/1/chrome/installer/setup/setup_...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/11267023/diff/1/chrome/installer/setup/setup_...
chrome/installer/setup/setup_main.cc:462: const installer::ChannelInfo&
channel_info(app_host_state->channel());
On 2012/10/25 14:46:53, grt wrote:
> same here: don't use the channel info for this.  check uninstall_command() for
> the switch you care about.

Done.

https://codereview.chromium.org/11267023/diff/1/chrome/installer/setup/setup_...
chrome/installer/setup/setup_main.cc:466: *status =
installer::HIGHER_VERSION_EXISTS;
On 2012/10/25 14:46:53, grt wrote:
> please introduce a new code rather than overloading an existing one.

Not doing this anymore, so moot.

https://codereview.chromium.org/11267023/diff/1/chrome/installer/util/chrome_...
File chrome/installer/util/chrome_app_host_distribution.cc (right):

https://codereview.chromium.org/11267023/diff/1/chrome/installer/util/chrome_...
chrome/installer/util/chrome_app_host_distribution.cc:63: return L"A standalone
platform for Chrome apps.";
This has not been built for Chromium.

https://codereview.chromium.org/11267023/diff/1/chrome/installer/util/chrome_...
chrome/installer/util/chrome_app_host_distribution.cc:63: return L"A standalone
platform for Chrome apps.";
Will make this localizable once we create shortcuts.  Moot for this CL.

https://codereview.chromium.org/11267023/diff/1/chrome/installer/util/chrome_...
File chrome/installer/util/chrome_app_host_operations.cc (right):

https://codereview.chromium.org/11267023/diff/1/chrome/installer/util/chrome_...
chrome/installer/util/chrome_app_host_operations.cc:101: modified |=
channel_info->SetAppHost(false);
Done (taking note of short-circuiting).  Note that the caller
InstallerState::SetChannelFlags() uses "|=".  Should that be fixed, as well?

https://codereview.chromium.org/11267023/diff/1/chrome/installer/util/install...
File chrome/installer/util/installer_state.cc (right):

https://codereview.chromium.org/11267023/diff/1/chrome/installer/util/install...
chrome/installer/util/installer_state.cc:134: if
(prefs.install_chrome_app_launcher() || prefs.install_chrome_app_host()) {
On 2012/10/25 02:23:03, erikwright wrote:
> It seems OK to me just to simplify this by treating the presence of both flags
> as being the same as "--app-launcher".
> 
> If that doesn't cause problems elsewhere, I'd do it just to remove some code
> complexity.

Done. So --app-host + --app-launcher = --app-launcher; and if App Launcher is
installed, then --app-host becomes --app-launcher.

[grt (UTC plus 2), 2012-10-30 13:02:55]

lg

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

https://codereview.chromium.org/11267023/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install.cc:390:
string16(L"--").append(ASCIIToUTF16(::switches::kShowAppList)));
On 2012/10/29 21:15:16, huangs1 wrote:
> Done.  However, note that this leads to 2 spaces, since
> app_host_cmd.GetCommandLineString() would return
> " --show-app-list", and another gets added later.    (Deferring to different
> CL).

If the spaces cause no harm, just do the simple thing and leave them.

https://codereview.chromium.org/11267023/diff/1/chrome/installer/util/chrome_...
File chrome/installer/util/chrome_app_host_distribution.cc (right):

https://codereview.chromium.org/11267023/diff/1/chrome/installer/util/chrome_...
chrome/installer/util/chrome_app_host_distribution.cc:63: return L"A standalone
platform for Chrome apps.";
On 2012/10/29 21:15:16, huangs1 wrote:
> This has not been built for Chromium.

Is it possible to put in a safeguard so that it never will be built for
Chromium?

https://codereview.chromium.org/11267023/diff/1/chrome/installer/util/chrome_...
File chrome/installer/util/chrome_app_host_operations.cc (right):

https://codereview.chromium.org/11267023/diff/1/chrome/installer/util/chrome_...
chrome/installer/util/chrome_app_host_operations.cc:101: modified |=
channel_info->SetAppHost(false);
On 2012/10/29 21:15:16, huangs1 wrote:
> Done (taking note of short-circuiting).  Note that the caller
> InstallerState::SetChannelFlags() uses "|=".  Should that be fixed, as well?

I don't feel strongly that it should, although I regret writing it :-(

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/setup/ins...
File chrome/installer/setup/install.cc (right):

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/setup/ins...
chrome/installer/setup/install.cc:356: // |base_properties|: The basic
properties to set on every shortcut installed
please revert all shortcut-related changes if you're not doing them in this CL.

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

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/util/util...
chrome/installer/util/util_constants.cc:21: // Install Chrome App Launcher.
Please add to this comment explaining that, in your words, "--app-host +
--app-launcher = --app-launcher; and if App
Launcher is installed, then --app-host becomes --app-launcher."  But maybe a bit
more gracefully than that.

[erikwright (departed), 2012-10-30 14:20:39]

LG.

Is this really all that's required to get the Add/Remove registration? If so,
kudos to grt@ for the prior work to make this possible.

https://codereview.chromium.org/11267023/diff/1/chrome/installer/util/chrome_...
File chrome/installer/util/chrome_app_host_distribution.cc (right):

https://codereview.chromium.org/11267023/diff/1/chrome/installer/util/chrome_...
chrome/installer/util/chrome_app_host_distribution.cc:63: return L"A standalone
platform for Chrome apps.";
On 2012/10/25 14:46:53, grt wrote:
> On 2012/10/25 02:23:03, erikwright wrote:
> > Needs to become a localizable resource.
> > 
> > (Also below);
> 
> Is this ever built for branding=Chromium?

We don't do anything to prevent it, but there are a lot of steps between where
we are today and making this useful in Chromium.

https://codereview.chromium.org/11267023/diff/1/chrome/installer/util/chrome_...
chrome/installer/util/chrome_app_host_distribution.cc:63: return L"A standalone
platform for Chrome apps.";
On 2012/10/30 13:02:55, grt wrote:
> On 2012/10/29 21:15:16, huangs1 wrote:
> > This has not been built for Chromium.
> 
> Is it possible to put in a safeguard so that it never will be built for
> Chromium?

I spoke with grt, as long as the strings are internationalized (i.e., defined
separately and appropriately for Chrome and Chromium builds), it's not an issue.

If these strings are only required for the shortcuts CL, please remove them from
this one.

If they are required for the Add/Remove registration, please do the
internationalization.

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/setup/ins...
File chrome/installer/setup/install.cc (right):

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/setup/ins...
chrome/installer/setup/install.cc:356: // |base_properties|: The basic
properties to set on every shortcut installed
On 2012/10/30 13:02:55, grt wrote:
> please revert all shortcut-related changes if you're not doing them in this
CL.

Feel free to ask for help to split them into a separate CL without losing them.

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/util/chro...
File chrome/installer/util/chrome_app_host_distribution.cc (right):

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/util/chro...
chrome/installer/util/chrome_app_host_distribution.cc:52: // See
AppListController::GetAppModelId().
separate

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/util/chro...
chrome/installer/util/chrome_app_host_distribution.cc:68: return L"A standalone
platform for Chrome apps.";
Needs to be internationalized (moved to a resource file).

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/util/chro...
chrome/installer/util/chrome_app_host_distribution.cc:72: return L"A standalone
platform for Chrome apps.";
Ditto.

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/util/chro...
chrome/installer/util/chrome_app_host_distribution.cc:126: return true;
separate

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/util/chro...
File chrome/installer/util/chrome_app_host_operations.cc (right):

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/util/chro...
chrome/installer/util/chrome_app_host_operations.cc:100: if (set &&
channel_info->SetAppHost(false))
My intuition is that, if we are being asked to remove flags, we should remove
both.

If we are being asked to set flags, we should set the one that is appropriate
and un-set the other.

grt?

[grt (UTC plus 2), 2012-10-30 14:29:22]

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/util/chro...
File chrome/installer/util/chrome_app_host_operations.cc (right):

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/util/chro...
chrome/installer/util/chrome_app_host_operations.cc:100: if (set &&
channel_info->SetAppHost(false))
On 2012/10/30 14:20:39, erikwright wrote:
> My intuition is that, if we are being asked to remove flags, we should remove
> both.
> 
> If we are being asked to set flags, we should set the one that is appropriate
> and un-set the other.
> 
> grt?

Yes, good catch!

[gab, 2012-10-30 16:50:21]

I don't think this will work as far as showing up in Add/Remove Programs is
concerned.

To show up in Add/Remove programs you have to register in RegisteredApplications
(or maybe its something else but I thought that's trigger it).

Currently this is only done for product.is_chrome(), see call to
RegisterChromeOnMachine() in install.cc.

shell_util.cc has many subset of keys to register, some refactoring might be
needed to split only those you need (i.e. you don't need to be declared as
handling HTTP, you don't need a DelegateExecute, etc.); then you'll likely need
a new method in shell_util.cc to register only those... haven't put much thought
into this yet, but I'm surprised if this works as is..?

You could also get individual apps to show up in Add/Remove programs; let's
discuss this in person if this is a goal.

Cheers,
Gab

[erikwright (departed), 2012-10-30 16:53:12]

On 2012/10/30 16:50:21, gab wrote:
> I don't think this will work as far as showing up in Add/Remove Programs is
> concerned.
> 
> To show up in Add/Remove programs you have to register in
RegisteredApplications
> (or maybe its something else but I thought that's trigger it).
> 
> Currently this is only done for product.is_chrome(), see call to
> RegisterChromeOnMachine() in install.cc.
> 
> shell_util.cc has many subset of keys to register, some refactoring might be
> needed to split only those you need (i.e. you don't need to be declared as
> handling HTTP, you don't need a DelegateExecute, etc.); then you'll likely
need
> a new method in shell_util.cc to register only those... haven't put much
thought
> into this yet, but I'm surprised if this works as is..?
> 
> You could also get individual apps to show up in Add/Remove programs; let's
> discuss this in person if this is a goal.

Not a goal. Only the App Launcher will appear there.

> 
> Cheers,
> Gab

[grt (UTC plus 2), 2012-10-30 17:24:46]

On 2012/10/30 16:50:21, gab wrote:
> I don't think this will work as far as showing up in Add/Remove Programs is
> concerned.
> 
> To show up in Add/Remove programs you have to register in
RegisteredApplications
> (or maybe its something else but I thought that's trigger it).

RegisteredApplications is for the Default Programs control panel.  Add/Remove
programs shows what's listed in
...\Software\Microsoft\Windows\CurrentVersion\Uninstall (among other places, I
believe).

[huangs, 2012-10-30 17:46:55]

When I ran the new code, the uninstall entry DID appear, and appears to do the
uninstall successfully.

[gab, 2012-10-30 18:10:15]

On 2012/10/30 17:24:46, grt wrote:
> On 2012/10/30 16:50:21, gab wrote:
> > I don't think this will work as far as showing up in Add/Remove Programs is
> > concerned.
> > 
> > To show up in Add/Remove programs you have to register in
> RegisteredApplications
> > (or maybe its something else but I thought that's trigger it).
> 
> RegisteredApplications is for the Default Programs control panel.  Add/Remove
> programs shows what's listed in
> ...\Software\Microsoft\Windows\CurrentVersion\Uninstall (among other places, I
> believe).

Aaah I see, so I guess this is done independently by the installer then? (I
guess it must since this works for Chrome Frame...).

Sorry for my ignorance :)

[huangs, 2012-10-30 20:35:07]

PTAL.

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/setup/ins...
File chrome/installer/setup/install.cc (right):

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/setup/ins...
chrome/installer/setup/install.cc:356: // |base_properties|: The basic
properties to set on every shortcut installed
Will remove the "if".  The rest is just minor cleanup.  Will remove if you
insist.

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/util/chro...
File chrome/installer/util/chrome_app_host_distribution.cc (right):

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/util/chro...
chrome/installer/util/chrome_app_host_distribution.cc:52: // See
AppListController::GetAppModelId().
On 2012/10/30 14:20:39, erikwright wrote:
> separate

Done (adding TODO).

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/util/chro...
chrome/installer/util/chrome_app_host_distribution.cc:68: return L"A standalone
platform for Chrome apps.";
On 2012/10/30 14:20:39, erikwright wrote:
> Needs to be internationalized (moved to a resource file).

Done.

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/util/chro...
chrome/installer/util/chrome_app_host_distribution.cc:72: return L"A standalone
platform for Chrome apps.";
On 2012/10/30 14:20:39, erikwright wrote:
> Ditto.

Done.

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/util/chro...
chrome/installer/util/chrome_app_host_distribution.cc:126: return true;
On 2012/10/30 14:20:39, erikwright wrote:
> separate

Done.  (Added TODO).

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/util/chro...
File chrome/installer/util/chrome_app_host_operations.cc (right):

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/util/chro...
chrome/installer/util/chrome_app_host_operations.cc:100: if (set &&
channel_info->SetAppHost(false))
On 2012/10/30 14:29:22, grt wrote:
> On 2012/10/30 14:20:39, erikwright wrote:
> > My intuition is that, if we are being asked to remove flags, we should
remove
> > both.
> > 
> > If we are being asked to set flags, we should set the one that is
appropriate
> > and un-set the other.
> > 
> > grt?
> 
> Yes, good catch!

Indeed!  Done and shortened.

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

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/util/util...
chrome/installer/util/util_constants.cc:21: // Install Chrome App Launcher.
On 2012/10/30 13:02:55, grt wrote:
> Please add to this comment explaining that, in your words, "--app-host +
> --app-launcher = --app-launcher; and if App
> Launcher is installed, then --app-host becomes --app-launcher."  But maybe a
bit
> more gracefully than that.

Done.

[gab, 2012-10-30 21:30:39]

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/setup/ins...
File chrome/installer/setup/install.cc (right):

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/setup/ins...
chrome/installer/setup/install.cc:356: // |base_properties|: The basic
properties to set on every shortcut installed
On 2012/10/30 20:35:07, huangs1 wrote:
> Will remove the "if".  The rest is just minor cleanup.  Will remove if you
> insist.

I don't see how the changes in install.cc/uninstall.cc are at all
necessary/useful by themselves.

I'm sure other things motivated the changes, but the motivation should come
along with the change, no point doing this in this CL imo.

[huangs, 2012-10-30 21:38:39]

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/setup/ins...
File chrome/installer/setup/install.cc (right):

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/setup/ins...
chrome/installer/setup/install.cc:356: // |base_properties|: The basic
properties to set on every shortcut installed
I didn't want these trivial changes to add visual clutter when the real stuff
comes.  But oh well; removing them.

[huangs, 2012-10-30 21:59:33]

uninstall.cc and install.cc now excluded form this CL. PTAL.

[erikwright (departed), 2012-10-30 22:53:52]

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/setup/ins...
File chrome/installer/setup/install.cc (right):

https://codereview.chromium.org/11267023/diff/9001/chrome/installer/setup/ins...
chrome/installer/setup/install.cc:356: // |base_properties|: The basic
properties to set on every shortcut installed
On 2012/10/30 21:38:39, huangs1 wrote:
> I didn't want these trivial changes to add visual clutter when the real stuff
> comes.  But oh well; removing them.

If the changes are good refactorings that stand on their own, create a separate
CL for them alone. Don't bundle stuff into other CLs just because it's
convenient (it's not convenient for your reviewers).

It's also not convenient for you. For example, if something causes you to revert
this CL, you will now be reverting more code. And what if someone then starts
coding against your refactored API? Now your revert gets harder.

And you are more likely to have to revert the more code you change. Or more
likely to fail to commit due to a merge conflict, etc.

[grt (UTC plus 2), 2012-10-31 17:41:48]

lgtm

[erikwright (departed), 2012-10-31 19:41:12]

LGTM with a few nits.

http://codereview.chromium.org/11267023/diff/24001/chrome/app/chromium_string...
File chrome/app/chromium_strings.grd (right):

http://codereview.chromium.org/11267023/diff/24001/chrome/app/chromium_string...
chrome/app/chromium_strings.grd:221: Chromium App Launcher is a standalone
platform for Chrome apps.
Chrome -> Chromium

http://codereview.chromium.org/11267023/diff/24001/chrome/app/chromium_string...
chrome/app/chromium_strings.grd:556: Launch Chrome apps
Chromium

http://codereview.chromium.org/11267023/diff/24001/chrome/installer/util/chro...
File chrome/installer/util/chrome_app_host_distribution.cc (right):

http://codereview.chromium.org/11267023/diff/24001/chrome/installer/util/chro...
chrome/installer/util/chrome_app_host_distribution.cc:51: // TODO(huangs):
Override ChromeAppHostDistribution::GetBaseAppId().
I don't think a TODO is needed for this as part of this CL.

http://codereview.chromium.org/11267023/diff/24001/chrome/installer/util/chro...
chrome/installer/util/chrome_app_host_distribution.cc:107: return L"Uninstall
Chrome App Host";
internationalize

http://codereview.chromium.org/11267023/diff/24001/chrome/installer/util/chro...
chrome/installer/util/chrome_app_host_distribution.cc:127: // TODO(huangs):
return true.
Not needed at this point.

[huangs, 2012-10-31 20:49:31]

PTAL.

https://codereview.chromium.org/11267023/diff/24001/chrome/app/chromium_strin...
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/11267023/diff/24001/chrome/app/chromium_strin...
chrome/app/chromium_strings.grd:221: Chromium App Launcher is a standalone
platform for Chrome apps.
On 2012/10/31 19:41:13, erikwright wrote:
> Chrome -> Chromium

Done.

https://codereview.chromium.org/11267023/diff/24001/chrome/app/chromium_strin...
chrome/app/chromium_strings.grd:556: Launch Chrome apps
On 2012/10/31 19:41:13, erikwright wrote:
> Chromium

Done.

https://codereview.chromium.org/11267023/diff/24001/chrome/installer/util/chr...
File chrome/installer/util/chrome_app_host_distribution.cc (right):

https://codereview.chromium.org/11267023/diff/24001/chrome/installer/util/chr...
chrome/installer/util/chrome_app_host_distribution.cc:51: // TODO(huangs):
Override ChromeAppHostDistribution::GetBaseAppId().
On 2012/10/31 19:41:13, erikwright wrote:
> I don't think a TODO is needed for this as part of this CL.

Done.

https://codereview.chromium.org/11267023/diff/24001/chrome/installer/util/chr...
chrome/installer/util/chrome_app_host_distribution.cc:107: return L"Uninstall
Chrome App Host";
On 2012/10/31 19:41:13, erikwright wrote:
> internationalize

Done.

https://codereview.chromium.org/11267023/diff/24001/chrome/installer/util/chr...
chrome/installer/util/chrome_app_host_distribution.cc:127: // TODO(huangs):
return true.
On 2012/10/31 19:41:13, erikwright wrote:
> Not needed at this point.

Done.

[erikwright (departed), 2012-11-01 00:05:38]

LGTM. I modified the issue description to something more appropriate for the
commit logs.

I removed the following, but if there are known issues could you please create
bugs for them?
- Work out issues involving coexistence of Chrome and (L).
- Work out issues involving App Host install (H) and (L).

[huangs, 2012-11-01 03:55:03]

I found many issues, but that was when I only used ninja build (no manifests). 
Will tackle these after the basic stuff are done.

[commit-bot: I haz the power, 2012-11-01 03:55:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/huangs@chromium.org/11267023/37003

[commit-bot: I haz the power, 2012-11-01 08:53:03]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-01 19:10:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/huangs@chromium.org/11267023/37003

[commit-bot: I haz the power, 2012-11-01 19:55:07]

Change committed as 165478