Recategorize ARC auth sign in errors.

components/arc/common/auth.mojom

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 module arc.mojom;
 
 // These values describe failure reason of sign-in.
 enum ArcSignInFailureReason {
+  // Negative values are reserved for internal use.

[victorhsieh, 2016/07/25 20:30:42]

What do you think to rename the enum to something like ArcSignInErrorCode, and
add SUCCESS to the enum (which I think is common in Chromium code base)?

[Luis Héctor Chávez, 2016/07/25 21:04:47]

If you are going to drastically change this enum, I'd suggest to use a Mojo
typemap to avoid the conversion in arc_auth_service.cc (Mojo can now use some
C++ types!). There is some support needed in Android-side to make that happen,
but I'm working on that.

[hidehiko, 2016/07/26 13:02:58]

Re: error code.
My understanding is; At the moment, this is an interface to let Chrome know
about ARC sign in failure cause.
If we want to keep the concept, adding SUCCESS to the error code sounds weird to
me.
Also, practically, because we have two APIs, OnSignInComplete() and
OnSignInFailed() separately, SUCCESS would never be sent from ARC.

Of course, changing the API, e.g. merging those two APIs, is an option. However,
I'd like to separate such a big change because this is cherry-pick target.

Re: typemap.
As this is target of cherry-pick, can I postpone to use typemap, too?

[victorhsieh, 2016/07/26 18:22:11]

Agree.  Although practically speaking, no SUCCESS makes it less succinct to
write code, as you have done by adding negative value in the corresponding
change.  But it's not what this change is about.

[hidehiko, 2016/07/26 18:50:48]

Acknowledged.

+  // The values are shuffled to keep the backward compatibility.
+  // Next value: 14.

[victorhsieh, 2016/07/25 20:30:42]

14 -> 15

[hidehiko, 2016/07/26 13:02:58]

Oops. Done.

   UNKNOWN_ERROR = 0,
-  NETWORK_ERROR = 1,
-  SERVICE_UNAVAILABLE = 2,
-  BAD_AUTHENTICATION = 3,
-  GMS_CORE_NOT_AVAILABLE = 4,
-  CLOUD_PROVISION_FLOW_FAIL = 5,
+
+  MOJO_VERSION_MISMATCH = 6,
+  MOJO_CALL_TIMEOUT = 7,
+
+  DEVICE_CHECK_IN_FAILED = 4,
+
+  // GMS errors.

[victorhsieh, 2016/07/25 20:30:42]

For the record, when I first started the review, I thought it'd be better to not
report error in such granularity (plus exposing error from 2 levels below
doesn't sound like healthy, plus they will likely add more error code in the
future).  So I was about to suggest perhaps using a major category with the
original error code as a tuple, and the error string would be like "sign-in
failed (error %d)".

But then I saw we use these values are used in UMA.  If we care to have every
category, I guess we can't avoid this.  I'm not strongly oppose this, but would
like to see if you think there is any alternative that would be (clearly)
better.

[hidehiko, 2016/07/26 13:02:58]

If we'd like to have details in UMA, this looks the most straightforward
approach, and cannot be avoidable, IMHO.

Alternative option;
- Drop minor categories, instead logging in ARC.
  - We cannot take UMA for minor categories, then.
- Split API into a few. E.g. OnGmsFailed(), OnDeviceCheckInFailed(), etc. and
pass some minor error enum to their arguments.
  - IMHO, this does not look to have much benefit, because the implementation
will just remap the enum to flatten UMA stats, and then call shared function.

From the recategorization of UNKNOWN_ERROR perspective, this approach looks more
straightforward, TBH. WDYT?

[victorhsieh, 2016/07/26 18:22:11]

How about at least not expose things that doesn't make any sense on ARC, such as
GMS_NEEDS_*?  I think those should never happen.

[hidehiko, 2016/07/26 18:50:48]

I'm not very familiar with GMS errors in details, so that I extracted from the
signature.
Could you list what should never practically happen in ARC case so that we can
squash the errors into GMS_SIGN_IN_FAILED?

Note that, IMHO we should keep NETWORK_ERROR, SERVICE_UNAVAILABLE,
BAD_AUTHENTICATION at least for now, considering cherry-pick consistency.

[victorhsieh, 2016/07/26 19:52:42]

I think we can remove GMS_NEEDS_* and probably GMS_EMPTY_CONSUMER_PKG_OR_SIG
too.

[Junichi Uekawa, 2016/07/27 08:52:20]

I am not too confident that they don't happen, we don't know.
But if you insist we can keep them to be UNKNOWN.

[hidehiko, 2016/07/27 17:36:12]

Thanks. So I removed GMS_NEEDS_* and GMS_EMPTY_CONSUMER_PKG_OR_SIG. These will
be categorized into GMS_SIGN_IN_FAILED.
Instead, I introduced _INTERNAL_ERRORs, which represent, e.g., some unexpected
Exception in client code, etc, rather than possible/explainable failing
scenarios.

[victorhsieh, 2016/07/27 18:20:00]

Those errors are pretty generic because they are also used by app auth, for
example.  But we are using it here for the very specific case of initial
sign-in, which uses a special auth API.  So I don't expect those cases to
happen.

+  GMS_SIGN_IN_FAILED = 8,
+  GMS_SIGN_IN_TIMEOUT = 9,
+  GMS_NETWORK_ERROR = 1,
+  GMS_SERVICE_UNAVAILABLE = 2,
+  GMS_BAD_AUTHENTICATION = 3,
+  GMS_EMPTY_CONSUMER_PKG_OR_SIG = 10,
+  GMS_NEEDS_2F = 11,
+  GMS_NEEDS_POST_SIGN_IN_FLOW = 12,
+  GMS_NEEDS_BROWSER = 13,
+
+  // Cloud provisioning errors.
+  CLOUD_PROVISION_FLOW_FAILED = 5,
+  CLOUD_PROVISION_FLOW_TIMEOUT = 14,
 };
 
 interface AuthHost {
   // Returns an authorization code, which can be used to sign in.
   GetAuthCodeDeprecated@0() => (string auth_code);
   // Returns an authorization code in case is_enforced is set, which can be used
   // to sign in.
   [MinVersion=1] GetAuthCode@1() => (string auth_code, bool is_enforced);
   // Gets whether the account is managed from Chrome OS.
   [MinVersion=3] GetIsAccountManaged@4() => (bool is_managed);
   // Notifies Chrome that the sign-in is completed successfully.
   [MinVersion=2] OnSignInComplete@2();
   // Notifies Chrome that the sign-in fails to complete and provides failure
   // reason.
   [MinVersion=2] OnSignInFailed@3(ArcSignInFailureReason reason);
 };
 
 interface AuthInstance {
   // Establishes full-duplex communication with the host.
   Init@0(AuthHost host_ptr);
 };