Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2437873006:
[Sync] Removed ScopdVector and raw news from ModelTypeRegistry. (Closed)
Description[Sync] Removed ScopdVector and raw news from ModelTypeRegistry.
BUG=658053
Committed: https://crrev.com/6754203e3e27c476757265c0bd35a823258aa7bf
Cr-Commit-Position: refs/heads/master@{#426856}
Patch Set 1 #Patch Set 2 : Moving DirectoryTypeDebugInfoEmitterMap typedef to be private. #
Total comments: 32
Patch Set 3 : Updates for Max. #Patch Set 4 : And removed the useless wrapping scoped_refptr. #
Messages
Total messages: 20 (11 generated)
The CQ bit was checked by skym@chromium.org to run a CQ dry run
skym@chromium.org changed reviewers: + maxbogue@chromium.org
PTAL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... File components/sync/engine_impl/get_updates_delegate.cc (right): https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/get_updates_delegate.cc:19: UpdateHandlerMap* update_handler_map) { Why are these passed in as pointers instead of const ref? Wouldn't that work? https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... File components/sync/engine_impl/model_type_registry.cc (right): https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:104: std::unique_ptr<DirectoryUpdateHandler> updater = auto https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:108: update_handler_map_.insert(std::make_pair(type, updater.get())).second) You can't do real things inside a DCHECK! They are compiled out in release builds. Please return these to the way they were before. Also it sounds like we shouldn't really care if we're doing this? https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:112: std::unique_ptr<DirectoryCommitContributor> committer = auto https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:138: std::unique_ptr<ModelTypeWorker> worker = base::MakeUnique<ModelTypeWorker>( auto https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:143: std::unique_ptr<CommitQueue> commit_queue_proxy = I think auto is even fine here; it'll just be downcast where it's used. https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:145: worker->AsWeakPtr(), scoped_refptr<base::SequencedTaskRunner>( Uuuuuh this wasn't you but I'm pretty sure we don't need to wrap base::ThreadTaskRunnerHandle::Get() with scoped_refptr<base::SequencedTaskRunner>(), can that be removed? https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:174: model_type_workers_.erase(std::remove_if( Since there will ever only be one thing you're removing here, can't you do foo.erase(foo.find(type)) or something like that? https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:242: for (const std::unique_ptr<ModelTypeWorker>& worker : model_type_workers_) { I actually think const auto& would be clearer here, because this way it almost looks like you're getting ownership... https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:289: DirectoryTypeDebugInfoEmitter* raw_emitter = nullptr; Do you even need to initialize this if it's getting set in both branches below? I have no idea what the best practice is here. https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:290: DirectoryTypeDebugInfoEmitterMap::iterator it = auto it https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:295: std::unique_ptr<DirectoryTypeDebugInfoEmitter> emitter = auto https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:311: for (const std::unique_ptr<ModelTypeWorker>& worker : model_type_workers_) { const auto&
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... File components/sync/engine_impl/model_type_registry.cc (right): https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:174: model_type_workers_.erase(std::remove_if( On 2016/10/20 23:36:29, maxbogue wrote: > Since there will ever only be one thing you're removing here, can't you do > foo.erase(foo.find(type)) or something like that? Wow never mind please ignore this, for some reason I thought it was a map for a sec.
The CQ bit was checked by skym@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...
https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... File components/sync/engine_impl/get_updates_delegate.cc (right): https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/get_updates_delegate.cc:19: UpdateHandlerMap* update_handler_map) { On 2016/10/20 23:36:28, maxbogue wrote: > Why are these passed in as pointers instead of const ref? Wouldn't that work? They totally could be passed as const refs, in fact everything between the Registry and these code could use const refs instead of raw pointers. But ApplyUpdates (and GetContribution) are not const methods. My C++ const correctness understanding is a bit shaky if this is the right thing to do or not. I could be convinced to do this in a follow up CL. Ultimately though I think the correct solution is to pass an interface around, and not strongly typed maps. So I reverted most of my changes in this area. I think ideally the registry would only have a single data structure for UpdateHandlers and CommitContributors. And it would contain unique_ptrs. And it would not be exposed as per https://www.chromium.org/developers/smart-pointer-guidelines https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... File components/sync/engine_impl/model_type_registry.cc (right): https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:104: std::unique_ptr<DirectoryUpdateHandler> updater = On 2016/10/20 23:36:29, maxbogue wrote: > auto I don't know why I cannot seem to remember to do this. Maybe this will be the time it sticks. Done. https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:108: update_handler_map_.insert(std::make_pair(type, updater.get())).second) On 2016/10/20 23:36:29, maxbogue wrote: > You can't do real things inside a DCHECK! They are compiled out in release > builds. Please return these to the way they were before. Also it sounds like we > shouldn't really care if we're doing this? Doh! I don't know what I was thinking :(. Thanks for catching this. Why do you say it sounds like we shouldn't really care? I would love to remove these DCHECKs. https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:112: std::unique_ptr<DirectoryCommitContributor> committer = On 2016/10/20 23:36:29, maxbogue wrote: > auto Done. https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:138: std::unique_ptr<ModelTypeWorker> worker = base::MakeUnique<ModelTypeWorker>( On 2016/10/20 23:36:29, maxbogue wrote: > auto Done. https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:143: std::unique_ptr<CommitQueue> commit_queue_proxy = On 2016/10/20 23:36:29, maxbogue wrote: > I think auto is even fine here; it'll just be downcast where it's used. Done. https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:145: worker->AsWeakPtr(), scoped_refptr<base::SequencedTaskRunner>( On 2016/10/20 23:36:29, maxbogue wrote: > Uuuuuh this wasn't you but I'm pretty sure we don't need to wrap > base::ThreadTaskRunnerHandle::Get() with > scoped_refptr<base::SequencedTaskRunner>(), can that be removed? Done. https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:174: model_type_workers_.erase(std::remove_if( On 2016/10/21 00:40:29, maxbogue wrote: > On 2016/10/20 23:36:29, maxbogue wrote: > > Since there will ever only be one thing you're removing here, can't you do > > foo.erase(foo.find(type)) or something like that? > > Wow never mind please ignore this, for some reason I thought it was a map for a > sec. Acknowledged. https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:242: for (const std::unique_ptr<ModelTypeWorker>& worker : model_type_workers_) { On 2016/10/20 23:36:29, maxbogue wrote: > I actually think const auto& would be clearer here, because this way it almost > looks like you're getting ownership... Done. https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:279: for (const std::unique_ptr<ModelTypeWorker>& worker : model_type_workers_) { Added auto as well. https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:289: DirectoryTypeDebugInfoEmitter* raw_emitter = nullptr; On 2016/10/20 23:36:29, maxbogue wrote: > Do you even need to initialize this if it's getting set in both branches below? > I have no idea what the best practice is here. I cannot find any guidance on what is best. Staying with the safe option. https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:290: DirectoryTypeDebugInfoEmitterMap::iterator it = On 2016/10/20 23:36:29, maxbogue wrote: > auto it Done. https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:295: std::unique_ptr<DirectoryTypeDebugInfoEmitter> emitter = On 2016/10/20 23:36:29, maxbogue wrote: > auto Done. https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:311: for (const std::unique_ptr<ModelTypeWorker>& worker : model_type_workers_) { On 2016/10/20 23:36:29, maxbogue wrote: > const auto& Done.
lgtm! https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... File components/sync/engine_impl/get_updates_delegate.cc (right): https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/get_updates_delegate.cc:19: UpdateHandlerMap* update_handler_map) { On 2016/10/21 16:15:28, skym wrote: > On 2016/10/20 23:36:28, maxbogue wrote: > > Why are these passed in as pointers instead of const ref? Wouldn't that work? > > They totally could be passed as const refs, in fact everything between the > Registry and these code could use const refs instead of raw pointers. But > ApplyUpdates (and GetContribution) are not const methods. My C++ const > correctness understanding is a bit shaky if this is the right thing to do or > not. I could be convinced to do this in a follow up CL. > > Ultimately though I think the correct solution is to pass an interface around, > and not strongly typed maps. So I reverted most of my changes in this area. I > think ideally the registry would only have a single data structure for > UpdateHandlers and CommitContributors. And it would contain unique_ptrs. And it > would not be exposed as per > https://www.chromium.org/developers/smart-pointer-guidelines Sounds better. https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... File components/sync/engine_impl/model_type_registry.cc (right): https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:108: update_handler_map_.insert(std::make_pair(type, updater.get())).second) On 2016/10/21 16:15:28, skym wrote: > On 2016/10/20 23:36:29, maxbogue wrote: > > You can't do real things inside a DCHECK! They are compiled out in release > > builds. Please return these to the way they were before. Also it sounds like > we > > shouldn't really care if we're doing this? > > Doh! I don't know what I was thinking :(. Thanks for catching this. > > Why do you say it sounds like we shouldn't really care? I would love to remove > these DCHECKs. ...I don't know why I said that. I think I got confused and wrote that before I saw the DCHECKs below and forgot to update teh comment -_-
https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... File components/sync/engine_impl/get_updates_delegate.cc (right): https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/get_updates_delegate.cc:19: UpdateHandlerMap* update_handler_map) { On 2016/10/21 17:33:45, maxbogue wrote: > On 2016/10/21 16:15:28, skym wrote: > > On 2016/10/20 23:36:28, maxbogue wrote: > > > Why are these passed in as pointers instead of const ref? Wouldn't that > work? > > > > They totally could be passed as const refs, in fact everything between the > > Registry and these code could use const refs instead of raw pointers. But > > ApplyUpdates (and GetContribution) are not const methods. My C++ const > > correctness understanding is a bit shaky if this is the right thing to do or > > not. I could be convinced to do this in a follow up CL. > > > > Ultimately though I think the correct solution is to pass an interface around, > > and not strongly typed maps. So I reverted most of my changes in this area. I > > think ideally the registry would only have a single data structure for > > UpdateHandlers and CommitContributors. And it would contain unique_ptrs. And > it > > would not be exposed as per > > https://www.chromium.org/developers/smart-pointer-guidelines > > Sounds better. Acknowledged. https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... File components/sync/engine_impl/model_type_registry.cc (right): https://codereview.chromium.org/2437873006/diff/20001/components/sync/engine_... components/sync/engine_impl/model_type_registry.cc:108: update_handler_map_.insert(std::make_pair(type, updater.get())).second) On 2016/10/21 17:33:45, maxbogue wrote: > On 2016/10/21 16:15:28, skym wrote: > > On 2016/10/20 23:36:29, maxbogue wrote: > > > You can't do real things inside a DCHECK! They are compiled out in release > > > builds. Please return these to the way they were before. Also it sounds like > > we > > > shouldn't really care if we're doing this? > > > > Doh! I don't know what I was thinking :(. Thanks for catching this. > > > > Why do you say it sounds like we shouldn't really care? I would love to remove > > these DCHECKs. > > ...I don't know why I said that. I think I got confused and wrote that before I > saw the DCHECKs below and forgot to update teh comment -_- Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by skym@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Sync] Removed ScopdVector and raw news from ModelTypeRegistry. BUG=658053 ========== to ========== [Sync] Removed ScopdVector and raw news from ModelTypeRegistry. BUG=658053 Committed: https://crrev.com/6754203e3e27c476757265c0bd35a823258aa7bf Cr-Commit-Position: refs/heads/master@{#426856} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/6754203e3e27c476757265c0bd35a823258aa7bf Cr-Commit-Position: refs/heads/master@{#426856} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
