|
|
Created:
3 years, 9 months ago by skym Modified:
3 years, 8 months ago CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, marq+watch_chromium.org, sync-reviews_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Sync] Adding missing DependsOn to *ProfileSyncServiceFactory.
It looks like most of the recently added model types have not been
adding their associated KeyedService dependencies to
*ProfileSyncServiceFactory. This change updates the dependencies to
include everything used by both *ProfileSyncServiceFactory and
*ChromeSyncClient, along with some alphabetization for the ios
DependOn()s.
Couldn't add a dependency to SupervisedUserServiceFactory because it
apparently already depends on ProfileSyncServiceFactory, so added TODOs
to fix this.
BUG=701326
Review-Url: https://codereview.chromium.org/2768923005
Cr-Commit-Position: refs/heads/master@{#460133}
Committed: https://chromium.googlesource.com/chromium/src/+/31da0bbe6ad57750e8821aec9380a0dcd3ce0f54
Patch Set 1 #Patch Set 2 : Added gcm, removed circular dependency DependsOn, updated doc. #Patch Set 3 : This time with less incorrect namespace. #Patch Set 4 : Wrapped some supervised user deps in not android flags, added flag closing comments. #
Total comments: 1
Patch Set 5 : Added crbug. #
Messages
Total messages: 38 (22 generated)
The CQ bit was checked by skym@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Sync] Adding missing DependsOn to *ProfileSyncServiceFactory. BUG= ========== to ========== [Sync] Adding missing DependsOn to *ProfileSyncServiceFactory. It looks like most of the recently added model types have not been adding their associated KeyedService dependencies to *ProfileSyncServiceFactory. This change updates the dependencies to include everything used by both *ProfileSyncServiceFactory and *ChromeSyncClient, along with some alphabetization for the ios DependOn()s. Couldn't add a dependency to SupervisedUserServiceFactory because it apparently already depends on ProfileSyncServiceFactory, so added TODOs to fix this. BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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 skym@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by skym@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
skym@chromium.org changed reviewers: + pavely@chromium.org
PTAL
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
skym@chromium.org changed reviewers: + treib@chromium.org
Adding treib@ for supervised_user/ ... Sorry for the hassle, it's just a comment.
On 2017/03/25 03:11:46, skym wrote: > Adding treib@ for supervised_user/ ... Sorry for the hassle, it's just a > comment. Comment lgtm, but I'd like to understand what the problem is. Why can't (shouldn't?) SUService depend on ProfileSyncService? Also, is there a bug that could be referenced?
brian.w.petroski@gmail.com changed reviewers: + brian.w.petroski@gmail.com
lgtm Fix Google sync
lgtm Fix Google sync
Description was changed from ========== [Sync] Adding missing DependsOn to *ProfileSyncServiceFactory. It looks like most of the recently added model types have not been adding their associated KeyedService dependencies to *ProfileSyncServiceFactory. This change updates the dependencies to include everything used by both *ProfileSyncServiceFactory and *ChromeSyncClient, along with some alphabetization for the ios DependOn()s. Couldn't add a dependency to SupervisedUserServiceFactory because it apparently already depends on ProfileSyncServiceFactory, so added TODOs to fix this. BUG= ========== to ========== [Sync] Adding missing DependsOn to *ProfileSyncServiceFactory. It looks like most of the recently added model types have not been adding their associated KeyedService dependencies to *ProfileSyncServiceFactory. This change updates the dependencies to include everything used by both *ProfileSyncServiceFactory and *ChromeSyncClient, along with some alphabetization for the ios DependOn()s. Couldn't add a dependency to SupervisedUserServiceFactory because it apparently already depends on ProfileSyncServiceFactory, so added TODOs to fix this. BUG=701326 ==========
sdefresne@chromium.org changed reviewers: + sdefresne@chromium.org
https://codereview.chromium.org/2768923005/diff/60001/ios/chrome/browser/sync... File ios/chrome/browser/sync/ios_chrome_profile_sync_service_factory.cc (right): https://codereview.chromium.org/2768923005/diff/60001/ios/chrome/browser/sync... ios/chrome/browser/sync/ios_chrome_profile_sync_service_factory.cc:98: // The ProfileSyncService depends on various SyncableServices being around I think this is incorrect as if A depends on B, then B will be created before A and destroyed after A, so if ProfileSyncService requires service Foo to be around when it is shutdown, then Foo needs to depends on ProfileSyncService, not the other way around. In fact, it looks to me like ProfileSyncService is abusing the dependency mechanism of KeyedService factories here as I don't see any direct or indirect dependencies between those services when I look at the code (no service is injected, ...).
On 2017/03/27 08:13:40, Marc Treib wrote: > On 2017/03/25 03:11:46, skym wrote: > > Adding treib@ for supervised_user/ ... Sorry for the hassle, it's just a > > comment. > > Comment lgtm, but I'd like to understand what the problem is. Why can't > (shouldn't?) SUService depend on ProfileSyncService? > > Also, is there a bug that could be referenced? Created crbug.com/705545. I added what context I understand to the bug. The only problem I see is that ChromeSyncClient (and so transitively ProfileSyncService) accesses SupervisedUserServiceFactory when it tries to fetch the SupervisedUserWhitelistService. While there are many things I don't understand, one question about SUService's initialization Marc, is there anything that's guaranteeing that the SupervisedUserService's Init() method and subsequently PSS's AddPreferenceProvider() method is called in a timely fashion? I'm worried that Sync might end up performing the part of its initialization logic that checks preferred types before AddPreferenceProvider() is even called.
On 2017/03/27 15:34:06, sdefresne wrote: > https://codereview.chromium.org/2768923005/diff/60001/ios/chrome/browser/sync... > File ios/chrome/browser/sync/ios_chrome_profile_sync_service_factory.cc (right): > > https://codereview.chromium.org/2768923005/diff/60001/ios/chrome/browser/sync... > ios/chrome/browser/sync/ios_chrome_profile_sync_service_factory.cc:98: // The > ProfileSyncService depends on various SyncableServices being around > I think this is incorrect as if A depends on B, then B will be created before A > and destroyed after A, so if ProfileSyncService requires service Foo to be > around when it is shutdown, then Foo needs to depends on ProfileSyncService, not > the other way around. > > In fact, it looks to me like ProfileSyncService is abusing the dependency > mechanism of KeyedService factories here as I don't see any direct or indirect > dependencies between those services when I look at the code (no service is > injected, ...). I'm having a hard time following your logic. If Foo depending on PSS, then PSS would be created before Foo, and destroyed before PSS's shutdown. Which is what this comment is trying to avoid? I'd love to hear more about what the proper use of the dependency mechanism for KeyedServices is. I'm mostly trying to perpetuate the existing structure here and make sure it's consistent. But if it's wrong to begin with, I'd really like to understand why. Are you saying that dependencies should only be declared when a Factory uses another, and not when a Service uses another? I don't see why you need any service injection or what exactly you mean by that here. Sorry for all the questions, I want to understand!
On 2017/03/27 15:45:55, skym wrote: > On 2017/03/27 08:13:40, Marc Treib wrote: > > On 2017/03/25 03:11:46, skym wrote: > > > Adding treib@ for supervised_user/ ... Sorry for the hassle, it's just a > > > comment. > > > > Comment lgtm, but I'd like to understand what the problem is. Why can't > > (shouldn't?) SUService depend on ProfileSyncService? > > > > Also, is there a bug that could be referenced? > > Created crbug.com/705545. I added what context I understand to the bug. The only > problem I see is that ChromeSyncClient (and so transitively ProfileSyncService) > accesses SupervisedUserServiceFactory when it tries to fetch the > SupervisedUserWhitelistService. > > While there are many things I don't understand, one question about SUService's > initialization Marc, is there anything that's guaranteeing that the > SupervisedUserService's Init() method and subsequently PSS's > AddPreferenceProvider() method is called in a timely fashion? I'm worried that > Sync might end up performing the part of its initialization logic that checks > preferred types before AddPreferenceProvider() is even called. SUService::Init is called from ProfileManager::DoFinalInitForServices, here: https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_manager.... Presumably that's early enough to guarantee that the PreferenceProvider makes it in time?
On 2017/03/27 16:53:18, Marc Treib wrote: > On 2017/03/27 15:45:55, skym wrote: > > On 2017/03/27 08:13:40, Marc Treib wrote: > > > On 2017/03/25 03:11:46, skym wrote: > > > > Adding treib@ for supervised_user/ ... Sorry for the hassle, it's just a > > > > comment. > > > > > > Comment lgtm, but I'd like to understand what the problem is. Why can't > > > (shouldn't?) SUService depend on ProfileSyncService? > > > > > > Also, is there a bug that could be referenced? > > > > Created crbug.com/705545. I added what context I understand to the bug. The > only > > problem I see is that ChromeSyncClient (and so transitively > ProfileSyncService) > > accesses SupervisedUserServiceFactory when it tries to fetch the > > SupervisedUserWhitelistService. > > > > While there are many things I don't understand, one question about SUService's > > initialization Marc, is there anything that's guaranteeing that the > > SupervisedUserService's Init() method and subsequently PSS's > > AddPreferenceProvider() method is called in a timely fashion? I'm worried that > > Sync might end up performing the part of its initialization logic that checks > > preferred types before AddPreferenceProvider() is even called. > > SUService::Init is called from ProfileManager::DoFinalInitForServices, here: > https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_manager.... > > Presumably that's early enough to guarantee that the PreferenceProvider makes it > in time? Oooh, I completely missed that. Thanks! It looks like the ProfileManager actually uses the ProfileSyncService[Factory] in that exact same method (ProfileManager::DoFinalInitForServices). Maybe we could pass a single method interface/Callback into SUService::Init to make the AddPreferenceProvider call and remove the dependency on PSS[F]? WDYT?
On 2017/03/27 17:01:45, skym wrote: > On 2017/03/27 16:53:18, Marc Treib wrote: > > On 2017/03/27 15:45:55, skym wrote: > > > On 2017/03/27 08:13:40, Marc Treib wrote: > > > > On 2017/03/25 03:11:46, skym wrote: > > > > > Adding treib@ for supervised_user/ ... Sorry for the hassle, it's just a > > > > > comment. > > > > > > > > Comment lgtm, but I'd like to understand what the problem is. Why can't > > > > (shouldn't?) SUService depend on ProfileSyncService? > > > > > > > > Also, is there a bug that could be referenced? > > > > > > Created crbug.com/705545. I added what context I understand to the bug. The > > only > > > problem I see is that ChromeSyncClient (and so transitively > > ProfileSyncService) > > > accesses SupervisedUserServiceFactory when it tries to fetch the > > > SupervisedUserWhitelistService. > > > > > > While there are many things I don't understand, one question about > SUService's > > > initialization Marc, is there anything that's guaranteeing that the > > > SupervisedUserService's Init() method and subsequently PSS's > > > AddPreferenceProvider() method is called in a timely fashion? I'm worried > that > > > Sync might end up performing the part of its initialization logic that > checks > > > preferred types before AddPreferenceProvider() is even called. > > > > SUService::Init is called from ProfileManager::DoFinalInitForServices, here: > > > https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_manager.... > > > > Presumably that's early enough to guarantee that the PreferenceProvider makes > it > > in time? > > Oooh, I completely missed that. Thanks! It looks like the ProfileManager > actually uses the ProfileSyncService[Factory] in that exact same method > (ProfileManager::DoFinalInitForServices). Maybe we could pass a single method > interface/Callback into SUService::Init to make the AddPreferenceProvider call > and remove the dependency on PSS[F]? WDYT? Great idea, I didn't think of that! Yes, that should work as far as I can tell.
On 2017/03/27 15:56:09, skym wrote: > On 2017/03/27 15:34:06, sdefresne wrote: > > > https://codereview.chromium.org/2768923005/diff/60001/ios/chrome/browser/sync... > > File ios/chrome/browser/sync/ios_chrome_profile_sync_service_factory.cc > (right): > > > > > https://codereview.chromium.org/2768923005/diff/60001/ios/chrome/browser/sync... > > ios/chrome/browser/sync/ios_chrome_profile_sync_service_factory.cc:98: // The > > ProfileSyncService depends on various SyncableServices being around > > I think this is incorrect as if A depends on B, then B will be created before > A > > and destroyed after A, so if ProfileSyncService requires service Foo to be > > around when it is shutdown, then Foo needs to depends on ProfileSyncService, > not > > the other way around. > > > > In fact, it looks to me like ProfileSyncService is abusing the dependency > > mechanism of KeyedService factories here as I don't see any direct or indirect > > dependencies between those services when I look at the code (no service is > > injected, ...). > > I'm having a hard time following your logic. If Foo depending on PSS, then PSS > would be created before Foo, and destroyed before PSS's shutdown. Which is what > this comment is trying to avoid? Sorry, you're right, the dependency is correct (I read it backward). I still do not like this comment as it says that the dependencies are there to have proper destruction order while the dependency also force the creation of the services before the ProfileSyncService (i.e. it use something that has stronger guarantee than what it says it needs). > I'd love to hear more about what the proper use of the dependency mechanism for > KeyedServices is. I'm mostly trying to perpetuate the existing structure here > and make sure it's consistent. But if it's wrong to begin with, I'd really like > to understand why. Are you saying that dependencies should only be declared when > a Factory uses another, and not when a Service uses another? I don't see why you > need any service injection or what exactly you mean by that here. Sorry for all > the questions, I want to understand! So, my issue was that I didn't see how ProfileSyncService used those dependencies. Looking more at the code, it look like they are accessed through the syncer::SyncClient (that has concrete implementation in src/chrome and src/ios/chrome). The client itself does access the depended services via the factories. Usually for the other clients we prefer to inject the dependencies in the constructor because: 1. it make the code easier to test (as you can inject fakes, or at least you can create objects without using factories), 2. it make it clear that the dependency is required in the factory constructor. I would have expected to see code like this: IOSChromeProfileSyncServiceFactory::IOSChromeProfileSyncServiceFactory() : BrowserStateKeyedServiceFactory( "ProfileSyncService", BrowserStateDependencyManager::GetInstance()) { DependsOn(autofill::PersonalDataManagerFactory::GetInstance()); DependsOn(ios::AboutSigninInternalsFactory::GetInstance()); DependsOn(ios::BookmarkModelFactory::GetInstance()); DependsOn(SigninClientFactory::GetInstance()); DependsOn(ios::HistoryServiceFactory::GetInstance()); DependsOn(IOSChromeProfileInvalidationProviderFactory::GetInstance()); DependsOn(OAuth2TokenServiceFactory::GetInstance()); DependsOn(IOSChromePasswordStoreFactory::GetInstance()); DependsOn(ios::SigninManagerFactory::GetInstance()); DependsOn(ios::TemplateURLServiceFactory::GetInstance()); DependsOn(ios::WebDataServiceFactory::GetInstance()); DependsOn(ios::FaviconServiceFactory::GetInstance()); } std::unique_ptr<KeyedService> IOSChromeProfileSyncServiceFactory::BuildServiceInstanceFor( web::BrowserState* context) const { ios::ChromeBrowserState* browser_state = ios::ChromeBrowserState::FromBrowserState(context); ProfileSyncService::InitParams init_params; init_params.signin_wrapper = base::MakeUnique<SigninManagerWrapper>(signin); init_params.oauth2_token_service = OAuth2TokenServiceFactory::GetForBrowserState(browser_state); init_params.start_behavior = ProfileSyncService::MANUAL_START; init_params.sync_client = base::MakeUnique<IOSChromeSyncClient>( autofill::PersonalDataManagerFactory::GetForBrowserState(browser_state), ios::AboutSigninInternalsFactory::GetForBrowserState(browser_state), ... ); ... Here, it is clear that the services are used (since they are passed to the constructor of the client). As someone writing the code, when adding a new service, I easily see that I need to add it to both the constructor of the client, and as a dependency of the factory (because I'm going to look at how it is done for other service). As someone reviewing the code, it is easier to see that the dependency need to be added to the factory if it is added to the client. However, all of this is beyond the scope of the current CL. LGTM as is.
On 2017/03/28 12:01:08, sdefresne wrote: > On 2017/03/27 15:56:09, skym wrote: > > On 2017/03/27 15:34:06, sdefresne wrote: > > > > > > https://codereview.chromium.org/2768923005/diff/60001/ios/chrome/browser/sync... > > > File ios/chrome/browser/sync/ios_chrome_profile_sync_service_factory.cc > > (right): > > > > > > > > > https://codereview.chromium.org/2768923005/diff/60001/ios/chrome/browser/sync... > > > ios/chrome/browser/sync/ios_chrome_profile_sync_service_factory.cc:98: // > The > > > ProfileSyncService depends on various SyncableServices being around > > > I think this is incorrect as if A depends on B, then B will be created > before > > A > > > and destroyed after A, so if ProfileSyncService requires service Foo to be > > > around when it is shutdown, then Foo needs to depends on ProfileSyncService, > > not > > > the other way around. > > > > > > In fact, it looks to me like ProfileSyncService is abusing the dependency > > > mechanism of KeyedService factories here as I don't see any direct or > indirect > > > dependencies between those services when I look at the code (no service is > > > injected, ...). > > > > I'm having a hard time following your logic. If Foo depending on PSS, then PSS > > would be created before Foo, and destroyed before PSS's shutdown. Which is > what > > this comment is trying to avoid? > > Sorry, you're right, the dependency is correct (I read it backward). > > I still do not like this comment as it says that the dependencies are there to > have proper destruction order while the dependency also force the creation of > the services before the ProfileSyncService (i.e. it use something that has > stronger guarantee than what it says it needs). > > > I'd love to hear more about what the proper use of the dependency mechanism > for > > KeyedServices is. I'm mostly trying to perpetuate the existing structure here > > and make sure it's consistent. But if it's wrong to begin with, I'd really > like > > to understand why. Are you saying that dependencies should only be declared > when > > a Factory uses another, and not when a Service uses another? I don't see why > you > > need any service injection or what exactly you mean by that here. Sorry for > all > > the questions, I want to understand! > > > So, my issue was that I didn't see how ProfileSyncService used those > dependencies. Looking more at the code, it look like they are accessed through > the syncer::SyncClient (that has concrete implementation in src/chrome and > src/ios/chrome). The client itself does access the depended services via the > factories. Usually for the other clients we prefer to inject the dependencies in > the constructor because: > > 1. it make the code easier to test (as you can inject fakes, or at least you can > create objects without using factories), > 2. it make it clear that the dependency is required in the factory constructor. > > I would have expected to see code like this: > > IOSChromeProfileSyncServiceFactory::IOSChromeProfileSyncServiceFactory() > : BrowserStateKeyedServiceFactory( > "ProfileSyncService", > BrowserStateDependencyManager::GetInstance()) { > DependsOn(autofill::PersonalDataManagerFactory::GetInstance()); > DependsOn(ios::AboutSigninInternalsFactory::GetInstance()); > DependsOn(ios::BookmarkModelFactory::GetInstance()); > DependsOn(SigninClientFactory::GetInstance()); > DependsOn(ios::HistoryServiceFactory::GetInstance()); > DependsOn(IOSChromeProfileInvalidationProviderFactory::GetInstance()); > DependsOn(OAuth2TokenServiceFactory::GetInstance()); > DependsOn(IOSChromePasswordStoreFactory::GetInstance()); > DependsOn(ios::SigninManagerFactory::GetInstance()); > DependsOn(ios::TemplateURLServiceFactory::GetInstance()); > DependsOn(ios::WebDataServiceFactory::GetInstance()); > DependsOn(ios::FaviconServiceFactory::GetInstance()); > } > > std::unique_ptr<KeyedService> > IOSChromeProfileSyncServiceFactory::BuildServiceInstanceFor( > web::BrowserState* context) const { > ios::ChromeBrowserState* browser_state = > ios::ChromeBrowserState::FromBrowserState(context); > > ProfileSyncService::InitParams init_params; > init_params.signin_wrapper = base::MakeUnique<SigninManagerWrapper>(signin); > init_params.oauth2_token_service = > OAuth2TokenServiceFactory::GetForBrowserState(browser_state); > init_params.start_behavior = ProfileSyncService::MANUAL_START; > init_params.sync_client = > base::MakeUnique<IOSChromeSyncClient>( > > autofill::PersonalDataManagerFactory::GetForBrowserState(browser_state), > ios::AboutSigninInternalsFactory::GetForBrowserState(browser_state), > ... > ); > ... > > Here, it is clear that the services are used (since they are passed to the > constructor of the client). As someone writing the code, when adding a new > service, I easily see that I need to add it to both the constructor of the > client, and as a dependency of the factory (because I'm going to look at how it > is done for other service). As someone reviewing the code, it is easier to see > that the dependency need to be added to the factory if it is added to the > client. > > However, all of this is beyond the scope of the current CL. > > LGTM as is. Thanks for the detailed response! Created crbug.com/706000 to track fixing this, will try to get to it in the next couple weeks.
The CQ bit was checked by skym@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, brian.w.petroski@gmail.com, pavely@chromium.org Link to the patchset: https://codereview.chromium.org/2768923005/#ps80001 (title: "Added crbug.")
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": 80001, "attempt_start_ts": 1490714847345310, "parent_rev": "cb80fc5be2e9a107a6c1f47d3a6ccc0e4391a91a", "commit_rev": "31da0bbe6ad57750e8821aec9380a0dcd3ce0f54"}
Message was sent while issue was closed.
Description was changed from ========== [Sync] Adding missing DependsOn to *ProfileSyncServiceFactory. It looks like most of the recently added model types have not been adding their associated KeyedService dependencies to *ProfileSyncServiceFactory. This change updates the dependencies to include everything used by both *ProfileSyncServiceFactory and *ChromeSyncClient, along with some alphabetization for the ios DependOn()s. Couldn't add a dependency to SupervisedUserServiceFactory because it apparently already depends on ProfileSyncServiceFactory, so added TODOs to fix this. BUG=701326 ========== to ========== [Sync] Adding missing DependsOn to *ProfileSyncServiceFactory. It looks like most of the recently added model types have not been adding their associated KeyedService dependencies to *ProfileSyncServiceFactory. This change updates the dependencies to include everything used by both *ProfileSyncServiceFactory and *ChromeSyncClient, along with some alphabetization for the ios DependOn()s. Couldn't add a dependency to SupervisedUserServiceFactory because it apparently already depends on ProfileSyncServiceFactory, so added TODOs to fix this. BUG=701326 Review-Url: https://codereview.chromium.org/2768923005 Cr-Commit-Position: refs/heads/master@{#460133} Committed: https://chromium.googlesource.com/chromium/src/+/31da0bbe6ad57750e8821aec9380... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/31da0bbe6ad57750e8821aec9380... |