Fetch policy from Active Directory at the end of enrollment

Fetch policy from Active Directory at the end of enrollment

BUG=667325
TBR=alemate (mechanical changes to enterprise_enrollment_helper_impl.cc and enrollment_screen_handler.cc)
TBR=jwd (mechanical change to histograms.xml)

Committed: https://crrev.com/6c70a1e3ff4531c3b7af8d17863bf8b6c239eeca
Cr-Commit-Position: refs/heads/master@{#435909}

[Thiemo Nagel, 2016-12-01 11:51:39]

tnagel@chromium.org changed reviewers:
+ rsorokin@chromium.org

[Thiemo Nagel, 2016-12-01 11:51:40]

Hi Roman,

could you please take a look?

Thank you,
Thiemo

[Thiemo Nagel, 2016-12-01 11:53:07]

tnagel@chromium.org changed reviewers:
+ jwd@chromium.org

[Thiemo Nagel, 2016-12-01 11:53:07]

Hi Jesse,

could you please take a look at histograms.xml?

Thank you,
Thiemo

[Roman Sorokin (ftl), 2016-12-01 12:43:20]

https://codereview.chromium.org/2544773002/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/2544773002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:255: LOG(ERROR) <<
"Error in device policy store.";
Do we need that LOG? ReportResult should log the error.

https://codereview.chromium.org/2544773002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:496:
CHECK_EQ(true, install_attributes_->IsActiveDirectoryManaged());
s/CHECK_EQ(true, /CHECK(

https://codereview.chromium.org/2544773002/diff/1/chrome/browser/chromeos/set...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/2544773002/diff/1/chrome/browser/chromeos/set...
chrome/browser/chromeos/settings/device_settings_service.cc:136:
DCHECK(device_mode_ != policy::DEVICE_MODE_ENTERPRISE_AD);
Shouldn't that be CHECK?

[Thiemo Nagel, 2016-12-01 14:02:37]

The CQ bit was checked by tnagel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 14:02:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Thiemo Nagel, 2016-12-01 14:03:11]

Thank you, Roman.  Could you please take another look?

https://codereview.chromium.org/2544773002/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/2544773002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:255: LOG(ERROR) <<
"Error in device policy store.";
On 2016/12/01 12:43:19, Roman Sorokin wrote:
> Do we need that LOG? ReportResult should log the error.

You're right, but imho "Error in device policy store." is much clearer and
easier to understand than "Enrollment failed: 12, client: 0, validation: 0,
store: 6, lock: 0".

https://codereview.chromium.org/2544773002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:496:
CHECK_EQ(true, install_attributes_->IsActiveDirectoryManaged());
On 2016/12/01 12:43:19, Roman Sorokin wrote:
> s/CHECK_EQ(true, /CHECK(

What's wrong with using CHECK_EQ()?

https://codereview.chromium.org/2544773002/diff/1/chrome/browser/chromeos/set...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/2544773002/diff/1/chrome/browser/chromeos/set...
chrome/browser/chromeos/settings/device_settings_service.cc:136:
DCHECK(device_mode_ != policy::DEVICE_MODE_ENTERPRISE_AD);
On 2016/12/01 12:43:19, Roman Sorokin wrote:
> Shouldn't that be CHECK?

OK.  I've also filed
https://bugs.chromium.org/p/chromium/issues/detail?id=670280.

[Roman Sorokin (ftl), 2016-12-01 14:17:18]

https://codereview.chromium.org/2544773002/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/2544773002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:255: LOG(ERROR) <<
"Error in device policy store.";
On 2016/12/01 14:03:11, Thiemo Nagel (slow) wrote:
> On 2016/12/01 12:43:19, Roman Sorokin wrote:
> > Do we need that LOG? ReportResult should log the error.
> 
> You're right, but imho "Error in device policy store." is much clearer and
> easier to understand than "Enrollment failed: 12, client: 0, validation: 0,
> store: 6, lock: 0".
ok, but "this is bloating the Chrome executable by" 29 "Bytes"

https://codereview.chromium.org/2544773002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:496:
CHECK_EQ(true, install_attributes_->IsActiveDirectoryManaged());
On 2016/12/01 14:03:11, Thiemo Nagel (slow) wrote:
> On 2016/12/01 12:43:19, Roman Sorokin wrote:
> > s/CHECK_EQ(true, /CHECK(
> 
> What's wrong with using CHECK_EQ()?

looks like if (<smthg> == true) which should be just if(<smthg>)

[commit-bot: I haz the power, 2016-12-01 14:50:49]

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

[commit-bot: I haz the power, 2016-12-01 14:50:50]

Dry run: This issue passed the CQ dry run.

[Thiemo Nagel, 2016-12-01 17:05:59]

PTAL

https://codereview.chromium.org/2544773002/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/2544773002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:496:
CHECK_EQ(true, install_attributes_->IsActiveDirectoryManaged());
On 2016/12/01 14:17:18, Roman Sorokin wrote:
> On 2016/12/01 14:03:11, Thiemo Nagel (slow) wrote:
> > On 2016/12/01 12:43:19, Roman Sorokin wrote:
> > > s/CHECK_EQ(true, /CHECK(
> > 
> > What's wrong with using CHECK_EQ()?
> 
> looks like if (<smthg> == true) which should be just if(<smthg>)

I've changed it.  It seems that CHECK_EQ(true, ...) is rarely used across the
codebase.

[Roman Sorokin (ftl), 2016-12-02 09:53:52]

lgtm

[Thiemo Nagel, 2016-12-02 09:58:30]

Description was changed from

==========
Fetch policy from Active Directory at the end of enrollment

BUG=667325
==========

to

==========
Fetch policy from Active Directory at the end of enrollment

BUG=667325
TBR=alemate (mechanical changes to enterprise_enrollment_helper_impl.cc and
enrollment_screen_handler.cc)
TBR=jwd (mechanical change to histograms.xml)
==========

[Thiemo Nagel, 2016-12-02 09:58:44]

The CQ bit was checked by tnagel@chromium.org

[commit-bot: I haz the power, 2016-12-02 09:59:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 10:57:23]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480672724864750,
"parent_rev": "258fa2c7fafe842a5cf53789c7a026b519988afb", "commit_rev":
"2309805427d619f87dde2ba5872adc7926e3e80e"}

[commit-bot: I haz the power, 2016-12-02 10:57:47]

Description was changed from

==========
Fetch policy from Active Directory at the end of enrollment

BUG=667325
TBR=alemate (mechanical changes to enterprise_enrollment_helper_impl.cc and
enrollment_screen_handler.cc)
TBR=jwd (mechanical change to histograms.xml)
==========

to

==========
Fetch policy from Active Directory at the end of enrollment

BUG=667325
TBR=alemate (mechanical changes to enterprise_enrollment_helper_impl.cc and
enrollment_screen_handler.cc)
TBR=jwd (mechanical change to histograms.xml)
==========

[commit-bot: I haz the power, 2016-12-02 10:57:48]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-02 10:59:53]

Description was changed from

==========
Fetch policy from Active Directory at the end of enrollment

BUG=667325
TBR=alemate (mechanical changes to enterprise_enrollment_helper_impl.cc and
enrollment_screen_handler.cc)
TBR=jwd (mechanical change to histograms.xml)
==========

to

==========
Fetch policy from Active Directory at the end of enrollment

BUG=667325
TBR=alemate (mechanical changes to enterprise_enrollment_helper_impl.cc and
enrollment_screen_handler.cc)
TBR=jwd (mechanical change to histograms.xml)

Committed: https://crrev.com/6c70a1e3ff4531c3b7af8d17863bf8b6c239eeca
Cr-Commit-Position: refs/heads/master@{#435909}
==========

[commit-bot: I haz the power, 2016-12-02 10:59:54]

Patchset 3 (id:??) landed as
https://crrev.com/6c70a1e3ff4531c3b7af8d17863bf8b6c239eeca
Cr-Commit-Position: refs/heads/master@{#435909}