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

[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]

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]

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]

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]

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]

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]

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]

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]

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]

(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]

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]

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]

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

[commi...@chromium.org]

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

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

[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/

[asvi...@chromium.org]

LGTM

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

[commi...@chromium.org]

CQ is trying da patch. Follow status at

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


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

[commi...@chromium.org]

Change committed as 137662

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