Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2693243002:
chromeos: Make CrosDBusService not be a singleton. (Closed)
|
Created:
3 years, 10 months ago by Daniel Erat Modified:
3 years, 10 months ago CC:
chromium-reviews, hashimoto+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionchromeos: Make CrosDBusService not be a singleton.
Update the CrosDBusService class, currently only used to
implement org.chromium.LibCrosService, so it can be
instantiated multiple times for different services. This is
needed for mash, where some services will be implemented in
ash and others in Chrome.
BUG=692246
Review-Url: https://codereview.chromium.org/2693243002
Cr-Commit-Position: refs/heads/master@{#450957}
Committed: https://chromium.googlesource.com/chromium/src/+/cda054bfe94bdcbdb275b563d52dc7f1d443f94b
Patch Set 1 #Patch Set 2 : fix object_path.h include #
Total comments: 17
Patch Set 3 : address review comments #Patch Set 4 : whoops, fix reversed logic in IsUsingFakes check #
Messages
Total messages: 25 (17 generated)
The CQ bit was checked by derat@chromium.org to run a CQ dry run
derat@chromium.org changed reviewers: + hashimoto@chromium.org, jamescook@chromium.org, satorux@chromium.org, stevenjb@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM, though I'm certainly not the best reviewer for this. I defer to the //chromeos/dbus owners. :-) https://codereview.chromium.org/2693243002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/2693243002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:249: cros_dbus_service_ = CrosDBusService::Create( Hooray for making it owned and not a singleton! https://codereview.chromium.org/2693243002/diff/20001/chromeos/dbus/services/... File chromeos/dbus/services/cros_dbus_service.cc (left): https://codereview.chromium.org/2693243002/diff/20001/chromeos/dbus/services/... chromeos/dbus/services/cros_dbus_service.cc:139: VLOG(1) << "CrosDBusService Shutdown completed"; Was this VLOG not useful? It kinda feels like we either need VLOGs for both startup and shutdown, or we don't need them at all. https://codereview.chromium.org/2693243002/diff/20001/chromeos/dbus/services/... File chromeos/dbus/services/cros_dbus_service.cc (right): https://codereview.chromium.org/2693243002/diff/20001/chromeos/dbus/services/... chromeos/dbus/services/cros_dbus_service.cc:33: service_providers_(std::move(service_providers)) {} nit: Is there anything useful you can DCHECK here, like the service_name or object_path not being empty? I like preconditions. :-) https://codereview.chromium.org/2693243002/diff/20001/chromeos/dbus/services/... File chromeos/dbus/services/cros_dbus_service.h (right): https://codereview.chromium.org/2693243002/diff/20001/chromeos/dbus/services/... chromeos/dbus/services/cros_dbus_service.h:26: // CrosDBusService::ServiceProviderInterface. nit: I would include something in this comment about the legacy / weirdly-named LibCrosService. It's an important part of the history. Alternately, I would include examples of the services it will be used to provide.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2693243002/diff/20001/chromeos/dbus/services/... File chromeos/dbus/services/cros_dbus_service.cc (right): https://codereview.chromium.org/2693243002/diff/20001/chromeos/dbus/services/... chromeos/dbus/services/cros_dbus_service.cc:114: std::unique_ptr<CrosDBusService> CrosDBusService::CreateRealImpl( nit: this function looks pretty short. maybe just inline the code in Create() and remove this function? https://codereview.chromium.org/2693243002/diff/20001/chromeos/dbus/services/... File chromeos/dbus/services/cros_dbus_service_unittest.cc (right): https://codereview.chromium.org/2693243002/diff/20001/chromeos/dbus/services/... chromeos/dbus/services/cros_dbus_service_unittest.cc:35: }; while you are at it, i'd appreciate it if you could get rid of the gmock class. :) well, we'll still have to live with MockBus etc., but one less gmock class would be nice.
https://codereview.chromium.org/2693243002/diff/20001/chromeos/dbus/services/... File chromeos/dbus/services/cros_dbus_service.cc (right): https://codereview.chromium.org/2693243002/diff/20001/chromeos/dbus/services/... chromeos/dbus/services/cros_dbus_service.cc:93: CrosDBusServiceStubImpl() {} nit: =default here doesn't work? https://codereview.chromium.org/2693243002/diff/20001/chromeos/dbus/services/... chromeos/dbus/services/cros_dbus_service.cc:106: if (!base::SysInfo::IsRunningOnChromeOS() || !bus) How about using DBusThreadManager::IsUsingFakes instead? https://codereview.chromium.org/2693243002/diff/20001/chromeos/dbus/services/... chromeos/dbus/services/cros_dbus_service.cc:107: return std::unique_ptr<CrosDBusService>(new CrosDBusServiceStubImpl()); nit: base::MakeUnique?
https://codereview.chromium.org/2693243002/diff/20001/chromeos/dbus/services/... File chromeos/dbus/services/cros_dbus_service.cc (left): https://codereview.chromium.org/2693243002/diff/20001/chromeos/dbus/services/... chromeos/dbus/services/cros_dbus_service.cc:139: VLOG(1) << "CrosDBusService Shutdown completed"; On 2017/02/15 01:53:34, James Cook wrote: > Was this VLOG not useful? It kinda feels like we either need VLOGs for both > startup and shutdown, or we don't need them at all. uh... well, _i_ never used it. :-P i've deleted the other VLOG in CrosDBusServiceImpl's c'tor to match. https://codereview.chromium.org/2693243002/diff/20001/chromeos/dbus/services/... File chromeos/dbus/services/cros_dbus_service.cc (right): https://codereview.chromium.org/2693243002/diff/20001/chromeos/dbus/services/... chromeos/dbus/services/cros_dbus_service.cc:33: service_providers_(std::move(service_providers)) {} On 2017/02/15 01:53:34, James Cook wrote: > nit: Is there anything useful you can DCHECK here, like the service_name or > object_path not being empty? I like preconditions. :-) Done. https://codereview.chromium.org/2693243002/diff/20001/chromeos/dbus/services/... chromeos/dbus/services/cros_dbus_service.cc:93: CrosDBusServiceStubImpl() {} On 2017/02/15 08:44:06, hashimoto wrote: > nit: =default here doesn't work? hmm. i thought i'd seen compiler errors from one toolchain when i tried this, but maybe it was just an issue due to not defining a c'tor for the base class earlier. it appears to work now in both desktop and simple chrome builds. https://codereview.chromium.org/2693243002/diff/20001/chromeos/dbus/services/... chromeos/dbus/services/cros_dbus_service.cc:106: if (!base::SysInfo::IsRunningOnChromeOS() || !bus) On 2017/02/15 08:44:06, hashimoto wrote: > How about using DBusThreadManager::IsUsingFakes instead? i'll give it a try; hopefully it detects the same thing (i'm not sure how we initialize all tests). https://codereview.chromium.org/2693243002/diff/20001/chromeos/dbus/services/... chromeos/dbus/services/cros_dbus_service.cc:107: return std::unique_ptr<CrosDBusService>(new CrosDBusServiceStubImpl()); On 2017/02/15 08:44:06, hashimoto wrote: > nit: base::MakeUnique? ah, thanks. i wasn't sure about the rules around how a std::unique_ptr's templated type would be upcast when returning it. https://codereview.chromium.org/2693243002/diff/20001/chromeos/dbus/services/... chromeos/dbus/services/cros_dbus_service.cc:114: std::unique_ptr<CrosDBusService> CrosDBusService::CreateRealImpl( On 2017/02/15 06:48:01, satorux1 wrote: > nit: this function looks pretty short. maybe just inline the code in Create() > and remove this function? i would, except it's also needed by a test (see comment in header) https://codereview.chromium.org/2693243002/diff/20001/chromeos/dbus/services/... File chromeos/dbus/services/cros_dbus_service.h (right): https://codereview.chromium.org/2693243002/diff/20001/chromeos/dbus/services/... chromeos/dbus/services/cros_dbus_service.h:26: // CrosDBusService::ServiceProviderInterface. On 2017/02/15 01:53:34, James Cook wrote: > nit: I would include something in this comment about the legacy / weirdly-named > LibCrosService. It's an important part of the history. Alternately, I would > include examples of the services it will be used to provide. i'm fine with adding more details like this to the place where the LibCrosService instance is instantiated in chrome_browser_main_chromeos.cc, but don't think the history belongs here. https://codereview.chromium.org/2693243002/diff/20001/chromeos/dbus/services/... File chromeos/dbus/services/cros_dbus_service_unittest.cc (right): https://codereview.chromium.org/2693243002/diff/20001/chromeos/dbus/services/... chromeos/dbus/services/cros_dbus_service_unittest.cc:35: }; On 2017/02/15 06:48:01, satorux1 wrote: > while you are at it, i'd appreciate it if you could get rid of the gmock class. > :) well, we'll still have to live with MockBus etc., but one less gmock class > would be nice. i actually don't mind this particular usage of gmock, because it's small, simple, and self-contained, and only verifies what is actually being tested (i.e. that Start is called exactly once with the expected exported object). if i change this to instead be a handwritten stub class, i'll need to make it save the exported object that's passed to it, and then make the test class also save the exported object that it creates, and then compare them in the test body, right? i'm happy to change this in a followup change if you can explain more.
The CQ bit was checked by derat@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_chromeos_ozone_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 derat@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.
lgtm
The CQ bit was checked by derat@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2693243002/#ps60001 (title: "whoops, fix reversed logic in IsUsingFakes check")
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": 60001, "attempt_start_ts": 1487255649344430,
"parent_rev": "69e8419f8d98ee886c1f7b34413ab97d6237cba9", "commit_rev":
"cda054bfe94bdcbdb275b563d52dc7f1d443f94b"}
Message was sent while issue was closed.
Description was changed from ========== chromeos: Make CrosDBusService not be a singleton. Update the CrosDBusService class, currently only used to implement org.chromium.LibCrosService, so it can be instantiated multiple times for different services. This is needed for mash, where some services will be implemented in ash and others in Chrome. BUG=692246 ========== to ========== chromeos: Make CrosDBusService not be a singleton. Update the CrosDBusService class, currently only used to implement org.chromium.LibCrosService, so it can be instantiated multiple times for different services. This is needed for mash, where some services will be implemented in ash and others in Chrome. BUG=692246 Review-Url: https://codereview.chromium.org/2693243002 Cr-Commit-Position: refs/heads/master@{#450957} Committed: https://chromium.googlesource.com/chromium/src/+/cda054bfe94bdcbdb275b563d52d... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/cda054bfe94bdcbdb275b563d52d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
