Bump os-upgrade component of Active Setup version on-os-upgrade.

Bump os-upgrade component of Active Setup version on-os-upgrade.

Generalizes Active Setup version code that was previously specific to install_worker.cc.

BUG=488247, 502363
TEST=setup_unittests.exe --gtest_filter=InstallWorkerTest.AddAndRunActiveSetupWorkItems
install_util_unittests.exe (TODO)

[gab, 2015-07-01 02:31:57]

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

[gab, 2015-07-01 02:31:57]

@grt, WDYT of this approach as a final step on top of
https://codereview.chromium.org/1213913002/ ?

I think we should eventually split the Active Setup bits out of InstallUtil into
their own component, but for the sake of a "simpler" merge for now I think it
perhaps makes sense to put them in InstallUtil?

Let me know if that works for you and I'll add install_util_unittests for this.

Do you agree that it still makes sense to do the previous 2 steps as they are
now? i.e. we could simply use a CallbackWorkItem with this new method rather
than augment SetRegWorkItem, but I think I still prefer augmenting that API for
the integrated Rollback functionality.

Thanks!
Gab

[grt (UTC plus 2), 2015-07-01 15:59:14]

looks good. some small tweaks below.

https://codereview.chromium.org/1214163008/diff/1/chrome/installer/util/insta...
File chrome/installer/util/install_util.cc (right):

https://codereview.chromium.org/1214163008/diff/1/chrome/installer/util/insta...
chrome/installer/util/install_util.cc:139: std::vector<base::string16>
version_components = base::SplitString(
#include <vector>

https://codereview.chromium.org/1214163008/diff/1/chrome/installer/util/insta...
chrome/installer/util/install_util.cc:169: base::win::RegKey active_setup_key(
this will create the key if it doesn't exist. i don't think this is what you
really want here since there's no value to update in that case. i think it's
better to use:
  base::win::RegKey active_setup_key;
  if (active_setup_key.Open(...) != ERROR_SUCCESS) {
    ...log and return false;
  }

https://codereview.chromium.org/1214163008/diff/1/chrome/installer/util/insta...
chrome/installer/util/install_util.cc:171: KEY_QUERY_VALUE | KEY_SET_VALUE |
KEY_WOW64_32KEY);
AddActiveSetupWorkItems uses kWow64Default, so remove KEY_WOW64_32KEY here.

https://codereview.chromium.org/1214163008/diff/1/chrome/installer/util/insta...
chrome/installer/util/install_util.cc:173: VLOG(1) << "Active Setup key
invalid.";
LOG(ERROR) here and line 180? if not, why?

https://codereview.chromium.org/1214163008/diff/1/chrome/installer/util/insta...
chrome/installer/util/install_util.cc:184: base::string16 updated_version =
GetActiveSetupVersionFromExisting(
if |existing_version| is somehow malformed such that the split on line 139
doesn't satisfy the condition on 144, i think the right thing to do is for it to
end up being set to kActiveSetupMajorVersion,0,1,0 rather than 0,0,1,0.

https://codereview.chromium.org/1214163008/diff/1/chrome/installer/util/insta...
chrome/installer/util/install_util.cc:188: ERROR_SUCCESS;
is this how git cl format does it? i think this should be 4-space indent, not
aligned with "active_setup_key".

[gab, 2015-07-02 03:50:43]

Some updates, some replies, PTAL.

Thanks,
Gab

https://codereview.chromium.org/1214163008/diff/1/chrome/installer/util/insta...
File chrome/installer/util/install_util.cc (right):

https://codereview.chromium.org/1214163008/diff/1/chrome/installer/util/insta...
chrome/installer/util/install_util.cc:139: std::vector<base::string16>
version_components = base::SplitString(
On 2015/07/01 15:59:14, grt wrote:
> #include <vector>

Done.

https://codereview.chromium.org/1214163008/diff/1/chrome/installer/util/insta...
chrome/installer/util/install_util.cc:169: base::win::RegKey active_setup_key(
On 2015/07/01 15:59:14, grt wrote:
> this will create the key if it doesn't exist. i don't think this is what you
> really want here since there's no value to update in that case. i think it's
> better to use:
>   base::win::RegKey active_setup_key;
>   if (active_setup_key.Open(...) != ERROR_SUCCESS) {
>     ...log and return false;
>   }

Done.

https://codereview.chromium.org/1214163008/diff/1/chrome/installer/util/insta...
chrome/installer/util/install_util.cc:171: KEY_QUERY_VALUE | KEY_SET_VALUE |
KEY_WOW64_32KEY);
On 2015/07/01 15:59:14, grt wrote:
> AddActiveSetupWorkItems uses kWow64Default, so remove KEY_WOW64_32KEY here.

Done.

https://codereview.chromium.org/1214163008/diff/1/chrome/installer/util/insta...
chrome/installer/util/install_util.cc:173: VLOG(1) << "Active Setup key
invalid.";
On 2015/07/01 15:59:13, grt wrote:
> LOG(ERROR) here and line 180? if not, why?

I figured the callee already logs an error if this returns false and the detail
as to exactly what failed is perhaps more in the realm of "verbose-logging"?
WDYT>

https://codereview.chromium.org/1214163008/diff/1/chrome/installer/util/insta...
chrome/installer/util/install_util.cc:184: base::string16 updated_version =
GetActiveSetupVersionFromExisting(
On 2015/07/01 15:59:14, grt wrote:
> if |existing_version| is somehow malformed such that the split on line 139
> doesn't satisfy the condition on 144, i think the right thing to do is for it
to
> end up being set to kActiveSetupMajorVersion,0,1,0 rather than 0,0,1,0.

I thought about that too, not sure how to structure the API for this to fit
nicely though. As-is 43,0,1,0 would eventually come true on the next auto-update
-- do we think corruption is likely enough to care about a more complex solution
that supports immediate cleanup (I guess the beacons timing and potential
automatic shortcut installs on-os-upgrade become can of weird if we let this
happen..)? This is where I would potentially see the need to extract this code
to its own component that manages ALL of the Active Setup version logic...

https://codereview.chromium.org/1214163008/diff/1/chrome/installer/util/insta...
chrome/installer/util/install_util.cc:188: ERROR_SUCCESS;
On 2015/07/01 15:59:14, grt wrote:
> is this how git cl format does it? i think this should be 4-space indent, not
> aligned with "active_setup_key".

Yea this is git cl format (I agree with you but this didn't feel ugly enough for
me to turn down the change it made -- whereas I did turn it down when it did the
same thing on 178-179 above, putting ReadValue(x,y) on a single line ending with
"!=" with ERROR_SUCCESS aligned flush on the next line...).

[gab, 2015-07-02 03:51:15]

Patchset #3 (id:40001) has been deleted

[gab, 2015-07-07 14:25:54]

As discussed, went with what I think is a cleaner approach in
https://codereview.chromium.org/1223953003/ +
https://codereview.chromium.org/1223933005/.

Closing this issue.