Add DeviceDisablingManager to manage device disabling

Add DeviceDisablingManager to manage device disabling

This CL moves the logic that determines whether the device is disabled
from the DeviceDisabledScreen into a new DeviceDisablingManager. For
now, this is a no-op refactoring. In a follow-up CL, the new manager
will also become responsible for disabling the device on the fly
when the appropriate signal is received during a device policy fetch.

BUG=425574
TEST=New unit tests; updated unit and browser tests

Committed: https://crrev.com/4fd3a33e7c382fbf84757ef8bd56a2fd6fc5e7db
Cr-Commit-Position: refs/heads/master@{#303105}

[bartfab (slow), 2014-11-05 13:16:57]

bartfab@chromium.org changed reviewers:
+ achuith@chromium.org, antrim@chromium.org, thestig@chromium.org

[bartfab (slow), 2014-11-05 13:16:57]

Hi Lei,

Could you please take a look at:
chrome/browser/browser_process_platform_part_chromeos.*

Hi Achuith,

Could you please take a look at:
chrome/browser/chromeos/*

Hi Denis,

Could you please take a look at:
chrome/browser/chromeos/login/*

[achuithb, 2014-11-05 19:59:55]

owner lgtm.

Btw, would this feature benefit from an integration test? If so, let's file a
bug to track it.

https://codereview.chromium.org/692383005/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system/device_disabling_manager.cc (right):

https://codereview.chromium.org/692383005/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_manager.cc:61:
callback.Run(false);
Should there be a NOTREACHED here? Does this indicate a bug in the code?

https://codereview.chromium.org/692383005/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system/device_disabling_manager.h (right):

https://codereview.chromium.org/692383005/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_manager.h:21: // Remote device
disabling is triggered in two different ways, depending on the
Maybe add a sentence to express what device disabling is.

https://codereview.chromium.org/692383005/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_manager.h:44: const std::string&
GetDisabledMessage() const;
Shouldn't this be:
const std::string& disabled_message() const { return disabled_message_; }

https://codereview.chromium.org/692383005/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_manager.h:48: using
DeviceDisabledCheckCallback = base::Callback<void(bool)>;
This replaces typedef, right? I'm not able to find what the style guide says
about the placement of using - shouldn't this be at the very top? I feel like it
should.

https://codereview.chromium.org/692383005/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system/device_disabling_manager_unittest.cc
(right):

https://codereview.chromium.org/692383005/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_manager_unittest.cc:37:
~DeviceDisablingManagerTest();
virtual or override?

[Lei Zhang, 2014-11-05 22:28:54]

lgtm

[bartfab (slow), 2014-11-06 16:14:38]

https://codereview.chromium.org/692383005/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system/device_disabling_manager.cc (right):

https://codereview.chromium.org/692383005/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_manager.cc:61:
callback.Run(false);
On 2014/11/05 19:59:54, achuithb wrote:
> Should there be a NOTREACHED here? Does this indicate a bug in the code?

Yes, it does indicate a bug in the code. However, I am exercising this code path
in a test (to make sure that the error condition is handled correctly), so I
cannot do NOTREACHED() as it would crash the test. I added a LOG(ERROR).

https://codereview.chromium.org/692383005/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system/device_disabling_manager.h (right):

https://codereview.chromium.org/692383005/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_manager.h:21: // Remote device
disabling is triggered in two different ways, depending on the
On 2014/11/05 19:59:55, achuithb wrote:
> Maybe add a sentence to express what device disabling is.

Done.

https://codereview.chromium.org/692383005/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_manager.h:44: const std::string&
GetDisabledMessage() const;
On 2014/11/05 19:59:55, achuithb wrote:
> Shouldn't this be:
> const std::string& disabled_message() const { return disabled_message_; }

I always read the style guide as permitting either regular CamelCaps accessors
or hacker_style accessors. Upon re-reading the style guide right now, it seems
that only hacker_style is permitted. Maybe this changed at some point.

Done.

https://codereview.chromium.org/692383005/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_manager.h:48: using
DeviceDisabledCheckCallback = base::Callback<void(bool)>;
On 2014/11/05 19:59:55, achuithb wrote:
> This replaces typedef, right? I'm not able to find what the style guide says
> about the placement of using - shouldn't this be at the very top? I feel like
it
> should.

The style guide has not caught up to using yet. Treating it analogously to
typedefs makes sense. Done.

https://codereview.chromium.org/692383005/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system/device_disabling_manager_unittest.cc
(right):

https://codereview.chromium.org/692383005/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/device_disabling_manager_unittest.cc:37:
~DeviceDisablingManagerTest();
On 2014/11/05 19:59:55, achuithb wrote:
> virtual or override?

Actually, the explicit distructor is completely unnecessary. Removed.

[bartfab (slow), 2014-11-06 16:15:22]

> Btw, would this feature benefit from an integration test? If so, let's file a
> bug to track it.

An integration browser test is coming https://codereview.chromium.org/711433002/
which I just uploaded :).

[Denis Kuznetsov (DE-MUC), 2014-11-06 16:34:29]

https://chromiumcodereview.appspot.com/692383005/diff/40001/chrome/browser/ch...
File chrome/browser/chromeos/system/device_disabling_manager.cc (right):

https://chromiumcodereview.appspot.com/692383005/diff/40001/chrome/browser/ch...
chrome/browser/chromeos/system/device_disabling_manager.cc:64: 
Add a comment/DCHECK explicitly stating condition (e.g. policy is
RESTORE_MODE_DISABLED.

[Denis Kuznetsov (DE-MUC), 2014-11-06 17:07:35]

lgtm

[bartfab (slow), 2014-11-06 17:08:15]

https://chromiumcodereview.appspot.com/692383005/diff/40001/chrome/browser/ch...
File chrome/browser/chromeos/system/device_disabling_manager.cc (right):

https://chromiumcodereview.appspot.com/692383005/diff/40001/chrome/browser/ch...
chrome/browser/chromeos/system/device_disabling_manager.cc:64: 
On 2014/11/06 16:34:29, Denis Kuznetsov wrote:
> Add a comment/DCHECK explicitly stating condition (e.g. policy is
> RESTORE_MODE_DISABLED.

Done.

[bartfab (slow), 2014-11-06 20:40:37]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-11-06 20:41:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/692383005/120001

[commit-bot: I haz the power, 2014-11-06 21:08:06]

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

[commit-bot: I haz the power, 2014-11-06 21:08:07]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bartfab (slow), 2014-11-06 21:49:00]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-11-06 21:49:27]

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

[commit-bot: I haz the power, 2014-11-06 22:43:22]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-11-06 22:44:01]

Patchset 8 (id:??) landed as
https://crrev.com/4fd3a33e7c382fbf84757ef8bd56a2fd6fc5e7db
Cr-Commit-Position: refs/heads/master@{#303105}