Restoring the chrome variatioons client, with a fix for browser_test failures caused by it. (issue 10392007)

j...@chromium.org

unread,

May 14, 2012, 4:59:57 PM5/14/12

to ste...@chromium.org, chromium...@chromium.org, m...@chromium.org, ishe...@chromium.org, j...@chromium.org

Reviewers: SteveT,

Description:
Restoring the chrome variatioons client, with a fix for browser_test
failures
caused by it.

BUG=121695
TEST=

Please review this at https://chromiumcodereview.appspot.com/10392007/

SVN Base: svn://svn.chromium.org/chrome/trunk/src

Affected files:
M chrome/browser/browser_process.h
M chrome/browser/browser_process_impl.h
M chrome/browser/browser_process_impl.cc
M chrome/browser/chrome_browser_main.cc
A chrome/browser/metrics/proto/study.proto
A chrome/browser/metrics/proto/trials_seed.proto
A chrome/browser/metrics/variations_service.h
A chrome/browser/metrics/variations_service.cc
A chrome/browser/metrics/variations_service_unittest.cc
M chrome/browser/prefs/browser_prefs.cc
M chrome/chrome_browser.gypi
M chrome/common/pref_names.h
M chrome/common/pref_names.cc
M chrome/test/base/testing_browser_process.h
M chrome/test/base/testing_browser_process.cc

ste...@chromium.org

unread,

May 14, 2012, 5:08:57 PM5/14/12

to j...@chromium.org, chromium...@chromium.org, m...@chromium.org, ishe...@chromium.org, j...@chromium.org

Can you describe the specific fixes and changes since your last patch? It's
hard
for me to tell what's new and what's been reviewed already.

In the description you can mention the CL that was originally committed,
plus
some notes on that fixes are new. The unit tests are all Alexei's, right?

https://chromiumcodereview.appspot.com/10392007/diff/11018/chrome/browser/chrome_browser_main.cc
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/10392007/diff/11018/chrome/browser/chrome_browser_main.cc#newcode1871
chrome/browser/chrome_browser_main.cc:1871: if (parameters().ui_task ==
NULL) {
Why do we break into two ifs here if we didn't change the body in a way
that requires condition separation? I think you should probably
recombine them with the &&...

https://chromiumcodereview.appspot.com/10392007/diff/11018/chrome/browser/metrics/variations_service.h
File chrome/browser/metrics/variations_service.h (right):

https://chromiumcodereview.appspot.com/10392007/diff/11018/chrome/browser/metrics/variations_service.h#newcode49
chrome/browser/metrics/variations_service.h:49:
Nit: Remove superfluous break.

https://chromiumcodereview.appspot.com/10392007/diff/11018/chrome/browser/metrics/variations_service_unittest.cc
File chrome/browser/metrics/variations_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/10392007/diff/11018/chrome/browser/metrics/variations_service_unittest.cc#newcode18
chrome/browser/metrics/variations_service_unittest.cc:18:
Nit: I think you can get rid of this extra break.

https://chromiumcodereview.appspot.com/10392007/

j...@chromium.org

unread,

May 15, 2012, 10:58:36 AM5/15/12

to ste...@chromium.org, chromium...@chromium.org, m...@chromium.org, ishe...@chromium.org, j...@chromium.org

I added a patch set with the original variations service code (before
revert)
and a patch with the current version again for comparison.

The change was to make VariationsService not a singleton and instead have it
owned by the browser process. The details of that are implemented in
browser_process_impl.

There is another change in how the seed fetch is initiated. The call to
StartFetchingVariationsSeed is made conditional and the call can silently
fail
if the network is probably unavailable.

https://chromiumcodereview.appspot.com/10392007/diff/11018/chrome/browser/chrome_browser_main.cc
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/10392007/diff/11018/chrome/browser/chrome_browser_main.cc#newcode1871
chrome/browser/chrome_browser_main.cc:1871: if (parameters().ui_task ==
NULL) {

On 2012/05/14 21:08:57, SteveT wrote:
> Why do we break into two ifs here if we didn't change the body in a
way that
> requires condition separation? I think you should probably recombine
them with
> the &&...

They are broken apart because the variations_service stuff shouldn't be
in the inner if. I put it in while testing and forgot to pull it back
out.

https://chromiumcodereview.appspot.com/10392007/diff/11018/chrome/browser/metrics/variations_service.h
File chrome/browser/metrics/variations_service.h (right):

https://chromiumcodereview.appspot.com/10392007/diff/11018/chrome/browser/metrics/variations_service.h#newcode49
chrome/browser/metrics/variations_service.h:49:

On 2012/05/14 21:08:57, SteveT wrote:
> Nit: Remove superfluous break.

Done.

https://chromiumcodereview.appspot.com/10392007/diff/11018/chrome/browser/metrics/variations_service_unittest.cc
File chrome/browser/metrics/variations_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/10392007/diff/11018/chrome/browser/metrics/variations_service_unittest.cc#newcode18
chrome/browser/metrics/variations_service_unittest.cc:18:

On 2012/05/14 21:08:57, SteveT wrote:
> Nit: I think you can get rid of this extra break.

Done.

https://chromiumcodereview.appspot.com/10392007/

ste...@chromium.org

unread,

May 15, 2012, 12:56:46 PM5/15/12

to j...@chromium.org, chromium...@chromium.org, m...@chromium.org, ishe...@chromium.org, j...@chromium.org

lgtm

https://chromiumcodereview.appspot.com/10392007/diff/5018/chrome/browser/metrics/variations_service.cc
File chrome/browser/metrics/variations_service.cc (right):

https://chromiumcodereview.appspot.com/10392007/diff/5018/chrome/browser/metrics/variations_service.cc#newcode74
chrome/browser/metrics/variations_service.cc:74:
pending_seed_request_->SetMaxRetries(5);
nit: Might as well do it now and stick it in the anon namespace above.

https://chromiumcodereview.appspot.com/10392007/

j...@chromium.org

unread,

May 15, 2012, 2:31:23 PM5/15/12

to ste...@chromium.org, chromium...@chromium.org, m...@chromium.org, ishe...@chromium.org, j...@chromium.org

https://chromiumcodereview.appspot.com/10392007/diff/5018/chrome/browser/metrics/variations_service.cc
File chrome/browser/metrics/variations_service.cc (right):

https://chromiumcodereview.appspot.com/10392007/diff/5018/chrome/browser/metrics/variations_service.cc#newcode74
chrome/browser/metrics/variations_service.cc:74:
pending_seed_request_->SetMaxRetries(5);

On 2012/05/15 16:56:47, SteveT wrote:
> nit: Might as well do it now and stick it in the anon namespace above.

Done.

https://chromiumcodereview.appspot.com/10392007/

ishe...@chromium.org

unread,

May 15, 2012, 6:11:17 PM5/15/12

to j...@chromium.org, ste...@chromium.org, chromium...@chromium.org, m...@chromium.org, j...@chromium.org

https://chromiumcodereview.appspot.com/10392007/diff/6019/chrome/browser/browser_process.h
File chrome/browser/browser_process.h (right):

https://chromiumcodereview.appspot.com/10392007/diff/6019/chrome/browser/browser_process.h#newcode96
chrome/browser/browser_process.h:96: virtual VariationsService*
variations_service() = 0;
I still think this might be more appropriate as a member of the
MetricsService class. I'm not sure if there's someone who'd qualify as
an "owner" of the BrowserProcess code, who could comment on this

https://chromiumcodereview.appspot.com/10392007/diff/6019/chrome/browser/browser_process_impl.h
File chrome/browser/browser_process_impl.h (right):

https://chromiumcodereview.appspot.com/10392007/diff/6019/chrome/browser/browser_process_impl.h#newcode185
chrome/browser/browser_process_impl.h:185: bool
created_variations_service_;
nit: Why do we need this boolean? Can't this be inferred based on
whether |variations_service_| is null?

https://chromiumcodereview.appspot.com/10392007/diff/6019/chrome/browser/chrome_browser_main.cc
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/10392007/diff/6019/chrome/browser/chrome_browser_main.cc#newcode1877
chrome/browser/chrome_browser_main.cc:1877:
browser_process_->variations_service()->StartFetchingVariationsSeed();
nit: I think the nested if-stmts would be less surprising if this line
preceded the |translate_manager_| logic.

https://chromiumcodereview.appspot.com/10392007/

commi...@chromium.org

unread,

May 16, 2012, 8:55:44 AM5/16/12

to j...@chromium.org, ste...@chromium.org, chromium...@chromium.org, m...@chromium.org, ishe...@chromium.org, j...@chromium.org

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/j...@chromium.org/10392007/6019

https://chromiumcodereview.appspot.com/10392007/

ste...@chromium.org

unread,

May 16, 2012, 10:33:49 AM5/16/12

to j...@chromium.org, chromium...@chromium.org, m...@chromium.org, ishe...@chromium.org, j...@chromium.org

comment inline for Ilya

https://chromiumcodereview.appspot.com/10392007/diff/6019/chrome/browser/browser_process.h
File chrome/browser/browser_process.h (right):

https://chromiumcodereview.appspot.com/10392007/diff/6019/chrome/browser/browser_process.h#newcode96
chrome/browser/browser_process.h:96: virtual VariationsService*
variations_service() = 0;

Does the functionality in VariationsService have much to do with the
functionality in MetricsService? In fact, I don't think MetricsService
actually uses the VS at all.

I agree with you that we should avoid hanging more stuff off the browser
process if possible, but is it worth introducing a new member to the
class just for the class to hold on to it, without actually utilizing
it?

It seems like the VS has more to do with the FieldTrialList than
MetricsService, but I'm also reluctant to put it in there :)

Any thoughts on these questions?

https://chromiumcodereview.appspot.com/10392007/

j...@chromium.org

unread,

May 16, 2012, 10:47:31 AM5/16/12

to ste...@chromium.org, chromium...@chromium.org, m...@chromium.org, ishe...@chromium.org, j...@chromium.org

https://chromiumcodereview.appspot.com/10392007/diff/6019/chrome/browser/browser_process.h
File chrome/browser/browser_process.h (right):

https://chromiumcodereview.appspot.com/10392007/diff/6019/chrome/browser/browser_process.h#newcode96
chrome/browser/browser_process.h:96: virtual VariationsService*
variations_service() = 0;

On 2012/05/15 22:11:17, Ilya Sherman wrote:
> I still think this might be more appropriate as a member of the
MetricsService
> class. I'm not sure if there's someone who'd qualify as an "owner" of
the
> BrowserProcess code, who could comment on this

FWIW putting it here was a recommendation from dominich@.

https://chromiumcodereview.appspot.com/10392007/diff/6019/chrome/browser/browser_process_impl.h
File chrome/browser/browser_process_impl.h (right):

https://chromiumcodereview.appspot.com/10392007/diff/6019/chrome/browser/browser_process_impl.h#newcode185
chrome/browser/browser_process_impl.h:185: bool
created_variations_service_;

On 2012/05/15 22:11:17, Ilya Sherman wrote:
> nit: Why do we need this boolean? Can't this be inferred based on
whether
> |variations_service_| is null?

Done.

https://chromiumcodereview.appspot.com/10392007/diff/6019/chrome/browser/chrome_browser_main.cc
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/10392007/diff/6019/chrome/browser/chrome_browser_main.cc#newcode1877
chrome/browser/chrome_browser_main.cc:1877:
browser_process_->variations_service()->StartFetchingVariationsSeed();

On 2012/05/15 22:11:17, Ilya Sherman wrote:
> nit: I think the nested if-stmts would be less surprising if this line
preceded
> the |translate_manager_| logic.

Good idea.
Done.

https://chromiumcodereview.appspot.com/10392007/

ishe...@chromium.org

unread,

May 16, 2012, 1:57:22 PM5/16/12

to j...@chromium.org, ste...@chromium.org, chromium...@chromium.org, m...@chromium.org, j...@chromium.org

(LGTM)

https://chromiumcodereview.appspot.com/10392007/diff/6019/chrome/browser/browser_process.h
File chrome/browser/browser_process.h (right):

https://chromiumcodereview.appspot.com/10392007/diff/6019/chrome/browser/browser_process.h#newcode96
chrome/browser/browser_process.h:96: virtual VariationsService*
variations_service() = 0;

On 2012/05/16 14:47:31, Jesse Doherty wrote:
> On 2012/05/15 22:11:17, Ilya Sherman wrote:
> > I still think this might be more appropriate as a member of the
MetricsService
> > class. I'm not sure if there's someone who'd qualify as an "owner"
of the
> > BrowserProcess code, who could comment on this

> FWIW putting it here was a recommendation from dominich@.

Fair enough, I'm ok with keeping it here if that seems better than
having it be a member of the MetricsService -- I don't have any better
suggestions on where it might live.

https://chromiumcodereview.appspot.com/10392007/diff/6019/chrome/browser/browser_process.h#newcode96
chrome/browser/browser_process.h:96: virtual VariationsService*
variations_service() = 0;

On 2012/05/16 14:33:49, SteveT wrote:
> Does the functionality in VariationsService have much to do with the
> functionality in MetricsService? In fact, I don't think MetricsService
actually
> uses the VS at all.

> I agree with you that we should avoid hanging more stuff off the
browser process
> if possible, but is it worth introducing a new member to the class
just for the
> class to hold on to it, without actually utilizing it?

Agreed, the MetricsService does not use the VariationsService directly.
However, it does have the same lifetime, and is generally what I think
of as the "central dispatcher" for all things related to metrics. It's
certainly not a perfect fit; my main motivation was to avoid adding yet
another dependency to the core browser process files.

> It seems like the VS has more to do with the FieldTrialList than
MetricsService,
> but I'm also reluctant to put it in there :)

Yeah, if the FieldTrialList code wasn't in base/, that would almost
certainly be the right place to store this... but given that it *is* in
base/, that wouldn't really work.

commi...@chromium.org

unread,

May 16, 2012, 2:03:46 PM5/16/12

to j...@chromium.org, ste...@chromium.org, chromium...@chromium.org, m...@chromium.org, ishe...@chromium.org, j...@chromium.org

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/j...@chromium.org/10392007/5024

https://chromiumcodereview.appspot.com/10392007/

asvi...@chromium.org

unread,

May 16, 2012, 3:53:05 PM5/16/12

to j...@chromium.org, ste...@chromium.org, chromium...@chromium.org, m...@chromium.org, ishe...@chromium.org, j...@chromium.org

http://codereview.chromium.org/10392007/diff/5024/chrome/browser/chrome_browser_main.cc
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/10392007/diff/5024/chrome/browser/chrome_browser_main.cc#newcode1876
chrome/browser/chrome_browser_main.cc:1876: // Request new variations
seed information from server.
This comment should be above the
"browser_process_->variations_service()->StartFetchingVariationsSeed();"
line, no?

http://codereview.chromium.org/10392007/

asvi...@chromium.org

unread,

May 16, 2012, 3:53:08 PM5/16/12

to j...@chromium.org, ste...@chromium.org, chromium...@chromium.org, m...@chromium.org, ishe...@chromium.org, j...@chromium.org

http://codereview.chromium.org/10392007/

commi...@chromium.org

unread,

May 16, 2012, 3:53:56 PM5/16/12

to j...@chromium.org, ste...@chromium.org, asvi...@chromium.org, chromium...@chromium.org, m...@chromium.org, ishe...@chromium.org, j...@chromium.org

List of reviewers changed. asvi...@chromium.org did a drive-by without
LGTM'ing!

https://chromiumcodereview.appspot.com/10392007/

j...@chromium.org

unread,

May 16, 2012, 4:10:43 PM5/16/12

to ste...@chromium.org, asvi...@chromium.org, chromium...@chromium.org, m...@chromium.org, ishe...@chromium.org, j...@chromium.org

https://chromiumcodereview.appspot.com/10392007/diff/5024/chrome/browser/chrome_browser_main.cc
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/10392007/diff/5024/chrome/browser/chrome_browser_main.cc#newcode1876

chrome/browser/chrome_browser_main.cc:1876: // Request new variations
seed information from server.

On 2012/05/16 19:53:06, Alexei Svitkine wrote:
> This comment should be above the

"browser_process_->variations_service()->StartFetchingVariationsSeed();"
line,
> no?

Done.

https://chromiumcodereview.appspot.com/10392007/