Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
971383002:
Removing FRE UI for unicorn accounts. (Closed)
|
Created:
5 years, 9 months ago by merkulova Modified:
5 years, 9 months ago CC:
chromium-reviews, pam+watch_chromium.org, dzhioev+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDraft for removing FRE UI for unicorn accounts.
Subscribed UserSessionManager for ChildAccountService for waiting for flags-fetching. As long as info available or timeout expired, we make the decision on show/no show FRE UI.
BUG=463088
Committed: https://crrev.com/446a405c524d369155368d339b4ae29c428d6b22
Cr-Commit-Position: refs/heads/master@{#320062}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Changing the observer. #
Total comments: 28
Patch Set 3 : Added preference. Comments addressed. #
Total comments: 3
Patch Set 4 : Null checks removed. #
Total comments: 2
Patch Set 5 : Clean-up. #
Total comments: 14
Patch Set 6 : Switched to callback from observer. #
Total comments: 10
Patch Set 7 : Comments addressed. #
Total comments: 6
Patch Set 8 : Renames and comments adding. #Patch Set 9 : Rebase. #
Messages
Total messages: 33 (7 generated)
merkulova@chromium.org changed reviewers: + antrim@chromium.org, treib@chromium.org
Let's discuss what logic is better for our use-case.
I think the basic approach is fine - we'll have to wait until we know whether it's a child account. First round of comments below. https://codereview.chromium.org/971383002/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/971383002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/session/user_session_manager.cc:1118: AddFlagFetchingObserver(this); Could the "is child account" status already be known here? Then you wouldn't get any notification from the CAService. https://codereview.chromium.org/971383002/diff/1/chrome/browser/supervised_us... File chrome/browser/supervised_user/child_accounts/child_account_service.cc (right): https://codereview.chromium.org/971383002/diff/1/chrome/browser/supervised_us... chrome/browser/supervised_user/child_accounts/child_account_service.cc:321: FOR_EACH_OBSERVER(FlagsFetchingObserver, observer_list_, I'd only call the observers if is_child_account actually changed (so probably move this into SetIsChildAccount). https://codereview.chromium.org/971383002/diff/1/chrome/browser/supervised_us... File chrome/browser/supervised_user/child_accounts/flags_fetching_observer.h (right): https://codereview.chromium.org/971383002/diff/1/chrome/browser/supervised_us... chrome/browser/supervised_user/child_accounts/flags_fetching_observer.h:8: class FlagsFetchingObserver { I'd call this something like ChildAccountStatusObserver, and have only a single ChildAccountStatusChanged(bool is_child) method. All the other methods don't really tell the observer anything useful. Also, nit: indentation is wrong in this file.
I'll make the rename, let's discuss the logic part for now. https://codereview.chromium.org/971383002/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/971383002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/session/user_session_manager.cc:1118: AddFlagFetchingObserver(this); On 2015/03/04 09:59:15, Marc Treib wrote: > Could the "is child account" status already be known here? Then you wouldn't get > any notification from the CAService. Yes, it might be already know. Then we wait for the timeout. Then the FRE UI decision is processed based on user status (which was set much earlier) and it's ok. https://codereview.chromium.org/971383002/diff/1/chrome/browser/supervised_us... File chrome/browser/supervised_user/child_accounts/child_account_service.cc (right): https://codereview.chromium.org/971383002/diff/1/chrome/browser/supervised_us... chrome/browser/supervised_user/child_accounts/child_account_service.cc:321: FOR_EACH_OBSERVER(FlagsFetchingObserver, observer_list_, On 2015/03/04 09:59:15, Marc Treib wrote: > I'd only call the observers if is_child_account actually changed (so probably > move this into SetIsChildAccount). But we're actually waiting for the fetching to complete. No matter if the status change happened. https://codereview.chromium.org/971383002/diff/1/chrome/browser/supervised_us... File chrome/browser/supervised_user/child_accounts/flags_fetching_observer.h (right): https://codereview.chromium.org/971383002/diff/1/chrome/browser/supervised_us... chrome/browser/supervised_user/child_accounts/flags_fetching_observer.h:8: class FlagsFetchingObserver { On 2015/03/04 09:59:15, Marc Treib wrote: > I'd call this something like ChildAccountStatusObserver, and have only a single > ChildAccountStatusChanged(bool is_child) method. All the other methods don't > really tell the observer anything useful. > > Also, nit: indentation is wrong in this file. they say that fetching stopped (for any reason). Do you think it's not necessary for FRE decision? Ack regarding naming.
https://codereview.chromium.org/971383002/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/971383002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/session/user_session_manager.cc:1118: AddFlagFetchingObserver(this); On 2015/03/04 10:06:27, merkulova wrote: > On 2015/03/04 09:59:15, Marc Treib wrote: > > Could the "is child account" status already be known here? Then you wouldn't > get > > any notification from the CAService. > > Yes, it might be already know. Then we wait for the timeout. Then the FRE UI > decision is processed based on user status (which was set much earlier) and it's > ok. Then I'd add an "IsChildAccountStatusKnown" method to the CAService (returns true after flag fetching succeeded for the first time), so we don't have to wait for the timeout if we already know. (Or maybe introduce an enum ChildAccountStatus { CHILD, NON_CHILD, DONT_KNOW }?) We should probably also store it in a pref. https://codereview.chromium.org/971383002/diff/1/chrome/browser/supervised_us... File chrome/browser/supervised_user/child_accounts/child_account_service.cc (right): https://codereview.chromium.org/971383002/diff/1/chrome/browser/supervised_us... chrome/browser/supervised_user/child_accounts/child_account_service.cc:321: FOR_EACH_OBSERVER(FlagsFetchingObserver, observer_list_, On 2015/03/04 10:06:27, merkulova wrote: > On 2015/03/04 09:59:15, Marc Treib wrote: > > I'd only call the observers if is_child_account actually changed (so probably > > move this into SetIsChildAccount). > > But we're actually waiting for the fetching to complete. No matter if the status > change happened. Right. We'd also want to notify the observers when the new "is_child_account_status_known_" changes. https://codereview.chromium.org/971383002/diff/1/chrome/browser/supervised_us... File chrome/browser/supervised_user/child_accounts/flags_fetching_observer.h (right): https://codereview.chromium.org/971383002/diff/1/chrome/browser/supervised_us... chrome/browser/supervised_user/child_accounts/flags_fetching_observer.h:8: class FlagsFetchingObserver { On 2015/03/04 10:06:27, merkulova wrote: > On 2015/03/04 09:59:15, Marc Treib wrote: > > I'd call this something like ChildAccountStatusObserver, and have only a > single > > ChildAccountStatusChanged(bool is_child) method. All the other methods don't > > really tell the observer anything useful. > > > > Also, nit: indentation is wrong in this file. > > they say that fetching stopped (for any reason). Do you think it's not necessary > for FRE decision? > > Ack regarding naming. But the fetching never actually stops - if there's an error, we retry. So I'd say the observer doesn't actually care about the fetching at all, it just wants to know if this is a child account. I'd formulate the FRE logic as: Wait until we know whether this is a child account. If we still don't know after n seconds, give up and assume it's a regular account.
Changed the observer methods set to single ChildAccountStatusChanged. Looks like we can use it instead PropagateChildStatusToUser. But there might be issues with ctors of User class and CAService class synchronization then. https://codereview.chromium.org/971383002/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/971383002/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/session/user_session_manager.cc:1118: AddFlagFetchingObserver(this); On 2015/03/04 10:17:53, Marc Treib wrote: > On 2015/03/04 10:06:27, merkulova wrote: > > On 2015/03/04 09:59:15, Marc Treib wrote: > > > Could the "is child account" status already be known here? Then you wouldn't > > get > > > any notification from the CAService. > > > > Yes, it might be already know. Then we wait for the timeout. Then the FRE UI > > decision is processed based on user status (which was set much earlier) and > it's > > ok. > > Then I'd add an "IsChildAccountStatusKnown" method to the CAService (returns > true after flag fetching succeeded for the first time), so we don't have to wait > for the timeout if we already know. (Or maybe introduce an enum > ChildAccountStatus { CHILD, NON_CHILD, DONT_KNOW }?) > We should probably also store it in a pref. Done. https://codereview.chromium.org/971383002/diff/1/chrome/browser/supervised_us... File chrome/browser/supervised_user/child_accounts/child_account_service.cc (right): https://codereview.chromium.org/971383002/diff/1/chrome/browser/supervised_us... chrome/browser/supervised_user/child_accounts/child_account_service.cc:321: FOR_EACH_OBSERVER(FlagsFetchingObserver, observer_list_, On 2015/03/04 10:17:53, Marc Treib wrote: > On 2015/03/04 10:06:27, merkulova wrote: > > On 2015/03/04 09:59:15, Marc Treib wrote: > > > I'd only call the observers if is_child_account actually changed (so > probably > > > move this into SetIsChildAccount). > > > > But we're actually waiting for the fetching to complete. No matter if the > status > > change happened. > > Right. We'd also want to notify the observers when the new > "is_child_account_status_known_" changes. Done. https://codereview.chromium.org/971383002/diff/1/chrome/browser/supervised_us... File chrome/browser/supervised_user/child_accounts/flags_fetching_observer.h (right): https://codereview.chromium.org/971383002/diff/1/chrome/browser/supervised_us... chrome/browser/supervised_user/child_accounts/flags_fetching_observer.h:8: class FlagsFetchingObserver { On 2015/03/04 10:17:53, Marc Treib wrote: > On 2015/03/04 10:06:27, merkulova wrote: > > On 2015/03/04 09:59:15, Marc Treib wrote: > > > I'd call this something like ChildAccountStatusObserver, and have only a > > single > > > ChildAccountStatusChanged(bool is_child) method. All the other methods don't > > > really tell the observer anything useful. > > > > > > Also, nit: indentation is wrong in this file. > > > > they say that fetching stopped (for any reason). Do you think it's not > necessary > > for FRE decision? > > > > Ack regarding naming. > > But the fetching never actually stops - if there's an error, we retry. So I'd > say the observer doesn't actually care about the fetching at all, it just wants > to know if this is a child account. > I'd formulate the FRE logic as: Wait until we know whether this is a child > account. If we still don't know after n seconds, give up and assume it's a > regular account. Acknowledged.
https://codereview.chromium.org/971383002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/971383002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.cc:349: const int UserSessionManager::kFlagsFetchingLoginTimeoutMs = 1000; Can we make this large enough so that we get a second try if the first one fails? Something like 2 * the initial delay in the CAService. (You can also decrease the initial delay in the CAService a bit, so this timeout doesn't get too large.) https://codereview.chromium.org/971383002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.cc:795: if (!child_service) Can this actually happen? If so, add a comment please. https://codereview.chromium.org/971383002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.cc:797: if (!child_service->IsChildAccountStatusKnown()) { DCHECK(child_service->IsChildAccountStatusKnown()) << "message"; if (requesting_start_urls) { ... Or alternatively, remove the whole thing - you don't actually need the CAService here. https://codereview.chromium.org/971383002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.cc:799: } else if (requesting_start_urls_) { I think you can move this block into StopStatusObserving, then you don't need to repeat it. https://codereview.chromium.org/971383002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.cc:807: if (requesting_start_urls_) { ^^ https://codereview.chromium.org/971383002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.cc:1128: if (child_service) Here too: Can this actually happen? https://codereview.chromium.org/971383002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/session/user_session_manager.h (right): https://codereview.chromium.org/971383002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.h:239: static const int kFlagsFetchingLoginTimeoutMs; I think there's no reason for this to be in the header, right? I'd just make it a local constant in the .cc https://codereview.chromium.org/971383002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.h:430: bool requesting_start_urls_; This name sounds like "we're currently requesting the start URLs". Maybe "init_start_urls_was_postponed_"? https://codereview.chromium.org/971383002/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/child_accounts/child_account_service.cc (right): https://codereview.chromium.org/971383002/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/child_accounts/child_account_service.cc:321: account_status_ = is_child_account ? CHILD : NON_CHILD; "TODO(treib): Add a pref for this, so the info will survive Chrome restarts." https://codereview.chromium.org/971383002/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/child_accounts/child_account_service.h (right): https://codereview.chromium.org/971383002/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/child_accounts/child_account_service.h:126: ChildAccountStatus account_status_; Hm, I think this is actually only used to decide UNKNOWN or not UNKNOWN. If so, it should just be a bool. https://codereview.chromium.org/971383002/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/child_accounts/child_account_status_observer.h (right): https://codereview.chromium.org/971383002/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/child_accounts/child_account_status_observer.h:10: virtual void OnChildAccountStatusChanged() {} Add a "bool is_child" parameter?
Denis, could you join the review to discuss the proper timeout timing. https://codereview.chromium.org/971383002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/971383002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.cc:349: const int UserSessionManager::kFlagsFetchingLoginTimeoutMs = 1000; On 2015/03/04 16:31:48, Marc Treib wrote: > Can we make this large enough so that we get a second try if the first one > fails? Something like 2 * the initial delay in the CAService. (You can also > decrease the initial delay in the CAService a bit, so this timeout doesn't get > too large.) That's a tricky question: we want the FRE UI to appear quick enough so that user wouldn't be able to start the browsing or other activities on his own before the FRE comes out. I think that even 1 sec might be too long. We can't make the request for CAService much earlier as the user profile is not ready then. When it's ready, the session starts and user is able to interact with it. Denis, any comments on that from your side? https://codereview.chromium.org/971383002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.cc:795: if (!child_service) On 2015/03/04 16:31:48, Marc Treib wrote: > Can this actually happen? If so, add a comment please. I hope not, this all relies on CAServiceFactory. If it's fine, then I can remove it. WDYT, Marc? https://codereview.chromium.org/971383002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.cc:797: if (!child_service->IsChildAccountStatusKnown()) { On 2015/03/04 16:31:48, Marc Treib wrote: > DCHECK(child_service->IsChildAccountStatusKnown()) << "message"; > if (requesting_start_urls) { > ... > > Or alternatively, remove the whole thing - you don't actually need the CAService > here. Done. https://codereview.chromium.org/971383002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.cc:799: } else if (requesting_start_urls_) { On 2015/03/04 16:31:48, Marc Treib wrote: > I think you can move this block into StopStatusObserving, then you don't need to > repeat it. Done. https://codereview.chromium.org/971383002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.cc:807: if (requesting_start_urls_) { On 2015/03/04 16:31:48, Marc Treib wrote: > ^^ Done. https://codereview.chromium.org/971383002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.cc:1128: if (child_service) On 2015/03/04 16:31:48, Marc Treib wrote: > Here too: Can this actually happen? I hope no. It all relies on CAServiceFactory. If it's fine, then this can be removed. https://codereview.chromium.org/971383002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/session/user_session_manager.h (right): https://codereview.chromium.org/971383002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.h:239: static const int kFlagsFetchingLoginTimeoutMs; On 2015/03/04 16:31:48, Marc Treib wrote: > I think there's no reason for this to be in the header, right? I'd just make it > a local constant in the .cc Done. https://codereview.chromium.org/971383002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.h:430: bool requesting_start_urls_; On 2015/03/04 16:31:48, Marc Treib wrote: > This name sounds like "we're currently requesting the start URLs". Maybe > "init_start_urls_was_postponed_"? Done. https://codereview.chromium.org/971383002/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/child_accounts/child_account_service.cc (right): https://codereview.chromium.org/971383002/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/child_accounts/child_account_service.cc:321: account_status_ = is_child_account ? CHILD : NON_CHILD; On 2015/03/04 16:31:48, Marc Treib wrote: > "TODO(treib): Add a pref for this, so the info will survive Chrome restarts." Done. https://codereview.chromium.org/971383002/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/child_accounts/child_account_service.h (right): https://codereview.chromium.org/971383002/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/child_accounts/child_account_service.h:126: ChildAccountStatus account_status_; On 2015/03/04 16:31:48, Marc Treib wrote: > Hm, I think this is actually only used to decide UNKNOWN or not UNKNOWN. If so, > it should just be a bool. removed. Moved to pref. https://codereview.chromium.org/971383002/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/child_accounts/child_account_status_observer.h (right): https://codereview.chromium.org/971383002/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/child_accounts/child_account_status_observer.h:10: virtual void OnChildAccountStatusChanged() {} On 2015/03/04 16:31:48, Marc Treib wrote: > Add a "bool is_child" parameter? what for? do you think it will become useful? Also it looks like then we can use the observer instead PropagateChildStatusToUser. But there might be issues with ctors of User class and CAService class synchronization then.
https://codereview.chromium.org/971383002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/971383002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.cc:795: if (!child_service) On 2015/03/04 19:06:49, merkulova wrote: > On 2015/03/04 16:31:48, Marc Treib wrote: > > Can this actually happen? If so, add a comment please. > > I hope not, this all relies on CAServiceFactory. If it's fine, then I can remove > it. WDYT, Marc? It might (or might not) happen in some unit tests; it should not ever happen in actual usage. I'd remove all the null checks and see if any tests break. https://codereview.chromium.org/971383002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.cc:1128: if (child_service) On 2015/03/04 19:06:49, merkulova wrote: > On 2015/03/04 16:31:48, Marc Treib wrote: > > Here too: Can this actually happen? > I hope no. It all relies on CAServiceFactory. If it's fine, then this can be > removed. Yup, please remove :) https://codereview.chromium.org/971383002/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/child_accounts/child_account_status_observer.h (right): https://codereview.chromium.org/971383002/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/child_accounts/child_account_status_observer.h:10: virtual void OnChildAccountStatusChanged() {} On 2015/03/04 19:06:49, merkulova wrote: > On 2015/03/04 16:31:48, Marc Treib wrote: > > Add a "bool is_child" parameter? > > what for? do you think it will become useful? Yes, I'd expect so - if you're interested in changes to the child state, then you probably want to know the new state. And it just seems weird to tell the observer "hey, this changed", but not tell them what it changed *to*. > Also it looks like then we can use the observer instead > PropagateChildStatusToUser. But there might be > issues with ctors of User class and CAService class synchronization then. It would be great if we could get rid of PropagateChildStatusToUser! But I'd leave that for another CL. https://codereview.chromium.org/971383002/diff/40001/chrome/browser/supervise... File chrome/browser/supervised_user/child_accounts/child_account_service.cc (right): https://codereview.chromium.org/971383002/diff/40001/chrome/browser/supervise... chrome/browser/supervised_user/child_accounts/child_account_service.cc:333: if (status_was_known) { Wait, if the status was *not* known before, then surely we want to inform the observers as well?!
merkulova@chromium.org changed reviewers: + bauerb@chromium.org
+bauerb@ as browser_prefs.cc owner https://codereview.chromium.org/971383002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/971383002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.cc:795: if (!child_service) On 2015/03/05 10:08:07, Marc Treib wrote: > On 2015/03/04 19:06:49, merkulova wrote: > > On 2015/03/04 16:31:48, Marc Treib wrote: > > > Can this actually happen? If so, add a comment please. > > > > I hope not, this all relies on CAServiceFactory. If it's fine, then I can > remove > > it. WDYT, Marc? > > It might (or might not) happen in some unit tests; it should not ever happen in > actual usage. I'd remove all the null checks and see if any tests break. Done. https://codereview.chromium.org/971383002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.cc:1128: if (child_service) On 2015/03/05 10:08:07, Marc Treib wrote: > On 2015/03/04 19:06:49, merkulova wrote: > > On 2015/03/04 16:31:48, Marc Treib wrote: > > > Here too: Can this actually happen? > > I hope no. It all relies on CAServiceFactory. If it's fine, then this can be > > removed. > > Yup, please remove :) Done. https://codereview.chromium.org/971383002/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/child_accounts/child_account_status_observer.h (right): https://codereview.chromium.org/971383002/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/child_accounts/child_account_status_observer.h:10: virtual void OnChildAccountStatusChanged() {} On 2015/03/05 10:08:07, Marc Treib wrote: > On 2015/03/04 19:06:49, merkulova wrote: > > On 2015/03/04 16:31:48, Marc Treib wrote: > > > Add a "bool is_child" parameter? > > > > what for? do you think it will become useful? > > Yes, I'd expect so - if you're interested in changes to the child state, then > you probably want to know the new state. And it just seems weird to tell the > observer "hey, this changed", but not tell them what it changed *to*. > > > Also it looks like then we can use the observer instead > > PropagateChildStatusToUser. But there might be > > issues with ctors of User class and CAService class synchronization then. > > It would be great if we could get rid of PropagateChildStatusToUser! But I'd > leave that for another CL. Done. https://codereview.chromium.org/971383002/diff/40001/chrome/browser/supervise... File chrome/browser/supervised_user/child_accounts/child_account_service.cc (right): https://codereview.chromium.org/971383002/diff/40001/chrome/browser/supervise... chrome/browser/supervised_user/child_accounts/child_account_service.cc:333: if (status_was_known) { On 2015/03/05 10:08:07, Marc Treib wrote: > Wait, if the status was *not* known before, then surely we want to inform the > observers as well?! Good catch, thank you.
https://codereview.chromium.org/971383002/diff/40001/chrome/browser/supervise... File chrome/browser/supervised_user/child_accounts/child_account_service.cc (right): https://codereview.chromium.org/971383002/diff/40001/chrome/browser/supervise... chrome/browser/supervised_user/child_accounts/child_account_service.cc:333: if (status_was_known) { On 2015/03/10 09:18:22, merkulova wrote: > On 2015/03/05 10:08:07, Marc Treib wrote: > > Wait, if the status was *not* known before, then surely we want to inform the > > observers as well?! > > Good catch, thank you. I meant that we should notify observers whenever the status changes, regardless of whether it was known before or not. (The status can change from "child" to "not child" on graduation!) https://codereview.chromium.org/971383002/diff/60001/chrome/browser/supervise... File chrome/browser/supervised_user/child_accounts/child_account_service.cc (right): https://codereview.chromium.org/971383002/diff/60001/chrome/browser/supervise... chrome/browser/supervised_user/child_accounts/child_account_service.cc:288: flag_fetcher_.reset(new AccountServiceFlagFetcher( Don't re-add this code in this CL, please - see https://codereview.chromium.org/986303002/
https://codereview.chromium.org/971383002/diff/60001/chrome/browser/supervise... File chrome/browser/supervised_user/child_accounts/child_account_service.cc (right): https://codereview.chromium.org/971383002/diff/60001/chrome/browser/supervise... chrome/browser/supervised_user/child_accounts/child_account_service.cc:288: flag_fetcher_.reset(new AccountServiceFlagFetcher( On 2015/03/10 09:26:17, Marc Treib wrote: > Don't re-add this code in this CL, please - see > https://codereview.chromium.org/986303002/ Sorry, I think it was re-added on CL rebase.
https://codereview.chromium.org/971383002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/971383002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.cc:105: // Milliseconds until we timeout our attempt to fetch flags from Child Account Nit: Child Account Service is not a common name, so it's in lowercase. Also, if you put an definite article in front it reads easier. https://codereview.chromium.org/971383002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.cc:322: registry->RegisterBooleanPref(prefs::kChildAccountStatusKnown, false); So who registers this pref? UserSessionManager or ChildAccountService? https://codereview.chromium.org/971383002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/session/user_session_manager.h (right): https://codereview.chromium.org/971383002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.h:424: Profile* profile_fetching_flags_; Nit: this variable name reads to me like it stores the flags for profile fetching. Maybe rename it to |flag_fetching_profile_|? https://codereview.chromium.org/971383002/diff/80001/chrome/browser/supervise... File chrome/browser/supervised_user/child_accounts/child_account_service.h (right): https://codereview.chromium.org/971383002/diff/80001/chrome/browser/supervise... chrome/browser/supervised_user/child_accounts/child_account_service.h:45: // Registers the preferences related to Child Account Service. This comment is redundant. https://codereview.chromium.org/971383002/diff/80001/chrome/browser/supervise... File chrome/browser/supervised_user/child_accounts/child_account_status_observer.h (right): https://codereview.chromium.org/971383002/diff/80001/chrome/browser/supervise... chrome/browser/supervised_user/child_accounts/child_account_status_observer.h:9: public: "public:" is indented one space, then the method declarations are indented two spaces (i.e. one more than the previous line). https://codereview.chromium.org/971383002/diff/80001/chrome/browser/supervise... chrome/browser/supervised_user/child_accounts/child_account_status_observer.h:11: virtual void OnChildAccountStatusChanged(bool is_child) {} If this is only called when transitioning from unknown state to known state, maybe reflect that in the name (right now it looks like this method is called on any state change). Also, are you planning to add more methods to this interface? If not, consider using a base::Callback instead.
https://codereview.chromium.org/971383002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/session/user_session_manager.h (right): https://codereview.chromium.org/971383002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.h:424: Profile* profile_fetching_flags_; On 2015/03/10 09:55:12, Bernhard Bauer wrote: > Nit: this variable name reads to me like it stores the flags for profile > fetching. Maybe rename it to |flag_fetching_profile_|? It probably shouldn't talk about "flag fetching" at all, since that's really an implementation detail of the CAService. I don't have a good suggestion though... https://codereview.chromium.org/971383002/diff/80001/chrome/browser/supervise... File chrome/browser/supervised_user/child_accounts/child_account_status_observer.h (right): https://codereview.chromium.org/971383002/diff/80001/chrome/browser/supervise... chrome/browser/supervised_user/child_accounts/child_account_status_observer.h:11: virtual void OnChildAccountStatusChanged(bool is_child) {} On 2015/03/10 09:55:12, Bernhard Bauer wrote: > If this is only called when transitioning from unknown state to known state, > maybe reflect that in the name (right now it looks like this method is called on > any state change). I think it should be called on any state change, otherwise there's really no need for an Observer interface. > Also, are you planning to add more methods to this interface? If not, consider > using a base::Callback instead. Hm, I guess that's the alternative: A callback that's called exactly once, when we get the child state for the first time.
https://codereview.chromium.org/971383002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/971383002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.cc:105: // Milliseconds until we timeout our attempt to fetch flags from Child Account On 2015/03/10 09:55:12, Bernhard Bauer wrote: > Nit: Child Account Service is not a common name, so it's in lowercase. Also, if > you put an definite article in front it reads easier. Done. https://codereview.chromium.org/971383002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.cc:322: registry->RegisterBooleanPref(prefs::kChildAccountStatusKnown, false); On 2015/03/10 09:55:12, Bernhard Bauer wrote: > So who registers this pref? UserSessionManager or ChildAccountService? ChildAccountService. https://codereview.chromium.org/971383002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/session/user_session_manager.h (right): https://codereview.chromium.org/971383002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/session/user_session_manager.h:424: Profile* profile_fetching_flags_; On 2015/03/10 09:55:12, Bernhard Bauer wrote: > Nit: this variable name reads to me like it stores the flags for profile > fetching. Maybe rename it to |flag_fetching_profile_|? Done. https://codereview.chromium.org/971383002/diff/80001/chrome/browser/supervise... File chrome/browser/supervised_user/child_accounts/child_account_service.h (right): https://codereview.chromium.org/971383002/diff/80001/chrome/browser/supervise... chrome/browser/supervised_user/child_accounts/child_account_service.h:45: // Registers the preferences related to Child Account Service. On 2015/03/10 09:55:12, Bernhard Bauer wrote: > This comment is redundant. Done. https://codereview.chromium.org/971383002/diff/80001/chrome/browser/supervise... File chrome/browser/supervised_user/child_accounts/child_account_status_observer.h (right): https://codereview.chromium.org/971383002/diff/80001/chrome/browser/supervise... chrome/browser/supervised_user/child_accounts/child_account_status_observer.h:9: public: On 2015/03/10 09:55:12, Bernhard Bauer wrote: > "public:" is indented one space, then the method declarations are indented two > spaces (i.e. one more than the previous line). Acknowledged. https://codereview.chromium.org/971383002/diff/80001/chrome/browser/supervise... chrome/browser/supervised_user/child_accounts/child_account_status_observer.h:11: virtual void OnChildAccountStatusChanged(bool is_child) {} On 2015/03/10 10:43:34, Marc Treib wrote: > On 2015/03/10 09:55:12, Bernhard Bauer wrote: > > If this is only called when transitioning from unknown state to known state, > > maybe reflect that in the name (right now it looks like this method is called > on > > any state change). > > I think it should be called on any state change, otherwise there's really no > need for an Observer interface. > > > Also, are you planning to add more methods to this interface? If not, consider > > using a base::Callback instead. > > Hm, I guess that's the alternative: A callback that's called exactly once, when > we get the child state for the first time. Removed the class completely.
https://codereview.chromium.org/971383002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.h (right): https://codereview.chromium.org/971383002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.h:421: Profile* flag_fetching_profile_; I think this variable isn't needed anymore now :) https://codereview.chromium.org/971383002/diff/100001/chrome/browser/supervis... File chrome/browser/supervised_user/child_accounts/child_account_service.cc (right): https://codereview.chromium.org/971383002/diff/100001/chrome/browser/supervis... chrome/browser/supervised_user/child_accounts/child_account_service.cc:129: status_received_callback_list_.push_back(callback); What if the status is already known? We might want to call the callback directly, or otherwise at least add some warning that it won't ever be called. https://codereview.chromium.org/971383002/diff/100001/chrome/browser/supervis... chrome/browser/supervised_user/child_accounts/child_account_service.cc:326: } Also clear the list of callbacks here, since we won't need them again? https://codereview.chromium.org/971383002/diff/100001/chrome/browser/supervis... File chrome/browser/supervised_user/child_accounts/child_account_service.h (right): https://codereview.chromium.org/971383002/diff/100001/chrome/browser/supervis... chrome/browser/supervised_user/child_accounts/child_account_service.h:58: void AddChildStatusReceivedCallback(base::Closure callback); const base::Closure&
lgtm https://codereview.chromium.org/971383002/diff/100001/chrome/browser/supervis... File chrome/browser/supervised_user/child_accounts/child_account_service.h (right): https://codereview.chromium.org/971383002/diff/100001/chrome/browser/supervis... chrome/browser/supervised_user/child_accounts/child_account_service.h:58: void AddChildStatusReceivedCallback(base::Closure callback); Pass the callback as const ref.
https://codereview.chromium.org/971383002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.h (right): https://codereview.chromium.org/971383002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.h:421: Profile* flag_fetching_profile_; On 2015/03/10 12:01:35, Marc Treib wrote: > I think this variable isn't needed anymore now :) Done. https://codereview.chromium.org/971383002/diff/100001/chrome/browser/supervis... File chrome/browser/supervised_user/child_accounts/child_account_service.cc (right): https://codereview.chromium.org/971383002/diff/100001/chrome/browser/supervis... chrome/browser/supervised_user/child_accounts/child_account_service.cc:129: status_received_callback_list_.push_back(callback); On 2015/03/10 12:01:35, Marc Treib wrote: > What if the status is already known? We might want to call the callback > directly, or otherwise at least add some warning that it won't ever be called. https://codereview.chromium.org/971383002/diff/100001/chrome/browser/supervis... chrome/browser/supervised_user/child_accounts/child_account_service.cc:326: } On 2015/03/10 12:01:35, Marc Treib wrote: > Also clear the list of callbacks here, since we won't need them again? Done. https://codereview.chromium.org/971383002/diff/100001/chrome/browser/supervis... File chrome/browser/supervised_user/child_accounts/child_account_service.h (right): https://codereview.chromium.org/971383002/diff/100001/chrome/browser/supervis... chrome/browser/supervised_user/child_accounts/child_account_service.h:58: void AddChildStatusReceivedCallback(base::Closure callback); On 2015/03/10 12:02:44, Bernhard Bauer wrote: > Pass the callback as const ref. Done. https://codereview.chromium.org/971383002/diff/100001/chrome/browser/supervis... chrome/browser/supervised_user/child_accounts/child_account_service.h:58: void AddChildStatusReceivedCallback(base::Closure callback); On 2015/03/10 12:01:35, Marc Treib wrote: > const base::Closure& Done.
Thanks, LGTM!
https://chromiumcodereview.appspot.com/971383002/diff/120001/chrome/browser/c... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://chromiumcodereview.appspot.com/971383002/diff/120001/chrome/browser/c... chrome/browser/chromeos/login/session/user_session_manager.cc:797: init_start_urls_was_postponed_ = false; Please rename this flag to something like "Waiting for child account status" and comment the fact that this status should be known before call to InitalizeStartUrls() method. https://chromiumcodereview.appspot.com/971383002/diff/120001/chrome/browser/c... File chrome/browser/chromeos/login/session/user_session_manager.h (right): https://chromiumcodereview.appspot.com/971383002/diff/120001/chrome/browser/c... chrome/browser/chromeos/login/session/user_session_manager.h:252: void StopStatusObserving(); Maybe call it StopChildStatusObserving()? Name (especially without any comments) is a bit misleading. https://chromiumcodereview.appspot.com/971383002/diff/120001/chrome/browser/s... File chrome/browser/supervised_user/child_accounts/child_account_service.h (right): https://chromiumcodereview.appspot.com/971383002/diff/120001/chrome/browser/s... chrome/browser/supervised_user/child_accounts/child_account_service.h:53: bool IsChildAccountStatusKnown(); Some comments?
https://codereview.chromium.org/971383002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/971383002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:797: init_start_urls_was_postponed_ = false; On 2015/03/10 14:40:34, Denis Kuznetsov wrote: > Please rename this flag to something like "Waiting for child account status" and > comment the fact that this status should be known before call to > InitalizeStartUrls() method. Done. https://codereview.chromium.org/971383002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.h (right): https://codereview.chromium.org/971383002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.h:252: void StopStatusObserving(); On 2015/03/10 14:40:34, Denis Kuznetsov wrote: > Maybe call it StopChildStatusObserving()? > Name (especially without any comments) is a bit misleading. Done. https://codereview.chromium.org/971383002/diff/120001/chrome/browser/supervis... File chrome/browser/supervised_user/child_accounts/child_account_service.h (right): https://codereview.chromium.org/971383002/diff/120001/chrome/browser/supervis... chrome/browser/supervised_user/child_accounts/child_account_service.h:53: bool IsChildAccountStatusKnown(); On 2015/03/10 14:40:34, Denis Kuznetsov wrote: > Some comments? Done.
lgtm
The CQ bit was checked by merkulova@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, treib@chromium.org Link to the patchset: https://codereview.chromium.org/971383002/#ps140001 (title: "Renames and comments adding.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/971383002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by merkulova@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from antrim@chromium.org, treib@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/971383002/#ps160001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/971383002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/446a405c524d369155368d339b4ae29c428d6b22 Cr-Commit-Position: refs/heads/master@{#320062} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
