Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2729923003:
[Doodle] Automatically expire Doodles after their TTL (Closed)
|
Created:
3 years, 9 months ago by Marc Treib Modified:
3 years, 9 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Doodle] Automatically expire Doodles after their TTL
BUG=690467
Review-Url: https://codereview.chromium.org/2729923003
Cr-Commit-Position: refs/heads/master@{#454573}
Committed: https://chromium.googlesource.com/chromium/src/+/205bbc5a4c3b30710d63408842fe0408e5f81697
Patch Set 1 #
Total comments: 16
Patch Set 2 : rebase #Patch Set 3 : review #
Total comments: 8
Patch Set 4 : mastiz nits #
Messages
Total messages: 23 (14 generated)
The CQ bit was checked by treib@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...
treib@chromium.org changed reviewers: + fhorschig@chromium.org
PTAL! https://codereview.chromium.org/2729923003/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service_unittest.cc (right): https://codereview.chromium.org/2729923003/diff/1/components/doodle/doodle_se... components/doodle/doodle_service_unittest.cc:245: task_runner()->RunPendingTasks(); So, this is the famous and amazing TestSimpleTaskRunner in action. Admittedly it's less than optimal; it ignores time completely. But the only alternative I can see would be using a full MessageLoop with its MessageLoopTaskRunner. But that seems messy and hard to control, e.g. there's no way to inject a Clock as far as I can tell.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Pretty good, only nits and some comments in the tests. https://codereview.chromium.org/2729923003/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2729923003/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.cc:42: expired ? base::nullopt : doodle_config; nit: A reference isn't expensive, so why no go for a readable if? const base::Optional<DoodleConfig>& new_config = base::nullopt; if (time_to_live <= base::TimeDelta()) { // New config is already expired. new_config = doodle_config; } https://codereview.chromium.org/2729923003/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.cc:48: UpdateTimeToLive(time_to_live); The reason you do it twice instead of earlier is because the timer could expire before the observers are notified? (I would be fine with that) https://codereview.chromium.org/2729923003/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service_unittest.cc (left): https://codereview.chromium.org/2729923003/diff/1/components/doodle/doodle_se... components/doodle/doodle_service_unittest.cc:58: DoodleServiceTest() : fetcher_(nullptr) { expiry_timer_(nullptr)? https://codereview.chromium.org/2729923003/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service_unittest.cc (right): https://codereview.chromium.org/2729923003/diff/1/components/doodle/doodle_se... components/doodle/doodle_service_unittest.cc:64: task_runner_ = new base::TestSimpleTaskRunner(); any reason you don't initialize this earlier? (initializer-list) https://codereview.chromium.org/2729923003/diff/1/components/doodle/doodle_se... components/doodle/doodle_service_unittest.cc:82: base::TimeDelta SomeTime() const { return base::TimeDelta::FromHours(1); } When it was lower_case, it used to be pretty clear that it's only a get. Maybe call it |AnHour()| so it's clearer it's const. ... can we make it an implementation detail? https://codereview.chromium.org/2729923003/diff/1/components/doodle/doodle_se... components/doodle/doodle_service_unittest.cc:91: base::OneShotTimer* expiry_timer_; nitty nit: Would be nice to visually group the weak references. (moving up or down) https://codereview.chromium.org/2729923003/diff/1/components/doodle/doodle_se... components/doodle/doodle_service_unittest.cc:245: task_runner()->RunPendingTasks(); On 2017/03/02 18:51:02, Marc Treib wrote: > So, this is the famous and amazing TestSimpleTaskRunner in action. Admittedly > it's less than optimal; it ignores time completely. But the only alternative I > can see would be using a full MessageLoop with its MessageLoopTaskRunner. But > that seems messy and hard to control, e.g. there's no way to inject a Clock as > far as I can tell. What about TestMockTimeTaskRunner. As far as I know, it's the same but it allows to fast forward: https://cs.chromium.org/chromium/src/base/test/test_mock_time_task_runner.h?t... This even comes with a free Mock{Tick,}Clock. We used it once already: https://cs.chromium.org/chromium/src/components/ntp_snippets/remote/remote_su...
Comments addressed, PTAL again! https://codereview.chromium.org/2729923003/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2729923003/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.cc:42: expired ? base::nullopt : doodle_config; On 2017/03/02 20:12:59, fhorschig wrote: > nit: A reference isn't expensive, so why no go for a readable if? > > const base::Optional<DoodleConfig>& new_config = base::nullopt; > if (time_to_live <= base::TimeDelta()) { // New config is already expired. > new_config = doodle_config; > } This won't work; you can't assign to a reference. They must be initialized and then can't be changed. https://codereview.chromium.org/2729923003/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.cc:48: UpdateTimeToLive(time_to_live); On 2017/03/02 20:12:59, fhorschig wrote: > The reason you do it twice instead of earlier is because the timer > could expire before the observers are notified? > (I would be fine with that) No, the reason was so that I could early-out if the config hasn't changed (without notifying). I've restructured this method so that this call happens in only one place. https://codereview.chromium.org/2729923003/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service_unittest.cc (left): https://codereview.chromium.org/2729923003/diff/1/components/doodle/doodle_se... components/doodle/doodle_service_unittest.cc:58: DoodleServiceTest() : fetcher_(nullptr) { On 2017/03/02 20:12:59, fhorschig wrote: > expiry_timer_(nullptr)? Indeed, thanks! https://codereview.chromium.org/2729923003/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service_unittest.cc (right): https://codereview.chromium.org/2729923003/diff/1/components/doodle/doodle_se... components/doodle/doodle_service_unittest.cc:64: task_runner_ = new base::TestSimpleTaskRunner(); On 2017/03/02 20:12:59, fhorschig wrote: > any reason you don't initialize this earlier? > (initializer-list) Nope. Done. https://codereview.chromium.org/2729923003/diff/1/components/doodle/doodle_se... components/doodle/doodle_service_unittest.cc:82: base::TimeDelta SomeTime() const { return base::TimeDelta::FromHours(1); } On 2017/03/02 20:12:59, fhorschig wrote: > When it was lower_case, it used to be pretty clear that it's only a get. > Maybe call it |AnHour()| so it's clearer it's const. > ... can we make it an implementation detail? I don't know what you mean by "implementation detail". Anyway, it's gone now. https://codereview.chromium.org/2729923003/diff/1/components/doodle/doodle_se... components/doodle/doodle_service_unittest.cc:91: base::OneShotTimer* expiry_timer_; On 2017/03/02 20:12:59, fhorschig wrote: > nitty nit: Would be nice to visually group the weak references. > (moving up or down) To me, the "logical" grouping (clock + task_runner + timer belong together) is more important than the "weak" property. https://codereview.chromium.org/2729923003/diff/1/components/doodle/doodle_se... components/doodle/doodle_service_unittest.cc:245: task_runner()->RunPendingTasks(); On 2017/03/02 20:12:59, fhorschig wrote: > On 2017/03/02 18:51:02, Marc Treib wrote: > > So, this is the famous and amazing TestSimpleTaskRunner in action. Admittedly > > it's less than optimal; it ignores time completely. But the only alternative I > > can see would be using a full MessageLoop with its MessageLoopTaskRunner. But > > that seems messy and hard to control, e.g. there's no way to inject a Clock as > > far as I can tell. > > What about TestMockTimeTaskRunner. As far as I know, it's the same but it > allows to fast forward: > https://cs.chromium.org/chromium/src/base/test/test_mock_time_task_runner.h?t... > > This even comes with a free Mock{Tick,}Clock. > We used it once already: > https://cs.chromium.org/chromium/src/components/ntp_snippets/remote/remote_su... D'oh, forgot about that one again. Thanks for the pointer! Done.
lgtm https://codereview.chromium.org/2729923003/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2729923003/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.cc:42: expired ? base::nullopt : doodle_config; On 2017/03/03 10:06:57, Marc Treib wrote: > On 2017/03/02 20:12:59, fhorschig wrote: > > nit: A reference isn't expensive, so why no go for a readable if? > > > > const base::Optional<DoodleConfig>& new_config = base::nullopt; > > if (time_to_live <= base::TimeDelta()) { // New config is already expired. > > new_config = doodle_config; > > } > > This won't work; you can't assign to a reference. They must be initialized and > then can't be changed. Oh, sorry.
treib@chromium.org changed reviewers: + mastiz@chromium.org
+mastiz: Same procedure as every CL... :)
Nice! LGTM. https://codereview.chromium.org/2729923003/diff/40001/components/doodle/doodl... File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2729923003/diff/40001/components/doodle/doodl... components/doodle/doodle_service.cc:17: DCHECK(fetcher_); According to my earlier comment, you might want a DCHECK for expiry_timer_; https://codereview.chromium.org/2729923003/diff/40001/components/doodle/doodl... components/doodle/doodle_service.cc:42: expired ? base::nullopt : doodle_config; This seems legit but I'm always scared when references and ternary operators are combined, since it's easy to screw up if either of the two expressions results in a non-reference type. Please double check that tests cover both scenarios. https://codereview.chromium.org/2729923003/diff/40001/components/doodle/doodl... File components/doodle/doodle_service.h (right): https://codereview.chromium.org/2729923003/diff/40001/components/doodle/doodl... components/doodle/doodle_service.h:31: std::unique_ptr<base::OneShotTimer> expiry_timer); Can you document whether either of these two must not be null? https://codereview.chromium.org/2729923003/diff/40001/components/doodle/doodl... components/doodle/doodle_service.h:67: std::unique_ptr<base::OneShotTimer> expiry_timer_; Nit: move up before cached_config_, together with other injected dependencies.
The CQ bit was checked by treib@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...
Thanks! Nits addressed; no need to look again. https://codereview.chromium.org/2729923003/diff/40001/components/doodle/doodl... File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2729923003/diff/40001/components/doodle/doodl... components/doodle/doodle_service.cc:17: DCHECK(fetcher_); On 2017/03/03 11:13:15, mastiz wrote: > According to my earlier comment, you might want a DCHECK for expiry_timer_; Done. https://codereview.chromium.org/2729923003/diff/40001/components/doodle/doodl... components/doodle/doodle_service.cc:42: expired ? base::nullopt : doodle_config; On 2017/03/03 11:13:15, mastiz wrote: > This seems legit but I'm always scared when references and ternary operators are > combined, since it's easy to screw up if either of the two expressions results > in a non-reference type. Please double check that tests cover both scenarios. Yup, I have an explicit test for the already-expired case. https://codereview.chromium.org/2729923003/diff/40001/components/doodle/doodl... File components/doodle/doodle_service.h (right): https://codereview.chromium.org/2729923003/diff/40001/components/doodle/doodl... components/doodle/doodle_service.h:31: std::unique_ptr<base::OneShotTimer> expiry_timer); On 2017/03/03 11:13:16, mastiz wrote: > Can you document whether either of these two must not be null? Neither may be null. Comment added. https://codereview.chromium.org/2729923003/diff/40001/components/doodle/doodl... components/doodle/doodle_service.h:67: std::unique_ptr<base::OneShotTimer> expiry_timer_; On 2017/03/03 11:13:16, mastiz wrote: > Nit: move up before cached_config_, together with other injected dependencies. Done.
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 treib@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fhorschig@chromium.org, mastiz@chromium.org Link to the patchset: https://codereview.chromium.org/2729923003/#ps60001 (title: "mastiz nits")
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": 60001, "attempt_start_ts": 1488548204793270,
"parent_rev": "e992f9ff6341836f038ec48def91b8e85bc466cf", "commit_rev":
"205bbc5a4c3b30710d63408842fe0408e5f81697"}
Message was sent while issue was closed.
Description was changed from ========== [Doodle] Automatically expire Doodles after their TTL BUG=690467 ========== to ========== [Doodle] Automatically expire Doodles after their TTL BUG=690467 Review-Url: https://codereview.chromium.org/2729923003 Cr-Commit-Position: refs/heads/master@{#454573} Committed: https://chromium.googlesource.com/chromium/src/+/205bbc5a4c3b30710d63408842fe... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/205bbc5a4c3b30710d63408842fe... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
