Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2710003006:
[Doodle] Introduce a DoodleService (Closed)
|
Created:
3 years, 10 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] Introduce a DoodleService
So far, it caches the last DoodleConfig in memory, and notifies
observers on changes.
BUG=690467
Review-Url: https://codereview.chromium.org/2710003006
Cr-Commit-Position: refs/heads/master@{#453596}
Committed: https://chromium.googlesource.com/chromium/src/+/b4a29c2685ba66db8fc9a2d86a9124dbdc7647cd
Patch Set 1 #
Total comments: 17
Patch Set 2 : don't notify immediately #Patch Set 3 : equivalence #
Total comments: 19
Patch Set 4 : review #
Total comments: 2
Patch Set 5 : review2 #
Total comments: 22
Patch Set 6 : vitaliii review #Patch Set 7 : comment #
Messages
Total messages: 30 (13 generated)
treib@chromium.org changed reviewers: + fhorschig@chromium.org
PTApreliminaryL! This is still a work-in-progress: A few TODOs around, and the tests are... sparse :) Before spending much more time on that, I'd like to get agreement on the API. Thanks! https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_fe... File components/doodle/doodle_fetcher_impl.h (right): https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_fe... components/doodle/doodle_fetcher_impl.h:18: #include "base/time/clock.h" I think std::move needed this to be actually included. Of course, the alternative would be to just move the SetClockForTesting implementation into the .cc https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service.h (right): https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.h:30: // hasn't been downloaded yet. This (immediately notifying the just-added observer) is maybe a bit controversial. The alternative would be to add a "GetCurrentConfig", but I like the simplicity of having just one single way of passing the config around, namely through the observer. WDYT?
This side-effect of immediate notifications bothers me more than probably should... https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.cc:32: observer->OnDoodleConfigUpdated(cached_config_); If you decide to (re)move this line, could you DCHECK for a valid observer here? https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service.h (right): https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.h:20: class Observer { Is there a special reason that we use observers instead of callbacks? I am not sure how many clients would care about multiple updates of the doodle (and OnceCallbacks would be nice then). But I guess you have some use case in mind? E.g. you have NTPs and the Halloween doodle. 1. You open an NTP and get a Doodle. 2. You open a second NTP (or trigger some Refresh in any way). 3. The fetched doodle is different because there are multiple version of the Halloween doodle. 4. The perfectly fine doodle on the first NTP would change. (An unnecessary UI update that would make UX unhappy) Does that sound reasonable? https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.h:30: // hasn't been downloaded yet. On 2017/02/24 14:04:11, Marc Treib wrote: > This (immediately notifying the just-added observer) is maybe a bit > controversial. The alternative would be to add a "GetCurrentConfig", but I like > the simplicity of having just one single way of passing the config around, > namely through the observer. > WDYT? This side-effect can quickly slip one's attention and I expect very few people to read the documentation for an AddObserver method. So I would definitely prefer the second method. https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service_unittest.cc (right): https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_se... components/doodle/doodle_service_unittest.cc:76: service()->AddObserver(&observer); If you remove the "immediate call side effect"; declaring, adding and removing the observer can happen in ctor/dtor and expectations could still happen here. (You seem to use it very often but if you consider it an important part of the setup, feel free to ignore this comment).
Replies below. I haven't quite given up on the observer API yet ;) https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.cc:32: observer->OnDoodleConfigUpdated(cached_config_); On 2017/02/24 15:33:14, fhorschig wrote: > If you decide to (re)move this line, could you DCHECK for a valid observer here? ...sorry, I don't follow? https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service.h (right): https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.h:20: class Observer { On 2017/02/24 15:33:14, fhorschig wrote: > Is there a special reason that we use observers instead of callbacks? > I am not sure how many clients would care about multiple updates of > the doodle (and OnceCallbacks would be nice then). > But I guess you have some use case in mind? The current NTP integration is based on an observer. I think the main use case is startup: The NTP asks for a Doodle, but there isn't one yet (and we can't wait for it, because we don't want to slow down the page load). Instead, it gets called back as soon as the Doodle has been downloaded, and updates in-place. Now, we could implement the same thing without observers of course, but I think that'd make things more complex - clients would have to care about whether the Doodle comes from the cache or not. > E.g. you have NTPs and the Halloween doodle. > 1. You open an NTP and get a Doodle. > 2. You open a second NTP (or trigger some Refresh in any way). > 3. The fetched doodle is different because there are multiple > version of the Halloween doodle. > 4. The perfectly fine doodle on the first NTP would change. > (An unnecessary UI update that would make UX unhappy) It's by definition an update to a currently-not-visible UI though, so I don't think it's a big deal. Maybe we want to remove Doodles immediately when they expire? Not sure about that. > Does that sound reasonable? See above :) https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.h:30: // hasn't been downloaded yet. On 2017/02/24 15:33:14, fhorschig wrote: > On 2017/02/24 14:04:11, Marc Treib wrote: > > This (immediately notifying the just-added observer) is maybe a bit > > controversial. The alternative would be to add a "GetCurrentConfig", but I > like > > the simplicity of having just one single way of passing the config around, > > namely through the observer. > > WDYT? > > This side-effect can quickly slip one's attention and I expect very few people > to read the documentation for an AddObserver method. So I would definitely > prefer the second method. How about if we name it AddObserverAndNotify or something like that? https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service_unittest.cc (right): https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_se... components/doodle/doodle_service_unittest.cc:76: service()->AddObserver(&observer); On 2017/02/24 15:33:14, fhorschig wrote: > If you remove the "immediate call side effect"; declaring, adding and > removing the observer can happen in ctor/dtor and expectations could > still happen here. > > (You seem to use it very often but if you consider it an important part > of the setup, feel free to ignore this comment). Well, so far there are only two tests, so not all that often :D I'll see about moving this into the Test class once there are more tests (and once we've decided on an API). But I expect that some tests will have multiple observers, while some others might not need any, so maybe it's better to leave it explicitly in the test. We'll see.
You may keep your observer... (CIL) https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_fe... File components/doodle/doodle_fetcher_impl.h (right): https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_fe... components/doodle/doodle_fetcher_impl.h:18: #include "base/time/clock.h" On 2017/02/24 14:04:11, Marc Treib wrote: > I think std::move needed this to be actually included. Of course, the > alternative would be to just move the SetClockForTesting implementation into the > .cc Acknowledged. https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.cc:32: observer->OnDoodleConfigUpdated(cached_config_); On 2017/02/24 16:18:51, Marc Treib wrote: > On 2017/02/24 15:33:14, fhorschig wrote: > > If you decide to (re)move this line, could you DCHECK for a valid observer > here? > > ...sorry, I don't follow? I mean a "DCHECK(observer);". (Currently, it would crash anyway due to observer->...) https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service.h (right): https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.h:20: class Observer { On 2017/02/24 16:18:51, Marc Treib wrote: > On 2017/02/24 15:33:14, fhorschig wrote: > > Is there a special reason that we use observers instead of callbacks? > > I am not sure how many clients would care about multiple updates of > > the doodle (and OnceCallbacks would be nice then). > > But I guess you have some use case in mind? > > The current NTP integration is based on an observer. I think the main use case > is startup: The NTP asks for a Doodle, but there isn't one yet (and we can't > wait for it, because we don't want to slow down the page load). Instead, it gets > called back as soon as the Doodle has been downloaded, and updates in-place. > > Now, we could implement the same thing without observers of course, but I think > that'd make things more complex - clients would have to care about whether the > Doodle comes from the cache or not. Consideration of caching is a good argument. > > E.g. you have NTPs and the Halloween doodle. > > 1. You open an NTP and get a Doodle. > > 2. You open a second NTP (or trigger some Refresh in any way). > > 3. The fetched doodle is different because there are multiple > > version of the Halloween doodle. > > 4. The perfectly fine doodle on the first NTP would change. > > (An unnecessary UI update that would make UX unhappy) > > It's by definition an update to a currently-not-visible UI though, so I don't > think it's a big deal. > > Maybe we want to remove Doodles immediately when they expire? Not sure about > that. Hmmm, Remove expired Doodlesmakes sense. That alone would be argument enough for an Observer. Probably the Doodle is an edge case for UI updates. I cannot assess how important a persistent UI would be here. https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.h:30: // hasn't been downloaded yet. On 2017/02/24 16:18:51, Marc Treib wrote: > On 2017/02/24 15:33:14, fhorschig wrote: > > On 2017/02/24 14:04:11, Marc Treib wrote: > > > This (immediately notifying the just-added observer) is maybe a bit > > > controversial. The alternative would be to add a "GetCurrentConfig", but I > > like > > > the simplicity of having just one single way of passing the config around, > > > namely through the observer. > > > WDYT? > > > > This side-effect can quickly slip one's attention and I expect very few people > > to read the documentation for an AddObserver method. So I would definitely > > prefer the second method. > > How about if we name it AddObserverAndNotify or something like that? Definitely an improvement but I still prefer very clear communication. I don't think it is too much of a hassle to call a GetDoodleConfig function and subscribe afterwards for changes. Maybe I can roll along with this one if we find a catchy name. If I read the "AndNotify" part, I would intuitively assume to notify the service. ... but at least I would read the documentation in that case.
Comments addressed. No need to look again yet; I'll ping when the TODOs are gone and there are more tests :) https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.cc:32: observer->OnDoodleConfigUpdated(cached_config_); On 2017/02/24 16:38:20, fhorschig wrote: > On 2017/02/24 16:18:51, Marc Treib wrote: > > On 2017/02/24 15:33:14, fhorschig wrote: > > > If you decide to (re)move this line, could you DCHECK for a valid observer > > here? > > > > ...sorry, I don't follow? > > I mean a "DCHECK(observer);". > (Currently, it would crash anyway due to observer->...) ObserverList already checks that the object isn't null, so no point in checking again here. https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_se... File components/doodle/doodle_service.h (right): https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.h:20: class Observer { On 2017/02/24 16:38:20, fhorschig wrote: > On 2017/02/24 16:18:51, Marc Treib wrote: > > On 2017/02/24 15:33:14, fhorschig wrote: > > > Is there a special reason that we use observers instead of callbacks? > > > I am not sure how many clients would care about multiple updates of > > > the doodle (and OnceCallbacks would be nice then). > > > But I guess you have some use case in mind? > > > > The current NTP integration is based on an observer. I think the main use case > > is startup: The NTP asks for a Doodle, but there isn't one yet (and we can't > > wait for it, because we don't want to slow down the page load). Instead, it > gets > > called back as soon as the Doodle has been downloaded, and updates in-place. > > > > Now, we could implement the same thing without observers of course, but I > think > > that'd make things more complex - clients would have to care about whether the > > Doodle comes from the cache or not. > > Consideration of caching is a good argument. > > > > E.g. you have NTPs and the Halloween doodle. > > > 1. You open an NTP and get a Doodle. > > > 2. You open a second NTP (or trigger some Refresh in any way). > > > 3. The fetched doodle is different because there are multiple > > > version of the Halloween doodle. > > > 4. The perfectly fine doodle on the first NTP would change. > > > (An unnecessary UI update that would make UX unhappy) > > > > It's by definition an update to a currently-not-visible UI though, so I don't > > think it's a big deal. > > > > Maybe we want to remove Doodles immediately when they expire? Not sure about > > that. > Hmmm, Remove expired Doodlesmakes sense. That alone would be argument > enough for an Observer. > > Probably the Doodle is an edge case for UI updates. I cannot assess how > important a > persistent UI would be here. We're also keeping the status quo. I'm open for discussions about changing it, but let's do that separately from the API switch. One step at a time! https://codereview.chromium.org/2710003006/diff/1/components/doodle/doodle_se... components/doodle/doodle_service.h:30: // hasn't been downloaded yet. On 2017/02/24 16:38:20, fhorschig wrote: > On 2017/02/24 16:18:51, Marc Treib wrote: > > On 2017/02/24 15:33:14, fhorschig wrote: > > > On 2017/02/24 14:04:11, Marc Treib wrote: > > > > This (immediately notifying the just-added observer) is maybe a bit > > > > controversial. The alternative would be to add a "GetCurrentConfig", but I > > > like > > > > the simplicity of having just one single way of passing the config around, > > > > namely through the observer. > > > > WDYT? > > > > > > This side-effect can quickly slip one's attention and I expect very few > people > > > to read the documentation for an AddObserver method. So I would definitely > > > prefer the second method. > > > > How about if we name it AddObserverAndNotify or something like that? > > Definitely an improvement but I still prefer very clear communication. > I don't think it is too much of a hassle to call a GetDoodleConfig function > and subscribe afterwards for changes. > Maybe I can roll along with this one if we find a catchy name. If I read the > "AndNotify" part, I would intuitively assume to notify the service. > > ... but at least I would read the documentation in that case. Alright, auto-notifications on AddObserver are gone.
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...
Description was changed from ========== [Doodle] Introduce a DoodleService So far, it caches the last DoodleConfig in memory, and notified observers on changes. BUG=690467 ========== to ========== [Doodle] Introduce a DoodleService So far, it caches the last DoodleConfig in memory, and notifies observers on changes. BUG=690467 ==========
Ready for proper review now, PTAL!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks pretty good apart from some nits. https://codereview.chromium.org/2710003006/diff/40001/components/doodle/doodl... File components/doodle/doodle_service_unittest.cc (right): https://codereview.chromium.org/2710003006/diff/40001/components/doodle/doodl... components/doodle/doodle_service_unittest.cc:80: ASSERT_THAT(service()->config(), Eq(base::nullopt)); Why not EXPECT_THAT? the code wouldn't break and we would get more information about other failures. (It's debatable ... if you want to express a strong precondition, keep it and ignore the reoccurring remarks) https://codereview.chromium.org/2710003006/diff/40001/components/doodle/doodl... components/doodle/doodle_service_unittest.cc:103: ASSERT_THAT(service()->config(), Eq(config)); Same as above, I think an EXPECT is enough to break the test and we will still receive sth. like an inconsistent fetcher state. https://codereview.chromium.org/2710003006/diff/40001/components/doodle/doodl... components/doodle/doodle_service_unittest.cc:109: ASSERT_THAT(fetcher()->num_pending_callbacks(), Eq(1u)); How important is the state of the fetcher? I see that this might be worth an assert but in the test above, an expect was sufficient. https://codereview.chromium.org/2710003006/diff/40001/components/doodle/doodl... components/doodle/doodle_service_unittest.cc:126: ASSERT_THAT(service()->config(), Eq(config)); Same as above, EXPECT_THAT? https://codereview.chromium.org/2710003006/diff/40001/components/doodle/doodl... components/doodle/doodle_service_unittest.cc:138: ASSERT_FALSE(config.IsEquivalent(config2)); This looks like an invariant of the test code as no code changes config or config2: DoodleConfig config; config.doodle_type = DoodleType::SIMPLE; [...] // Code not modifying |content|. DoodleConfig config2; config2.doodle_type = DoodleType::SLIDESHOW; Why would you assert that? Have I missed a Config& parameter here? If you want to express that this condition is crucial for the test, I would DCHECK it. That way, you don't look for an error in your code but you know that the test was modified and is broken. --> go/zine-testing 4.I.A. https://codereview.chromium.org/2710003006/diff/40001/components/doodle/doodl... components/doodle/doodle_service_unittest.cc:152: ASSERT_THAT(service()->config(), Eq(config)); Same as above, EXPECT_THAT? https://codereview.chromium.org/2710003006/diff/40001/components/doodle/doodl... components/doodle/doodle_service_unittest.cc:164: ASSERT_TRUE(config.IsEquivalent(config2)); Same as above, DCHECK? Also, is there a special reason you sometimes use ASSERT_THAT(,Eq()) and sometimes ASSERT_TRUE(IsEquivalent())? (if there is none, I would prefer a consistent _THAT) https://codereview.chromium.org/2710003006/diff/40001/components/doodle/doodl... File components/doodle/doodle_types.cc (right): https://codereview.chromium.org/2710003006/diff/40001/components/doodle/doodl... components/doodle/doodle_types.cc:20: // computed as "now + time_to_live", so it'll never be exactly the same. s/so/because? (or 'otherwise'?) https://codereview.chromium.org/2710003006/diff/40001/components/doodle/doodl... File components/doodle/doodle_types.h (right): https://codereview.chromium.org/2710003006/diff/40001/components/doodle/doodl... components/doodle/doodle_types.h:71: // TODO(treib,fhorschig): Don't expose this? Clients don't care about it. What do you think of? private member with friend class? Moving implementation into the struct?
PTAL again! https://codereview.chromium.org/2710003006/diff/40001/components/doodle/doodl... File components/doodle/doodle_service_unittest.cc (right): https://codereview.chromium.org/2710003006/diff/40001/components/doodle/doodl... components/doodle/doodle_service_unittest.cc:80: ASSERT_THAT(service()->config(), Eq(base::nullopt)); On 2017/02/27 17:43:53, fhorschig wrote: > Why not EXPECT_THAT? the code wouldn't break and we would get more > information about other failures. > (It's debatable ... if you want to express a strong precondition, > keep it and ignore the reoccurring remarks) The pattern is: ASSERT for preconditions, EXPECT for the actual tested things. Here, if the service somehow already had a config, then the test below wouldn't test what it's supposed to test anymore. https://codereview.chromium.org/2710003006/diff/40001/components/doodle/doodl... components/doodle/doodle_service_unittest.cc:103: ASSERT_THAT(service()->config(), Eq(config)); On 2017/02/27 17:43:53, fhorschig wrote: > Same as above, I think an EXPECT is enough to break the test and > we will still receive sth. like an inconsistent fetcher state. Precondition https://codereview.chromium.org/2710003006/diff/40001/components/doodle/doodl... components/doodle/doodle_service_unittest.cc:109: ASSERT_THAT(fetcher()->num_pending_callbacks(), Eq(1u)); On 2017/02/27 17:43:59, fhorschig wrote: > How important is the state of the fetcher? > I see that this might be worth an assert but in the test above, > an expect was sufficient. Precondition https://codereview.chromium.org/2710003006/diff/40001/components/doodle/doodl... components/doodle/doodle_service_unittest.cc:126: ASSERT_THAT(service()->config(), Eq(config)); On 2017/02/27 17:43:59, fhorschig wrote: > Same as above, EXPECT_THAT? Precondition https://codereview.chromium.org/2710003006/diff/40001/components/doodle/doodl... components/doodle/doodle_service_unittest.cc:138: ASSERT_FALSE(config.IsEquivalent(config2)); On 2017/02/27 17:43:59, fhorschig wrote: > This looks like an invariant of the test code as no code changes > config or config2: > > DoodleConfig config; > config.doodle_type = DoodleType::SIMPLE; > [...] // Code not modifying |content|. > DoodleConfig config2; > config2.doodle_type = DoodleType::SLIDESHOW; > > Why would you assert that? Have I missed a Config& parameter here? > If you want to express that this condition is crucial for the test, > I would DCHECK it. That way, you don't look for an error in your code > but you know that the test was modified and is broken. > --> go/zine-testing 4.I.A. Well, it's arguable if a change in the IsEquivalent implementation should make this test fail or crash. But sure, since it won't make much of a difference in practice either way, done. https://codereview.chromium.org/2710003006/diff/40001/components/doodle/doodl... components/doodle/doodle_service_unittest.cc:152: ASSERT_THAT(service()->config(), Eq(config)); On 2017/02/27 17:43:59, fhorschig wrote: > Same as above, EXPECT_THAT? Precondition https://codereview.chromium.org/2710003006/diff/40001/components/doodle/doodl... components/doodle/doodle_service_unittest.cc:164: ASSERT_TRUE(config.IsEquivalent(config2)); On 2017/02/27 17:43:59, fhorschig wrote: > Same as above, DCHECK? Done. > Also, is there a special reason you sometimes use ASSERT_THAT(,Eq()) and > sometimes ASSERT_TRUE(IsEquivalent())? > > (if there is none, I would prefer a consistent _THAT) Equality != equivalence. To use ASSERT_THAT with IsEquivalent, I'd have to write a custom matcher based on IsEquivalent. IMO it's *much* easier to read with ASSERT_TRUE. I get using ASSERT_THAT(.., Eq(..)) over ASSERT_EQ for consistency, but IMO there's absolutely nothing wrong with ASSERT_TRUE/FALSE. Same reason you don't write "if (var == true)". https://codereview.chromium.org/2710003006/diff/40001/components/doodle/doodl... File components/doodle/doodle_types.cc (right): https://codereview.chromium.org/2710003006/diff/40001/components/doodle/doodl... components/doodle/doodle_types.cc:20: // computed as "now + time_to_live", so it'll never be exactly the same. On 2017/02/27 17:44:07, fhorschig wrote: > s/so/because? (or 'otherwise'?) "it" was supposed to refer to the expiry date. Reformulated to hopefully make it clearer. https://codereview.chromium.org/2710003006/diff/40001/components/doodle/doodl... File components/doodle/doodle_types.h (right): https://codereview.chromium.org/2710003006/diff/40001/components/doodle/doodl... components/doodle/doodle_types.h:71: // TODO(treib,fhorschig): Don't expose this? Clients don't care about it. On 2017/02/27 17:44:07, fhorschig wrote: > What do you think of? Nothing that I'd be very happy with, yet. > private member with friend class? Friend classes are generally discouraged. IMO this wouldn't be much better than a comment "you don't care about this field". > Moving implementation into the struct? Not sure what you mean by this? I think the "correct" design would be having different types between fetcher->service and service->client. But that's probably more trouble than it's worth. WDYT?
lgtm! https://codereview.chromium.org/2710003006/diff/40001/components/doodle/doodl... File components/doodle/doodle_types.h (right): https://codereview.chromium.org/2710003006/diff/40001/components/doodle/doodl... components/doodle/doodle_types.h:71: // TODO(treib,fhorschig): Don't expose this? Clients don't care about it. On 2017/02/28 10:25:49, Marc Treib wrote: > On 2017/02/27 17:44:07, fhorschig wrote: > > What do you think of? > > Nothing that I'd be very happy with, yet. > > > private member with friend class? > > Friend classes are generally discouraged. IMO this wouldn't be much better than > a comment "you don't care about this field". > > > Moving implementation into the struct? > > Not sure what you mean by this? > > I think the "correct" design would be having different types between > fetcher->service and service->client. But that's probably more trouble than it's > worth. WDYT? Sounds reasonable. We should decide that when we actually use it. (Thus, your ToDo seems appropriate) https://codereview.chromium.org/2710003006/diff/60001/components/doodle/doodl... File components/doodle/doodle_service_unittest.cc (right): https://codereview.chromium.org/2710003006/diff/60001/components/doodle/doodl... components/doodle/doodle_service_unittest.cc:85: EXPECT_THAT(fetcher()->num_pending_callbacks(), Eq(1u)); Precondition, here too?
https://codereview.chromium.org/2710003006/diff/60001/components/doodle/doodl... File components/doodle/doodle_service_unittest.cc (right): https://codereview.chromium.org/2710003006/diff/60001/components/doodle/doodl... components/doodle/doodle_service_unittest.cc:85: EXPECT_THAT(fetcher()->num_pending_callbacks(), Eq(1u)); On 2017/02/28 10:48:09, fhorschig wrote: > Precondition, here too? Sure, done. I've also added a separate test for this (calling the fetcher on Refresh()). Seemed like this should have some explicit coverage, even though it's implicitly covered by the Observer tests below.
treib@chromium.org changed reviewers: + vitaliii@chromium.org
Hi Vitalii, could you PTAL and use your awesome committer powers to approve? :) Thanks!
LGTM with nits. https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... File components/doodle/doodle_fetcher_impl.cc (right): https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... components/doodle/doodle_fetcher_impl.cc:26: const double kMaxTimeToLiveMS = 30.0 * 24 * 60 * 60 * 1000; // 30 days base::TimeDelta with base::TimeDelta::FromMilliseconds seems a clearer and a safer option. https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... components/doodle/doodle_fetcher_impl.cc:132: DLOG(WARNING) << "Doodle JSON is not valid"; s/valid"/valid." https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... components/doodle/doodle_fetcher_impl.cc:139: DLOG(WARNING) << "Doodle JSON reponse did not contain 'ddljson' key."; reSponse https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... components/doodle/doodle_fetcher_impl.cc:161: if (!ParseImage(ddljson, "large_image", &doodle.large_image)) { It is not clear why not having a |large_image| means no doodle. https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... File components/doodle/doodle_fetcher_impl.h (right): https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... components/doodle/doodle_fetcher_impl.h:94: // Allow for an injectable tick clock for testing. nit: s/tick clock/clock? https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... components/doodle/doodle_service.cc:36: // If there is no config before or after the update, there's nothing to do. s/or/and https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... File components/doodle/doodle_service_unittest.cc (right): https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... components/doodle/doodle_service_unittest.cc:76: TEST_F(DoodleServiceTest, FetchesConfigOnRefresh) { I felt like we prefer 'Should-Do-What-When' to 'Does-What-When'. However, the testing doc says: '"Does-What-When" notation is okay if you want to emphasize an action.' https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... components/doodle/doodle_service_unittest.cc:169: DCHECK(!config.IsEquivalent(config2)); nit: making a typo in config2 is much easier than in second_config. https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... components/doodle/doodle_service_unittest.cc:191: // Serve the request with an equivalent doodle config. The observer should get should not? https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... File components/doodle/doodle_types.cc (right): https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... components/doodle/doodle_types.cc:20: // |expiry_date| gets computed as "now + time_to_live", so when an identical This feels like too many implementation details. Why not "|expiry_date| depends on when fetch happens, so when an identical config gets re-fetched, the expiry date will be different."
Thanks! Replies below. https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... File components/doodle/doodle_fetcher_impl.cc (right): https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... components/doodle/doodle_fetcher_impl.cc:26: const double kMaxTimeToLiveMS = 30.0 * 24 * 60 * 60 * 1000; // 30 days On 2017/02/28 11:37:17, vitaliii wrote: > base::TimeDelta with base::TimeDelta::FromMilliseconds seems a clearer and a > safer option. We can't have constants of non-trivial types, because static initializers. This value is used in a FromMilliseconds below. https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... components/doodle/doodle_fetcher_impl.cc:132: DLOG(WARNING) << "Doodle JSON is not valid"; On 2017/02/28 11:37:17, vitaliii wrote: > s/valid"/valid." Done. https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... components/doodle/doodle_fetcher_impl.cc:139: DLOG(WARNING) << "Doodle JSON reponse did not contain 'ddljson' key."; On 2017/02/28 11:37:17, vitaliii wrote: > reSponse Done. https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... components/doodle/doodle_fetcher_impl.cc:161: if (!ParseImage(ddljson, "large_image", &doodle.large_image)) { On 2017/02/28 11:37:17, vitaliii wrote: > It is not clear why not having a |large_image| means no doodle. Per info from the Doodle folks, this is basically the only required field. I added a comment. https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... File components/doodle/doodle_fetcher_impl.h (right): https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... components/doodle/doodle_fetcher_impl.h:94: // Allow for an injectable tick clock for testing. On 2017/02/28 11:37:17, vitaliii wrote: > nit: s/tick clock/clock? Unrelated to this CL, but sure, done. https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... components/doodle/doodle_service.cc:36: // If there is no config before or after the update, there's nothing to do. On 2017/02/28 11:37:17, vitaliii wrote: > s/or/and Actually, neither formulation is really unambiguous. I made it: "If there isn't a config either before or after the update, there's nothing to do." https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... File components/doodle/doodle_service_unittest.cc (right): https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... components/doodle/doodle_service_unittest.cc:76: TEST_F(DoodleServiceTest, FetchesConfigOnRefresh) { On 2017/02/28 11:37:18, vitaliii wrote: > I felt like we prefer 'Should-Do-What-When' to 'Does-What-When'. > However, the testing doc says: '"Does-What-When" notation is okay if you want to > emphasize an action.' Ah, I didn't know we had a preference. I named these consistently with the existing DoodleFetcherImpl tests, which also use this style. https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... components/doodle/doodle_service_unittest.cc:169: DCHECK(!config.IsEquivalent(config2)); On 2017/02/28 11:37:18, vitaliii wrote: > nit: making a typo in config2 is much easier than in second_config. Good point. Renamed. https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... components/doodle/doodle_service_unittest.cc:191: // Serve the request with an equivalent doodle config. The observer should get On 2017/02/28 11:37:18, vitaliii wrote: > should not? Indeed, thanks! Too much copypasta... https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... File components/doodle/doodle_types.cc (right): https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... components/doodle/doodle_types.cc:20: // |expiry_date| gets computed as "now + time_to_live", so when an identical On 2017/02/28 11:37:18, vitaliii wrote: > This feels like too many implementation details. > Why not "|expiry_date| depends on when fetch happens, so when an identical > config gets re-fetched, the expiry date will be different." "too many implementation details" is kinda true, but IMO this is weird and unexpected enough that it deserves an explicit, detailed comment. This isn't really a great place for it, but I don't know where else to put it. Longer-term, I'd hope to get rid of the expiry_time member completely, since it's not really needed here. See the TODO in the header.
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...
a small comment about the comment. https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... components/doodle/doodle_service.cc:36: // If there is no config before or after the update, there's nothing to do. On 2017/02/28 13:06:49, Marc Treib wrote: > On 2017/02/28 11:37:17, vitaliii wrote: > > s/or/and > > Actually, neither formulation is really unambiguous. > > I made it: "If there isn't a config either before or after the update, there's > nothing to do." Both messages feel to me like !cached_config_.has_value() || !doodle_config.has_value(). What about "There was no config before and we did not get a new one, there's nothing to do"? Overall it may be my English problem, not your message, so feel free to ignore.
https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2710003006/diff/80001/components/doodle/doodl... components/doodle/doodle_service.cc:36: // If there is no config before or after the update, there's nothing to do. On 2017/02/28 13:13:28, vitaliii wrote: > On 2017/02/28 13:06:49, Marc Treib wrote: > > On 2017/02/28 11:37:17, vitaliii wrote: > > > s/or/and > > > > Actually, neither formulation is really unambiguous. > > > > I made it: "If there isn't a config either before or after the update, there's > > nothing to do." > > Both messages feel to me like !cached_config_.has_value() || > !doodle_config.has_value(). > What about "There was no config before and we did not get a new one, there's > nothing to do"? > > Overall it may be my English problem, not your message, so feel free to ignore. I agree that your formulation is much better. Done.
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, vitaliii@chromium.org Link to the patchset: https://codereview.chromium.org/2710003006/#ps120001 (title: "comment")
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": 1488288868679170,
"parent_rev": "e48ec244742e350bb55ad07dde1150022c536f84", "commit_rev":
"b4a29c2685ba66db8fc9a2d86a9124dbdc7647cd"}
Message was sent while issue was closed.
Description was changed from ========== [Doodle] Introduce a DoodleService So far, it caches the last DoodleConfig in memory, and notifies observers on changes. BUG=690467 ========== to ========== [Doodle] Introduce a DoodleService So far, it caches the last DoodleConfig in memory, and notifies observers on changes. BUG=690467 Review-Url: https://codereview.chromium.org/2710003006 Cr-Commit-Position: refs/heads/master@{#453596} Committed: https://chromium.googlesource.com/chromium/src/+/b4a29c2685ba66db8fc9a2d86a91... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/b4a29c2685ba66db8fc9a2d86a91... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
