Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2961723003:
[USS] Implement ApplySyncChanges and OnURLVisited/Modified/Deleted. (Closed)
Description[USS] Implement ApplySyncChanges and OnURLVisited/Modified/Deleted.
For deletion purpose, exposing function |GetURLRow| in URLDatabase to
HistoryBackend, so TypedURLSyncBridge can look up URLRow by URLID.
BUG=726158
Renaming a function.
UpdateUrlFromServer->MergeURLWithSync
The following 4 functions are ported from TypedUrlSyncableService.
DiffVisits
ShouldSyncVisit
UpdateFromSyncDB->UpdateFromSync
CreateOrUpdateSyncNode->UpdateSyncFromLocal
Review-Url: https://codereview.chromium.org/2961723003
Cr-Commit-Position: refs/heads/master@{#489789}
Committed: https://chromium.googlesource.com/chromium/src/+/7f12ab8632a25615e2d9e8e2593c74c46b6b1f2b
Patch Set 1 #
Total comments: 22
Patch Set 2 : review and rebase #
Total comments: 14
Patch Set 3 : update pavel's review #
Total comments: 4
Patch Set 4 : misunderstand #
Total comments: 5
Patch Set 5 : handle nullptr case in bridge #Messages
Total messages: 63 (47 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 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: This issue passed the CQ dry run.
Description was changed from ========== graft unittests BUG= ========== to ========== draft unittests BUG= ==========
Description was changed from ========== draft unittests BUG= ========== to ========== For deletion purpose, exposing function |GetURLRow| in URLDatabase to HistoryBackend, so TypedURLSyncBridge can look up URLRow by URLID. BUG=726158 Renaming a function. UpdateUrlFromServer->MergeURLWithSync The following 4 functions are ported from TypedUrlSyncableService. DiffVisits ShouldSyncVisit UpdateFromSyncDB->UpdateFromSync CreateOrUpdateSyncNode->UpdateSyncFromLocal ==========
Description was changed from ========== For deletion purpose, exposing function |GetURLRow| in URLDatabase to HistoryBackend, so TypedURLSyncBridge can look up URLRow by URLID. BUG=726158 Renaming a function. UpdateUrlFromServer->MergeURLWithSync The following 4 functions are ported from TypedUrlSyncableService. DiffVisits ShouldSyncVisit UpdateFromSyncDB->UpdateFromSync CreateOrUpdateSyncNode->UpdateSyncFromLocal ========== to ========== [USS] Implement ApplySyncChanges and OnURLVisited/Modified/Deleted For deletion purpose, exposing function |GetURLRow| in URLDatabase to HistoryBackend, so TypedURLSyncBridge can look up URLRow by URLID. BUG=726158 Renaming a function. UpdateUrlFromServer->MergeURLWithSync The following 4 functions are ported from TypedUrlSyncableService. DiffVisits ShouldSyncVisit UpdateFromSyncDB->UpdateFromSync CreateOrUpdateSyncNode->UpdateSyncFromLocal ==========
Patchset #1 (id:1) has been deleted
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: This issue passed the CQ dry run.
Patchset #1 (id:20001) has been deleted
Description was changed from ========== [USS] Implement ApplySyncChanges and OnURLVisited/Modified/Deleted For deletion purpose, exposing function |GetURLRow| in URLDatabase to HistoryBackend, so TypedURLSyncBridge can look up URLRow by URLID. BUG=726158 Renaming a function. UpdateUrlFromServer->MergeURLWithSync The following 4 functions are ported from TypedUrlSyncableService. DiffVisits ShouldSyncVisit UpdateFromSyncDB->UpdateFromSync CreateOrUpdateSyncNode->UpdateSyncFromLocal ========== to ========== [USS] Implement ApplySyncChanges and OnURLVisited/Modified/Deleted. For deletion purpose, exposing function |GetURLRow| in URLDatabase to HistoryBackend, so TypedURLSyncBridge can look up URLRow by URLID. BUG=726158 Renaming a function. UpdateUrlFromServer->MergeURLWithSync The following 4 functions are ported from TypedUrlSyncableService. DiffVisits ShouldSyncVisit UpdateFromSyncDB->UpdateFromSync CreateOrUpdateSyncNode->UpdateSyncFromLocal ==========
gangwu@chromium.org changed reviewers: + pavely@chromium.org
PTAL
https://codereview.chromium.org/2961723003/diff/40001/components/history/core... File components/history/core/browser/typed_url_sync_bridge.cc (right): https://codereview.chromium.org/2961723003/diff/40001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:204: for (const EntityChange& entity_change : entity_changes) { You need to call UpdateStorageKey for ACTION_ADD somewhere. This probably should have been caught by some unittest. https://codereview.chromium.org/2961723003/diff/40001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:214: // Ignoring empty URL in sync DB; url_row comes from history, not sync db. Dot at the end. Why do we expect and ignore rows with empty urls from history db? Is there a valid scenario that leads to this state? https://codereview.chromium.org/2961723003/diff/40001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:258: // Ignoring the case which no matching URLRow with URLID |url_id|. which => with? https://codereview.chromium.org/2961723003/diff/40001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:259: continue; Now that sync metadata lives in history db not finding url by id should be considered error state. I think it should at least log this fact. https://codereview.chromium.org/2961723003/diff/40001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:262: // Ignoring empty URL in sync DB; Why do we expect and ignore rows with empty urls from history db? https://codereview.chromium.org/2961723003/diff/40001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:327: return; // Sync processor not yet initialized, don't sync. change_processor() always returns valid pointer. You should check with IsTrackingMetadata() instead. https://codereview.chromium.org/2961723003/diff/40001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:995: // Ignore username and password, sonce history backend will remove user name sonce => since https://codereview.chromium.org/2961723003/diff/40001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:1114: return base::MakeUnique<EntityData>(); CreateEntityData came from TypedUrlSyncableService::AddTypedUrlToChangeList. The difference manifests when WriteToTypedUrlSpecifics returns true. AddTypedUrlToChangeList would skip the row and not add it to change_list, while CreateEntityData happily returns empty EntityData. It is very easy to make mistake and forget to check contents of return value. Could you check calls to CreateEntityData and ensure they are correct. Is there a unittest that ensures empty entity data is not passed to Put()? https://codereview.chromium.org/2961723003/diff/40001/components/history/core... File components/history/core/browser/typed_url_sync_bridge.h (right): https://codereview.chromium.org/2961723003/diff/40001/components/history/core... components/history/core/browser/typed_url_sync_bridge.h:153: // false and sets an unrecoverable error if the operation failed. I don't think it sets unrecoverable error anymore. https://codereview.chromium.org/2961723003/diff/40001/components/history/core... components/history/core/browser/typed_url_sync_bridge.h:155: syncer::MetadataChangeList* metadata_change_list); I don't think callers check return value of this function. Should they? Or should this function return void? https://codereview.chromium.org/2961723003/diff/40001/components/history/core... File components/history/core/browser/typed_url_sync_metadata_database.h (right): https://codereview.chromium.org/2961723003/diff/40001/components/history/core... components/history/core/browser/typed_url_sync_metadata_database.h:47: URLID StorageKeyToURLID(const std::string& storage_key); static?
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: This issue passed the CQ dry run.
updated https://codereview.chromium.org/2961723003/diff/40001/components/history/core... File components/history/core/browser/typed_url_sync_bridge.cc (right): https://codereview.chromium.org/2961723003/diff/40001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:204: for (const EntityChange& entity_change : entity_changes) { On 2017/07/06 19:28:29, pavely wrote: > You need to call UpdateStorageKey for ACTION_ADD somewhere. This probably should > have been caught by some unittest. Done. https://codereview.chromium.org/2961723003/diff/40001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:214: // Ignoring empty URL in sync DB; On 2017/07/06 19:28:29, pavely wrote: > url_row comes from history, not sync db. > Dot at the end. > > Why do we expect and ignore rows with empty urls from history db? Is there a > valid scenario that leads to this state? Done. https://codereview.chromium.org/2961723003/diff/40001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:258: // Ignoring the case which no matching URLRow with URLID |url_id|. On 2017/07/06 19:28:29, pavely wrote: > which => with? Done. https://codereview.chromium.org/2961723003/diff/40001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:259: continue; On 2017/07/06 19:28:29, pavely wrote: > Now that sync metadata lives in history db not finding url by id should be > considered error state. I think it should at least log this fact. Done. https://codereview.chromium.org/2961723003/diff/40001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:262: // Ignoring empty URL in sync DB; On 2017/07/06 19:28:29, pavely wrote: > Why do we expect and ignore rows with empty urls from history db? Done. https://codereview.chromium.org/2961723003/diff/40001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:327: return; // Sync processor not yet initialized, don't sync. On 2017/07/06 19:28:29, pavely wrote: > change_processor() always returns valid pointer. You should check with > IsTrackingMetadata() instead. Done. https://codereview.chromium.org/2961723003/diff/40001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:995: // Ignore username and password, sonce history backend will remove user name On 2017/07/06 19:28:29, pavely wrote: > sonce => since Done. https://codereview.chromium.org/2961723003/diff/40001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:1114: return base::MakeUnique<EntityData>(); On 2017/07/06 19:28:29, pavely wrote: > CreateEntityData came from TypedUrlSyncableService::AddTypedUrlToChangeList. The > difference manifests when WriteToTypedUrlSpecifics returns true. > AddTypedUrlToChangeList would skip the row and not add it to change_list, while > CreateEntityData happily returns empty EntityData. It is very easy to make > mistake and forget to check contents of return value. Could you check calls to > CreateEntityData and ensure they are correct. > Is there a unittest that ensures empty entity data is not passed to Put()? Done. https://codereview.chromium.org/2961723003/diff/40001/components/history/core... File components/history/core/browser/typed_url_sync_bridge.h (right): https://codereview.chromium.org/2961723003/diff/40001/components/history/core... components/history/core/browser/typed_url_sync_bridge.h:153: // false and sets an unrecoverable error if the operation failed. On 2017/07/06 19:28:29, pavely wrote: > I don't think it sets unrecoverable error anymore. Done. https://codereview.chromium.org/2961723003/diff/40001/components/history/core... components/history/core/browser/typed_url_sync_bridge.h:155: syncer::MetadataChangeList* metadata_change_list); On 2017/07/06 19:28:29, pavely wrote: > I don't think callers check return value of this function. Should they? Or > should this function return void? Done. https://codereview.chromium.org/2961723003/diff/40001/components/history/core... File components/history/core/browser/typed_url_sync_metadata_database.h (right): https://codereview.chromium.org/2961723003/diff/40001/components/history/core... components/history/core/browser/typed_url_sync_metadata_database.h:47: URLID StorageKeyToURLID(const std::string& storage_key); On 2017/07/06 19:28:29, pavely wrote: > static? Done.
https://codereview.chromium.org/2961723003/diff/60001/components/history/core... File components/history/core/browser/typed_url_sync_bridge.cc (right): https://codereview.chromium.org/2961723003/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:7: #include <unordered_set> Why do you need unordered set? https://codereview.chromium.org/2961723003/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:238: // Finally, update storage key in processor. // New entities were either ignored or written to history DB and assigned a storage key. Notify processor about updated storage keys. https://codereview.chromium.org/2961723003/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:370: if (!change_processor()) Same here. https://codereview.chromium.org/2961723003/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:1101: return base::MakeUnique<EntityData>(); One more option would be to return nullptr instead of empty EntityData. This way whoever tries to dereference the ptr will hit segfault instead of silently using empty data. Another benefit is that it will avoid allocating and initializing EntityData object just to free it in the next statement. One more way of thinking about it is that it is easier to reason about code's behavior when it doesn't create and pass around underinitialized objects. On second thought... Is there a scenario where you actually need to return valid empty EntityData from CreateEntityData? https://codereview.chromium.org/2961723003/diff/60001/components/history/core... File components/history/core/browser/typed_url_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2961723003/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_bridge_unittest.cc:388: void VerifyProcessorReceiveValidEntityData() { Receive => Received https://codereview.chromium.org/2961723003/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_bridge_unittest.cc:390: for (const auto& it : changes_multimap) { changes_multimap => processor().put_multimap() ? https://codereview.chromium.org/2961723003/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_bridge_unittest.cc:399: processor().put_multimap().find(storage_key); find is not guaranteed to return first element, it might return arbitrary one. http://en.cppreference.com/w/cpp/container/multimap/find Iterating over equivalent elements is not the most efficient way. Alternatively you can use equal_range and step back with second iterator. This uses the property that multimap iterators are bidirectional.
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...
Updated, PTAL. https://codereview.chromium.org/2961723003/diff/60001/components/history/core... File components/history/core/browser/typed_url_sync_bridge.cc (right): https://codereview.chromium.org/2961723003/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:7: #include <unordered_set> On 2017/07/14 19:15:58, pavely wrote: > Why do you need unordered set? Done. https://codereview.chromium.org/2961723003/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:238: // Finally, update storage key in processor. On 2017/07/14 19:15:57, pavely wrote: > // New entities were either ignored or written to history DB and assigned a > storage key. Notify processor about updated storage keys. Done. https://codereview.chromium.org/2961723003/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:370: if (!change_processor()) On 2017/07/14 19:15:57, pavely wrote: > Same here. Done. https://codereview.chromium.org/2961723003/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_bridge.cc:1101: return base::MakeUnique<EntityData>(); On 2017/07/14 19:15:57, pavely wrote: > One more option would be to return nullptr instead of empty EntityData. This way > whoever tries to dereference the ptr will hit segfault instead of silently using > empty data. Another benefit is that it will avoid allocating and initializing > EntityData object just to free it in the next statement. > One more way of thinking about it is that it is easier to reason about code's > behavior when it doesn't create and pass around underinitialized objects. > > On second thought... Is there a scenario where you actually need to return valid > empty EntityData from CreateEntityData? Done. https://codereview.chromium.org/2961723003/diff/60001/components/history/core... File components/history/core/browser/typed_url_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2961723003/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_bridge_unittest.cc:388: void VerifyProcessorReceiveValidEntityData() { On 2017/07/14 19:15:58, pavely wrote: > Receive => Received Done. https://codereview.chromium.org/2961723003/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_bridge_unittest.cc:390: for (const auto& it : changes_multimap) { On 2017/07/14 19:15:58, pavely wrote: > changes_multimap => processor().put_multimap() ? Done. https://codereview.chromium.org/2961723003/diff/60001/components/history/core... components/history/core/browser/typed_url_sync_bridge_unittest.cc:399: processor().put_multimap().find(storage_key); On 2017/07/14 19:15:58, pavely wrote: > find is not guaranteed to return first element, it might return arbitrary one. > http://en.cppreference.com/w/cpp/container/multimap/find > > Iterating over equivalent elements is not the most efficient way. Alternatively > you can use equal_range and step back with second iterator. This uses the > property that multimap iterators are bidirectional. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
Patchset #3 (id:80001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2961723003/diff/100001/components/history/cor... File components/history/core/browser/typed_url_sync_bridge.cc (right): https://codereview.chromium.org/2961723003/diff/100001/components/history/cor... components/history/core/browser/typed_url_sync_bridge.cc:1103: delete entity_data; Could you change return type of this function back to unique_ptr. Returning unique_ptr is correct since this function creates object and passes ownership to it to the caller. What I meant was to change single line "return base::MakeUnique<EntityData>();" to "return nullptr;" https://codereview.chromium.org/2961723003/diff/100001/components/history/cor... File components/history/core/browser/typed_url_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2961723003/diff/100001/components/history/cor... components/history/core/browser/typed_url_sync_bridge_unittest.cc:398: processor().put_multimap().find(storage_key); You still didn't address my comment. Imagine you inserted three entries with the key "a" and find() returns iterator to the third one. Count() will return 3, iterator++ will move to multimap::end() and EXPECT_NE in line 406 will fail. What if instead you did equal_range().second-- ?
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: This issue passed the CQ dry run.
misunderstood previously, update to correct way. https://codereview.chromium.org/2961723003/diff/100001/components/history/cor... File components/history/core/browser/typed_url_sync_bridge.cc (right): https://codereview.chromium.org/2961723003/diff/100001/components/history/cor... components/history/core/browser/typed_url_sync_bridge.cc:1103: delete entity_data; On 2017/07/18 01:17:56, pavely wrote: > Could you change return type of this function back to unique_ptr. Returning > unique_ptr is correct since this function creates object and passes ownership to > it to the caller. > > What I meant was to change single line "return base::MakeUnique<EntityData>();" > to "return nullptr;" Done. https://codereview.chromium.org/2961723003/diff/100001/components/history/cor... File components/history/core/browser/typed_url_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2961723003/diff/100001/components/history/cor... components/history/core/browser/typed_url_sync_bridge_unittest.cc:398: processor().put_multimap().find(storage_key); On 2017/07/18 01:17:56, pavely wrote: > You still didn't address my comment. Imagine you inserted three entries with the > key "a" and find() returns iterator to the third one. Count() will return 3, > iterator++ will move to multimap::end() and EXPECT_NE in line 406 will fail. > > What if instead you did equal_range().second-- ? > I see, I misunderstand the comments. fixed.
https://codereview.chromium.org/2961723003/diff/120001/components/history/cor... File components/history/core/browser/typed_url_sync_bridge.cc (right): https://codereview.chromium.org/2961723003/diff/120001/components/history/cor... components/history/core/browser/typed_url_sync_bridge.cc:274: batch->Put(key, CreateEntityData(url_row, visits_vector)); Is it possible for CreateEntityData to return nullptr here? Will whoever consumes this DataBatch handle it correctly? Same for GetAllData. https://codereview.chromium.org/2961723003/diff/120001/components/history/cor... File components/history/core/browser/typed_url_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2961723003/diff/120001/components/history/cor... components/history/core/browser/typed_url_sync_bridge_unittest.cc:398: if (eq_range.first == processor().put_multimap().end()) eq_range.first should be compared to eq_range.second. When you call equal_range("1") on a map with single element with key "2" both iterators will point to "2", neither will point to end().
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
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:140001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
updated https://codereview.chromium.org/2961723003/diff/120001/components/history/cor... File components/history/core/browser/typed_url_sync_bridge.cc (right): https://codereview.chromium.org/2961723003/diff/120001/components/history/cor... components/history/core/browser/typed_url_sync_bridge.cc:274: batch->Put(key, CreateEntityData(url_row, visits_vector)); On 2017/07/18 22:35:27, pavely wrote: > Is it possible for CreateEntityData to return nullptr here? Will whoever > consumes this DataBatch handle it correctly? > Same for GetAllData. Batch has already handled empty entitydata, but I think you are right, we should handle here as well. https://codereview.chromium.org/2961723003/diff/120001/components/history/cor... File components/history/core/browser/typed_url_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2961723003/diff/120001/components/history/cor... components/history/core/browser/typed_url_sync_bridge_unittest.cc:398: if (eq_range.first == processor().put_multimap().end()) On 2017/07/18 22:35:27, pavely wrote: > eq_range.first should be compared to eq_range.second. > When you call equal_range("1") on a map with single element with key "2" both > iterators will point to "2", neither will point to end(). Done.
lgtm https://codereview.chromium.org/2961723003/diff/120001/components/history/cor... File components/history/core/browser/typed_url_sync_bridge.cc (right): https://codereview.chromium.org/2961723003/diff/120001/components/history/cor... components/history/core/browser/typed_url_sync_bridge.cc:274: batch->Put(key, CreateEntityData(url_row, visits_vector)); On 2017/07/19 16:26:34, Gang Wu wrote: > On 2017/07/18 22:35:27, pavely wrote: > > Is it possible for CreateEntityData to return nullptr here? Will whoever > > consumes this DataBatch handle it correctly? > > Same for GetAllData. > > Batch has already handled empty entitydata, but I think you are right, we should > handle here as well. The problem is not with the batch, batch is just a container. The problem is with whoever consumes these entities in the end. For example GetData is called by SMTP after initialization to read data of uncommitted entities and send them to server. What would happen if CreateEntityData returned valid empty entity data and sync happily committed it to server? Would other clients get surprised by such data? On the other hand what would happen if SMTP asked for data that corresponds to storage key, but bridge simply didn't add anything to DataBatch, how would SMTP react to this?
gangwu@chromium.org changed reviewers: + brettw@chromium.org
Hi Brett, PTAL
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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": 160001, "attempt_start_ts": 1501109821231060,
"parent_rev": "5857227cc1f929683a667c52725d00575b44afd9", "commit_rev":
"7f12ab8632a25615e2d9e8e2593c74c46b6b1f2b"}
Message was sent while issue was closed.
Description was changed from ========== [USS] Implement ApplySyncChanges and OnURLVisited/Modified/Deleted. For deletion purpose, exposing function |GetURLRow| in URLDatabase to HistoryBackend, so TypedURLSyncBridge can look up URLRow by URLID. BUG=726158 Renaming a function. UpdateUrlFromServer->MergeURLWithSync The following 4 functions are ported from TypedUrlSyncableService. DiffVisits ShouldSyncVisit UpdateFromSyncDB->UpdateFromSync CreateOrUpdateSyncNode->UpdateSyncFromLocal ========== to ========== [USS] Implement ApplySyncChanges and OnURLVisited/Modified/Deleted. For deletion purpose, exposing function |GetURLRow| in URLDatabase to HistoryBackend, so TypedURLSyncBridge can look up URLRow by URLID. BUG=726158 Renaming a function. UpdateUrlFromServer->MergeURLWithSync The following 4 functions are ported from TypedUrlSyncableService. DiffVisits ShouldSyncVisit UpdateFromSyncDB->UpdateFromSync CreateOrUpdateSyncNode->UpdateSyncFromLocal Review-Url: https://codereview.chromium.org/2961723003 Cr-Commit-Position: refs/heads/master@{#489789} Committed: https://chromium.googlesource.com/chromium/src/+/7f12ab8632a25615e2d9e8e2593c... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:160001) as https://chromium.googlesource.com/chromium/src/+/7f12ab8632a25615e2d9e8e2593c... |
