Allow download Chrome page to set Chrome as default even if it is not a new i...

Allow download Chrome page to set Chrome as default even if it is not a new install.

Read "make_default_for_user" preference from master_preferences file and set Chrome to default in installer if we are updating Chrome instead of installing a new version.
The problem here is that now that we set the default browser preference from a web page, we are using master_preferences to communicate this desire to the browser.
Because the user already has Chrome installed on his/her machine, the master_preferences file will not be written to their machine.
Instead, we check for this preference in the installer itself, and set Chrome to be default for the user here.

BUG=53656
TEST=Have Chrome installed on your machine, but *not* default browser.  Run setup.exe to install a new version of Chrome, with a master_preferences file that includes the distribution preference "make_default_for_user":true. (This reflects what happens when the user downloads Chrome with the "make default" option checked).
Chrome should be default browser.


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

[kuchhal, 2010-09-01 23:22:33]

lgtm.

http://codereview.chromium.org/3342002/diff/7001/8001
File chrome/installer/setup/install.cc (right):

http://codereview.chromium.org/3342002/diff/7001/8001#newcode778
chrome/installer/setup/install.cc:778: &make_chrome_default);
indent by two more spaces.

http://codereview.chromium.org/3342002/diff/7001/8001#newcode794
chrome/installer/setup/install.cc:794: force_chrome_default_for_user);
I dont think the fourth param is needed. You can just pass (make_chrome_default
|| force_chrome_default_for_user) as the third parameter.

[Miranda Callahan, 2010-09-01 23:28:51]

http://codereview.chromium.org/3342002/diff/7001/8001
File chrome/installer/setup/install.cc (right):

http://codereview.chromium.org/3342002/diff/7001/8001#newcode778
chrome/installer/setup/install.cc:778: &make_chrome_default);
On 2010/09/01 23:22:33, kuchhal wrote:
> indent by two more spaces.

oops, done.

http://codereview.chromium.org/3342002/diff/7001/8001#newcode794
chrome/installer/setup/install.cc:794: force_chrome_default_for_user);
hmm, can I really always trust system_install to be FALSE in this case?

[huanr, 2010-09-01 23:35:40]

http://codereview.chromium.org/3342002/diff/7001/8001
File chrome/installer/setup/install.cc (right):

http://codereview.chromium.org/3342002/diff/7001/8001#newcode786
chrome/installer/setup/install.cc:786: result ==
installer_util::INSTALL_REPAIRED) {
Most likely the result will be *_INSTALL_EXISTS if a chrome user goes to
download page. So the code does nothing with those cases?

[Miranda Callahan, 2010-09-01 23:38:33]

On 2010/09/01 23:35:40, huanr wrote:
> http://codereview.chromium.org/3342002/diff/7001/8001
> File chrome/installer/setup/install.cc (right):
> 
> http://codereview.chromium.org/3342002/diff/7001/8001#newcode786
> chrome/installer/setup/install.cc:786: result ==
> installer_util::INSTALL_REPAIRED) {
> Most likely the result will be *_INSTALL_EXISTS if a chrome user goes to
> download page. So the code does nothing with those cases?

No, good point.  Added to the list of options, thanks for the catch.

[kuchhal, 2010-09-01 23:41:54]

Are you talking about these three cases?

  HIGHER_VERSION_EXISTS, // Higher version of Chrome already exists
  USER_LEVEL_INSTALL_EXISTS, // User level install already exists
  SYSTEM_LEVEL_INSTALL_EXISTS, // Machine level install already exists

All of them are error cases I believe. You shouldn't be changing anything on
the user machine in that case.

On Wed, Sep 1, 2010 at 4:38 PM, <mirandac@chromium.org> wrote:

> On 2010/09/01 23:35:40, huanr wrote:
>
>> http://codereview.chromium.org/3342002/diff/7001/8001
>> File chrome/installer/setup/install.cc (right):
>>
>
>  http://codereview.chromium.org/3342002/diff/7001/8001#newcode786
>> chrome/installer/setup/install.cc:786: result ==
>> installer_util::INSTALL_REPAIRED) {
>> Most likely the result will be *_INSTALL_EXISTS if a chrome user goes to
>> download page. So the code does nothing with those cases?
>>
>
> No, good point.  Added to the list of options, thanks for the catch.
>
>
>
> http://codereview.chromium.org/3342002/show
>

[Miranda Callahan, 2010-09-01 23:45:23]

On 2010/09/01 23:41:54, kuchhal wrote:
> Are you talking about these three cases?
> 
>   HIGHER_VERSION_EXISTS, // Higher version of Chrome already exists
>   USER_LEVEL_INSTALL_EXISTS, // User level install already exists
>   SYSTEM_LEVEL_INSTALL_EXISTS, // Machine level install already exists
> 
> All of them are error cases I believe. You shouldn't be changing anything on
> the user machine in that case.

Hmm, in setup_main.cc, line 191-192, it checks these data points to decide on
the system_install level -- comment is "this is an update" -- indicating that
Chromium will actually be updated on this path?  (Looking more closely for
clues)

[huanr, 2010-09-01 23:47:03]

What is the result code if a user with version X downloads and installs
version X?

Huan

On Wed, Sep 1, 2010 at 4:45 PM, <mirandac@chromium.org> wrote:

> On 2010/09/01 23:41:54, kuchhal wrote:
>
>> Are you talking about these three cases?
>>
>
>    HIGHER_VERSION_EXISTS, // Higher version of Chrome already exists
>>   USER_LEVEL_INSTALL_EXISTS, // User level install already exists
>>   SYSTEM_LEVEL_INSTALL_EXISTS, // Machine level install already exists
>>
>
>  All of them are error cases I believe. You shouldn't be changing anything
>> on
>> the user machine in that case.
>>
>
> Hmm, in setup_main.cc, line 191-192, it checks these data points to decide
> on
> the system_install level -- comment is "this is an update" -- indicating
> that
> Chromium will actually be updated on this path?  (Looking more closely for
> clues)
>
>
> http://codereview.chromium.org/3342002/show
>

[kuchhal, 2010-09-01 23:49:46]

> What is the result code if a user with version X downloads and installs
> version X?


IIRC INSTALL_REPAIRED

Hmm, in setup_main.cc, line 191-192, it checks these data points to decide
>> on
>> the system_install level -- comment is "this is an update" -- indicating
>> that
>> Chromium will actually be updated on this path?  (Looking more closely for
>> clues)
>
>
There it is figuring out the right error code to return and the right error
message to show once it has figured out that the existing install conflicts
with the current installer.

[Miranda Callahan, 2010-09-01 23:52:53]

On 2010/09/01 23:45:23, Miranda Callahan wrote:
> On 2010/09/01 23:41:54, kuchhal wrote:
> > Are you talking about these three cases?
> > 
> >   HIGHER_VERSION_EXISTS, // Higher version of Chrome already exists
> >   USER_LEVEL_INSTALL_EXISTS, // User level install already exists
> >   SYSTEM_LEVEL_INSTALL_EXISTS, // Machine level install already exists
> > 
> > All of them are error cases I believe. You shouldn't be changing anything on
> > the user machine in that case.
> 

as far as I can tell, the cases I need to consider are those that can be
returned from InstallNewVersion, in install.cc.  The correct cases here are:

INSTALL_REPAIRED
IN_USE_UPDATED
NEW_VERSION_UPDATED

I don't want FIRST_INSTALL_SUCCESS.  It looks to me like HIGHER_VERSION_EXISTS
is an error code, and USER_LEVEL and SYSTEM_LEVEL are simply used to write an
installer result (line 193 of setup_main.cc).

[Miranda Callahan, 2010-09-01 23:54:08]

On 2010/09/01 23:49:46, kuchhal wrote:
> > What is the result code if a user with version X downloads and installs
> > version X?
> 
> 
> IIRC INSTALL_REPAIRED
> 

This result is what I'm getting with all my testing, of installing and
overinstalling the same ToT build.

[Miranda Callahan, 2010-09-02 00:02:07]

On 2010/09/01 23:54:08, Miranda Callahan wrote:
> On 2010/09/01 23:49:46, kuchhal wrote:
> > > What is the result code if a user with version X downloads and installs
> > > version X?
> > 
> > 
> > IIRC INSTALL_REPAIRED
> > 
> 
> This result is what I'm getting with all my testing, of installing and
> overinstalling the same ToT build.

Uploaded what I hope is the final version. :-)

[kuchhal, 2010-09-02 00:08:19]

lgtm.

But just to point out something that I just realized. IN_USE_UPDATED would
be an interesting case to deal with. This result code means Chrome was
updated while it was running. And if we try to make a new version of Chrome
default while the old version is running, I am not sure what would happen.

- Lets say Chrome version x that is running is not registered on the machine
- Not installer runs and updates it to version x+1 and since
make-chrome-default bit is set, tried to make Chrome default
- To make default first installer tries to register Chrome which required
admin rights which installer might not have
- Installer will try to launch an elevated process in this case

I don't think it would break but there could be corner cases that I am not
thinking of.


On Wed, Sep 1, 2010 at 4:52 PM, <mirandac@chromium.org> wrote:

> On 2010/09/01 23:45:23, Miranda Callahan wrote:
>
>> On 2010/09/01 23:41:54, kuchhal wrote:
>> > Are you talking about these three cases?
>> >
>> >   HIGHER_VERSION_EXISTS, // Higher version of Chrome already exists
>> >   USER_LEVEL_INSTALL_EXISTS, // User level install already exists
>> >   SYSTEM_LEVEL_INSTALL_EXISTS, // Machine level install already exists
>> >
>> > All of them are error cases I believe. You shouldn't be changing
>> anything on
>> > the user machine in that case.
>>
>
>
> as far as I can tell, the cases I need to consider are those that can be
> returned from InstallNewVersion, in install.cc.  The correct cases here
> are:
>
> INSTALL_REPAIRED
> IN_USE_UPDATED
> NEW_VERSION_UPDATED
>
> I don't want FIRST_INSTALL_SUCCESS.  It looks to me like
> HIGHER_VERSION_EXISTS
> is an error code, and USER_LEVEL and SYSTEM_LEVEL are simply used to write
> an
> installer result (line 193 of setup_main.cc).
>
>
>
>
> http://codereview.chromium.org/3342002/show
>

[Miranda Callahan, 2010-09-02 00:14:23]

Hmm.  What you say scares me.  I think that because this patch is going into M6
this weekend, I will leave the IN_USE case out.  Failing to install as default
seems better to me than unknown behavior.

What do you think?


On 2010/09/02 00:08:19, kuchhal wrote:
> lgtm.
> 
> But just to point out something that I just realized. IN_USE_UPDATED would
> be an interesting case to deal with. This result code means Chrome was
> updated while it was running. And if we try to make a new version of Chrome
> default while the old version is running, I am not sure what would happen.
> 
> - Lets say Chrome version x that is running is not registered on the machine
> - Not installer runs and updates it to version x+1 and since
> make-chrome-default bit is set, tried to make Chrome default
> - To make default first installer tries to register Chrome which required
> admin rights which installer might not have
> - Installer will try to launch an elevated process in this case
> 
> I don't think it would break but there could be corner cases that I am not
> thinking of.
>

[kuchhal, 2010-09-02 00:18:37]

That certainly makes the change relatively safer. so it sounds fine to me.


On Wed, Sep 1, 2010 at 5:14 PM, <mirandac@chromium.org> wrote:

> Hmm.  What you say scares me.  I think that because this patch is going
> into M6
> this weekend, I will leave the IN_USE case out.  Failing to install as
> default
> seems better to me than unknown behavior.
>
> What do you think?
>
>
>
> On 2010/09/02 00:08:19, kuchhal wrote:
>
>> lgtm.
>>
>
>  But just to point out something that I just realized. IN_USE_UPDATED would
>> be an interesting case to deal with. This result code means Chrome was
>> updated while it was running. And if we try to make a new version of
>> Chrome
>> default while the old version is running, I am not sure what would happen.
>>
>
>  - Lets say Chrome version x that is running is not registered on the
>> machine
>> - Not installer runs and updates it to version x+1 and since
>> make-chrome-default bit is set, tried to make Chrome default
>> - To make default first installer tries to register Chrome which required
>> admin rights which installer might not have
>> - Installer will try to launch an elevated process in this case
>>
>
>  I don't think it would break but there could be corner cases that I am not
>> thinking of.
>>
>
>
>
> http://codereview.chromium.org/3342002/show
>

[huanr, 2010-09-02 02:30:12]

http://codereview.chromium.org/3342002/diff/18001/19001
File chrome/installer/setup/install.cc (right):

http://codereview.chromium.org/3342002/diff/18001/19001#newcode783
chrome/installer/setup/install.cc:783: result == installer_util::IN_USE_UPDATED
||
Did you update the CL? I still see IN_USE_UPDATED.

Other than that, lgtm