Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2561023002:
arc: ARC loading progress should not be shown when started from OOBE. (Closed)
|
Created:
4 years ago by khmel Modified:
3 years, 10 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, alemate+watch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, achuith+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: ARC loading progress should not be shown when started from OOBE.
This fix regression (also compared with M56) when ARC loading progress
is shown in case ARC is accepted from OOBE. In this case ARC is
supposed to start silently, showing UI only if user interaction is
required. For example press Sign-In button or in case of an error.
This also handles the edge case for managed ARC user. In this case
Opt UI is shown on PrimaryProfilePrepared stage and stays behind
the OOBE UI. This CL closes this redundant UI in case user accepts
ARC in OOBE.
BUG=b/33431526
TEST=Manually on device, managed/unmanaged user. Accept Arc in OOBE.
In case silent sign-in is enabled, no UI is shown and Play Store
starts once Arc is booted. In case silent sign-in is disabled
UI is shown when LSO is requested.
Start Arc manually from settings. Start page is shown. Accept Arc
and wait for LSO page is shown. Sign-out and sign-in again.
Arc continues from showing Arc loading progress.
Review-Url: https://codereview.chromium.org/2561023002
Cr-Commit-Position: refs/heads/master@{#447267}
Committed: https://chromium.googlesource.com/chromium/src/+/8d4da8a74f81f9cd097a12d4a77e5c8839a0d958
Patch Set 1 #Patch Set 2 : cleanup #
Total comments: 4
Patch Set 3 : use TermsAccepted event to start Arc for managed users #
Total comments: 18
Patch Set 4 : rebased #Patch Set 5 : oobe negotiator based #
Total comments: 5
Patch Set 6 : oobe negotiator only for managed user #Patch Set 7 : cleanup #
Total comments: 48
Patch Set 8 : comments addressed #Patch Set 9 : small fix #
Total comments: 24
Patch Set 10 : comments addresses #
Total comments: 10
Patch Set 11 : refactored #
Total comments: 50
Patch Set 12 : comments addressed #
Total comments: 20
Patch Set 13 : nits #
Total comments: 2
Patch Set 14 : rebased + comments updated #Messages
Total messages: 45 (20 generated)
khmel@chromium.org changed reviewers: + hidehiko@chromium.org, xiyuan@chromium.org
Hi Xiyuan and Hidehiko, PTAL
https://codereview.chromium.org/2561023002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2561023002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:501: // In case of managed Arc, UI is started OnPrimaryProfilePrepared which nit: OnPrimaryProfilePrepared -> OnPrimary*User*ProfilePrepared https://codereview.chromium.org/2561023002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:505: Close(); Is it possible to not open the UI at all instead of closing it here? e.g. integrate with ArcTermsOfServiceNegotiator so that it knows we are showing OOBE ToS ?
Thanks Xiyuan for comments! https://codereview.chromium.org/2561023002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2561023002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:501: // In case of managed Arc, UI is started OnPrimaryProfilePrepared which On 2016/12/08 20:27:28, xiyuan wrote: > nit: OnPrimaryProfilePrepared -> OnPrimary*User*ProfilePrepared Done. https://codereview.chromium.org/2561023002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:505: Close(); On 2016/12/08 20:27:28, xiyuan wrote: > Is it possible to not open the UI at all instead of closing it here? e.g. > integrate with ArcTermsOfServiceNegotiator so that it knows we are showing OOBE > ToS ? Yes, not opening is preferred. However found yet another problem. In case Arc managed enabled hiding OptIn does not have effect, because pref is not changed. I think it is better to analyze first run flag and listen for terms accepted change. OnPrimaryUserProfile is actually called before showing Arc OOBE. Could you PTAL to new variant? In this case we don't need to signal anything from OOBE and we have loose coupling here. WDYT?
https://codereview.chromium.org/2561023002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2561023002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:615: if (!IsFirstRunForUser()) This changes the existing behavior. For OOBE OptIn, we will not show ARC loading from 2nd login and onward. For non OOBE optin, since we don't have kEnableArcOOBEOptIn switch, we will never show ARC loading. Is this right? https://codereview.chromium.org/2561023002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:626: pref_change_registrar_.Add( nit: move into StartTermsOfServiceNegotiation() since OOBE ToS is conceptually a ToS negotiation too. https://codereview.chromium.org/2561023002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:636: if (profile_->GetPrefs()->GetBoolean(prefs::kArcTermsAccepted)) Why do we need to bail and keep observing kArcTermsAccepted? Also, I am wondering that maybe we can chain this with OnTermsOfServiceNegotiated() instead of OnOptInPreferenceChanged ?
As you may know, I put higher level question on the linked bug. Let's see Hiro-san's answer. Below is my comment for the current code, but I may change mine based on Hiro-san's answer. https://codereview.chromium.org/2561023002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2561023002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:81: bool IsFirstRunForUser() { The name looks not reflecting what this does, because the name does not implies the OOBE. Could you rename? E.g.; IsFirstRunForOobeEnabledUser() etc.? https://codereview.chromium.org/2561023002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:82: // Arc OOBE OptIn is optional for now. Test if it exists and current user is s/Arc/ARC/ for all comments in the CL, including CL description. https://codereview.chromium.org/2561023002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:615: if (!IsFirstRunForUser()) I'm confused. IIUC; OnPrimaryUserProfilePrepared is run before OOBE, you said. Then, this should be always false, otherwise this means we run ARC before ToS agreement on OOBE? https://codereview.chromium.org/2561023002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:625: if (IsArcManaged() && IsFirstRunForUser()) { This condition looks very complicated to me. Could you postpone running this method until OOBE ToS accepting condition meets, even on managed case, instead? https://codereview.chromium.org/2561023002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:640: DCHECK(IsArcManaged() && IsArcEnabled()); nit: please split DCHECK(). DCHECK(IsArcManaged()); DCHECK(IsArcEnabled()); https://codereview.chromium.org/2561023002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:814: (state_ == State::STOPPED && Good catch! Thanks for fixing.
Description was changed from
==========
arc: Arc loading progress should not be shown when started from OOBE.
This fix regression (also compared with M56) when Arc loading progress
is shown in case Arc is accepted from OOBE. In this case Arc is
supposed to start silently, showing UI only if user interaction is
required. For example press Sign-In button or in case of an error.
This also handles the edge case for managed Arc user. In this case
Opt UI is shown on PrimaryProfilePrepared stage and stays behind
the OOBE UI. This CL closes this redundant UI in case user accepts
Arc in OOBE.
BUG=b/33431526
TEST=Manually on device, managed/unmanaged user. Accept Arc in OOBE.
In case silent sign-in is enabled, no UI is shown and Play Store
starts once Arc is booted. In case silent sign-in is disabled
UI is shown when LSO is requested.
Start Arc manually from settings. Start page is shown. Accept Arc
and wait for LSO page is shown. Sign-out and sign-in again.
Arc continues from showing Arc loading progress.
==========
to
==========
arc: ARC loading progress should not be shown when started from OOBE.
This fix regression (also compared with M56) when ARC loading progress
is shown in case ARC is accepted from OOBE. In this case ARC is
supposed to start silently, showing UI only if user interaction is
required. For example press Sign-In button or in case of an error.
This also handles the edge case for managed ARC user. In this case
Opt UI is shown on PrimaryProfilePrepared stage and stays behind
the OOBE UI. This CL closes this redundant UI in case user accepts
ARC in OOBE.
BUG=b/33431526
TEST=Manually on device, managed/unmanaged user. Accept Arc in OOBE.
In case silent sign-in is enabled, no UI is shown and Play Store
starts once Arc is booted. In case silent sign-in is disabled
UI is shown when LSO is requested.
Start Arc manually from settings. Start page is shown. Accept Arc
and wait for LSO page is shown. Sign-out and sign-in again.
Arc continues from showing Arc loading progress.
==========
Patchset #4 (id:60001) has been deleted
Thank you for comments. Based on our discussion I think it is more logical to move OOBE dependent logic to separate place, OOBE negotiator and work via it. This would keep arc_session_manager clearer and also moves some logic from OOBE arc to new negotiator. PTAL https://codereview.chromium.org/2561023002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2561023002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:81: bool IsFirstRunForUser() { On 2016/12/09 06:59:57, hidehiko wrote: > The name looks not reflecting what this does, because the name does not implies > the OOBE. Could you rename? > E.g.; IsFirstRunForOobeEnabledUser() etc.? Nice name, thanks! https://codereview.chromium.org/2561023002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:82: // Arc OOBE OptIn is optional for now. Test if it exists and current user is On 2016/12/09 06:59:57, hidehiko wrote: > s/Arc/ARC/ for all comments in the CL, including CL description. Done. https://codereview.chromium.org/2561023002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:615: if (!IsFirstRunForUser()) On 2016/12/09 06:59:57, hidehiko wrote: > I'm confused. > > IIUC; > OnPrimaryUserProfilePrepared is run before OOBE, you said. > Then, this should be always false, otherwise this means we run ARC before ToS > agreement on OOBE? That is good point and in theory it is true :), however Chrome sends us OnOptInPreferenceChanged notification even if set Arc.Enabled managed pref to true. Resulting state of Arc.Enabled is not changed but notification is issued. That is quite confusing. Anyway I discard this code for more clear logic and avoid settings arc.enabled pref for managed ARC. https://codereview.chromium.org/2561023002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:615: if (!IsFirstRunForUser()) On 2016/12/09 00:01:45, xiyuan wrote: > This changes the existing behavior. > > For OOBE OptIn, we will not show ARC loading from 2nd login and onward. > > For non OOBE optin, since we don't have kEnableArcOOBEOptIn switch, we will > never show ARC loading. > > Is this right? ARC OptIn OOBE is shown only for first time user. In case kEnableArcOOBEOptIn is not set, IsFirstRunForUser is always false and this is legacy case. Anyway I drop this line as described in comment above. https://codereview.chromium.org/2561023002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:625: if (IsArcManaged() && IsFirstRunForUser()) { On 2016/12/09 06:59:57, hidehiko wrote: > This condition looks very complicated to me. > Could you postpone running this method until OOBE ToS accepting condition meets, > even on managed case, instead? I changed code to negotiator based solution. So this one is deprecated. https://codereview.chromium.org/2561023002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:626: pref_change_registrar_.Add( On 2016/12/09 00:01:45, xiyuan wrote: > nit: move into StartTermsOfServiceNegotiation() since OOBE ToS is conceptually a > ToS negotiation too. That is right, Done. https://codereview.chromium.org/2561023002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:636: if (profile_->GetPrefs()->GetBoolean(prefs::kArcTermsAccepted)) On 2016/12/09 00:01:45, xiyuan wrote: > Why do we need to bail and keep observing kArcTermsAccepted? > > Also, I am wondering that maybe we can chain this with > OnTermsOfServiceNegotiated() instead of OnOptInPreferenceChanged ? Agree, this is misprint. However it used to work in my tests because OnOptInPreferenceChanged was triggered even if I set managed Arc.enabled. https://codereview.chromium.org/2561023002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:640: DCHECK(IsArcManaged() && IsArcEnabled()); On 2016/12/09 06:59:57, hidehiko wrote: > nit: please split DCHECK(). > DCHECK(IsArcManaged()); > DCHECK(IsArcEnabled()); This code is discarded. https://codereview.chromium.org/2561023002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:814: (state_ == State::STOPPED && On 2016/12/09 06:59:57, hidehiko wrote: > Good catch! Thanks for fixing. Acknowledged. https://codereview.chromium.org/2561023002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2561023002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:478: if (!first_run_oobe_enabled_user) It is quite tricky to sync two processes which change state_. I had two options here. Wait and don't show ARC OOBE until any pending remove data request is completed or skip removing data for first user login.
lgtm Nice. https://codereview.chromium.org/2561023002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h (right): https://codereview.chromium.org/2561023002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h:40: PrefService* pref_service_; nit: PrefService* const
My biggest worry is a new condition combination added in the last PS, as commented inline and we discussed offline. Addressing it could have various way, so I'll take a look in details, when you upload a new PS. Again, thank you for discussion! - hidehiko https://codereview.chromium.org/2561023002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2561023002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:461: StartTermsOfServiceNegotiation(NegotiatorType::OOBE); For the record; As we discussed offline, could you avoid to add a new combination of state and preference, which is SHOWING_TERMS_OF_SERVICE and arc.enabled = false? Now, ArcSessionManager is complicated state machine, so I'd like avoid to make it more complicated situation...
Hi, I addressed Hidehiko's concern about not having arc.enabled = false and ToS negotiator. Now OOBE ToS negotiator only for Managed users. PTAL https://codereview.chromium.org/2561023002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2561023002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:461: StartTermsOfServiceNegotiation(NegotiatorType::OOBE); On 2016/12/15 03:04:07, hidehiko wrote: > For the record; > > As we discussed offline, could you avoid to add a new combination of state and > preference, which is SHOWING_TERMS_OF_SERVICE and arc.enabled = false? > > Now, ArcSessionManager is complicated state machine, so I'd like avoid to make > it more complicated situation... Done. https://codereview.chromium.org/2561023002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h (right): https://codereview.chromium.org/2561023002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h:40: PrefService* pref_service_; On 2016/12/14 17:07:32, xiyuan wrote: > nit: PrefService* const Done (discarded).
https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:82: bool IsFirstRunForOobeEnabledUser() { Could you comment "FirstRun" is the Chrome term meaning the first login session, and *not* meaning this is ARC's first run? https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:466: StartTermsOfServiceNegotiation(NegotiatorType::OOBE); I still do not want to have this function here. So, could you move this into OnOptInPrefChanged() as follows? void OnPrimaryUserProfilePrepared() { : s/OnOptInPrefChanged()/OnOptInPrefChanged(true /* is initial run */)/ : } void OnOptInPrefChanged() { OnOptInPrefChangedInternal(false /* is initial run */); } // Welcome better naming :-) void OnOptInPrefChangedInternal(bool is_initial_run) { : // almost current code. StartTermsOfServiceNegotiation(is_initial_run); } void StartTermsOfServiceNegotiation(bool is_initial_run) { : // check ABS running status. if (is_initial_run && IsFirstRunForOobeEnabledUser() && IsManaged()) { ... = base::MakeUnique<ArcTosManagedOobeNegotiator(...); } else if (arc_support_) { ... = base::MakeUnique<ArcTosDefaultNegotiator>(...); } if (negotiator) ...->StartNegotiation(...); } All other OnOptInPrefChanged() call can be OnOptInPrefChangedInternal(false); I skipped to comment for the code which relates to this. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.h (right): https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.h:129: bool has_message_host() const { return message_host_; } Please do not expose anything about related to message_host_. It is asynchronously connected, and this class is wrapping it to encapsulates the asynchronous-ness. IIUC, you do not need anymore? https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h (right): https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h:13: // Interface to handle the Terms-of-service agreement user action. s/agreement/accepting/ for word-consistency? https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h:20: // ToS. If user explicitly rejects ToS, invokes |callback| with |agreed| = s/ToS/terms of service/ https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h:24: virtual void StartNegotiation(const NegotiationCallback& callback); How about: void StartNegotiation(const NegotiationCallback& callback) { DCHECK(!pending_callback_); pending_callback_ = pending_callback_; StartNegotiationImpl(); } protected: virtual void StartNegotiationImpl() = 0; to encapsulate |callback| related operation in this class? https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h:27: void ReportResult(bool accepted); Please add documentation. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator_unittest.cc (right): https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator_unittest.cc:26: class ArcTermsOfServiceNegotiatorTest : public testing::Test { Rename to ArcTermsOfServiceDefaultNegotiatorTest? Rename the file, too, please. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.cc (right): https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.cc:29: if (!host || !host->GetOobeUI()) { Could you comment when this happens? Also, can we verify (DCHECK) that the OOBE flow is not yet completed here? Is above condition guarantees it? https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h (right): https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. Please add unittest. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h:13: class ArcTermsOfServiceScreenActor; Please remove. (Should be a part of ArcTermsOfServiceScreenActorObserver interface.) https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h:20: class ArcTermsOfServiceOOBENegotiator This is no longer negotiator for all OOBE case. Maybe ArcTermsOfServiceInitialOobeNegotiatorForManagedUser etc.? (hmm... long...). https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h:30: // chromeos::ArcTermsOfServiceScreenActorObserver: Move to private? https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc (right): https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc:59: profile->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, accepted); Looks a kind of race. Because both ArcTermsOfServiceScreen and ArcTermsOfServiceOobeNegotiator implement same observer, and there is no guarantee about the order. So, if the negotiator's called first, and in its call stack kArcTermsAccepted is set to false somehow, then this is called, it is wrongly set to true. Of course, if this is called first, and in its call stack, kArcTermsAccepted is set to false, in ArcSessionManager it is set to true again. In that sense, IMHO, observing kArcTermsAccepted approach sounds better to me, but I'm not strong opinion here as long as the race above is resolved cleanly. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc:60: if (!profile->GetPrefs()->IsManagedPreference(prefs::kArcEnabled)) Is this check really needed? https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/screens/arc_terms_of_service_screen_actor_observer.h (right): https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/screens/arc_terms_of_service_screen_actor_observer.h:14: class ArcTermsOfServiceScreenActorObserver { Class document please. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/screens/arc_terms_of_service_screen_actor_observer.h:28: ArcTermsOfServiceScreenActorObserver() = default; You can remove this. this Observer cannot be directly instantiated because of pure virtual methods.
Thank you for comments. PTAL https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:82: bool IsFirstRunForOobeEnabledUser() { On 2016/12/16 05:31:50, hidehiko wrote: > Could you comment "FirstRun" is the Chrome term meaning the first login session, > and *not* meaning this is ARC's first run? Done. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:466: StartTermsOfServiceNegotiation(NegotiatorType::OOBE); On 2016/12/16 05:31:50, hidehiko wrote: > I still do not want to have this function here. > So, could you move this into OnOptInPrefChanged() as follows? > > void OnPrimaryUserProfilePrepared() { > : > s/OnOptInPrefChanged()/OnOptInPrefChanged(true /* is initial run */)/ > : > } > > void OnOptInPrefChanged() { > OnOptInPrefChangedInternal(false /* is initial run */); > } > > // Welcome better naming :-) > void OnOptInPrefChangedInternal(bool is_initial_run) { > : // almost current code. > StartTermsOfServiceNegotiation(is_initial_run); > } > > void StartTermsOfServiceNegotiation(bool is_initial_run) { > : // check ABS running status. > if (is_initial_run && IsFirstRunForOobeEnabledUser() && IsManaged()) { > ... = base::MakeUnique<ArcTosManagedOobeNegotiator(...); > } else if (arc_support_) { > ... = base::MakeUnique<ArcTosDefaultNegotiator>(...); > } > > if (negotiator) > ...->StartNegotiation(...); > } > > All other OnOptInPrefChanged() call can be OnOptInPrefChangedInternal(false); > > I skipped to comment for the code which relates to this. Done. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.h (right): https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.h:129: bool has_message_host() const { return message_host_; } It is needed, please see void ArcSessionManager::OnTermsOfServiceNegotiated(bool accepted). This method starts Arc Loading progress. It is required for default case but not required for OOBE case. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h (right): https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h:13: // Interface to handle the Terms-of-service agreement user action. On 2016/12/16 05:31:51, hidehiko wrote: > s/agreement/accepting/ for word-consistency? Done. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h:20: // ToS. If user explicitly rejects ToS, invokes |callback| with |agreed| = On 2016/12/16 05:31:51, hidehiko wrote: > s/ToS/terms of service/ Done. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h:24: virtual void StartNegotiation(const NegotiationCallback& callback); On 2016/12/16 05:31:50, hidehiko wrote: > How about: > > void StartNegotiation(const NegotiationCallback& callback) { > DCHECK(!pending_callback_); > pending_callback_ = pending_callback_; > StartNegotiationImpl(); > } > > protected: > virtual void StartNegotiationImpl() = 0; > > to encapsulate |callback| related operation in this class? Done. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h:27: void ReportResult(bool accepted); On 2016/12/16 05:31:50, hidehiko wrote: > Please add documentation. Done. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator_unittest.cc (right): https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator_unittest.cc:26: class ArcTermsOfServiceNegotiatorTest : public testing::Test { On 2016/12/16 05:31:51, hidehiko wrote: > Rename to ArcTermsOfServiceDefaultNegotiatorTest? > Rename the file, too, please. Done. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.cc (right): https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.cc:29: if (!host || !host->GetOobeUI()) { On 2016/12/16 05:31:51, hidehiko wrote: > Could you comment when this happens? > > Also, can we verify (DCHECK) that the OOBE flow is not yet completed here? > Is above condition guarantees it? Changed to DCHECK https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h (right): https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/12/16 05:31:51, hidehiko wrote: > Please add unittest. Done. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h:13: class ArcTermsOfServiceScreenActor; On 2016/12/16 05:31:51, hidehiko wrote: > Please remove. (Should be a part of ArcTermsOfServiceScreenActorObserver > interface.) Done. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h:20: class ArcTermsOfServiceOOBENegotiator On 2016/12/16 05:31:51, hidehiko wrote: > This is no longer negotiator for all OOBE case. > Maybe ArcTermsOfServiceInitialOobeNegotiatorForManagedUser etc.? (hmm... > long...). Done. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h:30: // chromeos::ArcTermsOfServiceScreenActorObserver: On 2016/12/16 05:31:51, hidehiko wrote: > Move to private? Done. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc (right): https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc:59: profile->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, accepted); On 2016/12/16 05:31:51, hidehiko wrote: > Looks a kind of race. > Because both ArcTermsOfServiceScreen and ArcTermsOfServiceOobeNegotiator > implement same observer, and there is no guarantee about the order. So, if the > negotiator's called first, and in its call stack kArcTermsAccepted is set to > false somehow, then this is called, it is wrongly set to true. Of course, if > this is called first, and in its call stack, kArcTermsAccepted is set to false, > in ArcSessionManager it is set to true again. > > In that sense, IMHO, observing kArcTermsAccepted approach sounds better to me, > but I'm not strong opinion here as long as the race above is resolved cleanly. There is no race condition here. ArcTermsOfServiceScreen sets observer first. UserSessionManager::InitializeUserSession(). PS. This code would work even with race condition. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc:60: if (!profile->GetPrefs()->IsManagedPreference(prefs::kArcEnabled)) On 2016/12/16 05:31:51, hidehiko wrote: > Is this check really needed? This prevents calling redundant OnOptInProfileChanged https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/screens/arc_terms_of_service_screen_actor_observer.h (right): https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/screens/arc_terms_of_service_screen_actor_observer.h:14: class ArcTermsOfServiceScreenActorObserver { On 2016/12/16 05:31:51, hidehiko wrote: > Class document please. Done. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/screens/arc_terms_of_service_screen_actor_observer.h:28: ArcTermsOfServiceScreenActorObserver() = default; On 2016/12/16 05:31:51, hidehiko wrote: > You can remove this. > this Observer cannot be directly instantiated because of pure virtual methods. It is required due: ../../chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc:18:26: error: constructor for 'chromeos::ArcTermsOfServiceScreen' must explicitly initialize the base class 'chromeos::ArcTermsOfServiceScreenActorObserver' which does not have a default constructor
https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.h (right): https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.h:129: bool has_message_host() const { return message_host_; } On 2016/12/16 18:37:55, khmel wrote: > It is needed, please see void ArcSessionManager::OnTermsOfServiceNegotiated(bool > accepted). This method starts Arc Loading progress. It is required for default > case but not required for OOBE case. Oh I see. Then please use ui_page_, instead, for the page transition tracking. Considering that we do want to remove it directly, maybe we'd like to have another accessor, e.g. IsWindowOpened(), with comment about the timing issue (the actual window opening/closing may be delayed). https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.cc (right): https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.cc:29: if (!host || !host->GetOobeUI()) { On 2016/12/16 18:37:55, khmel wrote: > On 2016/12/16 05:31:51, hidehiko wrote: > > Could you comment when this happens? > > > > Also, can we verify (DCHECK) that the OOBE flow is not yet completed here? > > Is above condition guarantees it? > > Changed to DCHECK Comments are still missing. Please find my comment in the new PS, too. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc (right): https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc:59: profile->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, accepted); On 2016/12/16 18:37:55, khmel wrote: > On 2016/12/16 05:31:51, hidehiko wrote: > > Looks a kind of race. > > Because both ArcTermsOfServiceScreen and ArcTermsOfServiceOobeNegotiator > > implement same observer, and there is no guarantee about the order. So, if the > > negotiator's called first, and in its call stack kArcTermsAccepted is set to > > false somehow, then this is called, it is wrongly set to true. Of course, if > > this is called first, and in its call stack, kArcTermsAccepted is set to > false, > > in ArcSessionManager it is set to true again. > > > > In that sense, IMHO, observing kArcTermsAccepted approach sounds better to me, > > but I'm not strong opinion here as long as the race above is resolved cleanly. > > There is no race condition here. ArcTermsOfServiceScreen sets observer first. > UserSessionManager::InitializeUserSession(). > PS. This code would work even with race condition. Ah, maybe "race" was not a good word choice. Sorry for that. But I'm still worried about some points. 1) Depending on "ArcTermsOfServiceScreen callbacks is called earlier than ArcTermsOfServiceOobeNegotiatorForManagedUser" sounds like much fragile to me. It is because the registering order and the callback invocation order of ObserverList look just implementation details. 2) Your code sets kArcTermsAccepted twice, which sounds a source of bug. Actually, it is problematic in my scenario above. 3) Because ArcSessionManager and OOBE systems are designed to be independent, so life cycle management look unclear to me. So, if you continue to choose this new observer approach, please address those three. 1) Get rid of depending the order of same observer callback invocation. 2) Do not set kArcTermsAccepted twice per user action. 3) Add clear and detailed documentation / comments about life cycle management of two independent systems. Also, please add explicit tests for these to guard from breakage. Or, alternatively, I still think observing kArcTermsAccepted is an option. That approach does not have those concerns, IIUC. # Instead, it is probably necessary to document who sets the preference etc. WDYT? https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc:60: if (!profile->GetPrefs()->IsManagedPreference(prefs::kArcEnabled)) On 2016/12/16 18:37:55, khmel wrote: > On 2016/12/16 05:31:51, hidehiko wrote: > > Is this check really needed? > > This prevents calling redundant OnOptInProfileChanged According to the current implementation and comments, it looks designed to be called only when the value is actually changed. If not, it sounds like a bug? Who actually calls it? # I wish it were explicitly documented... cf) https://cs.chromium.org/chromium/src/components/prefs/json_pref_store.cc?sq=p... https://cs.chromium.org/chromium/src/components/prefs/in_memory_pref_store.cc... https://cs.chromium.org/chromium/src/services/preferences/public/cpp/pref_obs... and so on. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/screens/arc_terms_of_service_screen_actor_observer.h (right): https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/screens/arc_terms_of_service_screen_actor_observer.h:28: ArcTermsOfServiceScreenActorObserver() = default; On 2016/12/16 18:37:55, khmel wrote: > On 2016/12/16 05:31:51, hidehiko wrote: > > You can remove this. > > this Observer cannot be directly instantiated because of pure virtual methods. > > It is required due: > > ../../chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc:18:26: > error: constructor for 'chromeos::ArcTermsOfServiceScreen' must explicitly > initialize the base class 'chromeos::ArcTermsOfServiceScreenActorObserver' which > does not have a default constructor Ah, I see. Optional: alternative is removing DISALLOW_COPY_AND_ASSIGN from this interface, and let implement classes decide it. Up to you. https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:566: ArcSessionManagerTest::TearDown(); Please call this after L568 (in general TearDown should be done in the reverse order of SetUp). https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc (right): https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc:38: // User cancels terms-of-service agreement UI by clicking "Cancel" button As you're here, could you s/agreement/accepting/, for word consistency? https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.h (right): https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.h:33: protected: I think this can be private. https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h (right): https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h:19: // Invokes the|callback| asynchronously with "|agreed| = true" if user accepts Also, as you're here, could you s/agreed/accepted/ for consistency, too? https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator_unittest.cc (right): https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. Please rename the file, too. https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator_for_managed_user.cc (right): https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator_for_managed_user.cc:32: chromeos::LoginDisplayHost* host = chromeos::LoginDisplayHost::default_host(); There are many occurrence to find ArcTermsOfServiceScreenActor*. So could you introduce a getter in anonymous namespace? chromeos::ArcTermsOfServiceScreenActor* GetScreenActor() { // Inject testing instance. if (actor_for_testing) return actor_for_testing; chormeos::LoginDisplayHost* host = chromeos::LoginDisplayHost::default_host(); if (!host) return nullptr; DCHECK(host->GetOobeUI()); return host->GetOobeUI()->GetArcTermsOfServiceScreenActor(); } https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator_for_managed_user.cc:37: if (host && host->GetOobeUI()) IIUC; - in most case, we do not need RemoveObserver() in dtor, because it should be already removed in HandleTermsAccepted(). - The only exception is, when the arc.enabled preference is set to false, so that negotiation procedure is cancelled. In such a case, host and OOBE UI instances should be alive always. Is my understanding true? Could you comment to cover all the case where this RemoveObserver is actually needed? https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator_for_managed_user.cc:47: chromeos::LoginDisplayHost* host = chromeos::LoginDisplayHost::default_host(); Could you comment that who "conceptually" guarantees this is alive at this moment? (Also please see my comments about observing the actor and preference.) It's better to have a very detailed documentation about the lifetime management across modules (ArcSessionManager, this class, and ArcTermsOfServiceScreenActor). https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator_for_managed_user.h (right): https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator_for_managed_user.h:5: #ifndef CHROME_BROWSER_CHROMEOS_ARC_OPTIN_ARC_TERMS_OF_SERVICE_OOBE_NEGOTIATOR_FOR_MANAGED_USER_H_ arc_terms_of_service_initial_oobe_negotiator_for_managed_user? https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator_for_managed_user.h:14: // Handles the Terms-of-service agreement action for Arc managed user via OOBE s/agreement/accepting/ https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator_for_managed_user.h:28: void StartNegotiationImpl() override; Ditto: I think this can be private.
Thank you for comments PTAL https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.h (right): https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.h:129: bool has_message_host() const { return message_host_; } On 2016/12/19 15:49:44, hidehiko wrote: > On 2016/12/16 18:37:55, khmel wrote: > > It is needed, please see void > ArcSessionManager::OnTermsOfServiceNegotiated(bool > > accepted). This method starts Arc Loading progress. It is required for default > > case but not required for OOBE case. > > Oh I see. Then please use ui_page_, instead, for the page transition tracking. > Considering that we do want to remove it directly, maybe we'd like to have > another accessor, e.g. IsWindowOpened(), with comment about the timing issue > (the actual window opening/closing may be delayed). I added parameter force_activate to ShowArcLoading. Hope this would work better. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.cc (right): https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.cc:29: if (!host || !host->GetOobeUI()) { On 2016/12/19 15:49:44, hidehiko wrote: > On 2016/12/16 18:37:55, khmel wrote: > > On 2016/12/16 05:31:51, hidehiko wrote: > > > Could you comment when this happens? > > > > > > Also, can we verify (DCHECK) that the OOBE flow is not yet completed here? > > > Is above condition guarantees it? > > > > Changed to DCHECK > > Comments are still missing. Please find my comment in the new PS, too. Resolved in .h comments https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc (right): https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc:59: profile->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, accepted); On 2016/12/19 15:49:44, hidehiko wrote: > On 2016/12/16 18:37:55, khmel wrote: > > On 2016/12/16 05:31:51, hidehiko wrote: > > > Looks a kind of race. > > > Because both ArcTermsOfServiceScreen and ArcTermsOfServiceOobeNegotiator > > > implement same observer, and there is no guarantee about the order. So, if > the > > > negotiator's called first, and in its call stack kArcTermsAccepted is set to > > > false somehow, then this is called, it is wrongly set to true. Of course, if > > > this is called first, and in its call stack, kArcTermsAccepted is set to > > false, > > > in ArcSessionManager it is set to true again. > > > > > > In that sense, IMHO, observing kArcTermsAccepted approach sounds better to > me, > > > but I'm not strong opinion here as long as the race above is resolved > cleanly. > > > > There is no race condition here. ArcTermsOfServiceScreen sets observer first. > > UserSessionManager::InitializeUserSession(). > > PS. This code would work even with race condition. > > Ah, maybe "race" was not a good word choice. Sorry for that. > But I'm still worried about some points. > > 1) Depending on "ArcTermsOfServiceScreen callbacks is called earlier than > ArcTermsOfServiceOobeNegotiatorForManagedUser" sounds like much fragile to me. > It is because the registering order and the callback invocation order of > ObserverList look just implementation details. > 2) Your code sets kArcTermsAccepted twice, which sounds a source of bug. > Actually, it is problematic in my scenario above. > 3) Because ArcSessionManager and OOBE systems are designed to be independent, so > life cycle management look unclear to me. > > So, if you continue to choose this new observer approach, please address those > three. > 1) Get rid of depending the order of same observer callback invocation. > 2) Do not set kArcTermsAccepted twice per user action. > 3) Add clear and detailed documentation / comments about life cycle management > of two independent systems. > > Also, please add explicit tests for these to guard from breakage. > > Or, alternatively, I still think observing kArcTermsAccepted is an option. That > approach does not have those concerns, IIUC. > # Instead, it is probably necessary to document who sets the preference etc. > > WDYT? I don't see any problem in order which are they called. To test this I explicitly changed the order of calling and it still works. >> 2) Do not set kArcTermsAccepted twice per user action. removed. >> 3) Add clear and detailed documentation / comments about life cycle management >> of two independent systems. added comments in OOBE negotiator. >> Also, please add explicit tests for these to guard from breakage. Sorry, I don't understand about which test are you talking. If this is OOBE test for Arc pages, then there is TODO to implement such tests and this is big task outside of this CL. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc:60: if (!profile->GetPrefs()->IsManagedPreference(prefs::kArcEnabled)) On 2016/12/19 15:49:44, hidehiko wrote: > On 2016/12/16 18:37:55, khmel wrote: > > On 2016/12/16 05:31:51, hidehiko wrote: > > > Is this check really needed? > > > > This prevents calling redundant OnOptInProfileChanged > > According to the current implementation and comments, > it looks designed to be called only when the value is actually changed. > If not, it sounds like a bug? Who actually calls it? > # I wish it were explicitly documented... > > cf) > https://cs.chromium.org/chromium/src/components/prefs/json_pref_store.cc?sq=p... > https://cs.chromium.org/chromium/src/components/prefs/in_memory_pref_store.cc... > https://cs.chromium.org/chromium/src/services/preferences/public/cpp/pref_obs... > > and so on. I have no idea at this moment who calls this method. I think this is definitely out of scope of this CL. Even so changing this is very risky in terms of whole Chrome project. FYI, we already use this approach here: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_session_... https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/screens/arc_terms_of_service_screen_actor_observer.h (right): https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/screens/arc_terms_of_service_screen_actor_observer.h:28: ArcTermsOfServiceScreenActorObserver() = default; On 2016/12/19 15:49:44, hidehiko wrote: > On 2016/12/16 18:37:55, khmel wrote: > > On 2016/12/16 05:31:51, hidehiko wrote: > > > You can remove this. > > > this Observer cannot be directly instantiated because of pure virtual > methods. > > > > It is required due: > > > > > ../../chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc:18:26: > > error: constructor for 'chromeos::ArcTermsOfServiceScreen' must explicitly > > initialize the base class 'chromeos::ArcTermsOfServiceScreenActorObserver' > which > > does not have a default constructor > > Ah, I see. > Optional: alternative is removing DISALLOW_COPY_AND_ASSIGN from this interface, > and let implement classes decide it. Up to you. Noted, thanks for hint https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:566: ArcSessionManagerTest::TearDown(); On 2016/12/19 15:49:44, hidehiko wrote: > Please call this after L568 (in general TearDown should be done in the reverse > order of SetUp). Done. https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc (right): https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.cc:38: // User cancels terms-of-service agreement UI by clicking "Cancel" button On 2016/12/19 15:49:44, hidehiko wrote: > As you're here, could you s/agreement/accepting/, for word consistency? Done. https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.h (right): https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.h:33: protected: On 2016/12/19 15:49:44, hidehiko wrote: > I think this can be private. Done. https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h (right): https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h:19: // Invokes the|callback| asynchronously with "|agreed| = true" if user accepts On 2016/12/19 15:49:44, hidehiko wrote: > Also, as you're here, could you s/agreed/accepted/ for consistency, too? Done. https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator_unittest.cc (right): https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/12/19 15:49:44, hidehiko wrote: > Please rename the file, too. Done. https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator_for_managed_user.cc (right): https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator_for_managed_user.cc:32: chromeos::LoginDisplayHost* host = chromeos::LoginDisplayHost::default_host(); On 2016/12/19 15:49:44, hidehiko wrote: > There are many occurrence to find ArcTermsOfServiceScreenActor*. So could you > introduce a getter in anonymous namespace? > > chromeos::ArcTermsOfServiceScreenActor* GetScreenActor() { > // Inject testing instance. > if (actor_for_testing) > return actor_for_testing; > > chormeos::LoginDisplayHost* host = chromeos::LoginDisplayHost::default_host(); > if (!host) > return nullptr; > DCHECK(host->GetOobeUI()); > return host->GetOobeUI()->GetArcTermsOfServiceScreenActor(); > } Done. https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator_for_managed_user.cc:37: if (host && host->GetOobeUI()) On 2016/12/19 15:49:44, hidehiko wrote: > IIUC; > - in most case, we do not need RemoveObserver() in dtor, because it should be > already removed in HandleTermsAccepted(). > - The only exception is, when the arc.enabled preference is set to false, so > that negotiation procedure is cancelled. In such a case, host and OOBE UI > instances should be alive always. > > Is my understanding true? Could you comment to cover all the case where this > RemoveObserver is actually needed? >> arc.enabled preference is set to false It is impossible for ARC managed user. Removed in DTOR https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator_for_managed_user.cc:47: chromeos::LoginDisplayHost* host = chromeos::LoginDisplayHost::default_host(); On 2016/12/19 15:49:44, hidehiko wrote: > Could you comment that who "conceptually" guarantees this is alive at this > moment? > (Also please see my comments about observing the actor and preference.) > > It's better to have a very detailed documentation about the lifetime management > across modules (ArcSessionManager, this class, and > ArcTermsOfServiceScreenActor). Comments added. https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator_for_managed_user.h (right): https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator_for_managed_user.h:5: #ifndef CHROME_BROWSER_CHROMEOS_ARC_OPTIN_ARC_TERMS_OF_SERVICE_OOBE_NEGOTIATOR_FOR_MANAGED_USER_H_ On 2016/12/19 15:49:44, hidehiko wrote: > arc_terms_of_service_initial_oobe_negotiator_for_managed_user? Done. https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator_for_managed_user.h:14: // Handles the Terms-of-service agreement action for Arc managed user via OOBE On 2016/12/19 15:49:45, hidehiko wrote: > s/agreement/accepting/ Done. https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator_for_managed_user.h:28: void StartNegotiationImpl() override; On 2016/12/19 15:49:44, hidehiko wrote: > Ditto: I think this can be private. Done.
Sorry, I do not have enough time to have detailed review for this CL today. Mostly higher comments. Will take another look later. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc (right): https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc:59: profile->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, accepted); On 2016/12/19 17:48:10, khmel wrote: > On 2016/12/19 15:49:44, hidehiko wrote: > > On 2016/12/16 18:37:55, khmel wrote: > > > On 2016/12/16 05:31:51, hidehiko wrote: > > > > Looks a kind of race. > > > > Because both ArcTermsOfServiceScreen and ArcTermsOfServiceOobeNegotiator > > > > implement same observer, and there is no guarantee about the order. So, if > > the > > > > negotiator's called first, and in its call stack kArcTermsAccepted is set > to > > > > false somehow, then this is called, it is wrongly set to true. Of course, > if > > > > this is called first, and in its call stack, kArcTermsAccepted is set to > > > false, > > > > in ArcSessionManager it is set to true again. > > > > > > > > In that sense, IMHO, observing kArcTermsAccepted approach sounds better to > > me, > > > > but I'm not strong opinion here as long as the race above is resolved > > cleanly. > > > > > > There is no race condition here. ArcTermsOfServiceScreen sets observer > first. > > > UserSessionManager::InitializeUserSession(). > > > PS. This code would work even with race condition. > > > > Ah, maybe "race" was not a good word choice. Sorry for that. > > But I'm still worried about some points. > > > > 1) Depending on "ArcTermsOfServiceScreen callbacks is called earlier than > > ArcTermsOfServiceOobeNegotiatorForManagedUser" sounds like much fragile to me. > > It is because the registering order and the callback invocation order of > > ObserverList look just implementation details. > > 2) Your code sets kArcTermsAccepted twice, which sounds a source of bug. > > Actually, it is problematic in my scenario above. > > 3) Because ArcSessionManager and OOBE systems are designed to be independent, > so > > life cycle management look unclear to me. > > > > So, if you continue to choose this new observer approach, please address those > > three. > > 1) Get rid of depending the order of same observer callback invocation. > > 2) Do not set kArcTermsAccepted twice per user action. > > 3) Add clear and detailed documentation / comments about life cycle management > > of two independent systems. > > > > Also, please add explicit tests for these to guard from breakage. > > > > Or, alternatively, I still think observing kArcTermsAccepted is an option. > That > > approach does not have those concerns, IIUC. > > # Instead, it is probably necessary to document who sets the preference etc. > > > > WDYT? > > I don't see any problem in order which are they called. To test this I > explicitly changed the order of calling and it still works. > >> 2) Do not set kArcTermsAccepted twice per user action. > removed. > > >> 3) Add clear and detailed documentation / comments about life cycle > management > >> of two independent systems. > added comments in OOBE negotiator. > > >> Also, please add explicit tests for these to guard from breakage. > Sorry, I don't understand about which test are you talking. If this is OOBE test > for Arc pages, then there is TODO to implement such tests and this is big task > outside of this CL. I meant a test for this specific implementation. In order to guarantee; - kArcTermsAccepted is set only once. - lifecycle of actor and negotiator instances. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc:60: if (!profile->GetPrefs()->IsManagedPreference(prefs::kArcEnabled)) On 2016/12/19 17:48:10, khmel wrote: > On 2016/12/19 15:49:44, hidehiko wrote: > > On 2016/12/16 18:37:55, khmel wrote: > > > On 2016/12/16 05:31:51, hidehiko wrote: > > > > Is this check really needed? > > > > > > This prevents calling redundant OnOptInProfileChanged > > > > According to the current implementation and comments, > > it looks designed to be called only when the value is actually changed. > > If not, it sounds like a bug? Who actually calls it? > > # I wish it were explicitly documented... > > > > cf) > > > https://cs.chromium.org/chromium/src/components/prefs/json_pref_store.cc?sq=p... > > > https://cs.chromium.org/chromium/src/components/prefs/in_memory_pref_store.cc... > > > https://cs.chromium.org/chromium/src/services/preferences/public/cpp/pref_obs... > > > > and so on. > > I have no idea at this moment who calls this method. I think this is definitely > out of scope of this CL. Even so changing this is very risky in terms of whole > Chrome project. FYI, we already use this approach here: > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_session_... Even if there is similar code, code change for the unknown case is very danger. Could you investigate and identify the cause, first? https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator_for_managed_user.cc (right): https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator_for_managed_user.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. Could you add unittest for this class, too? (Sorry, for some reason, this was in my draft...) https://codereview.chromium.org/2561023002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2561023002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.h:81: // accepts "Terms Of Service" Wow, thanks! https://codereview.chromium.org/2561023002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2561023002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.cc:210: if (!force_activate && !message_host_) { Please do not depend on message_host_. It's life time management is out of ARC's control. Alternative is using ui_page_ as I suggested in my prev comment, or tracking the state in the ArcSessionManager. https://codereview.chromium.org/2561023002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.h (right): https://codereview.chromium.org/2561023002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.h:55: virtual void OnTermsAccepted(bool is_metrics_enabled, Thank you for the refactoring. Could you make another CL to do this, to make the focus clearer? https://codereview.chromium.org/2561023002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.h:108: void ShowArcLoading(bool force_activate); The flag looks unnecessary complexity to me. Client code can control whether it really needs to show the loading page or not. I guess, you introduced this not to touch the |message_host_| outside of this class, but in this class. However, it is what we should avoid to check the member to decide whether loading page should be shown or not. Please see also my comments in .cc file, too. https://codereview.chromium.org/2561023002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc (right): https://codereview.chromium.org/2561023002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc:59: if (!profile->GetPrefs()->IsManagedPreference(prefs::kArcEnabled)) IIUC, kArcTermsAccepted must be set here, otherwise non managed user case gets broken. If it is set here, to avoid two-times-setting for managed user case, it is necessary to do it in ARC side. Considering the consistency, we should have kArcTermsAccepted set to true before starting management state check in ArcSessionManager. (Maybe we should have DCHECK). For this perspective, ArcTermsOfServiceScreen's callback needs to be called before ARC's. So this introduces implicit dependency (observer callback order) between independent modules, which should be avoided. At the moment, I do not have a clean approach to address this. Note that, one hacky option is to add the observer to the actor in the OnOptInPrefChanged() call stack. However, this also introduces another implicit complexity and dependency, which should be avoided, too. So, proposal: considering we already have many iterations, but it looks not yet clear about how to resolve the above cleanly, could you move back to your original approach, observing pref, in this CL for now, to focus on fixing the current user visible problem (= not to show ToS Opt-In page for OOBE managed user case)? Later, once the problem above gets resolved with this observer approach, let's refactor it. WDYT?
https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc (right): https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc:59: profile->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, accepted); On 2016/12/20 15:20:45, hidehiko wrote: > On 2016/12/19 17:48:10, khmel wrote: > > On 2016/12/19 15:49:44, hidehiko wrote: > > > On 2016/12/16 18:37:55, khmel wrote: > > > > On 2016/12/16 05:31:51, hidehiko wrote: > > > > > Looks a kind of race. > > > > > Because both ArcTermsOfServiceScreen and ArcTermsOfServiceOobeNegotiator > > > > > implement same observer, and there is no guarantee about the order. So, > if > > > the > > > > > negotiator's called first, and in its call stack kArcTermsAccepted is > set > > to > > > > > false somehow, then this is called, it is wrongly set to true. Of > course, > > if > > > > > this is called first, and in its call stack, kArcTermsAccepted is set to > > > > false, > > > > > in ArcSessionManager it is set to true again. > > > > > > > > > > In that sense, IMHO, observing kArcTermsAccepted approach sounds better > to > > > me, > > > > > but I'm not strong opinion here as long as the race above is resolved > > > cleanly. > > > > > > > > There is no race condition here. ArcTermsOfServiceScreen sets observer > > first. > > > > UserSessionManager::InitializeUserSession(). > > > > PS. This code would work even with race condition. > > > > > > Ah, maybe "race" was not a good word choice. Sorry for that. > > > But I'm still worried about some points. > > > > > > 1) Depending on "ArcTermsOfServiceScreen callbacks is called earlier than > > > ArcTermsOfServiceOobeNegotiatorForManagedUser" sounds like much fragile to > me. > > > It is because the registering order and the callback invocation order of > > > ObserverList look just implementation details. > > > 2) Your code sets kArcTermsAccepted twice, which sounds a source of bug. > > > Actually, it is problematic in my scenario above. > > > 3) Because ArcSessionManager and OOBE systems are designed to be > independent, > > so > > > life cycle management look unclear to me. > > > > > > So, if you continue to choose this new observer approach, please address > those > > > three. > > > 1) Get rid of depending the order of same observer callback invocation. > > > 2) Do not set kArcTermsAccepted twice per user action. > > > 3) Add clear and detailed documentation / comments about life cycle > management > > > of two independent systems. > > > > > > Also, please add explicit tests for these to guard from breakage. > > > > > > Or, alternatively, I still think observing kArcTermsAccepted is an option. > > That > > > approach does not have those concerns, IIUC. > > > # Instead, it is probably necessary to document who sets the preference etc. > > > > > > WDYT? > > > > I don't see any problem in order which are they called. To test this I > > explicitly changed the order of calling and it still works. > > >> 2) Do not set kArcTermsAccepted twice per user action. > > removed. > > > > >> 3) Add clear and detailed documentation / comments about life cycle > > management > > >> of two independent systems. > > added comments in OOBE negotiator. > > > > >> Also, please add explicit tests for these to guard from breakage. > > Sorry, I don't understand about which test are you talking. If this is OOBE > test > > for Arc pages, then there is TODO to implement such tests and this is big task > > outside of this CL. > > I meant a test for this specific implementation. In order to guarantee; > - kArcTermsAccepted is set only once. > - lifecycle of actor and negotiator instances. Unit test that validates using OOBE negotiator already added. https://codereview.chromium.org/2561023002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc:60: if (!profile->GetPrefs()->IsManagedPreference(prefs::kArcEnabled)) On 2016/12/20 15:20:45, hidehiko wrote: > On 2016/12/19 17:48:10, khmel wrote: > > On 2016/12/19 15:49:44, hidehiko wrote: > > > On 2016/12/16 18:37:55, khmel wrote: > > > > On 2016/12/16 05:31:51, hidehiko wrote: > > > > > Is this check really needed? > > > > > > > > This prevents calling redundant OnOptInProfileChanged > > > > > > According to the current implementation and comments, > > > it looks designed to be called only when the value is actually changed. > > > If not, it sounds like a bug? Who actually calls it? > > > # I wish it were explicitly documented... > > > > > > cf) > > > > > > https://cs.chromium.org/chromium/src/components/prefs/json_pref_store.cc?sq=p... > > > > > > https://cs.chromium.org/chromium/src/components/prefs/in_memory_pref_store.cc... > > > > > > https://cs.chromium.org/chromium/src/services/preferences/public/cpp/pref_obs... > > > > > > and so on. > > > > I have no idea at this moment who calls this method. I think this is > definitely > > out of scope of this CL. Even so changing this is very risky in terms of whole > > Chrome project. FYI, we already use this approach here: > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_session_... > > Even if there is similar code, code change for the unknown case is very danger. > Could you investigate and identify the cause, first? I think investigation of this conceptual problem is outside of this CL. Please also see ref in my comment above that we have code that already worked with this approach. https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator_for_managed_user.cc (right): https://codereview.chromium.org/2561023002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator_for_managed_user.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/12/20 15:20:45, hidehiko wrote: > Could you add unittest for this class, too? (Sorry, for some reason, this was in > my draft...) I already added unit test that validates using ArcTermsOfServiceInitialOobeNegotiatorForManagedUser https://codereview.chromium.org/2561023002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2561023002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.h:81: // accepts "Terms Of Service" On 2016/12/20 15:20:45, hidehiko wrote: > Wow, thanks! Acknowledged. https://codereview.chromium.org/2561023002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2561023002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.cc:210: if (!force_activate && !message_host_) { On 2016/12/20 15:20:45, hidehiko wrote: > Please do not depend on message_host_. > It's life time management is out of ARC's control. > > Alternative is using ui_page_ as I suggested in my prev comment, or tracking the > state in the ArcSessionManager. Please see code below, L 219, where absent of message_host_ triggers showing the app. https://codereview.chromium.org/2561023002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.h (right): https://codereview.chromium.org/2561023002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.h:55: virtual void OnTermsAccepted(bool is_metrics_enabled, On 2016/12/20 15:20:45, hidehiko wrote: > Thank you for the refactoring. Could you make another CL to do this, to make the > focus clearer? If we are renaming comments in this CL why not to rename methods as well. Otherwise we have to rename both in separate CL. https://codereview.chromium.org/2561023002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.h:108: void ShowArcLoading(bool force_activate); On 2016/12/20 15:20:45, hidehiko wrote: > The flag looks unnecessary complexity to me. Client code can control whether it > really needs to show the loading page or not. > I guess, you introduced this not to touch the |message_host_| outside of this > class, but in this class. However, it is what we should avoid to check the > member to decide whether loading page should be shown or not. > > Please see also my comments in .cc file, too. Responded in cc file. https://codereview.chromium.org/2561023002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc (right): https://codereview.chromium.org/2561023002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc:59: if (!profile->GetPrefs()->IsManagedPreference(prefs::kArcEnabled)) On 2016/12/20 15:20:45, hidehiko wrote: > IIUC, kArcTermsAccepted must be set here, otherwise non managed user case gets > broken. > > If it is set here, to avoid two-times-setting for managed user case, > it is necessary to do it in ARC side. > > Considering the consistency, we should have kArcTermsAccepted set to true before > starting management state check in ArcSessionManager. (Maybe we should have > DCHECK). > > For this perspective, ArcTermsOfServiceScreen's callback needs to be called > before ARC's. So this introduces implicit dependency (observer callback order) > between independent modules, which should be avoided. > > At the moment, I do not have a clean approach to address this. > Note that, one hacky option is to add the observer to the actor in the > OnOptInPrefChanged() call stack. However, this also introduces another implicit > complexity and dependency, which should be avoided, too. > > So, proposal: considering we already have many iterations, but it looks not yet > clear about how to resolve the above cleanly, could you move back to your > original approach, observing pref, in this CL for now, to focus on fixing the > current user visible problem (= not to show ToS Opt-In page for OOBE managed > user case)? > Later, once the problem above gets resolved with this observer approach, let's > refactor it. > > WDYT? >> If it is set here, to avoid two-times-setting for managed user ?? case, >> it is necessary to do it in ARC side. FYI, kArcTermsAccepted is set in voidrcSessionManager::OnTermsOfServiceNegotiated(bool accepted) { And yes, I tested this code and managed case is not broken in real life. Observer approach has problem, if user presses 'Skip' kArcTermsAccepted is not set and pref should not be called, right? Which event is useful for this case? In this CL you request me to investigate why pref is called if we don't change actual value but here you request me to rely on this? I don't want to implement code that relies on signaling that pref was changed from "false default" to "false".
elijahtaylor@chromium.org changed reviewers: + elijahtaylor@chromium.org
It feels like there is some wheel spinning going on in this CL at this point. The bulk of the comments have been addressed, and the remaining ones are debatable. It might have been better to take this offline earlier to discuss instead of changing directions a couple times mid-CL, if there really are design issues with it. Can I suggest we focus on the very core of this CL functionality- and complexity-wise, to make sure we're doing the right thing there, and leave naming, nits, and matters of preference behind? If the code works and does not regress functionality, and does not increase complexity of this system substantially, I'd like to submit it more or less as is. @hidehiko, can you make a (hopefully final) pass with this in mind? Feel free to ping me offline to discuss if you need to.
Hi,
I refactored code after our meeting as agreed.
* Solution is based on PS #5 where OOBE negotiator is
introduced for managed/unmanaged users.
* arc.enabled pref is set on starting Arc OOBE OptIn in
order to match logic in ArcSessionManager
* Detect that UI is not shown based on current page
instead of attached host
---Additionally------
* arc_session_manager_unittest.cc is taken from PS #10
and extended to support both cases, managed/unmanaged.
* Renamed unit tests
arc_terms_of_service_default_negotiator_unittest.cc
* ArcSessionManagerDataRemovedWaiter is extracted from
browser tests to separate class to share functionality.
* I discarded some renaming because proper renaming
involves more files and better to address this in
separate CL(s).
PTAL
Thanks
The CQ bit was checked by khmel@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.
Thank you for clean up. As we discussed, the direction looks much nicer to me! Comments are mostly minor coding stuff, and/or more documentation and comments. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/BUILD.gn (right): https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/BUILD.gn:1465: "arc/arc_session_manager_data_removed_waiter.cc", Please make a library for testing, and link it from unit_tests and browser_tests, instead of adding the source to both. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:87: bool IsOobeOptInActive() { Could you add a unittest for this method? I think it's ok to expose it for testing via ArcSessionManager with comments. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:95: chromeos::LoginDisplayHost* host = chromeos::LoginDisplayHost::default_host(); nit: if (!chromeos::LoginDisplayHost::default_host()) return false; return true; https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:678: // Need user's explicit Terms Of Service agreement. Prevent race condition Could you elaborate the race condition more? For what case/what scenario do you want to change this behavior? https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:848: if (!IsOobeOptInActive()) { nit: Let's reduce the indent level. if (IsOobeOptInActive()) { ... } else if (support_host_) { ... } if (negotiator_) { ... } https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager_data_removed_waiter.cc (right): https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_data_removed_waiter.cc:20: run_loop_.reset(new base::RunLoop); run_loop_ = base::MakeUnique<RunLoop>(); cf) Please refer our style guide Luis sent. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager_data_removed_waiter.h (right): https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_data_removed_waiter.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. Could you mkdir c/b/c/arc/test directory, and put this there, as we do same thing for components/arc? https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_data_removed_waiter.h:20: class ArcSessionManagerDataRemovedWaiter : public ArcSessionManager::Observer { Optional: the name looks long now. How about ArcDataRemovedWaiter? ArcSessionManager is related but it looks the implementation details to me. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_data_removed_waiter.h:25: void Wait(); Could you document? https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:61: class FakeLoginDisplayHost : public chromeos::MockLoginDisplayHost { In Chrome, AFAIK, "Fake" is used meaning the light-weight real implementation. Mock is used in slightly more various way, but in many cases, including this case, gMock instance. Please get rid of Mock inheritance here. Please see also my comments below about gMock usage. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:79: class ArcSessionManagerTestBase : public testing::TestWithParam<bool> { Because the param is not used in this base, please document what it is for. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:573: fake_login_display_host_.reset(new FakeLoginDisplayHost()); MakeUnique, please. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:607: if (IsManagedUser()) Could you comment why managed user does not need to wait for data removal? https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:630: profile()->GetPrefs()->SetBoolean(prefs::kArcEnabled, true); Setting the preference and the Show() method name does not directly linked in general. Could you comment what are you emulating? https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:637: std::unique_ptr<FakeLoginDisplayHost> fake_login_display_host_; This is only for let default_host() return true, right? Then, using gMock here looks overkill. Also, I'm much worried about the longer term maintenance. Using gMock sometimes provides benefit for testing, specifically if the interface is well fixed. However, in Chrome, we edit many classes/interfaces as we want, and Mock sometimes makes such a maintenance difficult practically. I strongly recommend to avoid gMock unless it is really necessary. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:647: arc_session_manager()->OnPrimaryUserProfilePrepared(profile()); L647 - L649 looks a part of SetUp() and L656 looks a part of TearDown(). Ditto for below case. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h (right): https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h:19: // Invokes the|callback| asynchronously with "|agreed| = true" if user accepts nit: s/agreed/accepted/g (for only what you actually touched to focus on the CL's goal. I know you just moved, though). https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h:32: // Performs implementation specific action on starting negotiation. Maybe: s/on starting negotiation/to start negotiation/ ? https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.cc (right): https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.cc:15: chromeos::ArcTermsOfServiceScreenActor* actor_for_testing = nullptr; nit: please insert an empty line above. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.cc:50: if (screen_actor) What situation |screen_actor| gets nullptr? IIUC, when the actor gets destroyed, OnActorDestroyed() is called in its dtor. Thus, it looks always non-null. If there is the case, please comment when it happens. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h (right): https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h:26: static void SetArcTermsOfServiceScreenActorForTesting( Document? https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h:30: void HandleTermsAccepted(bool accepted); Document? https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/arc_terms_of_service_screen_handler.cc (right): https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/arc_terms_of_service_screen_handler.cc:155: // Enable ARC to match ArcSessionManager logic. Could you add more detailed comment? E.g., - listing example scenarios that do not match the expected ArcSessionManager. - What situations need to be take care about. - Why setting the preference works. etc.? We spent the discussion to figure out a good approach. So documenting it should be beneficial for future maintainers. If comment looks not a good way, a design doc is an alternative place to be written. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/arc_terms_of_service_screen_handler.h (right): https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/arc_terms_of_service_screen_handler.h:65: base::ObserverList<ArcTermsOfServiceScreenActorObserver> observer_list_; Can this be check_empty = true for better graceful shutdown check? https://cs.chromium.org/chromium/src/base/observer_list.h?q=base::ObserverLis...
Hi Hidehiko Thanks for comments PTAL https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/BUILD.gn (right): https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/BUILD.gn:1465: "arc/arc_session_manager_data_removed_waiter.cc", On 2017/01/30 10:12:35, hidehiko wrote: > Please make a library for testing, and link it from unit_tests and > browser_tests, instead of adding the source to both. Done. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:87: bool IsOobeOptInActive() { On 2017/01/30 10:12:35, hidehiko wrote: > Could you add a unittest for this method? > I think it's ok to expose it for testing via ArcSessionManager with comments. Done. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:95: chromeos::LoginDisplayHost* host = chromeos::LoginDisplayHost::default_host(); On 2017/01/30 10:12:35, hidehiko wrote: > nit: > > if (!chromeos::LoginDisplayHost::default_host()) > return false; > return true; Done. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:678: // Need user's explicit Terms Of Service agreement. Prevent race condition On 2017/01/30 10:12:35, hidehiko wrote: > Could you elaborate the race condition more? > For what case/what scenario do you want to change this behavior? This legacy problem actually. It is less visible in current implementation because of user iteration. With this implementation, when we set arc.enabled on OOBE show this is reproducible in 100%. We have code OnIsSyncingChanged which is called when profile is synced. From this callback we call OnOptInPreferenceChanged. syncing profile is async method which happens after initializing arc session manager. So following happens in case OOBE flow: * Initialize ArcSessionManager::OnPrimary... * Show Arc OOBE * Set arc.enabled - OnOptInPreferenceChanged is called. * ARC OOBE is shown to user. * Profile is synced, OnIsSyncingChanged called and OnOptInPreferenceChanged is called from it. This is second call of OnOptInPreferenceChanged and without this fix we call StartTermsOfServiceNegotiation twice which is wrong. Non-OOBE case. 1. Initialize ArcSessionManager::OnPrimary... 2. Profile is synced, OnIsSyncingChanged called but OnOptInPreferenceChanged is not called because arc.enabled is not set. 3. User activates Arc, let say via settings. arc.enabled is set and OnOptInPreferenceChanged is called and StartTermsOfServiceNegotiation is called once only. Next is variation of Non-OOBE case. 2a. User activates Arc before OnIsSyncingChanged. OnOptInPreferenceChanged is called and we show OptIn. 3a. While Terms are shown profile is actually synced and OnIsSyncingChanged called. Now we have similar problem with OOBE case. Last case is quite rare but I personally saw it few times. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:848: if (!IsOobeOptInActive()) { On 2017/01/30 10:12:35, hidehiko wrote: > nit: Let's reduce the indent level. > > if (IsOobeOptInActive()) { > ... > } else if (support_host_) { > ... > } > > if (negotiator_) { > ... > } Done. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager_data_removed_waiter.cc (right): https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_data_removed_waiter.cc:20: run_loop_.reset(new base::RunLoop); On 2017/01/30 10:12:35, hidehiko wrote: > run_loop_ = base::MakeUnique<RunLoop>(); > cf) Please refer our style guide Luis sent. Done. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager_data_removed_waiter.h (right): https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_data_removed_waiter.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/01/30 10:12:35, hidehiko wrote: > Could you mkdir c/b/c/arc/test directory, and put this there, > as we do same thing for components/arc? Done. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_data_removed_waiter.h:20: class ArcSessionManagerDataRemovedWaiter : public ArcSessionManager::Observer { On 2017/01/30 10:12:35, hidehiko wrote: > Optional: the name looks long now. How about ArcDataRemovedWaiter? > ArcSessionManager is related but it looks the implementation details to me. Done. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_data_removed_waiter.h:25: void Wait(); On 2017/01/30 10:12:35, hidehiko wrote: > Could you document? Done. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:61: class FakeLoginDisplayHost : public chromeos::MockLoginDisplayHost { On 2017/01/30 10:12:35, hidehiko wrote: > In Chrome, AFAIK, "Fake" is used meaning the light-weight real implementation. > Mock is used in slightly more various way, but in many cases, including this > case, gMock instance. > Please get rid of Mock inheritance here. > > Please see also my comments below about gMock usage. Done. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:79: class ArcSessionManagerTestBase : public testing::TestWithParam<bool> { On 2017/01/30 10:12:35, hidehiko wrote: > Because the param is not used in this base, please document what it is for. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:573: fake_login_display_host_.reset(new FakeLoginDisplayHost()); On 2017/01/30 10:12:35, hidehiko wrote: > MakeUnique, please. Done. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:607: if (IsManagedUser()) On 2017/01/30 10:12:35, hidehiko wrote: > Could you comment why managed user does not need to wait for data removal? Done. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:630: profile()->GetPrefs()->SetBoolean(prefs::kArcEnabled, true); On 2017/01/30 10:12:35, hidehiko wrote: > Setting the preference and the Show() method name does not directly linked in > general. > Could you comment what are you emulating? Done. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:637: std::unique_ptr<FakeLoginDisplayHost> fake_login_display_host_; On 2017/01/30 10:12:35, hidehiko wrote: > This is only for let default_host() return true, right? > Then, using gMock here looks overkill. > Also, I'm much worried about the longer term maintenance. > Using gMock sometimes provides benefit for testing, specifically if the > interface is well fixed. > However, in Chrome, we edit many classes/interfaces as we want, and Mock > sometimes makes such a maintenance difficult practically. > I strongly recommend to avoid gMock unless it is really necessary. Done. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:647: arc_session_manager()->OnPrimaryUserProfilePrepared(profile()); On 2017/01/30 10:12:35, hidehiko wrote: > L647 - L649 looks a part of SetUp() and L656 looks a part of TearDown(). > Ditto for below case. Done. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h (right): https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h:19: // Invokes the|callback| asynchronously with "|agreed| = true" if user accepts On 2017/01/30 10:12:35, hidehiko wrote: > nit: s/agreed/accepted/g (for only what you actually touched to focus on the > CL's goal. I know you just moved, though). Done. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h:32: // Performs implementation specific action on starting negotiation. On 2017/01/30 10:12:36, hidehiko wrote: > Maybe: s/on starting negotiation/to start negotiation/ ? Done. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.cc (right): https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.cc:15: chromeos::ArcTermsOfServiceScreenActor* actor_for_testing = nullptr; On 2017/01/30 10:12:36, hidehiko wrote: > nit: please insert an empty line above. Done. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.cc:50: if (screen_actor) On 2017/01/30 10:12:36, hidehiko wrote: > What situation |screen_actor| gets nullptr? > IIUC, when the actor gets destroyed, OnActorDestroyed() is called in its dtor. > Thus, it looks always non-null. > If there is the case, please comment when it happens. Done. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h (right): https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h:26: static void SetArcTermsOfServiceScreenActorForTesting( On 2017/01/30 10:12:36, hidehiko wrote: > Document? Done. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h:30: void HandleTermsAccepted(bool accepted); On 2017/01/30 10:12:36, hidehiko wrote: > Document? Done. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/arc_terms_of_service_screen_handler.cc (right): https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/arc_terms_of_service_screen_handler.cc:155: // Enable ARC to match ArcSessionManager logic. On 2017/01/30 10:12:36, hidehiko wrote: > Could you add more detailed comment? > E.g., > - listing example scenarios that do not match the expected ArcSessionManager. > - What situations need to be take care about. > - Why setting the preference works. > etc.? > > We spent the discussion to figure out a good approach. So documenting it should > be beneficial for future maintainers. > > If comment looks not a good way, a design doc is an alternative place to be > written. Done. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/arc_terms_of_service_screen_handler.h (right): https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/arc_terms_of_service_screen_handler.h:65: base::ObserverList<ArcTermsOfServiceScreenActorObserver> observer_list_; On 2017/01/30 10:12:36, hidehiko wrote: > Can this be check_empty = true for better graceful shutdown check? > https://cs.chromium.org/chromium/src/base/observer_list.h?q=base::ObserverLis... Done.
The CQ bit was checked by khmel@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by khmel@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.
Thank you for update. With your new comment, I believe it's much more understandable! Mostly minor comments. https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:678: // Need user's explicit Terms Of Service agreement. Prevent race condition On 2017/01/31 02:47:08, khmel wrote: > On 2017/01/30 10:12:35, hidehiko wrote: > > Could you elaborate the race condition more? > > For what case/what scenario do you want to change this behavior? > > This legacy problem actually. It is less visible in current implementation > because of user iteration. With this implementation, when we set arc.enabled on > OOBE show this is reproducible in 100%. We have code OnIsSyncingChanged which is > called when profile is synced. From this callback we call > OnOptInPreferenceChanged. syncing profile is async > method which happens after initializing arc session manager. > > So following happens in case OOBE flow: > * Initialize ArcSessionManager::OnPrimary... > * Show Arc OOBE > * Set arc.enabled - OnOptInPreferenceChanged is called. > * ARC OOBE is shown to user. > * Profile is synced, OnIsSyncingChanged called and OnOptInPreferenceChanged is > called from it. This is second call of OnOptInPreferenceChanged and without this > fix we call StartTermsOfServiceNegotiation twice which is wrong. > > Non-OOBE case. > 1. Initialize ArcSessionManager::OnPrimary... > 2. Profile is synced, OnIsSyncingChanged called but OnOptInPreferenceChanged is > not called because arc.enabled is not set. > 3. User activates Arc, let say via settings. arc.enabled is set and > OnOptInPreferenceChanged is called and StartTermsOfServiceNegotiation is called > once only. > > Next is variation of Non-OOBE case. > 2a. User activates Arc before OnIsSyncingChanged. OnOptInPreferenceChanged is > called and we show OptIn. > 3a. While Terms are shown profile is actually synced and OnIsSyncingChanged > called. Now we have similar problem with OOBE case. > > Last case is quite rare but I personally saw it few times. Thank you for detailed explanation! It's much clearer to me. So, what we need actually is checking no-dup call of OnOptInPreferenceChanged. At the moment, I'm fine with your current change for short term solution only for OOBE, but we need to fix the problem from root cause. Could you file a bug (with your explanation above) and add TODO(crbug.com/...)? https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:599: void SetUp() override { ArcSessionManagerTest::SetUp(); } nit: SetUp()/TearDown() do just calling parent methods. Could you remove? https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:622: // OOBE OptIn is Active in case of OOBE is started for new user and ARC OOBE nit: s/Active/active/ https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:647: AppendEnableArcOOBEOptInSwitch(); In common practice, parent's SetUp() should be called at first. Could you comment why you need to run this before parent's SetUp()? https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.cc (right): https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.cc:16: chromeos::ArcTermsOfServiceScreenActor* actor_for_testing = nullptr; Oops, sorry I overlooked in prev iteration... Global var name should start with "g_" prefix. So g_actor_for_testing. https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.cc:24: if (!host) Considering the DCHECK at L47, this should not happen. Could you DCHECK here, too? https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h (right): https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h:26: // Overrides ARC OOBE screen handler in unit tests, where OOBE UI is not nit: "overrdies" is used for method overriding in C++ context in most cases. How about "Inject ARC ..." ? https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h:32: // Helper to handle callbacks from This is what the method "is", but from future reader's point of view, what the method "does" (or "how" the method works) is more important. Cold you comment so? Also, in the latest PS, you introduced a new restriction that once StartNegotiationImpl() is called, then HandleTermsAccepted() must be called before the instance is destroyed. Could you comment it, too? https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h:45: // moment of initialization. However in case user signs out while ARC OOBE nit: initialization implies ctor. Maybe: "... is available on starting negotiation"? https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h:48: // chromeos::ArcTermsOfServiceScreenActor in last case. Solution is to keep I was confused a bit here, because the pointer to the Actor instance is passed to OnActorDestroyed. IIUC, you keep the raw ptr just for accessing to the Actor instance in a unified way (regardless of in On{Skip,Accept,ActorDestroyed}())?
Thank you for comments. PTAL Thanks https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2561023002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:678: // Need user's explicit Terms Of Service agreement. Prevent race condition On 2017/01/31 13:55:30, hidehiko wrote: > On 2017/01/31 02:47:08, khmel wrote: > > On 2017/01/30 10:12:35, hidehiko wrote: > > > Could you elaborate the race condition more? > > > For what case/what scenario do you want to change this behavior? > > > > This legacy problem actually. It is less visible in current implementation > > because of user iteration. With this implementation, when we set arc.enabled > on > > OOBE show this is reproducible in 100%. We have code OnIsSyncingChanged which > is > > called when profile is synced. From this callback we call > > OnOptInPreferenceChanged. syncing profile is async > > method which happens after initializing arc session manager. > > > > So following happens in case OOBE flow: > > * Initialize ArcSessionManager::OnPrimary... > > * Show Arc OOBE > > * Set arc.enabled - OnOptInPreferenceChanged is called. > > * ARC OOBE is shown to user. > > * Profile is synced, OnIsSyncingChanged called and OnOptInPreferenceChanged > is > > called from it. This is second call of OnOptInPreferenceChanged and without > this > > fix we call StartTermsOfServiceNegotiation twice which is wrong. > > > > Non-OOBE case. > > 1. Initialize ArcSessionManager::OnPrimary... > > 2. Profile is synced, OnIsSyncingChanged called but OnOptInPreferenceChanged > is > > not called because arc.enabled is not set. > > 3. User activates Arc, let say via settings. arc.enabled is set and > > OnOptInPreferenceChanged is called and StartTermsOfServiceNegotiation is > called > > once only. > > > > Next is variation of Non-OOBE case. > > 2a. User activates Arc before OnIsSyncingChanged. OnOptInPreferenceChanged is > > called and we show OptIn. > > 3a. While Terms are shown profile is actually synced and OnIsSyncingChanged > > called. Now we have similar problem with OOBE case. > > > > Last case is quite rare but I personally saw it few times. > > Thank you for detailed explanation! It's much clearer to me. > So, what we need actually is checking no-dup call of OnOptInPreferenceChanged. > At the moment, I'm fine with your current change for short term solution only > for OOBE, but we need to fix the problem from root cause. > Could you file a bug (with your explanation above) and add TODO(crbug.com/...)? Done. https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:599: void SetUp() override { ArcSessionManagerTest::SetUp(); } On 2017/01/31 13:55:30, hidehiko wrote: > nit: SetUp()/TearDown() do just calling parent methods. Could you remove? Done. https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:622: // OOBE OptIn is Active in case of OOBE is started for new user and ARC OOBE On 2017/01/31 13:55:30, hidehiko wrote: > nit: s/Active/active/ Done. https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:647: AppendEnableArcOOBEOptInSwitch(); On 2017/01/31 13:55:30, hidehiko wrote: > In common practice, parent's SetUp() should be called at first. > Could you comment why you need to run this before parent's SetUp()? Moved down, this is not principal here. It is common in unit_tests to set flags before calling base class. https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.cc (right): https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.cc:16: chromeos::ArcTermsOfServiceScreenActor* actor_for_testing = nullptr; On 2017/01/31 13:55:30, hidehiko wrote: > Oops, sorry I overlooked in prev iteration... > Global var name should start with "g_" prefix. So g_actor_for_testing. I got many comments from other reviewers that g_ is not required when placed in namespace Done as you suggest. https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.cc:24: if (!host) On 2017/01/31 13:55:30, hidehiko wrote: > Considering the DCHECK at L47, this should not happen. > Could you DCHECK here, too? Done. https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h (right): https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h:26: // Overrides ARC OOBE screen handler in unit tests, where OOBE UI is not On 2017/01/31 13:55:30, hidehiko wrote: > nit: "overrdies" is used for method overriding in C++ context in most cases. How > about "Inject ARC ..." ? Done. https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h:32: // Helper to handle callbacks from On 2017/01/31 13:55:30, hidehiko wrote: > This is what the method "is", but from future reader's point of view, what the > method "does" (or "how" the method works) is more important. Cold you comment > so? > > Also, in the latest PS, you introduced a new restriction that once > StartNegotiationImpl() is called, then HandleTermsAccepted() must be called > before the instance is destroyed. Could you comment it, too? Done. https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h:45: // moment of initialization. However in case user signs out while ARC OOBE On 2017/01/31 13:55:30, hidehiko wrote: > nit: initialization implies ctor. Maybe: "... is available on starting > negotiation"? Done. https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h:48: // chromeos::ArcTermsOfServiceScreenActor in last case. Solution is to keep On 2017/01/31 13:55:30, hidehiko wrote: > I was confused a bit here, because the pointer to the Actor instance is passed > to OnActorDestroyed. > IIUC, you keep the raw ptr just for accessing to the Actor instance in a unified > way (regardless of in On{Skip,Accept,ActorDestroyed}())? Yes, it is true. It is also a DCHECK that actors match in OnActorDestroyed.
LGTM! https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h (right): https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h:48: // chromeos::ArcTermsOfServiceScreenActor in last case. Solution is to keep On 2017/01/31 15:25:22, khmel wrote: > On 2017/01/31 13:55:30, hidehiko wrote: > > I was confused a bit here, because the pointer to the Actor instance is passed > > to OnActorDestroyed. > > IIUC, you keep the raw ptr just for accessing to the Actor instance in a > unified > > way (regardless of in On{Skip,Accept,ActorDestroyed}())? > > Yes, it is true. It is also a DCHECK that actors match in OnActorDestroyed. Ok, then the comment is not reflecting the actual situation, because there is a way to obtain the Actor (via passed as an argument). Maybe something like; If a user signs out while ARC OOBE opt-in is active, LoginDisplayHost is detached first then OnActorDestroyed is called. It means, in OnSkip() and OnAccept(), the Actor needs to be obtained via LoginDisplayHost, but in OnActorDestroyed(), the argument needs to be used. In order to use the same way to access the Actor, remember the pointer in StartNegotiationImpl(), and reset in HandleTermsAccepted(). https://codereview.chromium.org/2561023002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h (right): https://codereview.chromium.org/2561023002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h:33: // chromeos::ArcTermsOfServiceScreenActorObserver. It dispatches |accepted| nit: It removes observer from |screen_actor_|, resets it, and then dispatches |accepted|. (to be ordered).
Thank you for review! https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h (right): https://codereview.chromium.org/2561023002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h:48: // chromeos::ArcTermsOfServiceScreenActor in last case. Solution is to keep On 2017/01/31 15:47:09, hidehiko wrote: > On 2017/01/31 15:25:22, khmel wrote: > > On 2017/01/31 13:55:30, hidehiko wrote: > > > I was confused a bit here, because the pointer to the Actor instance is > passed > > > to OnActorDestroyed. > > > IIUC, you keep the raw ptr just for accessing to the Actor instance in a > > unified > > > way (regardless of in On{Skip,Accept,ActorDestroyed}())? > > > > Yes, it is true. It is also a DCHECK that actors match in OnActorDestroyed. > > Ok, then the comment is not reflecting the actual situation, because there is a > way to obtain the Actor (via passed as an argument). > > Maybe something like; > > If a user signs out while ARC OOBE opt-in is active, LoginDisplayHost is > detached first then OnActorDestroyed is called. > It means, in OnSkip() and OnAccept(), the Actor needs to be obtained via > LoginDisplayHost, but in OnActorDestroyed(), the argument needs to be used. > In order to use the same way to access the Actor, remember the pointer in > StartNegotiationImpl(), and reset in HandleTermsAccepted(). Done. https://codereview.chromium.org/2561023002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h (right): https://codereview.chromium.org/2561023002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h:33: // chromeos::ArcTermsOfServiceScreenActorObserver. It dispatches |accepted| On 2017/01/31 15:47:09, hidehiko wrote: > nit: It removes observer from |screen_actor_|, resets it, and then dispatches > |accepted|. > (to be ordered). Done.
The CQ bit was checked by khmel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, hidehiko@chromium.org Link to the patchset: https://codereview.chromium.org/2561023002/#ps280001 (title: "rebased + comments updated")
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": 280001, "attempt_start_ts": 1485880437165500,
"parent_rev": "2880cd2941fe8ea000741b846a20413e50630bcc", "commit_rev":
"8d4da8a74f81f9cd097a12d4a77e5c8839a0d958"}
Message was sent while issue was closed.
Description was changed from
==========
arc: ARC loading progress should not be shown when started from OOBE.
This fix regression (also compared with M56) when ARC loading progress
is shown in case ARC is accepted from OOBE. In this case ARC is
supposed to start silently, showing UI only if user interaction is
required. For example press Sign-In button or in case of an error.
This also handles the edge case for managed ARC user. In this case
Opt UI is shown on PrimaryProfilePrepared stage and stays behind
the OOBE UI. This CL closes this redundant UI in case user accepts
ARC in OOBE.
BUG=b/33431526
TEST=Manually on device, managed/unmanaged user. Accept Arc in OOBE.
In case silent sign-in is enabled, no UI is shown and Play Store
starts once Arc is booted. In case silent sign-in is disabled
UI is shown when LSO is requested.
Start Arc manually from settings. Start page is shown. Accept Arc
and wait for LSO page is shown. Sign-out and sign-in again.
Arc continues from showing Arc loading progress.
==========
to
==========
arc: ARC loading progress should not be shown when started from OOBE.
This fix regression (also compared with M56) when ARC loading progress
is shown in case ARC is accepted from OOBE. In this case ARC is
supposed to start silently, showing UI only if user interaction is
required. For example press Sign-In button or in case of an error.
This also handles the edge case for managed ARC user. In this case
Opt UI is shown on PrimaryProfilePrepared stage and stays behind
the OOBE UI. This CL closes this redundant UI in case user accepts
ARC in OOBE.
BUG=b/33431526
TEST=Manually on device, managed/unmanaged user. Accept Arc in OOBE.
In case silent sign-in is enabled, no UI is shown and Play Store
starts once Arc is booted. In case silent sign-in is disabled
UI is shown when LSO is requested.
Start Arc manually from settings. Start page is shown. Accept Arc
and wait for LSO page is shown. Sign-out and sign-in again.
Arc continues from showing Arc loading progress.
Review-Url: https://codereview.chromium.org/2561023002
Cr-Commit-Position: refs/heads/master@{#447267}
Committed:
https://chromium.googlesource.com/chromium/src/+/8d4da8a74f81f9cd097a12d4a77e...
==========
Message was sent while issue was closed.
Committed patchset #14 (id:280001) as https://chromium.googlesource.com/chromium/src/+/8d4da8a74f81f9cd097a12d4a77e... |
