Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1976153002:
Various NTPSnippetsService cleanups: (Closed)
|
Created:
4 years, 7 months ago by Marc Treib Modified:
4 years, 7 months ago Reviewers:
Bernhard Bauer CC:
chromium-reviews, mcwilliams+watch_chromium.org, dgn+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionVarious small NTPSnippetsService cleanups:
- Remove confusing "NTPSnippetsStorage" typedef
- Remove unimplemented method from the header
- Add state_ enum and DCHECK that Init/Shutdown are called correctly
- Clear SuggestionsService subscription on shutdown
BUG=none
Committed: https://crrev.com/1d2c78a1329d3c27eba60ed24b354f7c42b7302d
Cr-Commit-Position: refs/heads/master@{#394399}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : Shutdown() in tests #
Total comments: 8
Patch Set 4 : review #
Total comments: 3
Patch Set 5 : rebase #
Dependent Patchsets: Messages
Total messages: 25 (11 generated)
Description was changed from ========== Various NTPSnippetsService cleanups: - Remove confusing "NTPSnippetsStorage" typedef - Add did_init_/did_shutdown_ and DCHECK them - Clear SuggestionsService subscription on shutdown BUG=none ========== to ========== Various small NTPSnippetsService cleanups: - Remove confusing "NTPSnippetsStorage" typedef - Remove unimplemented method from the header - Add did_init_/did_shutdown_ and DCHECK them - Clear SuggestionsService subscription on shutdown BUG=none ==========
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/patch-status/1976153002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1976153002/20001
treib@chromium.org changed reviewers: + bauerb@chromium.org
PTAL!
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 tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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/patch-status/1976153002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1976153002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1976153002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1976153002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:215: if (!did_init_) Hm... under which circumstances would we shut down a service that has never been initialized? https://codereview.chromium.org/1976153002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/1976153002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:146: bool did_init_; Alternative would be to use a tri-state enum.
https://codereview.chromium.org/1976153002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1976153002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:215: if (!did_init_) On 2016/05/13 16:04:05, Bernhard Bauer wrote: > Hm... under which circumstances would we shut down a service that has never been > initialized? Shutdown is overridden from BrowserContextKeyedService, but Init isn't, so we shouldn't expect an exact 1:1 correspondence. If for some reason, the browser got torn down before Init was called, there might still be a Shutdown call. That said, I'm not completely sure if this actually happens in practice - I copied the exact checks from SupervisedUserService, assuming that whatever's there would be reasonable ;)
https://codereview.chromium.org/1976153002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1976153002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:215: if (!did_init_) On 2016/05/17 08:38:14, Marc Treib wrote: > On 2016/05/13 16:04:05, Bernhard Bauer wrote: > > Hm... under which circumstances would we shut down a service that has never > been > > initialized? > > Shutdown is overridden from BrowserContextKeyedService, but Init isn't, so we > shouldn't expect an exact 1:1 correspondence. If for some reason, the browser > got torn down before Init was called, there might still be a Shutdown call. > That said, I'm not completely sure if this actually happens in practice - I > copied the exact checks from SupervisedUserService, assuming that whatever's > there would be reasonable ;) Aww, I thought you would have learned by now 😉 Somewhat more srsly, SupervisedUserService is not a great example, because it has to be initialized at a specific point very early during profile loading (so that the profile is correctly marked as supervised, I think). I would be tempted to make this a DCHECK just so that we learn if this can actually happen (and I think it can't). Alternatively, we should be able to mark the factory as ServiceIsCreatedWithBrowserContext(), so that it *will* in fact be tied to profile creation more directly. (We can figure out the value of |enabled| in here instead of passing it in.) https://codereview.chromium.org/1976153002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/1976153002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:146: bool did_init_; On 2016/05/13 16:04:05, Bernhard Bauer wrote: > Alternative would be to use a tri-state enum. ^^^ WDYT?
https://codereview.chromium.org/1976153002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1976153002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:215: if (!did_init_) On 2016/05/17 09:23:40, Bernhard Bauer wrote: > On 2016/05/17 08:38:14, Marc Treib wrote: > > On 2016/05/13 16:04:05, Bernhard Bauer wrote: > > > Hm... under which circumstances would we shut down a service that has never > > been > > > initialized? > > > > Shutdown is overridden from BrowserContextKeyedService, but Init isn't, so we > > shouldn't expect an exact 1:1 correspondence. If for some reason, the browser > > got torn down before Init was called, there might still be a Shutdown call. > > That said, I'm not completely sure if this actually happens in practice - I > > copied the exact checks from SupervisedUserService, assuming that whatever's > > there would be reasonable ;) > > Aww, I thought you would have learned by now 😉 Somewhat more srsly, > SupervisedUserService is not a great example, because it has to be initialized > at a specific point very early during profile loading (so that the profile is > correctly marked as supervised, I think). > > I would be tempted to make this a DCHECK just so that we learn if this can > actually happen (and I think it can't). Alright, done. (Though really, we only care that Init isn't called twice. So if the !inited case does happen here, we'll probably just remove the DCHECK.) > Alternatively, we should be able to mark the factory as > ServiceIsCreatedWithBrowserContext(), so that it *will* in fact be tied to > profile creation more directly. (We can figure out the value of |enabled| in > here instead of passing it in.) We could, but why? We don't really care when exactly the service is created. I suppose "at profile creation" is as good as any other time, but presumably there's a reason why that isn't the default? https://codereview.chromium.org/1976153002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/1976153002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:146: bool did_init_; On 2016/05/17 09:23:40, Bernhard Bauer wrote: > On 2016/05/13 16:04:05, Bernhard Bauer wrote: > > Alternative would be to use a tri-state enum. > > ^^^ WDYT? I didn't forget, just wanted to sort out the other comment first :) Done. https://codereview.chromium.org/1976153002/diff/50001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1976153002/diff/50001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:189: DCHECK(state_ == State::NOT_INITED); DCHECK_EQ etc don't work with enum class :( (I guess we could cast everything to int, but at that point it's way less ugly to just use regular DCHECK IMO.)
lgtm https://codereview.chromium.org/1976153002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1976153002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:215: if (!did_init_) On 2016/05/17 11:21:18, Marc Treib wrote: > On 2016/05/17 09:23:40, Bernhard Bauer wrote: > > On 2016/05/17 08:38:14, Marc Treib wrote: > > > On 2016/05/13 16:04:05, Bernhard Bauer wrote: > > > > Hm... under which circumstances would we shut down a service that has > never > > > been > > > > initialized? > > > > > > Shutdown is overridden from BrowserContextKeyedService, but Init isn't, so > we > > > shouldn't expect an exact 1:1 correspondence. If for some reason, the > browser > > > got torn down before Init was called, there might still be a Shutdown call. > > > That said, I'm not completely sure if this actually happens in practice - I > > > copied the exact checks from SupervisedUserService, assuming that whatever's > > > there would be reasonable ;) > > > > Aww, I thought you would have learned by now 😉 Somewhat more srsly, > > SupervisedUserService is not a great example, because it has to be initialized > > at a specific point very early during profile loading (so that the profile is > > correctly marked as supervised, I think). > > > > I would be tempted to make this a DCHECK just so that we learn if this can > > actually happen (and I think it can't). > > Alright, done. > (Though really, we only care that Init isn't called twice. So if the !inited > case does happen here, we'll probably just remove the DCHECK.) > > > Alternatively, we should be able to mark the factory as > > ServiceIsCreatedWithBrowserContext(), so that it *will* in fact be tied to > > profile creation more directly. (We can figure out the value of |enabled| in > > here instead of passing it in.) > > We could, but why? We don't really care when exactly the service is created. I > suppose "at profile creation" is as good as any other time, but presumably > there's a reason why that isn't the default? The default is to create services lazily when they're accessed. That is a good default IMO, but we want to schedule refetching when we start. Mostly the reason why I would like to do that is to move stuff out of ProfileManager, which is starting to become this grab bag for random stuff to happen at profile creation time. (Not for this CL though.) https://codereview.chromium.org/1976153002/diff/50001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1976153002/diff/50001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:189: DCHECK(state_ == State::NOT_INITED); On 2016/05/17 11:21:18, Marc Treib wrote: > DCHECK_EQ etc don't work with enum class :( > (I guess we could cast everything to int, but at that point it's way less ugly > to just use regular DCHECK IMO.) Yuck. TBH, I'm less and less convinced that enum classes are All That. Oh, and I guess appending |state_| to the DCHECK to log it doesn't work either, because operator<< isn't defined, right?
Description was changed from ========== Various small NTPSnippetsService cleanups: - Remove confusing "NTPSnippetsStorage" typedef - Remove unimplemented method from the header - Add did_init_/did_shutdown_ and DCHECK them - Clear SuggestionsService subscription on shutdown BUG=none ========== to ========== Various small NTPSnippetsService cleanups: - Remove confusing "NTPSnippetsStorage" typedef - Remove unimplemented method from the header - Add state_ enum and DCHECK that Init/Shutdown are called correctly - Clear SuggestionsService subscription on shutdown BUG=none ==========
https://codereview.chromium.org/1976153002/diff/50001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1976153002/diff/50001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:189: DCHECK(state_ == State::NOT_INITED); On 2016/05/17 12:38:13, Bernhard Bauer wrote: > On 2016/05/17 11:21:18, Marc Treib wrote: > > DCHECK_EQ etc don't work with enum class :( > > (I guess we could cast everything to int, but at that point it's way less ugly > > to just use regular DCHECK IMO.) > > Yuck. TBH, I'm less and less convinced that enum classes are All That. > > Oh, and I guess appending |state_| to the DCHECK to log it doesn't work either, > because operator<< isn't defined, right? Yup, the missing operator<< is the reason DCHECK_EQ doesn't work. Would you prefer a non-class enum? Or alternatively, it should be possible to explicitly overload operator<< (and cast to int there). WDYT?
The CQ bit was checked by treib@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/1976153002/#ps70001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1976153002/70001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1976153002/70001
Message was sent while issue was closed.
Description was changed from ========== Various small NTPSnippetsService cleanups: - Remove confusing "NTPSnippetsStorage" typedef - Remove unimplemented method from the header - Add state_ enum and DCHECK that Init/Shutdown are called correctly - Clear SuggestionsService subscription on shutdown BUG=none ========== to ========== Various small NTPSnippetsService cleanups: - Remove confusing "NTPSnippetsStorage" typedef - Remove unimplemented method from the header - Add state_ enum and DCHECK that Init/Shutdown are called correctly - Clear SuggestionsService subscription on shutdown BUG=none ==========
Message was sent while issue was closed.
Committed patchset #5 (id:70001)
Message was sent while issue was closed.
Description was changed from ========== Various small NTPSnippetsService cleanups: - Remove confusing "NTPSnippetsStorage" typedef - Remove unimplemented method from the header - Add state_ enum and DCHECK that Init/Shutdown are called correctly - Clear SuggestionsService subscription on shutdown BUG=none ========== to ========== Various small NTPSnippetsService cleanups: - Remove confusing "NTPSnippetsStorage" typedef - Remove unimplemented method from the header - Add state_ enum and DCHECK that Init/Shutdown are called correctly - Clear SuggestionsService subscription on shutdown BUG=none Committed: https://crrev.com/1d2c78a1329d3c27eba60ed24b354f7c42b7302d Cr-Commit-Position: refs/heads/master@{#394399} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1d2c78a1329d3c27eba60ed24b354f7c42b7302d Cr-Commit-Position: refs/heads/master@{#394399} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
