Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1966133002:
Run RemoveArcData after a user has opted out (Closed)
|
Created:
4 years, 7 months ago by dspaid Modified:
4 years, 6 months ago Reviewers:
bartfab (slow), Junichi Uekawa, stevenjb, Daniel Erat, Luis Héctor Chávez, Andrew Wilson, hidehiko CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, hashimoto+watch_chromium.org, dzhioev+watch_chromium.org, achuith+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, hidehiko+watch_chomium.org, davemoore+watch_chromium.org, use bartfab instead Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRun RemoveArcData after a user has opted out
BUG=26784296
TEST=Manual testingin conjunction with chromeos side changes.
Committed: https://crrev.com/102f85d50db963e44004c991133c7c5e78844e7f
Cr-Commit-Position: refs/heads/master@{#399798}
Patch Set 1 #Patch Set 2 : Fix style issues #Patch Set 3 : #Patch Set 4 : added comment #
Total comments: 22
Patch Set 5 : #
Total comments: 11
Patch Set 6 : addressed comments #Patch Set 7 : Fixed DEPS #
Total comments: 10
Patch Set 8 : Addressed comments #
Total comments: 9
Patch Set 9 : Refactor use of kArcEnabled pref into PrefMember #Patch Set 10 : clean up boolean logic #
Total comments: 58
Patch Set 11 : Addressed comments #
Total comments: 8
Patch Set 12 : Add OnPrimaryUserProfilePrepared wrapper around ClearIfDisabled #Patch Set 13 : Add TODO to rename OnStateChanged #
Total comments: 4
Patch Set 14 : Remove unnecessary braces #Patch Set 15 : Fix broken tests #Patch Set 16 : More test fixes #Patch Set 17 : Test fixes #Patch Set 18 : Unregister observer on shutdown #
Total comments: 1
Patch Set 19 : Removed unneeded import #Patch Set 20 : Add cryptohome ID to dbus call #
Total comments: 13
Patch Set 21 : Addressed comments #
Total comments: 6
Patch Set 22 : Addressed comments #
Total comments: 9
Patch Set 23 : Adressed comments #Messages
Total messages: 70 (12 generated)
dspaid@chromium.org changed reviewers: + derat@chromium.org, uekawa@chromium.org
derat: This is chrome side change for the cros change you commented on uekawa: FYI
On 2016/05/12 05:41:21, dspaid wrote: > derat: This is chrome side change for the cros change you commented on > uekawa: FYI The description is missing a space s/TEST=Manual testingin conjunction with chromeos side changes./ TEST=Manual testing in conjunction with chromeos side changes./
https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/chrome_browser_main_chromeos.h (right): https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/chrome_browser_main_chromeos.h:22: class ArcDataManager; sort? https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/chrome_browser_main_chromeos.h:88: std::unique_ptr<arc::ArcDataManager> arc_data_manager_; naive question: why does this not live inside ArcServiceLauncher?
https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_data_manager.cc (right): https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_data_manager.cc:26: DCHECK(thread_checker_.CalledOnValidThread()); ThreadChecker appears to initially determine the "valid" thread based on where it's constructed, so i don't think that calling this from ArcDataManager's c'tor actually does anything. https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_data_manager.cc:45: void ArcDataManager::OnStateChanged(ArcBridgeService::State state) { DCHECK(thread_checker_.CalledOnValidThread()); https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_data_manager.cc:56: void ArcDataManager::ClearIfDisabled(Profile* profile) { DCHECK(thread_checker_.CalledOnValidThread()); https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_data_manager.cc:58: if (!profile->GetPrefs()->GetBoolean(prefs::kArcEnabled)) { if you don't plan to ever call this from somewhere else, how about just moving it into OnStateChanged? https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_data_manager.h (right): https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_data_manager.h:18: // This class controls the lifecycle of arc user data, removing it when nit: s/arc/ARC/ https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_data_manager.h:23: ~ArcDataManager(); override https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_data_manager.h:25: static ArcDataManager* Get(); if at all possible, please instead pass the instance to the classes that need to call it. static Get() methods are error-prone, as callers have no way of knowing when they're safe to be called during startup or shutdown, leading to either speculative null checks or segfaults. https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_data_manager.h:33: void OnStateChanged(ArcBridgeService::State state) override; nit: add an: // ArcBridgeService::Observer: comment at the top of this to document where it comes from
https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_data_manager.cc (right): https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_data_manager.cc:26: DCHECK(thread_checker_.CalledOnValidThread()); On 2016/05/12 16:33:34, Daniel Erat wrote: > ThreadChecker appears to initially determine the "valid" thread based on where > it's constructed, so i don't think that calling this from ArcDataManager's c'tor > actually does anything. Done. https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_data_manager.cc:45: void ArcDataManager::OnStateChanged(ArcBridgeService::State state) { On 2016/05/12 16:33:34, Daniel Erat wrote: > DCHECK(thread_checker_.CalledOnValidThread()); Done. https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_data_manager.cc:56: void ArcDataManager::ClearIfDisabled(Profile* profile) { On 2016/05/12 16:33:34, Daniel Erat wrote: > DCHECK(thread_checker_.CalledOnValidThread()); Done. https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_data_manager.cc:58: if (!profile->GetPrefs()->GetBoolean(prefs::kArcEnabled)) { On 2016/05/12 16:33:34, Daniel Erat wrote: > if you don't plan to ever call this from somewhere else, how about just moving > it into OnStateChanged? Its also called at login, and the enterprise team was interested in calling it in some other instances. https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_data_manager.h (right): https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_data_manager.h:18: // This class controls the lifecycle of arc user data, removing it when On 2016/05/12 16:33:35, Daniel Erat wrote: > nit: s/arc/ARC/ Done. https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_data_manager.h:23: ~ArcDataManager(); On 2016/05/12 16:33:34, Daniel Erat wrote: > override Done. https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_data_manager.h:25: static ArcDataManager* Get(); On 2016/05/12 16:33:35, Daniel Erat wrote: > if at all possible, please instead pass the instance to the classes that need to > call it. static Get() methods are error-prone, as callers have no way of knowing > when they're safe to be called during startup or shutdown, leading to either > speculative null checks or segfaults. I'm open to suggestions here. The current use is in user_session_manager.cc where similar ARC related code is accessed via static Get methods so I opted for consistency. I don't have an easy solution for passing an instance to the constructor of UserSessionManager, but I could add a public SetArcDataManager method to pass it. However we'd still have the problem of UserSessionManager not knowing if the instance had been set or not, so speculative null checks would remain. https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_data_manager.h:33: void OnStateChanged(ArcBridgeService::State state) override; On 2016/05/12 16:33:35, Daniel Erat wrote: > nit: add an: > > // ArcBridgeService::Observer: > > comment at the top of this to document where it comes from Done. https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/chrome_browser_main_chromeos.h (right): https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/chrome_browser_main_chromeos.h:22: class ArcDataManager; On 2016/05/12 08:44:15, Junichi Uekawa wrote: > sort? Done. https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/chrome_browser_main_chromeos.h:88: std::unique_ptr<arc::ArcDataManager> arc_data_manager_; On 2016/05/12 08:44:15, Junichi Uekawa wrote: > naive question: why does this not live inside ArcServiceLauncher? Service launcher right now only seems to be responsible for starting up and shutting down the arc bridge. I felt that adding in data management would be out of scope.
derat@chromium.org changed reviewers: + stevenjb@chromium.org
+steven in case he has advice about the static Get() method
m https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_data_manager.h (right): https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_data_manager.h:25: static ArcDataManager* Get(); On 2016/05/13 01:19:24, dspaid wrote: > On 2016/05/12 16:33:35, Daniel Erat wrote: > > if at all possible, please instead pass the instance to the classes that need > to > > call it. static Get() methods are error-prone, as callers have no way of > knowing > > when they're safe to be called during startup or shutdown, leading to either > > speculative null checks or segfaults. > > I'm open to suggestions here. The current use is in user_session_manager.cc > where similar ARC related code is accessed via static Get methods so I opted for > consistency. I don't have an easy solution for passing an instance to the > constructor of UserSessionManager, but I could add a public SetArcDataManager > method to pass it. However we'd still have the problem of UserSessionManager > not knowing if the instance had been set or not, so speculative null checks > would remain. We should at least make this consistent with other arc services and initialize this in ArcServiceLauncher. See other comment. https://codereview.chromium.org/1966133002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_data_manager.cc (right): https://codereview.chromium.org/1966133002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_data_manager.cc:50: user_manager::UserManager::Get()->GetPrimaryUser(); nit: The extra const here and in line 51 is a bit confusing to read and of little benefit. This pattern is uncommon in chrome and I would avoid it. (i.e. just use 'const user_manager::User* primary_user') https://codereview.chromium.org/1966133002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_data_manager.cc:58: DCHECK(ArcBridgeService::Get()->state() == ArcBridgeService::State::STOPPED); Is the calling code logically protected against this occurring? If there is any possibility of an edge case triggering this DCHECk then we should handle this instead (e.g. with LOG(ERROR) + return). https://codereview.chromium.org/1966133002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_data_manager.cc:59: if (!profile->GetPrefs()->GetBoolean(prefs::kArcEnabled)) { nit: Invert and early exit https://codereview.chromium.org/1966133002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_data_manager.h (right): https://codereview.chromium.org/1966133002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_data_manager.h:19: // necessary. May I suggest renaming this something like ArcUserDataManager? "ARC data manager" could refer to just about anything related to Arc and data :) https://codereview.chromium.org/1966133002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/chrome_browser_main_chromeos.h (right): https://codereview.chromium.org/1966133002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/chrome_browser_main_chromeos.h:88: std::unique_ptr<arc::ArcDataManager> arc_data_manager_; We should make this part of arc_service_launcher_ so that we only have the one awkward singleton to manage. We may want to rename ArcServiceLauncher at that point, but the more awkward singletons we have to manage the more likely we will introduce edge cases and shutdown crashes. (I would also suggest changing ArcServiceLauncher so that Initialize and Shutdown were static methods that create and destroy the global singleton instead of this implicit singleton model, but that can be discussed separately).
https://codereview.chromium.org/1966133002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_data_manager.cc (right): https://codereview.chromium.org/1966133002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_data_manager.cc:50: user_manager::UserManager::Get()->GetPrimaryUser(); On 2016/05/13 17:13:55, stevenjb wrote: > nit: The extra const here and in line 51 is a bit confusing to read and of > little benefit. This pattern is uncommon in chrome and I would avoid it. (i.e. > just use 'const user_manager::User* primary_user') > Probably a copy/paste bug on my side. https://codereview.chromium.org/1966133002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_data_manager.cc:58: DCHECK(ArcBridgeService::Get()->state() == ArcBridgeService::State::STOPPED); On 2016/05/13 17:13:55, stevenjb wrote: > Is the calling code logically protected against this occurring? If there is any > possibility of an edge case triggering this DCHECk then we should handle this > instead (e.g. with LOG(ERROR) + return). Done. https://codereview.chromium.org/1966133002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_data_manager.cc:59: if (!profile->GetPrefs()->GetBoolean(prefs::kArcEnabled)) { On 2016/05/13 17:13:54, stevenjb wrote: > nit: Invert and early exit Done. https://codereview.chromium.org/1966133002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/chrome_browser_main_chromeos.h (right): https://codereview.chromium.org/1966133002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/chrome_browser_main_chromeos.h:88: std::unique_ptr<arc::ArcDataManager> arc_data_manager_; On 2016/05/13 17:13:55, stevenjb wrote: > We should make this part of arc_service_launcher_ so that we only have the one > awkward singleton to manage. We may want to rename ArcServiceLauncher at that > point, but the more awkward singletons we have to manage the more likely we will > introduce edge cases and shutdown crashes. > > (I would also suggest changing ArcServiceLauncher so that Initialize and > Shutdown were static methods that create and destroy the global singleton > instead of this implicit singleton model, but that can be discussed separately). I'm not sure I understand how this would work. Right now there's way to get a pointer to ArcServiceLauncher from where this is used in UserSessionManager. Adding a static Get to ArcServiceLauncher just moves the problem from one class to another... If there's some guarantee that UserSessionManager will be initialized by the time ArcServiceLauncher is created I could add a SetArcServiceLauncher method, but similar would have to be done for every future use of these classes as well.
https://codereview.chromium.org/1966133002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/chrome_browser_main_chromeos.h (right): https://codereview.chromium.org/1966133002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/chrome_browser_main_chromeos.h:88: std::unique_ptr<arc::ArcDataManager> arc_data_manager_; On 2016/05/15 23:58:51, dspaid wrote: > On 2016/05/13 17:13:55, stevenjb wrote: > > We should make this part of arc_service_launcher_ so that we only have the one > > awkward singleton to manage. We may want to rename ArcServiceLauncher at that > > point, but the more awkward singletons we have to manage the more likely we > will > > introduce edge cases and shutdown crashes. > > > > (I would also suggest changing ArcServiceLauncher so that Initialize and > > Shutdown were static methods that create and destroy the global singleton > > instead of this implicit singleton model, but that can be discussed > separately). > > I'm not sure I understand how this would work. Right now there's way to get a > pointer to ArcServiceLauncher from where this is used in UserSessionManager. > Adding a static Get to ArcServiceLauncher just moves the problem from one class > to another... If there's some guarantee that UserSessionManager will be > initialized by the time ArcServiceLauncher is created I could add a > SetArcServiceLauncher method, but similar would have to be done for every future > use of these classes as well. The current model that Arc is using, from a somewhat quick glance, is: ArcServiceLauncher owns ArcServiceManager. ArcServiceManager owns a vector of services. Each service has a static getter, e.g. arc::ArcAuthService::Get(). I am proposing that we keep ownership of all arc related global instances in ArcServiceManager, including ArcUserDataManager. ArcUserDataManager whould still have a static Get() method, and we would still have potential issues with construction / destruction order, but at least we are not proliferating the number of arc globals owned elsewhere (i.e. here). Then, ideally, at some point we can pass ArcServiceLauncher (or something similar) to UserSessionManager (and other classes that use it) and get rid of the global accessors.
On 2016/05/16 15:56:06, stevenjb wrote: > https://codereview.chromium.org/1966133002/diff/80001/chrome/browser/chromeos... > File chrome/browser/chromeos/chrome_browser_main_chromeos.h (right): > > https://codereview.chromium.org/1966133002/diff/80001/chrome/browser/chromeos... > chrome/browser/chromeos/chrome_browser_main_chromeos.h:88: > std::unique_ptr<arc::ArcDataManager> arc_data_manager_; > On 2016/05/15 23:58:51, dspaid wrote: > > On 2016/05/13 17:13:55, stevenjb wrote: > > > We should make this part of arc_service_launcher_ so that we only have the > one > > > awkward singleton to manage. We may want to rename ArcServiceLauncher at > that > > > point, but the more awkward singletons we have to manage the more likely we > > will > > > introduce edge cases and shutdown crashes. > > > > > > (I would also suggest changing ArcServiceLauncher so that Initialize and > > > Shutdown were static methods that create and destroy the global singleton > > > instead of this implicit singleton model, but that can be discussed > > separately). > > > > I'm not sure I understand how this would work. Right now there's way to get a > > pointer to ArcServiceLauncher from where this is used in UserSessionManager. > > Adding a static Get to ArcServiceLauncher just moves the problem from one > class > > to another... If there's some guarantee that UserSessionManager will be > > initialized by the time ArcServiceLauncher is created I could add a > > SetArcServiceLauncher method, but similar would have to be done for every > future > > use of these classes as well. > > The current model that Arc is using, from a somewhat quick glance, is: > ArcServiceLauncher owns ArcServiceManager. > ArcServiceManager owns a vector of services. > Each service has a static getter, e.g. arc::ArcAuthService::Get(). > > I am proposing that we keep ownership of all arc related global instances in > ArcServiceManager, including ArcUserDataManager. ArcUserDataManager whould still > have a static Get() method, and we would still have potential issues with > construction / destruction order, but at least we are not proliferating the > number of arc globals owned elsewhere (i.e. here). > > Then, ideally, at some point we can pass ArcServiceLauncher (or something > similar) to UserSessionManager (and other classes that use it) and get rid of > the global accessors. Done.
https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_data_manager.h (right): https://codereview.chromium.org/1966133002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_data_manager.h:25: static ArcDataManager* Get(); On 2016/05/13 17:13:54, stevenjb wrote: > On 2016/05/13 01:19:24, dspaid wrote: > > On 2016/05/12 16:33:35, Daniel Erat wrote: > > > if at all possible, please instead pass the instance to the classes that > need > > to > > > call it. static Get() methods are error-prone, as callers have no way of > > knowing > > > when they're safe to be called during startup or shutdown, leading to either > > > speculative null checks or segfaults. > > > > I'm open to suggestions here. The current use is in user_session_manager.cc > > where similar ARC related code is accessed via static Get methods so I opted > for > > consistency. I don't have an easy solution for passing an instance to the > > constructor of UserSessionManager, but I could add a public SetArcDataManager > > method to pass it. However we'd still have the problem of UserSessionManager > > not knowing if the instance had been set or not, so speculative null checks > > would remain. > > We should at least make this consistent with other arc services and initialize > this in ArcServiceLauncher. See other comment. Done. https://codereview.chromium.org/1966133002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_data_manager.h (right): https://codereview.chromium.org/1966133002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_data_manager.h:19: // necessary. On 2016/05/13 17:13:55, stevenjb wrote: > May I suggest renaming this something like ArcUserDataManager? "ARC data > manager" could refer to just about anything related to Arc and data :) Done.
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
drive-by https://codereview.chromium.org/1966133002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/1966133002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/session/user_session_manager.cc:1163: arc::ArcServiceManager::Get()->OnPrimaryUserProfilePrepared(account_id); more or less in the same spirit of other comments in this review: would it be possible to pass both the AccountId and the profile prefs here to avoid adding arc::ArcUserDataService::Get()? https://codereview.chromium.org/1966133002/diff/120001/components/arc.gypi File components/arc.gypi (right): https://codereview.chromium.org/1966133002/diff/120001/components/arc.gypi#ne... components/arc.gypi:77: 'arc/userdata/arc_user_data_service.cc', nit: Use spaces instead of tabs https://codereview.chromium.org/1966133002/diff/120001/components/arc/userdat... File components/arc/userdata/arc_user_data_service.cc (right): https://codereview.chromium.org/1966133002/diff/120001/components/arc/userdat... components/arc/userdata/arc_user_data_service.cc:19: // Weak Pointer. This class is owned by ChromeBrowserMainPartsChromeos. nit: it's now owned by ArcServiceManager. https://codereview.chromium.org/1966133002/diff/120001/components/arc/userdat... components/arc/userdata/arc_user_data_service.cc:44: void ArcUserDataService::OnStateChanged(ArcBridgeService::State state) { OnStateChanged was added for tests and I'd like to avoid adding more consumers. Can you add a OnBridgeReady() and OnBridgeStopped() to the Observer? I'll go fix the other non-test consumer of OnStateChanged() later. https://codereview.chromium.org/1966133002/diff/120001/components/arc/userdat... components/arc/userdata/arc_user_data_service.cc:71: } Shouldn't you return if it's not stopped?
https://codereview.chromium.org/1966133002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/1966133002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/session/user_session_manager.cc:1163: arc::ArcServiceManager::Get()->OnPrimaryUserProfilePrepared(account_id); On 2016/05/17 16:11:55, Luis Héctor Chávez wrote: > more or less in the same spirit of other comments in this review: would it be > possible to pass both the AccountId and the profile prefs here to avoid adding > arc::ArcUserDataService::Get()? Done. https://codereview.chromium.org/1966133002/diff/120001/components/arc.gypi File components/arc.gypi (right): https://codereview.chromium.org/1966133002/diff/120001/components/arc.gypi#ne... components/arc.gypi:77: 'arc/userdata/arc_user_data_service.cc', On 2016/05/17 16:11:55, Luis Héctor Chávez wrote: > nit: Use spaces instead of tabs Done. https://codereview.chromium.org/1966133002/diff/120001/components/arc/userdat... File components/arc/userdata/arc_user_data_service.cc (right): https://codereview.chromium.org/1966133002/diff/120001/components/arc/userdat... components/arc/userdata/arc_user_data_service.cc:19: // Weak Pointer. This class is owned by ChromeBrowserMainPartsChromeos. On 2016/05/17 16:11:55, Luis Héctor Chávez wrote: > nit: it's now owned by ArcServiceManager. Done. https://codereview.chromium.org/1966133002/diff/120001/components/arc/userdat... components/arc/userdata/arc_user_data_service.cc:44: void ArcUserDataService::OnStateChanged(ArcBridgeService::State state) { On 2016/05/17 16:11:55, Luis Héctor Chávez wrote: > OnStateChanged was added for tests and I'd like to avoid adding more consumers. > Can you add a OnBridgeReady() and OnBridgeStopped() to the Observer? I'll go fix > the other non-test consumer of OnStateChanged() later. Done. https://codereview.chromium.org/1966133002/diff/120001/components/arc/userdat... components/arc/userdata/arc_user_data_service.cc:71: } On 2016/05/17 16:11:55, Luis Héctor Chávez wrote: > Shouldn't you return if it's not stopped? Done.
https://codereview.chromium.org/1966133002/diff/140001/components/arc.gypi File components/arc.gypi (right): https://codereview.chromium.org/1966133002/diff/140001/components/arc.gypi#ne... components/arc.gypi:77: 'arc/userdata/arc_user_data_service.cc', it feels weird to have an underscore between "user" and "data" in the filename but not in the directory name. change the directory to "user_data" too? https://codereview.chromium.org/1966133002/diff/140001/components/arc/DEPS File components/arc/DEPS (right): https://codereview.chromium.org/1966133002/diff/140001/components/arc/DEPS#ne... components/arc/DEPS:2: "+chrome/common", i'm not sure that this is permitted. i don't see any existing dependencies on chrome/ in components/*/DEPS right now. given the amount of arc code that lives outside of chrome/, there's probably some other way to check if it's enabled without looking at a chrome pref, right?
https://codereview.chromium.org/1966133002/diff/140001/components/arc/DEPS File components/arc/DEPS (right): https://codereview.chromium.org/1966133002/diff/140001/components/arc/DEPS#ne... components/arc/DEPS:2: "+chrome/common", On 2016/05/18 00:31:26, Daniel Erat wrote: > i'm not sure that this is permitted. i don't see any existing dependencies on > chrome/ in components/*/DEPS right now. > > given the amount of arc code that lives outside of chrome/, there's probably > some other way to check if it's enabled without looking at a chrome pref, right? I was worried about that, and it was one of the reasons that I originally had this in chrome/browser/chromeos/arc... As far as I can tell there's no way to get the arc enabled state outside of looking at the user preferences. Presumably I could also pass the preference key along with the PrefService pointer (or pass a callback to specifically get the kArcEnabled preference), but both seem pretty hacky.
https://codereview.chromium.org/1966133002/diff/140001/components/arc/DEPS File components/arc/DEPS (right): https://codereview.chromium.org/1966133002/diff/140001/components/arc/DEPS#ne... components/arc/DEPS:2: "+chrome/common", On 2016/05/18 00:41:43, dspaid wrote: > On 2016/05/18 00:31:26, Daniel Erat wrote: > > i'm not sure that this is permitted. i don't see any existing dependencies on > > chrome/ in components/*/DEPS right now. > > > > given the amount of arc code that lives outside of chrome/, there's probably > > some other way to check if it's enabled without looking at a chrome pref, > right? > > I was worried about that, and it was one of the reasons that I originally had > this in chrome/browser/chromeos/arc... As far as I can tell there's no way to > get the arc enabled state outside of looking at the user preferences. > Presumably I could also pass the preference key along with the PrefService > pointer (or pass a callback to specifically get the kArcEnabled preference), but > both seem pretty hacky. how will the pref be updated, and is it used by anything else? i only see mentions to it in ArcAuthService in my checkout. do you expect the opt-out state to be needed by other code in components/arc in the future? if so, it seems like it'd be cleaner to pass the opt-out state into some more-central place in the component where it can be distributed to everything that needs it. that'll probably make testing easier as well.
On 2016/05/18 00:58:27, Daniel Erat wrote: > https://codereview.chromium.org/1966133002/diff/140001/components/arc/DEPS > File components/arc/DEPS (right): > > https://codereview.chromium.org/1966133002/diff/140001/components/arc/DEPS#ne... > components/arc/DEPS:2: "+chrome/common", > On 2016/05/18 00:41:43, dspaid wrote: > > On 2016/05/18 00:31:26, Daniel Erat wrote: > > > i'm not sure that this is permitted. i don't see any existing dependencies > on > > > chrome/ in components/*/DEPS right now. > > > > > > given the amount of arc code that lives outside of chrome/, there's probably > > > some other way to check if it's enabled without looking at a chrome pref, > > right? > > > > I was worried about that, and it was one of the reasons that I originally had > > this in chrome/browser/chromeos/arc... As far as I can tell there's no way to > > get the arc enabled state outside of looking at the user preferences. > > Presumably I could also pass the preference key along with the PrefService > > pointer (or pass a callback to specifically get the kArcEnabled preference), > but > > both seem pretty hacky. > > how will the pref be updated, and is it used by anything else? i only see > mentions to it in ArcAuthService in my checkout. > > do you expect the opt-out state to be needed by other code in components/arc in > the future? if so, it seems like it'd be cleaner to pass the opt-out state into > some more-central place in the component where it can be distributed to > everything that needs it. that'll probably make testing easier as well. Pref is updated by the user in their settings. It may also be updated by an administrator in a enterprise setting. I can't think of any other pieces that will need the preference information right now, but I wouldn't be entirely surprised if they came up in the future.
https://codereview.chromium.org/1966133002/diff/140001/components/arc/DEPS File components/arc/DEPS (right): https://codereview.chromium.org/1966133002/diff/140001/components/arc/DEPS#ne... components/arc/DEPS:2: "+chrome/common", On 2016/05/18 00:58:27, Daniel Erat wrote: > On 2016/05/18 00:41:43, dspaid wrote: > > On 2016/05/18 00:31:26, Daniel Erat wrote: > > > i'm not sure that this is permitted. i don't see any existing dependencies > on > > > chrome/ in components/*/DEPS right now. > > > > > > given the amount of arc code that lives outside of chrome/, there's probably > > > some other way to check if it's enabled without looking at a chrome pref, > > right? > > > > I was worried about that, and it was one of the reasons that I originally had > > this in chrome/browser/chromeos/arc... As far as I can tell there's no way to > > get the arc enabled state outside of looking at the user preferences. > > Presumably I could also pass the preference key along with the PrefService > > pointer (or pass a callback to specifically get the kArcEnabled preference), > but > > both seem pretty hacky. > > how will the pref be updated, and is it used by anything else? i only see > mentions to it in ArcAuthService in my checkout. > > do you expect the opt-out state to be needed by other code in components/arc in > the future? if so, it seems like it'd be cleaner to pass the opt-out state into > some more-central place in the component where it can be distributed to > everything that needs it. that'll probably make testing easier as well. chrome/ is definitely not permitted from components :( If you can't inject the dependency, maybe you need to move the code to chrome/browser/chromeos/arc. https://codereview.chromium.org/1966133002/diff/140001/components/arc/arc_bri... File components/arc/arc_bridge_service.cc (right): https://codereview.chromium.org/1966133002/diff/140001/components/arc/arc_bri... components/arc/arc_bridge_service.cc:506: if (state == State::STOPPED) nit: else if
i would try really hard to inject the dependency. it's hopefully straightforward to either notify ArcBridgeService about changes to the pref's value (so other components/arc classes can get it from there) or introduce an ArcUserDataService::Delegate interface that can be implemented within chrome/ to query the value. alternately, is it possible to construct something like a PrefMember (components/prefs/pref_member.h) from within chrome/ and then pass that to the arc class?
Refactored and removed dependency on chrome/common. https://codereview.chromium.org/1966133002/diff/140001/components/arc.gypi File components/arc.gypi (right): https://codereview.chromium.org/1966133002/diff/140001/components/arc.gypi#ne... components/arc.gypi:77: 'arc/userdata/arc_user_data_service.cc', On 2016/05/18 00:31:26, Daniel Erat wrote: > it feels weird to have an underscore between "user" and "data" in the filename > but not in the directory name. change the directory to "user_data" too? Done. https://codereview.chromium.org/1966133002/diff/140001/components/arc/DEPS File components/arc/DEPS (right): https://codereview.chromium.org/1966133002/diff/140001/components/arc/DEPS#ne... components/arc/DEPS:2: "+chrome/common", On 2016/05/19 16:10:27, Luis Héctor Chávez (slow 5-23) wrote: > On 2016/05/18 00:58:27, Daniel Erat wrote: > > On 2016/05/18 00:41:43, dspaid wrote: > > > On 2016/05/18 00:31:26, Daniel Erat wrote: > > > > i'm not sure that this is permitted. i don't see any existing dependencies > > on > > > > chrome/ in components/*/DEPS right now. > > > > > > > > given the amount of arc code that lives outside of chrome/, there's > probably > > > > some other way to check if it's enabled without looking at a chrome pref, > > > right? > > > > > > I was worried about that, and it was one of the reasons that I originally > had > > > this in chrome/browser/chromeos/arc... As far as I can tell there's no way > to > > > get the arc enabled state outside of looking at the user preferences. > > > Presumably I could also pass the preference key along with the PrefService > > > pointer (or pass a callback to specifically get the kArcEnabled preference), > > but > > > both seem pretty hacky. > > > > how will the pref be updated, and is it used by anything else? i only see > > mentions to it in ArcAuthService in my checkout. > > > > do you expect the opt-out state to be needed by other code in components/arc > in > > the future? if so, it seems like it'd be cleaner to pass the opt-out state > into > > some more-central place in the component where it can be distributed to > > everything that needs it. that'll probably make testing easier as well. > > chrome/ is definitely not permitted from components :( If you can't inject the > dependency, maybe you need to move the code to chrome/browser/chromeos/arc. Done. https://codereview.chromium.org/1966133002/diff/140001/components/arc/arc_bri... File components/arc/arc_bridge_service.cc (right): https://codereview.chromium.org/1966133002/diff/140001/components/arc/arc_bri... components/arc/arc_bridge_service.cc:506: if (state == State::STOPPED) On 2016/05/19 16:10:27, Luis Héctor Chávez (slow 5-23) wrote: > nit: else if Done.
https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... components/arc/arc_service_manager.cc:67: arc_user_data_service_.reset(new ArcUserDataService(this)); nit: arc_user_data_service_ = base::MakeUnique<ArcUserDataService>(this); https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... components/arc/arc_service_manager.cc:103: if (arc_enabled_pref_) DCHECK(thread_checker_.CalledOnValidThread()); https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... components/arc/arc_service_manager.cc:105: arc_enabled_pref_ = std::unique_ptr<BooleanPrefMember>( nit: arc_enabled_pref_ = base::MakeUnique<BooleanPrefMember>(); https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... components/arc/arc_service_manager.cc:116: return arc_enabled_pref_ && arc_enabled_pref_->GetValue(); DCHECK(thread_checker_.CalledOnValidThread()); https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... components/arc/arc_service_manager.h:47: std::string pref_name); nit: const std::string& pref_name https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... components/arc/arc_service_manager.h:50: bool IsArcEnabled(); bool IsArcEnabled() const; https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... components/arc/arc_service_manager.h:79: std::unique_ptr<BooleanPrefMember> arc_enabled_pref_; nit: Comment? https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... File components/arc/user_data/arc_user_data_service.h (right): https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:24: ArcUserDataService(ArcServiceManager* arc_service_manager); |arc_service_manager| can be made const. https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:27: // Checks if arc is both stopped and disabled (opt-out) and triggers removal nit: s/opt-out/not opt-in/. https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:32: // the user has opted out. nit: s/opted out/not opted-in/. https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:40: ArcServiceManager* arc_service_manager_; nit: const ArcServiceManager* const arc_service_manager_;
https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service.h:71: virtual void OnBridgeReady() {} adding these seems strange. why can't observers just use OnStateChanged() to get the same information? https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... components/arc/arc_service_manager.cc:77: if (arc_enabled_pref_) it doesn't seem like you need this. PrefMemberBase's d'tor already calls Destroy(). https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... components/arc/arc_service_manager.cc:104: arc_enabled_pref_->Destroy(); you shouldn't need this here either https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... components/arc/arc_service_manager.h:47: std::string pref_name); On 2016/05/20 15:03:58, Luis Héctor Chávez (slow 5-23) wrote: > nit: const std::string& pref_name "enabled_pref_name"? also document what the pref contains in the comment. or better, maybe just pass a unique_ptr<BooleanPrefMember> here instead of the service and name? https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... components/arc/arc_service_manager.h:49: // Has the user enabled arc in their preferences. nit: end with question mark https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... File components/arc/user_data/arc_user_data_service.cc (right): https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.cc:17: arc_service_manager_ = arc_service_manager; assign this in an initialization list instead https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.cc:25: void ArcUserDataService::OnBridgeStopped() { method order in .cc should match .h https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.cc:29: if (account_id != user_prefs_account_id_) { how does this happen? would it be cleaner to check it in ArcServiceManager? https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.cc:30: LOG(WARNING) << "User preferences not loaded for primary user"; nit: include both ids in the warning message to make it easier to track down what went wrong? https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.cc:39: user_prefs_account_id_ = account_id; i'm confused about how this works. it looks like this member is only assigned here, but it's checked in OnBridgeStopped(), which calls this method. do you always expect ClearIfDisabled() to be called at least once before OnBridgeStopped() is called? https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... File components/arc/user_data/arc_user_data_service.h (right): https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:24: ArcUserDataService(ArcServiceManager* arc_service_manager); On 2016/05/20 15:03:59, Luis Héctor Chávez (slow 5-23) wrote: > |arc_service_manager| can be made const. add 'explicit' too https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:27: // Checks if arc is both stopped and disabled (opt-out) and triggers removal On 2016/05/20 15:03:59, Luis Héctor Chávez (slow 5-23) wrote: > nit: s/opt-out/not opt-in/. s/arc/ARC/ here and in other comments https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:33: // ArcBridgeService::Observer: nit: move this comment above "Called whenever" (but i'd probably just delete it; usually overridden methods aren't commented in headers) https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:39: AccountId user_prefs_account_id_ = EmptyAccountId(); add a comment documenting what this contains; also add a blank line after it https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:41: DISALLOW_COPY_AND_ASSIGN(ArcUserDataService); nit: add blank line before this one
https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service.h:71: virtual void OnBridgeReady() {} On 2016/05/20 17:41:05, Daniel Erat wrote: > adding these seems strange. why can't observers just use OnStateChanged() to get > the same information? Luis commented that OnStateChanged was originally intended for testing and requested that I add these new methods instead. https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... components/arc/arc_service_manager.cc:67: arc_user_data_service_.reset(new ArcUserDataService(this)); On 2016/05/20 15:03:58, Luis Héctor Chávez (slow 5-23) wrote: > nit: arc_user_data_service_ = base::MakeUnique<ArcUserDataService>(this); Done. https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... components/arc/arc_service_manager.cc:77: if (arc_enabled_pref_) On 2016/05/20 17:41:05, Daniel Erat wrote: > it doesn't seem like you need this. PrefMemberBase's d'tor already calls > Destroy(). Done. https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... components/arc/arc_service_manager.cc:103: if (arc_enabled_pref_) On 2016/05/20 15:03:58, Luis Héctor Chávez (slow 5-23) wrote: > DCHECK(thread_checker_.CalledOnValidThread()); Done. https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... components/arc/arc_service_manager.cc:104: arc_enabled_pref_->Destroy(); On 2016/05/20 17:41:05, Daniel Erat wrote: > you shouldn't need this here either Done. https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... components/arc/arc_service_manager.cc:105: arc_enabled_pref_ = std::unique_ptr<BooleanPrefMember>( On 2016/05/20 15:03:58, Luis Héctor Chávez (slow 5-23) wrote: > nit: arc_enabled_pref_ = base::MakeUnique<BooleanPrefMember>(); Acknowledged. https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... components/arc/arc_service_manager.cc:116: return arc_enabled_pref_ && arc_enabled_pref_->GetValue(); On 2016/05/20 15:03:58, Luis Héctor Chávez (slow 5-23) wrote: > DCHECK(thread_checker_.CalledOnValidThread()); Done. https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... components/arc/arc_service_manager.h:47: std::string pref_name); On 2016/05/20 17:41:05, Daniel Erat wrote: > On 2016/05/20 15:03:58, Luis Héctor Chávez (slow 5-23) wrote: > > nit: const std::string& pref_name > > "enabled_pref_name"? also document what the pref contains in the comment. > > or better, maybe just pass a unique_ptr<BooleanPrefMember> here instead of the > service and name? Done. https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... components/arc/arc_service_manager.h:49: // Has the user enabled arc in their preferences. On 2016/05/20 17:41:05, Daniel Erat wrote: > nit: end with question mark Done. https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... components/arc/arc_service_manager.h:50: bool IsArcEnabled(); On 2016/05/20 15:03:58, Luis Héctor Chávez (slow 5-23) wrote: > bool IsArcEnabled() const; Done. https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_ser... components/arc/arc_service_manager.h:79: std::unique_ptr<BooleanPrefMember> arc_enabled_pref_; On 2016/05/20 15:03:58, Luis Héctor Chávez (slow 5-23) wrote: > nit: Comment? Done. https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... File components/arc/user_data/arc_user_data_service.cc (right): https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.cc:17: arc_service_manager_ = arc_service_manager; On 2016/05/20 17:41:06, Daniel Erat wrote: > assign this in an initialization list instead Done. https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.cc:25: void ArcUserDataService::OnBridgeStopped() { On 2016/05/20 17:41:06, Daniel Erat wrote: > method order in .cc should match .h Done. https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.cc:29: if (account_id != user_prefs_account_id_) { On 2016/05/20 17:41:05, Daniel Erat wrote: > how does this happen? would it be cleaner to check it in ArcServiceManager? As far as I know it shouldn't happen. But if for some reason it did it would be very bad (data loss) so I'd rather check for it. I don't think it would be any cleaner to check from ArcServiceManager, since this method is called directly from ArcBridgeService and any logic in ArcServiceManager would probably have to be as some kind of state return in IsArcEnabled. https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.cc:30: LOG(WARNING) << "User preferences not loaded for primary user"; On 2016/05/20 17:41:06, Daniel Erat wrote: > nit: include both ids in the warning message to make it easier to track down > what went wrong? Done. https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.cc:39: user_prefs_account_id_ = account_id; On 2016/05/20 17:41:06, Daniel Erat wrote: > i'm confused about how this works. it looks like this member is only assigned > here, but it's checked in OnBridgeStopped(), which calls this method. do you > always expect ClearIfDisabled() to be called at least once before > OnBridgeStopped() is called? Yes, ClearIfDisabled is called explicitly from OnPrimaryProfilePrepared, which should always be called before the bridge is started. Its unfortunately very indirect, but its the best I could come up with given the difficulty in accessing preferences from components/arc. https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... File components/arc/user_data/arc_user_data_service.h (right): https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:24: ArcUserDataService(ArcServiceManager* arc_service_manager); On 2016/05/20 15:03:59, Luis Héctor Chávez (slow 5-23) wrote: > |arc_service_manager| can be made const. Done. https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:24: ArcUserDataService(ArcServiceManager* arc_service_manager); On 2016/05/20 17:41:06, Daniel Erat wrote: > On 2016/05/20 15:03:59, Luis Héctor Chávez (slow 5-23) wrote: > > |arc_service_manager| can be made const. > > add 'explicit' too Done. https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:27: // Checks if arc is both stopped and disabled (opt-out) and triggers removal On 2016/05/20 15:03:59, Luis Héctor Chávez (slow 5-23) wrote: > nit: s/opt-out/not opt-in/. Done. https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:27: // Checks if arc is both stopped and disabled (opt-out) and triggers removal On 2016/05/20 17:41:06, Daniel Erat wrote: > On 2016/05/20 15:03:59, Luis Héctor Chávez (slow 5-23) wrote: > > nit: s/opt-out/not opt-in/. > > s/arc/ARC/ here and in other comments Done. https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:32: // the user has opted out. On 2016/05/20 15:03:59, Luis Héctor Chávez (slow 5-23) wrote: > nit: s/opted out/not opted-in/. Done. https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:33: // ArcBridgeService::Observer: On 2016/05/20 17:41:06, Daniel Erat wrote: > nit: move this comment above "Called whenever" (but i'd probably just delete it; > usually overridden methods aren't commented in headers) Done. https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:39: AccountId user_prefs_account_id_ = EmptyAccountId(); On 2016/05/20 17:41:06, Daniel Erat wrote: > add a comment documenting what this contains; also add a blank line after it Done. https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:40: ArcServiceManager* arc_service_manager_; On 2016/05/20 15:03:58, Luis Héctor Chávez (slow 5-23) wrote: > nit: const ArcServiceManager* const arc_service_manager_; Done. https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:41: DISALLOW_COPY_AND_ASSIGN(ArcUserDataService); On 2016/05/20 17:41:06, Daniel Erat wrote: > nit: add blank line before this one Done.
https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service.h:71: virtual void OnBridgeReady() {} On 2016/05/23 01:22:36, dspaid wrote: > On 2016/05/20 17:41:05, Daniel Erat wrote: > > adding these seems strange. why can't observers just use OnStateChanged() to > get > > the same information? > > Luis commented that OnStateChanged was originally intended for testing and > requested that I add these new methods instead. luis, can you add some more details here? is the issue that the only states that non-test consumers should care about are STOPPED and READY? if so, could you also rename OnStateChanged to OnStateChangedForTest (if you weren't planning to do so already)? https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... File components/arc/user_data/arc_user_data_service.cc (right): https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.cc:29: if (account_id != user_prefs_account_id_) { On 2016/05/23 01:22:37, dspaid wrote: > On 2016/05/20 17:41:05, Daniel Erat wrote: > > how does this happen? would it be cleaner to check it in ArcServiceManager? > > As far as I know it shouldn't happen. But if for some reason it did it would be > very bad (data loss) so I'd rather check for it. > I don't think it would be any cleaner to check from ArcServiceManager, since > this method is called directly from ArcBridgeService and any logic in > ArcServiceManager would probably have to be as some kind of state return in > IsArcEnabled. mind making this a LOG(ERROR), then?
https://codereview.chromium.org/1966133002/diff/200001/components/arc/arc_ser... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1966133002/diff/200001/components/arc/arc_ser... components/arc/arc_service_manager.h:49: // Has the user enabled arc in their preferences? nit: s/arc/ARC/ https://codereview.chromium.org/1966133002/diff/200001/components/arc/arc_ser... components/arc/arc_service_manager.h:79: // User preference indicating whether or not arc has been enabled. nit: s/arc/ARC/ https://codereview.chromium.org/1966133002/diff/200001/components/arc/user_da... File components/arc/user_data/arc_user_data_service.h (right): https://codereview.chromium.org/1966133002/diff/200001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:29: void ClearIfDisabled(const AccountId& account_id); i think that this would be clearer if it were named OnPrimaryUserProfilePrepared or SetPrimaryUserAccountId or something similar. that way, it's obvious when it's supposed to be called, and it makes more sense that it's setting user_prefs_account_id_ (which should probably also be renamed to something like primary_user_account_id_), and all the logic about what actually needs to be done (i.e. clearing user data if arc is disabled) will be contained within this class. https://codereview.chromium.org/1966133002/diff/200001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:31: // Called whenever the arc bridge is stopped to potentially remove data if nit: s/arc/ARC/ also add a comment first describing where this method comes from, e.g. // ArcBridgeService::Observer:
https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service.h:71: virtual void OnBridgeReady() {} On 2016/05/24 14:51:22, Daniel Erat wrote: > On 2016/05/23 01:22:36, dspaid wrote: > > On 2016/05/20 17:41:05, Daniel Erat wrote: > > > adding these seems strange. why can't observers just use OnStateChanged() to > > get > > > the same information? > > > > Luis commented that OnStateChanged was originally intended for testing and > > requested that I add these new methods instead. > > luis, can you add some more details here? is the issue that the only states that > non-test consumers should care about are STOPPED and READY? if so, could you > also rename OnStateChanged to OnStateChangedForTest (if you weren't planning to > do so already)? changing to OnStateChangedForTest SGTM.
https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service.h:71: virtual void OnBridgeReady() {} On 2016/05/24 16:18:28, Luis Héctor Chávez wrote: > On 2016/05/24 14:51:22, Daniel Erat wrote: > > On 2016/05/23 01:22:36, dspaid wrote: > > > On 2016/05/20 17:41:05, Daniel Erat wrote: > > > > adding these seems strange. why can't observers just use OnStateChanged() > to > > > get > > > > the same information? > > > > > > Luis commented that OnStateChanged was originally intended for testing and > > > requested that I add these new methods instead. > > > > luis, can you add some more details here? is the issue that the only states > that > > non-test consumers should care about are STOPPED and READY? if so, could you > > also rename OnStateChanged to OnStateChangedForTest (if you weren't planning > to > > do so already)? > > changing to OnStateChangedForTest SGTM. Luis, is this something you'll do in the cleanup CL you mentioned earlier, or do you want me to put together another CL for that? I'd rather not increase the scope of this CL. https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... File components/arc/user_data/arc_user_data_service.cc (right): https://codereview.chromium.org/1966133002/diff/180001/components/arc/user_da... components/arc/user_data/arc_user_data_service.cc:29: if (account_id != user_prefs_account_id_) { On 2016/05/24 14:51:22, Daniel Erat wrote: > On 2016/05/23 01:22:37, dspaid wrote: > > On 2016/05/20 17:41:05, Daniel Erat wrote: > > > how does this happen? would it be cleaner to check it in ArcServiceManager? > > > > As far as I know it shouldn't happen. But if for some reason it did it would > be > > very bad (data loss) so I'd rather check for it. > > I don't think it would be any cleaner to check from ArcServiceManager, since > > this method is called directly from ArcBridgeService and any logic in > > ArcServiceManager would probably have to be as some kind of state return in > > IsArcEnabled. > > mind making this a LOG(ERROR), then? Done. https://codereview.chromium.org/1966133002/diff/200001/components/arc/arc_ser... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1966133002/diff/200001/components/arc/arc_ser... components/arc/arc_service_manager.h:49: // Has the user enabled arc in their preferences? On 2016/05/24 16:00:01, Daniel Erat wrote: > nit: s/arc/ARC/ Done. https://codereview.chromium.org/1966133002/diff/200001/components/arc/arc_ser... components/arc/arc_service_manager.h:79: // User preference indicating whether or not arc has been enabled. On 2016/05/24 16:00:01, Daniel Erat wrote: > nit: s/arc/ARC/ Done. https://codereview.chromium.org/1966133002/diff/200001/components/arc/user_da... File components/arc/user_data/arc_user_data_service.h (right): https://codereview.chromium.org/1966133002/diff/200001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:29: void ClearIfDisabled(const AccountId& account_id); On 2016/05/24 16:00:01, Daniel Erat wrote: > i think that this would be clearer if it were named OnPrimaryUserProfilePrepared > or SetPrimaryUserAccountId or something similar. that way, it's obvious when > it's supposed to be called, and it makes more sense that it's setting > user_prefs_account_id_ (which should probably also be renamed to something like > primary_user_account_id_), and all the logic about what actually needs to be > done (i.e. clearing user data if arc is disabled) will be contained within this > class. Done. https://codereview.chromium.org/1966133002/diff/200001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:31: // Called whenever the arc bridge is stopped to potentially remove data if On 2016/05/24 16:00:01, Daniel Erat wrote: > nit: s/arc/ARC/ > > also add a comment first describing where this method comes from, e.g. > > // ArcBridgeService::Observer: Done.
https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service.h:71: virtual void OnBridgeReady() {} On 2016/05/25 00:06:48, dspaid wrote: > On 2016/05/24 16:18:28, Luis Héctor Chávez wrote: > > On 2016/05/24 14:51:22, Daniel Erat wrote: > > > On 2016/05/23 01:22:36, dspaid wrote: > > > > On 2016/05/20 17:41:05, Daniel Erat wrote: > > > > > adding these seems strange. why can't observers just use > OnStateChanged() > > to > > > > get > > > > > the same information? > > > > > > > > Luis commented that OnStateChanged was originally intended for testing and > > > > requested that I add these new methods instead. > > > > > > luis, can you add some more details here? is the issue that the only states > > that > > > non-test consumers should care about are STOPPED and READY? if so, could you > > > also rename OnStateChanged to OnStateChangedForTest (if you weren't planning > > to > > > do so already)? > > > > changing to OnStateChangedForTest SGTM. > > Luis, is this something you'll do in the cleanup CL you mentioned earlier, or do > you want me to put together another CL for that? I'd rather not increase the > scope of this CL. it's fine to defer for later, just add a TODO for me.
https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1966133002/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service.h:71: virtual void OnBridgeReady() {} On 2016/05/25 00:11:01, Luis Héctor Chávez wrote: > On 2016/05/25 00:06:48, dspaid wrote: > > On 2016/05/24 16:18:28, Luis Héctor Chávez wrote: > > > On 2016/05/24 14:51:22, Daniel Erat wrote: > > > > On 2016/05/23 01:22:36, dspaid wrote: > > > > > On 2016/05/20 17:41:05, Daniel Erat wrote: > > > > > > adding these seems strange. why can't observers just use > > OnStateChanged() > > > to > > > > > get > > > > > > the same information? > > > > > > > > > > Luis commented that OnStateChanged was originally intended for testing > and > > > > > requested that I add these new methods instead. > > > > > > > > luis, can you add some more details here? is the issue that the only > states > > > that > > > > non-test consumers should care about are STOPPED and READY? if so, could > you > > > > also rename OnStateChanged to OnStateChangedForTest (if you weren't > planning > > > to > > > > do so already)? > > > > > > changing to OnStateChangedForTest SGTM. > > > > Luis, is this something you'll do in the cleanup CL you mentioned earlier, or > do > > you want me to put together another CL for that? I'd rather not increase the > > scope of this CL. > > it's fine to defer for later, just add a TODO for me. Done.
forgot to ask earlier: do you plan to add unit tests for this? https://codereview.chromium.org/1966133002/diff/240001/components/arc/user_da... File components/arc/user_data/arc_user_data_service.cc (right): https://codereview.chromium.org/1966133002/diff/240001/components/arc/user_da... components/arc/user_data/arc_user_data_service.cc:48: if (ArcBridgeService::Get()->state() != ArcBridgeService::State::STOPPED) { is the bridge always expected to be stopped when the primary user profile is prepared?
Trying to get tests figured out... Having problems getting a fake version of ArcServiceManager injected without more significant refactoring. https://codereview.chromium.org/1966133002/diff/240001/components/arc/user_da... File components/arc/user_data/arc_user_data_service.cc (right): https://codereview.chromium.org/1966133002/diff/240001/components/arc/user_da... components/arc/user_data/arc_user_data_service.cc:48: if (ArcBridgeService::Get()->state() != ArcBridgeService::State::STOPPED) { On 2016/05/25 02:55:26, Daniel Erat wrote: > is the bridge always expected to be stopped when the primary user profile is > prepared? Yes
After trying various things I wasn't particularly happy with any of the testing due to the way the other arc classes are structured. Given that I'm about to start on chrome OS autotests to cover all of this behavior I'd like to submit this as is and re-visit unit tests in the future.
lgtm
owner lgtm https://codereview.chromium.org/1966133002/diff/240001/components/arc/user_da... File components/arc/user_data/arc_user_data_service.cc (right): https://codereview.chromium.org/1966133002/diff/240001/components/arc/user_da... components/arc/user_data/arc_user_data_service.cc:54: } nit: no {}
https://codereview.chromium.org/1966133002/diff/240001/components/arc/user_da... File components/arc/user_data/arc_user_data_service.cc (right): https://codereview.chromium.org/1966133002/diff/240001/components/arc/user_da... components/arc/user_data/arc_user_data_service.cc:54: } On 2016/06/08 03:38:48, stevenjb wrote: > nit: no {} Done.
dspaid@chromium.org changed reviewers: + atwilson@google.com
+atwilson for update to test fix in chrome/browser/policy Had to add a few files to update tests. Also ended up having to explicitly call Destroy on the PrefMember for unit tests to pass.
+atwilson for update to test fix in chrome/browser/policy Had to add a few files to update tests. Also ended up having to explicitly call Destroy on the PrefMember for unit tests to pass.
bartfab@chromium.org changed reviewers: + bartfab@chromium.org
https://codereview.chromium.org/1966133002/diff/340001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/1966133002/diff/340001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:114: #include "components/prefs/pref_member.h" This file does not declare any PrefMembers. It looks like the including is missing in some other file further upstream instead.
On 2016/06/10 11:05:40, bartfab (slow) wrote: > https://codereview.chromium.org/1966133002/diff/340001/chrome/browser/policy/... > File chrome/browser/policy/policy_browsertest.cc (right): > > https://codereview.chromium.org/1966133002/diff/340001/chrome/browser/policy/... > chrome/browser/policy/policy_browsertest.cc:114: #include > "components/prefs/pref_member.h" > This file does not declare any PrefMembers. It looks like the including is > missing in some other file further upstream instead. Sorry for the spam, looks like the file changed between rebases and the parts I needed to change are gone now. File removed from the CL.
The CQ bit was checked by dspaid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, derat@chromium.org Link to the patchset: https://codereview.chromium.org/1966133002/#ps360001 (title: "Removed unneeded import")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1966133002/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Luis: If you could take another look and LGTM if it looks good I need your LGTM for the components/arc stuff.
Hi dspaid, As we chatted offline, could you please send cryptohome ID in RemoveArcData() so we can locate android-data directory? Getting cryptohome ID is very simple. Please refer: https://codereview.chromium.org/2023803003/
On 2016/06/13 04:21:23, Shuhei Takahashi wrote: > Hi dspaid, > > As we chatted offline, could you please send cryptohome ID in RemoveArcData() so > we can locate android-data directory? > > Getting cryptohome ID is very simple. Please refer: > https://codereview.chromium.org/2023803003/ Done
On 2016/06/13 01:57:58, dspaid wrote: > Luis: > If you could take another look and LGTM if it looks good I need your LGTM for > the components/arc stuff. hidehiko is also an owner, we don't need to wait 24 hours.
On 2016/06/13 06:41:26, Junichi Uekawa wrote: > On 2016/06/13 01:57:58, dspaid wrote: > > Luis: > > If you could take another look and LGTM if it looks good I need your LGTM for > > the components/arc stuff. > > hidehiko is also an owner, we don't need to wait 24 hours. Yes, I know. However Luis has already looked through and made comments on the CL, and hidehiko has more pending reviews right now. I can re-assign if you want it in today, but otherwise I'd rather keep current owners.
hidehiko@chromium.org changed reviewers: + hidehiko@chromium.org
I'm happy to help unblock you, and that's why we have an OWNER, me, in our office. :-). https://codereview.chromium.org/1966133002/diff/380001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1966133002/diff/380001/components/arc/arc_bri... components/arc/arc_bridge_service.h:72: virtual void OnBridgeReady() {} Unused. Please remove. https://codereview.chromium.org/1966133002/diff/380001/components/arc/arc_ser... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/1966133002/diff/380001/components/arc/arc_ser... components/arc/arc_service_manager.cc:131: arc_enabled_pref_ = nullptr; nit: arc_enabled_pref_.reset() seems more commonly used? https://codereview.chromium.org/1966133002/diff/380001/components/arc/user_da... File components/arc/user_data/arc_user_data_service.cc (right): https://codereview.chromium.org/1966133002/diff/380001/components/arc/user_da... components/arc/user_data/arc_user_data_service.cc:19: ArcBridgeService::Get()->AddObserver(this); Please use arc_bridge_service(), with inheriting ArcService. (Please see also my comment in the header file). https://codereview.chromium.org/1966133002/diff/380001/components/arc/user_da... components/arc/user_data/arc_user_data_service.cc:49: if (ArcBridgeService::Get()->state() != ArcBridgeService::State::STOPPED) { Ditto for arc_bridge_service(). https://codereview.chromium.org/1966133002/diff/380001/components/arc/user_da... File components/arc/user_data/arc_user_data_service.h (right): https://codereview.chromium.org/1966133002/diff/380001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:22: class ArcUserDataService : public ArcBridgeService::Observer { Please inherit ArcService, so that the lifetime management can be aligned to other ArcService classes. https://codereview.chromium.org/1966133002/diff/380001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:46: const ArcServiceManager* const arc_service_manager_; Please note that an instance of this class is managed by this arc_service_manager_, so that it is ensured that arc_service_manager_ is alive while the instance is alive.
https://codereview.chromium.org/1966133002/diff/380001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1966133002/diff/380001/components/arc/arc_bri... components/arc/arc_bridge_service.h:72: virtual void OnBridgeReady() {} On 2016/06/13 09:05:04, hidehiko wrote: > Unused. Please remove. This was explicitly requested by Luis. https://codereview.chromium.org/1966133002/diff/380001/components/arc/arc_ser... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/1966133002/diff/380001/components/arc/arc_ser... components/arc/arc_service_manager.cc:131: arc_enabled_pref_ = nullptr; On 2016/06/13 09:05:04, hidehiko wrote: > nit: arc_enabled_pref_.reset() seems more commonly used? Done. https://codereview.chromium.org/1966133002/diff/380001/components/arc/user_da... File components/arc/user_data/arc_user_data_service.cc (right): https://codereview.chromium.org/1966133002/diff/380001/components/arc/user_da... components/arc/user_data/arc_user_data_service.cc:19: ArcBridgeService::Get()->AddObserver(this); On 2016/06/13 09:05:04, hidehiko wrote: > Please use arc_bridge_service(), with inheriting ArcService. (Please see also my > comment in the header file). Done. https://codereview.chromium.org/1966133002/diff/380001/components/arc/user_da... components/arc/user_data/arc_user_data_service.cc:49: if (ArcBridgeService::Get()->state() != ArcBridgeService::State::STOPPED) { On 2016/06/13 09:05:05, hidehiko wrote: > Ditto for arc_bridge_service(). Done. https://codereview.chromium.org/1966133002/diff/380001/components/arc/user_da... File components/arc/user_data/arc_user_data_service.h (right): https://codereview.chromium.org/1966133002/diff/380001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:22: class ArcUserDataService : public ArcBridgeService::Observer { On 2016/06/13 09:05:05, hidehiko wrote: > Please inherit ArcService, so that the lifetime management can be aligned to > other ArcService classes. Done https://codereview.chromium.org/1966133002/diff/380001/components/arc/user_da... components/arc/user_data/arc_user_data_service.h:46: const ArcServiceManager* const arc_service_manager_; On 2016/06/13 09:05:05, hidehiko wrote: > Please note that an instance of this class is managed by this > arc_service_manager_, so that it is ensured that arc_service_manager_ is alive > while the instance is alive. Done.
It looks the newest revision is not uploaded? https://codereview.chromium.org/1966133002/diff/380001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1966133002/diff/380001/components/arc/arc_bri... components/arc/arc_bridge_service.h:72: virtual void OnBridgeReady() {} On 2016/06/14 00:38:17, dspaid wrote: > On 2016/06/13 09:05:04, hidehiko wrote: > > Unused. Please remove. > > This was explicitly requested by Luis. Ah, got it. There is a plan to refactor https://cs.chromium.org/chromium/src/chrome/browser/ui/app_list/arc/arc_app_l... I think it's better to added this in that CL, but it's not a big problem then.
https://codereview.chromium.org/1966133002/diff/400001/components/arc/arc_ser... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/1966133002/diff/400001/components/arc/arc_ser... components/arc/arc_service_manager.cc:71: arc_user_data_service_ = new ArcUserDataService(this, arc_bridge_service()); Can we move the instance creation into OnPrimaryUserProfilePrepared(), similar to ArcNotificationManager? https://codereview.chromium.org/1966133002/diff/400001/components/arc/arc_ser... components/arc/arc_service_manager.cc:106: arc_enabled_pref_ = std::move(arc_enabled_pref); Could you pass the instance to the ArcUserDataService's ctor, and let it keep the instance's ownership? Then, you don't need IsArcEnabled in this class, instead ArcUserDataService can check the value directly. And, you can split the cycle deps, too. https://codereview.chromium.org/1966133002/diff/400001/components/arc/arc_ser... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1966133002/diff/400001/components/arc/arc_ser... components/arc/arc_service_manager.h:9: #include <string> unused?
https://codereview.chromium.org/1966133002/diff/400001/components/arc/arc_ser... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/1966133002/diff/400001/components/arc/arc_ser... components/arc/arc_service_manager.cc:71: arc_user_data_service_ = new ArcUserDataService(this, arc_bridge_service()); On 2016/06/14 02:05:15, hidehiko wrote: > Can we move the instance creation into OnPrimaryUserProfilePrepared(), similar > to ArcNotificationManager? Done. https://codereview.chromium.org/1966133002/diff/400001/components/arc/arc_ser... components/arc/arc_service_manager.cc:106: arc_enabled_pref_ = std::move(arc_enabled_pref); On 2016/06/14 02:05:15, hidehiko wrote: > Could you pass the instance to the ArcUserDataService's ctor, and let it keep > the instance's ownership? > Then, you don't need IsArcEnabled in this class, instead ArcUserDataService can > check the value directly. And, you can split the cycle deps, too. Done. https://codereview.chromium.org/1966133002/diff/400001/components/arc/arc_ser... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1966133002/diff/400001/components/arc/arc_ser... components/arc/arc_service_manager.h:9: #include <string> On 2016/06/14 02:05:15, hidehiko wrote: > unused? Done.
Mostly style comments, except missing AddObserver(). https://codereview.chromium.org/1966133002/diff/420001/components/arc/arc_ser... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/1966133002/diff/420001/components/arc/arc_ser... components/arc/arc_service_manager.cc:70: nit: unnecessary. https://codereview.chromium.org/1966133002/diff/420001/components/arc/arc_ser... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1966133002/diff/420001/components/arc/arc_ser... components/arc/arc_service_manager.h:23: class ArcUserDataService; nit: unnecessary. https://codereview.chromium.org/1966133002/diff/420001/components/arc/user_da... File components/arc/user_data/arc_user_data_service.cc (right): https://codereview.chromium.org/1966133002/diff/420001/components/arc/user_da... components/arc/user_data/arc_user_data_service.cc:20: arc_enabled_pref_(std::move(arc_enabled_pref)) {} I think, you accidentally removed AddObserver from here? https://codereview.chromium.org/1966133002/diff/420001/components/arc/user_da... components/arc/user_data/arc_user_data_service.cc:42: const AccountId& account_id) { Could you pass this to the ctor, too, to be aligned with ArcNotificationManager? https://codereview.chromium.org/1966133002/diff/420001/components/arc/user_da... components/arc/user_data/arc_user_data_service.cc:44: ClearIfDisabled(); nit: Maybe move this to ctor, too?
https://codereview.chromium.org/1966133002/diff/420001/components/arc/arc_ser... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/1966133002/diff/420001/components/arc/arc_ser... components/arc/arc_service_manager.cc:70: On 2016/06/14 05:13:01, hidehiko wrote: > nit: unnecessary. Done. https://codereview.chromium.org/1966133002/diff/420001/components/arc/user_da... File components/arc/user_data/arc_user_data_service.cc (right): https://codereview.chromium.org/1966133002/diff/420001/components/arc/user_da... components/arc/user_data/arc_user_data_service.cc:20: arc_enabled_pref_(std::move(arc_enabled_pref)) {} On 2016/06/14 05:13:02, hidehiko wrote: > I think, you accidentally removed AddObserver from here? Done. https://codereview.chromium.org/1966133002/diff/420001/components/arc/user_da... components/arc/user_data/arc_user_data_service.cc:42: const AccountId& account_id) { On 2016/06/14 05:13:01, hidehiko wrote: > Could you pass this to the ctor, too, to be aligned with ArcNotificationManager? Done. https://codereview.chromium.org/1966133002/diff/420001/components/arc/user_da... components/arc/user_data/arc_user_data_service.cc:44: ClearIfDisabled(); On 2016/06/14 05:13:01, hidehiko wrote: > nit: Maybe move this to ctor, too? Done.
lgtm
The CQ bit was checked by dspaid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, derat@chromium.org Link to the patchset: https://codereview.chromium.org/1966133002/#ps440001 (title: "Adressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1966133002/440001
Message was sent while issue was closed.
Committed patchset #23 (id:440001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Run RemoveArcData after a user has opted out BUG=26784296 TEST=Manual testingin conjunction with chromeos side changes. ========== to ========== Run RemoveArcData after a user has opted out BUG=26784296 TEST=Manual testingin conjunction with chromeos side changes. Committed: https://crrev.com/102f85d50db963e44004c991133c7c5e78844e7f Cr-Commit-Position: refs/heads/master@{#399798} ==========
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/102f85d50db963e44004c991133c7c5e78844e7f Cr-Commit-Position: refs/heads/master@{#399798} |
