Componentize kExperimentLabelSeparator & BuildExperimentDateString.

Componentize kExperimentLabelSeparator & BuildExperimentDateString.

Move kExperimentLabelSeparator & BuildExperimentDateString into
the variations components so that chrome/common/variations can be
componentized in a followup CL (in order to be shared with iOS).

BUG=520070
Committed: https://crrev.com/1eeb90f911d88e13f29545b6d3c49270061a2984
Cr-Commit-Position: refs/heads/master@{#343246}

[sdefresne, 2015-08-12 16:45:58]

Patchset #2 (id:20001) has been deleted

[sdefresne, 2015-08-12 16:46:02]

Patchset #1 (id:1) has been deleted

[sdefresne, 2015-08-12 16:54:57]

sdefresne@chromium.org changed reviewers:
+ robertshield@chromium.org, stevet@chromium.org

[sdefresne, 2015-08-12 17:22:39]

robertshield, can you take a look at
  - chrome/installer/*
  - chrome/chrome_installer*

stevet, can you take a look at
  - chrome/common/variations/*
  - components/variations/*

[robertshield, 2015-08-12 18:05:55]

installer changes LGTM

[sdefresne, 2015-08-12 18:47:57]

Do you have an idea of what could cause the compilation error on windows?
I've not moved were the missing symbol is defined but somehow it is no
longer found.

Le mer. 12 août 2015 à 20:05, <robertshield@chromium.org> a écrit :

> installer changes LGTM
>
> https://codereview.chromium.org/1291763002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[robertshield, 2015-08-12 19:37:38]

On 2015/08/12 18:47:57, sdefresne wrote:
> Do you have an idea of what could cause the compilation error on windows?
> I've not moved were the missing symbol is defined but somehow it is no
> longer found.
> 

Weird. Maybe check the .ninja files to make absolutely sure the .obj the
constant is defined in is being included in the link line for the failing
targets?

> Le mer. 12 août 2015 à 20:05, <mailto:robertshield@chromium.org> a écrit :
> 
> > installer changes LGTM
> >
> > https://codereview.chromium.org/1291763002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sdefresne, 2015-08-13 07:57:13]

sdefresne@chromium.org changed reviewers:
+ asvitkine@chromium.org
- stevet@chromium.org

[sdefresne, 2015-08-13 07:57:13]

-stevet (OOO), +asvitkine: can you review
 - chrome/common/variations
 - components/variations

[Alexei Svitkine (slow), 2015-08-13 15:18:29]

https://codereview.chromium.org/1291763002/diff/60002/chrome/installer/util/g...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/1291763002/diff/60002/chrome/installer/util/g...
chrome/installer/util/google_update_settings.cc:30: #include
"components/variations/variations_experiment_util.h"
Do you need to update any usage of those declaration in this file too, since the
namespace is changing? Or is this actually an unused include?

https://codereview.chromium.org/1291763002/diff/60002/chrome/installer/util/g...
File chrome/installer/util/google_update_settings_unittest.cc (right):

https://codereview.chromium.org/1291763002/diff/60002/chrome/installer/util/g...
chrome/installer/util/google_update_settings_unittest.cc:26: #include
"components/variations/variations_experiment_util.h"
Ditto.

https://codereview.chromium.org/1291763002/diff/60002/components/variations/v...
File components/variations/variations_experiment_util.h (right):

https://codereview.chromium.org/1291763002/diff/60002/components/variations/v...
components/variations/variations_experiment_util.h:19: extern const wchar_t
kExperimentLabels[];
Hmm, this doesn't seem to be used anywhere in variations code.

Perhaps move this to google_update_settings.cc/h?

[sdefresne, 2015-08-13 16:46:49]

https://codereview.chromium.org/1291763002/diff/60002/chrome/installer/util/g...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/1291763002/diff/60002/chrome/installer/util/g...
chrome/installer/util/google_update_settings.cc:30: #include
"components/variations/variations_experiment_util.h"
On 2015/08/13 at 15:18:29, Alexei Svitkine wrote:
> Do you need to update any usage of those declaration in this file too, since
the namespace is changing? Or is this actually an unused include?

Done.

https://codereview.chromium.org/1291763002/diff/60002/chrome/installer/util/g...
File chrome/installer/util/google_update_settings_unittest.cc (right):

https://codereview.chromium.org/1291763002/diff/60002/chrome/installer/util/g...
chrome/installer/util/google_update_settings_unittest.cc:26: #include
"components/variations/variations_experiment_util.h"
On 2015/08/13 at 15:18:29, Alexei Svitkine wrote:
> Ditto.

Done.

https://codereview.chromium.org/1291763002/diff/60002/components/variations/v...
File components/variations/variations_experiment_util.h (right):

https://codereview.chromium.org/1291763002/diff/60002/components/variations/v...
components/variations/variations_experiment_util.h:19: extern const wchar_t
kExperimentLabels[];
On 2015/08/13 at 15:18:29, Alexei Svitkine wrote:
> Hmm, this doesn't seem to be used anywhere in variations code.
> 
> Perhaps move this to google_update_settings.cc/h?

Ah sorry, you reviewed one of my test patchset to try to understand compilation
failure on Windows. I think I do understand why it is failing now, and this is
because google_update_experiment_util.h now only contains a single symbol which
is not directly used in the dynamic library that export it, and then later the
code linking against the library does not find the symbol.

I agree that this should not be here. Moved.

[Alexei Svitkine (slow), 2015-08-13 16:52:05]

lgtm

https://codereview.chromium.org/1291763002/diff/130001/chrome/installer/util/...
File chrome/installer/util/google_update_constants.h (right):

https://codereview.chromium.org/1291763002/diff/130001/chrome/installer/util/...
chrome/installer/util/google_update_constants.h:82: #if defined(OS_WIN)
Can you check if this file is already Windows-specific? If so, no need for this
ifdef.

Ditto for the .cc.

[sdefresne, 2015-08-13 16:54:55]

On 2015/08/13 at 16:46:49, sdefresne wrote:
>
https://codereview.chromium.org/1291763002/diff/60002/chrome/installer/util/g...
> File chrome/installer/util/google_update_settings.cc (right):
> 
>
https://codereview.chromium.org/1291763002/diff/60002/chrome/installer/util/g...
> chrome/installer/util/google_update_settings.cc:30: #include
"components/variations/variations_experiment_util.h"
> On 2015/08/13 at 15:18:29, Alexei Svitkine wrote:
> > Do you need to update any usage of those declaration in this file too, since
the namespace is changing? Or is this actually an unused include?
> 
> Done.
> 
>
https://codereview.chromium.org/1291763002/diff/60002/chrome/installer/util/g...
> File chrome/installer/util/google_update_settings_unittest.cc (right):
> 
>
https://codereview.chromium.org/1291763002/diff/60002/chrome/installer/util/g...
> chrome/installer/util/google_update_settings_unittest.cc:26: #include
"components/variations/variations_experiment_util.h"
> On 2015/08/13 at 15:18:29, Alexei Svitkine wrote:
> > Ditto.
> 
> Done.
> 
>
https://codereview.chromium.org/1291763002/diff/60002/components/variations/v...
> File components/variations/variations_experiment_util.h (right):
> 
>
https://codereview.chromium.org/1291763002/diff/60002/components/variations/v...
> components/variations/variations_experiment_util.h:19: extern const wchar_t
kExperimentLabels[];
> On 2015/08/13 at 15:18:29, Alexei Svitkine wrote:
> > Hmm, this doesn't seem to be used anywhere in variations code.
> > 
> > Perhaps move this to google_update_settings.cc/h?
> 
> Ah sorry, you reviewed one of my test patchset to try to understand
compilation failure on Windows. I think I do understand why it is failing now,
and this is because google_update_experiment_util.h now only contains a single
symbol which is not directly used in the dynamic library that export it, and
then later the code linking against the library does not find the symbol.
> 
> I agree that this should not be here. Moved.

OK. I don't understand why this does not build on Windows.

google_update::kExperimentLabels is only used in three files:
- chrome/installer/gcapi/gcapi_reactivation_test.cc
- chrome/installer/util/google_update_settings.cc
- chrome/installer/util/google_update_settings_unittest.cc

All of them use other constants from google_update_constants.h
- gcapi_reactivation_test.cc uses google_update::kChromeUpgradeCode
- google_update_settings.cc uses google_update::kRegAggregateMethod
- google_update_settings_unittest.cc uses google_update::kRegPathClientState

So I would expect that when linking against those file, to have the same state
of failure/success for both google_update::kExperimentLabels and the
corresponding constant, however only the google_update::kExperimentLabels is
reported as missing at link time:

installer_util_nacl_win64.lib(installer_util_nacl_win64.google_update_settings.obj)
: error LNK2019: unresolved external symbol "wchar_t const * const
google_update::kExperimentLabels" (?kExperimentLabels@google_update@@3QB_WB)
referenced in function "public: static bool __cdecl
GoogleUpdateSettings::ReadExperimentLabels(bool,class
std::basic_string<wchar_t,struct std::char_traits<wchar_t>,class
std::allocator<wchar_t> > *)"
(?ReadExperimentLabels@GoogleUpdateSettings@@SA_N_NPEAV?$basic_string@_WU?$char_traits@_W@std@@V?$allocator@_W@2@@std@@@Z)

As I do not have access to a machine Windows, this is really hard for me to
debug.

It looks like the build did succeed on Windows when I moved kExperimentLabels
into //components/variation (see patchset #7). asvitkine, would you allow me to
move the constant there (even if it is used) to workaround this mysterious
linker error?

[Alexei Svitkine (slow), 2015-08-13 16:58:50]

We should just figure out the cause of the error and fix it. I can help take a
look later today.

Can you start by trying to remove the OS_WIN check per my comment above - just
in case there might be something with that (since that differs from the other
constants)?

[Alexei Svitkine (slow), 2015-08-13 17:01:26]

The other thing I suggest checking is that the targets that fail to link
actually end up depending on the .cc file where the constant is defined - not
just the variations component.

[sdefresne, 2015-08-13 17:04:32]

On 2015/08/13 at 17:01:26, asvitkine wrote:
> The other thing I suggest checking is that the targets that fail to link
actually end up depending on the .cc file where the constant is defined - not
just the variations component.

Done. And I think it may fix the issue. I guess that google_constants.cc never
#include-d build/build_config.h and thus OS_WIN was not defined and the constant
discarded. Stupid me, I should have though of that (instead of assuming that
OS_WIN would be defined on the command-line via -DOS_WIN).

[sdefresne, 2015-08-13 17:06:33]

On 2015/08/13 at 17:01:26, asvitkine wrote:
> The other thing I suggest checking is that the targets that fail to link
actually end up depending on the .cc file where the constant is defined - not
just the variations component.

google_update_constants.cc and google_update_settings.cc are in the same target,
so all target depending on one depends on the other.

[Alexei Svitkine (slow), 2015-08-13 17:27:08]

lgtm

[sdefresne, 2015-08-13 18:46:27]

On 2015/08/13 at 17:27:08, asvitkine wrote:
> lgtm

thank you for the review and the help

[sdefresne, 2015-08-13 18:46:36]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2015-08-13 18:46:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertshield@chromium.org
Link to the patchset: https://codereview.chromium.org/1291763002/#ps150001
(title: "Remove "#if defined(OS_WIN)" from google_update_constants.cc")

[commit-bot: I haz the power, 2015-08-13 18:47:11]

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

[commit-bot: I haz the power, 2015-08-13 18:52:15]

Committed patchset #7 (id:150001)

[commit-bot: I haz the power, 2015-08-13 18:52:49]

Patchset 7 (id:??) landed as
https://crrev.com/1eeb90f911d88e13f29545b6d3c49270061a2984
Cr-Commit-Position: refs/heads/master@{#343246}