Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2737453003:
Fix ArcSessionManager state machine, part 1. (Closed)
|
Created:
3 years, 9 months ago by hidehiko Modified:
3 years, 9 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, oshima+watch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, sadrul, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, kalyank, davemoore+watch_chromium.org, Luis Héctor Chávez, victorhsieh, emaxx, Sergey Poromov Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix ArcSessionManager state machine, part 1.
This CL fixes the SHOWING_TERMS_OF_SERVICE part.
- Renamed to NEGOTIATING_ to reflect its actual responsibility.
- Move checks if ToS negotiation is skipped into the state.
- For better encapsulation.
- Removing state machine transition from STOPPED to
CHECKING_ANDROID_MANAGEMENT as simplification.
- Get rid of CancelAuthCode() on Terms of Service rejecting.
BUG=657687
BUG=b/31079732
TEST=Ran trybots.
Review-Url: https://codereview.chromium.org/2737453003
Cr-Commit-Position: refs/heads/master@{#455690}
Committed: https://chromium.googlesource.com/chromium/src/+/13c620a0df3f13dfb9f43c92faf2b608e0c63215
Patch Set 1 #
Total comments: 20
Patch Set 2 : rebase #Patch Set 3 : Address comments. #
Total comments: 2
Patch Set 4 : Rebase #
Messages
Total messages: 32 (18 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...
hidehiko@chromium.org changed reviewers: + yusukes@chromium.org
Yusuke-san, PTAL. FYI: victorhsieh@, I hope this CL helps your extraction work for crbug.com/698418. If you've already done some part of it, and it's conflicting with this way, please let me know. Let's resolve for better way. poromov@, I unset the ToS accept flag for Kiosk, because it actually does not accept, instead skip ToS. AFAIK, this is no-op from user's point of view. Also, along with the change, I removed your TODO, because this is something permanent. Note that Yury redesigned OOBE negotiation flow, so no one sets kArcTermsAccepted in advance. If you have any concerns, please let me know. emaxx@, thank you for your refactoring at crrev.com/2731453003. AFAIK, this should not conceptually conflict with yours, and I'm planning to wait for your CL is landed first, and then rebase on top of it. https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:705: if (AreArcAllOptInPreferencesManaged()) { Note: crrev.com/2731453003 moves the utility to c/b/c/arc/arc_util, which helps to extract this function. https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:714: // This happens, e.g., when a user accepted the Terms of service on Opt-in Note: 1) of the original comment is obsolete. khmel@ redisigned the OOBE flow, so for better adaption to the ASM state transition. https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.h:69: // STOPPED -> NEGOTIATING_TERMS_OF_SERVICE: On request to enable. Note: More detailed comment will come as a part of TODO at L79.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hidehiko@chromium.org changed reviewers: + skuhne@chromium.org
R+=skuhne@. Oops, I overlooked I touched c/b/ui/ash/launcher code... Stefan, could you take a look at them as OWNER? Thanks, - hidehiko
Description was changed from
==========
Fix ArcSessionManager state machine, part 1.
This CL fixes the SHOWING_TERMS_OF_SERVICE part.
- Renamed to NAGOTIATING_ to reflect its actual resposibility.
- Move checks if ToS negotiation is skipped into the state.
- For better encapsulation.
- Removing state machine transition from STOPPED to
CHECKING_ANDROID_MANAGEMENT as simplification.
- Get rid of CancelAuthCode() on Terms of Service rejecting.
BUG=657687
BUG=b/31079732
TEST=Ran trybots.
==========
to
==========
Fix ArcSessionManager state machine, part 1.
This CL fixes the SHOWING_TERMS_OF_SERVICE part.
- Renamed to NEGOTIATING_ to reflect its actual responsibility.
- Move checks if ToS negotiation is skipped into the state.
- For better encapsulation.
- Removing state machine transition from STOPPED to
CHECKING_ANDROID_MANAGEMENT as simplification.
- Get rid of CancelAuthCode() on Terms of Service rejecting.
BUG=657687
BUG=b/31079732
TEST=Ran trybots.
==========
https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:67: // Update UMA with user cancel only if error is not currently shown. Updates https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:580: // kArcTermsAccepted check below. "below" is ambiguous now. Can you rephrase? https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:594: // Check Android management in parallel. Can you factor out L594-604 too? Since you have ArcSessionManager::StartArcAndroidManagementCheck() for the non-background check, having a function for the background one seems cleaner and easier to read to me. https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:667: if (terms_of_service_negotiator_) { It was unclear to me when this variable can be a nullptr. Is this for dealing with |g_disable_ui_for_testing|??? Can you add a code comment? https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:697: if (IsArcKioskMode()) { When is this check necessary? Because of the IsArcKioskMode() check at L582, RequestEnableImpl() does not seem to call into this function at least. https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:746: android_management_checker_.reset(new ArcAndroidManagementChecker( MakeUnique? (L317 too) https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.h:71: // accepts with "Terms Of Service" s/with//
https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:594: // Check Android management in parallel. On 2017/03/06 20:52:16, Yusuke Sato wrote: > Can you factor out L594-604 too? Since you have > ArcSessionManager::StartArcAndroidManagementCheck() for the non-background > check, having a function for the background one seems cleaner and easier to read > to me. Just noticed you're doing it in another CL already.
victorhsieh@chromium.org changed reviewers: + victorhsieh@chromium.org
There will be some conflict but shouldn't be too bad. I also just you an email to discuss about some intention of current code, related to my ToS refactoring. https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:740: // Don't show UI for this progress if it was not shown. optional: I'm doing the same thing locally, and I wonder if it makes sense to change the comment to "// Show UI only if opt-in is ongoing". WDYT?
c/b/ui/ash/launcher 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...
Thank you for review. PTAL. https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:67: // Update UMA with user cancel only if error is not currently shown. On 2017/03/06 20:52:16, Yusuke Sato wrote: > Updates Done. https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:580: // kArcTermsAccepted check below. On 2017/03/06 20:52:16, Yusuke Sato wrote: > "below" is ambiguous now. Can you rephrase? The comment looks obsolete, so fully rewrote. To follow the order of comment (actually prod -> testing), reordered the condition terms. https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:594: // Check Android management in parallel. On 2017/03/06 22:04:56, Yusuke Sato wrote: > On 2017/03/06 20:52:16, Yusuke Sato wrote: > > Can you factor out L594-604 too? Since you have > > ArcSessionManager::StartArcAndroidManagementCheck() for the non-background > > check, having a function for the background one seems cleaner and easier to > read > > to me. > > Just noticed you're doing it in another CL already. Yes, it's done. Thank you for review for the CL, too. https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:667: if (terms_of_service_negotiator_) { On 2017/03/06 20:52:16, Yusuke Sato wrote: > It was unclear to me when this variable can be a nullptr. Is this for dealing > with |g_disable_ui_for_testing|??? Can you add a code comment? Yes, it's testing purpose. Added comment with DCHECK. https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:697: if (IsArcKioskMode()) { On 2017/03/06 20:52:16, Yusuke Sato wrote: > When is this check necessary? Because of the IsArcKioskMode() check at L582, > RequestEnableImpl() does not seem to call into this function at least. Good catch! So, I removed this from here, and added DCHECKs in StartToSNegotiation(). The comment at L695- is moved to RequestEnableImpl()'s condition, with some rephrasing. https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:740: // Don't show UI for this progress if it was not shown. On 2017/03/06 23:11:52, victorhsieh wrote: > optional: I'm doing the same thing locally, and I wonder if it makes sense to > change the comment to "// Show UI only if opt-in is ongoing". WDYT? Rephrased for positive case, with its background. Note that "opt-in is ongoing" is not the condition here, because even after OOBE ToS negotiation, if management check fails, ARC support is shown with retry button. On clicking the retry, we need to show loading. Hmm... complicated... https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:746: android_management_checker_.reset(new ArcAndroidManagementChecker( On 2017/03/06 20:52:16, Yusuke Sato wrote: > MakeUnique? (L317 too) Done in another CL. As for L317, I'm making another CL to extract the launcher part. Let me replace it in the CL. https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2737453003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.h:71: // accepts with "Terms Of Service" On 2017/03/06 20:52:16, Yusuke Sato wrote: > s/with// Done. https://codereview.chromium.org/2737453003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2737453003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:122: // This is used to delete the Play user ID if ARC is disabled for an Note: Sorry, mistakenly mixed rebase. https://codereview.chromium.org/2737453003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:200: // Remove Play user ID for Active Directory managed devices. Note: Sorry, mistakenly mixed rebase.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
lgtm
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, yusukes@chromium.org, victorhsieh@chromium.org Link to the patchset: https://codereview.chromium.org/2737453003/#ps60001 (title: "Rebase")
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
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_...)
On 2017/03/09 06:46:11, commit-bot: I haz the power wrote: > 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_...) Looks unrelated...? Resending to CQ, for now.
The CQ bit was checked by hidehiko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489042300130910,
"parent_rev": "3b0d20258d8d2009595e515010ae04f92208f4ee", "commit_rev":
"13c620a0df3f13dfb9f43c92faf2b608e0c63215"}
Message was sent while issue was closed.
Description was changed from
==========
Fix ArcSessionManager state machine, part 1.
This CL fixes the SHOWING_TERMS_OF_SERVICE part.
- Renamed to NEGOTIATING_ to reflect its actual responsibility.
- Move checks if ToS negotiation is skipped into the state.
- For better encapsulation.
- Removing state machine transition from STOPPED to
CHECKING_ANDROID_MANAGEMENT as simplification.
- Get rid of CancelAuthCode() on Terms of Service rejecting.
BUG=657687
BUG=b/31079732
TEST=Ran trybots.
==========
to
==========
Fix ArcSessionManager state machine, part 1.
This CL fixes the SHOWING_TERMS_OF_SERVICE part.
- Renamed to NEGOTIATING_ to reflect its actual responsibility.
- Move checks if ToS negotiation is skipped into the state.
- For better encapsulation.
- Removing state machine transition from STOPPED to
CHECKING_ANDROID_MANAGEMENT as simplification.
- Get rid of CancelAuthCode() on Terms of Service rejecting.
BUG=657687
BUG=b/31079732
TEST=Ran trybots.
Review-Url: https://codereview.chromium.org/2737453003
Cr-Commit-Position: refs/heads/master@{#455690}
Committed:
https://chromium.googlesource.com/chromium/src/+/13c620a0df3f13dfb9f43c92faf2...
==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/13c620a0df3f13dfb9f43c92faf2... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
