Remove installation of Chrome Frame.

Remove installation of Chrome Frame.

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

[grt (UTC plus 2), 2013-12-17 18:49:03]

See
https://docs.google.com/a/google.com/document/d/1Qta8dVSNh8Uc5SOmUnKDGTuR5kkh...
for logic I applied for these changes. Thanks.

[gab, 2013-12-19 01:18:31]

So all that's left for CF is uninstall logic?

product/installer_state awareness of chrome_frame (and switches) still exist for
uninstall purposes only?

lgtm w/ comments below; this is beautiful :)!

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/setup/i...
File chrome/installer/setup/install.cc (left):

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/setup/i...
chrome/installer/setup/install.cc:518: if
(!RemoveFromMovesPendingReboot(installer_state.target_path()))
This appears to have been the only remaining call to this method; remove it too?

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/setup/i...
File chrome/installer/setup/install.cc (right):

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/setup/i...
chrome/installer/setup/install.cc:510:
DCHECK(installer_state.products().size());
Replace check for foo.size(), but !foo.empty()

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/setup/i...
File chrome/installer/setup/install_worker.cc (left):

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:1423:
uninstall_cmd->AppendSwitch(installer::switches::kDeleteProfile);
Add a comment at the uninstall code-site where this switch is still used to say
when it used to be set?

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/setup/s...
File chrome/installer/setup/setup_main.cc (left):

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/setup/s...
chrome/installer/setup/setup_main.cc:1549: const uint32 kAppHostBit = 1 <<
BrowserDistribution::CHROME_APP_HOST;
kAppHostBit also appears to be unused, we can probably much simplify the logic
around |higher_products| above now that only CHROME_BROWSER seems to matter.

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/util/in...
File chrome/installer/util/installer_state_unittest.cc (right):

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/util/in...
chrome/installer/util/installer_state_unittest.cc:640: // Confirm that the old
state is gone.
What is re-initializing InstallerState supposed to be testing? Why isn't this a
separate test?

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/util/in...
chrome/installer/util/installer_state_unittest.cc:645:
EXPECT_TRUE(wcsstr(installer_state.target_path().value().c_str(),
optional: StringUtil has methods to do this, I don't see a reason to use wcsstr
here.

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/util/lo...
File chrome/installer/util/logging_installer.cc (right):

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/util/lo...
chrome/installer/util/logging_installer.cc:126: log_path =
log_path.Append(kLogFilename);
Rename log_path -> tmp_path; and return tmp_path.Append(kLogFilename) directly.

Then each block is 1 line and you can remove the {}

(optional: you can remove {} from line 117-119 too)

[grt (UTC plus 2), 2013-12-19 20:57:05]

Thanks, ptal.

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/setup/i...
File chrome/installer/setup/install.cc (left):

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/setup/i...
chrome/installer/setup/install.cc:518: if
(!RemoveFromMovesPendingReboot(installer_state.target_path()))
On 2013/12/19 01:18:31, gab wrote:
> This appears to have been the only remaining call to this method; remove it
too?

I've re-added Erik's TODO since I think it still applies. I wonder if there's
actually a bug here for the non-Chrome Frame case.

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/setup/i...
File chrome/installer/setup/install.cc (right):

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/setup/i...
chrome/installer/setup/install.cc:510:
DCHECK(installer_state.products().size());
On 2013/12/19 01:18:31, gab wrote:
> Replace check for foo.size(), but !foo.empty()

Done.

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/setup/i...
File chrome/installer/setup/install_worker.cc (left):

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:1423:
uninstall_cmd->AppendSwitch(installer::switches::kDeleteProfile);
On 2013/12/19 01:18:31, gab wrote:
> Add a comment at the uninstall code-site where this switch is still used to
say
> when it used to be set?

There's already one there saying that MSI uninstalls explicitly include the
flag, so I think we're good.

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/setup/s...
File chrome/installer/setup/setup_main.cc (left):

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/setup/s...
chrome/installer/setup/setup_main.cc:1549: const uint32 kAppHostBit = 1 <<
BrowserDistribution::CHROME_APP_HOST;
On 2013/12/19 01:18:31, gab wrote:
> kAppHostBit also appears to be unused, we can probably much simplify the logic
> around |higher_products| above now that only CHROME_BROWSER seems to matter.

I'd like to limit the scope of this change to just CF stuff. Cleaning up app
host stuff is covered by http://crbug.com/297647.

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/util/in...
File chrome/installer/util/installer_state_unittest.cc (right):

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/util/in...
chrome/installer/util/installer_state_unittest.cc:640: // Confirm that the old
state is gone.
On 2013/12/19 01:18:31, gab wrote:
> What is re-initializing InstallerState supposed to be testing? Why isn't this
a
> separate test?

This test is called InitializeTwice. It tests that calling .Initialize() on an
already-initialized instance properly re-initializes all state. I'm not sure
what in this would be pulled into a separate test.

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/util/in...
chrome/installer/util/installer_state_unittest.cc:645:
EXPECT_TRUE(wcsstr(installer_state.target_path().value().c_str(),
On 2013/12/19 01:18:31, gab wrote:
> optional: StringUtil has methods to do this, I don't see a reason to use
wcsstr
> here.

I must be missing them. I see StartsWith and EndsWith, but not Contains or
something that sounds like it.

[gab, 2013-12-20 19:05:53]

lgtm % first comment about leaving unreachable code :(

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/setup/i...
File chrome/installer/setup/install.cc (left):

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/setup/i...
chrome/installer/setup/install.cc:518: if
(!RemoveFromMovesPendingReboot(installer_state.target_path()))
On 2013/12/19 20:57:06, grt wrote:
> On 2013/12/19 01:18:31, gab wrote:
> > This appears to have been the only remaining call to this method; remove it
> too?
> 
> I've re-added Erik's TODO since I think it still applies. I wonder if there's
> actually a bug here for the non-Chrome Frame case.

Hmmm... so we're going to leave a call that's used by nobody else than a
comment?!

I don't actually want this CL to take this on, but we should make an action item
(i.e. a bug + link to bug in comment) to promptly look into this, rotting
unreachable code isn't desired.

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/util/in...
File chrome/installer/util/installer_state_unittest.cc (right):

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/util/in...
chrome/installer/util/installer_state_unittest.cc:640: // Confirm that the old
state is gone.
On 2013/12/19 20:57:06, grt wrote:
> On 2013/12/19 01:18:31, gab wrote:
> > What is re-initializing InstallerState supposed to be testing? Why isn't
this
> a
> > separate test?
> 
> This test is called InitializeTwice. It tests that calling .Initialize() on an
> already-initialized instance properly re-initializes all state. I'm not sure
> what in this would be pulled into a separate test.

Ah ok, I see what this is testing now, my bad; it had nothing to do with CF
other than that CF happened to be part of the command-line to test re-initialize
with.

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/util/in...
chrome/installer/util/installer_state_unittest.cc:645:
EXPECT_TRUE(wcsstr(installer_state.target_path().value().c_str(),
On 2013/12/19 20:57:06, grt wrote:
> On 2013/12/19 01:18:31, gab wrote:
> > optional: StringUtil has methods to do this, I don't see a reason to use
> wcsstr
> > here.
> 
> I must be missing them. I see StartsWith and EndsWith, but not Contains or
> something that sounds like it.

I can't find anything other than "base/i18n/string_search.h" either (which is
overkill for this); weird, I could swear we had those...

[grt (UTC plus 2), 2013-12-23 17:58:34]

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/setup/i...
File chrome/installer/setup/install.cc (left):

https://codereview.chromium.org/117433002/diff/60001/chrome/installer/setup/i...
chrome/installer/setup/install.cc:518: if
(!RemoveFromMovesPendingReboot(installer_state.target_path()))
On 2013/12/20 19:05:54, gab wrote:
> On 2013/12/19 20:57:06, grt wrote:
> > On 2013/12/19 01:18:31, gab wrote:
> > > This appears to have been the only remaining call to this method; remove
it
> > too?
> > 
> > I've re-added Erik's TODO since I think it still applies. I wonder if
there's
> > actually a bug here for the non-Chrome Frame case.
> 
> Hmmm... so we're going to leave a call that's used by nobody else than a
> comment?!

Well, when you put it that way...

> I don't actually want this CL to take this on, but we should make an action
item
> (i.e. a bug + link to bug in comment) to promptly look into this, rotting
> unreachable code isn't desired.

Agreed. To play devil's advocate, though, it will rot no more or less now than
it would have before since it has test coverage.

I've noodled in my noodle about Erik's concern. From memory, this was needed for
Chrome Frame but not Chrome since the installer requires the user to quit Chrome
before proceeding. As such, uninstalling Chrome should always be able to delete
the files without having to resort to the DeleteAfterRebootHelper. In practice,
there is a race so perhaps the safest thing to do is to leave the call here for
all products. I can't think of a downside to this, so I've made it so.

Thanks for pushing back on this.

[commit-bot: I haz the power, 2013-12-23 17:58:56]

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

[gab, 2013-12-23 18:08:27]

Thanks, typo below.

Cheers,
Gab

https://codereview.chromium.org/117433002/diff/250001/chrome/installer/setup/...
File chrome/installer/setup/install.cc (right):

https://codereview.chromium.org/117433002/diff/250001/chrome/installer/setup/...
chrome/installer/setup/install.cc:517: << "Error accessing pending mvoes
value.";
nit: s/mvoes/moves

[gab, 2013-12-23 18:08:42]

Thanks, re-lgtm % typo below.

Cheers,
Gab

[grt (UTC plus 2), 2013-12-23 18:12:21]

Doh!

https://codereview.chromium.org/117433002/diff/250001/chrome/installer/setup/...
File chrome/installer/setup/install.cc (right):

https://codereview.chromium.org/117433002/diff/250001/chrome/installer/setup/...
chrome/installer/setup/install.cc:517: << "Error accessing pending mvoes
value.";
On 2013/12/23 18:08:29, gab wrote:
> nit: s/mvoes/moves

Done.

[commit-bot: I haz the power, 2013-12-23 18:12:40]

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

[commit-bot: I haz the power, 2013-12-23 20:34:12]

Change committed as 242400

[Nikita (slow), 2013-12-24 13:22:54]

A revert of this CL has been created in
https://codereview.chromium.org/115993006/ by nkostylev@chromium.org.

The reason for reverting is: Reverting this change just to confirm that sizes
change is triggered by it..

[Nikita (slow), 2013-12-24 13:26:08]

Please ignore this test revert.
It is not applying cleanly so I'll just abandon it.

[gab, 2013-12-24 15:09:25]

On 2013/12/24 13:26:08, Nikita Kostylev OOO Dec 26-29 wrote:
> Please ignore this test revert.
> It is not applying cleanly so I'll just abandon it.

FWIW, sizes have **improved** thanks to this CL; the script is smart enough to
also fail on improvements (forcing us to lower the expectations accordingly),
I'll lower the expectations now.

Cheers,
Gab

[Nikita (slow), 2013-12-24 15:21:02]

Yes, that was an improvement.
See https://codereview.chromium.org/117413007/

[gab, 2013-12-24 15:41:59]

On 2013/12/24 15:21:02, Nikita Kostylev OOO Dec 26-29 wrote:
> Yes, that was an improvement.
> See https://codereview.chromium.org/117413007/

Right, but still need to update the expectations for setup.exe/chrome.exe, see
https://codereview.chromium.org/120183005/

Cheers,
Gab