Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1647833006:
[Sync] Request dependency services in sync client with implicit access type. (Closed)
|
Created:
4 years, 10 months ago by pavely Modified:
4 years, 10 months ago Reviewers:
skym CC:
chromium-reviews, tim+watch_chromium.org, maxbogue+watch_chromium.org, plaree+watch_chromium.org, zea+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Sync] Request dependency services in sync client with implicit access type.
Currently all services that sync client depends on are requested with
ServiceAccessType::EXPLICIT_ACCESS. Explicit access should be used for
operations that are caused by user input.
The typical behavior of service factories is when service is requested
with implicit access in incognito mode service factory returns nullptr.
Sync should not be syncing in incognito mode so this behavior should be
acceptable.
R=skym@chromium.org
BUG=558320
Committed: https://crrev.com/ec1099f3a40ac978b0755bcfae7e622720d0e2cb
Cr-Commit-Position: refs/heads/master@{#373041}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Addressed Sky's feedback. #Patch Set 3 : Add comment to remaining use of EXPLICIT_ACCESS #Patch Set 4 : Revert to explicit access for HistoryService to fix integration tests. #Messages
Total messages: 15 (6 generated)
Mostly just questions to help me understand what's going on. https://codereview.chromium.org/1647833006/diff/1/chrome/browser/sync/chrome_... File chrome/browser/sync/chrome_sync_client.cc (right): https://codereview.chromium.org/1647833006/diff/1/chrome/browser/sync/chrome_... chrome/browser/sync/chrome_sync_client.cc:148: profile_, ServiceAccessType::EXPLICIT_ACCESS); What about here? https://codereview.chromium.org/1647833006/diff/1/chrome/browser/sync/chrome_... chrome/browser/sync/chrome_sync_client.cc:191: web_data_service_ = WebDataServiceFactory::GetAutofillWebDataForProfile( Kinda odd we cache some (webdata/password), but fetch favicon/history on demand. https://codereview.chromium.org/1647833006/diff/1/chrome/browser/sync/chrome_... chrome/browser/sync/chrome_sync_client.cc:205: component_factory_.reset(new ProfileSyncComponentsFactoryImpl( Wow this constructor is big. https://codereview.chromium.org/1647833006/diff/1/chrome/browser/sync/chrome_... chrome/browser/sync/chrome_sync_client.cc:234: favicon::FaviconService* ChromeSyncClient::GetFaviconService() { Why doesn't this just delegate to the sync_sessions_client_? Am I missing something? https://codereview.chromium.org/1647833006/diff/1/chrome/browser/sync/chrome_... chrome/browser/sync/chrome_sync_client.cc:240: history::HistoryService* ChromeSyncClient::GetHistoryService() { Same delegate question. https://codereview.chromium.org/1647833006/diff/1/chrome/browser/sync/chrome_... chrome/browser/sync/chrome_sync_client.cc:243: profile_, ServiceAccessType::EXPLICIT_ACCESS); and here https://codereview.chromium.org/1647833006/diff/1/chrome/browser/sync/chrome_... chrome/browser/sync/chrome_sync_client.cc:360: profile_, ServiceAccessType::EXPLICIT_ACCESS); and here. Also why is this repeating logic instead of reusing an existing GetHistoryService() method? https://codereview.chromium.org/1647833006/diff/1/chrome/browser/sync/chrome_... chrome/browser/sync/chrome_sync_client.cc:479: ServiceAccessType::EXPLICIT_ACCESS); and here. Also why isn't this using cached password_store_?
https://codereview.chromium.org/1647833006/diff/1/chrome/browser/sync/chrome_... File chrome/browser/sync/chrome_sync_client.cc (right): https://codereview.chromium.org/1647833006/diff/1/chrome/browser/sync/chrome_... chrome/browser/sync/chrome_sync_client.cc:148: profile_, ServiceAccessType::EXPLICIT_ACCESS); On 2016/01/28 20:20:55, skym wrote: > What about here? Done. https://codereview.chromium.org/1647833006/diff/1/chrome/browser/sync/chrome_... chrome/browser/sync/chrome_sync_client.cc:191: web_data_service_ = WebDataServiceFactory::GetAutofillWebDataForProfile( On 2016/01/28 20:20:55, skym wrote: > Kinda odd we cache some (webdata/password), but fetch favicon/history on demand. Don't know why. Maybe factory needs to be called on UI thread (since it accesses profile) while service pointer is exposed on backend thread through GetSyncableServiceForType. Another reason could be that instantiating some services incurs costly IO which we are trying to avoid at startup and ChromeSyncClient gets initialized very early on. https://codereview.chromium.org/1647833006/diff/1/chrome/browser/sync/chrome_... chrome/browser/sync/chrome_sync_client.cc:205: component_factory_.reset(new ProfileSyncComponentsFactoryImpl( On 2016/01/28 20:20:55, skym wrote: > Wow this constructor is big. Acknowledged. https://codereview.chromium.org/1647833006/diff/1/chrome/browser/sync/chrome_... chrome/browser/sync/chrome_sync_client.cc:234: favicon::FaviconService* ChromeSyncClient::GetFaviconService() { On 2016/01/28 20:20:55, skym wrote: > Why doesn't this just delegate to the sync_sessions_client_? Am I missing > something? To be honest don't know. https://codereview.chromium.org/1647833006/diff/1/chrome/browser/sync/chrome_... chrome/browser/sync/chrome_sync_client.cc:243: profile_, ServiceAccessType::EXPLICIT_ACCESS); On 2016/01/28 20:20:55, skym wrote: > and here Done. https://codereview.chromium.org/1647833006/diff/1/chrome/browser/sync/chrome_... chrome/browser/sync/chrome_sync_client.cc:360: profile_, ServiceAccessType::EXPLICIT_ACCESS); On 2016/01/28 20:20:55, skym wrote: > and here. Also why is this repeating logic instead of reusing an existing > GetHistoryService() method? HistoryServiceFactory::GetForProfile works slightly differently. For implicit access it checks value of preference. Pref service in turn checks that it is called on the right thread (I assume UI thread). In our case this check fails because ChromeSyncClient::GetSyncableServiceForType is called on backend thread. I could fix this by saving reference to history service in Initialize, but I don't know perf implications so I'm not going to risk it. https://codereview.chromium.org/1647833006/diff/1/chrome/browser/sync/chrome_... chrome/browser/sync/chrome_sync_client.cc:479: ServiceAccessType::EXPLICIT_ACCESS); On 2016/01/28 20:20:55, skym wrote: > and here. Also why isn't this using cached password_store_? Here I thought that ClearBrowsingData must be called in response to user action therefore left EXPLICIT_ACCESS. It turns out this function is called as part of sync restore. Changed it to use password_store_.
LGTM https://codereview.chromium.org/1647833006/diff/1/chrome/browser/sync/chrome_... File chrome/browser/sync/chrome_sync_client.cc (right): https://codereview.chromium.org/1647833006/diff/1/chrome/browser/sync/chrome_... chrome/browser/sync/chrome_sync_client.cc:360: profile_, ServiceAccessType::EXPLICIT_ACCESS); On 2016/01/28 22:30:53, pavely wrote: > On 2016/01/28 20:20:55, skym wrote: > > and here. Also why is this repeating logic instead of reusing an existing > > GetHistoryService() method? > > HistoryServiceFactory::GetForProfile works slightly differently. For implicit > access it checks value of preference. Pref service in turn checks that it is > called on the right thread (I assume UI thread). In our case this check fails > because ChromeSyncClient::GetSyncableServiceForType is called on backend thread. > > I could fix this by saving reference to history service in Initialize, but I > don't know perf implications so I'm not going to risk it. Makes sense. Were you going to change this to be IMPLICIT_ACCESS as well? It didn't seem to get updated in Patch Set #2.
The CQ bit was checked by pavely@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org Link to the patchset: https://codereview.chromium.org/1647833006/#ps40001 (title: "Add comment to remaining use of EXPLICIT_ACCESS")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1647833006/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1647833006/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by pavely@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org Link to the patchset: https://codereview.chromium.org/1647833006/#ps60001 (title: "Revert to explicit access for HistoryService to fix integration tests.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1647833006/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1647833006/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Sync] Request dependency services in sync client with implicit access type. Currently all services that sync client depends on are requested with ServiceAccessType::EXPLICIT_ACCESS. Explicit access should be used for operations that are caused by user input. The typical behavior of service factories is when service is requested with implicit access in incognito mode service factory returns nullptr. Sync should not be syncing in incognito mode so this behavior should be acceptable. R=skym@chromium.org BUG=558320 ========== to ========== [Sync] Request dependency services in sync client with implicit access type. Currently all services that sync client depends on are requested with ServiceAccessType::EXPLICIT_ACCESS. Explicit access should be used for operations that are caused by user input. The typical behavior of service factories is when service is requested with implicit access in incognito mode service factory returns nullptr. Sync should not be syncing in incognito mode so this behavior should be acceptable. R=skym@chromium.org BUG=558320 Committed: https://crrev.com/ec1099f3a40ac978b0755bcfae7e622720d0e2cb Cr-Commit-Position: refs/heads/master@{#373041} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ec1099f3a40ac978b0755bcfae7e622720d0e2cb Cr-Commit-Position: refs/heads/master@{#373041} |
