Support transition between chrome and user mode console

chromeos/dbus/console_service_client.cc

+// Copyright (c) 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/bind.h"
+#include "chromeos/dbus/console_service_client.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "dbus/bus.h"
+#include "dbus/exported_object.h"
+#include "dbus/message.h"
+#include "third_party/cros_system_api/dbus/service_constants.h"
+
+namespace dbus {
+class Bus;
+class MethodCall;
+class ExportedObject;
+}
+
+static chromeos::ConsoleServiceClient* s_console_service_client = NULL;
+
+class ConsoleServiceClientImpl : public chromeos::ConsoleServiceClient {
+ public:
+  ConsoleServiceClientImpl();
+  ~ConsoleServiceClientImpl();

[Daniel Erat, 2014/10/31 18:15:02]

add override

[dsodman, 2014/10/31 18:43:25]

Done.

+
+  void AddObserver(Observer* observer) override;
+  void RemoveObserver(Observer* observer) override;
+  bool HasObserver(Observer* observer) override;
+
+  void OnOwnership(const std::string& service_name, bool success);

[Daniel Erat, 2014/10/31 18:15:02]

these three methods seem like they should all be private instead of public

[dsodman, 2014/10/31 18:43:25]

Done.

+
+  void OnExported(const std::string& interface_name,
+                  const std::string& method_name,
+                  bool success);
+
+  void ActivateConsole(dbus::MethodCall* method_call,
+                       dbus::ExportedObject::ResponseSender response_sender);
+
+ protected:
+  void Init(dbus::Bus* bus) override;
+
+ private:
+  dbus::Bus* bus_;
+  ObserverList<Observer> observers_;
+  scoped_refptr<dbus::ExportedObject> exported_object_;
+  base::WeakPtrFactory<ConsoleServiceClientImpl> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(ConsoleServiceClientImpl);
+};
+
+ConsoleServiceClientImpl::ConsoleServiceClientImpl()
+    : bus_(NULL), weak_ptr_factory_(this) {
+}
+
+ConsoleServiceClientImpl::~ConsoleServiceClientImpl() {
+  s_console_service_client = NULL;
+}
+
+void ConsoleServiceClientImpl::AddObserver(Observer* observer) {
+  observers_.AddObserver(observer);
+}
+
+void ConsoleServiceClientImpl::RemoveObserver(Observer* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+bool ConsoleServiceClientImpl::HasObserver(Observer* observer) {
+  return observers_.HasObserver(observer);
+}
+
+// OnOwnership

[Daniel Erat, 2014/10/31 18:15:02]

please delete these method names in comments; i haven't seen them anywhere else
in chrome

[dsodman, 2014/10/31 18:43:25]

Done.

+// This method will be called when the class receives ownership of the bus.
+// |success| will be true if ownership succeeded and false otherwise.

[Daniel Erat, 2014/10/31 18:15:02]

these comments should live above the corresponding method declarations rather
than above the definitions

[dsodman, 2014/10/31 18:43:25]

Done.

+void ConsoleServiceClientImpl::OnOwnership(const std::string& service_name,
+                                           bool success) {
+  LOG_IF(FATAL, !success) << "failed to own: " << service_name;
+}
+
+// OnExported
+// This method will be called when a dbus method is exported.  The |success|
+// parameter will be true if the export is successful.  It will be false
+// otherwise.
+void ConsoleServiceClientImpl::OnExported(const std::string& interface_name,
+                                          const std::string& method_name,
+                                          bool success) {
+  if (!success) {
+    LOG(ERROR) << "failed to export " << interface_name << "." << method_name;
+  }
+}
+
+// ActivateConsole
+// This method will get called when a external process sends the dbus
+// method ConsoleServiceInterface.ActivateConsole.  The method receives
+// a single integer parameter |console_id| which indicates the console id.
+// Chrome  will take ownership of the display service when the console
+// id is 0 or 1.  It will relinquish control when the console_id is 2 or more
+// to allow other entities (such as the console) to take control of the
+// display.
+void ConsoleServiceClientImpl::ActivateConsole(
+    dbus::MethodCall* method_call,
+    dbus::ExportedObject::ResponseSender response_sender) {
+  dbus::MessageReader reader(method_call);
+  int console_id = 0;
+  if (reader.PopInt32(&console_id)) {
+    FOR_EACH_OBSERVER(Observer, observers_, OnActivateConsole(console_id));
+  } else {
+    LOG(ERROR) << "Unable to parse " << chromeos::kActivateConsole
+               << " request";
+  }
+  response_sender.Run(dbus::Response::FromMethodCall(method_call));
+}
+
+// Init
+// This will be called to initialize the class after the dbus system is
+// operational.   This cannot be in the constructor because there is no
+// guarantee that the constructor will be called after the dbus service is
+// running.
+void ConsoleServiceClientImpl::Init(dbus::Bus* bus) {
+  bus_ = chromeos::DBusThreadManager::Get()->GetSystemBus();
+
+  if (!bus_) {
+    LOG(ERROR) << "Unable to get system d-bus for the ConsoleServiceClient";

[Daniel Erat, 2014/10/31 18:15:02]

any reason that this shouldn't just be CHECK(bus_)? logging an error that no one
will see and returning doesn't seem ideal

[dsodman, 2014/10/31 18:43:24]

Done.

+    return;
+  }
+
+  bus_->RequestOwnership(chromeos::kConsoleServiceName,

[Daniel Erat, 2014/10/31 18:15:02]

it seems a bit strange to use a new service here instead of making this be part
of kLibCrosServiceName

[dsodman, 2014/10/31 18:43:25]

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.

+                         dbus::Bus::REQUIRE_PRIMARY_ALLOW_REPLACEMENT,
+                         base::Bind(&ConsoleServiceClientImpl::OnOwnership,
+                                    base::Unretained(this)));
+
+  exported_object_ = bus_->GetExportedObject(
+      dbus::ObjectPath(dbus::ObjectPath(chromeos::kConsoleServicePath)));
+
+  exported_object_->ExportMethod(
+      chromeos::kConsoleServiceInterface,
+      chromeos::kActivateConsole,
+      base::Bind(&ConsoleServiceClientImpl::ActivateConsole,
+                 weak_ptr_factory_.GetWeakPtr()),
+      base::Bind(&ConsoleServiceClientImpl::OnExported,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+namespace chromeos {
+
+ConsoleServiceClient::ConsoleServiceClient() {
+}
+
+ConsoleServiceClient::~ConsoleServiceClient() {
+}
+
+ConsoleServiceClient* ConsoleServiceClient::Create() {
+  s_console_service_client = new ConsoleServiceClientImpl();
+  CHECK_EQ(NULL, s_console_service_client);

[Daniel Erat, 2014/10/31 18:15:02]

this check is wrong. i think that steven meant that you should check that it's
NULL *before* allocating a new one, not after.

[dsodman, 2014/10/31 18:43:25]

I see.  Fixed.

+  return s_console_service_client;
+}
+
+ConsoleServiceClient* ConsoleServiceClient::GetInstance() {
+  return s_console_service_client;
+}
+
+}  // namespace chromeos