New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 860147 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 16
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 3
Type: Task



Sign in to add a comment

Remove base::SupportsWeakPtr from ModelTypeSyncBridge

Project Member Reported by jkrcal@chromium.org, Jul 4

Issue description

This should not be needed (much) now after the bridge is not the delegate for ModelTypeController any more.

See the TODO at https://cs.chromium.org/chromium/src/components/sync/model/model_type_sync_bridge.h?l=35
 
Labels: sync-fixit-2018q3
Labels: -Type-Bug OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows Type-Task
Owner: treib@chromium.org
Status: Started (was: Available)
Taking a look. It seems that most ModelTypeSyncBridge subclasses do still require WeakPtr support though, so we'll see...
Prototype CL: https://crrev.com/c/1131177

The following bridge implementation do still require WeakPtr support:
ConsentSyncBridgeImpl
ReadingListStore
SessionSyncBridge
DeviceInfoSyncBridge
UserEventSyncBridge

Overall, I'm not entirely sure this is a worthwhile change:
Many (though not all) do still require WeakPtrs, but we could use WeakPtrFactory instead of SupportsWeakPtr and thus make it private...
Huh, I did not expect so many bridges to need it themselves :|

I'd lean towards WeakPtrFactories where it makes sense. Feel free to also drop the bug by removing the TODO...
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 16

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6b5064ba933c18760fa2fbb1e55df796bc312dfe

commit 6b5064ba933c18760fa2fbb1e55df796bc312dfe
Author: Marc Treib <treib@chromium.org>
Date: Mon Jul 16 14:17:31 2018

Remove base::SupportsWeakPtr from ModelTypeSyncBridge

Instead, implement WeakPtr support in those subclasses that require it.

Bug:  860147 
Change-Id: I906cb9dff790b1e4873bc5c402a73eabadddae9e
Reviewed-on: https://chromium-review.googlesource.com/1131177
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Olivier Robin <olivierrobin@chromium.org>
Reviewed-by: Markus Heintz <markusheintz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575247}
[modify] https://crrev.com/6b5064ba933c18760fa2fbb1e55df796bc312dfe/components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc
[modify] https://crrev.com/6b5064ba933c18760fa2fbb1e55df796bc312dfe/components/autofill/core/browser/webdata/autocomplete_sync_bridge.h
[modify] https://crrev.com/6b5064ba933c18760fa2fbb1e55df796bc312dfe/components/autofill/core/browser/webdata/autofill_profile_sync_bridge.cc
[modify] https://crrev.com/6b5064ba933c18760fa2fbb1e55df796bc312dfe/components/autofill/core/browser/webdata/autofill_profile_sync_bridge.h
[modify] https://crrev.com/6b5064ba933c18760fa2fbb1e55df796bc312dfe/components/consent_auditor/consent_sync_bridge_impl.cc
[modify] https://crrev.com/6b5064ba933c18760fa2fbb1e55df796bc312dfe/components/consent_auditor/consent_sync_bridge_impl.h
[modify] https://crrev.com/6b5064ba933c18760fa2fbb1e55df796bc312dfe/components/reading_list/core/reading_list_store.cc
[modify] https://crrev.com/6b5064ba933c18760fa2fbb1e55df796bc312dfe/components/reading_list/core/reading_list_store.h
[modify] https://crrev.com/6b5064ba933c18760fa2fbb1e55df796bc312dfe/components/sync/device_info/device_info_sync_bridge.cc
[modify] https://crrev.com/6b5064ba933c18760fa2fbb1e55df796bc312dfe/components/sync/device_info/device_info_sync_bridge.h
[modify] https://crrev.com/6b5064ba933c18760fa2fbb1e55df796bc312dfe/components/sync/model/model_type_sync_bridge.h
[modify] https://crrev.com/6b5064ba933c18760fa2fbb1e55df796bc312dfe/components/sync/user_events/fake_user_event_service.cc
[modify] https://crrev.com/6b5064ba933c18760fa2fbb1e55df796bc312dfe/components/sync/user_events/fake_user_event_service.h
[modify] https://crrev.com/6b5064ba933c18760fa2fbb1e55df796bc312dfe/components/sync/user_events/no_op_user_event_service.cc
[modify] https://crrev.com/6b5064ba933c18760fa2fbb1e55df796bc312dfe/components/sync/user_events/no_op_user_event_service.h
[modify] https://crrev.com/6b5064ba933c18760fa2fbb1e55df796bc312dfe/components/sync/user_events/user_event_service.h
[modify] https://crrev.com/6b5064ba933c18760fa2fbb1e55df796bc312dfe/components/sync/user_events/user_event_service_impl.cc
[modify] https://crrev.com/6b5064ba933c18760fa2fbb1e55df796bc312dfe/components/sync/user_events/user_event_service_impl.h
[modify] https://crrev.com/6b5064ba933c18760fa2fbb1e55df796bc312dfe/components/sync/user_events/user_event_sync_bridge.cc
[modify] https://crrev.com/6b5064ba933c18760fa2fbb1e55df796bc312dfe/components/sync/user_events/user_event_sync_bridge.h
[modify] https://crrev.com/6b5064ba933c18760fa2fbb1e55df796bc312dfe/components/sync_sessions/session_sync_bridge.cc
[modify] https://crrev.com/6b5064ba933c18760fa2fbb1e55df796bc312dfe/components/sync_sessions/session_sync_bridge.h

Status: Fixed (was: Started)

Sign in to add a comment