Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2485233002:
Do not run Android management check for re-auth case. (Closed)
|
Created:
4 years, 1 month ago by hidehiko Modified:
4 years, 1 month ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, alemate+watch_chromium.org, sadrul, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, khmel+watch_chromium.org, lhchavez+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, kalyank, davemoore+watch_chromium.org, Polina Bondarenko, khmel Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not run Android management check for re-auth case.
This CL removes the Android management check from re-auth
code flow and OOBE flow. Along with the change, onRetry
implementation is moved from UI code to native code.
BUG=657687
BUG=b/31079732
BUG=b/32431329
TEST=Ran on test device. Ran bots.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/7f74db3b9daf5ea99df3a30d272c80271750e89a
Cr-Commit-Position: refs/heads/master@{#432128}
Patch Set 1 #
Total comments: 26
Patch Set 2 : rebase #Patch Set 3 : Address comments. #
Total comments: 26
Patch Set 4 : address comments #Patch Set 5 : rebase #
Messages
Total messages: 29 (14 generated)
Description was changed from ========== Do not run Android management check for re-auth case. This CL removes the Android management check from re-auth code flow and OOBE flow. Along with the change, onRetry implementation is moved from UI code to native code. BUG=657687 BUG=b/31079732 BUG=b/32431329 TEST=Ran on test device. Ran bots. ========== to ========== Do not run Android management check for re-auth case. This CL removes the Android management check from re-auth code flow and OOBE flow. Along with the change, onRetry implementation is moved from UI code to native code. BUG=657687 BUG=b/31079732 BUG=b/32431329 TEST=Ran on test device. Ran bots. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run
hidehiko@chromium.org changed reviewers: + lhchavez@chromium.org
PTAL.
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.
Added some note for making review easier. Also note that I changed the first auth flow as following: Old: StartLso -> PrepareContextForAuthCodeRequest() -> OnContextReady() -> Android management check -> OnAndroidManagementChecked() -> OnAndroidManagementPassed() -> FetchAuth() -> (UI of sign-in) -> SetAuthCodeAndStartArc(). New: StartAndroidManagementCheck() -> OnAndroidManagementChecked() -> PrepareContextForAuthCodeRequest() -> OnContextReady() -> FetchAuthCode() -> (UI) -> SetAuthCodeAndStartArc(). Note that, for re-auth flow, we start the new flow from PrepareContextForAuthCodeRequest(). https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_service.cc (left): https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:797: DCHECK_EQ(state_, State::FETCHING_CODE); here, state_ should be State::ACTIVE in most cases, but may be not (race issue... It needs to be addressed later.). https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:832: profile_->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, true); Note: Moved to OnTermsAccepted(). Actually, this is not a part of Lso starting. https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:1016: if (state_ == State::ACTIVE) { Because AndroidManagement check is no longer running for re-auth flow, so this is gone. https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:1022: if (profile_->GetPrefs()->GetBoolean(prefs::kArcSignedIn) || This is migrated into OnAndroidManagementChecked(). https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_service.h (left): https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.h:229: void StartAndroidManagementClient(); Note: No implemention. No usage...
hidehiko@chromium.org changed reviewers: + skuhne@chromium.org, xiyuan@chromium.org
R+=xiyuan@ for c/b/r/c/arc_support, skuhne@ for c/b/u/a/launcher. Thank you for review in advance, - hidehiko
Please look at comments. After that c/b/u/a/l lgtm https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.h:65: TERMS, // Showing "Terms Of Service" to user. Could TERMS be replaced with something more expressive like "SHOW_TOS" - or if length doesn't matter "SHOW_TERMS_OF_SERVICE"? https://codereview.chromium.org/2485233002/diff/1/chrome/browser/ui/ash/launc... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc (right): https://codereview.chromium.org/2485233002/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:3598: ValidateArcState(true, true, arc::ArcAuthService::State::TERMS, Could you add a comment here towards why there is the terms of service shown to the user?
https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:822: SetState(State::ANDROID_MANAGEMENT_CHECK); Why ANDROID_MANAGEMENT_CHECK instead of FETCHING_CODE?
https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:398: void ArcAuthService::PrepareContextForAuthCodeRequest() { Is it possible to split this method so we have tighter guarantees about what state is it when this is called? That way we can eliminate the pattern of SetState(...); PrepareContextforAuthCodeRequest(); and instead try to enforce the pattern SetState(...); PerformAsyncOperation(); // or reach a steady state. https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:938: SetState(State::ANDROID_MANAGEMENT_CHECK); Is it possible to have a DCHECK_EQ(state_, ...) every time SetState is called? That way the transitions are more explicit. https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:1103: default: nit: remove the default: case and move the NOTREACHED(); return os; outside the switch so the compiler yells at us if this is not exhaustive. https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.h:62: enum class State { Can you add a state transition diagram here similar to what we have in ABS? https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.h:65: TERMS, // Showing "Terms Of Service" to user. Also, let's try to use a convention of using present participle for async operations (-ing suffix, AuthService is currently XXX) and past participle (-ed suffix)/adjective (AuthService is XXX) for steady states. so can you please change the state names to SHOWING_TERMS_OF_SERVICE and CHECKING_ANDROID_MANAGEMENT? https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.h:66: ANDROID_MANAGEMENT_CHECK, // Checkng Android management status. nit: s/Checkng/Checking/
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. PTAL. https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:398: void ArcAuthService::PrepareContextForAuthCodeRequest() { On 2016/11/10 23:30:58, Luis Héctor Chávez wrote: > Is it possible to split this method so we have tighter guarantees about what > state is it when this is called? That way we can eliminate the pattern of > > SetState(...); > PrepareContextforAuthCodeRequest(); > > and instead try to enforce the pattern > > SetState(...); > PerformAsyncOperation(); // or reach a steady state. Hmm... Maybe I do not understand what you mean. Do you mean "please define different functions for auth-token prefetching and re-auth flow", so that SetState(...) can be forced to be called always? Then, can we skip it? I know this is NOT a good code. But; - The prefetching code will be migrated into re-auth in a following CL. - This is existing code, so it is not the regression. - Splitting the code path into two looks almost just duplicating the code. WDYT? If I'm wrong, please fix me. https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:822: SetState(State::ANDROID_MANAGEMENT_CHECK); On 2016/11/10 17:55:21, xiyuan wrote: > Why ANDROID_MANAGEMENT_CHECK instead of FETCHING_CODE? Sorry this was my mistake. We no longer need StartLso(). Removed. https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:938: SetState(State::ANDROID_MANAGEMENT_CHECK); On 2016/11/10 23:30:58, Luis Héctor Chávez wrote: > Is it possible to have a DCHECK_EQ(state_, ...) every time SetState is called? > That way the transitions are more explicit. WDY mean? SetState(...) is just "state_ = ...". So; SetState(CHECKING_ANDROID_MANAGEMENT); DCHECK_EQ(CHECKING_ANDROID_MANAGEMENT, state_); looks redundant to me? Note: let me keep SetState() for now. It will be gone when we introduce ArcSessionManager. https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:1103: default: On 2016/11/10 23:30:58, Luis Héctor Chávez wrote: > nit: remove the default: case and move the NOTREACHED(); return os; outside the > switch so the compiler yells at us if this is not exhaustive. Addressed in a separate CL. https://codereview.chromium.org/2495273002/ https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.h:62: enum class State { On 2016/11/10 23:30:58, Luis Héctor Chávez wrote: > Can you add a state transition diagram here similar to what we have in ABS? Done. https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.h:65: TERMS, // Showing "Terms Of Service" to user. On 2016/11/10 23:30:58, Luis Héctor Chávez wrote: > Also, let's try to use a convention of using present participle for async > operations (-ing suffix, AuthService is currently XXX) and past participle (-ed > suffix)/adjective (AuthService is XXX) for steady states. > > so can you please change the state names to SHOWING_TERMS_OF_SERVICE and > CHECKING_ANDROID_MANAGEMENT? Done. https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.h:66: ANDROID_MANAGEMENT_CHECK, // Checkng Android management status. On 2016/11/10 23:30:58, Luis Héctor Chávez wrote: > nit: s/Checkng/Checking/ Done. https://codereview.chromium.org/2485233002/diff/1/chrome/browser/ui/ash/launc... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc (right): https://codereview.chromium.org/2485233002/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:3598: ValidateArcState(true, true, arc::ArcAuthService::State::TERMS, On 2016/11/10 16:12:31, Mr4D wrote: > Could you add a comment here towards why there is the terms of service shown to > the user? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:398: void ArcAuthService::PrepareContextForAuthCodeRequest() { On 2016/11/14 10:55:09, hidehiko wrote: > On 2016/11/10 23:30:58, Luis Héctor Chávez wrote: > > Is it possible to split this method so we have tighter guarantees about what > > state is it when this is called? That way we can eliminate the pattern of > > > > SetState(...); > > PrepareContextforAuthCodeRequest(); > > > > and instead try to enforce the pattern > > > > SetState(...); > > PerformAsyncOperation(); // or reach a steady state. > > Hmm... Maybe I do not understand what you mean. > > Do you mean "please define different functions for auth-token prefetching and > re-auth flow", so that SetState(...) can be forced to be called always? > > Then, can we skip it? I know this is NOT a good code. But; > - The prefetching code will be migrated into re-auth in a following CL. > - This is existing code, so it is not the regression. > - Splitting the code path into two looks almost just duplicating the code. > > WDYT? > > If I'm wrong, please fix me. If this will be addressed in a following CL, then I'm fine with it. The goal should be to have more clear and understandable transitions between states. https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:938: SetState(State::ANDROID_MANAGEMENT_CHECK); On 2016/11/14 10:55:09, hidehiko wrote: > On 2016/11/10 23:30:58, Luis Héctor Chávez wrote: > > Is it possible to have a DCHECK_EQ(state_, ...) every time SetState is called? > > That way the transitions are more explicit. > > WDY mean? > SetState(...) is just "state_ = ...". So; > > SetState(CHECKING_ANDROID_MANAGEMENT); > DCHECK_EQ(CHECKING_ANDROID_MANAGEMENT, state_); > > looks redundant to me? > > Note: let me keep SetState() for now. It will be gone when we introduce > ArcSessionManager. Sorry, I meant before assigning. In other words, making sure the transition is happening from an expected state (if possible avoid being in an expected set of states to simplify logic and make it more understandable). https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:379: RequestAccountInfoInternal( What states can this call be made in? ACTIVE and CHECKING_ANDROID_MANAGEMENT? https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:453: DCHECK(state_ != State::ACTIVE || IsAuthCodeRequest()); IIUC this can be DCHECK_(state_ == State::FETCHING_CODE || IsAuthCodeRequest()); https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:604: SetState(State::STOPPED); DCHECK_EQ(State::NOT_INITIALIZED, state_); https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:753: SetState(State::FETCHING_CODE); DCHECK_EQ(State::CHECKING_ANDROID_MANAGEMENT, state_); https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:840: SetState(State::ACTIVE); Can we check that state is either FETCHING_CODE or STOPPED? https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:966: SetState(State::SHOWING_TERMS_OF_SERVICE); DCHECK_EQ(State::STOPPED, state_); https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:995: SetState(State::CHECKING_ANDROID_MANAGEMENT); DCHECK_EQ(State::SHOWING_TERMS_OF_SERVICE, state_); Since you can also call this function while it is already in the CHECKING_ANDROID_MANAGEMENT state, maybe you can move this DCHECK_EQ/SetState combo to L1091? https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:1118: case State::ACTIVE: If State::ACTIVE, you also need to check if there is an ongoing request for an auth code. https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.h:66: // down so that this is destructed. nit: s/destructed/destroyed/ https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.h:82: // State transition should be as follws: nit: s/follws/follows/ https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.h:89: // STOPPED -> SHOWINIG_TERMS_OF_SERVICE: when arc.enabled preference is set. nit: s/SHOWINIG/SHOWING/
Thank you for review. PTAL. Note: I'd like to focus on refactoring of CHECKING_ANDROID_MANAGEMENT case, and to minimize working on state machine. The reason is just because the existing state machine looks too complicated and unmanageable at the moment. However, reworking on it needs some more refactoring like this CL does. I believe I understand your thought, and also I'm on the same page. However, technically reworking on the statemachine just now sounds much difficult and ineffective. As for DCHECK, I added some which look straightforward and have no-problematic edge cases. I hope we can agree at some point. https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:398: void ArcAuthService::PrepareContextForAuthCodeRequest() { On 2016/11/14 17:06:50, Luis Héctor Chávez wrote: > On 2016/11/14 10:55:09, hidehiko wrote: > > On 2016/11/10 23:30:58, Luis Héctor Chávez wrote: > > > Is it possible to split this method so we have tighter guarantees about what > > > state is it when this is called? That way we can eliminate the pattern of > > > > > > SetState(...); > > > PrepareContextforAuthCodeRequest(); > > > > > > and instead try to enforce the pattern > > > > > > SetState(...); > > > PerformAsyncOperation(); // or reach a steady state. > > > > Hmm... Maybe I do not understand what you mean. > > > > Do you mean "please define different functions for auth-token prefetching and > > re-auth flow", so that SetState(...) can be forced to be called always? > > > > Then, can we skip it? I know this is NOT a good code. But; > > - The prefetching code will be migrated into re-auth in a following CL. > > - This is existing code, so it is not the regression. > > - Splitting the code path into two looks almost just duplicating the code. > > > > WDYT? > > > > If I'm wrong, please fix me. > > If this will be addressed in a following CL, then I'm fine with it. The goal > should be to have more clear and understandable transitions between states. I will. However, TBH, I do not think it is reasonable to address the problem in this CL due to its own current complexity, unfortunately. I'll keep in my mind about your comment, and will make it happen later. https://codereview.chromium.org/2485233002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_service.cc:938: SetState(State::ANDROID_MANAGEMENT_CHECK); On 2016/11/14 17:06:50, Luis Héctor Chávez wrote: > On 2016/11/14 10:55:09, hidehiko wrote: > > On 2016/11/10 23:30:58, Luis Héctor Chávez wrote: > > > Is it possible to have a DCHECK_EQ(state_, ...) every time SetState is > called? > > > That way the transitions are more explicit. > > > > WDY mean? > > SetState(...) is just "state_ = ...". So; > > > > SetState(CHECKING_ANDROID_MANAGEMENT); > > DCHECK_EQ(CHECKING_ANDROID_MANAGEMENT, state_); > > > > looks redundant to me? > > > > Note: let me keep SetState() for now. It will be gone when we introduce > > ArcSessionManager. > > Sorry, I meant before assigning. In other words, making sure the transition is > happening from an expected state (if possible avoid being in an expected set of > states to simplify logic and make it more understandable). I got your point. However, currently, the state transition is un-manageable, IMHO... Many methods are called without taking care of its state already. Even just adding DCHECK is sometimes difficult or more complex than simple thought, and it looks not practical to me at the moment. In following CLs, I'll give it a try to have clearer state machine... https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:379: RequestAccountInfoInternal( On 2016/11/14 17:06:50, Luis Héctor Chávez wrote: > What states can this call be made in? ACTIVE and CHECKING_ANDROID_MANAGEMENT? Practically, ACTIVE. However, some browser tests calls AuthHost implementation under non-ACTIVE state. Considering that extracting AuthHost implementation will naturally resolve the issue, can we leave it, for now? https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:453: DCHECK(state_ != State::ACTIVE || IsAuthCodeRequest()); On 2016/11/14 17:06:50, Luis Héctor Chávez wrote: > IIUC this can be DCHECK_(state_ == State::FETCHING_CODE || IsAuthCodeRequest()); Added slightly more straight forward checking. How about this? https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:604: SetState(State::STOPPED); On 2016/11/14 17:06:50, Luis Héctor Chávez wrote: > DCHECK_EQ(State::NOT_INITIALIZED, state_); Done. https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:753: SetState(State::FETCHING_CODE); On 2016/11/14 17:06:50, Luis Héctor Chávez wrote: > DCHECK_EQ(State::CHECKING_ANDROID_MANAGEMENT, state_); Practically, we expect STOPPED here. However, state_ can be any value except ACTIVE or SHOWING_TERMS_OF_SERVICE, because OnOptInPreferenceChanged() can be called at anytime... Let's address this in ArcSessionManager CL in more precise. https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:840: SetState(State::ACTIVE); On 2016/11/14 17:06:50, Luis Héctor Chávez wrote: > Can we check that state is either FETCHING_CODE or STOPPED? Unfortunately, StartArc() can be called in other state_, e.g., CHECKING_ANDROID_MANAGEMENT (in IsOptInVerificationDisabled() == true case). Can we leave it at the moment? https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:966: SetState(State::SHOWING_TERMS_OF_SERVICE); On 2016/11/14 17:06:50, Luis Héctor Chávez wrote: > DCHECK_EQ(State::STOPPED, state_); Practically, we expect STOPPED, but technically this can be in other values, e.g. CHECKING_ANDROID_MANAGEMENT or FETCHING_CODE. (kind of race issue...) Can we address this later? https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:995: SetState(State::CHECKING_ANDROID_MANAGEMENT); On 2016/11/14 17:06:50, Luis Héctor Chávez wrote: > DCHECK_EQ(State::SHOWING_TERMS_OF_SERVICE, state_); > > Since you can also call this function while it is already in the > CHECKING_ANDROID_MANAGEMENT state, maybe you can move this DCHECK_EQ/SetState > combo to L1091? Added DCHECK. I know the state machine handling in this class is complicated. However, it is too complicated to handle it within only this CL, and I'd like to focus on fixing check-android-management phase in this CL. So, can we revisit here in a following CL? https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:1118: case State::ACTIVE: On 2016/11/14 17:06:50, Luis Héctor Chávez wrote: > If State::ACTIVE, you also need to check if there is an ongoing request for an > auth code. Done in PrepareContextForAuthCodeRequest(). https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.h:66: // down so that this is destructed. On 2016/11/14 17:06:50, Luis Héctor Chávez wrote: > nit: s/destructed/destroyed/ Done. https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.h:82: // State transition should be as follws: On 2016/11/14 17:06:50, Luis Héctor Chávez wrote: > nit: s/follws/follows/ Done. https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.h:89: // STOPPED -> SHOWINIG_TERMS_OF_SERVICE: when arc.enabled preference is set. On 2016/11/14 17:06:50, Luis Héctor Chávez wrote: > nit: s/SHOWINIG/SHOWING/ Done.
lgtm https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:379: RequestAccountInfoInternal( On 2016/11/14 19:07:57, hidehiko wrote: > On 2016/11/14 17:06:50, Luis Héctor Chávez wrote: > > What states can this call be made in? ACTIVE and CHECKING_ANDROID_MANAGEMENT? > > Practically, ACTIVE. > However, some browser tests calls AuthHost implementation under non-ACTIVE > state. Considering that extracting AuthHost implementation will naturally > resolve the issue, can we leave it, for now? sgtm https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:453: DCHECK(state_ != State::ACTIVE || IsAuthCodeRequest()); On 2016/11/14 19:07:57, hidehiko wrote: > On 2016/11/14 17:06:50, Luis Héctor Chávez wrote: > > IIUC this can be DCHECK_(state_ == State::FETCHING_CODE || > IsAuthCodeRequest()); > > Added slightly more straight forward checking. How about this? sgtm https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:840: SetState(State::ACTIVE); On 2016/11/14 19:07:57, hidehiko wrote: > On 2016/11/14 17:06:50, Luis Héctor Chávez wrote: > > Can we check that state is either FETCHING_CODE or STOPPED? > > Unfortunately, StartArc() can be called in other state_, e.g., > CHECKING_ANDROID_MANAGEMENT (in IsOptInVerificationDisabled() == true case). Can > we leave it at the moment? sure https://codereview.chromium.org/2485233002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:966: SetState(State::SHOWING_TERMS_OF_SERVICE); On 2016/11/14 19:07:57, hidehiko wrote: > On 2016/11/14 17:06:50, Luis Héctor Chávez wrote: > > DCHECK_EQ(State::STOPPED, state_); > > Practically, we expect STOPPED, but technically this can be in other values, > e.g. CHECKING_ANDROID_MANAGEMENT or FETCHING_CODE. (kind of race issue...) Can > we address this later? sure
lgtm
Thank you for review. Submitting.
The CQ bit was checked by hidehiko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skuhne@chromium.org, xiyuan@chromium.org, lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2485233002/#ps80001 (title: "rebase")
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.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Do not run Android management check for re-auth case. This CL removes the Android management check from re-auth code flow and OOBE flow. Along with the change, onRetry implementation is moved from UI code to native code. BUG=657687 BUG=b/31079732 BUG=b/32431329 TEST=Ran on test device. Ran bots. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Do not run Android management check for re-auth case. This CL removes the Android management check from re-auth code flow and OOBE flow. Along with the change, onRetry implementation is moved from UI code to native code. BUG=657687 BUG=b/31079732 BUG=b/32431329 TEST=Ran on test device. Ran bots. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/7f74db3b9daf5ea99df3a30d272c80271750e89a Cr-Commit-Position: refs/heads/master@{#432128} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/7f74db3b9daf5ea99df3a30d272c80271750e89a Cr-Commit-Position: refs/heads/master@{#432128} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
