Fixed handling of 'oem_device_requisition' VPD value.

Fixed handling of 'oem_device_requisition' VPD value.

BUG=348238
TEST=manual

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

[xiyuan, 2014-03-01 04:38:58]

https://codereview.chromium.org/183923008/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc
(right):

https://codereview.chromium.org/183923008/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:135: //
OEM statistics are only loaded when OOBE is not completed.
nit: Move to the comment to above line 129? It feels more relevant there.

https://codereview.chromium.org/183923008/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h
(right):

https://codereview.chromium.org/183923008/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h:118: //
Initializes requisition settings at OOBwith values from VPD.
OOBwith -> OOBE with?

https://codereview.chromium.org/183923008/diff/20001/chrome/browser/resources...
File chrome/browser/resources/login/display_manager.js (right):

https://codereview.chromium.org/183923008/diff/20001/chrome/browser/resources...
chrome/browser/resources/login/display_manager.js:607:
onConfirmDeviceRequisitionPrompt_: function(value) {
Looks like the version at line 597-600 is the one we want to keep. This one here
should be removed.

[zel, 2014-03-01 21:30:26]

+mnissler for policy changes

https://codereview.chromium.org/183923008/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc
(right):

https://codereview.chromium.org/183923008/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:135: //
OEM statistics are only loaded when OOBE is not completed.
On 2014/03/01 04:38:58, xiyuan wrote:
> nit: Move to the comment to above line 129? It feels more relevant there.

Done.

https://codereview.chromium.org/183923008/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h
(right):

https://codereview.chromium.org/183923008/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h:118: //
Initializes requisition settings at OOBwith values from VPD.
On 2014/03/01 04:38:58, xiyuan wrote:
> OOBwith -> OOBE with?

Done.

https://codereview.chromium.org/183923008/diff/20001/chrome/browser/resources...
File chrome/browser/resources/login/display_manager.js (right):

https://codereview.chromium.org/183923008/diff/20001/chrome/browser/resources...
chrome/browser/resources/login/display_manager.js:607:
onConfirmDeviceRequisitionPrompt_: function(value) {
On 2014/03/01 04:38:58, xiyuan wrote:
> Looks like the version at line 597-600 is the one we want to keep. This one
here
> should be removed.

Done.

[xiyuan, 2014-03-01 21:35:37]

lgtm

[zel, 2014-03-02 01:31:33]

The CQ bit was checked by zelidrag@chromium.org

[commit-bot: I haz the power, 2014-03-02 01:31:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zelidrag@chromium.org/183923008/60001

[commit-bot: I haz the power, 2014-03-02 02:21:04]

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

[commit-bot: I haz the power, 2014-03-02 02:21:04]

Retried try job too often on win_x64_rel for step(s) base_unittests,
chrome_elf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[Mattias Nissler (ping if slow), 2014-03-02 08:35:07]

https://codereview.chromium.org/183923008/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc
(right):

https://codereview.chromium.org/183923008/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:181:
local_state_->SetBoolean(prefs::kDeviceEnrollmentCanExit, false);
This will become problematic once we're starting to use device requisitions in
cases other than remora, and want to pass information to the server without
enforcing enrollment. Can we do this only for remora, or better, get these from
VPD as well?

[zel, 2014-03-03 19:43:39]

https://codereview.chromium.org/183923008/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc
(right):

https://codereview.chromium.org/183923008/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:181:
local_state_->SetBoolean(prefs::kDeviceEnrollmentCanExit, false);
On 2014/03/02 08:35:08, Mattias Nissler wrote:
> This will become problematic once we're starting to use device requisitions in
> cases other than remora, and want to pass information to the server without
> enforcing enrollment. Can we do this only for remora, or better, get these
from
> VPD as well?

This should stay the way it is since it's only called from UI (ctrl+alt+h) and
we don't have ability to tweak these settings from there.

I did implement your comments in InitalizeRequisition() instead. Values of
kDeviceEnrollmentAutoStart and kDeviceEnrollmentCanExit can be seeded from VPD
now and 'remora' with set them automatically.

https://codereview.chromium.org/183923008/diff/40002/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc
(right):

https://codereview.chromium.org/183923008/diff/40002/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:314: if
(requisition == kRemoraRequisition) {
mnissler@, this is where your suggestions from SetDeviceRequisition() ended
up...

[zel, 2014-03-03 21:17:03]

The CQ bit was checked by zelidrag@chromium.org

[commit-bot: I haz the power, 2014-03-03 21:17:24]

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

[commit-bot: I haz the power, 2014-03-03 22:26:30]

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

[commit-bot: I haz the power, 2014-03-03 22:26:30]

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...

[zel, 2014-03-03 22:31:49]

The CQ bit was checked by zelidrag@chromium.org

[commit-bot: I haz the power, 2014-03-03 22:36:14]

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

[Mattias Nissler (ping if slow), 2014-03-03 22:44:57]

New version LGTM.

I put an inline comment with a suggestion for further simplification for your
consideration.

https://codereview.chromium.org/183923008/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc
(right):

https://codereview.chromium.org/183923008/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:179: if
(requisition == kNoRequisition) {
Looking at calling code, we might actually get away with setting an empty string
in kDeviceEnrollmentRequisition instead of using the kNoRequisition sentinel
value.

[commit-bot: I haz the power, 2014-03-04 00:16:33]

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

[commit-bot: I haz the power, 2014-03-04 00:16:34]

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...

[zel, 2014-03-04 02:29:12]

The CQ bit was checked by zelidrag@chromium.org

[zel, 2014-03-04 02:31:36]

https://codereview.chromium.org/183923008/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc
(right):

https://codereview.chromium.org/183923008/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:179: if
(requisition == kNoRequisition) {
On 2014/03/03 22:44:57, Mattias Nissler wrote:
> Looking at calling code, we might actually get away with setting an empty
string
> in kDeviceEnrollmentRequisition instead of using the kNoRequisition sentinel
> value.

An empty string would mean 'absent value' while kNoRequisition is explicitly
overriding values from VPD.

[commit-bot: I haz the power, 2014-03-04 02:31:51]

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

[zel, 2014-03-04 02:32:15]

The CQ bit was unchecked by zelidrag@chromium.org

[zel, 2014-03-04 02:33:10]

The CQ bit was checked by zelidrag@chromium.org

[commit-bot: I haz the power, 2014-03-04 05:54:50]

Change committed as 254685