|
|
Created:
3 years, 8 months ago by Marc Treib Modified:
3 years, 8 months ago Reviewers:
Peter Kasting CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionTemplateURLService: notify observers of shutdown
This allows observers that might outlive the service to unregister
themselves, and to clear out any raw pointers to the service they might
have.
In the same spirit, verify that there are no observers left during
TemplateURLService destruction.
BUG=704518
Review-Url: https://codereview.chromium.org/2815643007
Cr-Commit-Position: refs/heads/master@{#464351}
Committed: https://chromium.googlesource.com/chromium/src/+/9f5ce2d4fb3d35f5b2b22ad4f486d93b0d1acd50
Patch Set 1 #Patch Set 2 : fix #
Total comments: 3
Patch Set 3 : review #
Dependent Patchsets: Messages
Total messages: 22 (17 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
treib@chromium.org changed reviewers: + pkasting@chromium.org
PTAL! See the dependent CL (https://codereview.chromium.org/2805133004) for what motivated this. Thanks! https://codereview.chromium.org/2815643007/diff/20001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2815643007/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:876: observer.OnTemplateURLServiceShuttingDown(); We could also notify in the destructor rather than in Shutdown(), since the TemplateURLService still exists in the meantime. But after shutdown, it won't be very useful, so I think it's better to notify here already.
LGTM https://codereview.chromium.org/2815643007/diff/20001/components/search_engin... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2815643007/diff/20001/components/search_engin... components/search_engines/template_url_service.h:774: base::ObserverList<TemplateURLServiceObserver, true> model_observers_; I don't think we should change this. There's no real reason the observer list needs to be empty on destruction. It's not true that any observer is also necessarily holding a raw pointer to |this|. This will also let us avoid changing template_url_service_test_util.cc.
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! https://codereview.chromium.org/2815643007/diff/20001/components/search_engin... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2815643007/diff/20001/components/search_engin... components/search_engines/template_url_service.h:774: base::ObserverList<TemplateURLServiceObserver, true> model_observers_; On 2017/04/13 00:03:36, Peter Kasting wrote: > I don't think we should change this. There's no real reason the observer list > needs to be empty on destruction. It's not true that any observer is also > necessarily holding a raw pointer to |this|. Not necessarily, but I think that most would, since the notification they get is only "something changed". They'll have to ask the TemplateURLService to find out what exactly happened. But I'll trust your instinct here - reverted. > This will also let us avoid changing template_url_service_test_util.cc. 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 pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2815643007/#ps40001 (title: "review")
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": 40001, "attempt_start_ts": 1492074068241470, "parent_rev": "4ce69f8079d73522380aa9c7ddb1847fdd940a99", "commit_rev": "9f5ce2d4fb3d35f5b2b22ad4f486d93b0d1acd50"}
Message was sent while issue was closed.
Description was changed from ========== TemplateURLService: notify observers of shutdown This allows observers that might outlive the service to unregister themselves, and to clear out any raw pointers to the service they might have. In the same spirit, verify that there are no observers left during TemplateURLService destruction. BUG=704518 ========== to ========== TemplateURLService: notify observers of shutdown This allows observers that might outlive the service to unregister themselves, and to clear out any raw pointers to the service they might have. In the same spirit, verify that there are no observers left during TemplateURLService destruction. BUG=704518 Review-Url: https://codereview.chromium.org/2815643007 Cr-Commit-Position: refs/heads/master@{#464351} Committed: https://chromium.googlesource.com/chromium/src/+/9f5ce2d4fb3d35f5b2b22ad4f486... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/9f5ce2d4fb3d35f5b2b22ad4f486... |