Chromad: Switch AuthPolicyClient to use cros_system_api enums.

Chromad: Switch AuthPolicyClient to use cros_system_api enums.

BUG=659732
TEST=manual

Committed: https://crrev.com/c36703e9325eeb51a1582043e2c536eb2078ad6b
Cr-Commit-Position: refs/heads/master@{#441026}

[Roman Sorokin (ftl), 2016-12-19 13:30:51]

rsorokin@chromium.org changed reviewers:
+ alemate@chromium.org, hashimoto@chromium.org

[Roman Sorokin (ftl), 2016-12-19 13:30:51]

Alex, PTAL
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.{h,cc}

Hashimoto, PTAL
chromeos/dbus/*

[Alexander Alekseev, 2016-12-21 14:45:30]

lgtm with nits.

https://codereview.chromium.org/2587993002/diff/1/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2587993002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:588:
authpolicy::ADJoinErrorType code) {
nit: please document here that code may be outside of
authpolicy::ADJoinErrorType domain because this value actually comes from DBUS
integer value . (Probably this is the reason you are using default: clause
here.)

https://codereview.chromium.org/2587993002/diff/1/chromeos/dbus/auth_policy_c...
File chromeos/dbus/auth_policy_client.h (right):

https://codereview.chromium.org/2587993002/diff/1/chromeos/dbus/auth_policy_c...
chromeos/dbus/auth_policy_client.h:21: public:
nit: please document here that error-code may be outside of
authpolicy::ADJoinErrorType domain because this value actually comes from DBUS
integer value .

[Roman Sorokin (ftl), 2016-12-21 17:20:12]

rsorokin@chromium.org changed reviewers:
+ stevenjb@chromium.org
- hashimoto@chromium.org

[Roman Sorokin (ftl), 2016-12-21 17:20:13]

Hi Steven, PTAL at
chromeos/dbus/*

[stevenjb, 2016-12-21 18:12:11]

https://codereview.chromium.org/2587993002/diff/1/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2587993002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:588:
authpolicy::ADJoinErrorType code) {
On 2016/12/21 14:45:30, Alexander Alekseev wrote:
> nit: please document here that code may be outside of
> authpolicy::ADJoinErrorType domain because this value actually comes from DBUS
> integer value . (Probably this is the reason you are using default: clause
> here.)

authpolicy::ADJoinErrorType is a DBus constant, so that shouldn't be possible.
If we are concerned about that, we could test for that when we cast the integer
in the dbus code. We shouldn't have to document that in the implementation code,
that kind of defeats the purpose of using an enum.

[Roman Sorokin (ftl), 2016-12-23 09:56:59]

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

[commit-bot: I haz the power, 2016-12-23 09:57:13]

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

[Roman Sorokin (ftl), 2016-12-23 10:03:05]

Hey Steven, PTAL. Thanks!

https://codereview.chromium.org/2587993002/diff/1/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2587993002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:588:
authpolicy::ADJoinErrorType code) {
On 2016/12/21 18:12:11, stevenjb wrote:
> On 2016/12/21 14:45:30, Alexander Alekseev wrote:
> > nit: please document here that code may be outside of
> > authpolicy::ADJoinErrorType domain because this value actually comes from
DBUS
> > integer value . (Probably this is the reason you are using default: clause
> > here.)
> 
> authpolicy::ADJoinErrorType is a DBus constant, so that shouldn't be possible.
> If we are concerned about that, we could test for that when we cast the
integer
> in the dbus code. We shouldn't have to document that in the implementation
code,
> that kind of defeats the purpose of using an enum.
> 

I added _COUNT member to the enum and check if the number we got from D-BUS is
within range.

[commit-bot: I haz the power, 2016-12-23 11:14:06]

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

[commit-bot: I haz the power, 2016-12-23 11:14:07]

Dry run: This issue passed the CQ dry run.

[Roman Sorokin (ftl), 2016-12-27 16:28:55]

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

[commit-bot: I haz the power, 2016-12-27 16:29:01]

Dry run: 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-27 17:26:24]

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

[commit-bot: I haz the power, 2016-12-27 17:26:25]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2016-12-27 17:52:26]

https://codereview.chromium.org/2587993002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2587993002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:572:
default:
We should list the unhandled cases explicitly and return here and not have a
default, then the compiler will complain if a new unhandled case is added. We
can also add NOTREACHED() after the switch.

https://codereview.chromium.org/2587993002/diff/60001/chromeos/dbus/auth_poli...
File chromeos/dbus/auth_policy_client.cc (right):

https://codereview.chromium.org/2587993002/diff/60001/chromeos/dbus/auth_poli...
chromeos/dbus/auth_policy_client.cc:131: error =
static_cast<authpolicy::ErrorType>(res);
Rather than duplicate this logic, add a helper function:
authpolicy::ErrorType error = GetErrorFromReader(reader);

[Roman Sorokin (ftl), 2016-12-28 10:58:45]

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

[commit-bot: I haz the power, 2016-12-28 10:58:49]

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

[Roman Sorokin (ftl), 2016-12-28 11:04:09]

Hey Steven, PTAL. Thanks!

https://codereview.chromium.org/2587993002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2587993002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:572:
default:
On 2016/12/27 17:52:26, stevenjb wrote:
> We should list the unhandled cases explicitly and return here and not have a
> default, then the compiler will complain if a new unhandled case is added. We
> can also add NOTREACHED() after the switch.

I think it's better to put NOTREACHED() after the default statement. That way
rolling out new cases from chromeos repo won't break chrome build. WDYT?

https://codereview.chromium.org/2587993002/diff/60001/chromeos/dbus/auth_poli...
File chromeos/dbus/auth_policy_client.cc (right):

https://codereview.chromium.org/2587993002/diff/60001/chromeos/dbus/auth_poli...
chromeos/dbus/auth_policy_client.cc:131: error =
static_cast<authpolicy::ErrorType>(res);
On 2016/12/27 17:52:26, stevenjb wrote:
> Rather than duplicate this logic, add a helper function:
> authpolicy::ErrorType error = GetErrorFromReader(reader);

Done.

[stevenjb, 2016-12-28 18:09:33]

On 2016/12/28 11:04:09, Roman Sorokin wrote:
> Hey Steven, PTAL. Thanks!
> 
>
https://codereview.chromium.org/2587993002/diff/60001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
> (right):
> 
>
https://codereview.chromium.org/2587993002/diff/60001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:572:
> default:
> On 2016/12/27 17:52:26, stevenjb wrote:
> > We should list the unhandled cases explicitly and return here and not have a
> > default, then the compiler will complain if a new unhandled case is added.
We
> > can also add NOTREACHED() after the switch.
> 
> I think it's better to put NOTREACHED() after the default statement. That way
> rolling out new cases from chromeos repo won't break chrome build. WDYT?
> 
We generally avoid default specifically so that when new enums are added the
code must be updated. That said, it does make a deps roll more complicated since
the enum would need to be fixed at the same time. I suppose using default is
fine then, but I wouldn't use NOTREACHED in that case since it would break Debug
builds, I would use LOG(WARNING) instead.

[stevenjb, 2016-12-28 18:12:43]

https://codereview.chromium.org/2587993002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2587993002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:594:
NOTREACHED();
As mentioned, either make this LOG(WARNING) or remove 'default'. Also, if we we
handle 'default' we should probably still call 'invalidateAd'.

[Roman Sorokin (ftl), 2016-12-28 20:04:22]

On 2016/12/28 18:09:33, stevenjb wrote:
> On 2016/12/28 11:04:09, Roman Sorokin wrote:
> > Hey Steven, PTAL. Thanks!
> > 
> >
>
https://codereview.chromium.org/2587993002/diff/60001/chrome/browser/ui/webui...
> > File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2587993002/diff/60001/chrome/browser/ui/webui...
> > chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:572:
> > default:
> > On 2016/12/27 17:52:26, stevenjb wrote:
> > > We should list the unhandled cases explicitly and return here and not have
a
> > > default, then the compiler will complain if a new unhandled case is added.
> We
> > > can also add NOTREACHED() after the switch.
> > 
> > I think it's better to put NOTREACHED() after the default statement. That
way
> > rolling out new cases from chromeos repo won't break chrome build. WDYT?
> > 
> We generally avoid default specifically so that when new enums are added the
> code must be updated. That said, it does make a deps roll more complicated
since
> the enum would need to be fixed at the same time. I suppose using default is
> fine then, but I wouldn't use NOTREACHED in that case since it would break
Debug
> builds, I would use LOG(WARNING) instead.

NOTREACHED should not break Debug builds. It would fail during the runtime. Am I
wrong?

[stevenjb, 2016-12-28 20:53:04]

On 2016/12/28 20:04:22, Roman Sorokin wrote:
> On 2016/12/28 18:09:33, stevenjb wrote:
> > On 2016/12/28 11:04:09, Roman Sorokin wrote:
> > > Hey Steven, PTAL. Thanks!
> > > 
> > >
> >
>
https://codereview.chromium.org/2587993002/diff/60001/chrome/browser/ui/webui...
> > > File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2587993002/diff/60001/chrome/browser/ui/webui...
> > > chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:572:
> > > default:
> > > On 2016/12/27 17:52:26, stevenjb wrote:
> > > > We should list the unhandled cases explicitly and return here and not
have
> a
> > > > default, then the compiler will complain if a new unhandled case is
added.
> > We
> > > > can also add NOTREACHED() after the switch.
> > > 
> > > I think it's better to put NOTREACHED() after the default statement. That
> way
> > > rolling out new cases from chromeos repo won't break chrome build. WDYT?
> > > 
> > We generally avoid default specifically so that when new enums are added the
> > code must be updated. That said, it does make a deps roll more complicated
> since
> > the enum would need to be fixed at the same time. I suppose using default is
> > fine then, but I wouldn't use NOTREACHED in that case since it would break
> Debug
> > builds, I would use LOG(WARNING) instead.
> 
> NOTREACHED should not break Debug builds. It would fail during the runtime. Am
I
> wrong?

I'm not sure how "should not break Debug builds" != "fail during runtime".

NOTREACHED would break any developer running a Debug build if the new error were
encountered. This may be unlikely since it is an error code, but it is still
better practice to use LOG(WARNING) in this situation. NOTREACHED should only be
used for code that should be logically impossible, e.g. falling through a switch
statement for an enum without a 'default' case.

[Roman Sorokin (ftl), 2016-12-29 07:21:21]

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

[commit-bot: I haz the power, 2016-12-29 07:21:28]

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

[Roman Sorokin (ftl), 2016-12-29 07:21:42]

Hey Steven, PTAL. Thanks!

https://codereview.chromium.org/2587993002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2587993002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:594:
NOTREACHED();
On 2016/12/28 18:12:43, stevenjb wrote:
> As mentioned, either make this LOG(WARNING) or remove 'default'. Also, if we
we
> handle 'default' we should probably still call 'invalidateAd'.
> 

Done.

[commit-bot: I haz the power, 2016-12-29 07:22:57]

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

[commit-bot: I haz the power, 2016-12-29 07:22:58]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Roman Sorokin (ftl), 2016-12-29 09:55:40]

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

[commit-bot: I haz the power, 2016-12-29 09:55:50]

Dry run: 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-29 10:33:24]

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

[commit-bot: I haz the power, 2016-12-29 10:33:25]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2016-12-29 21:09:54]

lgtm w/ comment or default moved to end of switch.

https://codereview.chromium.org/2587993002/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2587993002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:573:
LOG(WARNING) << "Unhandled error code: " << code;
default should still be after any cases. We could add a comment here calling out
that this is falling through to call invalidateAd, but I think duplicating that
code is better, especially since the comment does not apply for 'default'.

[Roman Sorokin (ftl), 2016-12-30 11:25:32]

The CQ bit was checked by rsorokin@chromium.org

[Roman Sorokin (ftl), 2016-12-30 11:25:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
alemate@chromium.org, stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2587993002/#ps160001
(title: "Move default to the end of switch.")

[commit-bot: I haz the power, 2016-12-30 11:25:42]

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

[Roman Sorokin (ftl), 2016-12-30 11:25:58]

https://codereview.chromium.org/2587993002/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2587993002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:573:
LOG(WARNING) << "Unhandled error code: " << code;
On 2016/12/29 21:09:54, stevenjb wrote:
> default should still be after any cases. We could add a comment here calling
out
> that this is falling through to call invalidateAd, but I think duplicating
that
> code is better, especially since the comment does not apply for 'default'.

Done.

[commit-bot: I haz the power, 2016-12-30 11:58:30]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1483097132273290,
"parent_rev": "1558c162f9b0b6f002d261c2226e05359b51c30b", "commit_rev":
"0442c4d30d9a49e975df10310ada75bb162009c9"}

[commit-bot: I haz the power, 2016-12-30 11:59:00]

Description was changed from

==========
Chromad: Switch AuthPolicyClient to use cros_system_api enums.

BUG=659732
TEST=manual
==========

to

==========
Chromad: Switch AuthPolicyClient to use cros_system_api enums.

BUG=659732
TEST=manual

Review-Url: https://codereview.chromium.org/2587993002
==========

[commit-bot: I haz the power, 2016-12-30 11:59:03]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2017-01-02 15:54:24]

Description was changed from

==========
Chromad: Switch AuthPolicyClient to use cros_system_api enums.

BUG=659732
TEST=manual

Review-Url: https://codereview.chromium.org/2587993002
==========

to

==========
Chromad: Switch AuthPolicyClient to use cros_system_api enums.

BUG=659732
TEST=manual

Committed: https://crrev.com/c36703e9325eeb51a1582043e2c536eb2078ad6b
Cr-Commit-Position: refs/heads/master@{#441026}
==========

[commit-bot: I haz the power, 2017-01-02 15:54:25]

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