Review Feedback (2)

  1. Remvoe weak_ptr for syncer and make sure get_record_cb_ won't get
  called when sync_thread is not running
  2. Remove unnecessary forward declaration

chromium_src/chrome/browser/sync/profile_sync_service_factory.cc

@@ -4,24 +4,15 @@
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "components/sync/driver/profile_sync_service.h"
-
-class Profile;
-
-namespace {
-std::unique_ptr<syncer::ProfileSyncService> BraveBuildServiceInstanceFor(
-    Profile* profile,
-    syncer::ProfileSyncService::InitParams init_params);
-}  // namespace
-
-#include "../../../../../chrome/browser/sync/profile_sync_service_factory.cc"  // NOLINT
-
 #include "brave/components/brave_sync/buildflags/buildflags.h"
 
 #if BUILDFLAG(ENABLE_BRAVE_SYNC)
 #include "brave/components/brave_sync/brave_profile_sync_service_impl.h"
 using brave_sync::BraveProfileSyncServiceImpl;
 #endif
 
+class Profile;
+
 namespace {
 
 std::unique_ptr<syncer::ProfileSyncService> BraveBuildServiceInstanceFor(
@@ -36,3 +27,5 @@ std::unique_ptr<syncer::ProfileSyncService> BraveBuildServiceInstanceFor(
 }
 
 }  // namespace
+
+#include "../../../../../chrome/browser/sync/profile_sync_service_factory.cc"  // NOLINT

chromium_src/components/sync/driver/glue/sync_engine_backend.cc

@@ -46,13 +46,14 @@ void OnPollSyncCycleOnOwnerThread(base::WeakPtr<SyncEngineImpl> sync_engine,
   if (sync_engine.get())
     static_cast<BraveProfileSyncService*>(
         BraveGetSyncEngineHost(sync_engine.get()))
-        ->OnPollSyncCycle(cb, wevent);
+        ->OnPollSyncCycle(std::move(cb), wevent);
 }
 
 void OnPollSyncCycle(WeakHandle<SyncEngineImpl> sync_engine_impl,
                      GetRecordsCallback cb,
                      base::WaitableEvent* wevent) {
-  sync_engine_impl.Call(FROM_HERE, &OnPollSyncCycleOnOwnerThread, cb, wevent);
+  sync_engine_impl.Call(FROM_HERE, &OnPollSyncCycleOnOwnerThread,
+                        base::Passed(&cb), wevent);
 }
 #endif
 

chromium_src/components/sync/driver/profile_sync_service.cc

@@ -43,7 +43,7 @@ void OnPollSyncCycle(base::WeakPtr<ProfileSyncService> profile_sync_service,
                      base::WaitableEvent* wevent) {
   if (profile_sync_service.get()) {
     static_cast<BraveProfileSyncService*>(profile_sync_service.get())
-        ->OnPollSyncCycle(cb, wevent);
+        ->OnPollSyncCycle(std::move(cb), wevent);
   }
 }
 #endif

chromium_src/components/sync/engine_impl/sync_scheduler_impl.cc

@@ -18,7 +18,7 @@ void SyncSchedulerImpl::OnNudgeSyncCycle(
 void SyncSchedulerImpl::OnPollSyncCycle(brave_sync::GetRecordsCallback cb,
                                         base::WaitableEvent* wevent) {
   DCHECK(poll_sync_cycle_delegate_function_);
-  poll_sync_cycle_delegate_function_.Run(cb, wevent);
+  poll_sync_cycle_delegate_function_.Run(std::move(cb), wevent);
 }
 
 }  // namespace syncer

chromium_src/components/sync/engine_impl/syncer.cc

@@ -8,6 +8,8 @@
 #include <memory>
 #include <utility>
 
+#include "base/bind.h"
+
 #include "../../../../../components/sync/engine_impl/syncer.cc"  // NOLINT
 
 namespace syncer {
@@ -23,9 +25,9 @@ void Syncer::DownloadBraveRecords(SyncCycle* cycle) {
   // syncer will be alive as long as sync is enabled
   brave_records_.reset();
   brave_sync::GetRecordsCallback on_get_records =
-      base::BindRepeating(&Syncer::OnGetRecords, AsWeakPtr());
+      base::BindOnce(&Syncer::OnGetRecords, base::Unretained(this));
   base::WaitableEvent wevent;
-  cycle->delegate()->OnPollSyncCycle(on_get_records, &wevent);
+  cycle->delegate()->OnPollSyncCycle(std::move(on_get_records), &wevent);
   // Make sure OnGetRecords will be the next task on sync thread
   wevent.Wait();
 }

chromium_src/components/sync/engine_impl/syncer.h

@@ -6,7 +6,6 @@
 #ifndef BRAVE_CHROMIUM_SRC_COMPONENTS_SYNC_ENGINE_IMPL_SYNCER_H_
 #define BRAVE_CHROMIUM_SRC_COMPONENTS_SYNC_ENGINE_IMPL_SYNCER_H_
 
-#include "base/memory/weak_ptr.h"
 #include "brave/components/brave_sync/jslib_messages.h"
 #include "brave/components/brave_sync/jslib_messages_fwd.h"
 

components/brave_sync/brave_profile_sync_service_impl.cc

@@ -128,7 +128,7 @@ std::unique_ptr<SyncRecord> CreateDeleteBookmarkByObjectId(
 void DoDispatchGetRecordsCallback(
     GetRecordsCallback cb,
     std::unique_ptr<brave_sync::RecordsList> records) {
-  cb.Run(std::move(records));
+  std::move(cb).Run(std::move(records));
 }
 
 void AddSyncEntityInfo(jslib::Bookmark* bookmark,
@@ -301,7 +301,7 @@ void BraveProfileSyncServiceImpl::OnResetSync() {
     ResetSyncInternal();
   } else {
     // We have to send delete record and wait for library deleted response then
-    // we can reset it by ResetInternal()
+    // we can reset it by ResetSyncInternal()
     const std::string device_id = brave_sync_prefs_->GetThisDeviceId();
     OnDeleteDevice(device_id);
     reseting_ = true;
@@ -524,8 +524,9 @@ void BraveProfileSyncServiceImpl::OnResolvedSyncRecords(
     // Send records to syncer
     if (get_record_cb_)
       sync_thread_->task_runner()->PostTask(
-          FROM_HERE, base::BindOnce(&DoDispatchGetRecordsCallback,
-                                    get_record_cb_, std::move(records)));
+          FROM_HERE,
+          base::BindOnce(&DoDispatchGetRecordsCallback,
+                         std::move(get_record_cb_), std::move(records)));
     SignalWaitableEvent();
   } else if (category_name == kHistorySites) {
     NOTIMPLEMENTED();
@@ -608,6 +609,7 @@ void BraveProfileSyncServiceImpl::NotifyHaveSyncWords(
 }
 
 void BraveProfileSyncServiceImpl::ResetSyncInternal() {
+  SignalWaitableEvent();
   brave_sync_prefs_->SetPrevSeed(brave_sync_prefs_->GetSeed());
 
   brave_sync_prefs_->Clear();
@@ -912,7 +914,7 @@ void BraveProfileSyncServiceImpl::OnPollSyncCycle(GetRecordsCallback cb,
     return;
   }
 
-  get_record_cb_ = cb;
+  get_record_cb_ = std::move(cb);
   wevent_ = wevent;
 
   const bool bookmarks = brave_sync_prefs_->GetSyncBookmarksEnabled();
@@ -923,6 +925,7 @@ void BraveProfileSyncServiceImpl::OnPollSyncCycle(GetRecordsCallback cb,
 }
 
 void BraveProfileSyncServiceImpl::SignalWaitableEvent() {
+  std::move(get_record_cb_);
   if (wevent_) {
     wevent_->Signal();
     wevent_ = nullptr;

components/brave_sync/jslib_messages_fwd.h

@@ -40,10 +40,11 @@ typedef std::vector<SyncRecordAndExistingPtr> SyncRecordAndExistingList;
 
 using Uint8Array = std::vector<unsigned char>;
 using GetRecordsCallback =
-    base::RepeatingCallback<void(std::unique_ptr<RecordsList>)>;
-using NudgeSyncCycleDelegate = base::Callback<void(RecordsListPtr)>;
+    base::OnceCallback<void(std::unique_ptr<RecordsList>)>;
+using NudgeSyncCycleDelegate = base::RepeatingCallback<void(RecordsListPtr)>;
 using PollSyncCycleDelegate =
-    base::Callback<void(GetRecordsCallback, base::WaitableEvent* wevent)>;
+    base::RepeatingCallback<void(GetRecordsCallback,
+                                 base::WaitableEvent* wevent)>;
 
 }  // namespace brave_sync
 

patches/components-sync-engine_impl-syncer.h.patch

@@ -1,16 +1,7 @@
 diff --git a/components/sync/engine_impl/syncer.h b/components/sync/engine_impl/syncer.h
-index 23b389eada81c850c3a8856678d046e599a060da..48654f4c09558ea4d0ac246ebe16178019b30a0e 100644
+index 23b389eada81c850c3a8856678d046e599a060da..30ce667ae1e00e51bf4f227084da8f4ca30f8dc2 100644
 --- a/components/sync/engine_impl/syncer.h
 +++ b/components/sync/engine_impl/syncer.h
-@@ -32,7 +32,7 @@ class SyncCycle;
- // A Syncer instance expects to run on a dedicated thread.  Calls to SyncShare()
- // may take an unbounded amount of time because it may block on network I/O, on
- // lock contention, or on tasks posted to other threads.
--class Syncer {
-+class Syncer : public base::SupportsWeakPtr<Syncer> {
-  public:
-   explicit Syncer(CancelationSignal* cancelation_signal);
-   virtual ~Syncer();
 @@ -94,6 +94,7 @@ class Syncer {
    // Whether the syncer is in the middle of a sync attempt.
    bool is_syncing_;