diff --git a/components/reading_list/core/dual_reading_list_model.cc b/components/reading_list/core/dual_reading_list_model.cc index d0c3df4728d0e..abe0344b72595 100644 --- a/components/reading_list/core/dual_reading_list_model.cc +++ b/components/reading_list/core/dual_reading_list_model.cc @@ -97,6 +97,7 @@ base::flat_set DualReadingListModel::GetKeys() const { size_t DualReadingListModel::size() const { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + DCHECK(loaded()); DCHECK_EQ(unread_entry_count_ + read_entry_count_, GetKeys().size()); return unread_entry_count_ + read_entry_count_; @@ -496,6 +497,11 @@ void DualReadingListModel::RemoveObserver(ReadingListModelObserver* observer) { void DualReadingListModel::ReadingListModelBeganBatchUpdates( const ReadingListModel* model) { + DCHECK(!suppress_observer_notifications_); + + if (!loaded()) { + return; + } ++current_batch_updates_count_; if (current_batch_updates_count_ == 1) { for (auto& observer : observers_) { @@ -506,6 +512,11 @@ void DualReadingListModel::ReadingListModelBeganBatchUpdates( void DualReadingListModel::ReadingListModelCompletedBatchUpdates( const ReadingListModel* model) { + DCHECK(!suppress_observer_notifications_); + + if (!loaded()) { + return; + } --current_batch_updates_count_; if (current_batch_updates_count_ == 0) { for (auto& observer : observers_) { @@ -533,7 +544,7 @@ void DualReadingListModel::ReadingListModelLoaded( void DualReadingListModel::ReadingListWillRemoveEntry( const ReadingListModel* model, const GURL& url) { - if (suppress_observer_notifications_) { + if (!loaded() || suppress_observer_notifications_) { return; } @@ -553,7 +564,7 @@ void DualReadingListModel::ReadingListWillRemoveEntry( void DualReadingListModel::ReadingListDidRemoveEntry( const ReadingListModel* model, const GURL& url) { - if (suppress_observer_notifications_) { + if (!loaded() || suppress_observer_notifications_) { return; } @@ -572,7 +583,7 @@ void DualReadingListModel::ReadingListDidRemoveEntry( void DualReadingListModel::ReadingListWillMoveEntry( const ReadingListModel* model, const GURL& url) { - if (suppress_observer_notifications_) { + if (!loaded() || suppress_observer_notifications_) { return; } @@ -586,7 +597,7 @@ void DualReadingListModel::ReadingListWillMoveEntry( void DualReadingListModel::ReadingListDidMoveEntry( const ReadingListModel* model, const GURL& url) { - if (suppress_observer_notifications_) { + if (!loaded() || suppress_observer_notifications_) { return; } @@ -600,7 +611,7 @@ void DualReadingListModel::ReadingListDidMoveEntry( void DualReadingListModel::ReadingListWillAddEntry( const ReadingListModel* model, const ReadingListEntry& entry) { - if (suppress_observer_notifications_) { + if (!loaded() || suppress_observer_notifications_) { return; } @@ -623,7 +634,7 @@ void DualReadingListModel::ReadingListDidAddEntry( const ReadingListModel* model, const GURL& url, reading_list::EntrySource source) { - if (suppress_observer_notifications_) { + if (!loaded() || suppress_observer_notifications_) { return; } @@ -665,9 +676,10 @@ void DualReadingListModel::ReadingListDidUpdateEntry( } void DualReadingListModel::ReadingListDidApplyChanges(ReadingListModel* model) { - if (!suppress_observer_notifications_) { - NotifyObserversWithDidApplyChanges(); + if (!loaded() || suppress_observer_notifications_) { + return; } + NotifyObserversWithDidApplyChanges(); } DualReadingListModel::StorageStateForTesting diff --git a/components/reading_list/core/dual_reading_list_model_unittest.cc b/components/reading_list/core/dual_reading_list_model_unittest.cc index 95dd565b203b5..03eb3826d0fab 100644 --- a/components/reading_list/core/dual_reading_list_model_unittest.cc +++ b/components/reading_list/core/dual_reading_list_model_unittest.cc @@ -190,6 +190,24 @@ class DualReadingListModelTest : public testing::Test { /*initial_account_entries_builders=*/{}); } + bool TriggerAccountStorageLoadCompletionSignedInSyncDisabled( + std::vector initial_account_entries_builders = {}) { + auto metadata_batch = std::make_unique(); + sync_pb::ModelTypeState state; + state.set_initial_sync_state( + sync_pb::ModelTypeState_InitialSyncState_INITIAL_SYNC_DONE); + state.set_authenticated_account_id(kTestAccountId); + metadata_batch->SetModelTypeState(state); + + std::vector> initial_account_entries; + for (auto entry_builder : initial_account_entries_builders) { + initial_account_entries.push_back(entry_builder.Build()); + } + + return account_model_storage_ptr_->TriggerLoadCompletion( + std::move(initial_account_entries), std::move(metadata_batch)); + } + bool TriggerStorageLoadCompletionSignedInSyncDisabled( std::vector initial_local_entries_builders = {}, std::vector initial_account_entries_builders = {}) { @@ -304,6 +322,25 @@ TEST_F(DualReadingListModelTest, MetaDataClearedBeforeModelLoaded) { EXPECT_EQ(0ul, dual_model_->size()); } +TEST_F(DualReadingListModelTest, UpdatesFromSyncBeforeTheLocalModelIsLoaded) { + ResetStorage(); + TriggerAccountStorageLoadCompletionSignedInSyncDisabled( + /*initial_account_entries_builders=*/{ + TestEntryBuilder(kUrl, clock_.Now())}); + + EXPECT_CALL(observer_, ReadingListWillRemoveEntry).Times(0); + EXPECT_CALL(observer_, ReadingListDidRemoveEntry).Times(0); + EXPECT_CALL(observer_, ReadingListWillAddEntry).Times(0); + EXPECT_CALL(observer_, ReadingListDidAddEntry).Times(0); + EXPECT_CALL(observer_, ReadingListDidApplyChanges).Times(0); + + // DCHECKs verify that sync updates are issued as batch updates. + auto token = account_model_ptr_->BeginBatchUpdates(); + account_model_ptr_->SyncRemoveEntry(kUrl); + account_model_ptr_->AddEntry(TestEntryBuilder(kUrl, clock_.Now()).Build(), + reading_list::ADDED_VIA_SYNC); +} + TEST_F(DualReadingListModelTest, ReturnAccountModelSize) { ASSERT_TRUE(ResetStorageAndTriggerLoadCompletion( /*initial_local_or_syncable_entries_builders=*/{},