chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc - Issue 2561023002: arc: ARC loading progress should not be shown when started from OOBE.

Unified Diff: chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc

Issue 2561023002: arc: ARC loading progress should not be shown when started from OOBE. (Closed)

Patch Set: cleanup Created 4 years ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

« chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.cc ('K') | « chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.h ('k') | chrome/browser/chromeos/login/screens/arc_terms_of_service_screen_actor.h » ('j') | chrome/browser/chromeos/login/screens/arc_terms_of_service_screen_actor_observer.h » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc

diff --git a/chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc b/chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc

index 1da3072521b8c3086275456054483620c1aaf631..2c8b448ef2454ac6057379d307cfa3b5e7f6b301 100644

--- a/chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc

+++ b/chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc

@@ -4,6 +4,7 @@

#include "chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.h"

+#include "chrome/browser/chromeos/login/screens/arc_terms_of_service_screen_actor.h"

#include "chrome/browser/chromeos/login/screens/base_screen_delegate.h"

#include "chrome/browser/chromeos/login/wizard_controller.h"

#include "chrome/browser/metrics/metrics_reporting_state.h"

@@ -20,12 +21,12 @@ ArcTermsOfServiceScreen::ArcTermsOfServiceScreen(

: BaseScreen(base_screen_delegate), actor_(actor) {

DCHECK(actor_);

if (actor_)

- actor_->SetDelegate(this);

+ actor_->AddObserver(this);

}

ArcTermsOfServiceScreen::~ArcTermsOfServiceScreen() {

if (actor_)

- actor_->SetDelegate(nullptr);

+ actor_->RemoveObserver(this);

}

void ArcTermsOfServiceScreen::Show() {

@@ -56,7 +57,8 @@ void ArcTermsOfServiceScreen::OnAccept() {

void ArcTermsOfServiceScreen::ApplyTerms(bool accepted) {

Profile* profile = ProfileManager::GetActiveUserProfile();

profile->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, accepted);

hidehiko 2016/12/16 05:31:51 Looks a kind of race. Because both ArcTermsOfServi

khmel 2016/12/16 18:37:55 There is no race condition here. ArcTermsOfService

hidehiko 2016/12/19 15:49:44 Ah, maybe "race" was not a good word choice. Sorry

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?

khmel 2016/12/19 17:48:10 I don't see any problem in order which are they ca

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.

removed.

added comments in OOBE negotiator.

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.

hidehiko 2016/12/20 15:20:45 I meant a test for this specific implementation. I

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.

khmel 2016/12/20 16:27:57 Unit test that validates using OOBE negotiator alr

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.

- profile->GetPrefs()->SetBoolean(prefs::kArcEnabled, accepted);

+ if (!profile->GetPrefs()->IsManagedPreference(prefs::kArcEnabled))

hidehiko 2016/12/16 05:31:51 Is this check really needed?

khmel 2016/12/16 18:37:55 This prevents calling redundant OnOptInProfileChan

hidehiko 2016/12/19 15:49:44 According to the current implementation and commen

khmel 2016/12/19 17:48:10 I have no idea at this moment who calls this metho

hidehiko 2016/12/20 15:20:45 Even if there is similar code, code change for the

khmel 2016/12/20 16:27:57 I think investigation of this conceptual problem i

+ profile->GetPrefs()->SetBoolean(prefs::kArcEnabled, accepted);

Finish(BaseScreenDelegate::ARC_TERMS_OF_SERVICE_FINISHED);

}