Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2173103002:
Recategorize ARC auth sign in errors. (Closed)
|
Created:
4 years, 5 months ago by hidehiko Modified:
4 years, 4 months ago Reviewers:
elijahtaylor1, Junichi Uekawa, Luis Héctor Chávez, rickyz (no longer on Chrome), khmel, victorhsieh CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, viettrungluu+watch_chromium.org, hidehiko+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, lhchavez+watch_chromium.org, oshima+watch_chromium.org, darin (slow to review), ben+mojo_chromium.org, davemoore+watch_chromium.org, qsr+mojo_chromium.org, Junichi Uekawa Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRecategorize ARC auth sign in errors.
Currently, many errors are categorized to UNKNOWN_ERROR.
With this CL and client modification, we can assign more precise error code for each case.
BUG=630624
TEST=Trybot.
Committed: https://crrev.com/01cc9d9f7b222240d178b6820c6dc8ba5c40070c
Cr-Commit-Position: refs/heads/master@{#408602}
Patch Set 1 #Patch Set 2 : Fix test. #Patch Set 3 : Update error code. #
Total comments: 30
Patch Set 4 : rebase #Patch Set 5 : Address comments. #Patch Set 6 : rebase #
Total comments: 2
Patch Set 7 : Address comment #
Total comments: 2
Patch Set 8 : Address comments #
Total comments: 7
Patch Set 9 : Address comments. #
Total comments: 2
Patch Set 10 : Address comments. #
Total comments: 1
Messages
Total messages: 60 (35 generated)
The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== WIP: Recategorize ARC auth sign in errors. BUG=630624 TEST=Trybot. ========== to ========== Recategorize ARC auth sign in errors. Currently, many errors are categorized to UNKNOWN_ERROR. With this CL and client modification, we can assign more precise error code for each case. BUG=630624 TEST=Trybot. ==========
hidehiko@chromium.org changed reviewers: + elijahtaylor@chromium.org, victorhsieh@chromium.org
Victor, Elijah, PTAL.
https://codereview.chromium.org/2173103002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2173103002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:315: UpdateOptInCancelUMA(OptInCancelReason::CLOUD_PROVISION_FLOW_FAIL); Why it's always CLOUD_PROVISION_FLOW_FAIL now? https://codereview.chromium.org/2173103002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:323: case ProvisioningResult::GMS_SERVICE_UNAVAILABLE: FWIW, this SERVICE_UNAVAILABLE is just an edge case when it failed to contact the server for unexpected reason. I was confused to see this case not returning IDS_..._GMS_NOT_AVIALABLE at first. https://codereview.chromium.org/2173103002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/2173103002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.h:54: // Failed to Check in on the device. nit: checkin https://codereview.chromium.org/2173103002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.h:81: CLOUD_PROVISION_FLOW_TIMEOUT = 15, nit/optional: put this next to CLOUD_PROVISION_FLOW_FAILED? https://codereview.chromium.org/2173103002/diff/40001/components/arc/common/a... File components/arc/common/auth.mojom (right): https://codereview.chromium.org/2173103002/diff/40001/components/arc/common/a... components/arc/common/auth.mojom:9: // Negative values are reserved for internal use. 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)? https://codereview.chromium.org/2173103002/diff/40001/components/arc/common/a... components/arc/common/auth.mojom:11: // Next value: 14. 14 -> 15 https://codereview.chromium.org/2173103002/diff/40001/components/arc/common/a... components/arc/common/auth.mojom:19: // GMS errors. 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.
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
drive-by https://codereview.chromium.org/2173103002/diff/40001/components/arc/common/a... File components/arc/common/auth.mojom (right): https://codereview.chromium.org/2173103002/diff/40001/components/arc/common/a... components/arc/common/auth.mojom:9: // Negative values are reserved for internal use. On 2016/07/25 20:30:42, victorhsieh wrote: > 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)? 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.
uekawa@chromium.org changed reviewers: + uekawa@chromium.org
https://codereview.chromium.org/2173103002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2173103002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:124: return ProvisioningResult::CLOUD_PROVISION_FLOW_TIMEOUT; my eyes hurt trying to compare and find a typo / copy-paste error in the list. We should let the compiler do such a job. Some suggestions: 1. You could create a map and do a map.find() 2. #define MAP_ERROR_CODE(something) that generates case/return pair (or the data).
The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you for review. Because this is cherry-pick target, I'd like to just focus on recategorizing (setting precise category) UMA in this CL. I hope it is straightforward enough. I'm happy to take a look at/discuss the Mojo APIs concept change, or using better utilities/libraries, updating UI/messages etc. Though, I'd like to split the work from this CL. Thank you for understanding in advance. PTAL. Elijah, could you kindly double-check the new UMA is enough at the moment? https://codereview.chromium.org/2173103002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2173103002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:124: return ProvisioningResult::CLOUD_PROVISION_FLOW_TIMEOUT; On 2016/07/26 06:22:27, Junichi Uekawa wrote: > my eyes hurt trying to compare and find a typo / copy-paste error > in the list. We should let the compiler do such a job. > > Some suggestions: > > 1. You could create a map and do a map.find() > 2. #define MAP_ERROR_CODE(something) that generates case/return pair (or the > data). > > Replaced by a macro. https://codereview.chromium.org/2173103002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:315: UpdateOptInCancelUMA(OptInCancelReason::CLOUD_PROVISION_FLOW_FAIL); On 2016/07/25 20:30:41, victorhsieh wrote: > Why it's always CLOUD_PROVISION_FLOW_FAIL now? The previous code is sending dupped error status via OptInCancelUMA and ProvisiningResultUMA. Also, UpdateOptInCancelUMA is called in other places, so the log is mixed. With this, CLOUD_PROVISION_FLOW_FAIL means "sign-in failure", and provisioning UMA stats is something drill-down. I'm not very sure if we should keep using the same value or deprecate it and introduce another value, though. https://codereview.chromium.org/2173103002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:323: case ProvisioningResult::GMS_SERVICE_UNAVAILABLE: On 2016/07/25 20:30:41, victorhsieh wrote: > FWIW, this SERVICE_UNAVAILABLE is just an edge case when it failed to contact > the server for unexpected reason. I was confused to see this case not returning > IDS_..._GMS_NOT_AVIALABLE at first. SERVICE_UNAVAILABLE was used various purpose in the client code, so it was difficult to see what was happened via UMA. Now, GMS_SERVICE_UNAVAILABLE is returned in case that Android GMS services tells us it is unavailable. As for message, changing UI is out of scope in this CL, so let me keep it as is. Let's revisit UI later. https://codereview.chromium.org/2173103002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/2173103002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.h:54: // Failed to Check in on the device. On 2016/07/25 20:30:41, victorhsieh wrote: > nit: checkin Done. https://codereview.chromium.org/2173103002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.h:81: CLOUD_PROVISION_FLOW_TIMEOUT = 15, On 2016/07/25 20:30:42, victorhsieh wrote: > nit/optional: put this next to CLOUD_PROVISION_FLOW_FAILED? Can I keep this as is? 1 - 6 are in the order for backward compatibility. https://codereview.chromium.org/2173103002/diff/40001/components/arc/common/a... File components/arc/common/auth.mojom (right): https://codereview.chromium.org/2173103002/diff/40001/components/arc/common/a... components/arc/common/auth.mojom:9: // Negative values are reserved for internal use. On 2016/07/25 21:04:47, Luis Héctor Chávez wrote: > On 2016/07/25 20:30:42, victorhsieh wrote: > > 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)? > > 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. 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? https://codereview.chromium.org/2173103002/diff/40001/components/arc/common/a... components/arc/common/auth.mojom:11: // Next value: 14. On 2016/07/25 20:30:42, victorhsieh wrote: > 14 -> 15 Oops. Done. https://codereview.chromium.org/2173103002/diff/40001/components/arc/common/a... components/arc/common/auth.mojom:19: // GMS errors. On 2016/07/25 20:30:42, victorhsieh wrote: > 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. 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?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2173103002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/2173103002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.h:81: CLOUD_PROVISION_FLOW_TIMEOUT = 15, On 2016/07/26 13:02:58, hidehiko wrote: > On 2016/07/25 20:30:42, victorhsieh wrote: > > nit/optional: put this next to CLOUD_PROVISION_FLOW_FAILED? > > Can I keep this as is? > 1 - 6 are in the order for backward compatibility. Not sure I understand this. Aren't their value explicitly specificed? Or are they kept in order so that it's easier to understand, and if this is the case, wouldn't a comment be more helpful? https://codereview.chromium.org/2173103002/diff/40001/components/arc/common/a... File components/arc/common/auth.mojom (right): https://codereview.chromium.org/2173103002/diff/40001/components/arc/common/a... components/arc/common/auth.mojom:9: // Negative values are reserved for internal use. > 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. 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. https://codereview.chromium.org/2173103002/diff/40001/components/arc/common/a... components/arc/common/auth.mojom:19: // GMS errors. 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.
https://codereview.chromium.org/2173103002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/2173103002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.h:81: CLOUD_PROVISION_FLOW_TIMEOUT = 15, On 2016/07/26 18:22:11, victorhsieh wrote: > On 2016/07/26 13:02:58, hidehiko wrote: > > On 2016/07/25 20:30:42, victorhsieh wrote: > > > nit/optional: put this next to CLOUD_PROVISION_FLOW_FAILED? > > > > Can I keep this as is? > > 1 - 6 are in the order for backward compatibility. > > Not sure I understand this. Aren't their value explicitly specificed? Or are > they kept in order so that it's easier to understand, and if this is the case, > wouldn't a comment be more helpful? Sorry for my poor explanation. This is an enum designed to put "SIZE" at the end as a coding techinque. So, IMHO shuffling the values (like what I did in .mojom) may look confusing? WDYT? Note that moving just CLOUD_PROVISION_FLOW_TIMEOUT to just after CLOUD_PROVISION_FLOW_FAILED seems inconsistent to me, because other GMS_ errors are kept there. https://codereview.chromium.org/2173103002/diff/40001/components/arc/common/a... File components/arc/common/auth.mojom (right): https://codereview.chromium.org/2173103002/diff/40001/components/arc/common/a... components/arc/common/auth.mojom:9: // Negative values are reserved for internal use. On 2016/07/26 18:22:11, victorhsieh wrote: > > 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. > 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. Acknowledged. https://codereview.chromium.org/2173103002/diff/40001/components/arc/common/a... components/arc/common/auth.mojom:19: // GMS errors. On 2016/07/26 18:22:11, victorhsieh wrote: > 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. 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.
https://codereview.chromium.org/2173103002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/2173103002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.h:81: CLOUD_PROVISION_FLOW_TIMEOUT = 15, On 2016/07/26 18:50:48, hidehiko wrote: > On 2016/07/26 18:22:11, victorhsieh wrote: > > On 2016/07/26 13:02:58, hidehiko wrote: > > > On 2016/07/25 20:30:42, victorhsieh wrote: > > > > nit/optional: put this next to CLOUD_PROVISION_FLOW_FAILED? > > > > > > Can I keep this as is? > > > 1 - 6 are in the order for backward compatibility. > > > > Not sure I understand this. Aren't their value explicitly specificed? Or are > > they kept in order so that it's easier to understand, and if this is the case, > > wouldn't a comment be more helpful? > > Sorry for my poor explanation. > This is an enum designed to put "SIZE" at the end as a coding techinque. So, > IMHO shuffling the values (like what I did in .mojom) may look confusing? WDYT? > > Note that moving just CLOUD_PROVISION_FLOW_TIMEOUT to just after > CLOUD_PROVISION_FLOW_FAILED seems inconsistent to me, because other GMS_ errors > are kept there. The style is probably subjective, especially this is an enum. I still prefer to move CLOUD_PROVISION_FLOW_TIMEOUT, but it's up to you. https://codereview.chromium.org/2173103002/diff/40001/components/arc/common/a... File components/arc/common/auth.mojom (right): https://codereview.chromium.org/2173103002/diff/40001/components/arc/common/a... components/arc/common/auth.mojom:19: // GMS errors. > Could you list what should never practically happen in ARC case so that we can > squash the errors into GMS_SIGN_IN_FAILED? I think we can remove GMS_NEEDS_* and probably GMS_EMPTY_CONSUMER_PKG_OR_SIG too.
https://codereview.chromium.org/2173103002/diff/40001/components/arc/common/a... File components/arc/common/auth.mojom (right): https://codereview.chromium.org/2173103002/diff/40001/components/arc/common/a... components/arc/common/auth.mojom:19: // GMS errors. On 2016/07/26 19:52:42, victorhsieh wrote: > > Could you list what should never practically happen in ARC case so that we can > > squash the errors into GMS_SIGN_IN_FAILED? > > I think we can remove GMS_NEEDS_* and probably GMS_EMPTY_CONSUMER_PKG_OR_SIG > too. 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. https://codereview.chromium.org/2173103002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2173103002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:348: ShowUI(UIPage::ERROR_WITH_FEEDBACK, huh, so this is the only time we show error with feedback and wait, and other cases we don't let user send feedback report with logcat info? (i. e. GMS errors)
The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #7 (id:120001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you for review. PTAL. Also, could you take a look at ARC side CL, too, which is also updated along with Mojo change? Thanks! - hidehiko https://codereview.chromium.org/2173103002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/2173103002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.h:81: CLOUD_PROVISION_FLOW_TIMEOUT = 15, On 2016/07/26 19:52:42, victorhsieh wrote: > On 2016/07/26 18:50:48, hidehiko wrote: > > On 2016/07/26 18:22:11, victorhsieh wrote: > > > On 2016/07/26 13:02:58, hidehiko wrote: > > > > On 2016/07/25 20:30:42, victorhsieh wrote: > > > > > nit/optional: put this next to CLOUD_PROVISION_FLOW_FAILED? > > > > > > > > Can I keep this as is? > > > > 1 - 6 are in the order for backward compatibility. > > > > > > Not sure I understand this. Aren't their value explicitly specificed? Or > are > > > they kept in order so that it's easier to understand, and if this is the > case, > > > wouldn't a comment be more helpful? > > > > Sorry for my poor explanation. > > This is an enum designed to put "SIZE" at the end as a coding techinque. So, > > IMHO shuffling the values (like what I did in .mojom) may look confusing? > WDYT? > > > > Note that moving just CLOUD_PROVISION_FLOW_TIMEOUT to just after > > CLOUD_PROVISION_FLOW_FAILED seems inconsistent to me, because other GMS_ > errors > > are kept there. > > The style is probably subjective, especially this is an enum. I still prefer to > move CLOUD_PROVISION_FLOW_TIMEOUT, but it's up to you. Looks technical stuff, because "putting SIZE at last" requires that the values is beginning from 0, and consecutively ascending order. Anyway, let me keep this as is, now. I'm happy to change the code later, but I'd like to focus on "adding enums" in this CL. Added short comment instead. https://codereview.chromium.org/2173103002/diff/40001/components/arc/common/a... File components/arc/common/auth.mojom (right): https://codereview.chromium.org/2173103002/diff/40001/components/arc/common/a... components/arc/common/auth.mojom:19: // GMS errors. On 2016/07/27 08:52:20, Junichi Uekawa wrote: > On 2016/07/26 19:52:42, victorhsieh wrote: > > > Could you list what should never practically happen in ARC case so that we > can > > > squash the errors into GMS_SIGN_IN_FAILED? > > > > I think we can remove GMS_NEEDS_* and probably GMS_EMPTY_CONSUMER_PKG_OR_SIG > > too. > > 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. 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. https://codereview.chromium.org/2173103002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2173103002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:348: ShowUI(UIPage::ERROR_WITH_FEEDBACK, On 2016/07/27 08:52:20, Junichi Uekawa wrote: > huh, so this is the only time we show error with feedback and wait, and other > cases we don't let user send feedback report with logcat info? (i. e. GMS > errors) Could you please start a new thread about this topic, instead of here? I'd like to avoid starting discussion about a non-focus topic inside the code review thread.
lgtm Thanks for the patient! https://codereview.chromium.org/2173103002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/2173103002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.h:81: CLOUD_PROVISION_FLOW_TIMEOUT = 15, On 2016/07/27 17:36:12, hidehiko wrote: > On 2016/07/26 19:52:42, victorhsieh wrote: > > On 2016/07/26 18:50:48, hidehiko wrote: > > > On 2016/07/26 18:22:11, victorhsieh wrote: > > > > On 2016/07/26 13:02:58, hidehiko wrote: > > > > > On 2016/07/25 20:30:42, victorhsieh wrote: > > > > > > nit/optional: put this next to CLOUD_PROVISION_FLOW_FAILED? > > > > > > > > > > Can I keep this as is? > > > > > 1 - 6 are in the order for backward compatibility. > > > > > > > > Not sure I understand this. Aren't their value explicitly specificed? Or > > are > > > > they kept in order so that it's easier to understand, and if this is the > > case, > > > > wouldn't a comment be more helpful? > > > > > > Sorry for my poor explanation. > > > This is an enum designed to put "SIZE" at the end as a coding techinque. So, > > > IMHO shuffling the values (like what I did in .mojom) may look confusing? > > WDYT? > > > > > > Note that moving just CLOUD_PROVISION_FLOW_TIMEOUT to just after > > > CLOUD_PROVISION_FLOW_FAILED seems inconsistent to me, because other GMS_ > > errors > > > are kept there. > > > > The style is probably subjective, especially this is an enum. I still prefer > to > > move CLOUD_PROVISION_FLOW_TIMEOUT, but it's up to you. > > Looks technical stuff, because "putting SIZE at last" requires that the values > is beginning from 0, and consecutively ascending order. > > Anyway, let me keep this as is, now. I'm happy to change the code later, but I'd > like to focus on "adding enums" in this CL. Added short comment instead. I thought SIZE's value is derived from the previous assigned value, in this case 16 + 1? But anyway. https://codereview.chromium.org/2173103002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2173103002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:98: return ProvisioningResult::name nit: add a newline after the macro
https://codereview.chromium.org/2173103002/diff/40001/components/arc/common/a... File components/arc/common/auth.mojom (right): https://codereview.chromium.org/2173103002/diff/40001/components/arc/common/a... components/arc/common/auth.mojom:19: // GMS errors. > 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. 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.
lgtm
hidehiko@chromium.org changed reviewers: + rickyz@chromium.org
+R=rickyz@. Thank you all for your review. Ricky, could you review .mojom file as an OWNER? Thanks, - hidehiko https://codereview.chromium.org/2173103002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2173103002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:98: return ProvisioningResult::name On 2016/07/27 18:03:05, victorhsieh wrote: > nit: add a newline after the macro Done.
One important thing I missed the first round. https://codereview.chromium.org/2173103002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2173103002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:116: default: All Chromium code is compiled with -Wall (which implies -Wswitch), so if you remove the default here, missing a case will be a compile error rather than a runtime error. https://codereview.chromium.org/2173103002/diff/160001/components/arc/common/... File components/arc/common/auth.mojom (right): https://codereview.chromium.org/2173103002/diff/160001/components/arc/common/... components/arc/common/auth.mojom:8: enum ArcSignInFailureReason { This is going to fail validation (and thus cause a connection closure) for the brief period of time Android is newer than Chrome. We might need to do this in two steps, first adding the [Extensible] attribute to the enum and once both sides are in sync, we actually introduce this change. Alternatively, we can bump the host version and use that bump to detect if we can send anything with value > 5 from Android if you don't want to do this in two steps.
Thank you for comment, Luis. # Unfortunately, I do not have access to my dev env now, so please let me just reply, in order to have more iteration. # I'll address your comments, when I get back to the dev env, tomorrow in my TZ. Thanks! - hidehiko https://codereview.chromium.org/2173103002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2173103002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:116: default: On 2016/07/28 17:16:40, Luis Héctor Chávez wrote: > All Chromium code is compiled with -Wall (which implies -Wswitch), so if you > remove the default here, missing a case will be a compile error rather than a > runtime error. SG. Will do. https://codereview.chromium.org/2173103002/diff/160001/components/arc/common/... File components/arc/common/auth.mojom (right): https://codereview.chromium.org/2173103002/diff/160001/components/arc/common/... components/arc/common/auth.mojom:8: enum ArcSignInFailureReason { On 2016/07/28 17:16:40, Luis Héctor Chávez wrote: > This is going to fail validation (and thus cause a connection closure) for the > brief period of time Android is newer than Chrome. We might need to do this in > two steps, first adding the [Extensible] attribute to the enum and once both > sides are in sync, we actually introduce this change. > > Alternatively, we can bump the host version and use that bump to detect if we > can send anything with value > 5 from Android if you don't want to do this in > two steps. Thank you for pointing this out. How about adding [Extensible] in this CL? I was planning to wait for this roll, then to land the ARC side change, to avoid the error. Using [Extensible] makes more sense to me, but I think we do not need to split CLs in this case. Indeed, conceptually those are independent, but IMHO it could be acceptable size. Also, anyway, we need to wait for the roll, even with [Extensible]. WDYT?
https://codereview.chromium.org/2173103002/diff/160001/components/arc/common/... File components/arc/common/auth.mojom (right): https://codereview.chromium.org/2173103002/diff/160001/components/arc/common/... components/arc/common/auth.mojom:8: enum ArcSignInFailureReason { On 2016/07/28 17:41:37, hidehiko wrote: > On 2016/07/28 17:16:40, Luis Héctor Chávez wrote: > > This is going to fail validation (and thus cause a connection closure) for the > > brief period of time Android is newer than Chrome. We might need to do this in > > two steps, first adding the [Extensible] attribute to the enum and once both > > sides are in sync, we actually introduce this change. > > > > Alternatively, we can bump the host version and use that bump to detect if we > > can send anything with value > 5 from Android if you don't want to do this in > > two steps. > > Thank you for pointing this out. > How about adding [Extensible] in this CL? That should be done regardless. > > I was planning to wait for this roll, then to land the ARC side change, to avoid > the error. > Using [Extensible] makes more sense to me, but I think we do not need to split > CLs in this case. > Indeed, conceptually those are independent, but IMHO it could be acceptable > size. Also, anyway, we need to wait for the roll, even with [Extensible]. > > WDYT? Yeah, any way of ensuring that there are no Chrome OS builds with newer Android than Chrome works :)
mojom lgtm
The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL. https://codereview.chromium.org/2173103002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2173103002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:116: default: On 2016/07/28 17:41:37, hidehiko wrote: > On 2016/07/28 17:16:40, Luis Héctor Chávez wrote: > > All Chromium code is compiled with -Wall (which implies -Wswitch), so if you > > remove the default here, missing a case will be a compile error rather than a > > runtime error. > > SG. Will do. Done. https://codereview.chromium.org/2173103002/diff/160001/components/arc/common/... File components/arc/common/auth.mojom (right): https://codereview.chromium.org/2173103002/diff/160001/components/arc/common/... components/arc/common/auth.mojom:8: enum ArcSignInFailureReason { On 2016/07/28 17:45:41, Luis Héctor Chávez wrote: > On 2016/07/28 17:41:37, hidehiko wrote: > > On 2016/07/28 17:16:40, Luis Héctor Chávez wrote: > > > This is going to fail validation (and thus cause a connection closure) for > the > > > brief period of time Android is newer than Chrome. We might need to do this > in > > > two steps, first adding the [Extensible] attribute to the enum and once both > > > sides are in sync, we actually introduce this change. > > > > > > Alternatively, we can bump the host version and use that bump to detect if > we > > > can send anything with value > 5 from Android if you don't want to do this > in > > > two steps. > > > > Thank you for pointing this out. > > How about adding [Extensible] in this CL? > > That should be done regardless. > > > > > I was planning to wait for this roll, then to land the ARC side change, to > avoid > > the error. > > Using [Extensible] makes more sense to me, but I think we do not need to split > > CLs in this case. > > Indeed, conceptually those are independent, but IMHO it could be acceptable > > size. Also, anyway, we need to wait for the roll, even with [Extensible]. > > > > WDYT? > > Yeah, any way of ensuring that there are no Chrome OS builds with newer Android > than Chrome works :) Done.
lgtm with a nit https://codereview.chromium.org/2173103002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2173103002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:119: LOG(ERROR) << "unknown reason: " << static_cast<int>(reason); nit: s/LOG(ERROR)/NOTREACHED()/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you for review. Submitting. https://codereview.chromium.org/2173103002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2173103002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:119: LOG(ERROR) << "unknown reason: " << static_cast<int>(reason); On 2016/07/29 04:28:37, Luis Héctor Chávez wrote: > nit: s/LOG(ERROR)/NOTREACHED()/ Good point, Done.
The CQ bit was checked by hidehiko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from victorhsieh@chromium.org, uekawa@chromium.org, rickyz@chromium.org, lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2173103002/#ps200001 (title: "Address comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Recategorize ARC auth sign in errors. Currently, many errors are categorized to UNKNOWN_ERROR. With this CL and client modification, we can assign more precise error code for each case. BUG=630624 TEST=Trybot. ========== to ========== Recategorize ARC auth sign in errors. Currently, many errors are categorized to UNKNOWN_ERROR. With this CL and client modification, we can assign more precise error code for each case. BUG=630624 TEST=Trybot. ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Recategorize ARC auth sign in errors. Currently, many errors are categorized to UNKNOWN_ERROR. With this CL and client modification, we can assign more precise error code for each case. BUG=630624 TEST=Trybot. ========== to ========== Recategorize ARC auth sign in errors. Currently, many errors are categorized to UNKNOWN_ERROR. With this CL and client modification, we can assign more precise error code for each case. BUG=630624 TEST=Trybot. Committed: https://crrev.com/01cc9d9f7b222240d178b6820c6dc8ba5c40070c Cr-Commit-Position: refs/heads/master@{#408602} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/01cc9d9f7b222240d178b6820c6dc8ba5c40070c Cr-Commit-Position: refs/heads/master@{#408602}
Message was sent while issue was closed.
khmel@chromium.org changed reviewers: + khmel@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2173103002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_optin_uma.h (left): https://codereview.chromium.org/2173103002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_optin_uma.h:39: SUCCESS = 0, // Provisioning was successful. Would not we update tools/metrics/histograms/histograms.xml? <enum name="ArcProvisioningResult" type="int"> <summary>Defines Arc Provisioning success and failure reasons</summary> <int value="0" label="Success"/> <int value="1" label="Unclassified failure"/> <int value="2" label="Network failure"/> <int value="3" label="GMS Services are not available"/> <int value="4" label="Bad authentication returned by server"/> <int value="5" label="GMS Core is not available"/> <int value="6" label="Cloud provision flow failed"/> </enum> |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
