Simulate variation seeds using installed version string.

Simulate variation seeds using installed version string.

Exposes getting the installed version from UpgradeDetector and
uses it from VariationService when simulating new Finch seeds,
so that the simulation more accurately reflects what will happen
on a restart when that seed will be applied.

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

[Alexei Svitkine (slow), 2014-06-17 17:57:17]

Finnur: Please review changes to UpgradeDetectorImpl
Ilya: Please review changes to VariationsService (as Jesse is on vacation)

Also, perhaps the method I'm exposing from UpgradeDetectorImpl should live
outside of that class? If so, suggestions welcome for where it should live.
Thanks!

[Ilya Sherman, 2014-06-17 21:25:00]

https://codereview.chromium.org/333313003/diff/40001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/333313003/diff/40001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.cc:249: }
nit: It would be nice if there weren't multiple constructors for this class...
(not necessarily something that needs to be addressed as part of this CL, but
just something I'm noticing from looking at this code).  (Alternately, it would
be nice if we could C++11 already.)

https://codereview.chromium.org/333313003/diff/40001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.cc:440:
content::BrowserThread::FILE, FROM_HERE,
I believe that the shared blocking thread pool is preferred over the file thread
these days.

https://codereview.chromium.org/333313003/diff/40001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_service.h (right):

https://codereview.chromium.org/333313003/diff/40001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.h:16: #include
"base/version.h"
nit: Can this be forward-declared?

https://codereview.chromium.org/333313003/diff/40001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.h:150: // Performs a
variations seed simulation with the given |seed| and |version|.
From just reading the declaration and comment, it's unclear to me why this is a
void method.  What happens to the result of the simulation?

[Alexei Svitkine (slow), 2014-06-17 22:15:55]

https://codereview.chromium.org/333313003/diff/40001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/333313003/diff/40001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.cc:249: }
On 2014/06/17 21:24:59, Ilya Sherman wrote:
> nit: It would be nice if there weren't multiple constructors for this class...
> (not necessarily something that needs to be addressed as part of this CL, but
> just something I'm noticing from looking at this code).  (Alternately, it
would
> be nice if we could C++11 already.)

I think this can be cleaned up. File http://crbug.com/385843

https://codereview.chromium.org/333313003/diff/40001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.cc:440:
content::BrowserThread::FILE, FROM_HERE,
On 2014/06/17 21:24:59, Ilya Sherman wrote:
> I believe that the shared blocking thread pool is preferred over the file
thread
> these days.

When I try that, I get:

../../chrome/browser/metrics/variations/variations_service.cc:439:46: error: no
member named 'PostTaskAndReplyWithResult' in 'base::SequencedWorkerPool'
  content::BrowserThread::GetBlockingPool()->PostTaskAndReplyWithResult(
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~  ^

I suppose base::SequencedWorkerPool can be expanded with that API, but I'd
rather not shave an extra yak unless you insist. Let me know what you prefer.

https://codereview.chromium.org/333313003/diff/40001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_service.h (right):

https://codereview.chromium.org/333313003/diff/40001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.h:16: #include
"base/version.h"
On 2014/06/17 21:24:59, Ilya Sherman wrote:
> nit: Can this be forward-declared?

Done.

https://codereview.chromium.org/333313003/diff/40001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.h:150: // Performs a
variations seed simulation with the given |seed| and |version|.
On 2014/06/17 21:24:59, Ilya Sherman wrote:
> From just reading the declaration and comment, it's unclear to me why this is
a
> void method.  What happens to the result of the simulation?

Currently, it only logs histograms. In another CL (that's under review) it will
notify observers based on the simulation results. I've expanded the comment to
mention the current behavior.

[Ilya Sherman, 2014-06-17 22:27:53]

https://codereview.chromium.org/333313003/diff/40001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/333313003/diff/40001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.cc:249: }
On 2014/06/17 22:15:54, Alexei Svitkine wrote:
> On 2014/06/17 21:24:59, Ilya Sherman wrote:
> > nit: It would be nice if there weren't multiple constructors for this
class...
> > (not necessarily something that needs to be addressed as part of this CL,
but
> > just something I'm noticing from looking at this code).  (Alternately, it
> would
> > be nice if we could C++11 already.)
> 
> I think this can be cleaned up. File http://crbug.com/385843

Thanks :)

https://codereview.chromium.org/333313003/diff/40001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.cc:440:
content::BrowserThread::FILE, FROM_HERE,
On 2014/06/17 22:15:54, Alexei Svitkine wrote:
> On 2014/06/17 21:24:59, Ilya Sherman wrote:
> > I believe that the shared blocking thread pool is preferred over the file
> thread
> > these days.
> 
> When I try that, I get:
> 
> ../../chrome/browser/metrics/variations/variations_service.cc:439:46: error:
no
> member named 'PostTaskAndReplyWithResult' in 'base::SequencedWorkerPool'
>   content::BrowserThread::GetBlockingPool()->PostTaskAndReplyWithResult(
>   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~  ^
> 
> I suppose base::SequencedWorkerPool can be expanded with that API, but I'd
> rather not shave an extra yak unless you insist. Let me know what you prefer.

Can you use PostTaskAndReplyWithResult() from base/task_runner_util.h?

[Finnur, 2014-06-18 11:57:12]

UpgradeDetector changes LGTM.

[Finnur, 2014-06-18 11:57:44]

(PS. Didn't see your request yesterday due to national holiday).

[Alexei Svitkine (slow), 2014-06-18 15:18:56]

https://codereview.chromium.org/333313003/diff/40001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/333313003/diff/40001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.cc:440:
content::BrowserThread::FILE, FROM_HERE,
On 2014/06/17 22:27:53, Ilya Sherman wrote:
> On 2014/06/17 22:15:54, Alexei Svitkine wrote:
> > On 2014/06/17 21:24:59, Ilya Sherman wrote:
> > > I believe that the shared blocking thread pool is preferred over the file
> > thread
> > > these days.
> > 
> > When I try that, I get:
> > 
> > ../../chrome/browser/metrics/variations/variations_service.cc:439:46: error:
> no
> > member named 'PostTaskAndReplyWithResult' in 'base::SequencedWorkerPool'
> >   content::BrowserThread::GetBlockingPool()->PostTaskAndReplyWithResult(
> >   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~  ^
> > 
> > I suppose base::SequencedWorkerPool can be expanded with that API, but I'd
> > rather not shave an extra yak unless you insist. Let me know what you
prefer.
> 
> Can you use PostTaskAndReplyWithResult() from base/task_runner_util.h?

Done. Kind of silly that we have a bunch of different ways to do this, depending
on the situation (instance methods, static methods, free-standing functions,
sigh). Thanks for finding the right one here.

[Ilya Sherman, 2014-06-18 17:11:36]

LGTM, thanks.

https://codereview.chromium.org/333313003/diff/80001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_service.h (right):

https://codereview.chromium.org/333313003/diff/80001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.h:15: #include
"base/task_runner_util.h"
nit: Does this need to be in the header file, or can it be moved to the
implementation file?

[Alexei Svitkine (slow), 2014-06-18 17:17:18]

https://codereview.chromium.org/333313003/diff/80001/chrome/browser/metrics/v...
File chrome/browser/metrics/variations/variations_service.h (right):

https://codereview.chromium.org/333313003/diff/80001/chrome/browser/metrics/v...
chrome/browser/metrics/variations/variations_service.h:15: #include
"base/task_runner_util.h"
On 2014/06/18 17:11:36, Ilya Sherman wrote:
> nit: Does this need to be in the header file, or can it be moved to the
> implementation file?

Oops, meant to add it to the .cc file, but I guess I had the wrong file open.
Fixed now.

[Alexei Svitkine (slow), 2014-06-18 17:17:32]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-06-18 17:18:43]

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

[commit-bot: I haz the power, 2014-06-19 05:38:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-19 05:38:09]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[Alexei Svitkine (slow), 2014-06-19 13:51:52]

The CQ bit was checked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2014-06-19 13:51:58]

The CQ bit was unchecked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2014-06-19 14:33:03]

+sky for chrome/ OWNERS

[sky, 2014-06-19 15:51:11]

LGTM

[Alexei Svitkine (slow), 2014-06-19 16:25:16]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-06-19 16:28:54]

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

[Alexei Svitkine (slow), 2014-06-19 18:39:56]

The CQ bit was unchecked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2014-06-19 18:50:11]

Finnur, can you take another look?

I had to make a change to upgrade_detector_impl.cc to fix a compile issue on
Windows as the GetCriticalUpdateVersion() code actually needed variables that
were local to the new function. I've now introduced a helper function in the
anon namespace.

[Finnur, 2014-06-20 10:30:57]

LGTM

[Alexei Svitkine (slow), 2014-06-20 12:33:09]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-06-20 12:34:30]

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

[commit-bot: I haz the power, 2014-06-20 13:46:28]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-20 14:39:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-20 14:39:05]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[Alexei Svitkine (slow), 2014-06-20 14:42:42]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-06-20 14:44:12]

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

[commit-bot: I haz the power, 2014-06-20 15:18:00]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-20 17:20:46]

Change committed as 278740