Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2754703005:
[Device Discovery] Make DialRegistry a Singleton (Closed)
|
Created:
3 years, 9 months ago by zhaobin Modified:
3 years, 9 months ago CC:
chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org, feature-media-reviews_chromium.org, media-router+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Device Discovery] Make DialRegistry a Singleton
Make DialRegistry a Singleton so Extension API functions and DialMediaSinkService reference the same instance of DialRegistry.
BUG=687375
Review-Url: https://codereview.chromium.org/2754703005
Cr-Commit-Position: refs/heads/master@{#459539}
Committed: https://chromium.googlesource.com/chromium/src/+/936e728a2bef7552fb07018d7075a5e996332bea
Patch Set 1 #
Total comments: 5
Patch Set 2 : make DialRegistry singleton #
Total comments: 1
Patch Set 3 : change DialRegistry to leaky Singleton #
Total comments: 4
Patch Set 4 : resolve code review comments from Mark #
Total comments: 6
Patch Set 5 : resolve code review comments from Derek #Patch Set 6 : add TODO and fix BUILD.gn #Patch Set 7 : fix mac and windows compile errors #
Depends on Patchset: Messages
Total messages: 39 (23 generated)
zhaobin@chromium.org changed reviewers: + imcheng@chromium.org, mfoltz@chromium.org
Looks good - however maybe it doesn't need to be profile-keyed at all? Derek, WDYT? https://codereview.chromium.org/2754703005/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/dial/dial_api.h (right): https://codereview.chromium.org/2754703005/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/dial_api.h:35: // TODO(mfoltz): The threading model for this API needs to be rethought. At a Since the DialRegistry is going to now be shared among multiple clients running on the IO and UI threads, this paragraph no longer applies and can be removed. It would be nice to simplify this API to post fewer tasks to/from the IO thread to use the registry, but if the plan is to deprecate and remove chrome.dial then maybe it's no longer worth it. https://codereview.chromium.org/2754703005/diff/1/chrome/browser/media/router... File chrome/browser/media/router/discovery/dial/dial_registry.h (right): https://codereview.chromium.org/2754703005/diff/1/chrome/browser/media/router... chrome/browser/media/router/discovery/dial/dial_registry.h:33: class DialRegistry : public RefcountedKeyedService, I don't think the DialRegistry needs to be a keyed service - it should have the same set of devices across profiles, so only one needs to be created per browser process. Maybe just make it base::RefCounted<>? https://codereview.chromium.org/2754703005/diff/1/chrome/browser/media/router... File chrome/browser/media/router/discovery/dial/dial_registry_factory.h (right): https://codereview.chromium.org/2754703005/diff/1/chrome/browser/media/router... chrome/browser/media/router/discovery/dial/dial_registry_factory.h:24: static scoped_refptr<DialRegistry> GetForBrowserContext( If DialRegistry is not profile-keyed, this could just be DialRegistry::Get() which manages a base::Singleton - no factory object required.
Yeah, it seems DialRegistry does not depend on any BrowserContext logic. We should try making it a Singleton.
Make DialRegistry a singleton. https://codereview.chromium.org/2754703005/diff/1/chrome/browser/media/router... File chrome/browser/media/router/discovery/dial/dial_registry.h (right): https://codereview.chromium.org/2754703005/diff/1/chrome/browser/media/router... chrome/browser/media/router/discovery/dial/dial_registry.h:33: class DialRegistry : public RefcountedKeyedService, On 2017/03/18 18:04:58, mark a. foltz wrote: > I don't think the DialRegistry needs to be a keyed service - it should have the > same set of devices across profiles, so only one needs to be created per browser > process. Maybe just make it base::RefCounted<>? Code removed. https://codereview.chromium.org/2754703005/diff/1/chrome/browser/media/router... File chrome/browser/media/router/discovery/dial/dial_registry_factory.h (right): https://codereview.chromium.org/2754703005/diff/1/chrome/browser/media/router... chrome/browser/media/router/discovery/dial/dial_registry_factory.h:24: static scoped_refptr<DialRegistry> GetForBrowserContext( On 2017/03/18 18:04:58, mark a. foltz wrote: > If DialRegistry is not profile-keyed, this could just be DialRegistry::Get() > which manages a base::Singleton - no factory object required. Done.
LGTM We may not want the dtor to run on the UI thread as it could be a source of shutdown crashes. In that case it could be a Leaky singleton (not destroyed at shutdown). What do other Singleton objects on the IO thread do? Please update patch description with current design. m. https://codereview.chromium.org/2754703005/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_registry.cc (right): https://codereview.chromium.org/2754703005/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_registry.cc:193: repeating_timer_.reset(); Thanks for cleaning this up :)
Description was changed from ========== [Device Discovery] Make DialRegistry a RefcountedKeyedService Make DialRegistry a RefcountedKeyedService so Extension API functions and DialMediaSinkService reference the same instance of DialRegistry. BUG=687375 ========== to ========== [Device Discovery] Make DialRegistry a RefcountedKeyedService Make DialRegistry a Singleton so Extension API functions and DialMediaSinkService reference the same instance of DialRegistry. BUG=687375 ==========
On 2017/03/22 18:37:25, mark a. foltz wrote: > LGTM > > We may not want the dtor to run on the UI thread as it could be a source of > shutdown crashes. In that case it could be a Leaky singleton (not destroyed at > shutdown). What do other Singleton objects on the IO thread do? > > Please update patch description with current design. > > m. > > https://codereview.chromium.org/2754703005/diff/20001/chrome/browser/media/ro... > File chrome/browser/media/router/discovery/dial/dial_registry.cc (right): > > https://codereview.chromium.org/2754703005/diff/20001/chrome/browser/media/ro... > chrome/browser/media/router/discovery/dial/dial_registry.cc:193: > repeating_timer_.reset(); > Thanks for cleaning this up :) Find a thread discussing similar issue and people suggested using LeakySingletonTraits: https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/j5VfyEVTZZ8... Changed DialRegistry to a leaky singleton.
https://codereview.chromium.org/2754703005/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_registry.cc (right): https://codereview.chromium.org/2754703005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_registry.cc:53: NetworkChangeNotifier::AddNetworkChangeObserver(this); I would add a comment that this is a leaky singleton, so there's no code to remove |this| as an observer. https://codereview.chromium.org/2754703005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_registry.cc:58: NetworkChangeNotifier::RemoveNetworkChangeObserver(this); If you're using a leaky singleton then the dtor won't be called at all. So this can be made a no-op.
https://codereview.chromium.org/2754703005/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_registry.cc (right): https://codereview.chromium.org/2754703005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_registry.cc:53: NetworkChangeNotifier::AddNetworkChangeObserver(this); On 2017/03/22 21:52:24, mark a. foltz wrote: > I would add a comment that this is a leaky singleton, so there's no code to > remove |this| as an observer. Done. https://codereview.chromium.org/2754703005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_registry.cc:58: NetworkChangeNotifier::RemoveNetworkChangeObserver(this); On 2017/03/22 21:52:24, mark a. foltz wrote: > If you're using a leaky singleton then the dtor won't be called at all. So this > can be made a no-op. Done.
lgtm https://codereview.chromium.org/2754703005/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/dial/dial_api.cc (right): https://codereview.chromium.org/2754703005/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/dial/dial_api.cc:44: dial_registry_->StopPeriodicDiscovery(); Wouldn't that affect other observers of DialRegistry? It seems it should unregister itself from DialRegistry instead? https://codereview.chromium.org/2754703005/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_registry.h (right): https://codereview.chromium.org/2754703005/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_registry.h:105: // Create the DIAL registry and pass a reference to allow it to notify on nit: Remove this comment as it's not very useful. Move the second part of sentence to RegisterObserver with some rewording.
The CQ bit was checked by zhaobin@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...
https://codereview.chromium.org/2754703005/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/dial/dial_api.cc (right): https://codereview.chromium.org/2754703005/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/dial/dial_api.cc:44: dial_registry_->StopPeriodicDiscovery(); On 2017/03/23 23:06:00, imcheng wrote: > Wouldn't that affect other observers of DialRegistry? It seems it should > unregister itself from DialRegistry instead? Yes. It is fine for now since DialAPI is the only observer; and dtor is invoked when IO thread tears down (We only tear down IO thread when we close the browser?) Yes, we should UnregisterObserver() instead. StopPeriodicDiscovery() is a temp workaround here. Current impl does not StopDiscovery() when there is no observer (it only StopDiscovery() when there is no listener). Need to do some refactoring regarding when to start/stop discovery in DialRegistry when DialMediaSinkService is ready. Trying to keep this patch simple for now... https://codereview.chromium.org/2754703005/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_registry.h (right): https://codereview.chromium.org/2754703005/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_registry.h:105: // Create the DIAL registry and pass a reference to allow it to notify on On 2017/03/23 23:06:00, imcheng wrote: > nit: Remove this comment as it's not very useful. Move the second part of > sentence to RegisterObserver with some rewording. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2754703005/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/dial/dial_api.cc (right): https://codereview.chromium.org/2754703005/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/dial/dial_api.cc:44: dial_registry_->StopPeriodicDiscovery(); On 2017/03/24 00:42:35, zhaobin wrote: > On 2017/03/23 23:06:00, imcheng wrote: > > Wouldn't that affect other observers of DialRegistry? It seems it should > > unregister itself from DialRegistry instead? > > Yes. It is fine for now since DialAPI is the only observer; and dtor is invoked > when IO thread tears down (We only tear down IO thread when we close the > browser?) > > Yes, we should UnregisterObserver() instead. StopPeriodicDiscovery() is a temp > workaround here. Current impl does not StopDiscovery() when there is no observer > (it only StopDiscovery() when there is no listener). Need to do some refactoring > regarding when to start/stop discovery in DialRegistry when DialMediaSinkService > is ready. Trying to keep this patch simple for now... > Ok. Seems like the easiest way would be to have this class keep a listener count and Register/UnregisterObserver as needed. Could you add a TODO here?
The CQ bit was checked by zhaobin@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 zhaobin@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: linux_chromium_asan_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 zhaobin@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.
https://codereview.chromium.org/2754703005/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/dial/dial_api.cc (right): https://codereview.chromium.org/2754703005/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/dial/dial_api.cc:44: dial_registry_->StopPeriodicDiscovery(); On 2017/03/24 00:54:24, imcheng wrote: > On 2017/03/24 00:42:35, zhaobin wrote: > > On 2017/03/23 23:06:00, imcheng wrote: > > > Wouldn't that affect other observers of DialRegistry? It seems it should > > > unregister itself from DialRegistry instead? > > > > Yes. It is fine for now since DialAPI is the only observer; and dtor is > invoked > > when IO thread tears down (We only tear down IO thread when we close the > > browser?) > > > > Yes, we should UnregisterObserver() instead. StopPeriodicDiscovery() is a temp > > workaround here. Current impl does not StopDiscovery() when there is no > observer > > (it only StopDiscovery() when there is no listener). Need to do some > refactoring > > regarding when to start/stop discovery in DialRegistry when > DialMediaSinkService > > is ready. Trying to keep this patch simple for now... > > > > Ok. Seems like the easiest way would be to have this class keep a listener count > and Register/UnregisterObserver as needed. Could you add a TODO here? Done.
The CQ bit was checked by zhaobin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org, imcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2754703005/#ps120001 (title: "fix mac and windows compile errors")
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": 1490387212286750,
"parent_rev": "420b56c73fae6392e98998f0588af0f54d79436d", "commit_rev":
"936e728a2bef7552fb07018d7075a5e996332bea"}
Message was sent while issue was closed.
Description was changed from ========== [Device Discovery] Make DialRegistry a RefcountedKeyedService Make DialRegistry a Singleton so Extension API functions and DialMediaSinkService reference the same instance of DialRegistry. BUG=687375 ========== to ========== [Device Discovery] Make DialRegistry a RefcountedKeyedService Make DialRegistry a Singleton so Extension API functions and DialMediaSinkService reference the same instance of DialRegistry. BUG=687375 Review-Url: https://codereview.chromium.org/2754703005 Cr-Commit-Position: refs/heads/master@{#459539} Committed: https://chromium.googlesource.com/chromium/src/+/936e728a2bef7552fb07018d7075... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/936e728a2bef7552fb07018d7075...
Message was sent while issue was closed.
On 2017/03/24 at 20:35:22, commit-bot wrote: > Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/936e728a2bef7552fb07018d7075... Great! Do you mind updating the patch description post-facto? It still talks about a RefCountedKeyedService.
Message was sent while issue was closed.
Description was changed from ========== [Device Discovery] Make DialRegistry a RefcountedKeyedService Make DialRegistry a Singleton so Extension API functions and DialMediaSinkService reference the same instance of DialRegistry. BUG=687375 Review-Url: https://codereview.chromium.org/2754703005 Cr-Commit-Position: refs/heads/master@{#459539} Committed: https://chromium.googlesource.com/chromium/src/+/936e728a2bef7552fb07018d7075... ========== to ========== [Device Discovery] Make DialRegistry a Singleton Make DialRegistry a Singleton so Extension API functions and DialMediaSinkService reference the same instance of DialRegistry. BUG=687375 Review-Url: https://codereview.chromium.org/2754703005 Cr-Commit-Position: refs/heads/master@{#459539} Committed: https://chromium.googlesource.com/chromium/src/+/936e728a2bef7552fb07018d7075... ==========
Message was sent while issue was closed.
On 2017/03/24 21:09:59, mark a. foltz wrote: > On 2017/03/24 at 20:35:22, commit-bot wrote: > > Committed patchset #7 (id:120001) as > https://chromium.googlesource.com/chromium/src/+/936e728a2bef7552fb07018d7075... > > Great! Do you mind updating the patch description post-facto? It still talks > about a RefCountedKeyedService. Done. Sorry for missing that one... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
