Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2672753002:
Refactor History's CommitLaterTask with CancelableCallback (Closed)
DescriptionRefactor History's CommitLaterTask with CancelableCallback
Addresses an old TODO to replace the class with CancelableCallback,
which is almost a drop-in replacement.
base::Unretained(this) is used for the callback because there should be,
according to the DCHECK in the destructor, no scheduled commit during
destruction.
Review-Url: https://codereview.chromium.org/2672753002
Cr-Commit-Position: refs/heads/master@{#449735}
Committed: https://chromium.googlesource.com/chromium/src/+/87585300304e2a813e13b18c3fca1022c167bf91
Patch Set 1 #Patch Set 2 : Remove leftover friendship. #
Total comments: 5
Patch Set 3 : Use CancelableClosure instead, much simpler! #Patch Set 4 : Minor comment change. #
Total comments: 5
Patch Set 5 : Reverted paranoid use of WeakPtr. #
Total comments: 2
Patch Set 6 : Added comment as suggested. #Patch Set 7 : Explicitly close TestHistoryBackend. #
Messages
Total messages: 44 (27 generated)
The CQ bit was checked by mastiz@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...
Description was changed from ========== Refactor History's CommitLaterTask with cancelable tasks CancelableTaskTracker lacked support for delayed tasks which is now added and adopted by HistoryBackend, removing the former local implementation CommitLaterTask. BUG= ========== to ========== Refactor History's CommitLaterTask with cancelable tasks Addresses an old TODO to replace the class with base::WeakPtrFactory, in combination with CancelableTaskTracker which allows implementing CancelScheduledCommit(). CancelableTaskTracker lacked support for delayed tasks which is now added and adopted. Presumably other parts of the codebase can benefit from this functionality in the future. BUG= ==========
mastiz@chromium.org changed reviewers: + treib@chromium.org
treib@ for sanity-checking before looping in the owners.
Sanity check passed. I expect you'll be asked to add more tests for a base/ change. https://codereview.chromium.org/2672753002/diff/20001/base/task/cancelable_ta... File base/task/cancelable_task_tracker.cc (right): https://codereview.chromium.org/2672753002/diff/20001/base/task/cancelable_ta... base/task/cancelable_task_tracker.cc:107: CancelableTaskTracker::TaskId CancelableTaskTracker::PostDelayedTask( Hm, I guess this is juuust different enough from PostTaskAndReply that it doesn't make sense to share code :-/ https://codereview.chromium.org/2672753002/diff/20001/base/task/cancelable_ta... File base/task/cancelable_task_tracker_unittest.cc (right): https://codereview.chromium.org/2672753002/diff/20001/base/task/cancelable_ta... base/task/cancelable_task_tracker_unittest.cc:321: TEST_F(CancelableTaskTrackerTest, HasTrackedTasksPostDelayed) { I think there's a few more tests here that should be extended/duplicated to also cover the new PostDelayed https://codereview.chromium.org/2672753002/diff/20001/components/history/core... File components/history/core/browser/history_backend.cc (left): https://codereview.chromium.org/2672753002/diff/20001/components/history/core... components/history/core/browser/history_backend.cc:130: // TODO(brettw): bug 1165182: This should be replaced with a Wow, this might be the oldest Chrome bug I've seen :D
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by mastiz@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...
Description was changed from ========== Refactor History's CommitLaterTask with cancelable tasks Addresses an old TODO to replace the class with base::WeakPtrFactory, in combination with CancelableTaskTracker which allows implementing CancelScheduledCommit(). CancelableTaskTracker lacked support for delayed tasks which is now added and adopted. Presumably other parts of the codebase can benefit from this functionality in the future. BUG= ========== to ========== Refactor History's CommitLaterTask with cancelable callbacks Addresses an old TODO to replace the class with base::WeakPtrFactory, in combination with CancelableCallback, which allows implementing CancelScheduledCommit(). Strictly speaking, the WeakFactoryPtr doesn't seem necessary because scheduled commits are cancelled anyway during shutdown, but it seems safer to stick to the common pattern in Chromium and use weak pointers for callbacks. ==========
PTAL. Sorry for the noise but I rethought the approach using CancelableCallback instead and it turns out to be much simpler, and doesn't require changes in base. I might move the changes in base to a dedicated patch, in case the owners are interested, now that the code is written. https://codereview.chromium.org/2672753002/diff/20001/base/task/cancelable_ta... File base/task/cancelable_task_tracker.cc (right): https://codereview.chromium.org/2672753002/diff/20001/base/task/cancelable_ta... base/task/cancelable_task_tracker.cc:107: CancelableTaskTracker::TaskId CancelableTaskTracker::PostDelayedTask( On 2017/02/02 10:52:21, Marc Treib wrote: > Hm, I guess this is juuust different enough from PostTaskAndReply that it > doesn't make sense to share code :-/ Acknowledged, I think so. https://codereview.chromium.org/2672753002/diff/20001/base/task/cancelable_ta... File base/task/cancelable_task_tracker_unittest.cc (right): https://codereview.chromium.org/2672753002/diff/20001/base/task/cancelable_ta... base/task/cancelable_task_tracker_unittest.cc:321: TEST_F(CancelableTaskTrackerTest, HasTrackedTasksPostDelayed) { On 2017/02/02 10:52:21, Marc Treib wrote: > I think there's a few more tests here that should be extended/duplicated to also > cover the new PostDelayed I added these but then rethought the whole approach.
Much simpler indeed! I expect that the base/ changes would be rejected if there's no code that actually uses the new functionality. But that's someone else's decision :) https://codereview.chromium.org/2672753002/diff/60001/components/history/core... File components/history/core/browser/history_backend.cc (right): https://codereview.chromium.org/2672753002/diff/60001/components/history/core... components/history/core/browser/history_backend.cc:2207: if (!scheduled_commit_.IsCancelled()) So, here's the place where I might like that comment. https://codereview.chromium.org/2672753002/diff/60001/components/history/core... components/history/core/browser/history_backend.cc:2211: base::Bind(&HistoryBackend::Commit, weak_ptr_factory_.GetWeakPtr())); I don't think the WeakPtrFactory is required. Most things in Chromium where you pass a callback guarantee to not call you back if they're destroyed. So if you own the thing where you pass the callback, then you don't need WeakPtrs; you can just do base::Unretained(this). https://codereview.chromium.org/2672753002/diff/60001/components/history/core... File components/history/core/browser/history_backend.h (right): https://codereview.chromium.org/2672753002/diff/60001/components/history/core... components/history/core/browser/history_backend.h:831: // !IsCancelled() to see if there is a commit scheduled in the future, and we So IsCancelled()==true is the default state for an "empty" CancelableCallback? That is a bit surprising to me; maybe worth a comment in the .cc where we depend on that.
The CQ bit was checked by mastiz@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...
Description was changed from ========== Refactor History's CommitLaterTask with cancelable callbacks Addresses an old TODO to replace the class with base::WeakPtrFactory, in combination with CancelableCallback, which allows implementing CancelScheduledCommit(). Strictly speaking, the WeakFactoryPtr doesn't seem necessary because scheduled commits are cancelled anyway during shutdown, but it seems safer to stick to the common pattern in Chromium and use weak pointers for callbacks. ========== to ========== Refactor History's CommitLaterTask with CancelableCallback Addresses an old TODO to replace the class with CancelableCallback, which is almost a drop-in replacement. base::Unretained(this) is used for the callback because there should be, according to the DCHECK in the destructor, no scheduled commit during destruction. ==========
Thanks! https://codereview.chromium.org/2672753002/diff/60001/components/history/core... File components/history/core/browser/history_backend.cc (right): https://codereview.chromium.org/2672753002/diff/60001/components/history/core... components/history/core/browser/history_backend.cc:2207: if (!scheduled_commit_.IsCancelled()) On 2017/02/02 12:59:24, Marc Treib wrote: > So, here's the place where I might like that comment. Done. https://codereview.chromium.org/2672753002/diff/60001/components/history/core... components/history/core/browser/history_backend.cc:2211: base::Bind(&HistoryBackend::Commit, weak_ptr_factory_.GetWeakPtr())); On 2017/02/02 12:59:24, Marc Treib wrote: > I don't think the WeakPtrFactory is required. Most things in Chromium where you > pass a callback guarantee to not call you back if they're destroyed. So if you > own the thing where you pass the callback, then you don't need WeakPtrs; you can > just do base::Unretained(this). Done.
mastiz@chromium.org changed reviewers: + brettw@chromium.org
brettw@: I came across b/1165182 and decided to address it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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_...)
Thanks! LGTM. https://codereview.chromium.org/2672753002/diff/80001/components/history/core... File components/history/core/browser/history_backend.h (right): https://codereview.chromium.org/2672753002/diff/80001/components/history/core... components/history/core/browser/history_backend.h:831: // can use Cancel() to cancel the scheduled commit. There can be only one Can you mention here that the initial state for a default-initialized CancelableClosure is "canceled"? I had to go check the semantics because I was suspicious of your unconditional usage of IsCanceled in the code.
https://codereview.chromium.org/2672753002/diff/80001/components/history/core... File components/history/core/browser/history_backend.h (right): https://codereview.chromium.org/2672753002/diff/80001/components/history/core... components/history/core/browser/history_backend.h:831: // can use Cancel() to cancel the scheduled commit. There can be only one On 2017/02/08 04:58:30, brettw (plz ping after 24h) wrote: > Can you mention here that the initial state for a default-initialized > CancelableClosure is "canceled"? I had to go check the semantics because I was > suspicious of your unconditional usage of IsCanceled in the code. Done. I had a comment in the .cc file about this, added it here as well now.
The CQ bit was checked by mastiz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org Link to the patchset: https://codereview.chromium.org/2672753002/#ps100001 (title: "Added comment as suggested.")
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_tsan_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 mastiz@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.
mastiz@chromium.org changed reviewers: + gangwu@chromium.org
Adding gangwu@ because of newly added changes in typed_url_syncable_service_unittest.cc My change presumably introduces no behavioral changes. However, I suspect there's an actual bug in these tests and instead, what the tests should be doing, is commit/flush HistoryBackend writes (since the backend will by default defer and batch writes).
On 2017/02/08 12:23:50, mastiz wrote: > Adding gangwu@ because of newly added changes in > typed_url_syncable_service_unittest.cc > > My change presumably introduces no behavioral changes. However, I suspect > there's an actual bug in these tests and instead, what the tests should be > doing, is commit/flush HistoryBackend writes (since the backend will by default > defer and batch writes). gangwu@: friendly ping, thx.
lgtm
On 2017/02/09 08:14:07, mastiz wrote: > On 2017/02/08 12:23:50, mastiz wrote: > > Adding gangwu@ because of newly added changes in > > typed_url_syncable_service_unittest.cc > > > > My change presumably introduces no behavioral changes. However, I suspect > > there's an actual bug in these tests and instead, what the tests should be > > doing, is commit/flush HistoryBackend writes (since the backend will by > default > > defer and batch writes). > > gangwu@: friendly ping, thx. Thanks for fix this test issue!
The CQ bit was checked by mastiz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org Link to the patchset: https://codereview.chromium.org/2672753002/#ps120001 (title: "Explicitly close TestHistoryBackend.")
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": 120001, "attempt_start_ts": 1486755645567000,
"parent_rev": "daed1c58456d189c11069b1d6c334a085a257360", "commit_rev":
"87585300304e2a813e13b18c3fca1022c167bf91"}
Message was sent while issue was closed.
Description was changed from ========== Refactor History's CommitLaterTask with CancelableCallback Addresses an old TODO to replace the class with CancelableCallback, which is almost a drop-in replacement. base::Unretained(this) is used for the callback because there should be, according to the DCHECK in the destructor, no scheduled commit during destruction. ========== to ========== Refactor History's CommitLaterTask with CancelableCallback Addresses an old TODO to replace the class with CancelableCallback, which is almost a drop-in replacement. base::Unretained(this) is used for the callback because there should be, according to the DCHECK in the destructor, no scheduled commit during destruction. Review-Url: https://codereview.chromium.org/2672753002 Cr-Commit-Position: refs/heads/master@{#449735} Committed: https://chromium.googlesource.com/chromium/src/+/87585300304e2a813e13b18c3fca... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/87585300304e2a813e13b18c3fca...
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2697513002/ by msramek@chromium.org. The reason for reverting is: Suspect for introducing the memory leak in ~TypedUrlSyncableServiceTest_DeleteUrlAndVisits(). For more details, please see: https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20Chrom.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
