GCAPI should append to the existing experiment_labels instead of clobbering them.

GCAPI should append to the existing experiment_labels instead of clobbering them.

As described on http://crbug.com/266955#c7

Also adding AtExitManager to gcapi_test.exe; this is required to support MasterPreferences's LazyInstance used by BrowserDistribution, used by google_update's ReadExperimentLabels().

Introducing GCAPITestRegistryOverrider as a class to be added as a member to GCAPI test fixtures that require registry overriding; extracted from the existing GCAPIReactivationTest fixture.

Move Windows-specific variations_util.cc code to experiment_labels_win.cc

BUG=266955
TEST=gcapi_test.exe

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

[gab, 2013-08-29 22:10:53]

Robert/Alexei, PTAL.

Thanks!
Gab

[robertshield, 2013-08-30 01:13:53]

https://codereview.chromium.org/23579003/diff/5001/chrome/common/metrics/vari...
File chrome/common/metrics/variations/variations_util.cc (left):

https://codereview.chromium.org/23579003/diff/5001/chrome/common/metrics/vari...
chrome/common/metrics/variations/variations_util.cc:23: const char
kExperimentLabelSep[] = ";";
This has gone from a char to a wchar_t and doesn't seem to build on some
platforms.

[Alexei Svitkine (slow), 2013-08-30 02:16:47]

https://codereview.chromium.org/23579003/diff/5001/chrome/common/metrics/vari...
File chrome/common/metrics/variations/variations_util.cc (left):

https://codereview.chromium.org/23579003/diff/5001/chrome/common/metrics/vari...
chrome/common/metrics/variations/variations_util.cc:23: const char
kExperimentLabelSep[] = ";";
On 2013/08/30 01:13:53, robertshield wrote:
> This has gone from a char to a wchar_t and doesn't seem to build on some
> platforms.

It should be a char16, since on some platforms wchar_t is 32 bits.

[gab, 2013-08-30 02:43:18]

Done, will add tests to gcapi_tests tomorrow to test the new functionality.

https://codereview.chromium.org/23579003/diff/5001/chrome/common/metrics/vari...
File chrome/common/metrics/variations/variations_util.cc (left):

https://codereview.chromium.org/23579003/diff/5001/chrome/common/metrics/vari...
chrome/common/metrics/variations/variations_util.cc:23: const char
kExperimentLabelSep[] = ";";
On 2013/08/30 02:16:47, Alexei Svitkine wrote:
> On 2013/08/30 01:13:53, robertshield wrote:
> > This has gone from a char to a wchar_t and doesn't seem to build on some
> > platforms.
> 
> It should be a char16, since on some platforms wchar_t is 32 bits.

Good catch; thanks for the hint; done.

[Alexei Svitkine (slow), 2013-08-30 14:17:18]

https://codereview.chromium.org/23579003/diff/22001/chrome/common/metrics/var...
File chrome/common/metrics/variations/variations_util.cc (right):

https://codereview.chromium.org/23579003/diff/22001/chrome/common/metrics/var...
chrome/common/metrics/variations/variations_util.cc:134: labels,
google_update::kExperimentLabelSep, &entries);
Nit: wrap aligned

base::SplitStringUsingSubstr(labels, google_update::kExperimentLabelSep,
                             &entries);

https://codereview.chromium.org/23579003/diff/22001/chrome/installer/gcapi/gc...
File chrome/installer/gcapi/gcapi_omaha_experiment.cc (right):

https://codereview.chromium.org/23579003/diff/22001/chrome/installer/gcapi/gc...
chrome/installer/gcapi/gcapi_omaha_experiment.cc:45: if (existing_label_begin !=
string16::npos) {
I think it would be safer to also check that either existing_label_begin == 0 or
experiment_labels[existing_label_begin - 1] == ','.

So you don't end up finding this in the middle of some other labels.

Alternatively, just do what variations_util.cc does and use
SplitStringUsingSubstr() and iterate over each label and re-add them if they
don't match |label|. This way, even if there multiple existing duplicates,
they'll all be removed.

https://codereview.chromium.org/23579003/diff/22001/chrome/installer/util/goo...
File chrome/installer/util/google_update_constants.cc (right):

https://codereview.chromium.org/23579003/diff/22001/chrome/installer/util/goo...
chrome/installer/util/google_update_constants.cc:12: const char16
kExperimentLabelSep[] = L";";
Unfortunately, you can't use the L prefix for a string16 literal - since L
corresponds to wchar_t which is 32 bit on some platforms.

Perhaps you can change this to an actual single char16 instead of a string.
Then, you can just use ',' for the value. (Because for ascii chars, its the same
value between a char and a char16).

https://codereview.chromium.org/23579003/diff/22001/chrome/installer/util/goo...
File chrome/installer/util/google_update_constants.h (right):

https://codereview.chromium.org/23579003/diff/22001/chrome/installer/util/goo...
chrome/installer/util/google_update_constants.h:10: #include "base/string16.h"
I think this is "base/strings/string16.h" now.

[gab, 2013-08-30 17:53:10]

Thanks, PTAL.

https://codereview.chromium.org/23579003/diff/22001/chrome/common/metrics/var...
File chrome/common/metrics/variations/variations_util.cc (right):

https://codereview.chromium.org/23579003/diff/22001/chrome/common/metrics/var...
chrome/common/metrics/variations/variations_util.cc:134: labels,
google_update::kExperimentLabelSep, &entries);
On 2013/08/30 14:17:19, Alexei Svitkine wrote:
> Nit: wrap aligned
> 
> base::SplitStringUsingSubstr(labels, google_update::kExperimentLabelSep,
>                              &entries);

Done.

https://codereview.chromium.org/23579003/diff/22001/chrome/installer/gcapi/gc...
File chrome/installer/gcapi/gcapi_omaha_experiment.cc (right):

https://codereview.chromium.org/23579003/diff/22001/chrome/installer/gcapi/gc...
chrome/installer/gcapi/gcapi_omaha_experiment.cc:45: if (existing_label_begin !=
string16::npos) {
On 2013/08/30 14:17:19, Alexei Svitkine wrote:
> I think it would be safer to also check that either existing_label_begin == 0
or
> experiment_labels[existing_label_begin - 1] == ','.
> 
> So you don't end up finding this in the middle of some other labels.

This is mitigated by searching for (label + "="); or is even the '=' not
considered a special character?

> 
> Alternatively, just do what variations_util.cc does and use
> SplitStringUsingSubstr() and iterate over each label and re-add them if they
> don't match |label|. This way, even if there multiple existing duplicates,
> they'll all be removed.

I thought about that, but it's too complex for this purpose imo and existing
duplicates for |label| are not possible as this has always previously clobbered.

https://codereview.chromium.org/23579003/diff/22001/chrome/installer/util/goo...
File chrome/installer/util/google_update_constants.cc (right):

https://codereview.chromium.org/23579003/diff/22001/chrome/installer/util/goo...
chrome/installer/util/google_update_constants.cc:12: const char16
kExperimentLabelSep[] = L";";
On 2013/08/30 14:17:19, Alexei Svitkine wrote:
> Unfortunately, you can't use the L prefix for a string16 literal - since L
> corresponds to wchar_t which is 32 bit on some platforms.
> 
> Perhaps you can change this to an actual single char16 instead of a string.
> Then, you can just use ',' for the value. (Because for ascii chars, its the
same
> value between a char and a char16).

That doesn't work either as on Windows char16 can't be set to a non "L" value...

How about using wchar_t, but using WideToUTF16 where this matters (i.e. where
AsciiToUTF16 was previously used anyways..)?

https://codereview.chromium.org/23579003/diff/22001/chrome/installer/util/goo...
File chrome/installer/util/google_update_constants.h (right):

https://codereview.chromium.org/23579003/diff/22001/chrome/installer/util/goo...
chrome/installer/util/google_update_constants.h:10: #include "base/string16.h"
On 2013/08/30 14:17:19, Alexei Svitkine wrote:
> I think this is "base/strings/string16.h" now.

Right, my bad, done.

[Alexei Svitkine (slow), 2013-08-30 18:16:10]

Thanks. Were you planning to write a test for this?

https://codereview.chromium.org/23579003/diff/22001/chrome/installer/gcapi/gc...
File chrome/installer/gcapi/gcapi_omaha_experiment.cc (right):

https://codereview.chromium.org/23579003/diff/22001/chrome/installer/gcapi/gc...
chrome/installer/gcapi/gcapi_omaha_experiment.cc:45: if (existing_label_begin !=
string16::npos) {
On 2013/08/30 17:53:11, gab wrote:
> On 2013/08/30 14:17:19, Alexei Svitkine wrote:
> > I think it would be safer to also check that either existing_label_begin ==
0
> or
> > experiment_labels[existing_label_begin - 1] == ','.
> > 
> > So you don't end up finding this in the middle of some other labels.
> 
> This is mitigated by searching for (label + "="); or is even the '=' not
> considered a special character?

I guess it depends how they're parsed. If each entry gets split on the first
'=', then presumably the value can still have a "=" in it. That's why I was
suggesting the safer approach. (But perhaps like you say it may not be an issue
in practice.)

I'll leave it up to you.

> 
> > 
> > Alternatively, just do what variations_util.cc does and use
> > SplitStringUsingSubstr() and iterate over each label and re-add them if they
> > don't match |label|. This way, even if there multiple existing duplicates,
> > they'll all be removed.
> 
> I thought about that, but it's too complex for this purpose imo and existing
> duplicates for |label| are not possible as this has always previously
clobbered.

https://codereview.chromium.org/23579003/diff/22001/chrome/installer/util/goo...
File chrome/installer/util/google_update_constants.cc (right):

https://codereview.chromium.org/23579003/diff/22001/chrome/installer/util/goo...
chrome/installer/util/google_update_constants.cc:12: const char16
kExperimentLabelSep[] = L";";
On 2013/08/30 17:53:11, gab wrote:
> On 2013/08/30 14:17:19, Alexei Svitkine wrote:
> > Unfortunately, you can't use the L prefix for a string16 literal - since L
> > corresponds to wchar_t which is 32 bit on some platforms.
> > 
> > Perhaps you can change this to an actual single char16 instead of a string.
> > Then, you can just use ',' for the value. (Because for ascii chars, its the
> same
> > value between a char and a char16).
> 
> That doesn't work either as on Windows char16 can't be set to a non "L"
value...
> 
> How about using wchar_t, but using WideToUTF16 where this matters (i.e. where
> AsciiToUTF16 was previously used anyways..)?

I guess a cast of the non-L value to char16 would have worked too, but I'm okay
with this solution as well, mostly because those functions are only used on
Windows where WideToUTF16 is a no-op. (On other platforms all those functions
will be dead-stripped from the binary since they're not used.)

[Alexei Svitkine (slow), 2013-08-30 18:20:02]

Looks like you're getting link errors because on non-Windows platforms,
installer code isn't included in the build, which is now needed for the
separator symbol.

Maybe just revert the changes to variations_util.cc and put a TODO there to use
the constant from gcapi. Or alternatively we can ifdef OS_WIN out all those
functions having to do with experiment labels.

[gab, 2013-08-30 22:51:58]

Added promised tests.

Also extracted Windows specific code from variations_util.cc into
variations_util_win.cc; this allows that code to happily use Windows-specific
code...

Cheers!
Gab

[gab, 2013-08-30 22:57:14]

https://codereview.chromium.org/23579003/diff/22001/chrome/installer/gcapi/gc...
File chrome/installer/gcapi/gcapi_omaha_experiment.cc (right):

https://codereview.chromium.org/23579003/diff/22001/chrome/installer/gcapi/gc...
chrome/installer/gcapi/gcapi_omaha_experiment.cc:45: if (existing_label_begin !=
string16::npos) {
On 2013/08/30 18:16:11, Alexei Svitkine wrote:
> On 2013/08/30 17:53:11, gab wrote:
> > On 2013/08/30 14:17:19, Alexei Svitkine wrote:
> > > I think it would be safer to also check that either existing_label_begin
==
> 0
> > or
> > > experiment_labels[existing_label_begin - 1] == ','.
> > > 
> > > So you don't end up finding this in the middle of some other labels.
> > 
> > This is mitigated by searching for (label + "="); or is even the '=' not
> > considered a special character?
> 
> I guess it depends how they're parsed. If each entry gets split on the first
> '=', then presumably the value can still have a "=" in it. That's why I was
> suggesting the safer approach. (But perhaps like you say it may not be an
issue
> in practice.)
> 
> I'll leave it up to you.
> 
> > 
> > > 
> > > Alternatively, just do what variations_util.cc does and use
> > > SplitStringUsingSubstr() and iterate over each label and re-add them if
they
> > > don't match |label|. This way, even if there multiple existing duplicates,
> > > they'll all be removed.
> > 
> > I thought about that, but it's too complex for this purpose imo and existing
> > duplicates for |label| are not possible as this has always previously
> clobbered.
> 

Added extra safety for this.

https://codereview.chromium.org/23579003/diff/22001/chrome/installer/util/goo...
File chrome/installer/util/google_update_constants.cc (right):

https://codereview.chromium.org/23579003/diff/22001/chrome/installer/util/goo...
chrome/installer/util/google_update_constants.cc:12: const char16
kExperimentLabelSep[] = L";";
On 2013/08/30 18:16:11, Alexei Svitkine wrote:
> On 2013/08/30 17:53:11, gab wrote:
> > On 2013/08/30 14:17:19, Alexei Svitkine wrote:
> > > Unfortunately, you can't use the L prefix for a string16 literal - since L
> > > corresponds to wchar_t which is 32 bit on some platforms.
> > > 
> > > Perhaps you can change this to an actual single char16 instead of a
string.
> > > Then, you can just use ',' for the value. (Because for ascii chars, its
the
> > same
> > > value between a char and a char16).
> > 
> > That doesn't work either as on Windows char16 can't be set to a non "L"
> value...
> > 
> > How about using wchar_t, but using WideToUTF16 where this matters (i.e.
where
> > AsciiToUTF16 was previously used anyways..)?
> 
> I guess a cast of the non-L value to char16 would have worked too, but I'm
okay
> with this solution as well, mostly because those functions are only used on
> Windows where WideToUTF16 is a no-op. (On other platforms all those functions
> will be dead-stripped from the binary since they're not used.)

Pulled those functions out to a Windows specific file.

[Alexei Svitkine (slow), 2013-09-03 15:46:35]

https://codereview.chromium.org/23579003/diff/22001/chrome/installer/util/goo...
File chrome/installer/util/google_update_constants.cc (right):

https://codereview.chromium.org/23579003/diff/22001/chrome/installer/util/goo...
chrome/installer/util/google_update_constants.cc:12: const char16
kExperimentLabelSep[] = L";";
On 2013/08/30 22:57:14, gab wrote:
> Pulled those functions out to a Windows specific file.

I prefer the Windows specific file actually be renamed to
experiment_labels_win.cc with its own unittest file
(experiment_labels_unittest_win.cc).

https://codereview.chromium.org/23579003/diff/59001/chrome/common/metrics/var...
File chrome/common/metrics/variations/variations_util_unittest.cc (right):

https://codereview.chromium.org/23579003/diff/59001/chrome/common/metrics/var...
chrome/common/metrics/variations/variations_util_unittest.cc:12: #include
"chrome/common/metrics/variations/variations_util.h"
This should go to the top of the file. There were both conventions going on in
the Chromium codebase for a while, but the trend is now towards putting this at
the top.

Here's a discussion about it that made me change my mind (I previously used to
do it this way too):

https://codereview.chromium.org/12310109/#msg14

https://codereview.chromium.org/23579003/diff/59001/chrome/installer/gcapi/gc...
File chrome/installer/gcapi/gcapi_omaha_experiment.cc (right):

https://codereview.chromium.org/23579003/diff/59001/chrome/installer/gcapi/gc...
chrome/installer/gcapi/gcapi_omaha_experiment.cc:44: // First erase this label
from the existing experiment labels if it exists.
I think this logic is now sufficiently complicated that I'd rather see the
simpler code that uses SplitStringUsingSubstr() and iterates through the entries
appending them to form a new string.

Yes, that could would be less efficient, but it would be much more readable and
this is hardly performance critical and the input size is small anyway, so it
would still be very fast.

https://codereview.chromium.org/23579003/diff/59001/chrome/installer/gcapi/gc...
chrome/installer/gcapi/gcapi_omaha_experiment.cc:84: // GCAPI instance; this
makes tests realiable when crossing time boundaries on
Nit: reliable

https://codereview.chromium.org/23579003/diff/59001/chrome/installer/gcapi/gc...
chrome/installer/gcapi/gcapi_omaha_experiment.cc:106: const GCAPILabelState&
gcapi_label_state = g_gcapi_label_state.Get();
Instead of having this GCAPILabelState state in lazy instance, how about
changing the the functions GetCurrentRlzWeek() and
installer::BuildExperimentDateString() to take a base::Time parameter. That way,
you'll ensure the same Time is used for both the week and the time string
(removing the possibility of them being inconsistent if the time changes between
those two calls). 

If you do the above, you can then simplify things a lot here by no longer
needing a lazy instance of the GCAPILabelState structure - instead you can
simply cache the time value instead and call both functions with that param.

Then, you can make the GetGCAPIExperimentLabel() function do this:

static time_t time_value = 0;
if (time_value == 0)
  time_value = base::Time::Now().ToTimeT();

base::Time time = base::Time::FromTimeT(time_value);
...
GetCurrentRlzWeek(time);
installer::BuildExperimentDateString(time);

https://codereview.chromium.org/23579003/diff/59001/chrome/installer/gcapi/gc...
File chrome/installer/gcapi/gcapi_omaha_experiment.h (right):

https://codereview.chromium.org/23579003/diff/59001/chrome/installer/gcapi/gc...
chrome/installer/gcapi/gcapi_omaha_experiment.h:15: // Returns the experiment
label to be used by |label| for |brand_code|.
This comment is confusing. The experiment label to be used by label? What is
label?

https://codereview.chromium.org/23579003/diff/59001/chrome/installer/gcapi/gc...
File chrome/installer/gcapi/gcapi_registry_overriding_test.h (right):

https://codereview.chromium.org/23579003/diff/59001/chrome/installer/gcapi/gc...
chrome/installer/gcapi/gcapi_registry_overriding_test.h:13: class
GCAPIRegistryOverridingTest : public ::testing::Test {
Instead of exposing a test base class for this, consider just wrapping the
functionality into its own non-test class - e.g. GCAPIRegistryOverrideManager
that holds the registry_util::RegistryOverrideManager instance and having the
Setup() stuff be done in its ctor.

Then, any test class that needs this can just add an instance of
GCAPIRegistryOverrideManager as a member variable. With that approach, if
another test (that has its own test baseclass) also wants to override the GCAPI
registry stuff, it can still do it (vs. this design where it wouldn't be able to
since it already has its own base class).

https://codereview.chromium.org/23579003/diff/59001/chrome/installer/gcapi/gc...
chrome/installer/gcapi/gcapi_registry_overriding_test.h:15: void SetUp()
OVERRIDE;
Declare ctor and destructor explicitly and define them in the cc file.

https://codereview.chromium.org/23579003/diff/59001/chrome/installer/gcapi/gc...
chrome/installer/gcapi/gcapi_registry_overriding_test.h:19: };
DISALLOW_COPY_AND_ASSIGN?

[Alexei Svitkine (slow), 2013-10-08 13:05:57]

Gab, just wondering what's the status here?

[gab, 2013-10-08 15:14:12]

The status is:

 - I was filling in for macourteau@ while he was on vacation and he will pick
this up when he has a free cycle.

 - This used to be urgent, but no longer is, I'll ping you offline with details.

Cheers,
Gab

[Alexei Svitkine (slow), 2013-11-01 15:39:41]

On 2013/10/08 15:14:12, gab wrote:
> The status is:
> 
>  - I was filling in for macourteau@ while he was on vacation and he will pick
> this up when he has a free cycle.
> 
>  - This used to be urgent, but no longer is, I'll ping you offline with
details.
> 
> Cheers,
> Gab

We really need to land this, rather than letting it bitrot in codereview.

Otherwise, if someone builds the target from source and uses it, it will have
the same issue - even though they may believe the source should have this fixed
already (due to using Gab's compiled version).

(And since it's just a target in the Chrome tree, anyone could build it...)

[gab, 2013-11-05 22:20:37]

Hello Robert/Alexei, refactoring completed as requested; PTAL.

Thanks!
Gab

https://codereview.chromium.org/23579003/diff/22001/chrome/installer/util/goo...
File chrome/installer/util/google_update_constants.cc (right):

https://codereview.chromium.org/23579003/diff/22001/chrome/installer/util/goo...
chrome/installer/util/google_update_constants.cc:12: const char16
kExperimentLabelSep[] = L";";
On 2013/09/03 15:46:35, Alexei Svitkine wrote:
> On 2013/08/30 22:57:14, gab wrote:
> > Pulled those functions out to a Windows specific file.
> 
> I prefer the Windows specific file actually be renamed to
> experiment_labels_win.cc with its own unittest file
> (experiment_labels_unittest_win.cc).

Done.

https://codereview.chromium.org/23579003/diff/59001/chrome/common/metrics/var...
File chrome/common/metrics/variations/variations_util_unittest.cc (right):

https://codereview.chromium.org/23579003/diff/59001/chrome/common/metrics/var...
chrome/common/metrics/variations/variations_util_unittest.cc:12: #include
"chrome/common/metrics/variations/variations_util.h"
On 2013/09/03 15:46:35, Alexei Svitkine wrote:
> This should go to the top of the file. There were both conventions going on in
> the Chromium codebase for a while, but the trend is now towards putting this
at
> the top.
> 
> Here's a discussion about it that made me change my mind (I previously used to
> do it this way too):
> 
> https://codereview.chromium.org/12310109/#msg14

Done.

https://codereview.chromium.org/23579003/diff/59001/chrome/installer/gcapi/gc...
File chrome/installer/gcapi/gcapi_omaha_experiment.cc (right):

https://codereview.chromium.org/23579003/diff/59001/chrome/installer/gcapi/gc...
chrome/installer/gcapi/gcapi_omaha_experiment.cc:44: // First erase this label
from the existing experiment labels if it exists.
On 2013/09/03 15:46:35, Alexei Svitkine wrote:
> I think this logic is now sufficiently complicated that I'd rather see the
> simpler code that uses SplitStringUsingSubstr() and iterates through the
entries
> appending them to form a new string.
> 
> Yes, that could would be less efficient, but it would be much more readable
and
> this is hardly performance critical and the input size is small anyway, so it
> would still be very fast.

Done.

https://codereview.chromium.org/23579003/diff/59001/chrome/installer/gcapi/gc...
chrome/installer/gcapi/gcapi_omaha_experiment.cc:84: // GCAPI instance; this
makes tests realiable when crossing time boundaries on
On 2013/09/03 15:46:35, Alexei Svitkine wrote:
> Nit: reliable

"real" "liable" is nice, no?! :)

https://codereview.chromium.org/23579003/diff/59001/chrome/installer/gcapi/gc...
chrome/installer/gcapi/gcapi_omaha_experiment.cc:106: const GCAPILabelState&
gcapi_label_state = g_gcapi_label_state.Get();
On 2013/09/03 15:46:35, Alexei Svitkine wrote:
> Instead of having this GCAPILabelState state in lazy instance, how about
> changing the the functions GetCurrentRlzWeek() and
> installer::BuildExperimentDateString() to take a base::Time parameter. That
way,
> you'll ensure the same Time is used for both the week and the time string
> (removing the possibility of them being inconsistent if the time changes
between
> those two calls). 
> 
> If you do the above, you can then simplify things a lot here by no longer
> needing a lazy instance of the GCAPILabelState structure - instead you can
> simply cache the time value instead and call both functions with that param.
> 
> Then, you can make the GetGCAPIExperimentLabel() function do this:
> 
> static time_t time_value = 0;
> if (time_value == 0)
>   time_value = base::Time::Now().ToTimeT();
> 
> base::Time time = base::Time::FromTimeT(time_value);
> ...
> GetCurrentRlzWeek(time);
> installer::BuildExperimentDateString(time);

Done.

https://codereview.chromium.org/23579003/diff/59001/chrome/installer/gcapi/gc...
File chrome/installer/gcapi/gcapi_omaha_experiment.h (right):

https://codereview.chromium.org/23579003/diff/59001/chrome/installer/gcapi/gc...
chrome/installer/gcapi/gcapi_omaha_experiment.h:15: // Returns the experiment
label to be used by |label| for |brand_code|.
On 2013/09/03 15:46:35, Alexei Svitkine wrote:
> This comment is confusing. The experiment label to be used by label? What is
> label?

Clarified.

https://codereview.chromium.org/23579003/diff/59001/chrome/installer/gcapi/gc...
File chrome/installer/gcapi/gcapi_registry_overriding_test.h (right):

https://codereview.chromium.org/23579003/diff/59001/chrome/installer/gcapi/gc...
chrome/installer/gcapi/gcapi_registry_overriding_test.h:13: class
GCAPIRegistryOverridingTest : public ::testing::Test {
On 2013/09/03 15:46:35, Alexei Svitkine wrote:
> Instead of exposing a test base class for this, consider just wrapping the
> functionality into its own non-test class - e.g. GCAPIRegistryOverrideManager
> that holds the registry_util::RegistryOverrideManager instance and having the
> Setup() stuff be done in its ctor.
> 
> Then, any test class that needs this can just add an instance of
> GCAPIRegistryOverrideManager as a member variable. With that approach, if
> another test (that has its own test baseclass) also wants to override the
GCAPI
> registry stuff, it can still do it (vs. this design where it wouldn't be able
to
> since it already has its own base class).

Done.

https://codereview.chromium.org/23579003/diff/59001/chrome/installer/gcapi/gc...
chrome/installer/gcapi/gcapi_registry_overriding_test.h:15: void SetUp()
OVERRIDE;
On 2013/09/03 15:46:35, Alexei Svitkine wrote:
> Declare ctor and destructor explicitly and define them in the cc file.

Done.

https://codereview.chromium.org/23579003/diff/59001/chrome/installer/gcapi/gc...
chrome/installer/gcapi/gcapi_registry_overriding_test.h:19: };
On 2013/09/03 15:46:35, Alexei Svitkine wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

[Alexei Svitkine (slow), 2013-11-05 23:05:23]

Thanks for picking this up again! Looks good generally with a few more comments.

https://codereview.chromium.org/23579003/diff/388001/chrome/chrome_tests_unit...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/23579003/diff/388001/chrome/chrome_tests_unit...
chrome/chrome_tests_unit.gypi:1846:
'common/metrics/variations/experiment_labels_win_unittest.cc',
Name this experiment_labels_unittest_win.cc, that way the default gypi suffix
filter will exclude it on other platforms automatically.

https://codereview.chromium.org/23579003/diff/388001/chrome/common/metrics/va...
File chrome/common/metrics/variations/experiment_labels_win.cc (right):

https://codereview.chromium.org/23579003/diff/388001/chrome/common/metrics/va...
chrome/common/metrics/variations/experiment_labels_win.cc:35: label +=
installer::BuildExperimentDateString(base::Time::Now());
Can you make CreateSingleExperimentLabel() take a time param? That way,
BuildGoogleUpdateExperimentLabel() can query Now() only once so that all the
experiments it processes will have the same timestamp.

https://codereview.chromium.org/23579003/diff/388001/chrome/installer/gcapi/g...
File chrome/installer/gcapi/gcapi_omaha_experiment.cc (right):

https://codereview.chromium.org/23579003/diff/388001/chrome/installer/gcapi/g...
chrome/installer/gcapi/gcapi_omaha_experiment.cc:18: using base::TimeDelta;
Either remove these using decls and prefix the uses below, or remove the base::
prefixes below (make it consistent).

https://codereview.chromium.org/23579003/diff/388001/chrome/installer/gcapi/g...
chrome/installer/gcapi/gcapi_omaha_experiment.cc:54: if (!it->empty() &&
!StartsWith(*it + L"=", label, true)) {
Shouldn't this be "StartsWith(*it, label + "L=", true)"?

e.g. *it would be something like "foo=bar" and label would be "foo", so you're
testing whether "foo=bar" starts with "foo=", no?

If so, please make sure you have sufficient test coverage for this (maybe you do
and you forgot to run them after making this change...). Also, if you change it,
it probably makes sense to move |label + L"="| to a local var outside the loop,
to avoid the computation every iteration.

https://codereview.chromium.org/23579003/diff/388001/chrome/installer/gcapi/g...
File chrome/installer/gcapi/gcapi_omaha_experiment_test.cc (right):

https://codereview.chromium.org/23579003/diff/388001/chrome/installer/gcapi/g...
chrome/installer/gcapi/gcapi_omaha_experiment_test.cc:6: #include
"chrome/installer/gcapi/gcapi_omaha_experiment.h"
Put this as the first include, followed by a blank line.

https://codereview.chromium.org/23579003/diff/388001/chrome/installer/gcapi/g...
File chrome/installer/gcapi/gcapi_reactivation_test.cc (right):

https://codereview.chromium.org/23579003/diff/388001/chrome/installer/gcapi/g...
chrome/installer/gcapi/gcapi_reactivation_test.cc:24: GCAPIReactivationTest() {}
Nit you can omit this, since this is entirely in the cc file.

https://codereview.chromium.org/23579003/diff/388001/chrome/installer/util/go...
File chrome/installer/util/google_update_experiment_util.h (right):

https://codereview.chromium.org/23579003/diff/388001/chrome/installer/util/go...
chrome/installer/util/google_update_experiment_util.h:10: namespace base{
Nit: space before {

[gab, 2013-11-06 16:39:09]

Thanks, PTAL.

Cheers,
Gab

https://codereview.chromium.org/23579003/diff/388001/chrome/chrome_tests_unit...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/23579003/diff/388001/chrome/chrome_tests_unit...
chrome/chrome_tests_unit.gypi:1846:
'common/metrics/variations/experiment_labels_win_unittest.cc',
On 2013/11/05 23:05:23, Alexei Svitkine wrote:
> Name this experiment_labels_unittest_win.cc, that way the default gypi suffix
> filter will exclude it on other platforms automatically.

Actually the rule works for this ordering too:
https://code.google.com/p/chromium/codesearch#chromium/src/build/filename_rul...

I tend to think of it as:
_win_unittest.cc is a unitest for a _win.cc file
where as _unittest_win.cc is a win-specific test for a regular .cc file.

https://codereview.chromium.org/23579003/diff/388001/chrome/common/metrics/va...
File chrome/common/metrics/variations/experiment_labels_win.cc (right):

https://codereview.chromium.org/23579003/diff/388001/chrome/common/metrics/va...
chrome/common/metrics/variations/experiment_labels_win.cc:35: label +=
installer::BuildExperimentDateString(base::Time::Now());
On 2013/11/05 23:05:23, Alexei Svitkine wrote:
> Can you make CreateSingleExperimentLabel() take a time param? That way,
> BuildGoogleUpdateExperimentLabel() can query Now() only once so that all the
> experiments it processes will have the same timestamp.

Done.

https://codereview.chromium.org/23579003/diff/388001/chrome/installer/gcapi/g...
File chrome/installer/gcapi/gcapi_omaha_experiment.cc (right):

https://codereview.chromium.org/23579003/diff/388001/chrome/installer/gcapi/g...
chrome/installer/gcapi/gcapi_omaha_experiment.cc:18: using base::TimeDelta;
On 2013/11/05 23:05:23, Alexei Svitkine wrote:
> Either remove these using decls and prefix the uses below, or remove the
base::
> prefixes below (make it consistent).

Done.

https://codereview.chromium.org/23579003/diff/388001/chrome/installer/gcapi/g...
chrome/installer/gcapi/gcapi_omaha_experiment.cc:54: if (!it->empty() &&
!StartsWith(*it + L"=", label, true)) {
On 2013/11/05 23:05:23, Alexei Svitkine wrote:
> Shouldn't this be "StartsWith(*it, label + "L=", true)"?
> 
> e.g. *it would be something like "foo=bar" and label would be "foo", so you're
> testing whether "foo=bar" starts with "foo=", no?
> 
> If so, please make sure you have sufficient test coverage for this (maybe you
do
> and you forgot to run them after making this change...). Also, if you change
it,
> it probably makes sense to move |label + L"="| to a local var outside the
loop,
> to avoid the computation every iteration.

Good catch! Added a new test which fails with this implementation and passes
with the new one.

Thanks!

https://codereview.chromium.org/23579003/diff/388001/chrome/installer/gcapi/g...
File chrome/installer/gcapi/gcapi_omaha_experiment_test.cc (right):

https://codereview.chromium.org/23579003/diff/388001/chrome/installer/gcapi/g...
chrome/installer/gcapi/gcapi_omaha_experiment_test.cc:6: #include
"chrome/installer/gcapi/gcapi_omaha_experiment.h"
On 2013/11/05 23:05:23, Alexei Svitkine wrote:
> Put this as the first include, followed by a blank line.

Done.

https://codereview.chromium.org/23579003/diff/388001/chrome/installer/gcapi/g...
File chrome/installer/gcapi/gcapi_reactivation_test.cc (right):

https://codereview.chromium.org/23579003/diff/388001/chrome/installer/gcapi/g...
chrome/installer/gcapi/gcapi_reactivation_test.cc:24: GCAPIReactivationTest() {}
On 2013/11/05 23:05:23, Alexei Svitkine wrote:
> Nit you can omit this, since this is entirely in the cc file.

I added this as not putting it results in this compile error:

FAILED: ninja -t msvc -e environment.x86 -- "c:\Program Files (x86)\Microsoft
Visual Studio 10.0\VC\bin\cl.exe" /nologo /showIncludes /FC
@obj\chrome\installer\gcapi\gcapi_test.gcapi_reactivation_test.obj.rsp /c
..\..\chrome\installer\gcapi\gcapi_reactivation_test.cc
/Foobj\chrome\installer\gcapi\gcapi_test.gcapi_reactivation_test.obj
/Fdobj\chrome\gcapi_test.pdb 
d:\src\debug\src\chrome\installer\gcapi\gcapi_reactivation_test.cc(94) : error
C2512: 'GCAPIReactivationTest' : no appropriate default constructor available
d:\src\debug\src\chrome\installer\gcapi\gcapi_reactivation_test.cc(101) : error
C2512: 'GCAPIReactivationTest' : no appropriate default constructor available
d:\src\debug\src\chrome\installer\gcapi\gcapi_reactivation_test.cc(154) : error
C2512: 'GCAPIReactivationTest' : no appropriate default constructor available
d:\src\debug\src\chrome\installer\gcapi\gcapi_reactivation_test.cc(190) : error
C2512: 'GCAPIReactivationTest' : no appropriate default constructor available

I don't really understand why... it's related to adding a new field which has to
be implicitly destroyed, but I'm not sure why adding a default constructor
helps... I did that because this one failed to compile where as
GCAPIOmahaExperimentTest compiled and the presence of a constructor was the only
obvious difference between the two...

https://codereview.chromium.org/23579003/diff/388001/chrome/installer/util/go...
File chrome/installer/util/google_update_experiment_util.h (right):

https://codereview.chromium.org/23579003/diff/388001/chrome/installer/util/go...
chrome/installer/util/google_update_experiment_util.h:10: namespace base{
On 2013/11/05 23:05:23, Alexei Svitkine wrote:
> Nit: space before {

Interesting that presubmit didn't catch that... Done.

[Alexei Svitkine (slow), 2013-11-06 22:45:04]

LGTM!

https://codereview.chromium.org/23579003/diff/388001/chrome/chrome_tests_unit...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/23579003/diff/388001/chrome/chrome_tests_unit...
chrome/chrome_tests_unit.gypi:1846:
'common/metrics/variations/experiment_labels_win_unittest.cc',
On 2013/11/06 16:39:10, gab wrote:
> On 2013/11/05 23:05:23, Alexei Svitkine wrote:
> > Name this experiment_labels_unittest_win.cc, that way the default gypi
suffix
> > filter will exclude it on other platforms automatically.
> 
> Actually the rule works for this ordering too:
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/filename_rul...
> 
> I tend to think of it as:
> _win_unittest.cc is a unitest for a _win.cc file
> where as _unittest_win.cc is a win-specific test for a regular .cc file.

You're right - it didn't always use to be the case, though.

I checked another place that has this logic - the presubmit file that chooses
which trybots to use for your CL and looks like that one also works with this
format now, so all good:

https://code.google.com/p/chromium/codesearch#chromium/src/PRESUBMIT.py&l=1038

https://codereview.chromium.org/23579003/diff/388001/chrome/installer/gcapi/g...
File chrome/installer/gcapi/gcapi_reactivation_test.cc (right):

https://codereview.chromium.org/23579003/diff/388001/chrome/installer/gcapi/g...
chrome/installer/gcapi/gcapi_reactivation_test.cc:24: GCAPIReactivationTest() {}
On 2013/11/06 16:39:10, gab wrote:
> On 2013/11/05 23:05:23, Alexei Svitkine wrote:
> > Nit you can omit this, since this is entirely in the cc file.
> 
> I added this as not putting it results in this compile error:
> 
> FAILED: ninja -t msvc -e environment.x86 -- "c:\Program Files (x86)\Microsoft
> Visual Studio 10.0\VC\bin\cl.exe" /nologo /showIncludes /FC
> @obj\chrome\installer\gcapi\gcapi_test.gcapi_reactivation_test.obj.rsp /c
> ..\..\chrome\installer\gcapi\gcapi_reactivation_test.cc
> /Foobj\chrome\installer\gcapi\gcapi_test.gcapi_reactivation_test.obj
> /Fdobj\chrome\gcapi_test.pdb 
> d:\src\debug\src\chrome\installer\gcapi\gcapi_reactivation_test.cc(94) : error
> C2512: 'GCAPIReactivationTest' : no appropriate default constructor available
> d:\src\debug\src\chrome\installer\gcapi\gcapi_reactivation_test.cc(101) :
error
> C2512: 'GCAPIReactivationTest' : no appropriate default constructor available
> d:\src\debug\src\chrome\installer\gcapi\gcapi_reactivation_test.cc(154) :
error
> C2512: 'GCAPIReactivationTest' : no appropriate default constructor available
> d:\src\debug\src\chrome\installer\gcapi\gcapi_reactivation_test.cc(190) :
error
> C2512: 'GCAPIReactivationTest' : no appropriate default constructor available
> 
> I don't really understand why... it's related to adding a new field which has
to
> be implicitly destroyed, but I'm not sure why adding a default constructor
> helps... I did that because this one failed to compile where as
> GCAPIOmahaExperimentTest compiled and the presence of a constructor was the
only
> obvious difference between the two...

Weird, okay.

[commit-bot: I haz the power, 2013-11-06 23:08:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/23579003/558001

[commit-bot: I haz the power, 2013-11-07 06:04:32]

Change committed as 233493

[rohitrao (ping after 24h), 2013-11-08 15:18:09]

Hi all,

I initially moved BuildGoogleUpdateExperimentLabel() into variations_util.h
because we needed to use it on iOS.  This CL moves it back into an _win specific
file and changes the code to use windows-only constructs, which doesn't work for
us =)

I've cc'ed you on the associated bug.

Thanks!
  Rohit

[gab, 2013-11-08 15:22:17]

On 2013/11/08 15:18:09, rohitrao wrote:
> Hi all,
> 
> I initially moved BuildGoogleUpdateExperimentLabel() into variations_util.h
> because we needed to use it on iOS.  This CL moves it back into an _win
specific
> file and changes the code to use windows-only constructs, which doesn't work
for
> us =)
> 
> I've cc'ed you on the associated bug.
> 
> Thanks!
>   Rohit

I have no objections with it being moved to a non-win only section; IIRC Alexei
requested that this be made Windows-only as it was used by no other platforms as
it is (and since this CL slept for a while this has been true for a couple
months at least...).

[Alexei Svitkine (slow), 2013-11-08 15:22:40]

On 2013/11/08 15:18:09, rohitrao wrote:
> Hi all,
> 
> I initially moved BuildGoogleUpdateExperimentLabel() into variations_util.h
> because we needed to use it on iOS.  This CL moves it back into an _win
specific
> file and changes the code to use windows-only constructs, which doesn't work
for
> us =)
> 
> I've cc'ed you on the associated bug.
> 
> Thanks!
>   Rohit

Ah, I didn't realise we were using it on iOS as well.

Sounds like we should rename the file to experiment_labels.cc (without the _win)
suffix and cut the dependency on google_update::kExperimentLabelSep (or have
that constant also live in a place that iOS can use).

Sorry for the breakage - too bad the relevant iOS code isn't upstreamed - else
the bots would have presumably told us of this breakage before it happened.

[stuartmorgan, 2013-11-08 15:39:15]

On 2013/11/08 15:22:40, Alexei Svitkine wrote:
> Sorry for the breakage - too bad the relevant iOS code isn't upstreamed - else
> the bots would have presumably told us of this breakage before it happened.

The decision made by Chrome leadership after extensive discussion was that we
should no longer upstream into chrome/, and instead move all shared code into
layered components. This is an *enormous* undertaking, so unsurprisingly it's
taking a long time. It is unfortunate in cases like this, yes, but we all have
to live with the reality that this kind of thing will happen, and need to be
unwound again, in the meantime.