Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(132)

Issue 2771943003: arc: Skip GMS Sign-In in case Android is already signed-in. (Closed)

Created:
3 years, 9 months ago by khmel
Modified:
3 years, 8 months ago
CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, oshima+watch_chromium.org, yusukes+watch_chromium.org, viettrungluu+watch_chromium.org, hidehiko+watch_chromium.org, yzshen+watch_chromium.org, khmel+watch_chromium.org, asvitkine+watch_chromium.org, Aaron Boodman, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, darin (slow to review), abarth-chromium, davemoore+watch_chromium.org, qsr+mojo_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

arc: Skip GMS Sign-In in case Android is already signed-in. There are users' feedback reports where GMS Sign-In was attempted on already signed account. This operation for re-sign in general is safe. However Play Store may be busy with downloading and initializing. This is the reason of real user failures with GMS Sign-In timeout. We need detect this case and skip GMS Sign-in in case user is already signed-in. This can happen on retry when Android was actually signed in but failed later due another reason. TBR=lhchavez@chromium.org BUG=704982 TEST=Manually various cases and observe result in logcat and chrome://histogramms Review-Url: https://codereview.chromium.org/2771943003 Cr-Commit-Position: refs/heads/master@{#460030} Committed: https://chromium.googlesource.com/chromium/src/+/eb71d90d60b690242af7c4fd623238071ecefba7

Patch Set 1 #

Total comments: 2

Patch Set 2 : comment updated #

Total comments: 14

Patch Set 3 : nits #

Patch Set 4 : nit #

Patch Set 5 : del transient enum #

Total comments: 1

Patch Set 6 : remove SIZE from mojom and use LAST_VALUE + 1 in UMA reporting #

Total comments: 10

Patch Set 7 : nits #

Patch Set 8 : use sparse UMA #

Unified diffs Side-by-side diffs Delta from patch set Stats (+95 lines, -6 lines) Patch
M chrome/browser/chromeos/arc/arc_auth_context.h View 2 chunks +5 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/arc/arc_auth_context.cc View 2 chunks +5 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/arc/arc_auth_service.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/chromeos/arc/arc_auth_service.cc View 1 2 3 4 7 chunks +18 lines, -2 lines 0 comments Download
M chrome/browser/chromeos/arc/arc_optin_uma.h View 1 2 3 4 2 chunks +3 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/arc/arc_optin_uma.cc View 1 2 3 4 5 6 7 1 chunk +9 lines, -2 lines 0 comments Download
M components/arc/common/auth.mojom View 1 2 3 4 5 6 5 chunks +33 lines, -2 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 2 chunks +21 lines, -0 lines 0 comments Download

Messages

Total messages: 49 (15 generated)
khmel
Hi Luis, PTAL, Thanks!
3 years, 9 months ago (2017-03-24 18:06:59 UTC) #3
Luis Héctor Chávez
https://codereview.chromium.org/2771943003/diff/1/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2771943003/diff/1/tools/metrics/histograms/histograms.xml#newcode1651 tools/metrics/histograms/histograms.xml:1651: + not recorded in case account status check failed ...
3 years, 9 months ago (2017-03-24 22:02:32 UTC) #8
khmel
Thank you Luis for comments Also adding isherman@ for UMA dcheng@ for mojo PTAL, Thanks! ...
3 years, 9 months ago (2017-03-24 22:40:22 UTC) #10
Ilya Sherman
Metrics LGTM, thanks. https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos/arc/arc_optin_uma.h File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos/arc/arc_optin_uma.h#newcode122 chrome/browser/chromeos/arc/arc_optin_uma.h:122: enum class AccountCheckStatus : int { ...
3 years, 9 months ago (2017-03-24 23:22:09 UTC) #11
khmel
Thank you Ilya for review! https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos/arc/arc_optin_uma.h File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos/arc/arc_optin_uma.h#newcode122 chrome/browser/chromeos/arc/arc_optin_uma.h:122: enum class AccountCheckStatus : ...
3 years, 9 months ago (2017-03-24 23:51:54 UTC) #12
dcheng
https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos/arc/arc_auth_service.cc File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos/arc/arc_auth_service.cc#newcode226 chrome/browser/chromeos/arc/arc_auth_service.cc:226: NOTREACHED() << "Unknown account check status"; Please typemap this ...
3 years, 9 months ago (2017-03-25 00:55:04 UTC) #13
khmel
Thank you for your comments https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos/arc/arc_auth_service.cc File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos/arc/arc_auth_service.cc#newcode226 chrome/browser/chromeos/arc/arc_auth_service.cc:226: NOTREACHED() << "Unknown account ...
3 years, 9 months ago (2017-03-25 01:24:14 UTC) #14
yzshen1
https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos/arc/arc_optin_uma.h File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos/arc/arc_optin_uma.h#newcode139 chrome/browser/chromeos/arc/arc_optin_uma.h:139: SIZE, On 2017/03/25 00:55:04, dcheng wrote: > Another possibility ...
3 years, 9 months ago (2017-03-27 16:53:05 UTC) #16
Ilya Sherman
https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos/arc/arc_optin_uma.h File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos/arc/arc_optin_uma.h#newcode139 chrome/browser/chromeos/arc/arc_optin_uma.h:139: SIZE, On 2017/03/27 16:53:05, yzshen1 wrote: > On 2017/03/25 ...
3 years, 9 months ago (2017-03-27 17:00:40 UTC) #17
yzshen1
https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos/arc/arc_optin_uma.h File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos/arc/arc_optin_uma.h#newcode139 chrome/browser/chromeos/arc/arc_optin_uma.h:139: SIZE, On 2017/03/27 17:00:40, Ilya Sherman wrote: > On ...
3 years, 9 months ago (2017-03-27 17:07:46 UTC) #18
khmel
https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos/arc/arc_optin_uma.h File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos/arc/arc_optin_uma.h#newcode139 chrome/browser/chromeos/arc/arc_optin_uma.h:139: SIZE, On 2017/03/27 17:07:46, yzshen1 wrote: > On 2017/03/27 ...
3 years, 9 months ago (2017-03-27 17:17:39 UTC) #19
yzshen1
On 2017/03/27 17:17:39, khmel wrote: > https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos/arc/arc_optin_uma.h > File chrome/browser/chromeos/arc/arc_optin_uma.h (right): > > https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos/arc/arc_optin_uma.h#newcode139 > ...
3 years, 9 months ago (2017-03-27 17:43:22 UTC) #20
khmel
On 2017/03/27 17:43:22, yzshen1 wrote: > On 2017/03/27 17:17:39, khmel wrote: > > > https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos/arc/arc_optin_uma.h ...
3 years, 9 months ago (2017-03-27 17:54:56 UTC) #21
yzshen1
On 2017/03/27 17:54:56, khmel wrote: > On 2017/03/27 17:43:22, yzshen1 wrote: > > On 2017/03/27 ...
3 years, 9 months ago (2017-03-27 18:04:51 UTC) #22
khmel
On 2017/03/27 18:04:51, yzshen1 wrote: > On 2017/03/27 17:54:56, khmel wrote: > > On 2017/03/27 ...
3 years, 9 months ago (2017-03-27 18:27:55 UTC) #23
dcheng
LGTM I have a CL in progress to fix the scoped enum issue: https://codereview.chromium.org/2776853002/. I ...
3 years, 9 months ago (2017-03-27 18:31:09 UTC) #24
yzshen1
LGTM for using mojom enum with UMA.
3 years, 9 months ago (2017-03-27 18:32:58 UTC) #25
Ilya Sherman
https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeos/arc/arc_optin_uma.cc File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeos/arc/arc_optin_uma.cc#newcode68 chrome/browser/chromeos/arc/arc_optin_uma.cc:68: DCHECK(status <= mojom::AccountCheckStatus::CHECK_FAILED); nit: DCHECK_LE https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeos/arc/arc_optin_uma.cc#newcode71 chrome/browser/chromeos/arc/arc_optin_uma.cc:71: static_cast<int>(mojom::AccountCheckStatus::CHECK_FAILED) + ...
3 years, 9 months ago (2017-03-27 19:03:28 UTC) #26
khmel
https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeos/arc/arc_optin_uma.cc File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeos/arc/arc_optin_uma.cc#newcode68 chrome/browser/chromeos/arc/arc_optin_uma.cc:68: DCHECK(status <= mojom::AccountCheckStatus::CHECK_FAILED); On 2017/03/27 19:03:28, Ilya Sherman wrote: ...
3 years, 9 months ago (2017-03-27 19:29:29 UTC) #27
Ilya Sherman
https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeos/arc/arc_optin_uma.cc File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeos/arc/arc_optin_uma.cc#newcode71 chrome/browser/chromeos/arc/arc_optin_uma.cc:71: static_cast<int>(mojom::AccountCheckStatus::CHECK_FAILED) + 1); On 2017/03/27 19:29:29, khmel wrote: > ...
3 years, 9 months ago (2017-03-27 20:07:37 UTC) #28
yzshen1
On 2017/03/27 20:07:37, Ilya Sherman wrote: > https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeos/arc/arc_optin_uma.cc > File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): > > https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeos/arc/arc_optin_uma.cc#newcode71 ...
3 years, 9 months ago (2017-03-27 20:10:48 UTC) #29
khmel
https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeos/arc/arc_optin_uma.cc File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeos/arc/arc_optin_uma.cc#newcode71 chrome/browser/chromeos/arc/arc_optin_uma.cc:71: static_cast<int>(mojom::AccountCheckStatus::CHECK_FAILED) + 1); On 2017/03/27 20:07:37, Ilya Sherman wrote: ...
3 years, 9 months ago (2017-03-27 20:35:57 UTC) #30
dcheng
On 2017/03/27 20:35:57, khmel wrote: > https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeos/arc/arc_optin_uma.cc > File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): > > https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeos/arc/arc_optin_uma.cc#newcode71 > ...
3 years, 9 months ago (2017-03-27 20:37:53 UTC) #31
Ilya Sherman
Apologies, Yury, for getting a bit into the weeds on this CL. I do think ...
3 years, 9 months ago (2017-03-27 20:54:29 UTC) #32
dcheng
On 2017/03/27 20:54:29, Ilya Sherman wrote: > Apologies, Yury, for getting a bit into the ...
3 years, 9 months ago (2017-03-27 21:00:50 UTC) #33
khmel
https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeos/arc/arc_optin_uma.cc File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeos/arc/arc_optin_uma.cc#newcode71 chrome/browser/chromeos/arc/arc_optin_uma.cc:71: static_cast<int>(mojom::AccountCheckStatus::CHECK_FAILED) + 1); On 2017/03/27 20:54:29, Ilya Sherman wrote: ...
3 years, 9 months ago (2017-03-27 21:01:41 UTC) #34
khmel
3 years, 9 months ago (2017-03-27 21:01:42 UTC) #35
Ilya Sherman
This CL LGTM with a sparse histogram -- and you can remove the note in ...
3 years, 9 months ago (2017-03-27 21:19:12 UTC) #36
dcheng
On 2017/03/27 21:19:12, Ilya Sherman wrote: > This CL LGTM with a sparse histogram -- ...
3 years, 9 months ago (2017-03-27 21:21:59 UTC) #37
khmel
Thank you all for review! Luis does not have access to the Chromium currently and ...
3 years, 9 months ago (2017-03-27 21:30:34 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2771943003/140001
3 years, 9 months ago (2017-03-27 21:31:48 UTC) #42
commit-bot: I haz the power
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/180885)
3 years, 8 months ago (2017-03-28 02:18:10 UTC) #44
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2771943003/140001
3 years, 8 months ago (2017-03-28 04:06:39 UTC) #46
commit-bot: I haz the power
3 years, 8 months ago (2017-03-28 06:37:23 UTC) #49
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/eb71d90d60b690242af7c4fd6232...

Powered by Google App Engine
This is Rietveld 408576698