Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
697493002:
Support transition between chrome and user mode console (Closed)
|
Created:
6 years, 1 month ago by dsodman Modified:
6 years, 1 month ago CC:
chromium-reviews, ozone-reviews_chromium.org, hashimoto+watch_chromium.org, oshima+watch_chromium.org, kalyank, piman+watch_chromium.org, stevenjb+watch_chromium.org, dnicoara Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionSupport transition between chrome and user mode console
Freon supports a user mode console that is drm based. So,
to transition between the user console and chrome, we will
initiate dbus messages to effect the transition. This change
is the chromeos/dbus part.
BUG=406031
TEST=test with user mode console
Committed: https://crrev.com/37c2c20f5e5936c0fcd853edba763f21316d7739
Cr-Commit-Position: refs/heads/master@{#302666}
Patch Set 1 #Patch Set 2 : Support transition between chrome and user-mode console #Patch Set 3 : Add DBUS API to support chrome/console transition #
Total comments: 35
Patch Set 4 : Add DBUS API to support transition between chrome and console #
Total comments: 16
Patch Set 5 : DBUS API to support transtion between chrome/console #Patch Set 6 : #
Total comments: 8
Patch Set 7 : upload again #
Total comments: 16
Patch Set 8 : #
Total comments: 20
Patch Set 9 : #
Total comments: 4
Patch Set 10 : fixed 2 mistakes in previous patch #
Messages
Total messages: 30 (4 generated)
dsodman@chromium.org changed reviewers: + derat@chromium.org, hashimoto@chromium.org, stevenjb@chromium.org
dbus api to support transition between console and chrome. Will be used by a follow-up change
https://codereview.chromium.org/697493002/diff/40001/chromeos/chromeos.gyp File chromeos/chromeos.gyp (right): https://codereview.chromium.org/697493002/diff/40001/chromeos/chromeos.gyp#ne... chromeos/chromeos.gyp:79: 'dbus/console_service.h', console_service_client.h https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... File chromeos/dbus/console_service_client.cc (right): https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:15: ConsoleServiceClient* ConsoleServiceClient::instance = NULL; File local in anonymous namespace, 's_console_service_client = NULL'; https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:17: class ConsoleServiceClientImpl : public ConsoleServiceClient { The implementation can also be in an anonymous namespace. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:19: ConsoleServiceClientImpl(); This needs a destructor override. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:23: } With the exception of trivial accessors, it is better to either not inline any methods (preferred) or inline all of them (acceptable for small classes with only short implementations). https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:33: void OnOwnership(const std::string& service_name, bool success); Include a brief comment for each of these methods. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:43: virtual void Init(dbus::Bus* bus) override; No 'virtual' for overrides. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:54: void ConsoleServiceClientImpl::OnOwnership(const std::string& service_name, Ordering of method implementations should match declaration order. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:67: ConsoleServiceClientImpl::ConsoleServiceClientImpl() : weak_ptr_factory_(this) { bus_ needs to be initialized. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:71: instance = this; CHECK_EQ(NULL, s_chonsole_service_client); ALso, better to set this in Create() instead. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:96: LOG(ERROR) << "ConsoleServiceClientImpl::Init"; VLOG https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... File chromeos/dbus/console_service_client.h (right): https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.h:6: #define CHROMEOS_DBUS_CROS_CONSOLE_SERVICE_CLIENT_H_ s/CROS_// https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.h:16: } None of these appear to be used in this header. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.h:24: virtual ~Observer() {} Destructor should be protected. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.h:25: virtual void OnActivateConsole(int console_id) {} This should be a pure virtual (= 0) https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.h:34: virtual ~ConsoleServiceClient() {} Use override: ~ConsoleServiceClient() override; Also, do not inline virtual destructors (or any virtual method) *unless* the class is a pure virtual class (i.e. no implemented methods). https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.h:40: static ConsoleServiceClient* instance; Do not make this a class member, make it a file local variable, e.g. s_console_service_client. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/org.chromi... File chromeos/dbus/org.chromium.ConsoleService.conf (right): https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/org.chromi... chromeos/dbus/org.chromium.ConsoleService.conf:16: </busconfig> I don't think we actually need/use this in the chrome repo?
Thanks for taking a look. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... File chromeos/dbus/console_service_client.cc (right): https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:15: ConsoleServiceClient* ConsoleServiceClient::instance = NULL; On 2014/10/31 17:00:46, stevenjb wrote: > File local in anonymous namespace, 's_console_service_client = NULL'; > Done. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:17: class ConsoleServiceClientImpl : public ConsoleServiceClient { On 2014/10/31 17:00:46, stevenjb wrote: > The implementation can also be in an anonymous namespace. Done. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:19: ConsoleServiceClientImpl(); On 2014/10/31 17:00:46, stevenjb wrote: > This needs a destructor override. Done. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:23: } On 2014/10/31 17:00:46, stevenjb wrote: > With the exception of trivial accessors, it is better to either not inline any > methods (preferred) or inline all of them (acceptable for small classes with > only short implementations). Done. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:33: void OnOwnership(const std::string& service_name, bool success); On 2014/10/31 17:00:46, stevenjb wrote: > Include a brief comment for each of these methods. Done. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:43: virtual void Init(dbus::Bus* bus) override; On 2014/10/31 17:00:46, stevenjb wrote: > No 'virtual' for overrides. Done. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:54: void ConsoleServiceClientImpl::OnOwnership(const std::string& service_name, On 2014/10/31 17:00:46, stevenjb wrote: > Ordering of method implementations should match declaration order. Done. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:67: ConsoleServiceClientImpl::ConsoleServiceClientImpl() : weak_ptr_factory_(this) { On 2014/10/31 17:00:46, stevenjb wrote: > bus_ needs to be initialized. Done. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:71: instance = this; On 2014/10/31 17:00:46, stevenjb wrote: > CHECK_EQ(NULL, s_chonsole_service_client); > ALso, better to set this in Create() instead. Done. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:96: LOG(ERROR) << "ConsoleServiceClientImpl::Init"; On 2014/10/31 17:00:46, stevenjb wrote: > VLOG Done. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... File chromeos/dbus/console_service_client.h (right): https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.h:6: #define CHROMEOS_DBUS_CROS_CONSOLE_SERVICE_CLIENT_H_ On 2014/10/31 17:00:46, stevenjb wrote: > s/CROS_// Done. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.h:16: } On 2014/10/31 17:00:46, stevenjb wrote: > None of these appear to be used in this header. Done. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.h:24: virtual ~Observer() {} On 2014/10/31 17:00:46, stevenjb wrote: > Destructor should be protected. Done. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.h:25: virtual void OnActivateConsole(int console_id) {} On 2014/10/31 17:00:47, stevenjb wrote: > This should be a pure virtual (= 0) Done. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.h:34: virtual ~ConsoleServiceClient() {} On 2014/10/31 17:00:46, stevenjb wrote: > Use override: > ~ConsoleServiceClient() override; > Also, do not inline virtual destructors (or any virtual method) *unless* the > class is a pure virtual class (i.e. no implemented methods). Done. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.h:40: static ConsoleServiceClient* instance; On 2014/10/31 17:00:46, stevenjb wrote: > Do not make this a class member, make it a file local variable, e.g. > s_console_service_client. Done. https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/org.chromi... File chromeos/dbus/org.chromium.ConsoleService.conf (right): https://codereview.chromium.org/697493002/diff/40001/chromeos/dbus/org.chromi... chromeos/dbus/org.chromium.ConsoleService.conf:16: </busconfig> On 2014/10/31 17:00:47, stevenjb wrote: > I don't think we actually need/use this in the chrome repo? Where does it belong then? I was following the example of LibCrosService which lives in the chrome repo and is pulled to the device via chromeos-chrome ebuild.
derat@chromium.org changed reviewers: + satorux@chromium.org
adding satorux@ for input on where the libcros d-bus service should live. there are already methods exported by it that would prefer to not be browser-specific (LivenessServiceProvider and DisplayPowerServiceProvider ar the two that i know of). https://codereview.chromium.org/697493002/diff/60001/chromeos/dbus/console_se... File chromeos/dbus/console_service_client.cc (right): https://codereview.chromium.org/697493002/diff/60001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:24: ~ConsoleServiceClientImpl(); add override https://codereview.chromium.org/697493002/diff/60001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:30: void OnOwnership(const std::string& service_name, bool success); these three methods seem like they should all be private instead of public https://codereview.chromium.org/697493002/diff/60001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:71: // OnOwnership please delete these method names in comments; i haven't seen them anywhere else in chrome https://codereview.chromium.org/697493002/diff/60001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:73: // |success| will be true if ownership succeeded and false otherwise. these comments should live above the corresponding method declarations rather than above the definitions https://codereview.chromium.org/697493002/diff/60001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:122: LOG(ERROR) << "Unable to get system d-bus for the ConsoleServiceClient"; any reason that this shouldn't just be CHECK(bus_)? logging an error that no one will see and returning doesn't seem ideal https://codereview.chromium.org/697493002/diff/60001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:126: bus_->RequestOwnership(chromeos::kConsoleServiceName, it seems a bit strange to use a new service here instead of making this be part of kLibCrosServiceName https://codereview.chromium.org/697493002/diff/60001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:153: CHECK_EQ(NULL, s_console_service_client); this check is wrong. i think that steven meant that you should check that it's NULL *before* allocating a new one, not after. https://codereview.chromium.org/697493002/diff/60001/chromeos/dbus/console_se... File chromeos/dbus/console_service_client.h (right): https://codereview.chromium.org/697493002/diff/60001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.h:14: class CHROMEOS_EXPORT ConsoleServiceClient : public DBusClient { should this be named ConsoleServiceProvider? i thought that all of the other classes in this directory are named "Client" because they're clients of system daemons running on chrome os. this class doesn't appear to do that; it offers a service for other processes to call. it seems much more similar to the classes in chrome/browser/chromeos/dbus than the ones in chromeos/dbus. i think that the right approach is probably to move c/b/chromeos/dbus/cros_dbus_service.h to chromeos/dbus and then make this class derive from it instead of from DBusClient.
Thanks for taking a look https://codereview.chromium.org/697493002/diff/60001/chromeos/dbus/console_se... File chromeos/dbus/console_service_client.cc (right): https://codereview.chromium.org/697493002/diff/60001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:24: ~ConsoleServiceClientImpl(); On 2014/10/31 18:15:02, Daniel Erat wrote: > add override Done. https://codereview.chromium.org/697493002/diff/60001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:30: void OnOwnership(const std::string& service_name, bool success); On 2014/10/31 18:15:02, Daniel Erat wrote: > these three methods seem like they should all be private instead of public Done. https://codereview.chromium.org/697493002/diff/60001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:71: // OnOwnership On 2014/10/31 18:15:02, Daniel Erat wrote: > please delete these method names in comments; i haven't seen them anywhere else > in chrome Done. https://codereview.chromium.org/697493002/diff/60001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:73: // |success| will be true if ownership succeeded and false otherwise. On 2014/10/31 18:15:02, Daniel Erat wrote: > these comments should live above the corresponding method declarations rather > than above the definitions Done. https://codereview.chromium.org/697493002/diff/60001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:122: LOG(ERROR) << "Unable to get system d-bus for the ConsoleServiceClient"; On 2014/10/31 18:15:02, Daniel Erat wrote: > any reason that this shouldn't just be CHECK(bus_)? logging an error that no one > will see and returning doesn't seem ideal Done. https://codereview.chromium.org/697493002/diff/60001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:126: bus_->RequestOwnership(chromeos::kConsoleServiceName, On 2014/10/31 18:15:02, Daniel Erat wrote: > it seems a bit strange to use a new service here instead of making this be part > of kLibCrosServiceName That was what was done initially. This console service is really not browser related and being in chrome/browser/... is not really right in my opinion. https://codereview.chromium.org/697493002/diff/60001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.cc:153: CHECK_EQ(NULL, s_console_service_client); On 2014/10/31 18:15:02, Daniel Erat wrote: > this check is wrong. i think that steven meant that you should check that it's > NULL *before* allocating a new one, not after. I see. Fixed. https://codereview.chromium.org/697493002/diff/60001/chromeos/dbus/console_se... File chromeos/dbus/console_service_client.h (right): https://codereview.chromium.org/697493002/diff/60001/chromeos/dbus/console_se... chromeos/dbus/console_service_client.h:14: class CHROMEOS_EXPORT ConsoleServiceClient : public DBusClient { On 2014/10/31 18:15:02, Daniel Erat wrote: > should this be named ConsoleServiceProvider? > > i thought that all of the other classes in this directory are named "Client" > because they're clients of system daemons running on chrome os. this class > doesn't appear to do that; it offers a service for other processes to call. it > seems much more similar to the classes in chrome/browser/chromeos/dbus than the > ones in chromeos/dbus. > > i think that the right approach is probably to move > c/b/chromeos/dbus/cros_dbus_service.h to chromeos/dbus and then make this class > derive from it instead of from DBusClient. I think that is a much bigger change than what I'm trying to do with the console. I would prefer to get this submitted to unblock testing, and then do a separate follow-on change investigates what it would take to move c/b/chromeos/dbus/cros_dbus_service.h to chromeos/dbus and make the console a proper "client"
i'd prefer that you implement this as a CrosDBusService::ServiceProviderInterface in chrome/browser/chromeos/dbus for now; it really belongs in the existing libcros service instead of being part of a new service (which would be difficult to change later). i've filed issue 429354 to track moving some of those service providers to the top-level chromeos/ directory -- we already have ones that need to move, so this new service provider can just get moved when the rest of them do.
On 2014/10/31 22:42:59, Daniel Erat wrote: > i'd prefer that you implement this as a > CrosDBusService::ServiceProviderInterface in chrome/browser/chromeos/dbus for > now; it really belongs in the existing libcros service instead of being part of > a new service (which would be difficult to change later). > > i've filed issue 429354 to track moving some of those service providers to the > top-level chromeos/ directory -- we already have ones that need to move, so this > new service provider can just get moved when the rest of them do. Alright, i've moved the code back. Let's try this agian.
dsodman@google.com changed reviewers: + dsodman@google.com
thanks. please also fix the commit message (via the "Edit Issue" link in rietveld); it looks like the message is duplicated right now. https://codereview.chromium.org/697493002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/dbus/console_service_provider.cc (right): https://codereview.chromium.org/697493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: remove "(c) " https://codereview.chromium.org/697493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.cc:56: if (!success) { nit: omit curly brackets since the statement is a single line https://codereview.chromium.org/697493002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/dbus/console_service_provider.h (right): https://codereview.chromium.org/697493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:30: virtual void OnActivateConsole(int console_id) = 0; do you really need this observer interface? can ActivateConsole() instead just call DisplayConfigurator directly? if so, that seems like it'd be simpler. https://codereview.chromium.org/697493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:46: // This method will get called when a external process sends the dbus nit: s/sends/calls/ https://codereview.chromium.org/697493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:47: // method ConsoleServiceInterface.ActivateConsole. The method receives nit: remove "ConsoleServiceInterface." here since that is no longer the case https://codereview.chromium.org/697493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:48: // a single integer parameter |console_id| which indicates the console id. nit: remove redundant "which indicates the console id" https://codereview.chromium.org/697493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:49: // Chrome will take ownership of the display service when the console nit: remove extra space between "Chrome" and "will" https://codereview.chromium.org/697493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:52: // display. what's the point of taking an int here and treating 0 and 1 specially, instead of just taking a bool? looking at the other change, it's unclear to me that this id is used for anything else: void DisplayConfigurator::OnActivateConsole(int console_id) { LOG(ERROR) << "DisplayConfigurator::ActivateConsole: " << console_id; if (console_id < 2) { SetDisplayMode(display_state()); } else { RelinquishControl(); } }
Updated based on feedback. Tested what I could from home but can't test out the feature until I get into the office on Monday morning.
i think the upload didn't work; the latest version i see is still patch set 6 with my comments on it.
On 2014/11/01 23:53:35, Daniel Erat wrote: > i think the upload didn't work; the latest version i see is still patch set 6 > with my comments on it. Uploaded again
https://codereview.chromium.org/697493002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/dbus/console_service_provider.cc (right): https://codereview.chromium.org/697493002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.cc:38: else nit: you need curly brackets for the if/else now that the SetDisplayMode call wraps (but see below naming suggestion that would bring it back to a single line) https://codereview.chromium.org/697493002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.cc:40: display_configurator->display_state()); asking it to set the display to the already-current mode seems risky; i could easily see someone optimizing this to be a no-op in the future. how about adding a TakeControl() method to match the RelinquishControl() method you call above? https://codereview.chromium.org/697493002/diff/120001/ui/display/chromeos/dis... File ui/display/chromeos/display_configurator.cc (right): https://codereview.chromium.org/697493002/diff/120001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator.cc:211: // native_display_delegate_->RelinquishDisplayControl(); please don't check in commented-out code; just replace this whole commented section with NOTIMPLEMENTED()
https://codereview.chromium.org/697493002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/dbus/console_service_provider.h (right): https://codereview.chromium.org/697493002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:7: #Include <string> https://codereview.chromium.org/697493002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:11: #include "dbus/message.h" #include "base/memory/weak_ptr.h" https://codereview.chromium.org/697493002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:28: virtual ~ConsoleServiceProvider(); ~ConsoleServiceProvider() override; https://codereview.chromium.org/697493002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:30: void Start(scoped_refptr<dbus::ExportedObject> exported_object) override; Comment above: // ServiceProviderInterface https://codereview.chromium.org/697493002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:48: base::WeakPtrFactory<ConsoleServiceProvider> weak_ptr_factory_; nit: blank line
Updated per review feedback https://codereview.chromium.org/697493002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/dbus/console_service_provider.cc (right): https://codereview.chromium.org/697493002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.cc:38: else On 2014/11/02 13:48:34, Daniel Erat wrote: > nit: you need curly brackets for the if/else now that the SetDisplayMode call > wraps (but see below naming suggestion that would bring it back to a single > line) Done. https://codereview.chromium.org/697493002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.cc:40: display_configurator->display_state()); On 2014/11/02 13:48:34, Daniel Erat wrote: > asking it to set the display to the already-current mode seems risky; i could > easily see someone optimizing this to be a no-op in the future. how about adding > a TakeControl() method to match the RelinquishControl() method you call above? Done. https://codereview.chromium.org/697493002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/dbus/console_service_provider.h (right): https://codereview.chromium.org/697493002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:7: On 2014/11/03 16:35:42, stevenjb wrote: > #Include <string> Done. https://codereview.chromium.org/697493002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:11: #include "dbus/message.h" On 2014/11/03 16:35:41, stevenjb wrote: > #include "base/memory/weak_ptr.h" Done. https://codereview.chromium.org/697493002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:28: virtual ~ConsoleServiceProvider(); On 2014/11/03 16:35:42, stevenjb wrote: > ~ConsoleServiceProvider() override; Done. https://codereview.chromium.org/697493002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:30: void Start(scoped_refptr<dbus::ExportedObject> exported_object) override; On 2014/11/03 16:35:42, stevenjb wrote: > Comment above: // ServiceProviderInterface Done. https://codereview.chromium.org/697493002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:48: base::WeakPtrFactory<ConsoleServiceProvider> weak_ptr_factory_; On 2014/11/03 16:35:41, stevenjb wrote: > nit: blank line Done. https://codereview.chromium.org/697493002/diff/120001/ui/display/chromeos/dis... File ui/display/chromeos/display_configurator.cc (right): https://codereview.chromium.org/697493002/diff/120001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator.cc:211: // native_display_delegate_->RelinquishDisplayControl(); On 2014/11/02 13:48:34, Daniel Erat wrote: > please don't check in commented-out code; just replace this whole commented > section with NOTIMPLEMENTED() Done.
chrome/browser/chromeos/dbus lgtm. hashimoto@, could you also take a look? https://codereview.chromium.org/697493002/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/dbus/console_service_provider.h (right): https://codereview.chromium.org/697493002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:25: // The main client is the console application. This can nit: three spaces -> one/two spaces?
chrome/browser/chromeos/dbus lgtm https://codereview.chromium.org/697493002/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/dbus/console_service_provider.cc (right): https://codereview.chromium.org/697493002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.cc:6: #include "chrome/browser/chromeos/dbus/console_service_provider.h" nit: This include should be put on top separately http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Ord... https://codereview.chromium.org/697493002/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/dbus/console_service_provider.h (right): https://codereview.chromium.org/697493002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:14: #include "dbus/message.h" nit: This include is not needed thanks to forward declarations?
looks fine to me after some nits are addressed. thanks! https://codereview.chromium.org/697493002/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/dbus/console_service_provider.h (right): https://codereview.chromium.org/697493002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:10: #include "base/bind.h" you probably don't need this include here, right? https://codereview.chromium.org/697493002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:18: class Response; it doesn't look like this forward declaration is used https://codereview.chromium.org/697493002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:23: // This class provides an api for external apps to notify while we're providing comment nits: s/api/API/ on this line and s/chrome/Chrome/ on the next line https://codereview.chromium.org/697493002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:38: // control of the display and chrome can take ownership. nit: s/chrome/Chrome/ https://codereview.chromium.org/697493002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:44: // the display server and needs chrome to release ownership. nit: s/chrome/Chrome/, s/display server/display/ (unless there's a server that i don't know about, in which case the TakeDisplayOwnership comment should probably mention it too) https://codereview.chromium.org/697493002/diff/140001/ui/display/chromeos/dis... File ui/display/chromeos/display_configurator.h (right): https://codereview.chromium.org/697493002/diff/140001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator.h:149: // and chrome and take control. is the "and" here supposed to be "can"? nit: s/chrome/Chrome/ https://codereview.chromium.org/697493002/diff/140001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator.h:153: // chrome should relinquish it. nit: s/chrome/Chrome/
Thanks for looking. Updated patch per review feedback https://codereview.chromium.org/697493002/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/dbus/console_service_provider.cc (right): https://codereview.chromium.org/697493002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.cc:6: #include "chrome/browser/chromeos/dbus/console_service_provider.h" On 2014/11/04 08:57:43, hashimoto wrote: > nit: This include should be put on top separately > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Ord... Done. https://codereview.chromium.org/697493002/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/dbus/console_service_provider.h (right): https://codereview.chromium.org/697493002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:10: #include "base/bind.h" On 2014/11/04 15:18:49, Daniel Erat wrote: > you probably don't need this include here, right? Done. https://codereview.chromium.org/697493002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:14: #include "dbus/message.h" On 2014/11/04 08:57:44, hashimoto wrote: > nit: This include is not needed thanks to forward declarations? Done. https://codereview.chromium.org/697493002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:18: class Response; On 2014/11/04 15:18:50, Daniel Erat wrote: > it doesn't look like this forward declaration is used Done. https://codereview.chromium.org/697493002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:23: // This class provides an api for external apps to notify On 2014/11/04 15:18:49, Daniel Erat wrote: > while we're providing comment nits: s/api/API/ on this line and s/chrome/Chrome/ > on the next line Done. https://codereview.chromium.org/697493002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:25: // The main client is the console application. This can On 2014/11/04 06:42:30, satorux1 wrote: > nit: three spaces -> one/two spaces? Done. https://codereview.chromium.org/697493002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:38: // control of the display and chrome can take ownership. On 2014/11/04 15:18:50, Daniel Erat wrote: > nit: s/chrome/Chrome/ Done. https://codereview.chromium.org/697493002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.h:44: // the display server and needs chrome to release ownership. On 2014/11/04 15:18:50, Daniel Erat wrote: > nit: s/chrome/Chrome/, s/display server/display/ (unless there's a server that i > don't know about, in which case the TakeDisplayOwnership comment should probably > mention it too) Done. https://codereview.chromium.org/697493002/diff/140001/ui/display/chromeos/dis... File ui/display/chromeos/display_configurator.h (right): https://codereview.chromium.org/697493002/diff/140001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator.h:149: // and chrome and take control. On 2014/11/04 15:18:50, Daniel Erat wrote: > is the "and" here supposed to be "can"? > > nit: s/chrome/Chrome/ Done. https://codereview.chromium.org/697493002/diff/140001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator.h:153: // chrome should relinquish it. On 2014/11/04 15:18:50, Daniel Erat wrote: > nit: s/chrome/Chrome/ Done.
https://codereview.chromium.org/697493002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/dbus/console_service_provider.cc (right): https://codereview.chromium.org/697493002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.cc:5: #include "third_party/cros_system_api/dbus/service_constants.h" wrong one here -- console_service_provider.h should be on top and service_constants.h should be sorted with the other ones. https://codereview.chromium.org/697493002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.cc:16: } you shouldn't have these forward decls in the .cc file, i don't think
(i also updated the description to not be duplicated)
Thanks for catching those. https://codereview.chromium.org/697493002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/dbus/console_service_provider.cc (right): https://codereview.chromium.org/697493002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.cc:5: #include "third_party/cros_system_api/dbus/service_constants.h" On 2014/11/04 16:45:18, Daniel Erat wrote: > wrong one here -- console_service_provider.h should be on top and > service_constants.h should be sorted with the other ones. Done. https://codereview.chromium.org/697493002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/dbus/console_service_provider.cc:16: } On 2014/11/04 16:45:18, Daniel Erat wrote: > you shouldn't have these forward decls in the .cc file, i don't think Done.
lgtm
The CQ bit was checked by dsodman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/697493002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/37c2c20f5e5936c0fcd853edba763f21316d7739 Cr-Commit-Position: refs/heads/master@{#302666} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
