[Metrics] Move CachingPermutedEntropyProvider into the variations component.

[Metrics] Move CachingPermutedEntropyProvider into the variations component.

BUG=374298
TEST=compiles
R=asvitkine@chromium.org, blundell@chromium.org

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

[blundell, 2014-05-19 11:04:32]

https://codereview.chromium.org/293623003/diff/1/components/variations/cachin...
File components/variations/caching_permuted_entropy_provider.h (right):

https://codereview.chromium.org/293623003/diff/1/components/variations/cachin...
components/variations/caching_permuted_entropy_provider.h:12: #include
"components/variations/proto/permuted_entropy_cache.pb.h"
I believe that the target generating the headers from the protos needs to be
distinct from the target in which this file is built so that the target in which
this file is built can depend on it and thus ensure that the headers are present
at build time.

https://codereview.chromium.org/293623003/diff/1/components/variations/cachin...
components/variations/caching_permuted_entropy_provider.h:17: namespace metrics
{
namespace variations? (in all the moved files)

https://codereview.chromium.org/293623003/diff/1/components/variations/pref_n...
File components/variations/pref_names.h (right):

https://codereview.chromium.org/293623003/diff/1/components/variations/pref_n...
components/variations/pref_names.h:8: namespace metrics {
namespace variations?

[Ilya Sherman, 2014-05-19 11:16:27]

https://codereview.chromium.org/293623003/diff/1/components/variations/cachin...
File components/variations/caching_permuted_entropy_provider.h (right):

https://codereview.chromium.org/293623003/diff/1/components/variations/cachin...
components/variations/caching_permuted_entropy_provider.h:12: #include
"components/variations/proto/permuted_entropy_cache.pb.h"
On 2014/05/19 11:04:32, blundell wrote:
> I believe that the target generating the headers from the protos needs to be
> distinct from the target in which this file is built so that the target in
which
> this file is built can depend on it and thus ensure that the headers are
present
> at build time.

Done.

https://codereview.chromium.org/293623003/diff/1/components/variations/cachin...
components/variations/caching_permuted_entropy_provider.h:17: namespace metrics
{
On 2014/05/19 11:04:32, blundell wrote:
> namespace variations? (in all the moved files)

Ack.  (See my reply on the other file.)

https://codereview.chromium.org/293623003/diff/1/components/variations/pref_n...
File components/variations/pref_names.h (right):

https://codereview.chromium.org/293623003/diff/1/components/variations/pref_n...
components/variations/pref_names.h:8: namespace metrics {
On 2014/05/19 11:04:32, blundell wrote:
> namespace variations?

Using "metrics" for consistency with the other files in the variations
component.  We should eventually switch over to "variations", but it probably
makes sense to do that all at once for the whole component.

[blundell, 2014-05-19 11:18:29]

LGTM

[Alexei Svitkine (slow), 2014-05-19 11:23:03]

https://codereview.chromium.org/293623003/diff/20001/components/variations.gypi
File components/variations.gypi (right):

https://codereview.chromium.org/293623003/diff/20001/components/variations.gy...
components/variations.gypi:40: 'target_name': 'variations_proto',
I think it's OK to keep them in the same target (makes the build graph smaller).

But if you really insist of having a separate target, we need to update the
targets that depend on this (I think chrome_browser.gypi and maybe also
chrome_common.gypi) to explicitly depend on this target, since dependencies are
only transitive for linking, not for including generated proto headers.

[blundell, 2014-05-19 11:25:25]

https://codereview.chromium.org/293623003/diff/20001/components/variations.gypi
File components/variations.gypi (right):

https://codereview.chromium.org/293623003/diff/20001/components/variations.gy...
components/variations.gypi:40: 'target_name': 'variations_proto',
I don't think they can be in the same target, as the order between the action
that generates the pb.h header and the compilation of the files including that
header wouldn't be defined AFAIK.

On 2014/05/19 11:23:03, Alexei Svitkine wrote:
> I think it's OK to keep them in the same target (makes the build graph
smaller).
> 
> But if you really insist of having a separate target, we need to update the
> targets that depend on this (I think chrome_browser.gypi and maybe also
> chrome_common.gypi) to explicitly depend on this target, since dependencies
are
> only transitive for linking, not for including generated proto headers.

[blundell, 2014-05-19 11:26:55]

https://codereview.chromium.org/293623003/diff/20001/components/variations.gypi
File components/variations.gypi (right):

https://codereview.chromium.org/293623003/diff/20001/components/variations.gy...
components/variations.gypi:40: 'target_name': 'variations_proto',
On 2014/05/19 11:23:03, Alexei Svitkine wrote:
> I think it's OK to keep them in the same target (makes the build graph
smaller).
> 
> But if you really insist of having a separate target, we need to update the
> targets that depend on this (I think chrome_browser.gypi and maybe also
> chrome_common.gypi) to explicitly depend on this target, since dependencies
are
> only transitive for linking, not for including generated proto headers.

Good point. We can do an export_dependent_settings in the variations target
instead, as that will export the hard_dependency to targets depending on
variations.

[blundell, 2014-05-19 11:37:04]

As discussed offline, my initial worry was incorrect and it's fine to have the
protos in the same target as .cc files including the generated proto headers
(cf. the comments in build/protoc.gypi). Apologies!

[Ilya Sherman, 2014-05-19 11:41:21]

Also tweaked a unittest to compile with the moved prefs constant's location.

https://codereview.chromium.org/293623003/diff/20001/components/variations.gypi
File components/variations.gypi (right):

https://codereview.chromium.org/293623003/diff/20001/components/variations.gy...
components/variations.gypi:40: 'target_name': 'variations_proto',
On 2014/05/19 11:26:56, blundell wrote:
> On 2014/05/19 11:23:03, Alexei Svitkine wrote:
> > I think it's OK to keep them in the same target (makes the build graph
> smaller).
> > 
> > But if you really insist of having a separate target, we need to update the
> > targets that depend on this (I think chrome_browser.gypi and maybe also
> > chrome_common.gypi) to explicitly depend on this target, since dependencies
> are
> > only transitive for linking, not for including generated proto headers.
> 
> Good point. We can do an export_dependent_settings in the variations target
> instead, as that will export the hard_dependency to targets depending on
> variations.

Restored the original .gyp structure.

[Alexei Svitkine (slow), 2014-05-19 11:45:47]

LGTM % nit

https://codereview.chromium.org/293623003/diff/40001/components/variations/pr...
File components/variations/pref_names.h (right):

https://codereview.chromium.org/293623003/diff/40001/components/variations/pr...
components/variations/pref_names.h:12: // Keep alphabetized, and document each
in the .cc file.
Nit: Add an empty line after this, so reader doesn't confuse this for the
comment of the actual pref on the next line.

[Ilya Sherman, 2014-05-19 11:47:20]

https://codereview.chromium.org/293623003/diff/40001/components/variations/pr...
File components/variations/pref_names.h (right):

https://codereview.chromium.org/293623003/diff/40001/components/variations/pr...
components/variations/pref_names.h:12: // Keep alphabetized, and document each
in the .cc file.
On 2014/05/19 11:45:47, Alexei Svitkine wrote:
> Nit: Add an empty line after this, so reader doesn't confuse this for the
> comment of the actual pref on the next line.

Done.

[Ilya Sherman, 2014-05-19 11:47:28]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-05-19 11:47:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/isherman@chromium.org/293623003/60001

[commit-bot: I haz the power, 2014-05-19 15:19:18]

Change committed as 271393