Remove Chrome's ProgID from filetype associations at uninstall.

Remove Chrome's ProgID from filetype associations at uninstall.

And notify the shell after Chrome is unregistered so that Chrome's icon disappears from things like icons on the destop.

BUG=149089
TEST=install Chrome, make it the default, uninstall it, run One Note, click a link.  IE should open.

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

[gab, 2012-09-19 16:11:16]

Comments below.

https://codereview.chromium.org/10946030/diff/4001/chrome/installer/setup/uni...
File chrome/installer/setup/uninstall.cc (right):

https://codereview.chromium.org/10946030/diff/4001/chrome/installer/setup/uni...
chrome/installer/setup/uninstall.cc:754: InstallUtil::ValueEquals
prog_id_pred(prog_id);
|prog_id| here could be suffixed and unsuffixed (based on the
|browser_entry_suffix| parameter). DeleteChromeRegistrationKeys can be called
with no suffix even when the install present is suffixed (see line 1064 in this
file). So that it's technically possible that this would remove registrations
that are there to make a system-level Chrome default for this user when
uninstalling a user-level install for that same user...

It probably makes sense to only do this extra uninstall step iff |remove_all|.

https://codereview.chromium.org/10946030/diff/4001/chrome/installer/setup/uni...
chrome/installer/setup/uninstall.cc:755: for (const wchar_t* const* filetype =
&ShellUtil::kFileAssociations[0];
I feel this is overly complicated to loop through an array, why not
for (int i = 0; ShellUtil::kFileAssociations[i] != NULL; ++i)
as per shell_util.cc:1339 ?

https://codereview.chromium.org/10946030/diff/4001/chrome/installer/setup/uni...
chrome/installer/setup/uninstall.cc:758:
parent_key.push_back(FilePath::kSeparators[0]);
If |parent_key| is always going to have the separator, why not make the
separator part of the base section (i.e. adding it before initializing
|base_length|)?

https://codereview.chromium.org/10946030/diff/4001/chrome/installer/setup/uni...
chrome/installer/setup/uninstall.cc:760:
InstallUtil::DeleteRegistryValueIf(root, parent_key.c_str(), L"",
optional: I don't like that this method takes wchar_t* only to assert they are
not NULL... I would prefer if it would take string16s, but this is beyond the
scope of this CL, so feel free to ignore :).

https://codereview.chromium.org/10946030/diff/4001/chrome/installer/setup/uni...
chrome/installer/setup/uninstall.cc:1064:
DeleteChromeRegistrationKeys(browser_dist, HKEY_CURRENT_USER, string16(),
here.

[grt (UTC plus 2), 2012-09-19 19:43:15]

PTAL

https://chromiumcodereview.appspot.com/10946030/diff/4001/chrome/installer/se...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/10946030/diff/4001/chrome/installer/se...
chrome/installer/setup/uninstall.cc:754: InstallUtil::ValueEquals
prog_id_pred(prog_id);
On 2012/09/19 16:11:16, gab wrote:
> |prog_id| here could be suffixed and unsuffixed (based on the
> |browser_entry_suffix| parameter). DeleteChromeRegistrationKeys can be called
> with no suffix even when the install present is suffixed (see line 1064 in
this
> file). So that it's technically possible that this would remove registrations
> that are there to make a system-level Chrome default for this user when
> uninstalling a user-level install for that same user...
> 
> It probably makes sense to only do this extra uninstall step iff |remove_all|.

Great catch.  Updated as discussed.

https://chromiumcodereview.appspot.com/10946030/diff/4001/chrome/installer/se...
chrome/installer/setup/uninstall.cc:755: for (const wchar_t* const* filetype =
&ShellUtil::kFileAssociations[0];
On 2012/09/19 16:11:16, gab wrote:
> I feel this is overly complicated to loop through an array, why not
> for (int i = 0; ShellUtil::kFileAssociations[i] != NULL; ++i)
> as per shell_util.cc:1339 ?

The size of the array isn't known here as it is in shell_util.cc.

https://chromiumcodereview.appspot.com/10946030/diff/4001/chrome/installer/se...
chrome/installer/setup/uninstall.cc:758:
parent_key.push_back(FilePath::kSeparators[0]);
On 2012/09/19 16:11:16, gab wrote:
> If |parent_key| is always going to have the separator, why not make the
> separator part of the base section (i.e. adding it before initializing
> |base_length|)?

Done.

https://chromiumcodereview.appspot.com/10946030/diff/4001/chrome/installer/se...
chrome/installer/setup/uninstall.cc:760:
InstallUtil::DeleteRegistryValueIf(root, parent_key.c_str(), L"",
On 2012/09/19 16:11:16, gab wrote:
> optional: I don't like that this method takes wchar_t* only to assert they are
> not NULL... I would prefer if it would take string16s, but this is beyond the
> scope of this CL, so feel free to ignore :).

Ignored.  It's common to call this fn with a literal.  I don't like forcing the
construction of strings when not necessary.

[gab, 2012-09-19 19:58:50]

LGTM w/ consideration of the suggested changes.

https://chromiumcodereview.appspot.com/10946030/diff/4001/chrome/installer/se...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/10946030/diff/4001/chrome/installer/se...
chrome/installer/setup/uninstall.cc:755: for (const wchar_t* const* filetype =
&ShellUtil::kFileAssociations[0];
On 2012/09/19 19:43:15, grt wrote:
> On 2012/09/19 16:11:16, gab wrote:
> > I feel this is overly complicated to loop through an array, why not
> > for (int i = 0; ShellUtil::kFileAssociations[i] != NULL; ++i)
> > as per shell_util.cc:1339 ?
> 
> The size of the array isn't known here as it is in shell_util.cc.

Hmmm, the suggested code doesn't depend on the size of the array...

https://chromiumcodereview.appspot.com/10946030/diff/9001/chrome/installer/se...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/10946030/diff/9001/chrome/installer/se...
chrome/installer/setup/uninstall.cc:621: BrowserDistribution* dist, HKEY root,
nit: move "HKEY root" to its own line here too.

https://chromiumcodereview.appspot.com/10946030/diff/9001/chrome/installer/se...
chrome/installer/setup/uninstall.cc:630: const FilePath
&target_path(installer_state.target_path());
|target_path| is only used once, consider inlining it below.

(otherwise => nit: s/FilePath &target_path/FilePath& target_path)

https://chromiumcodereview.appspot.com/10946030/diff/9001/chrome/installer/se...
chrome/installer/setup/uninstall.cc:758: (installer_state.level() ==
InstallerState::USER_LEVEL &&
Why not use "!installer_state.system_install()"? That call, on top of being
shorter, has the benefit of making the DCHECK calls to verify |level_| was
initialized I believe.

https://chromiumcodereview.appspot.com/10946030/diff/9001/chrome/installer/se...
File chrome/installer/setup/uninstall.h (right):

https://chromiumcodereview.appspot.com/10946030/diff/9001/chrome/installer/se...
chrome/installer/setup/uninstall.h:30: BrowserDistribution* dist, HKEY root,
nit: wrap "HKEY root" on its own line.

[grt (UTC plus 2), 2012-09-19 20:04:52]

tanx!

https://chromiumcodereview.appspot.com/10946030/diff/4001/chrome/installer/se...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/10946030/diff/4001/chrome/installer/se...
chrome/installer/setup/uninstall.cc:755: for (const wchar_t* const* filetype =
&ShellUtil::kFileAssociations[0];
On 2012/09/19 19:58:50, gab wrote:
> On 2012/09/19 19:43:15, grt wrote:
> > On 2012/09/19 16:11:16, gab wrote:
> > > I feel this is overly complicated to loop through an array, why not
> > > for (int i = 0; ShellUtil::kFileAssociations[i] != NULL; ++i)
> > > as per shell_util.cc:1339 ?
> > 
> > The size of the array isn't known here as it is in shell_util.cc.
> 
> Hmmm, the suggested code doesn't depend on the size of the array...

Oh, I see.  I read the suggestion as knowing arraysize.  Anyway, I like the
iterator approach rather than the indexing approach, so I'm keeping it.  :-)

https://chromiumcodereview.appspot.com/10946030/diff/9001/chrome/installer/se...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/10946030/diff/9001/chrome/installer/se...
chrome/installer/setup/uninstall.cc:621: BrowserDistribution* dist, HKEY root,
On 2012/09/19 19:58:50, gab wrote:
> nit: move "HKEY root" to its own line here too.

Indeed!  Done.

https://chromiumcodereview.appspot.com/10946030/diff/9001/chrome/installer/se...
chrome/installer/setup/uninstall.cc:630: const FilePath
&target_path(installer_state.target_path());
On 2012/09/19 19:58:50, gab wrote:
> |target_path| is only used once, consider inlining it below.
> 
> (otherwise => nit: s/FilePath &target_path/FilePath& target_path)

Done.

https://chromiumcodereview.appspot.com/10946030/diff/9001/chrome/installer/se...
chrome/installer/setup/uninstall.cc:758: (installer_state.level() ==
InstallerState::USER_LEVEL &&
On 2012/09/19 19:58:50, gab wrote:
> Why not use "!installer_state.system_install()"? That call, on top of being
> shorter, has the benefit of making the DCHECK calls to verify |level_| was
> initialized I believe.

Done.

[commit-bot: I haz the power, 2012-09-19 20:06:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grt@chromium.org/10946030/10002

[gab, 2012-09-19 20:17:04]

Whoopsy!

https://chromiumcodereview.appspot.com/10946030/diff/10002/chrome/installer/s...
File chrome/installer/setup/uninstall.h (right):

https://chromiumcodereview.appspot.com/10946030/diff/10002/chrome/installer/s...
chrome/installer/setup/uninstall.h:30: BrowserDistribution* dist, HKEY root,
Hmmm, looks like you forgot to wrap "HKEY root" here :)!

[grt (UTC plus 2), 2012-09-19 20:20:18]

thanks again

https://chromiumcodereview.appspot.com/10946030/diff/10002/chrome/installer/s...
File chrome/installer/setup/uninstall.h (right):

https://chromiumcodereview.appspot.com/10946030/diff/10002/chrome/installer/s...
chrome/installer/setup/uninstall.h:30: BrowserDistribution* dist, HKEY root,
On 2012/09/19 20:17:04, gab wrote:
> Hmmm, looks like you forgot to wrap "HKEY root" here :)!

Done.

[commit-bot: I haz the power, 2012-09-19 20:20:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grt@chromium.org/10946030/10003

[commit-bot: I haz the power, 2012-09-19 22:16:03]

Change committed as 157624

[gab, 2012-09-22 05:20:22]

Whoopsy, see comment below :(...

https://codereview.chromium.org/10946030/diff/10003/chrome/installer/setup/un...
File chrome/installer/setup/uninstall.cc (right):

https://codereview.chromium.org/10946030/diff/10003/chrome/installer/setup/un...
chrome/installer/setup/uninstall.cc:760: KEY_QUERY_VALUE).Valid())) {
Arg, I just tested this, it's wrong as it doesn't remove registrations on system
installs.

The condition is missing "|| installer_state.system_install()", but that would
be redundant so the condition below is equivalent I believe:

installer_state.system_install() ||
!suffix.empty() ||
!base::win::RegKey(HKEY_LOCAL_MACHINE, reg_prog_id.c_str(),
                              KEY_QUERY_VALUE).Valid())) {

i.e. Removing the keys when uninstalling a system-level install is always ok. It
is also ok when uninstalling a user-level install; except if the suffix is empty
and there is a system-level Chrome present.

Sorry for missing that in the original review :(...

[grt (UTC plus 2), 2012-09-22 12:09:09]

https://chromiumcodereview.appspot.com/10946030/diff/10003/chrome/installer/s...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/10946030/diff/10003/chrome/installer/s...
chrome/installer/setup/uninstall.cc:760: KEY_QUERY_VALUE).Valid())) {
On 2012/09/22 05:20:22, gab wrote:
> Arg, I just tested this, it's wrong as it doesn't remove registrations on
system
> installs.
> 
> The condition is missing "|| installer_state.system_install()", but that would
> be redundant so the condition below is equivalent I believe:
> 
> installer_state.system_install() ||
> !suffix.empty() ||
> !base::win::RegKey(HKEY_LOCAL_MACHINE, reg_prog_id.c_str(),
>                               KEY_QUERY_VALUE).Valid())) {
> 
> i.e. Removing the keys when uninstalling a system-level install is always ok.
It
> is also ok when uninstalling a user-level install; except if the suffix is
empty
> and there is a system-level Chrome present.
> 
> Sorry for missing that in the original review :(...

Facepalm.  I'll fix it.