Installer string cleaning following Chrome Frame removal.

Installer string cleaning following Chrome Frame removal.

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

[gab, 2014-01-09 21:59:15]

https://codereview.chromium.org/132583002/diff/30001/chrome/installer/setup/s...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/132583002/diff/30001/chrome/installer/setup/s...
chrome/installer/setup/setup_main.cc:585: NOTREACHED();
It is possible that an old multi-install system-level CF remains on the system
though?

Wouldn't installing a user-level Chrome in that case trip this?

[grt (UTC plus 2), 2014-01-10 12:10:52]

https://codereview.chromium.org/132583002/diff/30001/chrome/installer/setup/s...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/132583002/diff/30001/chrome/installer/setup/s...
chrome/installer/setup/setup_main.cc:585: NOTREACHED();
On 2014/01/09 21:59:15, gab wrote:
> It is possible that an old multi-install system-level CF remains on the system
> though?

No. They would have already been removed by now by
UninstallMultiChromeFrameIfPresent in setup_main.

> Wouldn't installing a user-level Chrome in that case trip this?

I don't think so, but please double-check my logic. Thanks.

[gab, 2014-01-10 16:28:02]

https://codereview.chromium.org/132583002/diff/30001/chrome/installer/setup/s...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/132583002/diff/30001/chrome/installer/setup/s...
chrome/installer/setup/setup_main.cc:585: NOTREACHED();
On 2014/01/10 12:10:52, grt wrote:
> On 2014/01/09 21:59:15, gab wrote:
> > It is possible that an old multi-install system-level CF remains on the
system
> > though?
> 
> No. They would have already been removed by now by
> UninstallMultiChromeFrameIfPresent in setup_main.
> 
> > Wouldn't installing a user-level Chrome in that case trip this?
> 
> I don't think so, but please double-check my logic. Thanks.

Well if you install user-level Chrome, it means you're likely not running
elevated, and thus you can't run UninstallMultiChromeFrameIfPresent() for a
system-level multi-CF, right? You could try to elevate (don't think we do that
currently?), but that could also fail.

So this code path would hit if a multi-CF is still present and you try to
install user-level Chrome, right?

In fact, isn't this a bigger problem for which we don't have a current solution
for?

[grt (UTC plus 2), 2014-01-10 20:15:13]

On 2014/01/10 16:28:02, gab wrote:
>
https://codereview.chromium.org/132583002/diff/30001/chrome/installer/setup/s...
> File chrome/installer/setup/setup_main.cc (right):
> 
>
https://codereview.chromium.org/132583002/diff/30001/chrome/installer/setup/s...
> chrome/installer/setup/setup_main.cc:585: NOTREACHED();
> On 2014/01/10 12:10:52, grt wrote:
> > On 2014/01/09 21:59:15, gab wrote:
> > > It is possible that an old multi-install system-level CF remains on the
> system
> > > though?
> > 
> > No. They would have already been removed by now by
> > UninstallMultiChromeFrameIfPresent in setup_main.
> > 
> > > Wouldn't installing a user-level Chrome in that case trip this?
> > 
> > I don't think so, but please double-check my logic. Thanks.
> 
> Well if you install user-level Chrome, it means you're likely not running
> elevated, and thus you can't run UninstallMultiChromeFrameIfPresent() for a
> system-level multi-CF, right? You could try to elevate (don't think we do that
> currently?), but that could also fail.
> 
> So this code path would hit if a multi-CF is still present and you try to
> install user-level Chrome, right?
> 
> In fact, isn't this a bigger problem for which we don't have a current
solution
> for?

You're right that a user-level install/update of Chrome can't uninstall
system-level multi-install Chrome Frame. I don't think that's an issue, though.
There's no conflict with installing user-level Chrome when a system-level Chrome
Frame is present (multi-install or not). If system-level Chrome is present
(multi-install or not), it will be launched. Otherwise, the user-level Chrome
install will succeed. Do you see the code doing something different?

[gab, 2014-01-10 20:41:38]

https://codereview.chromium.org/132583002/diff/30001/chrome/installer/setup/s...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/132583002/diff/30001/chrome/installer/setup/s...
chrome/installer/setup/setup_main.cc:585: NOTREACHED();
On 2014/01/10 16:28:03, gab wrote:
> On 2014/01/10 12:10:52, grt wrote:
> > On 2014/01/09 21:59:15, gab wrote:
> > > It is possible that an old multi-install system-level CF remains on the
> system
> > > though?
> > 
> > No. They would have already been removed by now by
> > UninstallMultiChromeFrameIfPresent in setup_main.
> > 
> > > Wouldn't installing a user-level Chrome in that case trip this?
> > 
> > I don't think so, but please double-check my logic. Thanks.
> 
> Well if you install user-level Chrome, it means you're likely not running
> elevated, and thus you can't run UninstallMultiChromeFrameIfPresent() for a
> system-level multi-CF, right? You could try to elevate (don't think we do that
> currently?), but that could also fail.
> 
> So this code path would hit if a multi-CF is still present and you try to
> install user-level Chrome, right?
> 
> In fact, isn't this a bigger problem for which we don't have a current
solution
> for?

Ah ok, just realized this was doing the check whether the product being
installed is also found at system-level (not whether any other product is found
at system-level).

So indeed existing CF installs don't matter. What matters is that we are no
longer installing CF.

How about changing the comment to:

// It's no longer possible for the product being installed to be anything other
than Chrome.

?

Other than that, lgtm!

[grt (UTC plus 2), 2014-01-10 21:42:27]

https://codereview.chromium.org/132583002/diff/30001/chrome/installer/setup/s...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/132583002/diff/30001/chrome/installer/setup/s...
chrome/installer/setup/setup_main.cc:585: NOTREACHED();
On 2014/01/10 20:41:39, gab wrote:
> On 2014/01/10 16:28:03, gab wrote:
> > On 2014/01/10 12:10:52, grt wrote:
> > > On 2014/01/09 21:59:15, gab wrote:
> > > > It is possible that an old multi-install system-level CF remains on the
> > system
> > > > though?
> > > 
> > > No. They would have already been removed by now by
> > > UninstallMultiChromeFrameIfPresent in setup_main.
> > > 
> > > > Wouldn't installing a user-level Chrome in that case trip this?
> > > 
> > > I don't think so, but please double-check my logic. Thanks.
> > 
> > Well if you install user-level Chrome, it means you're likely not running
> > elevated, and thus you can't run UninstallMultiChromeFrameIfPresent() for a
> > system-level multi-CF, right? You could try to elevate (don't think we do
that
> > currently?), but that could also fail.
> > 
> > So this code path would hit if a multi-CF is still present and you try to
> > install user-level Chrome, right?
> > 
> > In fact, isn't this a bigger problem for which we don't have a current
> solution
> > for?
> 
> Ah ok, just realized this was doing the check whether the product being
> installed is also found at system-level (not whether any other product is
found
> at system-level).
> 
> So indeed existing CF installs don't matter. What matters is that we are no
> longer installing CF.
> 
> How about changing the comment to:
> 
> // It's no longer possible for the product being installed to be anything
other
> than Chrome.
> 
> ?
> 
> Other than that, lgtm!

Done.

Thanks for putting a lot of thought into this. I appreciate the scrutiny!

[commit-bot: I haz the power, 2014-01-10 21:44:25]

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

[commit-bot: I haz the power, 2014-01-10 23:37:45]

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

[commit-bot: I haz the power, 2014-01-11 02:12:55]

Change committed as 244305