Add support for the quick-enable-cf command to the installer. This encompase...

Add support for the quick-enable-cf command to the installer.  This encompases:
- Adding facilities for new-style Google Update commands (ProductCommand, ProductCommands)
- Adding support to the validator to validate the quick-enable-cf command
- Adding juju to the installation and uninstallation flows to put the command into place when installing/upgrading Chrome in multi-install mode when CF is either not installed or is in ready-mode, and making sure the command is not there when Chrome Frame is installed.

BUG=none
TEST=Install Chrome in multi-install mode and see if the Google Update version key for the binaries (app guid {4DC8B4CA-1BDA-483e-B5FA-D3C12E15B62D}) contains a key named "quick-enable-cf" that has a CommandLine value indicating setup.exe w/ --multi-install [ --system-level ] --quick-enable-cf, a SendsPings value of 1, and a WebAccessible value of 1.  Then try the other variations, like install CF in ready-mode and make sure that quick-enable-cf is still there.  Make sure that installing CF causes it to be removed, etc.


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

[grt (UTC plus 2), 2011-03-01 21:20:22]

Gents,

Please have an early look at this CL.  It requires
http://codereview.chromium.org/6598065/, so it won't have a run through the
trybots until I commit that one.

Erik: please have a look at the new values in google_update_constants.cc and the
registry code in product_command.cc and product_commands.cc.  Let me know if
anything has/will change as far as the registry structure goes (i.e., the new
key to house all commands under an app's Clients key that you mentioned).

Thanks.

[tommi (sloooow) - chröme, 2011-03-01 23:33:12]

great! I'm going to go over it again tomorrow as it's a sizable changelist, but
it looks pretty good.

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:953:
DCHECK(cf->HasOption(installer::kOptionReadyMode));
couldn't we also get here for a 'single' install (not ready-mode)?

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:962: BrowserDistribution::CHROME_FRAME)
== NULL) {
What if there exists a system install of gcf and this is a user install?

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/util/instal...
File chrome/installer/util/installation_validator_unittest.cc (right):

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/util/instal...
chrome/installer/util/installation_validator_unittest.cc:326: type_to_modifiers_
= new InstallationTypeToModifiers();
DCHECK(type_to_modifiers_ == NULL) first?

[robertshield, 2011-03-02 14:58:57]

just nits from me. LG, pending an answer to Tommi's question about user vs
system CF installs in install_worker.cc

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:938: } else {
Can we move the DCHECKS on setup_path and new_version here since as per the
header file comments, they are always required for installation?

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
File chrome/installer/setup/install_worker.h (right):

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.h:140: // binaries' version key.  
|setup_path| and |new_version| are required when
Three spaces after a period? That's a lot of space.

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/util/google...
File chrome/installer/util/google_update_constants.cc (right):

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/util/google...
chrome/installer/util/google_update_constants.cc:30: const wchar_t
kRegMSIField[] = L"msi";
kRegMSI... should come after kRegMetrics...

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/util/produc...
File chrome/installer/util/product_commands.cc (right):

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/util/produc...
chrome/installer/util/product_commands.cc:47: << "\" as is does not appear to
hold a product command.";
*as it

[erikwright (departed), 2011-03-02 15:18:16]

Also, the Omaha team has asked to use the term AppCommand instead of
ProductCommand.

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:991: cmd_key.append(1,
L'\\').append(kCmdQuickEnableCf);
add \\Commands here, after the version key.

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
File chrome/installer/setup/install_worker.h (right):

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.h:141: // processing product installation;
they may be NULL when uninstalling.
'may' or 'must'?

After reviewing the implementation, I understand that this function decides,
based on the installer and installation state, whether to add, remove, or leave
unmodified the product commands. I think the comment here could better reflect
that fact.

Even when reading the call sites, the name is a bit confusing. Ideally, the name
would be something that said
"AddWorkItemsToAddOrRemoveQuickEnableProductCommand" but that's a bit long :(

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/util/produc...
File chrome/installer/util/product_commands.h (right):

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/util/produc...
chrome/installer/util/product_commands.h:39: // (typically a
BrowserDistribution's "version key").
Update to reflect that it's "Commands" under ...

[grt (UTC plus 2), 2011-03-02 19:48:28]

Thanks for the feedback.  PTAL.  I will likely upload two additional updates to
this CL: one which renames ProductCommand to AppCommand, and one which adds a
unit test for AddQuickEnableWorkItems.

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:907: void AddQuickEnableWorkItems(const
InstallerState& installer_state,
This function made me so sad that I rewrote it.  The logic is now based on the
target state rather than the transitions themselves.  I think this makes it much
easier to reason about its correctness.  I'm working on a unit test that I'll
add in an update prior to commit.

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:938: } else {
On 2011/03/02 14:58:57, robertshield wrote:
> Can we move the DCHECKS on setup_path and new_version here since as per the
> header file comments, they are always required for installation? 

Done.

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:953:
DCHECK(cf->HasOption(installer::kOptionReadyMode));
On 2011/03/01 23:33:13, tommi wrote:
> couldn't we also get here for a 'single' install (not ready-mode)?

I don't think so (at least, that wasn't my plan).  The first block is hit for
single CF or multi-but-not-ready-mode CF, which leaves only multi-ready-mode-CF
for this block.  Let me know if you think I've messed up the logic here.

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:962: BrowserDistribution::CHROME_FRAME)
== NULL) {
On 2011/03/01 23:33:13, tommi wrote:
> What if there exists a system install of gcf and this is a user install?

Robert, Erik, and I just discussed this and reached the conclusion that user-
and system-level should be handled independently as far as adding or removing
the command in the registry goes.

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:991: cmd_key.append(1,
L'\\').append(kCmdQuickEnableCf);
On 2011/03/02 15:18:16, erikwright wrote:
> add \\Commands here, after the version key.

Done.

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
File chrome/installer/setup/install_worker.h (right):

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.h:140: // binaries' version key.  
|setup_path| and |new_version| are required when
On 2011/03/02 14:58:57, robertshield wrote:
> Three spaces after a period? That's a lot of space.

Inflation, don't you know.

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.h:141: // processing product installation;
they may be NULL when uninstalling.
On 2011/03/02 15:18:16, erikwright wrote:
> 'may' or 'must'?

Nothing will break if they aren't NULL on uninstall, so I wouldn't say must. 
I've updated the comment to make it clear that they're not used on uninstall.

> After reviewing the implementation, I understand that this function decides,
> based on the installer and installation state, whether to add, remove, or
leave
> unmodified the product commands. I think the comment here could better reflect
> that fact.

Done.

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/util/google...
File chrome/installer/util/google_update_constants.cc (right):

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/util/google...
chrome/installer/util/google_update_constants.cc:30: const wchar_t
kRegMSIField[] = L"msi";
On 2011/03/02 14:58:57, robertshield wrote:
> kRegMSI... should come after kRegMetrics...

Done.

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/util/instal...
File chrome/installer/util/installation_validator_unittest.cc (right):

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/util/instal...
chrome/installer/util/installation_validator_unittest.cc:326: type_to_modifiers_
= new InstallationTypeToModifiers();
On 2011/03/01 23:33:13, tommi wrote:
> DCHECK(type_to_modifiers_ == NULL) first?

Done.

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/util/produc...
File chrome/installer/util/product_commands.cc (right):

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/util/produc...
chrome/installer/util/product_commands.cc:47: << "\" as is does not appear to
hold a product command.";
On 2011/03/02 14:58:57, robertshield wrote:
> *as it

Done.

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/util/produc...
File chrome/installer/util/product_commands.h (right):

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/util/produc...
chrome/installer/util/product_commands.h:39: // (typically a
BrowserDistribution's "version key").
On 2011/03/02 15:18:16, erikwright wrote:
> Update to reflect that it's "Commands" under ...

Done.

[tommi (sloooow) - chröme, 2011-03-02 20:30:58]

lgtm

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:907: void AddQuickEnableWorkItems(const
InstallerState& installer_state,
On 2011/03/02 19:48:28, grt wrote:
> This function made me so sad that I rewrote it.  The logic is now based on the
> target state rather than the transitions themselves.  I think this makes it
much
> easier to reason about its correctness.  I'm working on a unit test that I'll
> add in an update prior to commit.

thanks. more easily digestible :)

http://codereview.chromium.org/6588003/diff/9003/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:945: const Product* chrome =
This reads like a question without the question mark.
Does it mean that if Chrome multi is being uninstalled, then we treat that as
not having Chrome?

[erikwright (departed), 2011-03-02 20:36:19]

LGTM WRT matching my changes to Omaha. Will let the others opine on the other
bits of this latest patchset.

On 2011/03/02 20:30:58, tommi wrote:
> lgtm
> 
>
http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
> File chrome/installer/setup/install_worker.cc (right):
> 
>
http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
> chrome/installer/setup/install_worker.cc:907: void
AddQuickEnableWorkItems(const
> InstallerState& installer_state,
> On 2011/03/02 19:48:28, grt wrote:
> > This function made me so sad that I rewrote it.  The logic is now based on
the
> > target state rather than the transitions themselves.  I think this makes it
> much
> > easier to reason about its correctness.  I'm working on a unit test that
I'll
> > add in an update prior to commit.
> 
> thanks. more easily digestible :)
> 
>
http://codereview.chromium.org/6588003/diff/9003/chrome/installer/setup/insta...
> chrome/installer/setup/install_worker.cc:945: const Product* chrome =
> This reads like a question without the question mark.
> Does it mean that if Chrome multi is being uninstalled, then we treat that as
> not having Chrome?

[grt (UTC plus 2), 2011-03-02 20:44:47]

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:907: void AddQuickEnableWorkItems(const
InstallerState& installer_state,
On 2011/03/02 20:30:58, tommi wrote:
> On 2011/03/02 19:48:28, grt wrote:
> > This function made me so sad that I rewrote it.  The logic is now based on
the
> > target state rather than the transitions themselves.  I think this makes it
> much
> > easier to reason about its correctness.  I'm working on a unit test that
I'll
> > add in an update prior to commit.
> 
> thanks. more easily digestible :)

<burp>

http://codereview.chromium.org/6588003/diff/9003/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:945: const Product* chrome =
On 2011/03/02 20:30:58, tommi wrote:
> This reads like a question without the question mark.

A better comment would be "Forget about multi-install Chrome if it is being
uninstalled."  I've changed it thus.

> Does it mean that if Chrome multi is being uninstalled, then we treat that as
> not having Chrome?

Right.  In step 1, we set have_multi_chrome if the machine had multi-install
Chrome at the outset.  In this step, we clear have_multi_chrome if this is an
uninstall operation and we're uninstalling that multi-install Chrome.  Sound
good?

[tommi (sloooow) - chröme, 2011-03-02 20:54:49]

sounds good.  lgtm++

On Wed, Mar 2, 2011 at 3:44 PM, <grt@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
> File chrome/installer/setup/install_worker.cc (right):
>
>
>
http://codereview.chromium.org/6588003/diff/7050/chrome/installer/setup/insta...
> chrome/installer/setup/install_worker.cc:907: void
> AddQuickEnableWorkItems(const InstallerState& installer_state,
> On 2011/03/02 20:30:58, tommi wrote:
>
>> On 2011/03/02 19:48:28, grt wrote:
>> > This function made me so sad that I rewrote it.  The logic is now
>>
> based on the
>
>> > target state rather than the transitions themselves.  I think this
>>
> makes it
>
>> much
>> > easier to reason about its correctness.  I'm working on a unit test
>>
> that I'll
>
>> > add in an update prior to commit.
>>
>
>  thanks. more easily digestible :)
>>
>
> <burp>
>
>
>
>
http://codereview.chromium.org/6588003/diff/9003/chrome/installer/setup/insta...
> chrome/installer/setup/install_worker.cc:945: const Product* chrome =
> On 2011/03/02 20:30:58, tommi wrote:
>
>> This reads like a question without the question mark.
>>
>
> A better comment would be "Forget about multi-install Chrome if it is
> being uninstalled."  I've changed it thus.
>
>
>  Does it mean that if Chrome multi is being uninstalled, then we treat
>>
> that as
>
>> not having Chrome?
>>
>
> Right.  In step 1, we set have_multi_chrome if the machine had
> multi-install Chrome at the outset.  In this step, we clear
> have_multi_chrome if this is an uninstall operation and we're
> uninstalling that multi-install Chrome.  Sound good?
>
>
> http://codereview.chromium.org/6588003/
>

[robertshield, 2011-03-02 20:58:04]

LGTM.

New install_worker.cc logic is a lot easier to follow. Some nits on the
comments:

http://codereview.chromium.org/6588003/diff/9003/chrome/installer/setup/insta...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/6588003/diff/9003/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:959: // Maybe we're installing
multi-install Chrome.
nit: "Maybe" -> "Check if" here and elsewhere in this function.

http://codereview.chromium.org/6588003/diff/9003/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:986: // Chrome Frame isn't (to be)
installed, or is (to be) in ready-mode but
Consider rephrasing to "Chrome Frame isn't (to be) installed or is (to be)
installed only in ready-mode, while multi-install Chrome is (to be) installed."

http://codereview.chromium.org/6588003/diff/9003/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:993: // Some product (Chrome Frame) is
being uninstalled.  Use the path to the
Simplify to "Chrome Frame is being uninstalled."

http://codereview.chromium.org/6588003/diff/9003/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:1001: // Some product (Chrome) is being
installed, updated, or otherwise being
Simplify to "Chrome is being [...]

[grt (UTC plus 2), 2011-03-02 21:06:37]

Comments address, thanks.  I've also uploaded an update with the ProductCommand
-> AppCommand rename.  Now to unit test!

http://codereview.chromium.org/6588003/diff/9003/chrome/installer/setup/insta...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/6588003/diff/9003/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:959: // Maybe we're installing
multi-install Chrome.
On 2011/03/02 20:58:04, robertshield wrote:
> nit: "Maybe" -> "Check if" here and elsewhere in this function.

Done.

http://codereview.chromium.org/6588003/diff/9003/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:986: // Chrome Frame isn't (to be)
installed, or is (to be) in ready-mode but
On 2011/03/02 20:58:04, robertshield wrote:
> Consider rephrasing to "Chrome Frame isn't (to be) installed or is (to be)
> installed only in ready-mode, while multi-install Chrome is (to be)
installed."

Done.

http://codereview.chromium.org/6588003/diff/9003/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:993: // Some product (Chrome Frame) is
being uninstalled.  Use the path to the
On 2011/03/02 20:58:04, robertshield wrote:
> Simplify to "Chrome Frame is being uninstalled."

Done.

http://codereview.chromium.org/6588003/diff/9003/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:1001: // Some product (Chrome) is being
installed, updated, or otherwise being
On 2011/03/02 20:58:04, robertshield wrote:
> Simplify to "Chrome is being [...]

Done.

[tommi (sloooow) - chröme, 2011-03-02 21:13:03]

http://codereview.chromium.org/6588003/diff/57/chrome/installer/util/app_comm...
File chrome/installer/util/app_command.h (right):

http://codereview.chromium.org/6588003/diff/57/chrome/installer/util/app_comm...
chrome/installer/util/app_command.h:56: std::wstring command_line_;
should we be using CommandLine and not std::wstring?

[grt (UTC plus 2), 2011-03-02 21:41:34]

http://codereview.chromium.org/6588003/diff/57/chrome/installer/util/app_comm...
File chrome/installer/util/app_command.h (right):

http://codereview.chromium.org/6588003/diff/57/chrome/installer/util/app_comm...
chrome/installer/util/app_command.h:56: std::wstring command_line_;
On 2011/03/02 21:13:03, tommi wrote:
> should we be using CommandLine and not std::wstring?

I chose not to in this case because I figured it would potentially invoke extra
object creation and parsing and stuff when it wasn't needed.  This class is
really meant to be a close representation of the data in the registry.  I'm open
to changing it if you think there's a benefit.

[tommi (sloooow) - chröme, 2011-03-02 21:49:23]

The only benefit is that it is *the class* for command lines.  Same as
FilePath vs wstring really.
In this case we don't need it, but if ever someone wants to convert it to a
CommandLine
object, we might run into a bug we recently had:

CommandLine cmd(app_command.command_line());

This is a bug because the CommandLine constructor expects the string to be
a program path and not a command line.  Still, because the property is
called
command_line(), it's not immediately obvious from reading the code.


On Wed, Mar 2, 2011 at 4:41 PM, <grt@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6588003/diff/57/chrome/installer/util/app_comm...
> File chrome/installer/util/app_command.h (right):
>
>
>
http://codereview.chromium.org/6588003/diff/57/chrome/installer/util/app_comm...
> chrome/installer/util/app_command.h:56: std::wstring command_line_;
> On 2011/03/02 21:13:03, tommi wrote:
>
>> should we be using CommandLine and not std::wstring?
>>
>
> I chose not to in this case because I figured it would potentially
> invoke extra object creation and parsing and stuff when it wasn't
> needed.  This class is really meant to be a close representation of the
> data in the registry.  I'm open to changing it if you think there's a
> benefit.
>
>
> http://codereview.chromium.org/6588003/
>

[tommi (sloooow) - chröme, 2011-03-02 22:04:37]

actually, I'm not entirely correct here.  The CommandLine constructor
accepts a
FilePath and not wstring.  So the bug would be:

CommandLine cmd(FilePath(app_command.command_line()));

Which makes the bug a lot more obvious - unless the developer in question
*cough*
gets distracted by a "most vexing parse" compiler error and fixes the
compiler error
and not the actual bug! *cough* *cough*  I need to do something about that
cough :)

On Wed, Mar 2, 2011 at 4:49 PM, Tommi <tommi@chromium.org> wrote:

> The only benefit is that it is *the class* for command lines.  Same as
> FilePath vs wstring really.
> In this case we don't need it, but if ever someone wants to convert it to a
> CommandLine
> object, we might run into a bug we recently had:
>
> CommandLine cmd(app_command.command_line());
>
> This is a bug because the CommandLine constructor expects the string to be
> a program path and not a command line.  Still, because the property is
> called
> command_line(), it's not immediately obvious from reading the code.
>
>
> On Wed, Mar 2, 2011 at 4:41 PM, <grt@chromium.org> wrote:
>
>>
>>
>>
http://codereview.chromium.org/6588003/diff/57/chrome/installer/util/app_comm...
>> File chrome/installer/util/app_command.h (right):
>>
>>
>>
http://codereview.chromium.org/6588003/diff/57/chrome/installer/util/app_comm...
>> chrome/installer/util/app_command.h:56: std::wstring command_line_;
>> On 2011/03/02 21:13:03, tommi wrote:
>>
>>> should we be using CommandLine and not std::wstring?
>>>
>>
>> I chose not to in this case because I figured it would potentially
>> invoke extra object creation and parsing and stuff when it wasn't
>> needed.  This class is really meant to be a close representation of the
>> data in the registry.  I'm open to changing it if you think there's a
>> benefit.
>>
>>
>> http://codereview.chromium.org/6588003/
>>
>
>

[grt (UTC plus 2), 2011-03-03 01:33:07]

On 2011/03/02 22:04:37, tommi wrote:
> actually, I'm not entirely correct here.  The CommandLine constructor
> accepts a
> FilePath and not wstring.  So the bug would be:
> 
> CommandLine cmd(FilePath(app_command.command_line()));
> 
> Which makes the bug a lot more obvious - unless the developer in question
> *cough*
> gets distracted by a "most vexing parse" compiler error and fixes the
> compiler error
> and not the actual bug! *cough* *cough*  I need to do something about that
> cough :)

Sounds like a nasty cough.  I think maybe you should consider going back to the
doctor.  :-)

That aside, you have a good point: it's easy for someone to chance upon this
code and not realize the ins-and-outs of strings, paths, and command lines.  Do
you think it's sufficient to have this comment on the command_line() method in
app_command.h:

  // Returns the command-line for the app command as it is represented in the
  // registry.  Use CommandLine::FromString() on this value to check arguments
  // or to launch the command.

[tommi (sloooow) - chröme, 2011-03-03 02:39:01]

sgtm.

On Wed, Mar 2, 2011 at 8:33 PM, <grt@chromium.org> wrote:

> On 2011/03/02 22:04:37, tommi wrote:
>
>> actually, I'm not entirely correct here.  The CommandLine constructor
>> accepts a
>> FilePath and not wstring.  So the bug would be:
>>
>
>  CommandLine cmd(FilePath(app_command.command_line()));
>>
>
>  Which makes the bug a lot more obvious - unless the developer in question
>> *cough*
>> gets distracted by a "most vexing parse" compiler error and fixes the
>> compiler error
>> and not the actual bug! *cough* *cough*  I need to do something about that
>> cough :)
>>
>
> Sounds like a nasty cough.  I think maybe you should consider going back to
> the
> doctor.  :-)
>

I did! he's the one that told me that my parse was vexing!


>
> That aside, you have a good point: it's easy for someone to chance upon
> this
> code and not realize the ins-and-outs of strings, paths, and command lines.
>  Do
> you think it's sufficient to have this comment on the command_line() method
> in
> app_command.h:
>
>  // Returns the command-line for the app command as it is represented in
> the
>  // registry.  Use CommandLine::FromString() on this value to check
> arguments
>  // or to launch the command.
>
>
> http://codereview.chromium.org/6588003/
>

[grt (UTC plus 2), 2011-03-03 18:54:06]

This update includes new GMock-based tests in install_worker_unittest.cc to test
the implementation of AddQuickEnableWorkItems.  Everything else is the same as
the last modulo updates to the tree.  I have no more plans with this CL, so
please give the new tests a look and let me know what you think.  Thanks.

[tommi (sloooow) - chröme, 2011-03-03 19:21:44]

lgtm

[robertshield, 2011-03-03 19:32:26]

LGTM

http://codereview.chromium.org/6588003/diff/13001/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker_unittest.cc (right):

http://codereview.chromium.org/6588003/diff/13001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker_unittest.cc:172:
install_path.Append(installer::kSetupExe));
don't you also need to add a --uninstall switch here or is that added elsewhere?

http://codereview.chromium.org/6588003/diff/13001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker_unittest.cc:205:
install_path.Append(installer::kSetupExe));
ditto

[grt (UTC plus 2), 2011-03-03 19:38:44]

http://codereview.chromium.org/6588003/diff/13001/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker_unittest.cc (right):

http://codereview.chromium.org/6588003/diff/13001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker_unittest.cc:172:
install_path.Append(installer::kSetupExe));
On 2011/03/03 19:32:26, robertshield wrote:
> don't you also need to add a --uninstall switch here or is that added
elsewhere?

Nice catch!  Done.

http://codereview.chromium.org/6588003/diff/13001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker_unittest.cc:205:
install_path.Append(installer::kSetupExe));
On 2011/03/03 19:32:26, robertshield wrote:
> ditto

Done.