Uninstall multi-install Chrome Frame when updated.

Uninstall multi-install Chrome Frame when updated.

If multi-install Chrome Frame is present when a multi-install install or
update is processed, CF is uninstalled prior to ordinary processing. The
binaries will be removed if they are not in use. Otherwise, they will
stick around until the next update for which they are not in use.

BUG=316496
R=robertshield@chromium.org

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

[grt (UTC plus 2), 2013-11-29 04:23:03]

Hi Robert,

This appears to do the trick and also fixes a bug or two with the binaries
uninstall code. I'll do some more manual testing before landing. Cheers.

https://codereview.chromium.org/96193003/diff/40001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/96193003/diff/40001/PRESUBMIT.py#newcode839
PRESUBMIT.py:839: r"^chrome[\\\/]installer[\\\/]setup[\\\/].*$"))
LOG() in Chrome's installer for Windows doesn't spam the console, so don't scan
setup's files when looking for bad logging.

https://codereview.chromium.org/96193003/diff/40001/chrome/installer/setup/se...
File chrome/installer/setup/setup_main.cc (left):

https://codereview.chromium.org/96193003/diff/40001/chrome/installer/setup/se...
chrome/installer/setup/setup_main.cc:704: installer::InstallStatus
InstallProducts(
code motion only except for what is now line 902.

https://codereview.chromium.org/96193003/diff/40001/chrome/installer/setup/se...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/96193003/diff/40001/chrome/installer/setup/se...
chrome/installer/setup/setup_main.cc:816: void UninstallBinariesIfUnused(
this is more or less what used to be on line 1788, but now using state that more
accurately reflects what is being done. i believe that the prior code may have
been crashy (although i don't see corresponding crash reports for some reason),
and may have been prone to leaving an empty directory behind during cleanup.

https://codereview.chromium.org/96193003/diff/40001/chrome/installer/setup/se...
chrome/installer/setup/setup_main.cc:902:
UninstallBinariesIfUnused(original_state, *installer_state, &install_status);
this line is the only change between the old InstallProducts and this one.

[grt (UTC plus 2), 2013-11-29 04:24:41]

+maruel for PRESUBMIT.py OWNERS review

[M-A Ruel, 2013-11-29 13:33:58]

presubmit rubberstamped lgtm

[robertshield, 2013-11-29 18:50:25]

As discussed, may want to add the helper dll here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/installer/u...

https://codereview.chromium.org/96193003/diff/70001/chrome/installer/setup/se...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/96193003/diff/70001/chrome/installer/setup/se...
chrome/installer/setup/setup_main.cc:1440: const Product* chrome_frame =
uninstall_state.FindProduct(
s/chrome_frame/chrome_frame_product/ ?

https://codereview.chromium.org/96193003/diff/70001/chrome/installer/setup/se...
chrome/installer/setup/setup_main.cc:1445: // Don't accept no for an answer.
This also suppresses the UI right?

https://codereview.chromium.org/96193003/diff/70001/chrome/installer/util/ins...
File chrome/installer/util/installer_state.h (right):

https://codereview.chromium.org/96193003/diff/70001/chrome/installer/util/ins...
chrome/installer/util/installer_state.h:230: void Clear();
Can we add a unit test for Clear()?

[grt (UTC plus 2), 2013-12-02 15:28:30]

PTAL, thanks.

https://codereview.chromium.org/96193003/diff/70001/chrome/installer/setup/se...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/96193003/diff/70001/chrome/installer/setup/se...
chrome/installer/setup/setup_main.cc:1440: const Product* chrome_frame =
uninstall_state.FindProduct(
On 2013/11/29 18:50:25, robertshield wrote:
> s/chrome_frame/chrome_frame_product/ ? 

Done.

https://codereview.chromium.org/96193003/diff/70001/chrome/installer/setup/se...
chrome/installer/setup/setup_main.cc:1445: // Don't accept no for an answer.
On 2013/11/29 18:50:25, robertshield wrote:
> This also suppresses the UI right?

I had thought it would, but it turns out that the UI is done elsewhere and won't
happen as a result of this call.

https://codereview.chromium.org/96193003/diff/70001/chrome/installer/util/ins...
File chrome/installer/util/installer_state.h (right):

https://codereview.chromium.org/96193003/diff/70001/chrome/installer/util/ins...
chrome/installer/util/installer_state.h:230: void Clear();
On 2013/11/29 18:50:25, robertshield wrote:
> Can we add a unit test for Clear()?

I've added one for re-initializing an instance.

[grt (UTC plus 2), 2013-12-03 02:08:17]

ping

[robertshield, 2013-12-03 02:20:04]

lgtm

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

https://codereview.chromium.org/96193003/diff/120001/chrome/installer/setup/s...
chrome/installer/setup/setup_main.cc:826: LOG(INFO) << "Uninstalling unused
binaries";
VLOG(1) instead?

[grt (UTC plus 2), 2013-12-03 13:37:51]

Will land as-is, and will change logging in a separate change if necessary.
Thanks for the review.

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

https://codereview.chromium.org/96193003/diff/120001/chrome/installer/setup/s...
chrome/installer/setup/setup_main.cc:826: LOG(INFO) << "Uninstalling unused
binaries";
On 2013/12/03 02:20:05, robertshield wrote:
> VLOG(1) instead?

I thought this would be useful to have in the installer log unconditionally. It
should be a one-time message for any given installation. I figure that if there
are problems with this in the wild, it might help to have it in the log when
examining logs from users. Whaddya think?

[commit-bot: I haz the power, 2013-12-03 13:38:07]

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

[commit-bot: I haz the power, 2013-12-03 13:38:11]

Failed to apply patch for PRESUBMIT.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file PRESUBMIT.py
  Hunk #1 FAILED at 835.
  1 out of 1 hunk FAILED -- saving rejects to file PRESUBMIT.py.rej

Patch:       PRESUBMIT.py
Index: PRESUBMIT.py
diff --git a/PRESUBMIT.py b/PRESUBMIT.py
index
f861ab5ab6c8fcfa4ca78cc96103de38df1660e7..0e810be46b72078540f154375c668b827cc9c444
100644
--- a/PRESUBMIT.py
+++ b/PRESUBMIT.py
@@ -835,6 +835,7 @@ def _CheckSpamLogging(input_api, output_api):
                 _TEST_CODE_EXCLUDED_PATHS +
                 input_api.DEFAULT_BLACK_LIST +
                 (r"^base[\\\/]logging\.h$",
+                 r"^chrome[\\\/]installer[\\\/]setup[\\\/].*",
                  r"^remoting[\\\/]base[\\\/]logging\.h$",
                  r"^sandbox[\\\/]linux[\\\/].*",))
   source_file_filter = lambda x: input_api.FilterSourceFile(

[commit-bot: I haz the power, 2013-12-03 19:26:08]

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

[commit-bot: I haz the power, 2013-12-03 23:46:50]

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

[commit-bot: I haz the power, 2013-12-04 02:55:24]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-04 03:29:13]

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

[commit-bot: I haz the power, 2013-12-04 05:52:59]

Change committed as 238588