Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2787893003:
[Sync] TypedUrlSyncableService should filter expired visits from local (Closed)
|
Created:
3 years, 8 months ago by Gang Wu Modified:
3 years, 8 months ago Reviewers:
Nicolas Zea CC:
chromium-reviews, sync-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Sync] TypedUrlSyncableService should filter expired visits from local
HistoryBackend may not delete expired visits yet, so TypedUrlSyncableService should filter out expired visits to avoid too many visits send to sync.
BUG=702020
Review-Url: https://codereview.chromium.org/2787893003
Cr-Commit-Position: refs/heads/master@{#461250}
Committed: https://chromium.googlesource.com/chromium/src/+/57f4ccc6434151ec13788b503f8296b33dc9890e
Patch Set 1 : Add tests #Patch Set 2 : fix bug #
Total comments: 12
Patch Set 3 : code review for zea #
Total comments: 2
Patch Set 4 : more verify #
Messages
Total messages: 35 (28 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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 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: 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_...)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== add tests fix without test yet BUG= ========== to ========== [Sync] TypedUrlSyncableService should filter expired visits from local HistoryBackend may not delete expired visits yet, so TypedUrlSyncableService should filter out expired visits to avoid too many visits send to sync. BUG=702020 ==========
gangwu@chromium.org changed reviewers: + zea@chromium.org
PTAL
Mostly LG with some nits https://codereview.chromium.org/2787893003/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc (right): https://codereview.chromium.org/2787893003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc:132: // Sync should not receive expired visits. Pull this logic out into a helper method that lives in typed_urls_helper.cc? https://codereview.chromium.org/2787893003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc:133: WriteTransaction trans(FROM_HERE, GetSyncService(0)->GetUserShare()); Why are these writetransaction/writenode? Wouldn't Read work just as well? https://codereview.chromium.org/2787893003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc:141: #if defined(OS_MACOSX) Looks like this had been disabled for a long time on OSX. I wonder if it's still flaky. Would be good to try to re-enable this, particularly given how it tests the behavior we're interested in. https://codereview.chromium.org/2787893003/diff/60001/components/history/core... File components/history/core/browser/typed_url_syncable_service.cc (right): https://codereview.chromium.org/2787893003/diff/60001/components/history/core... components/history/core/browser/typed_url_syncable_service.cc:958: bool TypedUrlSyncableService::FixupURLAndGetVisits(URLRow* url, Update the comment for this method in the header file? (Currently it claims it only returns false if we fail to load from the DB) https://codereview.chromium.org/2787893003/diff/60001/components/history/core... components/history/core/browser/typed_url_syncable_service.cc:1004: size_t num_of_expired_visit = 0; naming nit: num_of_expired_visit -> num_expired_visits https://codereview.chromium.org/2787893003/diff/60001/components/history/core... components/history/core/browser/typed_url_syncable_service.cc:1015: DLOG(WARNING) << "All visits are expired for url: " << url->url(); Should this be a warning? Seems like expected behavior. Maybe DVLOG(1)?
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. https://codereview.chromium.org/2787893003/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc (right): https://codereview.chromium.org/2787893003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc:132: // Sync should not receive expired visits. On 2017/03/31 18:06:05, Nicolas Zea wrote: > Pull this logic out into a helper method that lives in typed_urls_helper.cc? Done. https://codereview.chromium.org/2787893003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc:133: WriteTransaction trans(FROM_HERE, GetSyncService(0)->GetUserShare()); On 2017/03/31 18:06:05, Nicolas Zea wrote: > Why are these writetransaction/writenode? Wouldn't Read work just as well? Done. https://codereview.chromium.org/2787893003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc:141: #if defined(OS_MACOSX) On 2017/03/31 18:06:05, Nicolas Zea wrote: > Looks like this had been disabled for a long time on OSX. I wonder if it's still > flaky. Would be good to try to re-enable this, particularly given how it tests > the behavior we're interested in. Yes, will do it in another CL to avoid this CL got revert due to test flaky again. crbug.com/707303 https://codereview.chromium.org/2787893003/diff/60001/components/history/core... File components/history/core/browser/typed_url_syncable_service.cc (right): https://codereview.chromium.org/2787893003/diff/60001/components/history/core... components/history/core/browser/typed_url_syncable_service.cc:958: bool TypedUrlSyncableService::FixupURLAndGetVisits(URLRow* url, On 2017/03/31 18:06:05, Nicolas Zea wrote: > Update the comment for this method in the header file? (Currently it claims it > only returns false if we fail to load from the DB) Done. https://codereview.chromium.org/2787893003/diff/60001/components/history/core... components/history/core/browser/typed_url_syncable_service.cc:1004: size_t num_of_expired_visit = 0; On 2017/03/31 18:06:05, Nicolas Zea wrote: > naming nit: num_of_expired_visit -> num_expired_visits Done. https://codereview.chromium.org/2787893003/diff/60001/components/history/core... components/history/core/browser/typed_url_syncable_service.cc:1015: DLOG(WARNING) << "All visits are expired for url: " << url->url(); On 2017/03/31 18:06:05, Nicolas Zea wrote: > Should this be a warning? Seems like expected behavior. Maybe DVLOG(1)? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with a nit https://codereview.chromium.org/2787893003/diff/80001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc (right): https://codereview.chromium.org/2787893003/diff/80001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc:171: ASSERT_EQ(1U, urls.size()); May as well verify the sync directory has the url now.
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
https://codereview.chromium.org/2787893003/diff/80001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc (right): https://codereview.chromium.org/2787893003/diff/80001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc:171: ASSERT_EQ(1U, urls.size()); On 2017/03/31 21:09:55, Nicolas Zea wrote: > May as well verify the sync directory has the url now. If sync directory did nto has that, another client should not receive the update, but I think there is no harm to have more checks.
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.
The CQ bit was checked by gangwu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org Link to the patchset: https://codereview.chromium.org/2787893003/#ps100001 (title: "more verify")
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": 100001, "attempt_start_ts": 1490998206399740,
"parent_rev": "8befd9e691b73db52cc1d6edb6a99c1043a15b89", "commit_rev":
"57f4ccc6434151ec13788b503f8296b33dc9890e"}
Message was sent while issue was closed.
Description was changed from ========== [Sync] TypedUrlSyncableService should filter expired visits from local HistoryBackend may not delete expired visits yet, so TypedUrlSyncableService should filter out expired visits to avoid too many visits send to sync. BUG=702020 ========== to ========== [Sync] TypedUrlSyncableService should filter expired visits from local HistoryBackend may not delete expired visits yet, so TypedUrlSyncableService should filter out expired visits to avoid too many visits send to sync. BUG=702020 Review-Url: https://codereview.chromium.org/2787893003 Cr-Commit-Position: refs/heads/master@{#461250} Committed: https://chromium.googlesource.com/chromium/src/+/57f4ccc6434151ec13788b503f82... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://chromium.googlesource.com/chromium/src/+/57f4ccc6434151ec13788b503f82... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
