Update Chrome's Active Setup version per component instead of as a single string.

Update Chrome's Active Setup version per component instead of as a single string.

The installer's install/update step is now only responsible to update the
MM part of the MM,XX,YY,ZZ version number.

BUG=488247, 502363
TEST=setup_unittests.exe --gtest_filter=InstallWorkerTest.AddAndRunActiveSetupWorkItems

[gab, 2015-06-26 22:07:43]

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

[gab, 2015-06-26 22:07:44]

Greg PTAL, this is based on top of https://codereview.chromium.org/1220473002/
and will also need to be followed by a small CL to actually bump YY
on-os-upgrade.

Cheers,
Gab

[grt (UTC plus 2), 2015-06-29 17:41:50]

https://codereview.chromium.org/1213913002/diff/1/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/1213913002/diff/1/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:539: // Returns the version to be used
for Active Setup as MM/XX/YY/ZZ where MM is
MM,XX,YY,ZZ?

https://codereview.chromium.org/1213913002/diff/1/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:546: // done by setup.exe
--configure-user- settings on user login by way of Active
--configure-user-settings

https://codereview.chromium.org/1213913002/diff/1/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:556: if (version_components.size() !=
4U) {
  if (version_components.size() != 4U)
    version_components.assign(4, L"0");

https://codereview.chromium.org/1213913002/diff/1/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:566: return
JoinString(version_components, L",");
L','

https://codereview.chromium.org/1213913002/diff/1/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker_unittest.cc (right):

https://codereview.chromium.org/1213913002/diff/1/chrome/installer/setup/inst...
chrome/installer/setup/install_worker_unittest.cc:738: // Write a bogus value to
the key and confirm that executing the work items
I think "bogus" is the wrong word here; I expect a bogus value to be one that
doesn't conform to W,X,Y,Z. This looks like it tests a completely valid value
that should have its major version updated. Could you update the comment here,
and then add a test that does a few bogus values (empty, not a string, not a
W,X,Y,Z string, etc)?

[gab, 2015-06-30 18:36:57]

Patchset #2 (id:20001) has been deleted

[gab, 2015-06-30 18:40:25]

Patchset #3 (id:60001) has been deleted

[gab, 2015-06-30 18:40:39]

Done/replied below, PTAL.

Thanks,
Gab

https://codereview.chromium.org/1213913002/diff/1/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/1213913002/diff/1/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:539: // Returns the version to be used
for Active Setup as MM/XX/YY/ZZ where MM is
On 2015/06/29 17:41:49, grt wrote:
> MM,XX,YY,ZZ?

Major,X,Y,Z -- I don't think Major,Minor,Build,Revision makes sense here because
this is not what we use them for... hence I used X/Y/Z and described each one
further below. Let me know if you think there is a better way of phrasing that.

https://codereview.chromium.org/1213913002/diff/1/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:546: // done by setup.exe
--configure-user- settings on user login by way of Active
On 2015/06/29 17:41:49, grt wrote:
> --configure-user-settings

Done.

https://codereview.chromium.org/1213913002/diff/1/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:556: if (version_components.size() !=
4U) {
On 2015/06/29 17:41:49, grt wrote:
>   if (version_components.size() != 4U)
>     version_components.assign(4, L"0");

Done.

https://codereview.chromium.org/1213913002/diff/1/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:566: return
JoinString(version_components, L",");
On 2015/06/29 17:41:49, grt wrote:
> L','

Done.

https://codereview.chromium.org/1213913002/diff/1/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker_unittest.cc (right):

https://codereview.chromium.org/1213913002/diff/1/chrome/installer/setup/inst...
chrome/installer/setup/install_worker_unittest.cc:738: // Write a bogus value to
the key and confirm that executing the work items
On 2015/06/29 17:41:49, grt wrote:
> I think "bogus" is the wrong word here; I expect a bogus value to be one that
> doesn't conform to W,X,Y,Z. This looks like it tests a completely valid value
> that should have its major version updated. Could you update the comment here,
> and then add a test that does a few bogus values (empty, not a string, not a
> W,X,Y,Z string, etc)?

Refactored the test to make testing all of these values easy.

[grt (UTC plus 2), 2015-06-30 18:54:49]

https://codereview.chromium.org/1213913002/diff/1/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/1213913002/diff/1/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:539: // Returns the version to be used
for Active Setup as MM/XX/YY/ZZ where MM is
On 2015/06/30 18:40:39, gab wrote:
> On 2015/06/29 17:41:49, grt wrote:
> > MM,XX,YY,ZZ?
> 
> Major,X,Y,Z -- I don't think Major,Minor,Build,Revision makes sense here
because
> this is not what we use them for... hence I used X/Y/Z and described each one
> further below. Let me know if you think there is a better way of phrasing
that.

My suggestion was to use commas rather than slashes since that's what the
returned value looks like.

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

https://codereview.chromium.org/1213913002/diff/80001/chrome/installer/setup/...
chrome/installer/setup/install_worker_unittest.cc:707: enum class TestCaseStep {
this all seems pretty complicated. does it all boil down to "put some initial
value in the registry, run the work item with a new value, expect the right
result"? if so, can't this state machine be replaced with either a parameterized
test with (before,after) pairs or a simple loop over the pairs in a big test
function?

https://codereview.chromium.org/1213913002/diff/80001/chrome/installer/setup/...
chrome/installer/setup/install_worker_unittest.cc:764: SCOPED_TRACE(i);
kewl

[gab, 2015-07-02 03:12:33]

Addressed remaining comments, PTAL.

Thanks,
Gab

PS: Please CQ if it LGTYs.

https://codereview.chromium.org/1213913002/diff/1/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/1213913002/diff/1/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:539: // Returns the version to be used
for Active Setup as MM/XX/YY/ZZ where MM is
On 2015/06/30 18:54:49, grt wrote:
> On 2015/06/30 18:40:39, gab wrote:
> > On 2015/06/29 17:41:49, grt wrote:
> > > MM,XX,YY,ZZ?
> > 
> > Major,X,Y,Z -- I don't think Major,Minor,Build,Revision makes sense here
> because
> > this is not what we use them for... hence I used X/Y/Z and described each
one
> > further below. Let me know if you think there is a better way of phrasing
> that.
> 
> My suggestion was to use commas rather than slashes since that's what the
> returned value looks like.

Ah I see, hadn't noticed I'd used slashes (indeed incorrect => fixed) and was
thus thinking you were asking why I was labeling the components MM, XX, YY, and
ZZ.

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

https://codereview.chromium.org/1213913002/diff/80001/chrome/installer/setup/...
chrome/installer/setup/install_worker_unittest.cc:707: enum class TestCaseStep {
On 2015/06/30 18:54:49, grt wrote:
> this all seems pretty complicated. does it all boil down to "put some initial
> value in the registry, run the work item with a new value, expect the right
> result"? if so, can't this state machine be replaced with either a
parameterized
> test with (before,after) pairs or a simple loop over the pairs in a big test
> function?

Good point, at first I didn't like having to go through the overhead of the top
part numerous times, but it's really nothing in practice and, if anything, it
actually makes the test more realistic by performing a full "install/update"
from scratch every time.

Implemented this as a parametrized test and got a +74/-83 so I guess we can say
it's simpler for realz :-).

Thanks!

https://codereview.chromium.org/1213913002/diff/80001/chrome/installer/setup/...
chrome/installer/setup/install_worker_unittest.cc:764: SCOPED_TRACE(i);
On 2015/06/30 18:54:49, grt wrote:
> kewl

Yep :-)

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

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.