[Sync] Request dependency services in sync client with implicit access type.

chrome/browser/sync/chrome_sync_client.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/chrome_sync_client.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 122 matching lines @@
   ~SyncSessionsClientImpl() override {}
 
   // SyncSessionsClient implementation.
   bookmarks::BookmarkModel* GetBookmarkModel() override {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
     return BookmarkModelFactory::GetForProfile(profile_);
   }
   favicon::FaviconService* GetFaviconService() override {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
     return FaviconServiceFactory::GetForProfile(
-        profile_, ServiceAccessType::EXPLICIT_ACCESS);
+        profile_, ServiceAccessType::IMPLICIT_ACCESS);
   }
   history::HistoryService* GetHistoryService() override {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
     return HistoryServiceFactory::GetForProfile(
         profile_, ServiceAccessType::EXPLICIT_ACCESS);

[skym, 2016/01/28 20:20:55]

What about here?

[pavely, 2016/01/28 22:30:53]

Done.

   }
   bool ShouldSyncURL(const GURL& url) const override {
     if (url == GURL(chrome::kChromeUIHistoryURL)) {
       // The history page is treated specially as we want it to trigger syncable
       // events for UI purposes.
       return true;
     }
     return url.is_valid() && !url.SchemeIs(content::kChromeUIScheme) &&
            !url.SchemeIs(chrome::kChromeNativeScheme) && !url.SchemeIsFile();
   }
@@ skipping 22 matching lines @@
       sync_sessions_client_(new SyncSessionsClientImpl(profile)),
       browsing_data_remover_observer_(NULL),
       weak_ptr_factory_(this) {}
 
 ChromeSyncClient::~ChromeSyncClient() {
 }
 
 void ChromeSyncClient::Initialize() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   web_data_service_ = WebDataServiceFactory::GetAutofillWebDataForProfile(

[skym, 2016/01/28 20:20:55]

Kinda odd we cache some (webdata/password), but fetch favicon/history on demand.

[pavely, 2016/01/28 22:30:53]

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.

-      profile_, ServiceAccessType::EXPLICIT_ACCESS);
-  // TODO(crbug.com/558320) Is EXPLICIT_ACCESS appropriate here?
+      profile_, ServiceAccessType::IMPLICIT_ACCESS);
   password_store_ = PasswordStoreFactory::GetForProfile(
-      profile_, ServiceAccessType::EXPLICIT_ACCESS);
+      profile_, ServiceAccessType::IMPLICIT_ACCESS);
 
   // Component factory may already be set in tests.
   if (!GetSyncApiComponentFactory()) {
     const GURL sync_service_url = GetSyncServiceURL(
         *base::CommandLine::ForCurrentProcess(), chrome::GetChannel());
     ProfileOAuth2TokenService* token_service =
         ProfileOAuth2TokenServiceFactory::GetForProfile(profile_);
     net::URLRequestContextGetter* url_request_context_getter =
         profile_->GetRequestContext();
 
     component_factory_.reset(new ProfileSyncComponentsFactoryImpl(

[skym, 2016/01/28 20:20:55]

Wow this constructor is big.

[pavely, 2016/01/28 22:30:53]

Acknowledged.

         this, chrome::GetChannel(), chrome::GetVersionString(),
         ui::GetDeviceFormFactor() == ui::DEVICE_FORM_FACTOR_TABLET,
         *base::CommandLine::ForCurrentProcess(),
         prefs::kSavingBrowserHistoryDisabled, sync_service_url,
         content::BrowserThread::GetMessageLoopProxyForThread(
             content::BrowserThread::UI),
         content::BrowserThread::GetMessageLoopProxyForThread(
             content::BrowserThread::DB),
         token_service, url_request_context_getter, web_data_service_,
         password_store_));
   }
 }
 
 sync_driver::SyncService* ChromeSyncClient::GetSyncService() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   return ProfileSyncServiceFactory::GetSyncServiceForBrowserContext(profile_);
 }
 
 PrefService* ChromeSyncClient::GetPrefService() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   return profile_->GetPrefs();
 }
 
 bookmarks::BookmarkModel* ChromeSyncClient::GetBookmarkModel() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   return BookmarkModelFactory::GetForProfile(profile_);
 }
 
 favicon::FaviconService* ChromeSyncClient::GetFaviconService() {

[skym, 2016/01/28 20:20:55]

Why doesn't this just delegate to the sync_sessions_client_? Am I missing
something?

[pavely, 2016/01/28 22:30:53]

To be honest don't know.

   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   return FaviconServiceFactory::GetForProfile(
-      profile_, ServiceAccessType::EXPLICIT_ACCESS);
+      profile_, ServiceAccessType::IMPLICIT_ACCESS);
 }
 
 history::HistoryService* ChromeSyncClient::GetHistoryService() {

[skym, 2016/01/28 20:20:55]

Same delegate question.

   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   return HistoryServiceFactory::GetForProfile(
       profile_, ServiceAccessType::EXPLICIT_ACCESS);

[skym, 2016/01/28 20:20:55]

and here

[pavely, 2016/01/28 22:30:53]

Done.

 }
 
 autofill::PersonalDataManager* ChromeSyncClient::GetPersonalDataManager() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   return autofill::PersonalDataManagerFactory::GetForProfile(profile_);
 }
 
 sync_driver::ClearBrowsingDataCallback
 ChromeSyncClient::GetClearBrowsingDataCallback() {
   return base::Bind(&ChromeSyncClient::ClearBrowsingData,
@@ skipping 96 matching lines @@
       return ThemeServiceFactory::GetForProfile(profile_)->
           GetThemeSyncableService()->AsWeakPtr();
 #endif
     case syncer::HISTORY_DELETE_DIRECTIVES: {
       history::HistoryService* history = GetHistoryService();
       return history ? history->AsWeakPtr()
                      : base::WeakPtr<history::HistoryService>();
     }
     case syncer::TYPED_URLS: {
       history::HistoryService* history = HistoryServiceFactory::GetForProfile(
           profile_, ServiceAccessType::EXPLICIT_ACCESS);

[skym, 2016/01/28 20:20:55]

and here. Also why is this repeating logic instead of reusing an existing
GetHistoryService() method?

[pavely, 2016/01/28 22:30:53]

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.

[skym, 2016/01/28 22:41:13]

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.

       if (!history)
         return base::WeakPtr<history::TypedUrlSyncableService>();
       return history->GetTypedUrlSyncableService()->AsWeakPtr();
     }
 #if defined(ENABLE_SPELLCHECK)
     case syncer::DICTIONARY:
       return SpellcheckServiceFactory::GetForContext(profile_)->
           GetCustomDictionary()->AsWeakPtr();
 #endif
     case syncer::FAVICON_IMAGES:
@@ skipping 98 matching lines @@
 }
 
 void ChromeSyncClient::ClearBrowsingData(base::Time start, base::Time end) {
   BrowsingDataRemover* remover =
       BrowsingDataRemoverFactory::GetForBrowserContext(profile_);
   remover->Remove(BrowsingDataRemover::TimeRange(start, end),
                   BrowsingDataRemover::REMOVE_ALL, BrowsingDataHelper::ALL);
 
   scoped_refptr<password_manager::PasswordStore> password =
       PasswordStoreFactory::GetForProfile(profile_,
                                           ServiceAccessType::EXPLICIT_ACCESS);

[skym, 2016/01/28 20:20:55]

and here. Also why isn't this using cached password_store_?

[pavely, 2016/01/28 22:30:53]

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_.

   password->RemoveLoginsSyncedBetween(start, end);
 }
 
 void ChromeSyncClient::SetBrowsingDataRemoverObserverForTesting(
     BrowsingDataRemover::Observer* observer) {
   BrowsingDataRemover* remover =
       BrowsingDataRemoverFactory::GetForBrowserContext(profile_);
   if (browsing_data_remover_observer_)
     remover->RemoveObserver(browsing_data_remover_observer_);
 
@@ skipping 125 matching lines @@
   sync_service->RegisterDataTypeController(
       new SupervisedUserSyncDataTypeController(syncer::SUPERVISED_USER_SETTINGS,
                                                error_callback, this, profile_));
   sync_service->RegisterDataTypeController(
       new SupervisedUserSyncDataTypeController(
           syncer::SUPERVISED_USER_WHITELISTS, error_callback, this, profile_));
 #endif
 }
 
 }  // namespace browser_sync