Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2711033002:
Fix Arc integration test. (Closed)
|
Created:
3 years, 10 months ago by lgcheng Modified:
3 years, 9 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, sync-reviews_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix Arc integration test.
After all the refactor/clean up of ArcSessionManager, clean up Arc
sync integration test setup/shut down logic and stop confusing
ArcSessionManager state machine with sync test profile.
Minor cleanup for ArcPackageSyncDataTypeController.
BUG=694261
TEST=Run sync_integration_test --gtest_filter=*Arc*. All Arc related tests passed.
Review-Url: https://codereview.chromium.org/2711033002
Cr-Commit-Position: refs/heads/master@{#454692}
Committed: https://chromium.googlesource.com/chromium/src/+/853b5ae0c2f47d981dd8b390d321a886e465c005
Patch Set 1 #
Total comments: 5
Patch Set 2 : Donnot touch instance_holder. #
Total comments: 3
Patch Set 3 : Remove ArcSessionManager dependency. #
Total comments: 2
Patch Set 4 : Rebase #
Total comments: 13
Patch Set 5 : Address hidehiko's comments. #
Total comments: 10
Patch Set 6 : Address hidehiko's comments. Rebase. #
Total comments: 5
Patch Set 7 : Nit fix. #
Total comments: 15
Patch Set 8 : Pure rebase. #Messages
Total messages: 43 (20 generated)
lgcheng@google.com changed reviewers: + lhchavez@chromium.org
Hi Luis, PTAL. Thanks! https://codereview.chromium.org/2711033002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2711033002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:233: profile_->GetPrefs()->SetBoolean(prefs::kArcDataRemoveRequested, false); state() is confused when multiple profiles are created. https://codereview.chromium.org/2711033002/diff/1/chrome/browser/chromeos/pro... File chrome/browser/chromeos/profiles/profile_helper.cc (right): https://codereview.chromium.org/2711033002/diff/1/chrome/browser/chromeos/pro... chrome/browser/chromeos/profiles/profile_helper.cc:469: void ProfileHelper::ClearUserToProfileMappingForTesting() { In Arc sync integration tests, users(not browser_test base user, sync test user added in setupService) are destroyed without notify this field thus lead to shutdown crash. https://codereview.chromium.org/2711033002/diff/1/chrome/browser/sync/test/in... File chrome/browser/sync/test/integration/single_client_arc_package_sync_test.cc (right): https://codereview.chromium.org/2711033002/diff/1/chrome/browser/sync/test/in... chrome/browser/sync/test/integration/single_client_arc_package_sync_test.cc:40: void SetUpOnMainThread() override { Moved from sync_arc_package_helper. It seems the execution order of SetFactoryForSyncTest() and setupService() is not guaranteed by compiler due to the complexity of construction of ArcAppListPrefs. https://codereview.chromium.org/2711033002/diff/1/chrome/browser/sync/test/in... File chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc (right): https://codereview.chromium.org/2711033002/diff/1/chrome/browser/sync/test/in... chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:41: void SetUpOnMainThread() override { Moved from sync_arc_package_helper. It seems the execution order of SetFactoryForSyncTest() and setupService() is not guaranteed by compiler due to the complexity of construction of ArcAppListPrefs.
hidehiko@chromium.org changed reviewers: + hidehiko@chromium.org
drive-by https://codereview.chromium.org/2711033002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2711033002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:233: profile_->GetPrefs()->SetBoolean(prefs::kArcDataRemoveRequested, false); On 2017/02/22 23:36:44, lgcheng wrote: > state() is confused when multiple profiles are created. Instead, could you avoid confusing the state machine in ArcSessionManager, because it means ArcSessionManager won't work properly then? I think there are several possibilities; ArcAppListPrefs is per profile (BrowserContextKeyedService), while ASM is a global singleton (owned by ArcServiceLauncher). So, maybe could you work on fixing ArcAppListPrefs so that it can do something good with a Profile instance which is different from the one ASM knows? Or, alternatively, ASM needs to be created per Profile. Though, IIUC, the current plan is to keep it singleton, for persistent ARC and login-screen work. Thus, this approach sounds a bit harder, I guess. Another approach is, creating a primary Profile and do something, then destroy it completely (incl. shutdown ARC), then creating another Profile and do something. In that way, Profile instance and tied ArcAppListPrefs can be one at once, so it won't hit the ASM restriction? WDYT? https://codereview.chromium.org/2711033002/diff/20001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc (right): https://codereview.chromium.org/2711033002/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:42: ArcAppListPrefsFactory::SetFactoryForSyncTest(); This runs after initial profile creation. Is it expected? If so, better to comment, I think.
On 2017/02/23 06:31:45, hidehiko wrote: > drive-by > > https://codereview.chromium.org/2711033002/diff/1/chrome/browser/chromeos/arc... > File chrome/browser/chromeos/arc/arc_session_manager.cc (right): > > https://codereview.chromium.org/2711033002/diff/1/chrome/browser/chromeos/arc... > chrome/browser/chromeos/arc/arc_session_manager.cc:233: > profile_->GetPrefs()->SetBoolean(prefs::kArcDataRemoveRequested, false); > On 2017/02/22 23:36:44, lgcheng wrote: > > state() is confused when multiple profiles are created. > > Instead, could you avoid confusing the state machine in ArcSessionManager, > because it means ArcSessionManager won't work properly then? > I think there are several possibilities; > > ArcAppListPrefs is per profile (BrowserContextKeyedService), while ASM is a > global singleton (owned by ArcServiceLauncher). > So, maybe could you work on fixing ArcAppListPrefs so that it can do something > good with a Profile instance which is different from the one ASM knows? > > Or, alternatively, ASM needs to be created per Profile. Though, IIUC, the > current plan is to keep it singleton, for persistent ARC and login-screen work. > Thus, this approach sounds a bit harder, I guess. > > Another approach is, creating a primary Profile and do something, then destroy > it completely (incl. shutdown ARC), then creating another Profile and do > something. > In that way, Profile instance and tied ArcAppListPrefs can be one at once, so it > won't hit the ASM restriction? > > WDYT? > > https://codereview.chromium.org/2711033002/diff/20001/chrome/browser/sync/tes... > File chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc > (right): > > https://codereview.chromium.org/2711033002/diff/20001/chrome/browser/sync/tes... > chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:42: > ArcAppListPrefsFactory::SetFactoryForSyncTest(); > This runs after initial profile creation. > Is it expected? If so, better to comment, I think. Yes, I understand current arc_session_manager state machine is complicated and racy enough.. Let me give some more trials to see if I can bring other solution.
https://codereview.chromium.org/2711033002/diff/20001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc (right): https://codereview.chromium.org/2711033002/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:42: ArcAppListPrefsFactory::SetFactoryForSyncTest(); On 2017/02/23 06:31:45, hidehiko wrote: > This runs after initial profile creation. > Is it expected? If so, better to comment, I think. When you mentioned initial profile, did you mean the browser_test base profile? That profile is not used in this sync test and it is just used to set up the browser test environment. Sync test create another 2 profiles and syncs data between them. All these workaround is to make the 2 sync_test related profiles can get through our ARC logic and do not cause crash during shut down of browser_test base profile.
h https://codereview.chromium.org/2711033002/diff/20001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc (right): https://codereview.chromium.org/2711033002/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:42: ArcAppListPrefsFactory::SetFactoryForSyncTest(); On 2017/02/23 18:27:05, lgcheng wrote: > On 2017/02/23 06:31:45, hidehiko wrote: > > This runs after initial profile creation. > > Is it expected? If so, better to comment, I think. > > When you mentioned initial profile, did you mean the browser_test base profile? > That profile is not used in this sync test and it is just used to set up the > browser test environment. Sync test create another 2 profiles and syncs data > between them. All these workaround is to make the 2 sync_test related profiles > can get through our ARC logic and do not cause crash during shut down of > browser_test base profile. Yes. I know it is not interesting for the test cases. However, it is actually created at first, and this global config is set after that. That means, the initial profile is created in the different manner from ones created later for actual testing. So, I thought it's better to comment about it to avoid confusing for future readers.
Hi Luis, PTAL a quick look. In current update, I put test check in ArcAppListPrefs and ArcPackageSyncDataTypeController. WDYT comparing to put the test check in arc::util? Note is PlaystoreEnabled() is not in arc::util so we may have to keep a test check in arc_app_list_prefs.
all right, this looks much better and less invasive! deferring to hidehiko@ (in case he has any residual concerns) and sync OWNERS. https://codereview.chromium.org/2711033002/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2711033002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:145: // Always Arc as enabled in sync test. nit: s/Arc/ARC/ https://codereview.chromium.org/2711033002/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs_factory.cc (right): https://codereview.chromium.org/2711033002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs_factory.cc:59: // Profiles are always treated as allowed to Arc in sync integration test. nit: s/Arc/ARC/.
Patchset #4 (id:60001) has been deleted
Hi, hidehiko@ PTAL. Rebase includes: Kind of revert https://codereview.chromium.org/2708693002/ And swallow update in https://codereview.chromium.org/2702723002/ Let me know if you have any concerns or better idea. Thanks! https://codereview.chromium.org/2711033002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2711033002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:270: For sync test profiles, I think they are safe and will not affect ArcSessionManger now. Although we still make ArcSessionManager observe ArcAppListPrefs created with synctest profiles, but no notifications will be sent to ArcSessionManager from those ArcAppListPrefs now. We can add more checks if desired. WDYT? https://codereview.chromium.org/2711033002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs_factory.cc (right): https://codereview.chromium.org/2711033002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs_factory.cc:54: // Profiles are always treated as allowed to ARC in sync integration test. We can replace this test check with arc:util api if that's better way.
https://codereview.chromium.org/2711033002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_util.h (right): https://codereview.chromium.org/2711033002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.h:38: void SetArcAllowedForSyncTesting(); I commented alternative approach, which this may not be needed. If you need to keep this kind of function, could you merge with DisallowArcForTesting()? E.g. SetArcAllowedForTesting(bool allowed). https://codereview.chromium.org/2711033002/diff/80001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.cc (right): https://codereview.chromium.org/2711033002/diff/80001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:302: arc::SetArcAvailableCommandLineForTesting(cl); Could you define the flag in each test? As ARC does not fully support multi-profile run, and we're adding some workaround for {Single,Two}ClientArcSyncTest only, so IMHO it's better to avoid running ARC for other tests. https://codereview.chromium.org/2711033002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2711033002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:270: On 2017/02/24 01:31:20, lgcheng wrote: > For sync test profiles, I think they are safe and will not affect > ArcSessionManger now. > > Although we still make ArcSessionManager observe ArcAppListPrefs created with > synctest profiles, but no notifications will be sent to ArcSessionManager from > those ArcAppListPrefs now. > > We can add more checks if desired. WDYT? I think we should not subscribe if profile_ is not the one ASM has. Otherwise OnArcPlayStoreEnaledChanged() can be called with unexpected value, when the Google Play Store enabled preference for profile which ASM has is updated. Indeed, the case may not practically happen *now*, just because the current test does not have such a code path. However, if someone in future hits it, it'd be very difficult to understand what's going on, so I'd like to avoid it. So, how about; Take ArcSessionManager pointer as ArcAppListPrefs's ctor. In is_sync_test_ == true case (in the factory), pass nullptr to the pointer. Otherwise (i.e., in real use) pass the real instance ptr. Then, you can get rid of ArcSessionManager::Get() and use the passed ptr. If it is nullptr, do nothing. If it is real ptr, subscribe, and call OnArcPlayStoreEnabledChanged(). Bonus is, in that way, you do not need to modify IsArcAllowedForProfile(), too, because SyncTest only needs ArcAppListPrefs instances. Note: this is just one idea, and there should be other alternatives. My focus is not to tie ASM, if the corresponding profile_ is different from the one ASM has. If it is very difficult to realize, could you add comment the reason, in details? WDYT? https://codereview.chromium.org/2711033002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs_factory.cc (right): https://codereview.chromium.org/2711033002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs_factory.cc:57: new arc::InstanceHolder<arc::mojom::AppInstance>()); As you're here, could you use base::MakeUnique()? https://codereview.chromium.org/2711033002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_package_sync_data_type_controller.cc (right): https://codereview.chromium.org/2711033002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_package_sync_data_type_controller.cc:40: return arc::IsArcPlayStoreEnabledForProfile(profile_) && ShouldSyncArc(); Wow, thank you for the fix!
Patchset #5 (id:100001) has been deleted
PTAL. Thanks! https://codereview.chromium.org/2711033002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_util.h (right): https://codereview.chromium.org/2711033002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.h:38: void SetArcAllowedForSyncTesting(); On 2017/02/24 18:22:26, hidehiko wrote: > I commented alternative approach, which this may not be needed. > If you need to keep this kind of function, could you merge with > DisallowArcForTesting()? E.g. SetArcAllowedForTesting(bool allowed). Done. https://codereview.chromium.org/2711033002/diff/80001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.cc (right): https://codereview.chromium.org/2711033002/diff/80001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:302: arc::SetArcAvailableCommandLineForTesting(cl); On 2017/02/24 18:22:26, hidehiko wrote: > Could you define the flag in each test? > As ARC does not fully support multi-profile run, > and we're adding some workaround for {Single,Two}ClientArcSyncTest only, > so IMHO it's better to avoid running ARC for other tests. Done. https://codereview.chromium.org/2711033002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2711033002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:270: On 2017/02/24 18:22:26, hidehiko wrote: > On 2017/02/24 01:31:20, lgcheng wrote: > > For sync test profiles, I think they are safe and will not affect > > ArcSessionManger now. > > > > Although we still make ArcSessionManager observe ArcAppListPrefs created with > > synctest profiles, but no notifications will be sent to ArcSessionManager from > > those ArcAppListPrefs now. > > > > We can add more checks if desired. WDYT? > > I think we should not subscribe if profile_ is not the one ASM has. > Otherwise OnArcPlayStoreEnaledChanged() can be called with unexpected value, > when the Google Play Store enabled preference for profile which ASM has is > updated. > > Indeed, the case may not practically happen *now*, just because the current test > does not have such a code path. However, if someone in future hits it, it'd be > very difficult to understand what's going on, so I'd like to avoid it. > > So, how about; > > Take ArcSessionManager pointer as ArcAppListPrefs's ctor. > In is_sync_test_ == true case (in the factory), pass nullptr to the pointer. > Otherwise (i.e., in real use) pass the real instance ptr. > > Then, you can get rid of ArcSessionManager::Get() and use the passed ptr. > If it is nullptr, do nothing. If it is real ptr, subscribe, and call > OnArcPlayStoreEnabledChanged(). > > Bonus is, in that way, you do not need to modify IsArcAllowedForProfile(), too, > because SyncTest only needs ArcAppListPrefs instances. > > Note: this is just one idea, and there should be other alternatives. > My focus is not to tie ASM, if the corresponding profile_ is different from the > one ASM has. > If it is very difficult to realize, could you add comment the reason, in > details? > > WDYT? Sync test need ArcPackageSyncDataTypeController which relies on IsArcAllowedForProfile(). So modification of IsArcAllowedForProfile() is still needed. Passing ptr of ASM to ArcAppListPrefs and get rid of ArcSessionManger::Get() seems not safe neither. It seems there is no guarantee of life circle. (See ~ArcAppListPrefs). I will try some alternatives. https://codereview.chromium.org/2711033002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs_factory.cc (right): https://codereview.chromium.org/2711033002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs_factory.cc:57: new arc::InstanceHolder<arc::mojom::AppInstance>()); On 2017/02/24 18:22:27, hidehiko wrote: > As you're here, could you use base::MakeUnique()? Done. https://codereview.chromium.org/2711033002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2711033002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.cc:20: namespace { Comments for type are in util.h.
Hi hidehiko, Would you PTAL when having a moment? Thanks!
https://codereview.chromium.org/2711033002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2711033002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:270: On 2017/02/25 01:40:38, lgcheng wrote: > On 2017/02/24 18:22:26, hidehiko wrote: > > On 2017/02/24 01:31:20, lgcheng wrote: > > > For sync test profiles, I think they are safe and will not affect > > > ArcSessionManger now. > > > > > > Although we still make ArcSessionManager observe ArcAppListPrefs created > with > > > synctest profiles, but no notifications will be sent to ArcSessionManager > from > > > those ArcAppListPrefs now. > > > > > > We can add more checks if desired. WDYT? > > > > I think we should not subscribe if profile_ is not the one ASM has. > > Otherwise OnArcPlayStoreEnaledChanged() can be called with unexpected value, > > when the Google Play Store enabled preference for profile which ASM has is > > updated. > > > > Indeed, the case may not practically happen *now*, just because the current > test > > does not have such a code path. However, if someone in future hits it, it'd be > > very difficult to understand what's going on, so I'd like to avoid it. > > > > So, how about; > > > > Take ArcSessionManager pointer as ArcAppListPrefs's ctor. > > In is_sync_test_ == true case (in the factory), pass nullptr to the pointer. > > Otherwise (i.e., in real use) pass the real instance ptr. > > > > Then, you can get rid of ArcSessionManager::Get() and use the passed ptr. > > If it is nullptr, do nothing. If it is real ptr, subscribe, and call > > OnArcPlayStoreEnabledChanged(). > > > > Bonus is, in that way, you do not need to modify IsArcAllowedForProfile(), > too, > > because SyncTest only needs ArcAppListPrefs instances. > > > > Note: this is just one idea, and there should be other alternatives. > > My focus is not to tie ASM, if the corresponding profile_ is different from > the > > one ASM has. > > If it is very difficult to realize, could you add comment the reason, in > > details? > > > > WDYT? > > Sync test need ArcPackageSyncDataTypeController which relies on > IsArcAllowedForProfile(). So modification of IsArcAllowedForProfile() is still > needed. > Good to know. Then, could you fake ReadyForStart(), instead? (IIUC, that is the problematic point, right?). I still am worried about allowing ARC to non-Primary user profile, because it is global state. If it is possible to fake in smaller point, that sounds safer. WDYT? > Passing ptr of ASM to ArcAppListPrefs and get rid of ArcSessionManger::Get() > seems not safe neither. It seems there is no guarantee of life circle. (See > ~ArcAppListPrefs). > > I will try some alternatives. > Oh, miscommunication. I was waiting for your alternatives, actually... Thank you for ping. https://codereview.chromium.org/2711033002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2711033002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.cc:32: default: No default section please. cf) arc++chromium-style guide. https://codereview.chromium.org/2711033002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util.h (right): https://codereview.chromium.org/2711033002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.h:19: ALLOWEDFORSYNCTEST, style: In chrome, we concat capital letter words with underscores. So, ALLOWED_FOR_SYNC_TEST, DISALLOWED_FOR_TEST etc. https://codereview.chromium.org/2711033002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.h:32: // In sync test, mutilpe ArcAppListPrefs are created and can confuse nit: s/mutilpe/multiple/ https://codereview.chromium.org/2711033002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.h:33: // ArcSessionManager. Let IsArcAllowedForProfile() simply return true to > Let IsArcAllowedForProfile() simply return true to stop confusing ArcSessionManager. This is not really true, IIUC. If IsArcAllowedForProfile() returns true for multiple Profile, it conflicts with the ASM's assumption that ASM works only for primary user Profile. Could you fix the comment? https://codereview.chromium.org/2711033002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.h:43: void SetArcAllowedForTesting(ArcAllowedForTestType type); Could you use the signature: void SetArcAllowedForTesting(bool allowed); Then in .cc file, you can set global value. https://codereview.chromium.org/2711033002/diff/120001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2711033002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:275: if (arc_session_manager->profile() && Hmm... I still do not want to remove the DCHECK other than sync test case, because they can trap unexpected usage. ASM life-time is a good point so my prev pointer code won't work as you pointed (thank you for pointing it). How about, instead, passing a bool flag to control usage of ASM? E.g. in ctor: ArcAppListPrefs::ArcAppListPrefs(..., bool use_arc_session_manager) : ..., use_arc_session_manager_(use_arc_session_manager) { ... } StartPrefs() { if (use_arc_session_manager_) { auto* arc_session_manager = arc::ArcSessionManager::Get(); if (!arc_session_manager) { NOTREACHED(); return; } // L271-L280. } ... remaining code ... } You can pass use_arc_session_manager flag from the Factory class. WDYT?
Description was changed from ========== Fix Arc integration test. BUG=694261 TEST=Run sync_integration_test --gtest_filter=*Arc*. All Arc related tests passed. ========== to ========== Fix Arc integration test. After all the refactor/clean up of ArcSessionManager, clean up Arc sync integration test setup/shut down logic and stop confuse ArcSessionManager state machine with sync test profile. BUG=694261 TEST=Run sync_integration_test --gtest_filter=*Arc*. All Arc related tests passed. ==========
Hi Hidehiko, Address your comments and remove sync test flag in arc::utils while keep sync test flag only in ArcAppListPrefsFactory. Keep the profile DCHECK() in ArcAppListPrefs. PTAL. Thanks! https://codereview.chromium.org/2711033002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2711033002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:270: On 2017/03/02 17:45:38, hidehiko wrote: > On 2017/02/25 01:40:38, lgcheng wrote: > > On 2017/02/24 18:22:26, hidehiko wrote: > > > On 2017/02/24 01:31:20, lgcheng wrote: > > > > For sync test profiles, I think they are safe and will not affect > > > > ArcSessionManger now. > > > > > > > > Although we still make ArcSessionManager observe ArcAppListPrefs created > > with > > > > synctest profiles, but no notifications will be sent to ArcSessionManager > > from > > > > those ArcAppListPrefs now. > > > > > > > > We can add more checks if desired. WDYT? > > > > > > I think we should not subscribe if profile_ is not the one ASM has. > > > Otherwise OnArcPlayStoreEnaledChanged() can be called with unexpected value, > > > when the Google Play Store enabled preference for profile which ASM has is > > > updated. > > > > > > Indeed, the case may not practically happen *now*, just because the current > > test > > > does not have such a code path. However, if someone in future hits it, it'd > be > > > very difficult to understand what's going on, so I'd like to avoid it. > > > > > > So, how about; > > > > > > Take ArcSessionManager pointer as ArcAppListPrefs's ctor. > > > In is_sync_test_ == true case (in the factory), pass nullptr to the pointer. > > > Otherwise (i.e., in real use) pass the real instance ptr. > > > > > > Then, you can get rid of ArcSessionManager::Get() and use the passed ptr. > > > If it is nullptr, do nothing. If it is real ptr, subscribe, and call > > > OnArcPlayStoreEnabledChanged(). > > > > > > Bonus is, in that way, you do not need to modify IsArcAllowedForProfile(), > > too, > > > because SyncTest only needs ArcAppListPrefs instances. > > > > > > Note: this is just one idea, and there should be other alternatives. > > > My focus is not to tie ASM, if the corresponding profile_ is different from > > the > > > one ASM has. > > > If it is very difficult to realize, could you add comment the reason, in > > > details? > > > > > > WDYT? > > > > Sync test need ArcPackageSyncDataTypeController which relies on > > IsArcAllowedForProfile(). So modification of IsArcAllowedForProfile() is still > > needed. > > > > Good to know. Then, could you fake ReadyForStart(), instead? (IIUC, that is the > problematic point, right?). > > I still am worried about allowing ARC to non-Primary user profile, because it is > global state. > If it is possible to fake in smaller point, that sounds safer. > > WDYT? > > > Passing ptr of ASM to ArcAppListPrefs and get rid of ArcSessionManger::Get() > > seems not safe neither. It seems there is no guarantee of life circle. (See > > ~ArcAppListPrefs). > > > > I will try some alternatives. > > > > Oh, miscommunication. I was waiting for your alternatives, actually... Thank you > for ping. I think this is the right way to go. PTAL at the update. https://codereview.chromium.org/2711033002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2711033002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.cc:32: default: On 2017/03/02 17:45:39, hidehiko wrote: > No default section please. > cf) arc++chromium-style guide. Modification for this file is abandoned. https://codereview.chromium.org/2711033002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_util.h (right): https://codereview.chromium.org/2711033002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_util.h:43: void SetArcAllowedForTesting(ArcAllowedForTestType type); On 2017/03/02 17:45:39, hidehiko wrote: > Could you use the signature: > > void SetArcAllowedForTesting(bool allowed); > > Then in .cc file, you can set global value. Generally abandon modification for this file. Keep only Flag in ArcAppListPrefsFactory for sync test. https://codereview.chromium.org/2711033002/diff/120001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2711033002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:275: if (arc_session_manager->profile() && On 2017/03/02 17:45:39, hidehiko wrote: > Hmm... I still do not want to remove the DCHECK other than sync test case, > because they can trap unexpected usage. > > ASM life-time is a good point so my prev pointer code won't work as you pointed > (thank you for pointing it). How about, instead, passing a bool flag to control > usage of ASM? > > E.g. in ctor: > ArcAppListPrefs::ArcAppListPrefs(..., bool use_arc_session_manager) > : ..., use_arc_session_manager_(use_arc_session_manager) { > ... > } > > StartPrefs() { > if (use_arc_session_manager_) { > auto* arc_session_manager = arc::ArcSessionManager::Get(); > if (!arc_session_manager) { > NOTREACHED(); > return; > } > // L271-L280. > } > ... remaining code ... > } > > You can pass use_arc_session_manager flag from the Factory class. > > WDYT? Agree with you concern. Reconstruct a little. Use ArcAppListFactory::IsSetForSyncTest() to handle the logic.1. to avoid create extra flag. 2. Keep the profile DCHECK(). WDYT?
Thank you for clean up! LGTM with minor comments. https://codereview.chromium.org/2711033002/diff/140001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/single_client_arc_package_sync_test.cc (right): https://codereview.chromium.org/2711033002/diff/140001/chrome/browser/sync/te... chrome/browser/sync/test/integration/single_client_arc_package_sync_test.cc:13: namespace base { nit: looks unnecessary? Argument type of overrding function is a part of parent's signature, I think? https://codereview.chromium.org/2711033002/diff/140001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc (right): https://codereview.chromium.org/2711033002/diff/140001/chrome/browser/sync/te... chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:12: namespace base { Ditto. https://codereview.chromium.org/2711033002/diff/140001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2711033002/diff/140001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:270: if (!ArcAppListPrefsFactory::IsFactorySetForSyncTest()) { Nice!
Description was changed from ========== Fix Arc integration test. After all the refactor/clean up of ArcSessionManager, clean up Arc sync integration test setup/shut down logic and stop confuse ArcSessionManager state machine with sync test profile. BUG=694261 TEST=Run sync_integration_test --gtest_filter=*Arc*. All Arc related tests passed. ========== to ========== Fix Arc integration test. After all the refactor/clean up of ArcSessionManager, clean up Arc sync integration test setup/shut down logic and stop confusing ArcSessionManager state machine with sync test profile. Minor cleanup for ArcPackageSyncDataTypeController. BUG=694261 TEST=Run sync_integration_test --gtest_filter=*Arc*. All Arc related tests passed. ==========
lgcheng@google.com changed reviewers: + skym@chromium.org, xiyuan@chromium.org
Hi Xiyuan PTAL c/b/c c/b/ui/app_list Hi Skym@ PTAL c/b/sync/test Thanks! https://codereview.chromium.org/2711033002/diff/140001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/single_client_arc_package_sync_test.cc (right): https://codereview.chromium.org/2711033002/diff/140001/chrome/browser/sync/te... chrome/browser/sync/test/integration/single_client_arc_package_sync_test.cc:13: namespace base { On 2017/03/03 12:55:56, hidehiko wrote: > nit: looks unnecessary? Argument type of overrding function is a part of > parent's signature, I think? Done. https://codereview.chromium.org/2711033002/diff/140001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc (right): https://codereview.chromium.org/2711033002/diff/140001/chrome/browser/sync/te... chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:12: namespace base { On 2017/03/03 12:55:56, hidehiko wrote: > Ditto. Done.
c/b/c c/b/ui/app_list lgtm
Thanks so much for fixing these test cases! Really appreciate it. c/b/sync/test lgtm. https://codereview.chromium.org/2711033002/diff/160001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/sync_arc_package_helper.cc (left): https://codereview.chromium.org/2711033002/diff/160001/chrome/browser/sync/te... chrome/browser/sync/test/integration/sync_arc_package_helper.cc:157: arc_session_manager->StartPreferenceHandler(); So much deleted setup code, awesome! https://codereview.chromium.org/2711033002/diff/160001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/sync_arc_package_helper.cc (right): https://codereview.chromium.org/2711033002/diff/160001/chrome/browser/sync/te... chrome/browser/sync/test/integration/sync_arc_package_helper.cc:130: arc_app_list_prefs->app_instance_holder()->SetInstance(nullptr); Why does this need to be nulled out first? https://codereview.chromium.org/2711033002/diff/160001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc (right): https://codereview.chromium.org/2711033002/diff/160001/chrome/browser/sync/te... chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:43: // This setting does not affect the profile created by InProcessBrowserTest. I don't understand how this comment is true. It seems like this sets a global static, that is then used every time ArcAppListPrefsFactory::BuildServiceInstanceFor() is called. https://codereview.chromium.org/2711033002/diff/160001/chrome/browser/sync/te... chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:101: ASSERT_TRUE(AwaitQuiescence()); I guess this is okay. I'd prefer you created a derived checker class from MultiClientStatusChangeChecker and implement IsExitConditionSatisfied() by calling AllProfilesHaveSameArcPackageDetails(). I'd like to remove AwaitQuiescence(), but it's fairly low priority. https://codereview.chromium.org/2711033002/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2711033002/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:270: if (!ArcAppListPrefsFactory::IsFactorySetForSyncTest()) { How come the other 3 places in this file that call arc::ArcSessionManager::Get() don't need special handling? https://codereview.chromium.org/2711033002/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:271: arc::ArcSessionManager* arc_session_manager = arc::ArcSessionManager::Get(); This seems like it would be more clean if you passed in a weak pointer to a pure virtual interface into ArcAppListPrefs's constructor, WDYT? https://codereview.chromium.org/2711033002/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:272: CHECK(arc_session_manager); Is CHECK really the appropriate macro here? https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
Thanks for review! skym@ Thanks for detail comments. I will discuss your concerns will hidehiko who lead the refactor. https://codereview.chromium.org/2711033002/diff/160001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/sync_arc_package_helper.cc (right): https://codereview.chromium.org/2711033002/diff/160001/chrome/browser/sync/te... chrome/browser/sync/test/integration/sync_arc_package_helper.cc:130: arc_app_list_prefs->app_instance_holder()->SetInstance(nullptr); On 2017/03/03 19:44:58, skym wrote: > Why does this need to be nulled out first? This is due to the logic in instance_holder that does not allow change non-null ptr to another non-null ptr. Also, create ArcAppListPrefs with null instance holder in factory is also not allowed. SetInstance(nullptr) is used to close the binding. The extra set null here will keep the logic instance_holder in prod untouched. https://codereview.chromium.org/2711033002/diff/160001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc (right): https://codereview.chromium.org/2711033002/diff/160001/chrome/browser/sync/te... chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:43: // This setting does not affect the profile created by InProcessBrowserTest. On 2017/03/03 19:44:58, skym wrote: > I don't understand how this comment is true. It seems like this sets a global > static, that is then used every time > ArcAppListPrefsFactory::BuildServiceInstanceFor() is called. The ArcAppListPrefs created with InProcessBrowserTest profile seems completed before this setting from what I read from the log I add, which is good because it means the sync test global setting doesnot affect non sync test profile. https://codereview.chromium.org/2711033002/diff/160001/chrome/browser/sync/te... chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:101: ASSERT_TRUE(AwaitQuiescence()); On 2017/03/03 19:44:58, skym wrote: > I guess this is okay. I'd prefer you created a derived checker class from > MultiClientStatusChangeChecker and implement IsExitConditionSatisfied() by > calling AllProfilesHaveSameArcPackageDetails(). I'd like to remove > AwaitQuiescence(), but it's fairly low priority. Acknowledged. https://codereview.chromium.org/2711033002/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2711033002/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:271: arc::ArcSessionManager* arc_session_manager = arc::ArcSessionManager::Get(); On 2017/03/03 19:44:58, skym wrote: > This seems like it would be more clean if you passed in a weak pointer to a pure > virtual interface into ArcAppListPrefs's constructor, WDYT? This is because the life cycle for ArcAppListPrefs and ArcSessionManger is not strictly the same. I tried to do what you mentioned, but lead to more problems than it solves. Also, it seems there is no plan to make strict condition for life cycle for the two object at this moment. https://codereview.chromium.org/2711033002/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:272: CHECK(arc_session_manager); On 2017/03/03 19:44:58, skym wrote: > Is CHECK really the appropriate macro here? > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Well I think hidehiko has his own reason for put the check here. But it seems that's out of the scope of this cl. This cl just excludes this code snippet for ArcAppListPrefs created with sync test profile.
The CQ bit was checked by lgcheng@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org Link to the patchset: https://codereview.chromium.org/2711033002/#ps160001 (title: "Nit fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by lgcheng@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lgcheng@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org, xiyuan@chromium.org, skym@chromium.org Link to the patchset: https://codereview.chromium.org/2711033002/#ps180001 (title: "Pure rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1488580599867710,
"parent_rev": "ae62c32ff5a0da751cf186a97a9d2f2ff4552d6d", "commit_rev":
"853b5ae0c2f47d981dd8b390d321a886e465c005"}
Message was sent while issue was closed.
Description was changed from ========== Fix Arc integration test. After all the refactor/clean up of ArcSessionManager, clean up Arc sync integration test setup/shut down logic and stop confusing ArcSessionManager state machine with sync test profile. Minor cleanup for ArcPackageSyncDataTypeController. BUG=694261 TEST=Run sync_integration_test --gtest_filter=*Arc*. All Arc related tests passed. ========== to ========== Fix Arc integration test. After all the refactor/clean up of ArcSessionManager, clean up Arc sync integration test setup/shut down logic and stop confusing ArcSessionManager state machine with sync test profile. Minor cleanup for ArcPackageSyncDataTypeController. BUG=694261 TEST=Run sync_integration_test --gtest_filter=*Arc*. All Arc related tests passed. Review-Url: https://codereview.chromium.org/2711033002 Cr-Commit-Position: refs/heads/master@{#454692} Committed: https://chromium.googlesource.com/chromium/src/+/853b5ae0c2f47d981dd8b390d321... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001) as https://chromium.googlesource.com/chromium/src/+/853b5ae0c2f47d981dd8b390d321...
Message was sent while issue was closed.
https://codereview.chromium.org/2711033002/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/2711033002/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:270: if (!ArcAppListPrefsFactory::IsFactorySetForSyncTest()) { On 2017/03/03 19:44:58, skym wrote: > How come the other 3 places in this file that call arc::ArcSessionManager::Get() > don't need special handling? Good catch. It should, IMHO. https://codereview.chromium.org/2711033002/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:271: arc::ArcSessionManager* arc_session_manager = arc::ArcSessionManager::Get(); On 2017/03/03 21:13:01, lgcheng wrote: > On 2017/03/03 19:44:58, skym wrote: > > This seems like it would be more clean if you passed in a weak pointer to a > pure > > virtual interface into ArcAppListPrefs's constructor, WDYT? > > This is because the life cycle for ArcAppListPrefs and ArcSessionManger is not > strictly the same. I tried to do what you mentioned, but lead to more problems > than it solves. > > Also, it seems there is no plan to make strict condition for life cycle for the > two object at this moment. As for life-time, there is a plan. crbug.com/672829. Once it is solved, we can assume ASM is alive while this instance is alive. WeakPtr is an option now, but considering the plan, just if check looks acceptable for now? Or, we can clean up the WeakPtr later. WDYT? https://codereview.chromium.org/2711033002/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:272: CHECK(arc_session_manager); On 2017/03/03 21:13:01, lgcheng wrote: > On 2017/03/03 19:44:58, skym wrote: > > Is CHECK really the appropriate macro here? > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > Well I think hidehiko has his own reason for put the check here. But it seems > that's out of the scope of this cl. This cl just excludes this code snippet for > ArcAppListPrefs created with sync test profile. Just historical reason, IIUC. So, DCHECK is preferred, I think.
Message was sent while issue was closed.
Patchset #9 (id:200001) has been deleted
Message was sent while issue was closed.
Patchset #9 (id:220001) has been deleted |
