Skip to content

Commit

Permalink
Prepare Bookmark{ChangeProcessor/ModelAssociator} for componentization
Browse files Browse the repository at this point in the history 
This CL prepares BookmarkChangeProcessor and BookmarkModelAssociator for
componentization by removing //chrome and //content dependencies. Specifically:

- Pass these classes a SyncClient instance rather than a Profile.
- Expand the SyncClient interface to include getters for the services they need.
- Replace usage of content::BrowserThread with base::ThreadChecker within the
  classes.

BUG=511299,511287

Review URL: https://codereview.chromium.org/1389123003

Cr-Commit-Position: refs/heads/master@{#353036}
  • Loading branch information
@colinblundell
colinblundell authored and Commit bot committed Oct 8, 2015
1 parent bb66edc commit de2169a
Show file tree
Hide file tree
Showing 17 changed files with 141 additions and 85 deletions.
12 changes: 12 additions & 0 deletions chrome/browser/sync/chrome_sync_client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "chrome/browser/autofill/personal_data_manager_factory.h"
#include "chrome/browser/bookmarks/bookmark_model_factory.h"
#include "chrome/browser/dom_distiller/dom_distiller_service_factory.h"
#include "chrome/browser/favicon/favicon_service_factory.h"
#include "chrome/browser/history/history_service_factory.h"
#include "chrome/browser/password_manager/password_store_factory.h"
#include "chrome/browser/prefs/pref_service_syncable_util.h"
Expand All @@ -17,6 +18,7 @@
#include "chrome/browser/themes/theme_service.h"
#include "chrome/browser/themes/theme_service_factory.h"
#include "chrome/browser/themes/theme_syncable_service.h"
#include "chrome/browser/undo/bookmark_undo_service_factory.h"
#include "chrome/browser/web_data_service_factory.h"
#include "components/autofill/core/browser/webdata/autocomplete_syncable_service.h"
#include "components/autofill/core/browser/webdata/autofill_profile_syncable_service.h"
Expand Down Expand Up @@ -100,6 +102,12 @@ bookmarks::BookmarkModel* ChromeSyncClient::GetBookmarkModel() {
return BookmarkModelFactory::GetForProfile(profile_);
}

favicon::FaviconService* ChromeSyncClient::GetFaviconService() {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
return FaviconServiceFactory::GetForProfile(
profile_, ServiceAccessType::EXPLICIT_ACCESS);
}

history::HistoryService* ChromeSyncClient::GetHistoryService() {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
return HistoryServiceFactory::GetForProfile(
Expand All @@ -125,6 +133,10 @@ ChromeSyncClient::GetWebDataService() {
profile_, ServiceAccessType::EXPLICIT_ACCESS);
}

BookmarkUndoService* ChromeSyncClient::GetBookmarkUndoServiceIfExists() {
return BookmarkUndoServiceFactory::GetForProfileIfExists(profile_);
}

base::WeakPtr<syncer::SyncableService>
ChromeSyncClient::GetSyncableServiceForType(syncer::ModelType type) {
if (!profile_) { // For tests.
Expand Down
2 changes: 2 additions & 0 deletions chrome/browser/sync/chrome_sync_client.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,12 @@ class ChromeSyncClient : public sync_driver::SyncClient {
sync_driver::SyncService* GetSyncService() override;
PrefService* GetPrefService() override;
bookmarks::BookmarkModel* GetBookmarkModel() override;
favicon::FaviconService* GetFaviconService() override;
history::HistoryService* GetHistoryService() override;
scoped_refptr<password_manager::PasswordStore> GetPasswordStore() override;
autofill::PersonalDataManager* GetPersonalDataManager() override;
scoped_refptr<autofill::AutofillWebDataService> GetWebDataService() override;
BookmarkUndoService* GetBookmarkUndoServiceIfExists() override;
base::WeakPtr<syncer::SyncableService> GetSyncableServiceForType(
syncer::ModelType type) override;

Expand Down
55 changes: 21 additions & 34 deletions chrome/browser/sync/glue/bookmark_change_processor.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,14 @@
#include "base/strings/string_number_conversions.h"
#include "base/strings/string_util.h"
#include "base/strings/utf_string_conversions.h"
#include "chrome/browser/bookmarks/bookmark_model_factory.h"
#include "chrome/browser/favicon/favicon_service_factory.h"
#include "chrome/browser/history/history_service_factory.h"
#include "chrome/browser/sync/profile_sync_service.h"
#include "chrome/browser/undo/bookmark_undo_service_factory.h"
#include "components/bookmarks/browser/bookmark_client.h"
#include "components/bookmarks/browser/bookmark_model.h"
#include "components/bookmarks/browser/bookmark_utils.h"
#include "components/favicon/core/favicon_service.h"
#include "components/history/core/browser/history_service.h"
#include "components/sync_driver/sync_client.h"
#include "components/undo/bookmark_undo_service.h"
#include "components/undo/bookmark_undo_utils.h"
#include "content/public/browser/browser_thread.h"
#include "sync/internal_api/public/change_record.h"
#include "sync/internal_api/public/read_node.h"
#include "sync/internal_api/public/write_node.h"
Expand All @@ -37,7 +32,6 @@

using bookmarks::BookmarkModel;
using bookmarks::BookmarkNode;
using content::BrowserThread;
using syncer::ChangeRecord;
using syncer::ChangeRecordList;

Expand All @@ -46,16 +40,15 @@ namespace browser_sync {
static const char kMobileBookmarksTag[] = "synced_bookmarks";

BookmarkChangeProcessor::BookmarkChangeProcessor(
Profile* profile,
sync_driver::SyncClient* sync_client,
BookmarkModelAssociator* model_associator,
sync_driver::DataTypeErrorHandler* error_handler)
: sync_driver::ChangeProcessor(error_handler),
bookmark_model_(NULL),
profile_(profile),
sync_client_(sync_client),
model_associator_(model_associator) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
DCHECK(model_associator);
DCHECK(profile);
DCHECK(sync_client);
DCHECK(error_handler);
}

Expand All @@ -65,9 +58,9 @@ BookmarkChangeProcessor::~BookmarkChangeProcessor() {
}

void BookmarkChangeProcessor::StartImpl() {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
DCHECK(thread_checker_.CalledOnValidThread());
DCHECK(!bookmark_model_);
bookmark_model_ = BookmarkModelFactory::GetForProfile(profile_);
bookmark_model_ = sync_client_->GetBookmarkModel();
DCHECK(bookmark_model_->loaded());
bookmark_model_->AddObserver(this);
}
Expand Down Expand Up @@ -554,7 +547,7 @@ void BookmarkChangeProcessor::ApplyChangesFromSyncModel(
const syncer::BaseTransaction* trans,
int64 model_version,
const syncer::ImmutableChangeRecordList& changes) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
DCHECK(thread_checker_.CalledOnValidThread());
// A note about ordering. Sync backend is responsible for ordering the change
// records in the following order:
//
Expand All @@ -580,7 +573,7 @@ void BookmarkChangeProcessor::ApplyChangesFromSyncModel(

// Changes made to the bookmark model due to sync should not be undoable.
ScopedSuspendBookmarkUndo suspend_undo(
BookmarkUndoServiceFactory::GetForProfileIfExists(profile_));
sync_client_->GetBookmarkUndoServiceIfExists());

// Notify UI intensive observers of BookmarkModel that we are about to make
// potentially significant changes to it, so the updates may be batched. For
Expand Down Expand Up @@ -698,18 +691,15 @@ void BookmarkChangeProcessor::ApplyChangesFromSyncModel(
if (dst) {
DCHECK(it->action == ChangeRecord::ACTION_UPDATE)
<< "ACTION_UPDATE should be seen if and only if the node is known.";
UpdateBookmarkWithSyncData(src, model, dst, profile_);
UpdateBookmarkWithSyncData(src, model, dst, sync_client_);

// Move all modified entries to the right. We'll fix it later.
model->Move(dst, parent, parent->child_count());
} else {
DCHECK(it->action == ChangeRecord::ACTION_ADD)
<< "ACTION_ADD should be seen if and only if the node is unknown.";

dst = CreateBookmarkNode(&src,
parent,
model,
profile_,
dst = CreateBookmarkNode(&src, parent, model, sync_client_,
parent->child_count());
if (!dst) {
// We ignore bookmarks we can't add. Chances are this is caused by
Expand Down Expand Up @@ -766,7 +756,7 @@ void BookmarkChangeProcessor::UpdateBookmarkWithSyncData(
const syncer::BaseNode& sync_node,
BookmarkModel* model,
const BookmarkNode* node,
Profile* profile) {
sync_driver::SyncClient* sync_client) {
DCHECK_EQ(sync_node.GetIsFolder(), node->is_folder());
const sync_pb::BookmarkSpecifics& specifics =
sync_node.GetBookmarkSpecifics();
Expand All @@ -778,7 +768,7 @@ void BookmarkChangeProcessor::UpdateBookmarkWithSyncData(
node,
base::Time::FromInternalValue(specifics.creation_time_us()));
}
SetBookmarkFavicon(&sync_node, node, model, profile);
SetBookmarkFavicon(&sync_node, node, model, sync_client);
model->SetNodeMetaInfoMap(node, *GetBookmarkMetaInfo(&sync_node));
}

Expand All @@ -802,11 +792,11 @@ const BookmarkNode* BookmarkChangeProcessor::CreateBookmarkNode(
const syncer::BaseNode* sync_node,
const BookmarkNode* parent,
BookmarkModel* model,
Profile* profile,
sync_driver::SyncClient* sync_client,
int index) {
return CreateBookmarkNode(base::UTF8ToUTF16(sync_node->GetTitle()),
GURL(sync_node->GetBookmarkSpecifics().url()),
sync_node, parent, model, profile, index);
sync_node, parent, model, sync_client, index);
}

// static
Expand All @@ -818,7 +808,7 @@ const BookmarkNode* BookmarkChangeProcessor::CreateBookmarkNode(
const syncer::BaseNode* sync_node,
const BookmarkNode* parent,
BookmarkModel* model,
Profile* profile,
sync_driver::SyncClient* sync_client,
int index) {
DCHECK(parent);

Expand All @@ -837,7 +827,7 @@ const BookmarkNode* BookmarkChangeProcessor::CreateBookmarkNode(
parent, index, title, url, create_time,
GetBookmarkMetaInfo(sync_node).get());
if (node)
SetBookmarkFavicon(sync_node, node, model, profile);
SetBookmarkFavicon(sync_node, node, model, sync_client);
}

return node;
Expand All @@ -849,7 +839,7 @@ bool BookmarkChangeProcessor::SetBookmarkFavicon(
const syncer::BaseNode* sync_node,
const BookmarkNode* bookmark_node,
BookmarkModel* bookmark_model,
Profile* profile) {
sync_driver::SyncClient* sync_client) {
const sync_pb::BookmarkSpecifics& specifics =
sync_node->GetBookmarkSpecifics();
const std::string& icon_bytes_str = specifics.favicon();
Expand All @@ -867,7 +857,7 @@ bool BookmarkChangeProcessor::SetBookmarkFavicon(
if (icon_url.is_empty())
icon_url = bookmark_node->url();

ApplyBookmarkFavicon(bookmark_node, profile, icon_url, icon_bytes);
ApplyBookmarkFavicon(bookmark_node, sync_client, icon_url, icon_bytes);

return true;
}
Expand Down Expand Up @@ -938,14 +928,11 @@ void BookmarkChangeProcessor::SetSyncNodeMetaInfo(
// static
void BookmarkChangeProcessor::ApplyBookmarkFavicon(
const BookmarkNode* bookmark_node,
Profile* profile,
sync_driver::SyncClient* sync_client,
const GURL& icon_url,
const scoped_refptr<base::RefCountedMemory>& bitmap_data) {
history::HistoryService* history = HistoryServiceFactory::GetForProfile(
profile, ServiceAccessType::EXPLICIT_ACCESS);
favicon::FaviconService* favicon_service =
FaviconServiceFactory::GetForProfile(profile,
ServiceAccessType::EXPLICIT_ACCESS);
history::HistoryService* history = sync_client->GetHistoryService();
favicon::FaviconService* favicon_service = sync_client->GetFaviconService();

history->AddPageNoVisitForBookmark(bookmark_node->url(),
bookmark_node->GetTitle());
Expand Down
21 changes: 14 additions & 7 deletions chrome/browser/sync/glue/bookmark_change_processor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <vector>

#include "base/compiler_specific.h"
#include "base/threading/thread_checker.h"
#include "chrome/browser/sync/glue/bookmark_model_associator.h"
#include "components/bookmarks/browser/bookmark_model_observer.h"
#include "components/bookmarks/browser/bookmark_node.h"
Expand All @@ -25,6 +26,10 @@ class WriteNode;
class WriteTransaction;
} // namespace syncer

namespace sync_driver {
class SyncClient;
}

namespace browser_sync {

// This class is responsible for taking changes from the BookmarkModel
Expand All @@ -34,7 +39,7 @@ namespace browser_sync {
class BookmarkChangeProcessor : public bookmarks::BookmarkModelObserver,
public sync_driver::ChangeProcessor {
public:
BookmarkChangeProcessor(Profile* profile,
BookmarkChangeProcessor(sync_driver::SyncClient* sync_client,
BookmarkModelAssociator* model_associator,
sync_driver::DataTypeErrorHandler* error_handler);
~BookmarkChangeProcessor() override;
Expand Down Expand Up @@ -89,7 +94,7 @@ class BookmarkChangeProcessor : public bookmarks::BookmarkModelObserver,
static void UpdateBookmarkWithSyncData(const syncer::BaseNode& sync_node,
bookmarks::BookmarkModel* model,
const bookmarks::BookmarkNode* node,
Profile* profile);
sync_driver::SyncClient* sync_client);

// Creates a bookmark node under the given parent node from the given sync
// node. Returns the newly created node. The created node is placed at the
Expand All @@ -98,7 +103,7 @@ class BookmarkChangeProcessor : public bookmarks::BookmarkModelObserver,
const syncer::BaseNode* sync_node,
const bookmarks::BookmarkNode* parent,
bookmarks::BookmarkModel* model,
Profile* profile,
sync_driver::SyncClient* sync_client,
int index);

// Overload of CreateBookmarkNode function above that helps to avoid
Expand All @@ -109,7 +114,7 @@ class BookmarkChangeProcessor : public bookmarks::BookmarkModelObserver,
const syncer::BaseNode* sync_node,
const bookmarks::BookmarkNode* parent,
bookmarks::BookmarkModel* model,
Profile* profile,
sync_driver::SyncClient* sync_client,
int index);

// Sets the favicon of the given bookmark node from the given sync node.
Expand All @@ -119,14 +124,14 @@ class BookmarkChangeProcessor : public bookmarks::BookmarkModelObserver,
static bool SetBookmarkFavicon(const syncer::BaseNode* sync_node,
const bookmarks::BookmarkNode* bookmark_node,
bookmarks::BookmarkModel* model,
Profile* profile);
sync_driver::SyncClient* sync_client);

// Applies the 1x favicon |bitmap_data| and |icon_url| to |bookmark_node|.
// |profile| is the profile that contains the HistoryService and BookmarkModel
// for the bookmark in question.
static void ApplyBookmarkFavicon(
const bookmarks::BookmarkNode* bookmark_node,
Profile* profile,
sync_driver::SyncClient* sync_client,
const GURL& icon_url,
const scoped_refptr<base::RefCountedMemory>& bitmap_data);

Expand Down Expand Up @@ -233,11 +238,13 @@ class BookmarkChangeProcessor : public bookmarks::BookmarkModelObserver,
// Returns false if |node| should not be synced.
bool CanSyncNode(const bookmarks::BookmarkNode* node);

base::ThreadChecker thread_checker_;

// The bookmark model we are processing changes from. Non-NULL when
// |running_| is true.
bookmarks::BookmarkModel* bookmark_model_;

Profile* profile_;
sync_driver::SyncClient* sync_client_;

// The two models should be associated according to this ModelAssociator.
BookmarkModelAssociator* model_associator_;
Expand Down
Loading

0 comments on commit de2169a

Please sign in to comment.