Componentize VariationsSeedStore and its unittest

Componentize VariationsSeedStore and its unittest

This code is shared with iOS. It has no //chrome-level dependencies, so
componentization is trivial.

BUG=516662
TBR=agl

Committed: https://crrev.com/0b2305c0941b65a0777a5806d5b39626bf317ee8
Cr-Commit-Position: refs/heads/master@{#345340}

[blundell, 2015-08-05 12:33:57]

blundell@chromium.org changed reviewers:
+ asvitkine@chromium.org

[blundell, 2015-08-05 12:33:58]

https://codereview.chromium.org/1271123003/diff/1/components/metrics/DEPS
File components/metrics/DEPS (right):

https://codereview.chromium.org/1271123003/diff/1/components/metrics/DEPS#new...
components/metrics/DEPS:7: "+crypto",
Do you know if this dependency is problematic from the POV of ChromeOS?

[Alexei Svitkine (slow), 2015-08-05 15:22:45]

Do you mind waiting on this until I get my other CL landed?

[blundell, 2015-08-05 15:23:51]

On 2015/08/05 15:22:45, Alexei Svitkine wrote:
> Do you mind waiting on this until I get my other CL landed?

Not at all. Could you still do an initial review to resolve any high-level
issues about where the code should go considering the new dependency?

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

https://codereview.chromium.org/1271123003/diff/1/components/metrics/variatio...
File components/metrics/variations_seed_store.cc (right):

https://codereview.chromium.org/1271123003/diff/1/components/metrics/variatio...
components/metrics/variations_seed_store.cc:5: #include
"components/metrics/variations_seed_store.h"
Why metrics and not components/variations?

That is the eventual place we want this to be. So if it's possible to move it
there now, seems preferable to do that in the first place.

[blundell, 2015-08-05 19:54:28]

https://codereview.chromium.org/1271123003/diff/1/components/metrics/variatio...
File components/metrics/variations_seed_store.cc (right):

https://codereview.chromium.org/1271123003/diff/1/components/metrics/variatio...
components/metrics/variations_seed_store.cc:5: #include
"components/metrics/variations_seed_store.h"
On 2015/08/05 19:52:19, Alexei Svitkine wrote:
> Why metrics and not components/variations?
> 
> That is the eventual place we want this to be. So if it's possible to move it
> there now, seems preferable to do that in the first place.

Because it depends on //components/metrics (line 13 below).

[Alexei Svitkine (slow), 2015-08-05 20:01:17]

I see.

The Gzip code is something a bunch of places would actually like to use.

e.g. sync/ just copy-pasted that code since they can't depend on components/

https://codereview.chromium.org/1246523003/

I'm not sure what's the right thing for it is. There was talk on putting it into
base/ - which would actually help some other use case (we were thinking of using
it to compress command-line params passed to content/ subprocesses). But it's
slightly problematic because it would add a new dependency on zlib to base/ -
which I'm not sure base/ owners would agree on.

I feel like maybe the cleanest is to make it its own component. Since it will be
lightweight, sync/ might be ok depending on it. And it would solve this issue as
well.

Let me ask the sync folks on the other review whether that would be acceptable
to them.

[Alexei Svitkine (slow), 2015-08-06 14:52:28]

On 2015/08/05 20:01:17, Alexei Svitkine wrote:
> I see.
> 
> The Gzip code is something a bunch of places would actually like to use.
> 
> e.g. sync/ just copy-pasted that code since they can't depend on components/
> 
> https://codereview.chromium.org/1246523003/
> 
> I'm not sure what's the right thing for it is. There was talk on putting it
into
> base/ - which would actually help some other use case (we were thinking of
using
> it to compress command-line params passed to content/ subprocesses). But it's
> slightly problematic because it would add a new dependency on zlib to base/ -
> which I'm not sure base/ owners would agree on.
> 
> I feel like maybe the cleanest is to make it its own component. Since it will
be
> lightweight, sync/ might be ok depending on it. And it would solve this issue
as
> well.
> 
> Let me ask the sync folks on the other review whether that would be acceptable
> to them.

Per discussion on https://codereview.chromium.org/1246523003, it sounds like
having a separate component for the gzip code is the way to go.

So I'd prefer if we could move the gzip utils to this separate component - then
variations_seed_store can depend on it and not on components/metrics - so that
it can be moved directly to components/variations rather than temporarily moving
to components/metrics as this CL is doing. Would you mind doing the above? (The
other possibility is to ask the author of the other CL to make the component -
since that CL hasn't landed yet.)

[blundell, 2015-08-06 20:26:29]

On 2015/08/06 14:52:28, Alexei Svitkine wrote:
> On 2015/08/05 20:01:17, Alexei Svitkine wrote:
> > I see.
> > 
> > The Gzip code is something a bunch of places would actually like to use.
> > 
> > e.g. sync/ just copy-pasted that code since they can't depend on components/
> > 
> > https://codereview.chromium.org/1246523003/
> > 
> > I'm not sure what's the right thing for it is. There was talk on putting it
> into
> > base/ - which would actually help some other use case (we were thinking of
> using
> > it to compress command-line params passed to content/ subprocesses). But
it's
> > slightly problematic because it would add a new dependency on zlib to base/
-
> > which I'm not sure base/ owners would agree on.
> > 
> > I feel like maybe the cleanest is to make it its own component. Since it
will
> be
> > lightweight, sync/ might be ok depending on it. And it would solve this
issue
> as
> > well.
> > 
> > Let me ask the sync folks on the other review whether that would be
acceptable
> > to them.
> 
> Per discussion on https://codereview.chromium.org/1246523003, it sounds like
> having a separate component for the gzip code is the way to go.
> 
> So I'd prefer if we could move the gzip utils to this separate component -
then
> variations_seed_store can depend on it and not on components/metrics - so that
> it can be moved directly to components/variations rather than temporarily
moving
> to components/metrics as this CL is doing. Would you mind doing the above?
(The
> other possibility is to ask the author of the other CL to make the component -
> since that CL hasn't landed yet.)

I don't mind doing that -- I'm off next week though, so it wouldn't land before
the week after next if I do it.

[blundell, 2015-08-07 12:30:46]

On 2015/08/06 20:26:29, blundell wrote:
> On 2015/08/06 14:52:28, Alexei Svitkine wrote:
> > On 2015/08/05 20:01:17, Alexei Svitkine wrote:
> > > I see.
> > > 
> > > The Gzip code is something a bunch of places would actually like to use.
> > > 
> > > e.g. sync/ just copy-pasted that code since they can't depend on
components/
> > > 
> > > https://codereview.chromium.org/1246523003/
> > > 
> > > I'm not sure what's the right thing for it is. There was talk on putting
it
> > into
> > > base/ - which would actually help some other use case (we were thinking of
> > using
> > > it to compress command-line params passed to content/ subprocesses). But
> it's
> > > slightly problematic because it would add a new dependency on zlib to
base/
> -
> > > which I'm not sure base/ owners would agree on.
> > > 
> > > I feel like maybe the cleanest is to make it its own component. Since it
> will
> > be
> > > lightweight, sync/ might be ok depending on it. And it would solve this
> issue
> > as
> > > well.
> > > 
> > > Let me ask the sync folks on the other review whether that would be
> acceptable
> > > to them.
> > 
> > Per discussion on https://codereview.chromium.org/1246523003, it sounds like
> > having a separate component for the gzip code is the way to go.
> > 
> > So I'd prefer if we could move the gzip utils to this separate component -
> then
> > variations_seed_store can depend on it and not on components/metrics - so
that
> > it can be moved directly to components/variations rather than temporarily
> moving
> > to components/metrics as this CL is doing. Would you mind doing the above?
> (The
> > other possibility is to ask the author of the other CL to make the component
-
> > since that CL hasn't landed yet.)
> 
> I don't mind doing that -- I'm off next week though, so it wouldn't land
before
> the week after next if I do it.

Before I go ahead with that, could we resolve the question about whether it's
acceptable for //components/variations to depend on //crypto? Thanks!

[Alexei Svitkine (slow), 2015-08-07 14:21:21]

It should be OK.
On 7 Aug 2015 8:30 am, <blundell@chromium.org> wrote:

> On 2015/08/06 20:26:29, blundell wrote:
>
>> On 2015/08/06 14:52:28, Alexei Svitkine wrote:
>> > On 2015/08/05 20:01:17, Alexei Svitkine wrote:
>> > > I see.
>> > >
>> > > The Gzip code is something a bunch of places would actually like to
>> use.
>> > >
>> > > e.g. sync/ just copy-pasted that code since they can't depend on
>>
> components/
>
>> > >
>> > > https://codereview.chromium.org/1246523003/
>> > >
>> > > I'm not sure what's the right thing for it is. There was talk on
>> putting
>>
> it
>
>> > into
>> > > base/ - which would actually help some other use case (we were
>> thinking of
>> > using
>> > > it to compress command-line params passed to content/ subprocesses).
>> But
>> it's
>> > > slightly problematic because it would add a new dependency on zlib to
>>
> base/
>
>> -
>> > > which I'm not sure base/ owners would agree on.
>> > >
>> > > I feel like maybe the cleanest is to make it its own component. Since
>> it
>> will
>> > be
>> > > lightweight, sync/ might be ok depending on it. And it would solve
>> this
>> issue
>> > as
>> > > well.
>> > >
>> > > Let me ask the sync folks on the other review whether that would be
>> acceptable
>> > > to them.
>> >
>> > Per discussion on https://codereview.chromium.org/1246523003, it
>> sounds like
>> > having a separate component for the gzip code is the way to go.
>> >
>> > So I'd prefer if we could move the gzip utils to this separate
>> component -
>> then
>> > variations_seed_store can depend on it and not on components/metrics -
>> so
>>
> that
>
>> > it can be moved directly to components/variations rather than
>> temporarily
>> moving
>> > to components/metrics as this CL is doing. Would you mind doing the
>> above?
>> (The
>> > other possibility is to ask the author of the other CL to make the
>> component
>>
> -
>
>> > since that CL hasn't landed yet.)
>>
>
> I don't mind doing that -- I'm off next week though, so it wouldn't land
>>
> before
>
>> the week after next if I do it.
>>
>
> Before I go ahead with that, could we resolve the question about whether
> it's
> acceptable for //components/variations to depend on //crypto? Thanks!
>
> https://codereview.chromium.org/1271123003/
>

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

[blundell, 2015-08-07 15:27:45]

On 2015/08/07 14:21:21, Alexei Svitkine wrote:
> It should be OK.

Thanks. I'll get to this the week after next week.

> On 7 Aug 2015 8:30 am, <mailto:blundell@chromium.org> wrote:
> 
> > On 2015/08/06 20:26:29, blundell wrote:
> >
> >> On 2015/08/06 14:52:28, Alexei Svitkine wrote:
> >> > On 2015/08/05 20:01:17, Alexei Svitkine wrote:
> >> > > I see.
> >> > >
> >> > > The Gzip code is something a bunch of places would actually like to
> >> use.
> >> > >
> >> > > e.g. sync/ just copy-pasted that code since they can't depend on
> >>
> > components/
> >
> >> > >
> >> > > https://codereview.chromium.org/1246523003/
> >> > >
> >> > > I'm not sure what's the right thing for it is. There was talk on
> >> putting
> >>
> > it
> >
> >> > into
> >> > > base/ - which would actually help some other use case (we were
> >> thinking of
> >> > using
> >> > > it to compress command-line params passed to content/ subprocesses).
> >> But
> >> it's
> >> > > slightly problematic because it would add a new dependency on zlib to
> >>
> > base/
> >
> >> -
> >> > > which I'm not sure base/ owners would agree on.
> >> > >
> >> > > I feel like maybe the cleanest is to make it its own component. Since
> >> it
> >> will
> >> > be
> >> > > lightweight, sync/ might be ok depending on it. And it would solve
> >> this
> >> issue
> >> > as
> >> > > well.
> >> > >
> >> > > Let me ask the sync folks on the other review whether that would be
> >> acceptable
> >> > > to them.
> >> >
> >> > Per discussion on https://codereview.chromium.org/1246523003, it
> >> sounds like
> >> > having a separate component for the gzip code is the way to go.
> >> >
> >> > So I'd prefer if we could move the gzip utils to this separate
> >> component -
> >> then
> >> > variations_seed_store can depend on it and not on components/metrics -
> >> so
> >>
> > that
> >
> >> > it can be moved directly to components/variations rather than
> >> temporarily
> >> moving
> >> > to components/metrics as this CL is doing. Would you mind doing the
> >> above?
> >> (The
> >> > other possibility is to ask the author of the other CL to make the
> >> component
> >>
> > -
> >
> >> > since that CL hasn't landed yet.)
> >>
> >
> > I don't mind doing that -- I'm off next week though, so it wouldn't land
> >>
> > before
> >
> >> the week after next if I do it.
> >>
> >
> > Before I go ahead with that, could we resolve the question about whether
> > it's
> > acceptable for //components/variations to depend on //crypto? Thanks!
> >
> > https://codereview.chromium.org/1271123003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Alexei Svitkine (slow), 2015-08-07 15:31:56]

Great - thanks for working on this!

On Fri, Aug 7, 2015 at 11:27 AM, <blundell@chromium.org> wrote:

> On 2015/08/07 14:21:21, Alexei Svitkine wrote:
>
>> It should be OK.
>>
>
> Thanks. I'll get to this the week after next week.
>
>
> On 7 Aug 2015 8:30 am, <mailto:blundell@chromium.org> wrote:
>>
>
> > On 2015/08/06 20:26:29, blundell wrote:
>> >
>> >> On 2015/08/06 14:52:28, Alexei Svitkine wrote:
>> >> > On 2015/08/05 20:01:17, Alexei Svitkine wrote:
>> >> > > I see.
>> >> > >
>> >> > > The Gzip code is something a bunch of places would actually like to
>> >> use.
>> >> > >
>> >> > > e.g. sync/ just copy-pasted that code since they can't depend on
>> >>
>> > components/
>> >
>> >> > >
>> >> > > https://codereview.chromium.org/1246523003/
>> >> > >
>> >> > > I'm not sure what's the right thing for it is. There was talk on
>> >> putting
>> >>
>> > it
>> >
>> >> > into
>> >> > > base/ - which would actually help some other use case (we were
>> >> thinking of
>> >> > using
>> >> > > it to compress command-line params passed to content/
>> subprocesses).
>> >> But
>> >> it's
>> >> > > slightly problematic because it would add a new dependency on zlib
>> to
>> >>
>> > base/
>> >
>> >> -
>> >> > > which I'm not sure base/ owners would agree on.
>> >> > >
>> >> > > I feel like maybe the cleanest is to make it its own component.
>> Since
>> >> it
>> >> will
>> >> > be
>> >> > > lightweight, sync/ might be ok depending on it. And it would solve
>> >> this
>> >> issue
>> >> > as
>> >> > > well.
>> >> > >
>> >> > > Let me ask the sync folks on the other review whether that would be
>> >> acceptable
>> >> > > to them.
>> >> >
>> >> > Per discussion on https://codereview.chromium.org/1246523003, it
>> >> sounds like
>> >> > having a separate component for the gzip code is the way to go.
>> >> >
>> >> > So I'd prefer if we could move the gzip utils to this separate
>> >> component -
>> >> then
>> >> > variations_seed_store can depend on it and not on components/metrics
>> -
>> >> so
>> >>
>> > that
>> >
>> >> > it can be moved directly to components/variations rather than
>> >> temporarily
>> >> moving
>> >> > to components/metrics as this CL is doing. Would you mind doing the
>> >> above?
>> >> (The
>> >> > other possibility is to ask the author of the other CL to make the
>> >> component
>> >>
>> > -
>> >
>> >> > since that CL hasn't landed yet.)
>> >>
>> >
>> > I don't mind doing that -- I'm off next week though, so it wouldn't land
>> >>
>> > before
>> >
>> >> the week after next if I do it.
>> >>
>> >
>> > Before I go ahead with that, could we resolve the question about whether
>> > it's
>> > acceptable for //components/variations to depend on //crypto? Thanks!
>> >
>> > https://codereview.chromium.org/1271123003/
>> >
>>
>
> To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://codereview.chromium.org/1271123003/
>

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

[blundell, 2015-08-20 08:30:48]

This is ready for re-review now.

[Alexei Svitkine (slow), 2015-08-24 15:52:25]

lgtm

[blundell, 2015-08-25 15:16:14]

blundell@chromium.org changed reviewers:
+ agl@chromium.org

[blundell, 2015-08-25 15:16:42]

TBR=agl for new dependencies from //components/variations to //crypto and
//third_party/protobuf, as this is just a shuffling of the pre-existing
dependency.

[blundell, 2015-08-25 15:16:50]

The CQ bit was checked by blundell@chromium.org

[blundell, 2015-08-25 15:16:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1271123003/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2015-08-25 15:17:23]

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

[commit-bot: I haz the power, 2015-08-25 15:54:49]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-08-25 15:55:51]

Patchset 6 (id:??) landed as
https://crrev.com/0b2305c0941b65a0777a5806d5b39626bf317ee8
Cr-Commit-Position: refs/heads/master@{#345340}