Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1691013002:
[Sync] USS: Implement SimpleMetadataChangeList. (Closed)
|
Created:
4 years, 10 months ago by maxbogue Modified:
4 years, 10 months ago CC:
chromium-reviews, tim+watch_chromium.org, maxbogue+watch_chromium.org, plaree+watch_chromium.org, zea+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@smtp3 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Sync] USS: Implement SimpleMetadataChangeList.
BUG=
Committed: https://crrev.com/8e0e9fe76dd49a8aa6d2db410e485a4265cc0d22
Cr-Commit-Position: refs/heads/master@{#375978}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Use nifty initializer syntax. #Patch Set 3 : Even more init syntax. #
Total comments: 1
Messages
Total messages: 27 (11 generated)
maxbogue@chromium.org changed reviewers: + skym@chromium.org
Sky, here's what I have so far. As I said, I'm not sure whether this should be the same class as the MCL made for Store interactions, but I think for now it's fine.
On 2016/02/11 17:33:13, maxbogue wrote: > Sky, here's what I have so far. As I said, I'm not sure whether this should be > the same class as the MCL made for Store interactions, but I think for now it's > fine. Yeah, the word "Simple" doesn't really translate to 'knows how to put changes into a ModelTypeStore' either.
maxbogue@chromium.org changed reviewers: + gangwu@chromium.org, pavely@chromium.org
maxbogue@chromium.org changed required reviewers: + pavely@chromium.org, skym@chromium.org
Pavel, PTAL as well. +cc Gang I think after this someone (Sky?) should split the TransferChanges method out into a BatchMetadataChangeList or something. Simple* usually implies the most basic possible implementation, which is what this is.
https://codereview.chromium.org/1691013002/diff/1/sync/internal_api/public/si... File sync/internal_api/public/simple_metadata_change_list.h (right): https://codereview.chromium.org/1691013002/diff/1/sync/internal_api/public/si... sync/internal_api/public/simple_metadata_change_list.h:20: // A MetadataChangeList implementation that is meant to be used in combination I'm leaving this comment as-is for now until we split out the batch stuff.
https://codereview.chromium.org/1691013002/diff/1/sync/internal_api/public/si... File sync/internal_api/public/simple_metadata_change_list.cc (right): https://codereview.chromium.org/1691013002/diff/1/sync/internal_api/public/si... sync/internal_api/public/simple_metadata_change_list.cc:43: return state_change_.get() != nullptr; When state_change_->type == CLEAR(set at line 22), it consider as has change?
https://codereview.chromium.org/1691013002/diff/1/sync/internal_api/public/si... File sync/internal_api/public/simple_metadata_change_list.cc (right): https://codereview.chromium.org/1691013002/diff/1/sync/internal_api/public/si... sync/internal_api/public/simple_metadata_change_list.cc:43: return state_change_.get() != nullptr; On 2016/02/12 18:36:08, Gang Wu wrote: > When state_change_->type == CLEAR(set at line 22), it consider as has change? Yes. If neither UpdateDataTypeState or ClearDataTypeState is called, there is no change to its state. if Clear* is called, its state should be cleared.
lgtm
lgtm % comments https://codereview.chromium.org/1691013002/diff/1/sync/internal_api/public/si... File sync/internal_api/public/simple_metadata_change_list.cc (right): https://codereview.chromium.org/1691013002/diff/1/sync/internal_api/public/si... sync/internal_api/public/simple_metadata_change_list.cc:15: state_change_.reset(new DataTypeStateChange()); I think you can use (new DataTypeStateChange{UPDATE, data_type_state}) here. https://codereview.chromium.org/1691013002/diff/1/sync/internal_api/public/si... File sync/internal_api/public/simple_metadata_change_list.h (right): https://codereview.chromium.org/1691013002/diff/1/sync/internal_api/public/si... sync/internal_api/public/simple_metadata_change_list.h:49: const MetadataChanges& GetMetadataChanges() const; Do you plan to call these functions from outside of TransferChanges()? If not then they should be private.
https://codereview.chromium.org/1691013002/diff/1/sync/internal_api/public/si... File sync/internal_api/public/simple_metadata_change_list.cc (right): https://codereview.chromium.org/1691013002/diff/1/sync/internal_api/public/si... sync/internal_api/public/simple_metadata_change_list.cc:15: state_change_.reset(new DataTypeStateChange()); On 2016/02/17 00:56:40, pavely wrote: > I think you can use (new DataTypeStateChange{UPDATE, data_type_state}) here. Done, and below! Thanks. https://codereview.chromium.org/1691013002/diff/1/sync/internal_api/public/si... File sync/internal_api/public/simple_metadata_change_list.h (right): https://codereview.chromium.org/1691013002/diff/1/sync/internal_api/public/si... sync/internal_api/public/simple_metadata_change_list.h:49: const MetadataChanges& GetMetadataChanges() const; On 2016/02/17 00:56:40, pavely wrote: > Do you plan to call these functions from outside of TransferChanges()? If not > then they should be private. They're gonna be called in tests, yes. TransferChanges is gonna be in its own class.
The CQ bit was checked by maxbogue@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gangwu@chromium.org, pavely@chromium.org Link to the patchset: https://codereview.chromium.org/1691013002/#ps40001 (title: "Even more init syntax.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1691013002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1691013002/40001
The CQ bit was unchecked by commit-bot@chromium.org
All required reviewers (with asterisk prefixes) have not yet approved this CL. No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Description was changed from ========== [Sync] USS: Implement SimpleMetadataChangeList. BUG= ========== to ========== [Sync] USS: Implement SimpleMetadataChangeList. BUG= ==========
maxbogue@chromium.org changed required reviewers: - skym@chromium.org
The CQ bit was checked by maxbogue@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1691013002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1691013002/40001
Message was sent while issue was closed.
Description was changed from ========== [Sync] USS: Implement SimpleMetadataChangeList. BUG= ========== to ========== [Sync] USS: Implement SimpleMetadataChangeList. BUG= ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Sync] USS: Implement SimpleMetadataChangeList. BUG= ========== to ========== [Sync] USS: Implement SimpleMetadataChangeList. BUG= Committed: https://crrev.com/8e0e9fe76dd49a8aa6d2db410e485a4265cc0d22 Cr-Commit-Position: refs/heads/master@{#375978} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8e0e9fe76dd49a8aa6d2db410e485a4265cc0d22 Cr-Commit-Position: refs/heads/master@{#375978}
Message was sent while issue was closed.
I guess I'm a little bit late at this point. https://codereview.chromium.org/1691013002/diff/1/sync/internal_api/public/si... File sync/internal_api/public/simple_metadata_change_list.cc (right): https://codereview.chromium.org/1691013002/diff/1/sync/internal_api/public/si... sync/internal_api/public/simple_metadata_change_list.cc:15: state_change_.reset(new DataTypeStateChange()); On 2016/02/17 00:56:40, pavely wrote: > I think you can use (new DataTypeStateChange{UPDATE, data_type_state}) here. ..This is uniform initialization syntax right? Currently banned. http://chromium-cpp.appspot.com/ > Dangerous without library support, see thread. Reevaulate once we have C++11 library support. https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GF96FshwHLw https://codereview.chromium.org/1691013002/diff/40001/sync/internal_api/publi... File sync/internal_api/public/simple_metadata_change_list.h (right): https://codereview.chromium.org/1691013002/diff/40001/sync/internal_api/publi... sync/internal_api/public/simple_metadata_change_list.h:14: #include "sync/internal_api/public/non_blocking_sync_common.h" Don't think you need this anymore.
Message was sent while issue was closed.
I was wrong! https://codereview.chromium.org/1691013002/diff/1/sync/internal_api/public/si... File sync/internal_api/public/simple_metadata_change_list.cc (right): https://codereview.chromium.org/1691013002/diff/1/sync/internal_api/public/si... sync/internal_api/public/simple_metadata_change_list.cc:15: state_change_.reset(new DataTypeStateChange()); On 2016/02/18 16:11:10, skym wrote: > On 2016/02/17 00:56:40, pavely wrote: > > I think you can use (new DataTypeStateChange{UPDATE, data_type_state}) here. > > ..This is uniform initialization syntax right? Currently banned. > > http://chromium-cpp.appspot.com/ > > > Dangerous without library support, see thread. Reevaulate once we have C++11 > library support. > > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GF96FshwHLw As Max pointed out to me, uniform initialization syntax is applying this style to classes, but this is a struct. Initializing structs this way has been a thing for a very long time. So, disregard my earlier concerns. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
