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

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

The CLs that this is restoring are http://codereview.chromium.org/10343007 and http://codereview.chromium.org/10381021. 

BUG=121695
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137662

[SteveT, 2012-05-14 21:08:57]

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/chr...
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/10392007/diff/11018/chrome/browser/chr...
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/met...
File chrome/browser/metrics/variations_service.h (right):

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

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

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

[jwd, 2012-05-15 14:58:36]

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/chr...
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/10392007/diff/11018/chrome/browser/chr...
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/met...
File chrome/browser/metrics/variations_service.h (right):

https://chromiumcodereview.appspot.com/10392007/diff/11018/chrome/browser/met...
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/met...
File chrome/browser/metrics/variations_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/10392007/diff/11018/chrome/browser/met...
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.

[SteveT, 2012-05-15 16:56:46]

lgtm

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

https://chromiumcodereview.appspot.com/10392007/diff/5018/chrome/browser/metr...
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.

[jwd, 2012-05-15 18:31:23]

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

https://chromiumcodereview.appspot.com/10392007/diff/5018/chrome/browser/metr...
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.

[Ilya Sherman, 2012-05-15 22:11:17]

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

https://chromiumcodereview.appspot.com/10392007/diff/6019/chrome/browser/brow...
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/brow...
File chrome/browser/browser_process_impl.h (right):

https://chromiumcodereview.appspot.com/10392007/diff/6019/chrome/browser/brow...
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/chro...
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/10392007/diff/6019/chrome/browser/chro...
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.

[commit-bot: I haz the power, 2012-05-16 12:55:44]

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

[SteveT, 2012-05-16 14:33:49]

comment inline for Ilya

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

https://chromiumcodereview.appspot.com/10392007/diff/6019/chrome/browser/brow...
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?

[jwd, 2012-05-16 14:47:31]

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

https://chromiumcodereview.appspot.com/10392007/diff/6019/chrome/browser/brow...
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/brow...
File chrome/browser/browser_process_impl.h (right):

https://chromiumcodereview.appspot.com/10392007/diff/6019/chrome/browser/brow...
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/chro...
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/10392007/diff/6019/chrome/browser/chro...
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.

[Ilya Sherman, 2012-05-16 17:57:22]

(LGTM)

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

https://chromiumcodereview.appspot.com/10392007/diff/6019/chrome/browser/brow...
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/brow...
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.

> Any thoughts on these questions?

[commit-bot: I haz the power, 2012-05-16 18:03:46]

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

[Alexei Svitkine (slow), 2012-05-16 19:53:05]

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

http://codereview.chromium.org/10392007/diff/5024/chrome/browser/chrome_brows...
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?

[commit-bot: I haz the power, 2012-05-16 19:53:56]

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

[jwd, 2012-05-16 20:10:43]

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

https://chromiumcodereview.appspot.com/10392007/diff/5024/chrome/browser/chro...
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.

[Alexei Svitkine (slow), 2012-05-16 20:11:57]

LGTM

[commit-bot: I haz the power, 2012-05-17 14:14:37]

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

[commit-bot: I haz the power, 2012-05-17 15:36:24]

Change committed as 137662