Turn VariationsService::GetVariationsServerURL into instance method

Turn VariationsService::GetVariationsServerURL into instance method

This will allow for followup work that uses |client_| within that
method.

Reland of https://codereview.chromium.org/1309843005 with the browsertests
fixed to remove the unsafe accesses to g_browser_process->variations_service().

BUG=516678
Committed: https://crrev.com/dcc53f65eb04018ca60b077a55aa5923ac5e8e9c
Cr-Commit-Position: refs/heads/master@{#346878}

[blundell, 2015-08-31 09:29:28]

Patchset #2 (id:20001) has been deleted

[blundell, 2015-08-31 12:06:41]

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

[blundell, 2015-08-31 12:06:41]

This is a reland that avoids the problem of the browsertests using a null
VariationsService. PS1 is https://codereview.chromium.org/1312423003/, and PS2
is the fix.

asvitkine: Could you review the delta from PS1 to PS2?
bartfab: Could you review the delta from PS1 to PS2 in the policy browsertests?

Thanks!

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

https://codereview.chromium.org/1313213005/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc
(right):

https://codereview.chromium.org/1313213005/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc:51:
g_browser_process->local_state(), nullptr),
It's pretty weird that with the change to pass in a client to VariationsService,
it takes the client as a scoped ptr but the notifier as a regular pointer, even
though it takes ownership of both.

Could you fix the API to take both by scoped_ptr? (Since you added inconsistency
when adding the client.) Thanks!

https://codereview.chromium.org/1313213005/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc:52:
g_browser_process->local_state(), NULL);
nullptr

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

https://codereview.chromium.org/1313213005/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_service.h:182:
VariationsURLIsValid);
Hmm, once VariationsService moves to a component, this becomes a layering
violation.

Can we instead add ForTesting() methods to avoid needing to add friends?

[blundell, 2015-09-01 08:50:31]

Patchset #3 (id:60001) has been deleted

[blundell, 2015-09-01 09:10:19]

Patchset #3 (id:80001) has been deleted

[blundell, 2015-09-01 10:03:06]

https://codereview.chromium.org/1313213005/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc
(right):

https://codereview.chromium.org/1313213005/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc:51:
g_browser_process->local_state(), nullptr),
On 2015/08/31 15:03:02, Alexei Svitkine wrote:
> It's pretty weird that with the change to pass in a client to
VariationsService,
> it takes the client as a scoped ptr but the notifier as a regular pointer,
even
> though it takes ownership of both.
> 
> Could you fix the API to take both by scoped_ptr? (Since you added
inconsistency
> when adding the client.) Thanks!

Done.

https://codereview.chromium.org/1313213005/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc:52:
g_browser_process->local_state(), NULL);
On 2015/08/31 15:03:02, Alexei Svitkine wrote:
> nullptr

Done.

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

https://codereview.chromium.org/1313213005/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_service.h:182:
VariationsURLIsValid);
On 2015/08/31 15:03:02, Alexei Svitkine wrote:
> Hmm, once VariationsService moves to a component, this becomes a layering
> violation.
> 
> Can we instead add ForTesting() methods to avoid needing to add friends?

Done.

[Alexei Svitkine (slow), 2015-09-01 14:50:26]

lgtm

[bartfab (slow), 2015-09-02 09:57:29]

chrome/browser/policy/*
chrome/browser/chromeos/policy/*

LGTM

https://codereview.chromium.org/1313213005/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc
(right):

https://codereview.chromium.org/1313213005/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc:47:
scoped_ptr<chrome_variations::VariationsService> service =
Nit: #include "base/memory/scoped_ptr.h"

https://codereview.chromium.org/1313213005/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc:50:
g_browser_process->local_state());
Nit: #include "chrome/browser/browser_process.h"

https://codereview.chromium.org/1313213005/diff/100001/chrome/browser/policy/...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/1313213005/diff/100001/chrome/browser/policy/...
chrome/browser/policy/policy_browsertest.cc:3780:
scoped_ptr<chrome_variations::VariationsService> service =
Nit: #include "base/memory/scoped_ptr.h"

[blundell, 2015-09-02 10:02:18]

https://codereview.chromium.org/1313213005/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc
(right):

https://codereview.chromium.org/1313213005/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc:47:
scoped_ptr<chrome_variations::VariationsService> service =
On 2015/09/02 09:57:29, bartfab wrote:
> Nit: #include "base/memory/scoped_ptr.h"

Done.

https://codereview.chromium.org/1313213005/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc:50:
g_browser_process->local_state());
On 2015/09/02 09:57:28, bartfab wrote:
> Nit: #include "chrome/browser/browser_process.h"

Done.

https://codereview.chromium.org/1313213005/diff/100001/chrome/browser/policy/...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/1313213005/diff/100001/chrome/browser/policy/...
chrome/browser/policy/policy_browsertest.cc:3780:
scoped_ptr<chrome_variations::VariationsService> service =
On 2015/09/02 09:57:29, bartfab wrote:
> Nit: #include "base/memory/scoped_ptr.h"

Done.

[blundell, 2015-09-02 10:03:30]

The CQ bit was checked by blundell@chromium.org

[blundell, 2015-09-02 10:03:31]

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

[commit-bot: I haz the power, 2015-09-02 10:03:42]

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

[commit-bot: I haz the power, 2015-09-02 10:33:27]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2015-09-02 10:34:04]

Patchset 5 (id:??) landed as
https://crrev.com/dcc53f65eb04018ca60b077a55aa5923ac5e8e9c
Cr-Commit-Position: refs/heads/master@{#346878}

[blundell, 2015-09-02 11:23:23]

A revert of this CL (patchset #5 id:140001) has been created in
https://codereview.chromium.org/1306913003/ by blundell@chromium.org.

The reason for reverting is: Gak! The CreateForTesting method can't call the
Create method because the latter returns null in non-official builds -- exactly
the problem from the original CL :P. I don't understand why this CL passed
trybots..