Support rollback with installerdata in mini_installer

Support downgrade with installerdata in mini_installer

BUG=607592
Committed: https://crrev.com/48268397f19a125554433ad6f324d663e25ad4c5
Cr-Commit-Position: refs/heads/master@{#390957}

[zmin, 2016-04-26 15:38:57]

Description was changed from

==========
Support rollback with installerdata in mini_installer

BUG=
==========

to

==========
Support rollback with installerdata in mini_installer

Pair with: cr/120811224

BUG=
==========

[zmin, 2016-04-26 15:43:28]

zmin@chromium.org changed reviewers:
+ grt@chromium.org

[zmin, 2016-04-26 15:43:28]

Test Manually with msi file and static build.

Auto test will be updated once I got test frame update.

[Georges Khalil, 2016-04-27 12:38:02]

georgesak@chromium.org changed reviewers:
+ georgesak@chromium.org

[Georges Khalil, 2016-04-27 12:38:02]

Flyby comments.

https://codereview.chromium.org/1914213002/diff/1/chrome/installer/setup/setu...
File chrome/installer/setup/setup_util.cc (right):

https://codereview.chromium.org/1914213002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_util.cc:547: std::string rollback_flag = "";
Should we always return false for non msi installs?

https://codereview.chromium.org/1914213002/diff/1/chrome/installer/setup/setu...
File chrome/installer/setup/setup_util.h (right):

https://codereview.chromium.org/1914213002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_util.h:124: bool IsRollbackEnabled(const
MasterPreferences& prefs);
nit: Add comment.
nit: I think naming this CanRollbackVersion makes more sense.

https://codereview.chromium.org/1914213002/diff/1/chrome/installer/util/util_...
File chrome/installer/util/util_constants.h (right):

https://codereview.chromium.org/1914213002/diff/1/chrome/installer/util/util_...
chrome/installer/util/util_constants.h:91: IN_USE_ROLLBACK      = 58,  // Chrome
successfully rollback but current
nit (comment suggestion): Successful chrome rollback but current version is
still running.

https://codereview.chromium.org/1914213002/diff/1/chrome/installer/util/util_...
chrome/installer/util/util_constants.h:93: OLD_VERSION_ROLLBACK = 59,  // Chrome
successfully rollback to old version.
nit (comment suggestion): Successful chrome rollback to an older version.

[zmin, 2016-04-27 15:20:19]

https://codereview.chromium.org/1914213002/diff/1/chrome/installer/setup/setu...
File chrome/installer/setup/setup_util.cc (right):

https://codereview.chromium.org/1914213002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_util.cc:547: std::string rollback_flag = "";
On 2016/04/27 12:38:02, Georges Khalil (slow) wrote:
> Should we always return false for non msi installs?

The rollback flag is stored in the installer data which is same as msi flag.
Anyone wants to rollback Chrome with mini_installer can simply change both flags
together. Here is a sample of installer data:

{"distribution":{"msi":false,"system_level":false,...,"rollback":"Enable"}}

https://codereview.chromium.org/1914213002/diff/1/chrome/installer/setup/setu...
File chrome/installer/setup/setup_util.h (right):

https://codereview.chromium.org/1914213002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_util.h:124: bool IsRollbackEnabled(const
MasterPreferences& prefs);
On 2016/04/27 12:38:02, Georges Khalil (slow) wrote:
> nit: Add comment.
> nit: I think naming this CanRollbackVersion makes more sense.

Done.

https://codereview.chromium.org/1914213002/diff/1/chrome/installer/util/util_...
File chrome/installer/util/util_constants.h (right):

https://codereview.chromium.org/1914213002/diff/1/chrome/installer/util/util_...
chrome/installer/util/util_constants.h:91: IN_USE_ROLLBACK      = 58,  // Chrome
successfully rollback but current
On 2016/04/27 12:38:02, Georges Khalil (slow) wrote:
> nit (comment suggestion): Successful chrome rollback but current version is
> still running.

Done.

https://codereview.chromium.org/1914213002/diff/1/chrome/installer/util/util_...
chrome/installer/util/util_constants.h:93: OLD_VERSION_ROLLBACK = 59,  // Chrome
successfully rollback to old version.
On 2016/04/27 12:38:02, Georges Khalil (slow) wrote:
> nit (comment suggestion): Successful chrome rollback to an older version.

Done.

[zmin, 2016-04-27 21:00:06]

Update with https://critique.corp.google.com/#review/120811224.

[Georges Khalil, 2016-04-28 14:36:30]

https://codereview.chromium.org/1914213002/diff/1/chrome/installer/setup/setu...
File chrome/installer/setup/setup_util.cc (right):

https://codereview.chromium.org/1914213002/diff/1/chrome/installer/setup/setu...
chrome/installer/setup/setup_util.cc:547: std::string rollback_flag = "";
On 2016/04/27 15:20:18, zmin wrote:
> On 2016/04/27 12:38:02, Georges Khalil (slow) wrote:
> > Should we always return false for non msi installs?
> 
> The rollback flag is stored in the installer data which is same as msi flag.
> Anyone wants to rollback Chrome with mini_installer can simply change both
flags
> together. Here is a sample of installer data:
> 
> {"distribution":{"msi":false,"system_level":false,...,"rollback":"Enable"}}
> 

Gotcha.

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

https://codereview.chromium.org/1914213002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/install.cc:188: const bool is_rollback_enabled) {
nit: change name to can_downgrade_version for consistency.

https://codereview.chromium.org/1914213002/diff/40001/chrome/installer/setup/...
File chrome/installer/setup/setup_util.h (right):

https://codereview.chromium.org/1914213002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_util.h:124: // Returns true if rollback is enabld
by installer data.
nit: update this comment for consistency (also enabld -> enabled).

https://codereview.chromium.org/1914213002/diff/40001/chrome/installer/util/u...
File chrome/installer/util/util_constants.h (right):

https://codereview.chromium.org/1914213002/diff/40001/chrome/installer/util/u...
chrome/installer/util/util_constants.h:91: IN_USE_ROLLBACK      = 58,  //
Successful chrome rollback but current version
nit: change names of both constants and comments for consistency.

[zmin, 2016-04-28 14:56:03]

Patchset #4 (id:60001) has been deleted

[zmin, 2016-04-28 15:02:31]

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

https://codereview.chromium.org/1914213002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/install.cc:188: const bool is_rollback_enabled) {
On 2016/04/28 14:36:30, Georges Khalil (slow) wrote:
> nit: change name to can_downgrade_version for consistency.

Done.

https://codereview.chromium.org/1914213002/diff/40001/chrome/installer/setup/...
File chrome/installer/setup/setup_util.h (right):

https://codereview.chromium.org/1914213002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/setup_util.h:124: // Returns true if rollback is enabld
by installer data.
On 2016/04/28 14:36:30, Georges Khalil (slow) wrote:
> nit: update this comment for consistency (also enabld -> enabled).

Done.

https://codereview.chromium.org/1914213002/diff/40001/chrome/installer/util/u...
File chrome/installer/util/util_constants.h (right):

https://codereview.chromium.org/1914213002/diff/40001/chrome/installer/util/u...
chrome/installer/util/util_constants.h:91: IN_USE_ROLLBACK      = 58,  //
Successful chrome rollback but current version
On 2016/04/28 14:36:30, Georges Khalil (slow) wrote:
> nit: change names of both constants and comments for consistency.

Done.

[zmin, 2016-04-28 16:45:24]

Description was changed from

==========
Support rollback with installerdata in mini_installer

Pair with: cr/120811224

BUG=
==========

to

==========
Support downgrade with installerdata in mini_installer

BUG=607592
==========

[grt (UTC plus 2), 2016-04-28 16:55:30]

looks pretty good. some small comments.

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

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/setup/...
chrome/installer/setup/install.cc:188: const bool can_downgrade_version) {
nit: no const for value type params

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/setup/...
chrome/installer/setup/install.cc:240: bool is_new_chrome_exe_exists =
base::PathExists(new_chrome_exe);
nit: new_chrome_exe_exists

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/setup/...
chrome/installer/setup/install.cc:253: VLOG(1) << "Version rollback to " <<
new_version << " while running "
rollback -> downgraded here and below. "rollback" is used in other contexts to
mean "an operation failed, so wind things back to where they were before we
started."

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/setup/...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/setup/...
chrome/installer/setup/setup_main.cc:1519: LOG(ERROR) << "Higher version of "
i think this ERROR will be confusing to see in the logs when downgrade is
allowed. could you make the log level conditional? VLOG(1) if allowed, ERROR
otherwise?

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/setup/...
File chrome/installer/setup/setup_util.cc (right):

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/setup/...
chrome/installer/setup/setup_util.cc:548:
prefs.GetBool(master_preferences::kAllowDowngrade, &allow_downgrade);
  return prefs.GetBool(...) && allow_downgrade;

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/setup/...
File chrome/installer/setup/setup_util.h (right):

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/setup/...
chrome/installer/setup/setup_util.h:125: bool CanDowngradeVersion(const
MasterPreferences& prefs);
IsDowngradeAllowed?

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/util/m...
File chrome/installer/util/master_preferences_constants.h (right):

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/util/m...
chrome/installer/util/master_preferences_constants.h:95: // Boolean. Bypass
reinstall version check if true.
comment nit:
// Boolean. Allows Chrome to be downgraded to a previous version if true.

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/util/u...
File chrome/installer/util/util_constants.h (right):

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/util/u...
chrome/installer/util/util_constants.h:91: IN_USE_DOWNGRADE     = 58,  //
Successfully downgrade chrome but current
please add these to InstallStatus enum in
tools/metrics/histograms/histograms.xml

[zmin, 2016-04-28 17:51:29]

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

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/setup/...
chrome/installer/setup/install.cc:188: const bool can_downgrade_version) {
On 2016/04/28 16:55:30, grt wrote:
> nit: no const for value type params

Done.

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/setup/...
chrome/installer/setup/install.cc:240: bool is_new_chrome_exe_exists =
base::PathExists(new_chrome_exe);
On 2016/04/28 16:55:30, grt wrote:
> nit: new_chrome_exe_exists

Done.

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/setup/...
chrome/installer/setup/install.cc:253: VLOG(1) << "Version rollback to " <<
new_version << " while running "
On 2016/04/28 16:55:30, grt wrote:
> rollback -> downgraded here and below. "rollback" is used in other contexts to
> mean "an operation failed, so wind things back to where they were before we
> started."

Done.

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/setup/...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/setup/...
chrome/installer/setup/setup_main.cc:1519: LOG(ERROR) << "Higher version of "
On 2016/04/28 16:55:30, grt wrote:
> i think this ERROR will be confusing to see in the logs when downgrade is
> allowed. could you make the log level conditional? VLOG(1) if allowed, ERROR
> otherwise?

How about we also bypass this part when downgrade is allowed. If we don't care
about whether there's higher version chrome or not, it doesn't make sense to
find them as well.

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/setup/...
File chrome/installer/setup/setup_util.cc (right):

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/setup/...
chrome/installer/setup/setup_util.cc:548:
prefs.GetBool(master_preferences::kAllowDowngrade, &allow_downgrade);
On 2016/04/28 16:55:30, grt wrote:
>   return prefs.GetBool(...) && allow_downgrade;

Done.

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/setup/...
File chrome/installer/setup/setup_util.h (right):

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/setup/...
chrome/installer/setup/setup_util.h:125: bool CanDowngradeVersion(const
MasterPreferences& prefs);
On 2016/04/28 16:55:30, grt wrote:
> IsDowngradeAllowed?

Actually I love this one. How about you, @georgesak?

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/util/m...
File chrome/installer/util/master_preferences_constants.h (right):

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/util/m...
chrome/installer/util/master_preferences_constants.h:95: // Boolean. Bypass
reinstall version check if true.
On 2016/04/28 16:55:30, grt wrote:
> comment nit:
> // Boolean. Allows Chrome to be downgraded to a previous version if true.

Done.

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/util/u...
File chrome/installer/util/util_constants.h (right):

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/util/u...
chrome/installer/util/util_constants.h:91: IN_USE_DOWNGRADE     = 58,  //
Successfully downgrade chrome but current
On 2016/04/28 16:55:30, grt wrote:
> please add these to InstallStatus enum in
> tools/metrics/histograms/histograms.xml

Done.

[grt (UTC plus 2), 2016-04-28 20:13:45]

nice! lgtm.

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/setup/...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/1914213002/diff/80001/chrome/installer/setup/...
chrome/installer/setup/setup_main.cc:1519: LOG(ERROR) << "Higher version of "
On 2016/04/28 17:51:29, zmin wrote:
> On 2016/04/28 16:55:30, grt wrote:
> > i think this ERROR will be confusing to see in the logs when downgrade is
> > allowed. could you make the log level conditional? VLOG(1) if allowed, ERROR
> > otherwise?
> 
> How about we also bypass this part when downgrade is allowed. If we don't care
> about whether there's higher version chrome or not, it doesn't make sense to
> find them as well.

sgtm

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

https://codereview.chromium.org/1914213002/diff/100001/chrome/installer/setup...
chrome/installer/setup/install.cc:253: VLOG(1) << "Version downgrade to " <<
new_version << " while running "
"downgraded" sounds more better to me

[zmin, 2016-04-28 20:20:37]

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

https://codereview.chromium.org/1914213002/diff/100001/chrome/installer/setup...
chrome/installer/setup/install.cc:253: VLOG(1) << "Version downgrade to " <<
new_version << " while running "
On 2016/04/28 20:13:45, grt wrote:
> "downgraded" sounds more better to me

Done.

[zmin, 2016-04-28 20:22:16]

zmin@chromium.org changed reviewers:
+ jwd@chromium.org

[zmin, 2016-04-28 20:22:16]

Hi Jesse,

Can you please review the histograms change. 

Thanks,
Owen

[jwd, 2016-05-02 14:44:25]

lgtm

[Georges Khalil, 2016-05-02 14:47:41]

lgtm

[zmin, 2016-05-02 14:48:08]

The CQ bit was checked by zmin@chromium.org

[zmin, 2016-05-02 14:48:08]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1914213002/#ps120001
(title: "cr")

[commit-bot: I haz the power, 2016-05-02 14:48:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1914213002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1914213002/120001

[commit-bot: I haz the power, 2016-05-02 16:09:07]

Description was changed from

==========
Support downgrade with installerdata in mini_installer

BUG=607592
==========

to

==========
Support downgrade with installerdata in mini_installer

BUG=607592
==========

[commit-bot: I haz the power, 2016-05-02 16:09:08]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-05-02 16:10:58]

Description was changed from

==========
Support downgrade with installerdata in mini_installer

BUG=607592
==========

to

==========
Support downgrade with installerdata in mini_installer

BUG=607592

Committed: https://crrev.com/48268397f19a125554433ad6f324d663e25ad4c5
Cr-Commit-Position: refs/heads/master@{#390957}
==========

[commit-bot: I haz the power, 2016-05-02 16:10:59]

Patchset 6 (id:??) landed as
https://crrev.com/48268397f19a125554433ad6f324d663e25ad4c5
Cr-Commit-Position: refs/heads/master@{#390957}