Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2077713002:
[USS] Store supports hosting multiple datatypes per database (Closed)
DescriptionThis is 2 of 2 checks in for making ModelTypeStore supports
multiple datatypes.
This check-in let ModelTypeStoreBackend can be shared with
multiple ModelTypeStore.
BUG=615214
Committed: https://crrev.com/71bdb554039faa51622aa87ebdb3b7eeae2dedf0
Cr-Commit-Position: refs/heads/master@{#403962}
Patch Set 1 : #Patch Set 2 : can create multiple backend base on path #
Total comments: 10
Patch Set 3 : change the way to create backend #
Total comments: 24
Patch Set 4 : Update base on pavel's comments #
Total comments: 22
Patch Set 5 : remove backend if init failed #
Total comments: 4
Patch Set 6 #
Total comments: 2
Patch Set 7 : remove includes #
Messages
Total messages: 29 (13 generated)
Patchset #3 (id:40001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:2) has been deleted
Description was changed from ========== first checkin BUG= ========== to ========== This is 2 of 2 checks in for making ModelTypeStore supports multiple datatypes. This check-in let ModelTypeStoreBackend can be shared with multiple ModelTypeStore. BUG=615214 ==========
gangwu@chromium.org changed reviewers: + maxbogue@chromium.org, pavely@chromium.org
PTAL
This is a good start. I have a couple of suggestions: 1.ModelTypeStoreBackend::GetOrCreateBackend creates backend on the thread it is executed therefore backend_map_ needs to be protected by lock. It is possible to get back reference to backend that didn’t run through Init yet. To compensate for that you added another error code and store init result as member of the backend. I suggest to only make backend available to consumers when it was fully initialized. Here is how it can be achieved. - ModelTypeStoreImpl::CreateStore doesn’t create store and backend, but instead posts to DB thread to GetOrCreateBackend - Once on DB thread, create backend, initialize it and insert into backend_map_. If backend is already in backend_map_ then it has already been initialized. - Once backend is retrieved from backend_map_ (or created and initialized), GetOrCreateBackend returns scoped_refptr to it along with initialization result which gets posted to originating thread to ModelTypeStoreImpl::BackendInitDone. - ModelTypeStoreImpl::BackendInitDone creates store and passes backend received through parameter. This approach doesn’t need lock (backend_map_ is only accessed from DB thread) and guarantees that only fully initialized backends make it to backend_map_ (no need to store init result. 2. Backend_map_ stores scoped_refptrs to backends which means that regular refcounting isn’t used to control object lifetime. Instead you need to post Backend::Disconnect which removes backend from map only if it has exactly one reference. Here is what I suggest: - store non-owning references in backend_map_ (raw pointers or WeakPtrs) - When store object is destroyed, post NoOpForBackendDtor to DB thread passing reference to backend. (Like it is done today) - When Backend::dtor is executed remove it from backend_map_ This allows you not to have special disconnect function. What do you think?
Posting the comments I'd made so far, but +1 to everything Pavel said! https://codereview.chromium.org/2077713002/diff/110001/components/sync_driver... File components/sync_driver/device_info_service_unittest.cc (right): https://codereview.chromium.org/2077713002/diff/110001/components/sync_driver... components/sync_driver/device_info_service_unittest.cc:185: void SetUp() override { I believe this should just be in the body of the constructor, and likewise with TearDown in the destructor. https://codereview.chromium.org/2077713002/diff/110001/sync/api/model_type_st... File sync/api/model_type_store.h (right): https://codereview.chromium.org/2077713002/diff/110001/sync/api/model_type_st... sync/api/model_type_store.h:55: UNINITIALIZED, If you switch to having an init callback, this won't be necessary. https://codereview.chromium.org/2077713002/diff/110001/sync/internal_api/publ... File sync/internal_api/public/model_type_store_backend.h (right): https://codereview.chromium.org/2077713002/diff/110001/sync/internal_api/publ... sync/internal_api/public/model_type_store_backend.h:72: ModelTypeStore::Result InitResult() const; It might be cleaner to just pass an init callback into the constructor. https://codereview.chromium.org/2077713002/diff/110001/sync/internal_api/publ... sync/internal_api/public/model_type_store_backend.h:75: friend class base::RefCountedThreadSafe<ModelTypeStoreBackend>; Why is this needed? The base class shouldn't need access into this class. https://codereview.chromium.org/2077713002/diff/110001/sync/internal_api/publ... sync/internal_api/public/model_type_store_backend.h:97: static base::LazyInstance<BackendMap> backend_map_; Can you explain why LazyInstance is necessary? Is it that you can't just instantiate it statically for threading reasons?
Patchset #4 (id:150001) has been deleted
Patchset #3 (id:130001) has been deleted
updated, PTAL https://codereview.chromium.org/2077713002/diff/110001/components/sync_driver... File components/sync_driver/device_info_service_unittest.cc (right): https://codereview.chromium.org/2077713002/diff/110001/components/sync_driver... components/sync_driver/device_info_service_unittest.cc:185: void SetUp() override { On 2016/06/23 18:22:22, maxbogue wrote: > I believe this should just be in the body of the constructor, and likewise with > TearDown in the destructor. Done. https://codereview.chromium.org/2077713002/diff/110001/sync/api/model_type_st... File sync/api/model_type_store.h (right): https://codereview.chromium.org/2077713002/diff/110001/sync/api/model_type_st... sync/api/model_type_store.h:55: UNINITIALIZED, On 2016/06/23 18:22:22, maxbogue wrote: > If you switch to having an init callback, this won't be necessary. Done. https://codereview.chromium.org/2077713002/diff/110001/sync/internal_api/publ... File sync/internal_api/public/model_type_store_backend.h (right): https://codereview.chromium.org/2077713002/diff/110001/sync/internal_api/publ... sync/internal_api/public/model_type_store_backend.h:72: ModelTypeStore::Result InitResult() const; On 2016/06/23 18:22:22, maxbogue wrote: > It might be cleaner to just pass an init callback into the constructor. Now, the callback will return backend object, so this is the easy way to return init result. https://codereview.chromium.org/2077713002/diff/110001/sync/internal_api/publ... sync/internal_api/public/model_type_store_backend.h:75: friend class base::RefCountedThreadSafe<ModelTypeStoreBackend>; On 2016/06/23 18:22:22, maxbogue wrote: > Why is this needed? The base class shouldn't need access into this class. Since dtor is private, this can let scoped_refptr can delete this class. https://codereview.chromium.org/2077713002/diff/110001/sync/internal_api/publ... sync/internal_api/public/model_type_store_backend.h:97: static base::LazyInstance<BackendMap> backend_map_; On 2016/06/23 18:22:22, maxbogue wrote: > Can you explain why LazyInstance is necessary? Is it that you can't just > instantiate it statically for threading reasons? if not, the compiler will say "declaration requires an exit-time destructor", so LazyInstance is the way to avoid it.
https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/mode... File sync/internal_api/model_type_store_backend.cc (right): https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/mode... sync/internal_api/model_type_store_backend.cc:47: scoped_refptr<base::SequencedTaskRunner> blocking_task_runner) { blocking_task_runner is not used. Could you remove it along with corresponding includes? https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/mode... sync/internal_api/model_type_store_backend.cc:48: if (backend_map_.Get().find(path) != backend_map_.Get().end()) { As we discussed, after checking that backend is not in backend_map_ create local variable for backend, create and initialize backend object. Only insert backend into the map and return to the caller if initialization is successful. https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/mode... File sync/internal_api/model_type_store_backend_unittest.cc (right): https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/mode... sync/internal_api/model_type_store_backend_unittest.cc:21: scoped_refptr<ModelTypeStoreBackend> CreateBackend() { Could you rename this function to GetOrCreateBackend. This way at callsite it is clear that not every call will create separate instance of backend. https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/mode... sync/internal_api/model_type_store_backend_unittest.cc:142: TEST_F(ModelTypeStoreBackendTest, TwoBeckendTest) { Here are the tests I would write for this change: - Creating two backends with the same path result in single object (one entry in the map), dropping all references to backend removes it from the map. - Creating two backends with different paths result in different backend objects. https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/mode... sync/internal_api/model_type_store_backend_unittest.cc:147: ASSERT_FALSE(backend_second->HasOneRef()); You don't need to check refcount of backend. It is implementation detail of backends. All you care is that two calls return the same object (the check above). https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/mode... sync/internal_api/model_type_store_backend_unittest.cc:149: backend = 0; backend = nullptr; https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/mode... File sync/internal_api/model_type_store_impl.cc (right): https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/mode... sync/internal_api/model_type_store_impl.cc:128: new ModelTypeStoreImpl(type, backend, blocking_task_runner)); You shouldn't create instance of store if GetOrCreateBackend failed. https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/publ... File sync/internal_api/public/model_type_store_backend.h (right): https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/publ... sync/internal_api/public/model_type_store_backend.h:42: static void Disconnect(scoped_refptr<ModelTypeStoreBackend>& backend); Remove declaration of Disconnect. https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/publ... sync/internal_api/public/model_type_store_backend.h:47: // void TakeEnvOwnership(std::unique_ptr<leveldb::Env> env); Remove TakeEnvOwnership and corresponding comment. https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/publ... sync/internal_api/public/model_type_store_backend.h:75: FRIEND_TEST_ALL_PREFIXES(ModelTypeStoreBackendTest, TwoBeckendTest); There is a typo in "TwoBeckendTest". Also one way to avoid FRIEND_TEST_ALL_PREFIXES is to declare test class to be friend of implementation class (as it is done above) and implement accessor/helper functions there. This way you don't expose all internals of implementation to test, but only small, controlled subset. See how SharedModelTypeProcessorTest does it for entities count : https://cs.chromium.org/chromium/src/sync/internal_api/shared_model_type_proc... https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/publ... sync/internal_api/public/model_type_store_backend.h:93: ModelTypeStore::Result init_result_; Init result should not be member of backend. If backend initialization fails backend should be immediately destroyed. Instead make Init() function return result of initialization and pass it to the caller of GetOrCreateBackend. https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/publ... File sync/internal_api/public/model_type_store_impl.h (right): https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/publ... sync/internal_api/public/model_type_store_impl.h:107: // Backend is useing scoped_refptr, but should be deleted on backend thread. Could you rephrase "Backend is useing scoped_refptr, but should be deleted on backend thread." to simply "Backend should be deleted on backend thread."
update https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/mode... File sync/internal_api/model_type_store_backend.cc (right): https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/mode... sync/internal_api/model_type_store_backend.cc:47: scoped_refptr<base::SequencedTaskRunner> blocking_task_runner) { On 2016/06/30 20:42:51, pavely wrote: > blocking_task_runner is not used. Could you remove it along with corresponding > includes? Done. https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/mode... sync/internal_api/model_type_store_backend.cc:48: if (backend_map_.Get().find(path) != backend_map_.Get().end()) { On 2016/06/30 20:42:51, pavely wrote: > As we discussed, after checking that backend is not in backend_map_ create local > variable for backend, create and initialize backend object. Only insert backend > into the map and return to the caller if initialization is successful. Done. https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/mode... File sync/internal_api/model_type_store_backend_unittest.cc (right): https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/mode... sync/internal_api/model_type_store_backend_unittest.cc:21: scoped_refptr<ModelTypeStoreBackend> CreateBackend() { On 2016/06/30 20:42:51, pavely wrote: > Could you rename this function to GetOrCreateBackend. This way at callsite it is > clear that not every call will create separate instance of backend. Done. https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/mode... sync/internal_api/model_type_store_backend_unittest.cc:142: TEST_F(ModelTypeStoreBackendTest, TwoBeckendTest) { On 2016/06/30 20:42:51, pavely wrote: > Here are the tests I would write for this change: > - Creating two backends with the same path result in single object (one entry in > the map), dropping all references to backend removes it from the map. > - Creating two backends with different paths result in different backend > objects. Done. https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/mode... sync/internal_api/model_type_store_backend_unittest.cc:147: ASSERT_FALSE(backend_second->HasOneRef()); On 2016/06/30 20:42:51, pavely wrote: > You don't need to check refcount of backend. It is implementation detail of > backends. All you care is that two calls return the same object (the check > above). Done. https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/mode... sync/internal_api/model_type_store_backend_unittest.cc:149: backend = 0; On 2016/06/30 20:42:51, pavely wrote: > backend = nullptr; Done. https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/mode... File sync/internal_api/model_type_store_impl.cc (right): https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/mode... sync/internal_api/model_type_store_impl.cc:128: new ModelTypeStoreImpl(type, backend, blocking_task_runner)); On 2016/06/30 20:42:51, pavely wrote: > You shouldn't create instance of store if GetOrCreateBackend failed. Done. https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/publ... File sync/internal_api/public/model_type_store_backend.h (right): https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/publ... sync/internal_api/public/model_type_store_backend.h:42: static void Disconnect(scoped_refptr<ModelTypeStoreBackend>& backend); On 2016/06/30 20:42:51, pavely wrote: > Remove declaration of Disconnect. Done. https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/publ... sync/internal_api/public/model_type_store_backend.h:47: // void TakeEnvOwnership(std::unique_ptr<leveldb::Env> env); On 2016/06/30 20:42:51, pavely wrote: > Remove TakeEnvOwnership and corresponding comment. Done. https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/publ... sync/internal_api/public/model_type_store_backend.h:75: FRIEND_TEST_ALL_PREFIXES(ModelTypeStoreBackendTest, TwoBeckendTest); On 2016/06/30 20:42:51, pavely wrote: > There is a typo in "TwoBeckendTest". > > Also one way to avoid FRIEND_TEST_ALL_PREFIXES is to declare test class to be > friend of implementation class (as it is done above) and implement > accessor/helper functions there. This way you don't expose all internals of > implementation to test, but only small, controlled subset. > > See how SharedModelTypeProcessorTest does it for entities count : > https://cs.chromium.org/chromium/src/sync/internal_api/shared_model_type_proc... Done. https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/publ... sync/internal_api/public/model_type_store_backend.h:93: ModelTypeStore::Result init_result_; On 2016/06/30 20:42:51, pavely wrote: > Init result should not be member of backend. If backend initialization fails > backend should be immediately destroyed. > Instead make Init() function return result of initialization and pass it to the > caller of GetOrCreateBackend. Done. https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/publ... File sync/internal_api/public/model_type_store_impl.h (right): https://codereview.chromium.org/2077713002/diff/170001/sync/internal_api/publ... sync/internal_api/public/model_type_store_impl.h:107: // Backend is useing scoped_refptr, but should be deleted on backend thread. On 2016/06/30 20:42:51, pavely wrote: > Could you rephrase "Backend is useing scoped_refptr, but should be deleted on > backend thread." to simply "Backend should be deleted on backend thread." Done.
https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/mode... File sync/internal_api/model_type_store_backend.cc (right): https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/mode... sync/internal_api/model_type_store_backend.cc:13: #include "base/sequenced_task_runner.h" You shouldn't need bind.h, location.h and sequenced_task_runner.h. Please remove. https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/mode... sync/internal_api/model_type_store_backend.cc:34: if (backend_map_.Get().find(path_) != backend_map_.Get().end()) { nit: map::erase works correctly if you try to erase element not present in map. You can skip find() check. https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/mode... sync/internal_api/model_type_store_backend.cc:51: ModelTypeStoreBackend* backend = new ModelTypeStoreBackend(path); Wrap backend into scoped_refptr early. This helps you avoid memory leak if early return is introduced in this function some time later. http://www.chromium.org/developers/smart-pointer-guidelines https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/mode... sync/internal_api/model_type_store_backend.cc:53: if (backend->Init(path, std::move(env)) == ModelTypeStore::Result::SUCCESS) { You can pass ModelTypeStore::Result* status as parameter and assign it here. This way you can discover Result in ModelTypeStore. https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/mode... File sync/internal_api/model_type_store_backend_unittest.cc (right): https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/mode... sync/internal_api/model_type_store_backend_unittest.cc:42: void PumpLoop() { I think MessageLoop is not needed in this test. https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/mode... sync/internal_api/model_type_store_backend_unittest.cc:47: bool TheBackendHasCreated(std::string path) { Better name would be BackendExists or BackendExistsForPath. https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/mode... File sync/internal_api/model_type_store_impl.cc (right): https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/mode... sync/internal_api/model_type_store_impl.cc:88: auto task = base::Bind(&ModelTypeStoreBackend::GetOrCreateBackend, path, To get result of store initialization you can allocate memory for result, pass pointer to it to GetOrCreateBackend and pass owning pointer to BackendInitDone. std::unique_ptr<Result> result(new Result); task = base::Bind(..... result.get()); reply = base::Bind(..... base::Passed(result)); See how PostTaskAndReplyWithResult is implemented: https://cs.chromium.org/chromium/src/base/task_runner_util.h?q=PostTaskAndRep... https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/mode... sync/internal_api/model_type_store_impl.cc:128: // TODO: handle error here See comment in ModelTypeStoreImpl::CreateStore. https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/publ... File sync/internal_api/public/model_type_store_backend.h (right): https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/publ... sync/internal_api/public/model_type_store_backend.h:12: #include "base/gtest_prod_util.h" I think you don't need gtest_prod_util.h anymore. https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/publ... sync/internal_api/public/model_type_store_backend.h:38: static scoped_refptr<ModelTypeStoreBackend> GetOrCreateBackend( Could you add function comment for GetOrCreateBackend. https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/publ... sync/internal_api/public/model_type_store_backend.h:83: static base::LazyInstance<BackendMap> backend_map_; Could you add comment for backend_map_. Mention that backend_map_ doesn't take reference to backend and therefore doesn't block backend destruction.
updated, PTAL https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/mode... File sync/internal_api/model_type_store_backend.cc (right): https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/mode... sync/internal_api/model_type_store_backend.cc:13: #include "base/sequenced_task_runner.h" On 2016/07/02 00:02:58, pavely wrote: > You shouldn't need bind.h, location.h and sequenced_task_runner.h. Please > remove. Done. https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/mode... sync/internal_api/model_type_store_backend.cc:34: if (backend_map_.Get().find(path_) != backend_map_.Get().end()) { On 2016/07/02 00:02:58, pavely wrote: > nit: map::erase works correctly if you try to erase element not present in map. > You can skip find() check. Done. https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/mode... sync/internal_api/model_type_store_backend.cc:51: ModelTypeStoreBackend* backend = new ModelTypeStoreBackend(path); On 2016/07/02 00:02:58, pavely wrote: > Wrap backend into scoped_refptr early. This helps you avoid memory leak if early > return is introduced in this function some time later. > http://www.chromium.org/developers/smart-pointer-guidelines Done. https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/mode... sync/internal_api/model_type_store_backend.cc:53: if (backend->Init(path, std::move(env)) == ModelTypeStore::Result::SUCCESS) { On 2016/07/02 00:02:58, pavely wrote: > You can pass ModelTypeStore::Result* status as parameter and assign it here. > This way you can discover Result in ModelTypeStore. Done. https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/mode... File sync/internal_api/model_type_store_backend_unittest.cc (right): https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/mode... sync/internal_api/model_type_store_backend_unittest.cc:42: void PumpLoop() { On 2016/07/02 00:02:58, pavely wrote: > I think MessageLoop is not needed in this test. Done. https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/mode... sync/internal_api/model_type_store_backend_unittest.cc:47: bool TheBackendHasCreated(std::string path) { On 2016/07/02 00:02:58, pavely wrote: > Better name would be BackendExists or BackendExistsForPath. Done. https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/mode... File sync/internal_api/model_type_store_impl.cc (right): https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/mode... sync/internal_api/model_type_store_impl.cc:88: auto task = base::Bind(&ModelTypeStoreBackend::GetOrCreateBackend, path, On 2016/07/02 00:02:59, pavely wrote: > To get result of store initialization you can allocate memory for result, pass > pointer to it to GetOrCreateBackend and pass owning pointer to BackendInitDone. > > std::unique_ptr<Result> result(new Result); > task = base::Bind(..... result.get()); > reply = base::Bind(..... base::Passed(result)); > > See how PostTaskAndReplyWithResult is implemented: > https://cs.chromium.org/chromium/src/base/task_runner_util.h?q=PostTaskAndRep... Done. https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/mode... sync/internal_api/model_type_store_impl.cc:128: // TODO: handle error here On 2016/07/02 00:02:59, pavely wrote: > See comment in ModelTypeStoreImpl::CreateStore. Done. https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/publ... File sync/internal_api/public/model_type_store_backend.h (right): https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/publ... sync/internal_api/public/model_type_store_backend.h:12: #include "base/gtest_prod_util.h" On 2016/07/02 00:02:59, pavely wrote: > I think you don't need gtest_prod_util.h anymore. Done. https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/publ... sync/internal_api/public/model_type_store_backend.h:38: static scoped_refptr<ModelTypeStoreBackend> GetOrCreateBackend( On 2016/07/02 00:02:59, pavely wrote: > Could you add function comment for GetOrCreateBackend. Done. https://codereview.chromium.org/2077713002/diff/190001/sync/internal_api/publ... sync/internal_api/public/model_type_store_backend.h:83: static base::LazyInstance<BackendMap> backend_map_; On 2016/07/02 00:02:59, pavely wrote: > Could you add comment for backend_map_. Mention that backend_map_ doesn't take > reference to backend and therefore doesn't block backend destruction. Done.
https://codereview.chromium.org/2077713002/diff/210001/sync/internal_api/mode... File sync/internal_api/model_type_store_backend_unittest.cc (right): https://codereview.chromium.org/2077713002/diff/210001/sync/internal_api/mode... sync/internal_api/model_type_store_backend_unittest.cc:34: std::unique_ptr<ModelTypeStore::Result> result( nit: Here you don't pass result though callbacks, you can create variable on the stack and pass its pointer to GetOrCreateBackend. Result result; GetOrCreateBackend(..., &result); https://codereview.chromium.org/2077713002/diff/210001/sync/internal_api/mode... File sync/internal_api/model_type_store_impl.cc (right): https://codereview.chromium.org/2077713002/diff/210001/sync/internal_api/mode... sync/internal_api/model_type_store_impl.cc:134: new ModelTypeStoreImpl(type, backend, blocking_task_runner)); You shouldn't create instance of store if GetOrCreateBackend failed. Store's ctor DCHECKS that backend is not null.
PTAL https://codereview.chromium.org/2077713002/diff/210001/sync/internal_api/mode... File sync/internal_api/model_type_store_backend_unittest.cc (right): https://codereview.chromium.org/2077713002/diff/210001/sync/internal_api/mode... sync/internal_api/model_type_store_backend_unittest.cc:34: std::unique_ptr<ModelTypeStore::Result> result( On 2016/07/06 04:47:55, pavely wrote: > nit: Here you don't pass result though callbacks, you can create variable on the > stack and pass its pointer to GetOrCreateBackend. > > Result result; > GetOrCreateBackend(..., &result); Done. https://codereview.chromium.org/2077713002/diff/210001/sync/internal_api/mode... File sync/internal_api/model_type_store_impl.cc (right): https://codereview.chromium.org/2077713002/diff/210001/sync/internal_api/mode... sync/internal_api/model_type_store_impl.cc:134: new ModelTypeStoreImpl(type, backend, blocking_task_runner)); On 2016/07/06 04:47:55, pavely wrote: > You shouldn't create instance of store if GetOrCreateBackend failed. Store's > ctor DCHECKS that backend is not null. Done.
lgtm % one comment. https://codereview.chromium.org/2077713002/diff/230001/sync/internal_api/mode... File sync/internal_api/model_type_store_backend_unittest.cc (right): https://codereview.chromium.org/2077713002/diff/230001/sync/internal_api/mode... sync/internal_api/model_type_store_backend_unittest.cc:57: base::MessageLoop sync_loop_; I think MessageLoop is not needed in this test. Also remove thread_task_runner_handle.h. Could you also clean up unused includes I left in my change: bind.h, message_loop.h and run_loop.h.
https://codereview.chromium.org/2077713002/diff/230001/sync/internal_api/mode... File sync/internal_api/model_type_store_backend_unittest.cc (right): https://codereview.chromium.org/2077713002/diff/230001/sync/internal_api/mode... sync/internal_api/model_type_store_backend_unittest.cc:57: base::MessageLoop sync_loop_; On 2016/07/06 18:51:57, pavely wrote: > I think MessageLoop is not needed in this test. > Also remove thread_task_runner_handle.h. > > Could you also clean up unused includes I left in my change: bind.h, > message_loop.h and run_loop.h. Done.
The CQ bit was checked by gangwu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pavely@chromium.org Link to the patchset: https://codereview.chromium.org/2077713002/#ps250001 (title: "remove includes")
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.
Description was changed from ========== This is 2 of 2 checks in for making ModelTypeStore supports multiple datatypes. This check-in let ModelTypeStoreBackend can be shared with multiple ModelTypeStore. BUG=615214 ========== to ========== This is 2 of 2 checks in for making ModelTypeStore supports multiple datatypes. This check-in let ModelTypeStoreBackend can be shared with multiple ModelTypeStore. BUG=615214 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:250001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== This is 2 of 2 checks in for making ModelTypeStore supports multiple datatypes. This check-in let ModelTypeStoreBackend can be shared with multiple ModelTypeStore. BUG=615214 ========== to ========== This is 2 of 2 checks in for making ModelTypeStore supports multiple datatypes. This check-in let ModelTypeStoreBackend can be shared with multiple ModelTypeStore. BUG=615214 Committed: https://crrev.com/71bdb554039faa51622aa87ebdb3b7eeae2dedf0 Cr-Commit-Position: refs/heads/master@{#403962} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/71bdb554039faa51622aa87ebdb3b7eeae2dedf0 Cr-Commit-Position: refs/heads/master@{#403962} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
