Create variations target that depends on base.

Create a variations component, so that it can be re-used.

Moving this code out of chrome/ paves the way for server-controlled system
field trials on ChromeOS. This allows building a new ChromeOS daemon binary
that depends on the new variations target, without needing to depend on all of
chrome/.

This CL moves several files from chrome/common/metrics and
chrome/browser/metrics into a separate component/variations target and
updates all the include paths referencing these. It does not do additional
cleanup, such as changing namespaces - this is left for a follow-up cleanup
CL.

BUG=266007
TEST=Existing unit tests.
R=joi@chromium.org, jwd@chromium.org, sky@chromium.org

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

[Alexei Svitkine (slow), 2013-08-23 14:41:40]

Hey Darin,

Could you take a look? This is adding a top-level variations/ directory, so I
need your approval. See the CL description for my reasoning on why I'm adding it
here. Let me know if it makes sense to you.

I will get someone else to review the low-level details of the CL re: the files
actually being moved, just want to get an agreement on the approach before I get
someone to look at it in detail.

Thanks!

[brettw, 2013-08-23 21:22:46]

"variations" doesn't mean much to me just looking at the toplevel. Do you think
there are other metrics stuff we'd eventually want to move out? We could
potentially have metrics/variations and move other metrics stuff here.

If there's not other metrics stuff we're thinking about moving, I still think we
should consider metrics/variations or just metrics/

[darin (slow to review), 2013-08-23 22:17:15]

"metrics" seems like a nice top level name.
On Aug 23, 2013 2:22 PM, <brettw@chromium.org> wrote:

> "variations" doesn't mean much to me just looking at the toplevel. Do you
> think
> there are other metrics stuff we'd eventually want to move out? We could
> potentially have metrics/variations and move other metrics stuff here.
>
> If there's not other metrics stuff we're thinking about moving, I still
> think we
> should consider metrics/variations or just metrics/
>
>
https://codereview.chromium.**org/23097007/<https://codereview.chromium.org/2...
>

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

[darin (slow to review), 2013-08-24 05:27:56]

On 2013/08/23 22:17:15, darin wrote:
> "metrics" seems like a nice top level name.
> On Aug 23, 2013 2:22 PM, <mailto:brettw@chromium.org> wrote:
> 
> > "variations" doesn't mean much to me just looking at the toplevel. Do you
> > think
> > there are other metrics stuff we'd eventually want to move out? We could
> > potentially have metrics/variations and move other metrics stuff here.
> >
> > If there's not other metrics stuff we're thinking about moving, I still
> > think we
> > should consider metrics/variations or just metrics/
> >
> >
>
https://codereview.chromium.**org/23097007/%3Chttps://codereview.chromium.org...>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

From jam@ who is on vacation:

"I think we don't need to use such a strong definition of what goes into
components, otherwise there'll be an increase in top level directories like
this. How I view components is that it's like a "shopping-cart" of extra
features that content embedders can add to it. A component doesn't have to
depend on content, i.e. like components that are shared between desktop Chrome
and iOS Chrome. And also, a component, like this one, doesn't have to be in the
same binary where content is. i.e. ChromeOS does depend on content, even if the
daemon referenced doesn't.

So I think this should go under src/components."

He said he also wasn't aware of the aforementioned discussion with content
owners.

-Darin

[Alexei Svitkine (slow), 2013-08-24 14:42:07]

On 2013/08/24 05:27:56, darin wrote:
> On 2013/08/23 22:17:15, darin wrote:
> > "metrics" seems like a nice top level name.
> > On Aug 23, 2013 2:22 PM, <mailto:brettw@chromium.org> wrote:
> > 
> > > "variations" doesn't mean much to me just looking at the toplevel. Do you
> > > think
> > > there are other metrics stuff we'd eventually want to move out? We could
> > > potentially have metrics/variations and move other metrics stuff here.
> > >
> > > If there's not other metrics stuff we're thinking about moving, I still
> > > think we
> > > should consider metrics/variations or just metrics/
> > >
> > >
> >
>
https://codereview.chromium.**org/23097007/%253Chttps://codereview.chromium.o...>
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> From jam@ who is on vacation:
> 
> "I think we don't need to use such a strong definition of what goes into
> components, otherwise there'll be an increase in top level directories like
> this. How I view components is that it's like a "shopping-cart" of extra
> features that content embedders can add to it. A component doesn't have to
> depend on content, i.e. like components that are shared between desktop Chrome
> and iOS Chrome. And also, a component, like this one, doesn't have to be in
the
> same binary where content is. i.e. ChromeOS does depend on content, even if
the
> daemon referenced doesn't.
> 
> So I think this should go under src/components."

That works for me, so I'll go with components/variations (since I don't see
other
metrics stuff needing to move any time soon), assuming everyone's ok with that.

> 
> He said he also wasn't aware of the aforementioned discussion with content
> owners.

My previous conversation about it was with @joi.

> 
> -Darin

[Jói, 2013-08-25 16:04:18]

Here's what I said in the earlier conversation; I think this echoes
what John said more or less:

> If it doesn't depend on src/content, then it doesn't necessarily
> belong in src/components.  However, there is precedent for putting
> stuff that is "useful for embedders of content" in src/components as
> long as src/content or layers below it do not need to depend on it;
> i.e. stuff that is only used above the content layer and might be used
> by an embedder of content is OK for src/components.  I guess you could
> say this falls in that category?

Cheers,
Jói


On Sat, Aug 24, 2013 at 2:42 PM,  <asvitkine@chromium.org> wrote:
> On 2013/08/24 05:27:56, darin wrote:
>>
>> On 2013/08/23 22:17:15, darin wrote:
>> > "metrics" seems like a nice top level name.
>> > On Aug 23, 2013 2:22 PM, <mailto:brettw@chromium.org> wrote:
>> >
>> > > "variations" doesn't mean much to me just looking at the toplevel. Do
>> > > you
>> > > think
>> > > there are other metrics stuff we'd eventually want to move out? We
>> > > could
>> > > potentially have metrics/variations and move other metrics stuff here.
>> > >
>> > > If there's not other metrics stuff we're thinking about moving, I
>> > > still
>> > > think we
>> > > should consider metrics/variations or just metrics/
>> > >
>> > >
>> >
>
>
>
https://codereview.chromium.**org/23097007/%253Chttps://codereview.chromium.o...>
>
>> > >
>> >
>> > To unsubscribe from this group and stop receiving emails from it, send
>> > an
>> email
>> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>>  From jam@ who is on vacation:
>
>
>> "I think we don't need to use such a strong definition of what goes into
>> components, otherwise there'll be an increase in top level directories
>> like
>> this. How I view components is that it's like a "shopping-cart" of extra
>> features that content embedders can add to it. A component doesn't have to
>> depend on content, i.e. like components that are shared between desktop
>> Chrome
>> and iOS Chrome. And also, a component, like this one, doesn't have to be
>> in
>
> the
>>
>> same binary where content is. i.e. ChromeOS does depend on content, even
>> if
>
> the
>>
>> daemon referenced doesn't.
>
>
>> So I think this should go under src/components."
>
>
> That works for me, so I'll go with components/variations (since I don't see
> other
> metrics stuff needing to move any time soon), assuming everyone's ok with
> that.
>
>
>
>> He said he also wasn't aware of the aforementioned discussion with content
>> owners.
>
>
> My previous conversation about it was with @joi.
>
>
>> -Darin
>
>
>
>
> https://codereview.chromium.org/23097007/

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

[jam, 2013-08-25 16:18:37]

cool, sounds like we're on the same page.

btw I just remembered that we also have src/base/metrics. I had forgotten
about that before. It seems that since this is metrics-related code, and it
doesn't depend on anything other than base, it belongs with that code? i.e.
src/base/metrics/variations?


On Sun, Aug 25, 2013 at 9:03 AM, Jói Sigurðsson <joi@chromium.org> wrote:

> Here's what I said in the earlier conversation; I think this echoes
> what John said more or less:
>
> > If it doesn't depend on src/content, then it doesn't necessarily
> > belong in src/components.  However, there is precedent for putting
> > stuff that is "useful for embedders of content" in src/components as
> > long as src/content or layers below it do not need to depend on it;
> > i.e. stuff that is only used above the content layer and might be used
> > by an embedder of content is OK for src/components.  I guess you could
> > say this falls in that category?
>
> Cheers,
> Jói
>
>
> On Sat, Aug 24, 2013 at 2:42 PM,  <asvitkine@chromium.org> wrote:
> > On 2013/08/24 05:27:56, darin wrote:
> >>
> >> On 2013/08/23 22:17:15, darin wrote:
> >> > "metrics" seems like a nice top level name.
> >> > On Aug 23, 2013 2:22 PM, <mailto:brettw@chromium.org> wrote:
> >> >
> >> > > "variations" doesn't mean much to me just looking at the toplevel.
> Do
> >> > > you
> >> > > think
> >> > > there are other metrics stuff we'd eventually want to move out? We
> >> > > could
> >> > > potentially have metrics/variations and move other metrics stuff
> here.
> >> > >
> >> > > If there's not other metrics stuff we're thinking about moving, I
> >> > > still
> >> > > think we
> >> > > should consider metrics/variations or just metrics/
> >> > >
> >> > >
> >> >
> >
> >
> > https://codereview.chromium.**org/23097007/%253Chttps://
> codereview.chromium.org/23097007/>
> >
> >> > >
> >> >
> >> > To unsubscribe from this group and stop receiving emails from it, send
> >> > an
> >> email
> >> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> >
> >>  From jam@ who is on vacation:
> >
> >
> >> "I think we don't need to use such a strong definition of what goes into
> >> components, otherwise there'll be an increase in top level directories
> >> like
> >> this. How I view components is that it's like a "shopping-cart" of extra
> >> features that content embedders can add to it. A component doesn't have
> to
> >> depend on content, i.e. like components that are shared between desktop
> >> Chrome
> >> and iOS Chrome. And also, a component, like this one, doesn't have to be
> >> in
> >
> > the
> >>
> >> same binary where content is. i.e. ChromeOS does depend on content, even
> >> if
> >
> > the
> >>
> >> daemon referenced doesn't.
> >
> >
> >> So I think this should go under src/components."
> >
> >
> > That works for me, so I'll go with components/variations (since I don't
> see
> > other
> > metrics stuff needing to move any time soon), assuming everyone's ok with
> > that.
> >
> >
> >
> >> He said he also wasn't aware of the aforementioned discussion with
> content
> >> owners.
> >
> >
> > My previous conversation about it was with @joi.
> >
> >
> >> -Darin
> >
> >
> >
> >
> > https://codereview.chromium.org/23097007/
>

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

[Alexei Svitkine (slow), 2013-08-26 00:12:43]

On 2013/08/25 16:18:37, jam wrote:
> btw I just remembered that we also have src/base/metrics. I had forgotten
> about that before. It seems that since this is metrics-related code, and it
> doesn't depend on anything other than base, it belongs with that code? i.e.
> src/base/metrics/variations?

I considered this and don't think this belongs in base. While it's true that
this only depends on base, I don't think the functionality in this library is
generally-useful enough to live under base/.

[jam, 2013-08-26 14:14:32]

On 2013/08/26 00:12:43, Alexei Svitkine wrote:
> On 2013/08/25 16:18:37, jam wrote:
> > btw I just remembered that we also have src/base/metrics. I had forgotten
> > about that before. It seems that since this is metrics-related code, and it
> > doesn't depend on anything other than base, it belongs with that code? i.e.
> > src/base/metrics/variations?
> 
> I considered this and don't think this belongs in base. While it's true that
> this only depends on base, I don't think the functionality in this library is
> generally-useful enough to live under base/.

How do you define generally-useful? It seems that this is metrics-related code
that will be used by more than one top level directory. While we could put it
under src\components, then we will have three metrics related directories:
base\metrics, components\variations, and chrome\browser\metrics.

base owners, wdyt?

[Alexei Svitkine (slow), 2013-08-26 14:22:58]

On Mon, Aug 26, 2013 at 10:14 AM, <jam@chromium.org> wrote:

> On 2013/08/26 00:12:43, Alexei Svitkine wrote:
>
>> On 2013/08/25 16:18:37, jam wrote:
>> > btw I just remembered that we also have src/base/metrics. I had
>> forgotten
>> > about that before. It seems that since this is metrics-related code,
>> and it
>> > doesn't depend on anything other than base, it belongs with that code?
>> i.e.
>> > src/base/metrics/variations?
>>
>
>  I considered this and don't think this belongs in base. While it's true
>> that
>> this only depends on base, I don't think the functionality in this
>> library is
>> generally-useful enough to live under base/.
>>
>
> How do you define generally-useful? It seems that this is metrics-related
> code
> that will be used by more than one top level directory. While we could put
> it
> under src\components, then we will have three metrics related directories:
> base\metrics, components\variations, and chrome\browser\metrics.
>
> base owners, wdyt?
>

Let me give an example that may shed some light on why I don't think this
should be in base.

This new target contains a protobuf that's used by the variations server to
provide the list of experiments to Chrome. This is a custom format that's
pretty specific to Chrome (though it will be consumed by the new ChromeOS
daemon process via this new library too). Still, just because it will be
used by two things (Chrome and the new ChromeOS daemon) doesn't mean it's a
generally-useful utility to have in base - it's not like base::Time or
string utils or standard crypto algos or other such things that many
different subsystems could re-use - it's something very specific to Chrome
that two things happen to need to use.


>
>
https://codereview.chromium.**org/23097007/<https://codereview.chromium.org/2...
>

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

[Alexei Svitkine (slow), 2013-08-26 17:50:22]

Brett, Darin: What do you think about whether it should go in
base/metrics/variations or components/variations?

(See my previous message for why I'm of the opinion it shouldn't go in base.)

Thanks!

[darin (slow to review), 2013-08-26 18:11:12]

I think the protobuf dependency suggests that this does not belong in base.
Let's go the src/components route.

Thanks,
-Darin


On Mon, Aug 26, 2013 at 10:50 AM, <asvitkine@chromium.org> wrote:

> Brett, Darin: What do you think about whether it should go in
> base/metrics/variations or components/variations?
>
> (See my previous message for why I'm of the opinion it shouldn't go in
> base.)
>
> Thanks!
>
>
https://codereview.chromium.**org/23097007/<https://codereview.chromium.org/2...
>

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

[Alexei Svitkine (slow), 2013-08-26 18:16:01]

All right, I'll go ahead and update this CL to move the new target go to
src/components/variations, assuming everyone is okay with that.

Thanks for everyone's guidance here.


On Mon, Aug 26, 2013 at 2:11 PM, Darin Fisher <darin@chromium.org> wrote:

> I think the protobuf dependency suggests that this does not belong in
> base. Let's go the src/components route.
>
> Thanks,
> -Darin
>
>
> On Mon, Aug 26, 2013 at 10:50 AM, <asvitkine@chromium.org> wrote:
>
>> Brett, Darin: What do you think about whether it should go in
>> base/metrics/variations or components/variations?
>>
>> (See my previous message for why I'm of the opinion it shouldn't go in
>> base.)
>>
>> Thanks!
>>
>>
https://codereview.chromium.**org/23097007/<https://codereview.chromium.org/2...
>>
>
>

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

[Alexei Svitkine (slow), 2013-08-27 16:42:59]

+jwd

I updated the CL to make this a component.

Jesse, can you review the move of the variations code before I ask an owner to
take a look at the actual CL?

[jwd, 2013-08-28 20:21:47]

lgtm

[Alexei Svitkine (slow), 2013-08-28 20:28:17]

Joi, could you review the move to components/? Thanks!

[Jói, 2013-08-29 09:37:33]

Looks like you're using SVN and did not use "svn move" to move the sources, so
they all look like they're new.  Could you use "svn move" so that we keep
history?  This also makes the code review easier.

You can preserve your existing changes to the files, just copy them somewhere,
then do the svn move, then copy them over the moved files.

Cheers,
Jói

https://codereview.chromium.org/23097007/diff/242001/components/variations/OW...
File components/variations/OWNERS (right):

https://codereview.chromium.org/23097007/diff/242001/components/variations/OW...
components/variations/OWNERS:1: asvitkine@chromium.org
You can also add to components/OWNERS so that this same list can edit
components/variations.gypi

[Alexei Svitkine (slow), 2013-08-29 15:36:57]

> Could you use "svn move" so that we keep history?

Done.

https://codereview.chromium.org/23097007/diff/242001/components/variations/OW...
File components/variations/OWNERS (right):

https://codereview.chromium.org/23097007/diff/242001/components/variations/OW...
components/variations/OWNERS:1: asvitkine@chromium.org
On 2013/08/29 09:37:33, Jói wrote:
> You can also add to components/OWNERS so that this same list can edit
> components/variations.gypi

Done.

[Jói, 2013-08-29 15:41:52]

//components LGTM with a nit/question.

https://codereview.chromium.org/23097007/diff/220001/components/variations/en...
File components/variations/entropy_provider.h (right):

https://codereview.chromium.org/23097007/diff/220001/components/variations/en...
components/variations/entropy_provider.h:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
Not sure we are supposed to update these any more.

[Alexei Svitkine (slow), 2013-08-29 15:56:52]

+sky for chrome/ OWNERS approval

https://codereview.chromium.org/23097007/diff/220001/components/variations/en...
File components/variations/entropy_provider.h (right):

https://codereview.chromium.org/23097007/diff/220001/components/variations/en...
components/variations/entropy_provider.h:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
On 2013/08/29 15:41:52, Jói wrote:
> Not sure we are supposed to update these any more.

You're probably right - I was treating this case as new files being added, but
since it's a move, they probably should keep their old dates.

Though, I also know the cq fixes them up anyway on commit for new files, so I
guess the question is whether it will treat this case as a new file and fix them
up anyway or not.

Anyway, I've reverted them back and we'll see what the cq does.

[sky, 2013-08-29 17:56:10]

LGTM

[commit-bot: I haz the power, 2013-08-29 17:59:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/23097007/215001

[commit-bot: I haz the power, 2013-08-29 17:59:31]

Failed to apply patch for components/variations/entropy_provider.cc:
While running patch -p0 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 5
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: components/variations/entropy_provider.cc
  |===================================================================
  |--- components/variations/entropy_provider.cc	(working copy)
  |+++ components/variations/entropy_provider.cc	(working copy)
  --------------------------
  No file to patch.  Skipping patch.
  2 out of 2 hunks ignored

Patch:   N   components/variations/entropy_provider.cc
Index: components/variations/entropy_provider.cc
===================================================================
--- components/variations/entropy_provider.cc	(working copy)
+++ components/variations/entropy_provider.cc	(working copy)
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/common/metrics/entropy_provider.h"
+#include "components/variations/entropy_provider.h"
 
 #include <algorithm>
 #include <limits>
@@ -12,7 +12,7 @@
 #include "base/rand_util.h"
 #include "base/sha1.h"
 #include "base/sys_byteorder.h"
-#include "chrome/common/metrics/metrics_util.h"
+#include "components/variations/metrics_util.h"
 
 namespace metrics {

[Alexei Svitkine (slow), 2013-08-29 18:02:09]

will commit manually, since the cq doesn't seem to like the svn mv'd diffs

[Alexei Svitkine (slow), 2013-08-29 18:03:27]

Committed patchset #4 manually as r220339 (presubmit successful).