Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2794413002:
[USS] Add SyncMetadataStore interface (Closed)
|
Created:
3 years, 8 months ago by Gang Wu Modified:
3 years, 8 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, sync-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UpdateSyncMetadata, ClearSyncMetadata, UpdateModelTypeState,
ClearModelTypeState into SyncMetadataStore interface.
Let AutofillTable inherit SyncMetadataStore.
Also create a SyncMetadataStoreChangeList for autofill.
Will do this change for typed url in next CL.
BUG=708275
Review-Url: https://codereview.chromium.org/2794413002
Cr-Commit-Position: refs/heads/master@{#466568}
Committed: https://chromium.googlesource.com/chromium/src/+/47d7aae76735286b86b85a3c582870677fce05d2
Patch Set 1 #
Total comments: 15
Patch Set 2 : update names #Messages
Total messages: 38 (26 generated)
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== fist try BUG= ========== to ========== Add UpdateSyncMetadata, ClearSyncMetadata, UpdateModelTypeState, ClearModelTypeState into MetadataTable interface. Let AutofillTable inherit MetadataTable. Also create an OnDiskMetadataChangeList for autofill. Will do this change for typed url in next CL. BUG=708275 ==========
gangwu@chromium.org changed reviewers: + pavely@chromium.org
Description was changed from ========== Add UpdateSyncMetadata, ClearSyncMetadata, UpdateModelTypeState, ClearModelTypeState into MetadataTable interface. Let AutofillTable inherit MetadataTable. Also create an OnDiskMetadataChangeList for autofill. Will do this change for typed url in next CL. BUG=708275 ========== to ========== Add UpdateSyncMetadata, ClearSyncMetadata, UpdateModelTypeState, ClearModelTypeState into MetadataTable interface. Let AutofillTable inherit MetadataTable. Also create an OnDiskMetadataChangeList for autofill. Will do this change for typed url in next CL. BUG=708275 ==========
PTAL
https://codereview.chromium.org/2794413002/diff/1/components/autofill/core/br... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2794413002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:221: ->TakeError(); If you decide to keep TakeError in MetadataChangeList then static_cast is not needed anymore. https://codereview.chromium.org/2794413002/diff/1/components/sync/model/metad... File components/sync/model/metadata_table.h (right): https://codereview.chromium.org/2794413002/diff/1/components/sync/model/metad... components/sync/model/metadata_table.h:19: // Interface used by the MetadataChangeList and database to communicate about This interface will be used by developers outside of sync team. Class comment should explain when would someone need this class and how to use/implement it. For example: MetadataTable defines interface implemented by model types for persisting sync metadata and datatype state. It allows model type to use common implementation of MetadataChangeList (OnDiskMetadataChangeList) instead of implementing their own. Model type in implementation of ModelTypeSyncBridge::CreateMetadataChangeList should create instance of OnDiskMetadataChangeList passing pointer to MetadataTable to its constructor. Implementations of MetadataTable methods should write updated metadata in model type specific sync metadata storage. https://codereview.chromium.org/2794413002/diff/1/components/sync/model/metad... components/sync/model/metadata_table.h:23: class MetadataTable { Word "Table" in MetadataTable was probably inspired by AutofillTable hierarchy of classes. When this interface is used with other types (Typed URL) Table might look out of place. This class defines interface for storing sync metadata in any storage (doesn't have to be table or sql). I think the name should reflect that it concerns sync metadata (there could be other metadata in model type's implementation) and shouldn't be restricted to tables. Here are some potential names: SyncMetadataStore SyncMetadataWriter SyncMetadataProvider https://codereview.chromium.org/2794413002/diff/1/components/sync/model/metad... components/sync/model/metadata_table.h:30: virtual bool UpdateSyncMetadata(syncer::ModelType model_type, Could you add description of return value for all these functions. https://codereview.chromium.org/2794413002/diff/1/components/sync/model_impl/... File components/sync/model_impl/in_memory_metadata_change_list.cc (right): https://codereview.chromium.org/2794413002/diff/1/components/sync/model_impl/... components/sync/model_impl/in_memory_metadata_change_list.cc:32: NOTREACHED(); I don't think we should assert NOTREACHED() here. TakeError should either be restricted to only OnDiskMCL and therefore shouldn't be present in MetadataChangeList and its derived classes, or it should be allowed to call on any implementation of MetadataChangeList. https://codereview.chromium.org/2794413002/diff/1/components/sync/model_impl/... File components/sync/model_impl/on_disk_metadata_change_list.h (right): https://codereview.chromium.org/2794413002/diff/1/components/sync/model_impl/... components/sync/model_impl/on_disk_metadata_change_list.h:24: class OnDiskMetadataChangeList : public MetadataChangeList { Both OnDiskMetadataChangeList and PassthroughMetadataChangeList persist metadata on disk which makes their names not descriptive of their purpose. Both these classes are really backed by other objects that perform IO operations (MetadataTable and ModelTypeStore). I think names of these classes should reflect that. This one for example could be MetadataTableChangeList.
skym@chromium.org changed reviewers: + skym@chromium.org
https://codereview.chromium.org/2794413002/diff/1/components/sync/model/metad... File components/sync/model/metadata_change_list.h (right): https://codereview.chromium.org/2794413002/diff/1/components/sync/model/metad... components/sync/model/metadata_change_list.h:51: virtual base::Optional<syncer::ModelError> TakeError() = 0; What was the decision yesterday about returning errors from MCLs? Is it just that we'll talk about it next week again?
https://codereview.chromium.org/2794413002/diff/1/components/sync/model/metad... File components/sync/model/metadata_change_list.h (right): https://codereview.chromium.org/2794413002/diff/1/components/sync/model/metad... components/sync/model/metadata_change_list.h:51: virtual base::Optional<syncer::ModelError> TakeError() = 0; On 2017/04/06 23:01:58, skym wrote: > What was the decision yesterday about returning errors from MCLs? Is it just > that we'll talk about it next week again? The decision was that we don't change the pattern in this CL, instead consider this as a separate change. If we decide to return errors from individual functions I think we'll need to refactor SMTP's functions to handle these errors.
skym@chromium.org changed reviewers: - skym@chromium.org
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
Patchset #2 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
name updated https://codereview.chromium.org/2794413002/diff/1/components/autofill/core/br... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2794413002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:221: ->TakeError(); On 2017/04/06 22:56:42, pavely wrote: > If you decide to keep TakeError in MetadataChangeList then static_cast is not > needed anymore. Done. https://codereview.chromium.org/2794413002/diff/1/components/sync/model/metad... File components/sync/model/metadata_change_list.h (right): https://codereview.chromium.org/2794413002/diff/1/components/sync/model/metad... components/sync/model/metadata_change_list.h:51: virtual base::Optional<syncer::ModelError> TakeError() = 0; On 2017/04/06 23:36:02, pavely wrote: > On 2017/04/06 23:01:58, skym wrote: > > What was the decision yesterday about returning errors from MCLs? Is it just > > that we'll talk about it next week again? > > The decision was that we don't change the pattern in this CL, instead consider > this as a separate change. If we decide to return errors from individual > functions I think we'll need to refactor SMTP's functions to handle these > errors. Done. https://codereview.chromium.org/2794413002/diff/1/components/sync/model/metad... File components/sync/model/metadata_table.h (right): https://codereview.chromium.org/2794413002/diff/1/components/sync/model/metad... components/sync/model/metadata_table.h:19: // Interface used by the MetadataChangeList and database to communicate about On 2017/04/06 22:56:42, pavely wrote: > This interface will be used by developers outside of sync team. Class comment > should explain when would someone need this class and how to use/implement it. > For example: > > MetadataTable defines interface implemented by model types for persisting sync > metadata and datatype state. It allows model type to use common implementation > of MetadataChangeList (OnDiskMetadataChangeList) instead of implementing their > own. > > Model type in implementation of ModelTypeSyncBridge::CreateMetadataChangeList > should create instance of OnDiskMetadataChangeList passing pointer to > MetadataTable to its constructor. > > Implementations of MetadataTable methods should write updated metadata in model > type specific sync metadata storage. Done. https://codereview.chromium.org/2794413002/diff/1/components/sync/model/metad... components/sync/model/metadata_table.h:23: class MetadataTable { On 2017/04/06 22:56:42, pavely wrote: > Word "Table" in MetadataTable was probably inspired by AutofillTable hierarchy > of classes. When this interface is used with other types (Typed URL) Table might > look out of place. This class defines interface for storing sync metadata in any > storage (doesn't have to be table or sql). I think the name should reflect that > it concerns sync metadata (there could be other metadata in model type's > implementation) and shouldn't be restricted to tables. Here are some potential > names: > SyncMetadataStore > SyncMetadataWriter > SyncMetadataProvider Done. https://codereview.chromium.org/2794413002/diff/1/components/sync/model/metad... components/sync/model/metadata_table.h:30: virtual bool UpdateSyncMetadata(syncer::ModelType model_type, On 2017/04/06 22:56:42, pavely wrote: > Could you add description of return value for all these functions. Done. https://codereview.chromium.org/2794413002/diff/1/components/sync/model_impl/... File components/sync/model_impl/in_memory_metadata_change_list.cc (right): https://codereview.chromium.org/2794413002/diff/1/components/sync/model_impl/... components/sync/model_impl/in_memory_metadata_change_list.cc:32: NOTREACHED(); On 2017/04/06 22:56:42, pavely wrote: > I don't think we should assert NOTREACHED() here. > TakeError should either be restricted to only OnDiskMCL and therefore shouldn't > be present in MetadataChangeList and its derived classes, or it should be > allowed to call on any implementation of MetadataChangeList. Done. https://codereview.chromium.org/2794413002/diff/1/components/sync/model_impl/... File components/sync/model_impl/on_disk_metadata_change_list.h (right): https://codereview.chromium.org/2794413002/diff/1/components/sync/model_impl/... components/sync/model_impl/on_disk_metadata_change_list.h:24: class OnDiskMetadataChangeList : public MetadataChangeList { On 2017/04/06 22:56:42, pavely wrote: > Both OnDiskMetadataChangeList and PassthroughMetadataChangeList persist metadata > on disk which makes their names not descriptive of their purpose. Both these > classes are really backed by other objects that perform IO operations > (MetadataTable and ModelTypeStore). I think names of these classes should > reflect that. > This one for example could be MetadataTableChangeList. Done.
Description was changed from ========== Add UpdateSyncMetadata, ClearSyncMetadata, UpdateModelTypeState, ClearModelTypeState into MetadataTable interface. Let AutofillTable inherit MetadataTable. Also create an OnDiskMetadataChangeList for autofill. Will do this change for typed url in next CL. BUG=708275 ========== to ========== Add UpdateSyncMetadata, ClearSyncMetadata, UpdateModelTypeState, ClearModelTypeState into SyncMetadataStore interface. Let AutofillTable inherit SyncMetadataStore. Also create a SyncMetadataStoreChangeList for autofill. Will do this change for typed url in next CL. BUG=708275 ==========
lgtm
The CQ bit was checked by gangwu@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
gangwu@chromium.org changed reviewers: + mathp@chromium.org
hi mathp@, PTAL
autofill lgtm
The CQ bit was checked by gangwu@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492972463868640,
"parent_rev": "81c2c04d0c269a02e9dd86803de226f219f93615", "commit_rev":
"47d7aae76735286b86b85a3c582870677fce05d2"}
Message was sent while issue was closed.
Description was changed from ========== Add UpdateSyncMetadata, ClearSyncMetadata, UpdateModelTypeState, ClearModelTypeState into SyncMetadataStore interface. Let AutofillTable inherit SyncMetadataStore. Also create a SyncMetadataStoreChangeList for autofill. Will do this change for typed url in next CL. BUG=708275 ========== to ========== Add UpdateSyncMetadata, ClearSyncMetadata, UpdateModelTypeState, ClearModelTypeState into SyncMetadataStore interface. Let AutofillTable inherit SyncMetadataStore. Also create a SyncMetadataStoreChangeList for autofill. Will do this change for typed url in next CL. BUG=708275 Review-Url: https://codereview.chromium.org/2794413002 Cr-Commit-Position: refs/heads/master@{#466568} Committed: https://chromium.googlesource.com/chromium/src/+/47d7aae76735286b86b85a3c5828... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001) as https://chromium.googlesource.com/chromium/src/+/47d7aae76735286b86b85a3c5828... |
