Add support for --multi-install to mini_installer, fixed a memory leak, and r...

Add support for --multi-install to mini_installer, fixed a memory leak, fixed use of Win32 registry API, and refactored SetFullInstallerFlag so that it can be exercised in unit tests (tests to follow).

When --multi-install is not on command-line, "-full" is still added to the appropriate product "ap" value in official builds.  When --multi-install is present, "-full" is added to the multi installer's "ap" value unless a product is being migrated from single-install to multi-install, in which case it is put on the product's "ap" value as before.

BUG=61609
TEST=Manual for now: run mini_installer.exe in various configurations, inspecting "ap" values as you go.


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

[grt (UTC plus 2), 2010-12-15 19:50:50]

This change adds support for --multi-install to the mini_installer.  It handles
installs/upgrades for multi-install products as well as migrations from single-
to multi- of installed products.

[tommi (sloooow) - chröme, 2010-12-15 20:15:43]

lgtm.

http://codereview.chromium.org/5875003/diff/1/chrome/installer/mini_installer...
File chrome/installer/mini_installer/mini_installer.cc (right):

http://codereview.chromium.org/5875003/diff/1/chrome/installer/mini_installer...
chrome/installer/mini_installer/mini_installer.cc:132: bool StrContains(const
wchar_t* str, const wchar_t *sub_str) {
Use StrStr[I] instead?

wchar_t *
->
wchar_t*

http://codereview.chromium.org/5875003/diff/1/chrome/installer/mini_installer...
chrome/installer/mini_installer/mini_installer.cc:165: return
SafeStrCopy(client_state_key, _countof(client_state_key),
chrome code typically uses arraysize().  Can we use that here?

http://codereview.chromium.org/5875003/diff/1/chrome/installer/mini_installer...
chrome/installer/mini_installer/mini_installer.cc:200: //    a ClientState key
in the registry.  Only it need be modified.
is there a 4? :

Two products are installed, one in multi, the other single.  The single will be
migrated as in case 3, and the multi will be upgraded as in case 2.

?

http://codereview.chromium.org/5875003/diff/1/chrome/installer/mini_installer...
chrome/installer/mini_installer/mini_installer.cc:228: if (key == 0) {
key == NULL

[tommi (sloooow) - chröme, 2010-12-15 20:17:28]

lgtm++ (one more nit)

http://codereview.chromium.org/5875003/diff/1/chrome/installer/mini_installer...
File chrome/installer/mini_installer/mini_installer.cc (right):

http://codereview.chromium.org/5875003/diff/1/chrome/installer/mini_installer...
chrome/installer/mini_installer/mini_installer.cc:178: wchar_t value[128];
zero initialize

[robertshield, 2010-12-15 20:25:44]

LGTM, nits:

http://codereview.chromium.org/5875003/diff/1/chrome/installer/mini_installer...
File chrome/installer/mini_installer/mini_installer.cc (right):

http://codereview.chromium.org/5875003/diff/1/chrome/installer/mini_installer...
chrome/installer/mini_installer/mini_installer.cc:132: bool StrContains(const
wchar_t* str, const wchar_t *sub_str) {
delete me (me as in this function, not me as in Robert).

http://codereview.chromium.org/5875003/diff/1/chrome/installer/mini_installer...
chrome/installer/mini_installer/mini_installer.cc:169: ERROR_SUCCESS;
For readability, please include () around the last condition, as in:

[...]
(::RegOpenKeyEx(...) == ERROR_SUCCESS);

http://codereview.chromium.org/5875003/diff/1/chrome/installer/mini_installer...
File chrome/installer/mini_installer/mini_installer.h (right):

http://codereview.chromium.org/5875003/diff/1/chrome/installer/mini_installer...
chrome/installer/mini_installer/mini_installer.h:28: // Google Update will use
full installer if this suffix is found in ap value.
grammar nits:
Google Update will use *the* full installer if this suffix is found in *the* ap
value.

[grt (UTC plus 2), 2010-12-16 05:59:13]

PTAL: I addressed all comments and fixed a number of problems with the way the
registry API was being used.

http://codereview.chromium.org/5875003/diff/1/chrome/installer/mini_installer...
File chrome/installer/mini_installer/mini_installer.cc (right):

http://codereview.chromium.org/5875003/diff/1/chrome/installer/mini_installer...
chrome/installer/mini_installer/mini_installer.cc:132: bool StrContains(const
wchar_t* str, const wchar_t *sub_str) {
On 2010/12/15 20:15:43, tommi wrote:
> Use StrStr[I] instead?
> 
> wchar_t *
> ->
> wchar_t*

Replaced with a call to StrStr, thanks.

http://codereview.chromium.org/5875003/diff/1/chrome/installer/mini_installer...
chrome/installer/mini_installer/mini_installer.cc:132: bool StrContains(const
wchar_t* str, const wchar_t *sub_str) {
On 2010/12/15 20:25:45, robertshield wrote:
> delete me (me as in this function, not me as in Robert).

Done.

http://codereview.chromium.org/5875003/diff/1/chrome/installer/mini_installer...
chrome/installer/mini_installer/mini_installer.cc:165: return
SafeStrCopy(client_state_key, _countof(client_state_key),
On 2010/12/15 20:15:43, tommi wrote:
> chrome code typically uses arraysize().  Can we use that here?

No due to dependency issues.

http://codereview.chromium.org/5875003/diff/1/chrome/installer/mini_installer...
chrome/installer/mini_installer/mini_installer.cc:169: ERROR_SUCCESS;
On 2010/12/15 20:25:45, robertshield wrote:
> For readability, please include () around the last condition, as in:
> 
> [...]
> (::RegOpenKeyEx(...) == ERROR_SUCCESS);

Done.

http://codereview.chromium.org/5875003/diff/1/chrome/installer/mini_installer...
chrome/installer/mini_installer/mini_installer.cc:178: wchar_t value[128];
On 2010/12/15 20:17:28, tommi wrote:
> zero initialize

The old code didn't, so I'm inclined not to add it.  Do you see a problem that
doing so would fix?

http://codereview.chromium.org/5875003/diff/1/chrome/installer/mini_installer...
chrome/installer/mini_installer/mini_installer.cc:200: //    a ClientState key
in the registry.  Only it need be modified.
On 2010/12/15 20:15:43, tommi wrote:
> is there a 4? :
> 
> Two products are installed, one in multi, the other single.  The single will
be
> migrated as in case 3, and the multi will be upgraded as in case 2.
> 
> ?

In this case, the single will only be migrated when --multi-install is added to
its Google Update config, which is case 3.

http://codereview.chromium.org/5875003/diff/1/chrome/installer/mini_installer...
chrome/installer/mini_installer/mini_installer.cc:228: if (key == 0) {
On 2010/12/15 20:15:43, tommi wrote:
> key == NULL

Done.

http://codereview.chromium.org/5875003/diff/1/chrome/installer/mini_installer...
File chrome/installer/mini_installer/mini_installer.h (right):

http://codereview.chromium.org/5875003/diff/1/chrome/installer/mini_installer...
chrome/installer/mini_installer/mini_installer.h:28: // Google Update will use
full installer if this suffix is found in ap value.
On 2010/12/15 20:25:45, robertshield wrote:
> grammar nits:
> Google Update will use *the* full installer if this suffix is found in *the*
ap
> value.

Done.

[tommi (sloooow) - chröme, 2010-12-16 06:32:25]

lgtm

http://codereview.chromium.org/5875003/diff/10001/chrome/installer/mini_insta...
File chrome/installer/mini_installer/mini_installer.cc (right):

http://codereview.chromium.org/5875003/diff/10001/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:134: LONG
ReadValueFromRegistry(HKEY key, const wchar_t* value_name, wchar_t* value,
nice

http://codereview.chromium.org/5875003/diff/10001/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:139:
reinterpret_cast<LPBYTE>(value),
nit: styleguide prefers BYTE* over LPBYTE iirc

http://codereview.chromium.org/5875003/diff/10001/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:147: if (byte_length /
sizeof(wchar_t) < size)
nit: use () for readability

http://codereview.chromium.org/5875003/diff/10001/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:226: ::RegCloseKey(key);
it's almost worth it to write a poor man's RegKey to take care of all the exit
points

[grt (UTC plus 2), 2010-12-16 17:36:34]

Introduced RegKey class to make the registry code a bit cleaner.  PT(Final)L

http://codereview.chromium.org/5875003/diff/10001/chrome/installer/mini_insta...
File chrome/installer/mini_installer/mini_installer.cc (right):

http://codereview.chromium.org/5875003/diff/10001/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:139:
reinterpret_cast<LPBYTE>(value),
On 2010/12/16 06:32:25, tommi wrote:
> nit: styleguide prefers BYTE* over LPBYTE iirc

Done.

http://codereview.chromium.org/5875003/diff/10001/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:147: if (byte_length /
sizeof(wchar_t) < size)
On 2010/12/16 06:32:25, tommi wrote:
> nit: use () for readability

Done.

http://codereview.chromium.org/5875003/diff/10001/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:226: ::RegCloseKey(key);
On 2010/12/16 06:32:25, tommi wrote:
> it's almost worth it to write a poor man's RegKey to take care of all the exit
> points

I was thinking along the same lines in the shower this morning.  Consider it
done.

[tommi (sloooow) - chröme, 2010-12-16 17:52:47]

lgtm!

http://codereview.chromium.org/5875003/diff/18001/chrome/installer/mini_insta...
File chrome/installer/mini_installer/mini_installer.cc (right):

http://codereview.chromium.org/5875003/diff/18001/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:79: size_t value_size) const;
ah how I wish chrome's regkey would return the actual value instead of dropping
it on the floor :)