Invoke AttachmentUploader and update AttachmentIds.

When GenericChangeProcessor calls AttachmentService::StoreAttachments
AttachmentService invokes AttachmentUploader to upload attachments.

When attachment gets uploaded AttachmentService invokes
AttachmentService::Delegate::OnAttachmentUploaded.

GenericChangeProcessor implements AttachmentService::Delegate for now.
When attachment is uploaded it call WriteTransaction to update entries
referring this attachment with new attachment id.

R=maniscalco@chromium.org
BUG=371629

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@271578 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/sync/glue/search_engine_data_type_controller_unittest.cc

@@ -61,8 +61,8 @@ class SyncSearchEngineDataTypeControllerTest : public testing::Test {
   void SetStartExpectations() {
     search_engine_dtc_->SetGenericChangeProcessorFactoryForTest(
         make_scoped_ptr<GenericChangeProcessorFactory>(
-            new FakeGenericChangeProcessorFactory(
-                make_scoped_ptr(new FakeGenericChangeProcessor()))));
+            new FakeGenericChangeProcessorFactory(make_scoped_ptr(
+                new FakeGenericChangeProcessor(profile_sync_factory_.get())))));
     EXPECT_CALL(model_load_callback_, Run(_, _));
     EXPECT_CALL(*profile_sync_factory_,
                 GetSyncableServiceForType(syncer::SEARCH_ENGINES)).

chrome/browser/sync/glue/shared_change_processor.cc

@@ -8,10 +8,7 @@
 #include "components/sync_driver/generic_change_processor.h"
 #include "components/sync_driver/generic_change_processor_factory.h"
 #include "components/sync_driver/sync_api_component_factory.h"
-#include "sync/api/attachments/attachment_service.h"
-#include "sync/api/attachments/attachment_service_impl.h"
 #include "sync/api/sync_change.h"
-#include "sync/internal_api/public/attachments/fake_attachment_uploader.h"
 
 using base::AutoLock;
 
@@ -69,23 +66,12 @@ base::WeakPtr<syncer::SyncableService> SharedChangeProcessor::Connect(
     return base::WeakPtr<syncer::SyncableService>();
   }
 
-  // TODO(maniscalco): Use a shared (one per profile) thread-safe instance of
-  // AttachmentUpload instead of creating a new one per AttachmentService (bug
-  // 369536).
-  scoped_ptr<syncer::AttachmentUploader> attachment_uploader(
-      new syncer::FakeAttachmentUploader);
-
-  scoped_ptr<syncer::AttachmentService> attachment_service(
-      new syncer::AttachmentServiceImpl(
-          sync_factory->CreateCustomAttachmentStoreForType(type),
-          attachment_uploader.Pass()));
-
-  generic_change_processor_ = processor_factory->CreateGenericChangeProcessor(
-      user_share,
-      error_handler,
-      local_service,
-      merge_result,
-      attachment_service.Pass()).release();
+  generic_change_processor_ =
+      processor_factory->CreateGenericChangeProcessor(user_share,
+                                                      error_handler,
+                                                      local_service,
+                                                      merge_result,
+                                                      sync_factory).release();
   return local_service;
 }
 

chrome/browser/sync/glue/ui_data_type_controller_unittest.cc

@@ -58,7 +58,8 @@ class SyncUIDataTypeControllerTest : public testing::Test {
 
  protected:
   void SetStartExpectations() {
-    scoped_ptr<FakeGenericChangeProcessor> p(new FakeGenericChangeProcessor());
+    scoped_ptr<FakeGenericChangeProcessor> p(
+        new FakeGenericChangeProcessor(profile_sync_factory_.get()));
     change_processor_ = p.get();
     scoped_ptr<GenericChangeProcessorFactory> f(
         new FakeGenericChangeProcessorFactory(p.Pass()));

chrome/browser/sync/profile_sync_components_factory.h

@@ -71,7 +71,7 @@ class ProfileSyncComponentsFactory
         : model_associator(ma), change_processor(cp) {}
   };
 
-  virtual ~ProfileSyncComponentsFactory() {}
+  virtual ~ProfileSyncComponentsFactory() OVERRIDE {}
 
   // Creates and registers enabled datatypes with the provided
   // ProfileSyncService.

chrome/browser/sync/profile_sync_components_factory_impl.cc

@@ -59,8 +59,10 @@
 #include "content/public/browser/browser_thread.h"
 #include "extensions/browser/extension_system.h"
 #include "sync/api/attachments/attachment_service.h"
+#include "sync/api/attachments/attachment_service_impl.h"
 #include "sync/api/syncable_service.h"
 #include "sync/internal_api/public/attachments/fake_attachment_store.h"
+#include "sync/internal_api/public/attachments/fake_attachment_uploader.h"
 
 #if defined(ENABLE_EXTENSIONS)
 #include "chrome/browser/extensions/api/storage/settings_sync_util.h"
@@ -564,13 +566,24 @@ base::WeakPtr<syncer::SyncableService> ProfileSyncComponentsFactoryImpl::
   }
 }
 
-scoped_ptr<syncer::AttachmentStore>
-    ProfileSyncComponentsFactoryImpl::CreateCustomAttachmentStoreForType(
-    syncer::ModelType type) {
-  scoped_ptr<syncer::AttachmentStore> store(
+scoped_ptr<syncer::AttachmentService>
+ProfileSyncComponentsFactoryImpl::CreateAttachmentService(
+    syncer::AttachmentService::Delegate* delegate) {
+  // TODO(maniscalco): Use a shared (one per profile) thread-safe instance of
+  // AttachmentUpload instead of creating a new one per AttachmentService (bug
+  // 369536).
+  scoped_ptr<syncer::AttachmentUploader> attachment_uploader(
+      new syncer::FakeAttachmentUploader);
+
+  scoped_ptr<syncer::AttachmentStore> attachment_store(
       new syncer::FakeAttachmentStore(
           BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO)));
-  return store.Pass();
+
+  scoped_ptr<syncer::AttachmentService> attachment_service(
+      new syncer::AttachmentServiceImpl(
+          attachment_store.Pass(), attachment_uploader.Pass(), delegate));
+
+  return attachment_service.Pass();
 }
 
 ProfileSyncComponentsFactory::SyncComponents

chrome/browser/sync/profile_sync_components_factory_impl.h

@@ -48,8 +48,8 @@ class ProfileSyncComponentsFactoryImpl : public ProfileSyncComponentsFactory {
 
   virtual base::WeakPtr<syncer::SyncableService> GetSyncableServiceForType(
       syncer::ModelType type) OVERRIDE;
-  virtual scoped_ptr<syncer::AttachmentStore>
-      CreateCustomAttachmentStoreForType(syncer::ModelType type) OVERRIDE;
+  virtual scoped_ptr<syncer::AttachmentService> CreateAttachmentService(
+      syncer::AttachmentService::Delegate* delegate) OVERRIDE;
 
   // Legacy datatypes that need to be converted to the SyncableService API.
   virtual SyncComponents CreateBookmarkSyncComponents(

chrome/browser/sync/profile_sync_components_factory_mock.cc

@@ -6,6 +6,7 @@
 #include "components/sync_driver/change_processor.h"
 #include "components/sync_driver/model_associator.h"
 #include "content/public/browser/browser_thread.h"
+#include "sync/api/attachments/attachment_service_impl.h"
 #include "sync/internal_api/public/attachments/fake_attachment_store.h"
 
 using browser_sync::AssociatorInterface;
@@ -28,18 +29,14 @@ ProfileSyncComponentsFactoryMock::ProfileSyncComponentsFactoryMock(
 
 ProfileSyncComponentsFactoryMock::~ProfileSyncComponentsFactoryMock() {}
 
-scoped_ptr<syncer::AttachmentStore>
-    ProfileSyncComponentsFactoryMock::CreateCustomAttachmentStoreForType(
-        syncer::ModelType type) {
-  scoped_ptr<syncer::AttachmentStore> store(
-      new syncer::FakeAttachmentStore(
-          content::BrowserThread::GetMessageLoopProxyForThread(
-              content::BrowserThread::IO)));
-  return store.Pass();
+scoped_ptr<syncer::AttachmentService>
+ProfileSyncComponentsFactoryMock::CreateAttachmentService(
+    syncer::AttachmentService::Delegate* delegate) {
+  return syncer::AttachmentServiceImpl::CreateForTest();
 }
 
 ProfileSyncComponentsFactory::SyncComponents
-    ProfileSyncComponentsFactoryMock::MakeSyncComponents() {
+ProfileSyncComponentsFactoryMock::MakeSyncComponents() {
   return SyncComponents(model_associator_.release(),
                         change_processor_.release());
 }

chrome/browser/sync/profile_sync_components_factory_mock.h

@@ -45,8 +45,8 @@ class ProfileSyncComponentsFactoryMock : public ProfileSyncComponentsFactory {
                    const base::FilePath& sync_folder));
   MOCK_METHOD1(GetSyncableServiceForType,
                base::WeakPtr<syncer::SyncableService>(syncer::ModelType));
-  virtual scoped_ptr<syncer::AttachmentStore>
-      CreateCustomAttachmentStoreForType(syncer::ModelType type) OVERRIDE;
+  virtual scoped_ptr<syncer::AttachmentService> CreateAttachmentService(
+      syncer::AttachmentService::Delegate* delegate) OVERRIDE;
   MOCK_METHOD2(CreateBookmarkSyncComponents,
       SyncComponents(ProfileSyncService* profile_sync_service,
                      browser_sync::DataTypeErrorHandler* error_handler));

components/sync_driver/fake_generic_change_processor.cc

@@ -11,12 +11,13 @@
 
 namespace browser_sync {
 
-FakeGenericChangeProcessor::FakeGenericChangeProcessor()
+FakeGenericChangeProcessor::FakeGenericChangeProcessor(
+    SyncApiComponentFactory* sync_factory)
     : GenericChangeProcessor(NULL,
                              base::WeakPtr<syncer::SyncableService>(),
                              base::WeakPtr<syncer::SyncMergeResult>(),
                              NULL,
-                             syncer::AttachmentServiceImpl::CreateForTest()),
+                             sync_factory),
       sync_model_has_user_created_nodes_(true),
       sync_model_has_user_created_nodes_success_(true) {}
 
@@ -75,7 +76,7 @@ FakeGenericChangeProcessorFactory::CreateGenericChangeProcessor(
     browser_sync::DataTypeErrorHandler* error_handler,
     const base::WeakPtr<syncer::SyncableService>& local_service,
     const base::WeakPtr<syncer::SyncMergeResult>& merge_result,
-    scoped_ptr<syncer::AttachmentService> attachment_service) {
+    SyncApiComponentFactory* sync_factory) {
   return processor_.PassAs<GenericChangeProcessor>();
 }
 

components/sync_driver/fake_generic_change_processor.h

@@ -8,6 +8,7 @@
 #include "components/sync_driver/generic_change_processor.h"
 
 #include "components/sync_driver/generic_change_processor_factory.h"
+#include "components/sync_driver/sync_api_component_factory.h"
 #include "sync/api/sync_error.h"
 
 namespace browser_sync {
@@ -15,7 +16,7 @@ namespace browser_sync {
 // A fake GenericChangeProcessor that can return arbitrary values.
 class FakeGenericChangeProcessor : public GenericChangeProcessor {
  public:
-  FakeGenericChangeProcessor();
+  FakeGenericChangeProcessor(SyncApiComponentFactory* sync_factory);
   virtual ~FakeGenericChangeProcessor();
 
   // Setters for GenericChangeProcessor implementation results.
@@ -52,7 +53,8 @@ class FakeGenericChangeProcessorFactory : public GenericChangeProcessorFactory {
       browser_sync::DataTypeErrorHandler* error_handler,
       const base::WeakPtr<syncer::SyncableService>& local_service,
       const base::WeakPtr<syncer::SyncMergeResult>& merge_result,
-      scoped_ptr<syncer::AttachmentService> attachment_service) OVERRIDE;
+      SyncApiComponentFactory* sync_factory) OVERRIDE;
+
  private:
   scoped_ptr<FakeGenericChangeProcessor> processor_;
   DISALLOW_COPY_AND_ASSIGN(FakeGenericChangeProcessorFactory);

components/sync_driver/generic_change_processor.cc

@@ -7,6 +7,7 @@
 #include "base/location.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
+#include "components/sync_driver/sync_api_component_factory.h"
 #include "sync/api/sync_change.h"
 #include "sync/api/sync_error.h"
 #include "sync/api/syncable_service.h"
@@ -91,12 +92,12 @@ GenericChangeProcessor::GenericChangeProcessor(
     const base::WeakPtr<syncer::SyncableService>& local_service,
     const base::WeakPtr<syncer::SyncMergeResult>& merge_result,
     syncer::UserShare* user_share,
-    scoped_ptr<syncer::AttachmentService> attachment_service)
+    SyncApiComponentFactory* sync_factory)
     : ChangeProcessor(error_handler),
       local_service_(local_service),
       merge_result_(merge_result),
       share_handle_(user_share),
-      attachment_service_(attachment_service.Pass()),
+      attachment_service_(sync_factory->CreateAttachmentService(this)),
       attachment_service_weak_ptr_factory_(attachment_service_.get()),
       attachment_service_proxy_(
           base::MessageLoopProxy::current(),
@@ -209,6 +210,12 @@ syncer::SyncError GenericChangeProcessor::UpdateDataTypeContext(
   return syncer::SyncError();
 }
 
+void GenericChangeProcessor::OnAttachmentUploaded(
+    const syncer::AttachmentId& attachment_id) {
+  syncer::WriteTransaction trans(FROM_HERE, share_handle());
+  trans.UpdateEntriesWithAttachmentId(attachment_id);
+}
+
 syncer::SyncError GenericChangeProcessor::GetAllSyncDataReturnError(
     syncer::ModelType type,
     syncer::SyncDataList* current_sync_data) const {

components/sync_driver/generic_change_processor.h

@@ -28,6 +28,7 @@ typedef std::vector<syncer::SyncData> SyncDataList;
 }  // namespace syncer
 
 namespace browser_sync {
+class SyncApiComponentFactory;
 
 // Datatype agnostic change processor. One instance of GenericChangeProcessor
 // is created for each datatype and lives on the datatype's thread. It then
@@ -39,6 +40,7 @@ namespace browser_sync {
 // be used on the same thread in which it was created.
 class GenericChangeProcessor : public ChangeProcessor,
                                public syncer::SyncChangeProcessor,
+                               public syncer::AttachmentService::Delegate,
                                public base::NonThreadSafe {
  public:
   // Create a change processor and connect it to the syncer.
@@ -47,7 +49,7 @@ class GenericChangeProcessor : public ChangeProcessor,
       const base::WeakPtr<syncer::SyncableService>& local_service,
       const base::WeakPtr<syncer::SyncMergeResult>& merge_result,
       syncer::UserShare* user_share,
-      scoped_ptr<syncer::AttachmentService> attachment_service);
+      SyncApiComponentFactory* sync_factory);
   virtual ~GenericChangeProcessor();
 
   // ChangeProcessor interface.
@@ -71,6 +73,10 @@ class GenericChangeProcessor : public ChangeProcessor,
       syncer::SyncChangeProcessor::ContextRefreshStatus refresh_status,
       const std::string& context) OVERRIDE;
 
+  // syncer::AttachmentService::Delegate implementation.
+  virtual void OnAttachmentUploaded(
+      const syncer::AttachmentId& attachment_id) OVERRIDE;
+
   // Similar to above, but returns a SyncError for use by direct clients
   // of GenericChangeProcessor that may need more error visibility.
   virtual syncer::SyncError GetAllSyncDataReturnError(

components/sync_driver/generic_change_processor_factory.cc

@@ -15,18 +15,17 @@ GenericChangeProcessorFactory::~GenericChangeProcessorFactory() {}
 
 scoped_ptr<GenericChangeProcessor>
 GenericChangeProcessorFactory::CreateGenericChangeProcessor(
-      syncer::UserShare* user_share,
-      browser_sync::DataTypeErrorHandler* error_handler,
-      const base::WeakPtr<syncer::SyncableService>& local_service,
-      const base::WeakPtr<syncer::SyncMergeResult>& merge_result,
-      scoped_ptr<syncer::AttachmentService> attachment_service) {
-    DCHECK(user_share);
-    return make_scoped_ptr(new GenericChangeProcessor(
-        error_handler,
-        local_service,
-        merge_result,
-        user_share,
-        attachment_service.Pass())).Pass();
+    syncer::UserShare* user_share,
+    browser_sync::DataTypeErrorHandler* error_handler,
+    const base::WeakPtr<syncer::SyncableService>& local_service,
+    const base::WeakPtr<syncer::SyncMergeResult>& merge_result,
+    SyncApiComponentFactory* sync_factory) {
+  DCHECK(user_share);
+  return make_scoped_ptr(new GenericChangeProcessor(error_handler,
+                                                    local_service,
+                                                    merge_result,
+                                                    user_share,
+                                                    sync_factory)).Pass();
   }
 
 }  // namespace browser_sync

components/sync_driver/generic_change_processor_factory.h

@@ -18,6 +18,7 @@ namespace browser_sync {
 
 class DataTypeErrorHandler;
 class GenericChangeProcessor;
+class SyncApiComponentFactory;
 
 // Because GenericChangeProcessors are created and used only from the model
 // thread, their lifetime is strictly shorter than other components like
@@ -26,6 +27,8 @@ class GenericChangeProcessor;
 // The GCP is created "on the fly" at just the right time, on just the right
 // thread. Given that, we use a factory to instantiate GenericChangeProcessors
 // so that tests can choose to use a fake processor (i.e instead of injection).
+// |sync_factory| is used to create AttachmentServicefor GenericChangeProcessor.
+// It is not retained after CreateGenericChangeProcessor exits.
 class GenericChangeProcessorFactory {
  public:
   GenericChangeProcessorFactory();
@@ -35,7 +38,8 @@ class GenericChangeProcessorFactory {
       browser_sync::DataTypeErrorHandler* error_handler,
       const base::WeakPtr<syncer::SyncableService>& local_service,
       const base::WeakPtr<syncer::SyncMergeResult>& merge_result,
-      scoped_ptr<syncer::AttachmentService> attachment_service);
+      SyncApiComponentFactory* sync_factory);
+
  private:
   DISALLOW_COPY_AND_ASSIGN(GenericChangeProcessorFactory);
 };

components/sync_driver/generic_change_processor_unittest.cc

@@ -9,6 +9,7 @@
 #include "base/message_loop/message_loop.h"
 #include "base/strings/stringprintf.h"
 #include "components/sync_driver/data_type_error_handler_mock.h"
+#include "components/sync_driver/sync_api_component_factory.h"
 #include "sync/api/attachments/attachment_service_impl.h"
 #include "sync/api/fake_syncable_service.h"
 #include "sync/api/sync_change.h"
@@ -49,7 +50,8 @@ MockAttachmentService::MockAttachmentService()
           scoped_ptr<syncer::AttachmentStore>(new syncer::FakeAttachmentStore(
               base::MessageLoopProxy::current())),
           scoped_ptr<syncer::AttachmentUploader>(
-              new syncer::FakeAttachmentUploader)) {
+              new syncer::FakeAttachmentUploader),
+          NULL) {
 }
 
 MockAttachmentService::~MockAttachmentService() {
@@ -66,6 +68,31 @@ std::vector<syncer::AttachmentList>* MockAttachmentService::attachment_lists() {
   return &attachment_lists_;
 }
 
+// MockSyncApiComponentFactory needed to initialize GenericChangeProcessor and
+// pass MockAttachmentService to it.
+class MockSyncApiComponentFactory : public SyncApiComponentFactory {
+ public:
+  MockSyncApiComponentFactory(
+      scoped_ptr<syncer::AttachmentService> attachment_service)
+      : attachment_service_(attachment_service.Pass()) {}
+
+  virtual base::WeakPtr<syncer::SyncableService> GetSyncableServiceForType(
+      syncer::ModelType type) OVERRIDE {
+    // Shouldn't be called for this test.
+    NOTREACHED();
+    return base::WeakPtr<syncer::SyncableService>();
+  }
+
+  virtual scoped_ptr<syncer::AttachmentService> CreateAttachmentService(
+      syncer::AttachmentService::Delegate* delegate) OVERRIDE {
+    EXPECT_TRUE(attachment_service_ != NULL);
+    return attachment_service_.Pass();
+  }
+
+ private:
+  scoped_ptr<syncer::AttachmentService> attachment_service_;
+};
+
 class SyncGenericChangeProcessorTest : public testing::Test {
  public:
   // It doesn't matter which type we use.  Just pick one.
@@ -93,12 +120,14 @@ class SyncGenericChangeProcessorTest : public testing::Test {
     // Take a pointer and trust that GenericChangeProcessor does not prematurely
     // destroy it.
     mock_attachment_service_ = mock_attachment_service.get();
-    change_processor_.reset(new GenericChangeProcessor(
-        &data_type_error_handler_,
-        syncable_service_ptr_factory_.GetWeakPtr(),
-        merge_result_ptr_factory_.GetWeakPtr(),
-        test_user_share_.user_share(),
+    sync_factory_.reset(new MockSyncApiComponentFactory(
         mock_attachment_service.PassAs<syncer::AttachmentService>()));
+    change_processor_.reset(
+        new GenericChangeProcessor(&data_type_error_handler_,
+                                   syncable_service_ptr_factory_.GetWeakPtr(),
+                                   merge_result_ptr_factory_.GetWeakPtr(),
+                                   test_user_share_.user_share(),
+                                   sync_factory_.get()));
   }
 
   virtual void TearDown() OVERRIDE {
@@ -142,6 +171,7 @@ class SyncGenericChangeProcessorTest : public testing::Test {
   DataTypeErrorHandlerMock data_type_error_handler_;
   syncer::TestUserShare test_user_share_;
   MockAttachmentService* mock_attachment_service_;
+  scoped_ptr<SyncApiComponentFactory> sync_factory_;
 
   scoped_ptr<GenericChangeProcessor> change_processor_;
 };
@@ -346,6 +376,44 @@ TEST_F(SyncGenericChangeProcessorTest,
   ASSERT_EQ(new_attachments_added[0].GetId(), new_attachments[0].GetId());
 }
 
+// Verify that after attachment is uploaded GenericChangeProcessor updates
+// corresponding entries
+TEST_F(SyncGenericChangeProcessorTest, AttachmentUploaded) {
+  std::string tag = "client_tag";
+  std::string title = "client_title";
+  sync_pb::EntitySpecifics specifics;
+  sync_pb::PreferenceSpecifics* pref_specifics = specifics.mutable_preference();
+  pref_specifics->set_name("test");
+  syncer::AttachmentList attachments;
+  scoped_refptr<base::RefCountedString> attachment_data =
+      new base::RefCountedString;
+  attachment_data->data() = kTestData;
+  attachments.push_back(syncer::Attachment::Create(attachment_data));
+
+  // Add a SyncData with two attachments.
+  syncer::SyncChangeList change_list;
+  change_list.push_back(
+      syncer::SyncChange(FROM_HERE,
+                         syncer::SyncChange::ACTION_ADD,
+                         syncer::SyncData::CreateLocalDataWithAttachments(
+                             tag, title, specifics, attachments)));
+  ASSERT_FALSE(
+      change_processor()->ProcessSyncChanges(FROM_HERE, change_list).IsSet());
+
+  sync_pb::AttachmentIdProto attachment_id_proto =
+      attachments[0].GetId().GetProto();
+  syncer::AttachmentId attachment_id =
+      syncer::AttachmentId::CreateFromProto(attachment_id_proto);
+
+  change_processor()->OnAttachmentUploaded(attachment_id);
+  syncer::ReadTransaction read_transaction(FROM_HERE, user_share());
+  syncer::ReadNode node(&read_transaction);
+  ASSERT_EQ(node.InitByClientTagLookup(syncer::PREFERENCES, tag),
+            syncer::BaseNode::INIT_OK);
+  syncer::AttachmentIdList attachment_ids = node.GetAttachmentIds();
+  EXPECT_EQ(1U, attachment_ids.size());
+}
+
 }  // namespace
 
 }  // namespace browser_sync

components/sync_driver/sync_api_component_factory.h

@@ -7,7 +7,7 @@
 
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
-#include "sync/api/attachments/attachment_store.h"
+#include "sync/api/attachments/attachment_service.h"
 #include "sync/api/syncable_service.h"
 #include "sync/internal_api/public/base/model_type.h"
 
@@ -17,17 +17,22 @@ namespace browser_sync {
 // service (like SyncableService) implementations.
 class SyncApiComponentFactory {
  public:
+  virtual ~SyncApiComponentFactory() {}
+
   // Returns a weak pointer to the syncable service specified by |type|.
   // Weak pointer may be unset if service is already destroyed.
   // Note: Should only be called from the model type thread.
   virtual base::WeakPtr<syncer::SyncableService> GetSyncableServiceForType(
       syncer::ModelType type) = 0;
 
-  // Returns the custom attachment store for a model type, if there is one.
-  // May return NULL, which implies sync should use a default implementation.
+  // Creates attachment service.
   // Note: Should only be called from the model type thread.
-  virtual scoped_ptr<syncer::AttachmentStore>
-      CreateCustomAttachmentStoreForType(syncer::ModelType type) = 0;
+  // |delegate| is optional delegate for AttachmentService to notify about
+  // asynchronous events (AttachmentUploaded). Pass NULL if delegate is not
+  // provided. AttachmentService doesn't take ownership of delegate, the pointer
+  // must be valid throughout AttachmentService lifetime.
+  virtual scoped_ptr<syncer::AttachmentService> CreateAttachmentService(
+      syncer::AttachmentService::Delegate* delegate) = 0;
 };
 
 }  // namespace browser_sync

sync/api/attachments/attachment_service.h

@@ -54,6 +54,19 @@ class SYNC_EXPORT AttachmentService {
 
   typedef base::Callback<void(const StoreResult&)> StoreCallback;
 
+  // An interface that embedder code implements to be notified about different
+  // events that originate from AttachmentService.
+  // This interface will be called from the same thread AttachmentService was
+  // created and called.
+  class Delegate {
+   public:
+    virtual ~Delegate() {}
+
+    // Attachment is uploaded to server and attachment_id is updated with server
+    // url.
+    virtual void OnAttachmentUploaded(const AttachmentId& attachment_id) = 0;
+  };
+
   AttachmentService();
   virtual ~AttachmentService();
 

sync/api/attachments/attachment_service_impl.cc

@@ -14,9 +14,11 @@ namespace syncer {
 
 AttachmentServiceImpl::AttachmentServiceImpl(
     scoped_ptr<AttachmentStore> attachment_store,
-    scoped_ptr<AttachmentUploader> attachment_uploader)
+    scoped_ptr<AttachmentUploader> attachment_uploader,
+    Delegate* delegate)
     : attachment_store_(attachment_store.Pass()),
       attachment_uploader_(attachment_uploader.Pass()),
+      delegate_(delegate),
       weak_ptr_factory_(this) {
   DCHECK(CalledOnValidThread());
   DCHECK(attachment_store_);
@@ -34,8 +36,8 @@ scoped_ptr<syncer::AttachmentService> AttachmentServiceImpl::CreateForTest() {
   scoped_ptr<AttachmentUploader> attachment_uploader(
       new FakeAttachmentUploader);
   scoped_ptr<syncer::AttachmentService> attachment_service(
-      new syncer::AttachmentServiceImpl(attachment_store.Pass(),
-                                        attachment_uploader.Pass()));
+      new syncer::AttachmentServiceImpl(
+          attachment_store.Pass(), attachment_uploader.Pass(), NULL));
   return attachment_service.Pass();
 }
 
@@ -66,8 +68,14 @@ void AttachmentServiceImpl::StoreAttachments(const AttachmentList& attachments,
                            base::Bind(&AttachmentServiceImpl::WriteDone,
                                       weak_ptr_factory_.GetWeakPtr(),
                                       callback));
-  // TODO(maniscalco): Ensure the linked attachments are schedule for upload to
-  // the server (bug 356351).
+  for (AttachmentList::const_iterator iter = attachments.begin();
+       iter != attachments.end();
+       ++iter) {
+    attachment_uploader_->UploadAttachment(
+        *iter,
+        base::Bind(&AttachmentServiceImpl::UploadDone,
+                   weak_ptr_factory_.GetWeakPtr()));
+  }
 }
 
 void AttachmentServiceImpl::OnSyncDataDelete(const SyncData& sync_data) {
@@ -124,4 +132,15 @@ void AttachmentServiceImpl::WriteDone(const StoreCallback& callback,
                                          base::Bind(callback, store_result));
 }
 
+void AttachmentServiceImpl::UploadDone(
+    const AttachmentUploader::UploadResult& result,
+    const AttachmentId& attachment_id) {
+  // TODO(pavely): crbug/372622: Deal with UploadAttachment failures.
+  if (result != AttachmentUploader::UPLOAD_SUCCESS)
+    return;
+  if (delegate_) {
+    delegate_->OnAttachmentUploaded(attachment_id);
+  }
+}
+
 }  // namespace syncer

sync/api/attachments/attachment_service_impl.h

@@ -18,8 +18,13 @@ namespace syncer {
 class SYNC_EXPORT AttachmentServiceImpl : public AttachmentService,
                                           public base::NonThreadSafe {
  public:
+  // |delegate| is optional delegate for AttachmentService to notify about
+  // asynchronous events (AttachmentUploaded). Pass NULL if delegate is not
+  // provided. AttachmentService doesn't take ownership of delegate, the pointer
+  // must be valid throughout AttachmentService lifetime.
   AttachmentServiceImpl(scoped_ptr<AttachmentStore> attachment_store,
-                        scoped_ptr<AttachmentUploader> attachment_uploader);
+                        scoped_ptr<AttachmentUploader> attachment_uploader,
+                        Delegate* delegate);
   virtual ~AttachmentServiceImpl();
 
   // Create an AttachmentServiceImpl suitable for use in tests.
@@ -45,9 +50,12 @@ class SYNC_EXPORT AttachmentServiceImpl : public AttachmentService,
                 const AttachmentStore::Result& result);
   void WriteDone(const StoreCallback& callback,
                  const AttachmentStore::Result& result);
+  void UploadDone(const AttachmentUploader::UploadResult& result,
+                  const AttachmentId& attachment_id);
 
   const scoped_ptr<AttachmentStore> attachment_store_;
   const scoped_ptr<AttachmentUploader> attachment_uploader_;
+  Delegate* delegate_;
   // Must be last data member.
   base::WeakPtrFactory<AttachmentServiceImpl> weak_ptr_factory_;
 

sync/internal_api/public/write_transaction.h

@@ -52,6 +52,10 @@ class SYNC_EXPORT WriteTransaction : public BaseTransaction {
       syncer::SyncChangeProcessor::ContextRefreshStatus refresh_status,
       const std::string& context);
 
+  // Attachment id was just updated. Propagate this change to all entries that
+  // refer this attachment id and set is_on_server for corresponding records.
+  void UpdateEntriesWithAttachmentId(const AttachmentId& attachment_id);
+
  protected:
   WriteTransaction() {}
 

sync/internal_api/write_transaction.cc

@@ -5,6 +5,7 @@
 #include "sync/internal_api/public/write_transaction.h"
 
 #include "sync/syncable/directory.h"
+#include "sync/syncable/mutable_entry.h"
 #include "sync/syncable/syncable_write_transaction.h"
 
 namespace syncer {
@@ -80,4 +81,17 @@ void WriteTransaction::SetDataTypeContext(
   // See crbug.com/360280
 }
 
+void WriteTransaction::UpdateEntriesWithAttachmentId(
+    const AttachmentId& attachment_id) {
+  syncable::Directory::Metahandles handles;
+  GetDirectory()->GetMetahandlesByAttachmentId(
+      transaction_, attachment_id.GetProto(), &handles);
+  for (syncable::Directory::Metahandles::iterator iter = handles.begin();
+       iter != handles.end();
+       ++iter) {
+    syncable::MutableEntry entry(transaction_, syncable::GET_BY_HANDLE, *iter);
+    entry.UpdateAttachmentIdWithServerInfo(attachment_id.GetProto());
+  }
+}
+
 }  // namespace syncer

sync/syncable/directory.cc

@@ -463,6 +463,22 @@ void Directory::UpdateAttachmentIndex(
   AddToAttachmentIndex(metahandle, new_metadata, lock);
 }
 
+void Directory::GetMetahandlesByAttachmentId(
+    BaseTransaction* trans,
+    const sync_pb::AttachmentIdProto& attachment_id_proto,
+    Metahandles* result) {
+  DCHECK(result);
+  result->clear();
+  ScopedKernelLock lock(this);
+  IndexByAttachmentId::const_iterator index_iter =
+      kernel_->index_by_attachment_id.find(attachment_id_proto.unique_id());
+  if (index_iter == kernel_->index_by_attachment_id.end())
+    return;
+  const MetahandleSet& metahandle_set = index_iter->second;
+  std::copy(
+      metahandle_set.begin(), metahandle_set.end(), back_inserter(*result));
+}
+
 bool Directory::unrecoverable_error_set(const BaseTransaction* trans) const {
   DCHECK(trans != NULL);
   return unrecoverable_error_set_;

sync/syncable/directory.h

@@ -397,6 +397,13 @@ class SYNC_EXPORT Directory {
   bool IsAttachmentLinked(
       const sync_pb::AttachmentIdProto& attachment_id_proto) const;
 
+  // Given attachment id return metahandles to all entries that reference this
+  // attachment.
+  void GetMetahandlesByAttachmentId(
+      BaseTransaction* trans,
+      const sync_pb::AttachmentIdProto& attachment_id_proto,
+      Metahandles* result);
+
  protected:  // for friends, mainly used by Entry constructors
   virtual EntryKernel* GetEntryByHandle(int64 handle);
   virtual EntryKernel* GetEntryByHandle(int64 metahandle,

sync/syncable/directory_unittest.cc

@@ -1599,21 +1599,64 @@ TEST_F(SyncableDirectoryTest, MutableEntry_PutAttachmentMetadata) {
         &trans, CREATE, PREFERENCES, trans.root_id(), "some entry");
     entry.PutId(TestIdFactory::FromNumber(-1));
     entry.PutIsUnsynced(true);
+
+    Directory::Metahandles metahandles;
     ASSERT_FALSE(dir()->IsAttachmentLinked(attachment_id_proto));
+    dir()->GetMetahandlesByAttachmentId(
+        &trans, attachment_id_proto, &metahandles);
+    ASSERT_TRUE(metahandles.empty());
 
     // Now add the attachment metadata and see that Directory believes it is
     // linked.
     entry.PutAttachmentMetadata(attachment_metadata);
     ASSERT_TRUE(dir()->IsAttachmentLinked(attachment_id_proto));
+    dir()->GetMetahandlesByAttachmentId(
+        &trans, attachment_id_proto, &metahandles);
+    ASSERT_FALSE(metahandles.empty());
+    ASSERT_EQ(metahandles[0], entry.GetMetahandle());
 
     // Clear out the attachment metadata and see that it's no longer linked.
     sync_pb::AttachmentMetadata empty_attachment_metadata;
     entry.PutAttachmentMetadata(empty_attachment_metadata);
     ASSERT_FALSE(dir()->IsAttachmentLinked(attachment_id_proto));
+    dir()->GetMetahandlesByAttachmentId(
+        &trans, attachment_id_proto, &metahandles);
+    ASSERT_TRUE(metahandles.empty());
   }
   ASSERT_FALSE(dir()->IsAttachmentLinked(attachment_id_proto));
 }
 
+// Verify that UpdateAttachmentId updates attachment_id and is_on_server flag.
+TEST_F(SyncableDirectoryTest, MutableEntry_UpdateAttachmentId) {
+  sync_pb::AttachmentMetadata attachment_metadata;
+  sync_pb::AttachmentMetadataRecord* r1 = attachment_metadata.add_record();
+  sync_pb::AttachmentMetadataRecord* r2 = attachment_metadata.add_record();
+  *r1->mutable_id() = syncer::CreateAttachmentIdProto();
+  *r2->mutable_id() = syncer::CreateAttachmentIdProto();
+  sync_pb::AttachmentIdProto attachment_id_proto = r1->id();
+
+  WriteTransaction trans(FROM_HERE, UNITTEST, dir().get());
+
+  MutableEntry entry(
+      &trans, CREATE, PREFERENCES, trans.root_id(), "some entry");
+  entry.PutId(TestIdFactory::FromNumber(-1));
+  entry.PutAttachmentMetadata(attachment_metadata);
+
+  const sync_pb::AttachmentMetadata& entry_metadata =
+      entry.GetAttachmentMetadata();
+  ASSERT_EQ(2, entry_metadata.record_size());
+  ASSERT_FALSE(entry_metadata.record(0).is_on_server());
+  ASSERT_FALSE(entry_metadata.record(1).is_on_server());
+  ASSERT_FALSE(entry.GetIsUnsynced());
+
+  // TODO(pavely): When we add server info to proto, add test for it here.
+  entry.UpdateAttachmentIdWithServerInfo(attachment_id_proto);
+
+  ASSERT_TRUE(entry_metadata.record(0).is_on_server());
+  ASSERT_FALSE(entry_metadata.record(1).is_on_server());
+  ASSERT_TRUE(entry.GetIsUnsynced());
+}
+
 // Verify that deleted entries with attachments will retain the attachments.
 TEST_F(SyncableDirectoryTest, Directory_DeleteDoesNotUnlinkAttachments) {
   sync_pb::AttachmentMetadata attachment_metadata;

sync/syncable/mutable_entry.cc

@@ -247,6 +247,25 @@ void MutableEntry::PutAttachmentMetadata(
   }
 }
 
+void MutableEntry::UpdateAttachmentIdWithServerInfo(
+    const sync_pb::AttachmentIdProto& updated_attachment_id) {
+  DCHECK(kernel_);
+  DCHECK(!updated_attachment_id.unique_id().empty());
+  write_transaction()->TrackChangesTo(kernel_);
+  sync_pb::AttachmentMetadata& attachment_metadata =
+      kernel_->mutable_ref(ATTACHMENT_METADATA);
+  for (int i = 0; i < attachment_metadata.record_size(); ++i) {
+    sync_pb::AttachmentMetadataRecord* record =
+        attachment_metadata.mutable_record(i);
+    if (record->id().unique_id() != updated_attachment_id.unique_id())
+      continue;
+    *record->mutable_id() = updated_attachment_id;
+    record->set_is_on_server(true);
+  }
+  kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
+  MarkForSyncing(this);
+}
+
 // This function sets only the flags needed to get this entry to sync.
 bool MarkForSyncing(MutableEntry* e) {
   DCHECK_NE(static_cast<MutableEntry*>(NULL), e);

sync/syncable/mutable_entry.h

@@ -62,6 +62,12 @@ class SYNC_EXPORT_PRIVATE MutableEntry : public ModelNeutralMutableEntry {
   void PutAttachmentMetadata(
       const sync_pb::AttachmentMetadata& attachment_metadata);
 
+  // Update attachment metadata, replace all records matching attachment id's
+  // unique id with updated attachment id that contains server info.
+  // Set is_in_server for corresponding records.
+  void UpdateAttachmentIdWithServerInfo(
+      const sync_pb::AttachmentIdProto& updated_attachment_id);
+
  private:
   // Kind of redundant. We should reduce the number of pointers
   // floating around if at all possible. Could we store this in Directory?