Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2622843002:
arc: Provide a per-service getter for ArcServiceManager (Closed)
|
Created:
3 years, 11 months ago by Luis Héctor Chávez Modified:
3 years, 11 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, yusukes+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: 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}
Committed: https://chromium.googlesource.com/chromium/src/+/ce096d5651c3984e1ed5a260e11ba77d5b37d580
Patch Set 1 #Patch Set 2 : Moved as much of the impl as possible to the .cc file #Patch Set 3 : I forgot how to ::type #
Total comments: 27
Patch Set 4 : Streamlined template magic and added tests #Patch Set 5 : Reworded a comment #
Total comments: 10
Patch Set 6 : Addressed feedback #
Total comments: 6
Patch Set 7 : Added a comment. #
Messages
Total messages: 34 (19 generated)
The CQ bit was checked by lhchavez@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 checked by lhchavez@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...
yusukes@chromium.org changed reviewers: + yusukes@chromium.org
https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.cc:86: DCHECK(named_services_.find(name) == named_services_.end()); What about making this a CHECK? Otherwise, L87 will be just no-op on production when two or more services have the same name, and such a mistake may be difficult to find/debug. DCHECK(thread_checker_.CalledOnValidThread()); const bool result = named_services_.insert(std::make_pair(name, std::move(service))).second; CHECK(result) << name << " is used twice"; } https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.h:35: // static const char Name[]; The variable name 'Name' doesn't seem to follow the coding style (it looks like a type while it's actually not.) Also, since ARC service classes tend to implement multiple interfaces from other namespaces like ash, adding a prefix to the special member seems to be a safer choice. Maybe 'static const char kArcServiceName[];' or something like that? https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.h:46: typedef char No[1]; nit: do we have a reason to skip 0? No can be [0] and Yes can be [1]. https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.h:50: static Yes& test(decltype(U::Name) *); nit: no space before *, maybe? https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.h:100: AddNamedServiceInternal(std::string(T::Name), std::move(service)); nit: the cast (to string) seems unnecessary https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.h:103: // Gets the named service from the managed services list. For developers who don't want to read the template magic, it's probably better to explicitly note something like 'You can call GetService<YourArcService>() only when YourArcService has a static member variable called kArcServiceName[].' https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.h:105: T* GetService() { I think GetService<T>() is easy to test. How about adding some unit tests?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hidehiko@chromium.org changed reviewers: + hidehiko@chromium.org
https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.cc:87: named_services_.insert(std::make_pair(name, std::move(service))); emplace? https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.h:43: template <typename T> Optional: How about minimizing template magic? E.g.; // GetName(arc_service) returns its corresponding kArcServiceName if exists, or empty string. // If T::kArcServiceName exists, this is used. template<typename T> auto GetName(T* unused) -> decltype(T::kArcServiceName, std::string()) { return T::kArcServiceName; } // Otherwise, this is used. std::string GetName(...) { return std::string(); } So, in AddService will be; template<typename T> void AddService(std::unique_ptr<T> service) { std::string name = GetName(service.get()); AddServiceInternal(name, std::move(service)); } template<typename T> T* GetService() { // Let compiler report an error for GetService<T> // if T does not have kArcServiceName. return GetServiceInternal(T::kArcServiceName); } in .cc; void AddServiceInternal(const std::string& name, std::unqiue_ptr<ArcService> service) { if (name.empty()) { services_.push_back(std::move(service)); } else { named_services_.emplace(name, std::move(service)); } } void GetServiceInternal(const std::string& name) { ... } https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.h:46: typedef char No[1]; On 2017/01/10 07:11:11, Yusuke Sato wrote: > nit: do we have a reason to skip 0? No can be [0] and Yes can be [1]. FYI: 0 length is illegal, IIUC. http://stackoverflow.com/questions/6180012/array-with-size-0 Anyway, let's use std::true_type and std::false_type instead. https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.h:50: static Yes& test(decltype(U::Name) *); On 2017/01/10 07:11:11, Yusuke Sato wrote: > nit: no space before *, maybe? Optional: Even in this way, trailing return type can be used. https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.h:56: enum : bool { HasName = sizeof(test<T>(nullptr)) == sizeof(Yes) }; With std::{true,false}_type, constexpr bool kValue = decltype(test<T>(nullptr))::value; looks simpler? https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.h:103: // Gets the named service from the managed services list. On 2017/01/10 07:11:11, Yusuke Sato wrote: > For developers who don't want to read the template magic, it's probably better > to explicitly note something like 'You can call GetService<YourArcService>() > only when YourArcService has a static member variable called kArcServiceName[].' Also, could you explicitly comment that, this will report a compile error if YourArcService::kArcServiceName is not declared? https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.h:153: std::unordered_map<std::string, std::unique_ptr<ArcService>> named_services_; Optional: how about mergine services_ and named_services_? // Unnamed service's name is "" (empty string). All non-empty key must be unique per service. unordered_multimap<std::string, std::unique_ptr<ArcService>> services_; I think, the cost should be ignoreable. WDYT?
The CQ bit was checked by lhchavez@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...
yusukes@ PTAL https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.cc:86: DCHECK(named_services_.find(name) == named_services_.end()); On 2017/01/10 07:11:10, Yusuke Sato wrote: > What about making this a CHECK? Otherwise, L87 will be just no-op on production > when two or more services have the same name, and such a mistake may be > difficult to find/debug. > > DCHECK(thread_checker_.CalledOnValidThread()); > const bool result = named_services_.insert(std::make_pair(name, > std::move(service))).second; > CHECK(result) << name << " is used twice"; > } CHECKs are frowned upon outside of base/. I will add some logging, though. https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.cc:87: named_services_.insert(std::make_pair(name, std::move(service))); On 2017/01/10 08:03:12, hidehiko wrote: > emplace? emplace is not supported for maps: https://chromium-cpp.appspot.com/#library-whitelist https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.h:35: // static const char Name[]; On 2017/01/10 07:11:11, Yusuke Sato wrote: > The variable name 'Name' doesn't seem to follow the coding style (it looks like > a type while it's actually not.) Also, since ARC service classes tend to > implement multiple interfaces from other namespaces like ash, adding a prefix to > the special member seems to be a safer choice. > > Maybe 'static const char kArcServiceName[];' or something like that? Done. https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.h:43: template <typename T> On 2017/01/10 08:03:12, hidehiko wrote: > Optional: How about minimizing template magic? E.g.; > > // GetName(arc_service) returns its corresponding kArcServiceName if exists, or > empty string. > > // If T::kArcServiceName exists, this is used. > template<typename T> > auto GetName(T* unused) -> decltype(T::kArcServiceName, std::string()) { > return T::kArcServiceName; > } > > // Otherwise, this is used. > std::string GetName(...) { > return std::string(); > } > > So, in AddService will be; > > template<typename T> > void AddService(std::unique_ptr<T> service) { > std::string name = GetName(service.get()); > AddServiceInternal(name, std::move(service)); > } > > template<typename T> > T* GetService() { > // Let compiler report an error for GetService<T> > // if T does not have kArcServiceName. > return GetServiceInternal(T::kArcServiceName); > } > > in .cc; > > void AddServiceInternal(const std::string& name, > std::unqiue_ptr<ArcService> service) { > if (name.empty()) { > services_.push_back(std::move(service)); > } else { > named_services_.emplace(name, std::move(service)); > } > } > > void GetServiceInternal(const std::string& name) { > ... > } Simplified the first function a bit, but done. https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.h:46: typedef char No[1]; On 2017/01/10 08:03:12, hidehiko wrote: > On 2017/01/10 07:11:11, Yusuke Sato wrote: > > nit: do we have a reason to skip 0? No can be [0] and Yes can be [1]. > > FYI: 0 length is illegal, IIUC. > http://stackoverflow.com/questions/6180012/array-with-size-0 > > Anyway, let's use std::true_type and std::false_type instead. This is a commonly-used Method Detector idiom: https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Member_Detector, but the alternative seems to work, so I'll use it. https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.h:50: static Yes& test(decltype(U::Name) *); On 2017/01/10 08:03:12, hidehiko wrote: > On 2017/01/10 07:11:11, Yusuke Sato wrote: > > nit: no space before *, maybe? > > Optional: Even in this way, trailing return type can be used. Acknowledged. https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.h:56: enum : bool { HasName = sizeof(test<T>(nullptr)) == sizeof(Yes) }; On 2017/01/10 08:03:12, hidehiko wrote: > With std::{true,false}_type, > > constexpr bool kValue = decltype(test<T>(nullptr))::value; > > looks simpler? Acknowledged. https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.h:100: AddNamedServiceInternal(std::string(T::Name), std::move(service)); On 2017/01/10 07:11:11, Yusuke Sato wrote: > nit: the cast (to string) seems unnecessary Done. https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.h:103: // Gets the named service from the managed services list. On 2017/01/10 08:03:12, hidehiko wrote: > On 2017/01/10 07:11:11, Yusuke Sato wrote: > > For developers who don't want to read the template magic, it's probably better > > to explicitly note something like 'You can call GetService<YourArcService>() > > only when YourArcService has a static member variable called > kArcServiceName[].' > > Also, could you explicitly comment that, this will report a compile error if > YourArcService::kArcServiceName is not declared? Done. https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.h:105: T* GetService() { On 2017/01/10 07:11:11, Yusuke Sato wrote: > I think GetService<T>() is easy to test. How about adding some unit tests? Done. https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.h:153: std::unordered_map<std::string, std::unique_ptr<ArcService>> named_services_; On 2017/01/10 08:03:12, hidehiko wrote: > Optional: how about mergine services_ and named_services_? > > // Unnamed service's name is "" (empty string). All non-empty key must be unique > per service. > unordered_multimap<std::string, std::unique_ptr<ArcService>> services_; > > I think, the cost should be ignoreable. WDYT? Done.
LGTM with some requests: https://codereview.chromium.org/2622843002/diff/80001/components/arc/arc_serv... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/2622843002/diff/80001/components/arc/arc_serv... components/arc/arc_service_manager.cc:92: DCHECK(!name.empty()); This may actually happen by developer's mistake. LOG(ERROR) and return nullptr, maybe with a unit test, might make developers' life easier? https://codereview.chromium.org/2622843002/diff/80001/components/arc/arc_serv... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/2622843002/diff/80001/components/arc/arc_serv... components/arc/arc_service_manager.h:31: // If an ArcService is declared with a Name, e.g.: s/Name/name/ is better now. https://codereview.chromium.org/2622843002/diff/80001/components/arc/arc_serv... components/arc/arc_service_manager.h:47: return T::kArcServiceName; LOG(ERROR) when T::kArcServiceName is an empty string? https://codereview.chromium.org/2622843002/diff/80001/components/arc/arc_serv... File components/arc/arc_service_manager_unittest.cc (right): https://codereview.chromium.org/2622843002/diff/80001/components/arc/arc_serv... components/arc/arc_service_manager_unittest.cc:38: const char NamedService::kArcServiceName[] = nit: space between L37 and 38 https://codereview.chromium.org/2622843002/diff/80001/components/arc/arc_serv... components/arc/arc_service_manager_unittest.cc:100: // Named services can only be added once, but can still be retrieved. Could you also check that SecondNamedService can be added after L111 (and they can be retrieved separately with GetService?)
https://codereview.chromium.org/2622843002/diff/80001/components/arc/arc_serv... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/2622843002/diff/80001/components/arc/arc_serv... components/arc/arc_service_manager.cc:92: DCHECK(!name.empty()); On 2017/01/10 19:30:57, Yusuke Sato wrote: > This may actually happen by developer's mistake. LOG(ERROR) and return nullptr, > maybe with a unit test, might make developers' life easier? The only way that is possible is if they would set an empty string as the name. Hopefully we catch those at review-time but Done. https://codereview.chromium.org/2622843002/diff/80001/components/arc/arc_serv... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/2622843002/diff/80001/components/arc/arc_serv... components/arc/arc_service_manager.h:31: // If an ArcService is declared with a Name, e.g.: On 2017/01/10 19:30:57, Yusuke Sato wrote: > s/Name/name/ is better now. > Done. https://codereview.chromium.org/2622843002/diff/80001/components/arc/arc_serv... components/arc/arc_service_manager.h:47: return T::kArcServiceName; On 2017/01/10 19:30:57, Yusuke Sato wrote: > LOG(ERROR) when T::kArcServiceName is an empty string? Done. https://codereview.chromium.org/2622843002/diff/80001/components/arc/arc_serv... File components/arc/arc_service_manager_unittest.cc (right): https://codereview.chromium.org/2622843002/diff/80001/components/arc/arc_serv... components/arc/arc_service_manager_unittest.cc:38: const char NamedService::kArcServiceName[] = On 2017/01/10 19:30:57, Yusuke Sato wrote: > nit: space between L37 and 38 Done. https://codereview.chromium.org/2622843002/diff/80001/components/arc/arc_serv... components/arc/arc_service_manager_unittest.cc:100: // Named services can only be added once, but can still be retrieved. On 2017/01/10 19:30:57, Yusuke Sato wrote: > Could you also check that SecondNamedService can be added after L111 (and they > can be retrieved separately with GetService?) Done.
The CQ bit was checked by lhchavez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusukes@chromium.org Link to the patchset: https://codereview.chromium.org/2622843002/#ps100001 (title: "Addressed feedback")
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
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by lhchavez@chromium.org
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
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
LGTM! https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.cc:87: named_services_.insert(std::make_pair(name, std::move(service))); On 2017/01/10 18:41:55, Luis Héctor Chávez wrote: > On 2017/01/10 08:03:12, hidehiko wrote: > > emplace? > > emplace is not supported for maps: > https://chromium-cpp.appspot.com/#library-whitelist Oh, good to know. Thank you! https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/2622843002/diff/40001/components/arc/arc_serv... components/arc/arc_service_manager.h:46: typedef char No[1]; On 2017/01/10 18:41:56, Luis Héctor Chávez wrote: > On 2017/01/10 08:03:12, hidehiko wrote: > > On 2017/01/10 07:11:11, Yusuke Sato wrote: > > > nit: do we have a reason to skip 0? No can be [0] and Yes can be [1]. > > > > FYI: 0 length is illegal, IIUC. > > http://stackoverflow.com/questions/6180012/array-with-size-0 > > > > Anyway, let's use std::true_type and std::false_type instead. > > This is a commonly-used Method Detector idiom: > https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Member_Detector, but the > alternative seems to work, so I'll use it. Yes, I know, but it was developed in C++03 era, and for this specific case, I thought we can redux the code a bit. Thank you for cleaning up! https://codereview.chromium.org/2622843002/diff/100001/components/arc/arc_ser... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/2622843002/diff/100001/components/arc/arc_ser... components/arc/arc_service_manager.h:43: // In order to avoid collisions, kArcServiceName should be the fully-qualified So, as you explicitly specify the template typename in caller, argument looks not necessary (but actually is needed). Could you add implementation note about it? Note: I was original thinking to use it for type inference, but I'm ok for the current implementation, as well. https://codereview.chromium.org/2622843002/diff/100001/components/arc/arc_ser... components/arc/arc_service_manager.h:47: if (strlen(T::kArcServiceName) == 0) Optional: or, static_assert for compile time check? static_assert(T::kArcServiceName[0] != '\0', ...);
One more minor comment. https://codereview.chromium.org/2622843002/diff/100001/components/arc/arc_ser... File components/arc/arc_service_manager.cc (left): https://codereview.chromium.org/2622843002/diff/100001/components/arc/arc_ser... components/arc/arc_service_manager.cc:7: #include <utility> nit: should we keep <utility> for make_pair/move used in this file directly?
On 2017/01/11 05:38:58, hidehiko wrote: > One more minor comment. > > https://codereview.chromium.org/2622843002/diff/100001/components/arc/arc_ser... > File components/arc/arc_service_manager.cc (left): > > https://codereview.chromium.org/2622843002/diff/100001/components/arc/arc_ser... > components/arc/arc_service_manager.cc:7: #include <utility> > nit: should we keep <utility> for make_pair/move used in this file directly? Oops. I meant, still LGTM.
https://codereview.chromium.org/2622843002/diff/100001/components/arc/arc_ser... File components/arc/arc_service_manager.cc (left): https://codereview.chromium.org/2622843002/diff/100001/components/arc/arc_ser... components/arc/arc_service_manager.cc:7: #include <utility> On 2017/01/11 05:38:58, hidehiko wrote: > nit: should we keep <utility> for make_pair/move used in this file directly? no because it's already included in the .h, and IWYU rules allow for depending on the header file inclusions. https://codereview.chromium.org/2622843002/diff/100001/components/arc/arc_ser... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/2622843002/diff/100001/components/arc/arc_ser... components/arc/arc_service_manager.h:43: // In order to avoid collisions, kArcServiceName should be the fully-qualified On 2017/01/11 05:17:45, hidehiko wrote: > So, as you explicitly specify the template typename in caller, > argument looks not necessary (but actually is needed). > Could you add implementation note about it? > > Note: I was original thinking to use it for type inference, > but I'm ok for the current implementation, as well. > Done. https://codereview.chromium.org/2622843002/diff/100001/components/arc/arc_ser... components/arc/arc_service_manager.h:47: if (strlen(T::kArcServiceName) == 0) On 2017/01/11 05:17:45, hidehiko wrote: > Optional: or, static_assert for compile time check? > static_assert(T::kArcServiceName[0] != '\0', ...); Tried that before, compiler didn't like it since T::kArcServiceName is not constexpr.
The CQ bit was checked by lhchavez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org, yusukes@chromium.org Link to the patchset: https://codereview.chromium.org/2622843002/#ps120001 (title: "Added a 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": 1484150502473140,
"parent_rev": "8a607b49cbcfbb114cd12dd80abc0997c5f3e379", "commit_rev":
"ce096d5651c3984e1ed5a260e11ba77d5b37d580"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} Committed: https://chromium.googlesource.com/chromium/src/+/ce096d5651c3984e1ed5a260e11b... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/ce096d5651c3984e1ed5a260e11b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
