Consider for ArcServiceManager to provide a getter of each ArcService's subclass. |
|||||
Issue description
Chrome Version: ToT
OS: ChromeOS.
Note: we (Luis, Yusuke and me) sometimes tempted to provide this and had a brief discussion of this topic. So, I think it's valuable to file a bug. We have nothing decided.
ArcServiceManager manages indivisual ArcService subclasses.
Now ArcServiceManager is working as a singleton, and some outer class sometimes want to access specific ArcService inheriting class's instance.
However, there is no convenient accessor for each.
At the moment, we do not have many, if there are more, we may want to revisit here.
Here are some approaches;
1) Using RTTI.
In C++11 world, the simplest way is using
unordered_map<std::type_index, std::unique_ptr<ArcService> > service_map_;
as a storage. Then;
// In header.
template<typename T>
T* GetService() {
return static_cast<T*>(GetServiceInternal(typeid(T)));
}
template<typename T>
void AddService(std::unique_ptr<T> service) {
AddServiceInternal(typeid(T), std::move(service));
}
// In source.
ArcService* GetServiceInternal(std::type_index key) {
auto iter = service_map_.find(key);
if (iter == service_map_.end())
return nullptr;
return iter->second;
}
void AddServiceInternal(std::type_index key, std::unique_ptr<T> service) {
service_map_[key] = std::move(service);
}
or something. However, type_index is not yet allowed (TBD) in Chromium.
cf) https://chromium-cpp.appspot.com/ "Type Info Enhancement"
Also, RTTI is actually discouraged to use (not strictly prohibited, though):
https://google.github.io/styleguide/cppguide.html#Run-Time_Type_Information__RTTI_
Pros: code is short enough. Do not need to edit the ArcServiceManager on adding a new ArcService subclass.
Cons: typeid is rarely used in Chromium. Style guide is discouraging.
2) Instead of using typeid, define unique "key" constant static field for each ArcService's subclass. Then, replace 1)'s type_index by "key"s.
Pros: same with above + following style guide. No typeid.
Cons: all ArcService's subclass must define key. No compiler guarantee of the relationship between "key" and class T (inheriting ArcService).
3) Using boilerplate approach.
This is conceptually simple. List all classes in the member.
And define each getters.
E.g.;
std::unique_ptr<ArcIntentHelperBridge> intent_helper_bridge_;
ArcIntentHelperBridge* intent_helper_bridge() { return ...get(); }
Pros: very straightforward.
Cons: long boilerplate is needed. Always need to edit ArcServiceManager on adding a new ArcService subclass.
Any alternatives?
,
Dec 9 2016
IIRC, our code is compiled with -fno-rtti except for a few third_party/ libraries like ICU. I guess this rules out 1). > However, there is no convenient accessor for each. > At the moment, we do not have many, if there are more, we may want to revisit here. Yes the current code is a bit messy, but at the same time, if we implement one of these (1, 2, or 3), chrome/ code will depend on many more interfaces of the services, which might introduce some maintenance burden in the long run. > Any alternatives? (Just a random idea.) Add a method like DoX() to ArcService possibly with an empty implementation, add DoXOnServices() to ArcServiceManager, and implement it like for (auto& s : services_) s.DoX(); ?
,
Dec 9 2016
Re #c2 ah that's true. That eliminates 3) then :/ If we use a templating approach like the proposed in 1) or 2), chrome/ won't artificially add new dependencies, since they will only be needed if they are compiled by the template. The caller would include the dependency anyways, so we wouldn't be adding anything by implementing 1) or 2). Just re-read #c0 and it seems like what I proposed in #c1 is basically option 2), just a bit worse due to the virtual dispatch. That means that the only viable approach is 2).
,
Dec 12 2016
Re #1 type safetiness: 1) and 2) are well-known technique named type-erasure. The type safetiness is encapsulated in ArcServiceManager's implementation. It means, if we see from outside of the ArcServiceManager, it is typesafe. Re #2 no-rtti: Oh, good point, and you're right. https://cs.chromium.org/chromium/src/build/config/BUILDCONFIG.gn?q=no_rtti+components&sq=package:chromium&l=483&dr=C Thank you for pointing this out. Re #2: Clarification, what do you mean about "many more interfaces"? If you mean, a list of #include "components/arc/foo/foo_service.h" in arc_service_manager.h, as Luis pointed in #3, option 1) or 2) should resolve it. If you mean, chrome/ code should only depends on components/arc/arc_service_manager.h and arc_service.h (i.e., from outside of ARC service, ArcServiceManager should be only an accessible interface), then your DoX() is the way to resolve it. Which did you mean? Re #2 DoX(): TBH, I'm slightly worried about introducing a mixed gigantic ArcService::DoX() method list and ArcServiceManager::Observer callback list, for longer term maintenance. WDYT? Anyway, it's up to how do we want to design ARC service interface to the outside services, I think. Re #3 GetName(): yes 2) is somehow compatible with your GetName() approach.
,
Dec 14 2016
I meant the latter, and proposed DoX() solution to prevent chrome/ depending on interfaces other than ArcService and ArcServiceManager. > worried about introducing a mixed gigantic ArcService::DoX() method list and ArcServiceManager::Observer callback list Ok, yet another proposal :) What about the following? * Keep ArcService as it is. Don't add DoX() to the class. * Instead, add more observer methods to ArcServiceManager::Observer as needed with careful code review. This will probably meet the needs because what ArcService subclasses mainly do is to receive information from ARC side (by implementing each mojom's XyzInstance interface.) They are usually passive ones. (A tangentially related topic: currently, to provide the Observer interface (OnAppsUpdated), ArcServiceManager is derived from arc::ArcIntentHelperObserver and #includes the observer's header file. However, I think we can remove both if we want. I didn't sent this out for a review but have a CL: https://codereview.chromium.org/2538263005/ )
,
Dec 14 2016
> Ok, yet another proposal :) What about the following? > > * Keep ArcService as it is. Don't add DoX() to the class. > * Instead, add more observer methods to ArcServiceManager::Observer as needed with careful code review. Thank you for proposal. I'm ok with this idea, too. Note that this has two cons; - As we do for ArcIntentHelperObserver, we cannot do clean shutdown as commented in ArcServiceLauncher::Initialize() (i.e. ArcService may be deleted with non-empty observer_list.) - We accepted this assuming it is encapsulated in ArcServiceManager. - Anyway, ArcServiceManager::Observer may contain longer callback list. - IIUC, to review this list is the Yusuke's main point, so that should be ok and practically limited. Luis, any thoughts? > https://codereview.chromium.org/2538263005/ I like the idea :-)
,
Dec 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7daca36ac601b5c720b71c1bd894d30d8b565393 commit 7daca36ac601b5c720b71c1bd894d30d8b565393 Author: yusukes <yusukes@chromium.org> Date: Thu Dec 15 02:51:47 2016 Simplify ArcServiceManager by stop deriving it from ArcIntentHelperObserver It is slightly weird for ArcServiceManager to handle (only) one of the ArcService instances it has in a special way with inheritance, and this CL fixes that. ArcServiceManager looks more neutral now and no longer has to #include arc_intent_helper_observer.h on the header side. This removes all virtual member functions from ArcServiceManager too. Instead, do the same thing with a pair of member variable and its getter function, which is more consistent with the existing ArcServiceManager code. All the implementation details go to the .cc side. This is a follow-up CL for https://codereview.chromium.org/2487623002/. BUG= 672840 TEST=try Review-Url: https://codereview.chromium.org/2538263005 Cr-Commit-Position: refs/heads/master@{#438718} [modify] https://crrev.com/7daca36ac601b5c720b71c1bd894d30d8b565393/chrome/browser/chromeos/arc/arc_service_launcher.cc [modify] https://crrev.com/7daca36ac601b5c720b71c1bd894d30d8b565393/components/arc/arc_service_manager.cc [modify] https://crrev.com/7daca36ac601b5c720b71c1bd894d30d8b565393/components/arc/arc_service_manager.h [modify] https://crrev.com/7daca36ac601b5c720b71c1bd894d30d8b565393/components/arc/intent_helper/arc_intent_helper_observer.h
,
Dec 15 2016
One more CL for making ArcServiceManager::Observer easier to maintain is up for review: https://codereview.chromium.org/2579083002/ What about closing this bug as WontFix for now, until we run into a case where ArcServiceManager::Observer is not sufficient?
,
Dec 15 2016
SGTM to delay for when it stops being sufficient, or we are cramming too many disparate stuff in there (say... > 5 events).
,
Dec 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4af2926f8b00bd4f18eedab66170dbd021624ce6 commit 4af2926f8b00bd4f18eedab66170dbd021624ce6 Author: yusukes <yusukes@chromium.org> Date: Fri Dec 16 03:42:28 2016 Rename OnAppsUpdated to OnIntentFiltersUpdated Previously, ArcIntentHelperBridge::OnIntentFiltersUpdated() called OnAppsUpdated() for each observer, but this was probably just confusing. This CL renames the observer method to fix the inconsistency and to make it easier to maintain the ArcServiceManager::Observer interface. This is a follow-up CL for https://codereview.chromium.org/2487623002/. BUG= 672840 TEST=try Review-Url: https://codereview.chromium.org/2579083002 Cr-Commit-Position: refs/heads/master@{#439008} [modify] https://crrev.com/4af2926f8b00bd4f18eedab66170dbd021624ce6/chrome/browser/chromeos/extensions/file_manager/event_router.cc [modify] https://crrev.com/4af2926f8b00bd4f18eedab66170dbd021624ce6/chrome/browser/chromeos/extensions/file_manager/event_router.h [modify] https://crrev.com/4af2926f8b00bd4f18eedab66170dbd021624ce6/components/arc/arc_service_manager.cc [modify] https://crrev.com/4af2926f8b00bd4f18eedab66170dbd021624ce6/components/arc/arc_service_manager.h [modify] https://crrev.com/4af2926f8b00bd4f18eedab66170dbd021624ce6/components/arc/intent_helper/arc_intent_helper_bridge.cc [modify] https://crrev.com/4af2926f8b00bd4f18eedab66170dbd021624ce6/components/arc/intent_helper/arc_intent_helper_bridge_unittest.cc [modify] https://crrev.com/4af2926f8b00bd4f18eedab66170dbd021624ce6/components/arc/intent_helper/arc_intent_helper_observer.h
,
Dec 16 2016
Based on comment #6 and #9, let me close this for now. Feel free to reopen if needs arise.
,
Jan 10 2017
Ran into this in the context of https://codereview.chromium.org/2580303002/ . I haven't been following the whole development of that feature, but based on its incremental nature, maybe by itself it'll exceed the > 5 event threshold, so I'm proposing nya@ to change his code as-if this were implemented and we can do this in parallel to avoid blocking him. I decided to go with hidehiko@'s approach #2 with the small addition of a bit of templating magic to avoid having to add a key to _all_ ArcServices (and thus reducing the number of cons to just one): https://codereview.chromium.org/2622843002
,
Jan 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ce096d5651c3984e1ed5a260e11ba77d5b37d580 commit ce096d5651c3984e1ed5a260e11ba77d5b37d580 Author: lhchavez <lhchavez@chromium.org> Date: Wed Jan 11 17:04:28 2017 arc: Provide a per-service getter for ArcServiceManager This change adds getters for individual ArcServices from ArcServiceManager. This prevents ArcServiceManager::Observer from becoming a place to put random stuff to observe that would be better suited for individual services to expose. BUG= 672840 TEST=git cl try Review-Url: https://codereview.chromium.org/2622843002 Cr-Commit-Position: refs/heads/master@{#442934} [modify] https://crrev.com/ce096d5651c3984e1ed5a260e11ba77d5b37d580/components/arc/BUILD.gn [modify] https://crrev.com/ce096d5651c3984e1ed5a260e11ba77d5b37d580/components/arc/arc_service_manager.cc [modify] https://crrev.com/ce096d5651c3984e1ed5a260e11ba77d5b37d580/components/arc/arc_service_manager.h [add] https://crrev.com/ce096d5651c3984e1ed5a260e11ba77d5b37d580/components/arc/arc_service_manager_unittest.cc
,
Jan 11 2017
There's a bit of cleanup needed to remove the intent-filter-related stuff from current ArcServiceManager's observer, but we can mark this as fixed.
,
Jan 11 2017
Doing the cleanup. (good side work for a constable)
,
Jan 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/aae2a1b19c41c9e56085365a72593a9332098b48 commit aae2a1b19c41c9e56085365a72593a9332098b48 Author: yusukes <yusukes@chromium.org> Date: Thu Jan 12 04:14:58 2017 arc: Remove ArcServiceManager::Observer The interface is no longer needed since we've added GetService<T>() to the manager class. BUG= 672840 TEST=try Review-Url: https://codereview.chromium.org/2623273003 Cr-Commit-Position: refs/heads/master@{#443152} [modify] https://crrev.com/aae2a1b19c41c9e56085365a72593a9332098b48/chrome/browser/chromeos/arc/arc_service_launcher.cc [modify] https://crrev.com/aae2a1b19c41c9e56085365a72593a9332098b48/chrome/browser/chromeos/extensions/file_manager/event_router.cc [modify] https://crrev.com/aae2a1b19c41c9e56085365a72593a9332098b48/chrome/browser/chromeos/extensions/file_manager/event_router.h [modify] https://crrev.com/aae2a1b19c41c9e56085365a72593a9332098b48/chrome/browser/chromeos/note_taking_helper.cc [modify] https://crrev.com/aae2a1b19c41c9e56085365a72593a9332098b48/chrome/browser/chromeos/note_taking_helper.h [modify] https://crrev.com/aae2a1b19c41c9e56085365a72593a9332098b48/components/arc/arc_service_manager.cc [modify] https://crrev.com/aae2a1b19c41c9e56085365a72593a9332098b48/components/arc/arc_service_manager.h [modify] https://crrev.com/aae2a1b19c41c9e56085365a72593a9332098b48/components/arc/arc_service_manager_unittest.cc [modify] https://crrev.com/aae2a1b19c41c9e56085365a72593a9332098b48/components/arc/intent_helper/arc_intent_helper_bridge.cc [modify] https://crrev.com/aae2a1b19c41c9e56085365a72593a9332098b48/components/arc/intent_helper/arc_intent_helper_bridge.h
,
Jan 12 2017
Done
,
Mar 1 2017
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by lhchavez@chromium.org
, Dec 9 2016Out of the three alternatives, the only really typesafe one is 3), so I'm a bit more amenable to that. We _can_ use 1) and add some additional checks to ensure that what we got out of the map really is the type we want (maybe NOTREACHED() + return nullptr), but 1) cannot be used as-is. But we can add a small twist to your proposal to be able to use it today: class ArcService { virtual const std::string& GetName() = 0; }; and then have the mapping by name instead of type_index? That would allow us to implement 1) with no RTTI.