Refactored the DeviceManagementService to get its parameters from a delegate.

Refactored the DeviceManagementService to get its parameters from a delegate.

The DeviceManagementService will be moved along with the policy/ code into a new
component, and thus it can't access //chrome nor //chromeos code. A recent
refactoring in that direction introduced a regression because it called
StatisticsProvider::GetMachineStatistic() before the StatisticsProvider was
initialized.

This used to work before because GetMachineStatistic() was called only once the
first request was sent to the server (which, on enrolled devices, happens in
a task that is immediately posted but the StatisticsProvider is already
initialized by then).

This change introduces a delegate that the DeviceManagementService uses to
get its configuration parameters, so that the call to GetMachineStatistic()
can again be performed only when a request is sent.

BUG=271392, 302798
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226499

[Joao da Silva, 2013-10-02 10:00:30]

Julian: please review

Steven: FYI. This simplifies your fix in
https://codereview.chromium.org/25112004 without breaking the policy component
work.

Thanks!

[pastarmovj, 2013-10-02 11:14:10]

LGTM with a couple of small nits.

https://codereview.chromium.org/25690003/diff/1/chrome/browser/policy/cloud/d...
File chrome/browser/policy/cloud/device_management_service.cc (right):

https://codereview.chromium.org/25690003/diff/1/chrome/browser/policy/cloud/d...
chrome/browser/policy/cloud/device_management_service.cc:534:
weak_ptr_factory_(this) {}
I think it makes sense to have a DCHECK(configuration_.get()) here to make sure
the configuration_ object is fine. It will help figure out issues earlier than
if it only crashes once the thing is invoked potentially much later.

https://codereview.chromium.org/25690003/diff/1/chrome/browser/policy/cloud/m...
File chrome/browser/policy/cloud/mock_device_management_service.cc (right):

https://codereview.chromium.org/25690003/diff/1/chrome/browser/policy/cloud/m...
chrome/browser/policy/cloud/mock_device_management_service.cc:139: return
"Chrome 1.2.3(456)";
nit: Can you please move those up to be named constants.

https://codereview.chromium.org/25690003/diff/1/chrome/browser/policy/cloud/m...
chrome/browser/policy/cloud/mock_device_management_service.cc:147: return
"Test|Unit|1.2.3";
ditto.

[Joao da Silva, 2013-10-02 11:37:40]

Thanks for the review!

https://codereview.chromium.org/25690003/diff/1/chrome/browser/policy/cloud/d...
File chrome/browser/policy/cloud/device_management_service.cc (right):

https://codereview.chromium.org/25690003/diff/1/chrome/browser/policy/cloud/d...
chrome/browser/policy/cloud/device_management_service.cc:534:
weak_ptr_factory_(this) {}
On 2013/10/02 11:14:10, pastarmovj wrote:
> I think it makes sense to have a DCHECK(configuration_.get()) here to make
sure
> the configuration_ object is fine. It will help figure out issues earlier than
> if it only crashes once the thing is invoked potentially much later.

Done.

https://codereview.chromium.org/25690003/diff/1/chrome/browser/policy/cloud/m...
File chrome/browser/policy/cloud/mock_device_management_service.cc (right):

https://codereview.chromium.org/25690003/diff/1/chrome/browser/policy/cloud/m...
chrome/browser/policy/cloud/mock_device_management_service.cc:139: return
"Chrome 1.2.3(456)";
On 2013/10/02 11:14:10, pastarmovj wrote:
> nit: Can you please move those up to be named constants.

Done.

https://codereview.chromium.org/25690003/diff/1/chrome/browser/policy/cloud/m...
chrome/browser/policy/cloud/mock_device_management_service.cc:147: return
"Test|Unit|1.2.3";
On 2013/10/02 11:14:10, pastarmovj wrote:
> ditto.

Done.

[commit-bot: I haz the power, 2013-10-02 11:37:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/25690003/11001

[commit-bot: I haz the power, 2013-10-02 13:31:57]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-10-02 13:33:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/25690003/11001

[commit-bot: I haz the power, 2013-10-02 18:05:02]

Change committed as 226499

[stevenjb, 2013-10-02 18:20:11]

This is definitely better, thanks.