mini_installer code cleanups (no functional changes).

mini_installer code cleanups (no functional changes).

- Move constants into *_constants.{cc,h}.
- Remove unused constants.
- Lay foundation for detailed process exit codes.

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

[grt (UTC plus 2), 2014-07-03 15:50:28]

Laying the foundation for some cleanups. I'll hold off on landing this until an
associated signing script change lands. Please take a look. Thanks.

[gab, 2014-07-07 13:45:02]

lg, comments below.

Cheers!
Gab

https://codereview.chromium.org/365143002/diff/20001/chrome/installer/mini_in...
File chrome/installer/mini_installer.gypi (right):

https://codereview.chromium.org/365143002/diff/20001/chrome/installer/mini_in...
chrome/installer/mini_installer.gypi:30:
'mini_installer/mini_installer_constants.h',
Why do we have these sources in two separate GYP/GYPI files? This feels wrong
(although not introduced in your CL)...

https://codereview.chromium.org/365143002/diff/20001/chrome/installer/mini_in...
File chrome/installer/mini_installer/exit_code.h (right):

https://codereview.chromium.org/365143002/diff/20001/chrome/installer/mini_in...
chrome/installer/mini_installer/exit_code.h:14: GENERIC_INITIALIZATION_FAILURE =
101,
Why is there a gap here? Add a comment above stating where new codes should go
given it doesn't look like a strictly incremental list of exit codes.

https://codereview.chromium.org/365143002/diff/20001/chrome/installer/mini_in...
File chrome/installer/mini_installer/mini_installer.cc (right):

https://codereview.chromium.org/365143002/diff/20001/chrome/installer/mini_in...
chrome/installer/mini_installer/mini_installer.cc:37: // The type of a process
exit code.
Remove this comment, the variable name is explicit enough.

https://codereview.chromium.org/365143002/diff/20001/chrome/installer/mini_in...
chrome/installer/mini_installer/mini_installer.cc:421: !cmd_line.append(L"\""))
{
Use the CommandLine API to build the command line string rather than building
the string manually?

https://codereview.chromium.org/365143002/diff/20001/chrome/installer/mini_in...
chrome/installer/mini_installer/mini_installer.cc:515: !cmd_line.append(L"\""))
Use CommandLine API here too.

https://codereview.chromium.org/365143002/diff/20001/chrome/installer/mini_in...
chrome/installer/mini_installer/mini_installer.cc:790: return GENERIC_ERROR;
This doesn't match what the comment above says (i.e. this is a signal about
component_build, not an error -- I agree that 0/1 are inappropriate choices of
error codes for this, please introduce specific error codes for this).

https://codereview.chromium.org/365143002/diff/20001/chrome/installer/mini_in...
File chrome/installer/mini_installer/mini_installer_constants.h (right):

https://codereview.chromium.org/365143002/diff/20001/chrome/installer/mini_in...
chrome/installer/mini_installer/mini_installer_constants.h:11: // The target
name of the installer extracted from resources.
Typically with pref names and constants I've noticed that we put the
descriptions/comments in the .cc file; this is nice as it avoids having to look
at 2 files when searching for the constant's value and its associated
definition.

e.g. chrome/common/pref_names.cc, chrome_switches.cc

https://codereview.chromium.org/365143002/diff/20001/chrome/installer/mini_in...
chrome/installer/mini_installer/mini_installer_constants.h:18: // Command line
switch names for setup.exe (sans switch prefixes).
I didn't originally understand what was meant by "sans switch prefixes"; until I
realized you meant without "--" prefix.

I don't think this needs to be mentioned (chrome_switches.(cc|h) uses the same
scheme and doesn't mention it).

[grt (UTC plus 2), 2014-07-07 14:24:06]

thanks. ptal.

https://codereview.chromium.org/365143002/diff/20001/chrome/installer/mini_in...
File chrome/installer/mini_installer.gypi (right):

https://codereview.chromium.org/365143002/diff/20001/chrome/installer/mini_in...
chrome/installer/mini_installer.gypi:30:
'mini_installer/mini_installer_constants.h',
On 2014/07/07 13:45:01, gab wrote:
> Why do we have these sources in two separate GYP/GYPI files? This feels wrong
> (although not introduced in your CL)...

i believe one is used by syzygy-related toolchain and/or steps. agreed, it's
less than good.

https://codereview.chromium.org/365143002/diff/20001/chrome/installer/mini_in...
File chrome/installer/mini_installer/exit_code.h (right):

https://codereview.chromium.org/365143002/diff/20001/chrome/installer/mini_in...
chrome/installer/mini_installer/exit_code.h:14: GENERIC_INITIALIZATION_FAILURE =
101,
On 2014/07/07 13:45:01, gab wrote:
> Why is there a gap here? Add a comment above stating where new codes should go
> given it doesn't look like a strictly incremental list of exit codes.

Done.

https://codereview.chromium.org/365143002/diff/20001/chrome/installer/mini_in...
File chrome/installer/mini_installer/mini_installer.cc (right):

https://codereview.chromium.org/365143002/diff/20001/chrome/installer/mini_in...
chrome/installer/mini_installer/mini_installer.cc:37: // The type of a process
exit code.
On 2014/07/07 13:45:01, gab wrote:
> Remove this comment, the variable name is explicit enough.

Done.

https://codereview.chromium.org/365143002/diff/20001/chrome/installer/mini_in...
chrome/installer/mini_installer/mini_installer.cc:421: !cmd_line.append(L"\""))
{
On 2014/07/07 13:45:01, gab wrote:
> Use the CommandLine API to build the command line string rather than building
> the string manually?

base/ is not available here.

https://codereview.chromium.org/365143002/diff/20001/chrome/installer/mini_in...
chrome/installer/mini_installer/mini_installer.cc:515: !cmd_line.append(L"\""))
On 2014/07/07 13:45:01, gab wrote:
> Use CommandLine API here too.

same comment

https://codereview.chromium.org/365143002/diff/20001/chrome/installer/mini_in...
chrome/installer/mini_installer/mini_installer.cc:790: return GENERIC_ERROR;
On 2014/07/07 13:45:01, gab wrote:
> This doesn't match what the comment above says (i.e. this is a signal about
> component_build, not an error -- I agree that 0/1 are inappropriate choices of
> error codes for this, please introduce specific error codes for this).

0/1 are the generic success/error codes for any executable, so the names do
match what the values traditionally mean. i've updated the comment accordingly.

https://codereview.chromium.org/365143002/diff/20001/chrome/installer/mini_in...
File chrome/installer/mini_installer/mini_installer_constants.h (right):

https://codereview.chromium.org/365143002/diff/20001/chrome/installer/mini_in...
chrome/installer/mini_installer/mini_installer_constants.h:11: // The target
name of the installer extracted from resources.
On 2014/07/07 13:45:01, gab wrote:
> Typically with pref names and constants I've noticed that we put the
> descriptions/comments in the .cc file; this is nice as it avoids having to
look
> at 2 files when searching for the constant's value and its associated
> definition.
> 
> e.g. chrome/common/pref_names.cc, chrome_switches.cc

Done.

https://codereview.chromium.org/365143002/diff/20001/chrome/installer/mini_in...
chrome/installer/mini_installer/mini_installer_constants.h:18: // Command line
switch names for setup.exe (sans switch prefixes).
On 2014/07/07 13:45:01, gab wrote:
> I didn't originally understand what was meant by "sans switch prefixes"; until
I
> realized you meant without "--" prefix.
> 
> I don't think this needs to be mentioned (chrome_switches.(cc|h) uses the same
> scheme and doesn't mention it).

i mentioned it because it is a change from the old constants, which included "
--" (which i really didn't like). the point was to make it clear to those who
knew the old code that it's not broken now.

[gab, 2014-07-07 15:00:18]

I still don't like using a generic error code for "query_component_build"; and
even with generic codes I think 0/1 are backwards in the current
implementation...

But lgtm otherwise if you feel that this is correct despite my pushback!

https://codereview.chromium.org/365143002/diff/40001/chrome/installer/mini_in...
File chrome/installer/mini_installer/mini_installer.cc (right):

https://codereview.chromium.org/365143002/diff/40001/chrome/installer/mini_in...
chrome/installer/mini_installer/mini_installer.cc:787: // build. This is used by
the tests in /src/chrome/test/mini_installer/.
So when someone "queries for component build", we return an error if it *is* a
component build... this feels backward...

[grt (UTC plus 2), 2014-07-08 14:02:19]

PTAL

https://codereview.chromium.org/365143002/diff/40001/chrome/installer/mini_in...
File chrome/installer/mini_installer/mini_installer.cc (right):

https://codereview.chromium.org/365143002/diff/40001/chrome/installer/mini_in...
chrome/installer/mini_installer/mini_installer.cc:787: // build. This is used by
the tests in /src/chrome/test/mini_installer/.
On 2014/07/07 15:00:18, gab wrote:
> So when someone "queries for component build", we return an error if it *is* a
> component build... this feels backward...

While being delusional in the sick bed yesterday, I remembered going through
this with Robert when he landed this change:

https://codereview.chromium.org/304673004/diff/20001/chrome/installer/mini_in...

as I said back then, I would have done it the other way. Since Robert's not here
to complain, we get to do whatever we want!

[gab, 2014-07-08 14:28:32]

Woot, lgtm++ :-)!

[grt (UTC plus 2), 2014-07-09 13:28:29]

The CQ bit was checked by grt@chromium.org

[commit-bot: I haz the power, 2014-07-09 13:29:52]

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

[commit-bot: I haz the power, 2014-07-09 14:33:15]

Change committed as 282032