MPRIS: Refactor to use DBusPropertiesInterface

BUG=992948
R=steimel,thestig,hashimoto

Change-Id: I39cb1737c914ecb37065f544538113f9b2db7efc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1778584Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Tommy Steimel <steimel@chromium.org>
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#695159}

chrome/browser/ui/views/status_icons/status_icon_linux_dbus.cc

@@ -17,9 +17,9 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/dbus/menu/menu.h"
-#include "components/dbus/menu/properties_interface.h"
-#include "components/dbus/menu/success_barrier_callback.h"
-#include "components/dbus/menu/types.h"
+#include "components/dbus/properties/dbus_properties.h"
+#include "components/dbus/properties/success_barrier_callback.h"
+#include "components/dbus/properties/types.h"
 #include "components/dbus/thread_linux/dbus_thread_linux.h"
 #include "dbus/bus.h"
 #include "dbus/exported_object.h"
@@ -279,7 +279,7 @@ void StatusIconLinuxDbus::OnOwnership(const std::string& service_name,
       bus_->GetExportedObject(dbus::ObjectPath(kPathDbusMenu)), barrier_);
   UpdateMenuImpl(delegate_->GetMenuModel(), false);
 
-  properties_ = std::make_unique<DbusPropertiesInterface>(item_, barrier_);
+  properties_ = std::make_unique<DbusProperties>(item_, barrier_);
   properties_->RegisterInterface(kInterfaceStatusNotifierItem);
   auto set_property = [&](const std::string& property_name, auto&& value) {
     properties_->SetProperty(kInterfaceStatusNotifierItem, property_name,

chrome/browser/ui/views/status_icons/status_icon_linux_dbus.h

@@ -24,7 +24,7 @@ class ImageSkia;
 }  // namespace gfx
 
 class DbusMenu;
-class DbusPropertiesInterface;
+class DbusProperties;
 
 // A status icon following the StatusNotifierItem specification.
 // https://www.freedesktop.org/wiki/Specifications/StatusNotifierItem/StatusNotifierItem/
@@ -93,7 +93,7 @@ class StatusIconLinuxDbus : public views::StatusIconLinux,
 
   base::RepeatingCallback<void(bool)> barrier_;
 
-  std::unique_ptr<DbusPropertiesInterface> properties_;
+  std::unique_ptr<DbusProperties> properties_;
 
   std::unique_ptr<DbusMenu> menu_;
   // A menu that contains the click action (if there is a click action) and a

components/BUILD.gn

@@ -387,7 +387,10 @@ test("components_unittests") {
   }
 
   if (use_dbus) {
-    deps += [ "//components/dbus/menu:unit_tests" ]
+    deps += [
+      "//components/dbus/menu:unit_tests",
+      "//components/dbus/properties:unit_tests",
+    ]
   }
 
   if (enable_plugins) {

components/dbus/menu/BUILD.gn

@@ -10,12 +10,6 @@ component("menu") {
     "menu.h",
     "menu_property_list.cc",
     "menu_property_list.h",
-    "properties_interface.cc",
-    "properties_interface.h",
-    "success_barrier_callback.cc",
-    "success_barrier_callback.h",
-    "types.cc",
-    "types.h",
   ]
   defines = [ "IS_DBUS_IMPL" ]
   deps = [
@@ -23,6 +17,7 @@ component("menu") {
     "//base:i18n",
   ]
   public_deps = [
+    "//components/dbus/properties",
     "//dbus",
     "//skia",
     "//ui/base",
@@ -37,8 +32,6 @@ source_set("unit_tests") {
   testonly = true
   sources = [
     "menu_property_list_unittest.cc",
-    "success_barrier_callback_unittest.cc",
-    "types_unittest.cc",
   ]
   deps = [
     ":menu",

components/dbus/menu/menu.cc

@@ -15,8 +15,8 @@
 #include "base/memory/scoped_refptr.h"
 #include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
-#include "components/dbus/menu/properties_interface.h"
-#include "components/dbus/menu/success_barrier_callback.h"
+#include "components/dbus/properties/dbus_properties.h"
+#include "components/dbus/properties/success_barrier_callback.h"
 #include "ui/base/accelerators/accelerator.h"
 #include "ui/base/models/menu_model.h"
 #include "ui/base/models/simple_menu_model.h"
@@ -163,7 +163,7 @@ DbusMenu::DbusMenu(dbus::ExportedObject* exported_object,
         base::BindRepeating(&DbusMenu::OnExported, weak_factory_.GetWeakPtr()));
   }
 
-  properties_ = std::make_unique<DbusPropertiesInterface>(menu_, barrier_);
+  properties_ = std::make_unique<DbusProperties>(menu_, barrier_);
   properties_->RegisterInterface(kInterfaceDbusMenu);
   auto set_property = [&](const std::string& property_name, auto&& value) {
     properties_->SetProperty(kInterfaceDbusMenu, property_name,

components/dbus/menu/menu.h

@@ -16,7 +16,7 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "components/dbus/menu/menu_property_list.h"
-#include "components/dbus/menu/types.h"
+#include "components/dbus/properties/types.h"
 #include "dbus/bus.h"
 #include "dbus/exported_object.h"
 #include "dbus/message.h"
@@ -25,7 +25,7 @@ namespace ui {
 class MenuModel;
 }
 
-class DbusPropertiesInterface;
+class DbusProperties;
 
 // Implements the com.canonical.dbusmenu interface.
 class COMPONENT_EXPORT(DBUS) DbusMenu {
@@ -152,7 +152,7 @@ class COMPONENT_EXPORT(DBUS) DbusMenu {
 
   base::RepeatingCallback<void(bool)> barrier_;
 
-  std::unique_ptr<DbusPropertiesInterface> properties_;
+  std::unique_ptr<DbusProperties> properties_;
 
   uint32_t revision_ = 0;
   int32_t last_item_id_ = 0;

components/dbus/menu/menu_property_list.h

@@ -9,7 +9,7 @@
 #include <string>
 #include <vector>
 
-#include "components/dbus/menu/types.h"
+#include "components/dbus/properties/types.h"
 
 using MenuPropertyList = std::vector<std::string>;
 using MenuItemProperties = std::map<std::string, DbusVariant>;

components/dbus/menu/menu_property_list_unittest.cc

@@ -7,7 +7,7 @@
 #include <memory>
 
 #include "base/strings/utf_string_conversions.h"
-#include "components/dbus/menu/types.h"
+#include "components/dbus/properties/types.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/accelerators/accelerator.h"
 #include "ui/base/models/menu_model.h"

components/dbus/properties/BUILD.gn

+# Copyright 2019 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.
+
+component("properties") {
+  sources = [
+    "dbus_properties.cc",
+    "dbus_properties.h",
+    "success_barrier_callback.cc",
+    "success_barrier_callback.h",
+    "types.cc",
+    "types.h",
+  ]
+  defines = [ "IS_DBUS_IMPL" ]
+  deps = [
+    "//base",
+    "//base:i18n",
+  ]
+  public_deps = [
+    "//dbus",
+  ]
+}
+
+source_set("unit_tests") {
+  testonly = true
+  sources = [
+    "success_barrier_callback_unittest.cc",
+    "types_unittest.cc",
+  ]
+  deps = [
+    ":properties",
+    "//base",
+    "//testing/gtest",
+  ]
+}

components/dbus/properties/DEPS

+include_rules = [
+  "+dbus",
+]

components/dbus/properties/OWNERS

+thestig@chromium.org
+thomasanderson@chromium.org

components/dbus/menu/properties_interface.cc → components/dbus/properties/dbus_properties.cc

@@ -2,13 +2,13 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "components/dbus/menu/properties_interface.h"
+#include "components/dbus/properties/dbus_properties.h"
 
 #include <dbus/dbus-shared.h>
 
 #include "base/bind.h"
 #include "base/stl_util.h"
-#include "components/dbus/menu/success_barrier_callback.h"
+#include "components/dbus/properties/success_barrier_callback.h"
 #include "dbus/exported_object.h"
 #include "dbus/message.h"
 
@@ -24,19 +24,17 @@ const char kSignalPropertiesChanged[] = "PropertiesChanged";
 
 }  // namespace
 
-DbusPropertiesInterface::DbusPropertiesInterface(
-    dbus::ExportedObject* exported_object,
+DbusProperties::DbusProperties(dbus::ExportedObject* exported_object,
                                InitializedCallback callback)
     : exported_object_(exported_object) {
   static constexpr struct {
     const char* name;
-    void (DbusPropertiesInterface::*callback)(
-        dbus::MethodCall*,
+    void (DbusProperties::*callback)(dbus::MethodCall*,
                                      dbus::ExportedObject::ResponseSender);
   } methods[3] = {
-      {kMethodPropertiesGetAll, &DbusPropertiesInterface::OnGetAllProperties},
-      {kMethodPropertiesGet, &DbusPropertiesInterface::OnGetProperty},
-      {kMethodPropertiesSet, &DbusPropertiesInterface::OnSetProperty},
+      {kMethodPropertiesGetAll, &DbusProperties::OnGetAllProperties},
+      {kMethodPropertiesGet, &DbusProperties::OnGetProperty},
+      {kMethodPropertiesSet, &DbusProperties::OnSetProperty},
   };
 
   barrier_ = SuccessBarrierCallback(base::size(methods), std::move(callback));
@@ -44,27 +42,76 @@ DbusPropertiesInterface::DbusPropertiesInterface(
     exported_object_->ExportMethod(
         DBUS_INTERFACE_PROPERTIES, method.name,
         base::BindRepeating(method.callback, weak_factory_.GetWeakPtr()),
-        base::BindRepeating(&DbusPropertiesInterface::OnExported,
+        base::BindRepeating(&DbusProperties::OnExported,
                             weak_factory_.GetWeakPtr()));
   }
 }
 
-DbusPropertiesInterface::~DbusPropertiesInterface() = default;
+DbusProperties::~DbusProperties() = default;
 
-void DbusPropertiesInterface::RegisterInterface(const std::string& interface) {
+void DbusProperties::RegisterInterface(const std::string& interface) {
   bool inserted =
       properties_.emplace(interface, std::map<std::string, DbusVariant>{})
           .second;
   DCHECK(inserted);
 }
 
-void DbusPropertiesInterface::OnExported(const std::string& interface_name,
+DbusVariant* DbusProperties::GetProperty(const std::string& interface,
+                                         const std::string& property_name) {
+  auto interface_it = properties_.find(interface);
+  if (interface_it == properties_.end())
+    return nullptr;
+  auto name_it = interface_it->second.find(property_name);
+  return name_it != interface_it->second.end() ? &name_it->second : nullptr;
+}
+
+void DbusProperties::PropertyUpdated(const std::string& interface,
+                                     const std::string& property_name,
+                                     bool send_change) {
+  if (!initialized_)
+    return;
+
+  // |signal| follows the PropertiesChanged API:
+  // org.freedesktop.DBus.Properties.PropertiesChanged(
+  //     STRING interface_name,
+  //     DICT<STRING,VARIANT> changed_properties,
+  //     ARRAY<STRING> invalidated_properties);
+  dbus::Signal signal(DBUS_INTERFACE_PROPERTIES, kSignalPropertiesChanged);
+  dbus::MessageWriter writer(&signal);
+  writer.AppendString(interface);
+
+  if (send_change) {
+    // Changed properties.
+    dbus::MessageWriter array_writer(nullptr);
+    writer.OpenArray("{sv}", &array_writer);
+    dbus::MessageWriter dict_entry_writer(nullptr);
+    array_writer.OpenDictEntry(&dict_entry_writer);
+    dict_entry_writer.AppendString(property_name);
+    properties_[interface][property_name].Write(&dict_entry_writer);
+    array_writer.CloseContainer(&dict_entry_writer);
+    writer.CloseContainer(&array_writer);
+
+    // Invalidated properties.
+    writer.AppendArrayOfStrings({});
+  } else {
+    // Changed properties.
+    DbusDictionary().Write(&writer);
+
+    // Invalidated properties.
+    writer.AppendArrayOfStrings({property_name});
+  }
+
+  exported_object_->SendSignal(&signal);
+}
+
+void DbusProperties::OnExported(const std::string& interface_name,
                                 const std::string& method_name,
                                 bool success) {
+  initialized_ = success;
   barrier_.Run(success);
 }
 
-void DbusPropertiesInterface::OnGetAllProperties(
+void DbusProperties::OnGetAllProperties(
     dbus::MethodCall* method_call,
     dbus::ExportedObject::ResponseSender response_sender) {
   // org.freedesktop.DBus.Properties.GetAll(in STRING interface_name,
@@ -93,7 +140,7 @@ void DbusPropertiesInterface::OnGetAllProperties(
     writer.CloseContainer(&array_writer);
   } else if (interface == DBUS_INTERFACE_PROPERTIES) {
     // There are no properties to give for this interface.
-    DbusArray<DbusDictEntry<DbusString, DbusVariant>>().Write(&writer);
+    DbusDictionary().Write(&writer);
   } else {
     // The given interface is not supported, so return a null response.
     response = nullptr;
@@ -102,7 +149,7 @@ void DbusPropertiesInterface::OnGetAllProperties(
   response_sender.Run(std::move(response));
 }
 
-void DbusPropertiesInterface::OnGetProperty(
+void DbusProperties::OnGetProperty(
     dbus::MethodCall* method_call,
     dbus::ExportedObject::ResponseSender response_sender) {
   // org.freedesktop.DBus.Properties.Get(in STRING interface_name,
@@ -125,47 +172,10 @@ void DbusPropertiesInterface::OnGetProperty(
   response_sender.Run(std::move(response));
 }
 
-void DbusPropertiesInterface::OnSetProperty(
+void DbusProperties::OnSetProperty(
     dbus::MethodCall* method_call,
     dbus::ExportedObject::ResponseSender response_sender) {
   // Not needed for now.
   NOTIMPLEMENTED();
   response_sender.Run(dbus::Response::FromMethodCall(method_call));
 }
-
-void DbusPropertiesInterface::EmitPropertiesChangedSignal(
-    const std::string& interface,
-    const std::string& property_name,
-    bool send_change) {
-  // |signal| follows the PropertiesChanged API:
-  // org.freedesktop.DBus.Properties.PropertiesChanged(
-  //     STRING interface_name,
-  //     DICT<STRING,VARIANT> changed_properties,
-  //     ARRAY<STRING> invalidated_properties);
-  dbus::Signal signal(DBUS_INTERFACE_PROPERTIES, kSignalPropertiesChanged);
-  dbus::MessageWriter writer(&signal);
-  writer.AppendString(interface);
-
-  if (send_change) {
-    // Changed properties.
-    dbus::MessageWriter array_writer(nullptr);
-    writer.OpenArray("{sv}", &array_writer);
-    dbus::MessageWriter dict_entry_writer(nullptr);
-    array_writer.OpenDictEntry(&dict_entry_writer);
-    dict_entry_writer.AppendString(property_name);
-    properties_[interface][property_name].Write(&dict_entry_writer);
-    array_writer.CloseContainer(&dict_entry_writer);
-    writer.CloseContainer(&array_writer);
-
-    // Invalidated properties.
-    writer.AppendArrayOfStrings({});
-  } else {
-    // Changed properties.
-    DbusArray<DbusDictEntry<DbusString, DbusVariant>>().Write(&writer);
-
-    // Invalidated properties.
-    writer.AppendArrayOfStrings({property_name});
-  }
-
-  exported_object_->SendSignal(&signal);
-}

components/dbus/menu/properties_interface.h → components/dbus/properties/dbus_properties.h

@@ -2,28 +2,28 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef COMPONENTS_DBUS_MENU_PROPERTIES_INTERFACE_H_
-#define COMPONENTS_DBUS_MENU_PROPERTIES_INTERFACE_H_
+#ifndef COMPONENTS_DBUS_PROPERTIES_DBUS_PROPERTIES_H_
+#define COMPONENTS_DBUS_PROPERTIES_DBUS_PROPERTIES_H_
 
 #include "base/callback_forward.h"
 #include "base/component_export.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/stl_util.h"
-#include "components/dbus/menu/types.h"
+#include "components/dbus/properties/types.h"
 #include "dbus/bus.h"
 #include "dbus/exported_object.h"
 
 // https://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-properties
-class COMPONENT_EXPORT(DBUS) DbusPropertiesInterface {
+class COMPONENT_EXPORT(DBUS) DbusProperties {
  public:
   using InitializedCallback = base::OnceCallback<void(bool success)>;
 
   // Registers method handlers for the properties interface.  The handlers will
   // not be removed until the bus is shut down.
-  DbusPropertiesInterface(dbus::ExportedObject* exported_object,
+  DbusProperties(dbus::ExportedObject* exported_object,
                  InitializedCallback callback);
-  ~DbusPropertiesInterface();
+  ~DbusProperties();
 
   void RegisterInterface(const std::string& interface);
 
@@ -33,13 +33,28 @@ class COMPONENT_EXPORT(DBUS) DbusPropertiesInterface {
                    T&& value,
                    bool emit_signal = true,
                    bool send_change = true) {
-    auto it = properties_.find(interface);
-    DCHECK(it != properties_.end());
-    (it->second)[name] = MakeDbusVariant(std::move(value));
-    if (emit_signal)
-      EmitPropertiesChangedSignal(interface, name, send_change);
+    auto interface_it = properties_.find(interface);
+    DCHECK(interface_it != properties_.end());
+    auto property_it = interface_it->second.find(name);
+    DbusVariant new_value = MakeDbusVariant(std::move(value));
+    const bool send_signal =
+        emit_signal && (property_it == interface_it->second.end() ||
+                        property_it->second != new_value);
+    (interface_it->second)[name] = std::move(new_value);
+    if (send_signal)
+      PropertyUpdated(interface, name, send_change);
   }
 
+  DbusVariant* GetProperty(const std::string& interface,
+                           const std::string& property_name);
+
+  // If emitting a PropertiesChangedSignal is desired, this should be called
+  // after an existing property is modified through any means other than
+  // SetProperty().
+  void PropertyUpdated(const std::string& interface,
+                       const std::string& property_name,
+                       bool send_change = true);
+
  private:
   void OnExported(const std::string& interface_name,
                   const std::string& method_name,
@@ -52,9 +67,7 @@ class COMPONENT_EXPORT(DBUS) DbusPropertiesInterface {
   void OnSetProperty(dbus::MethodCall* method_call,
                      dbus::ExportedObject::ResponseSender response_sender);
 
-  void EmitPropertiesChangedSignal(const std::string& interface,
-                                   const std::string& property_name,
-                                   bool send_change);
+  bool initialized_ = false;
 
   dbus::ExportedObject* exported_object_ = nullptr;
 
@@ -64,9 +77,9 @@ class COMPONENT_EXPORT(DBUS) DbusPropertiesInterface {
   // from property name to property value.
   std::map<std::string, std::map<std::string, DbusVariant>> properties_;
 
-  base::WeakPtrFactory<DbusPropertiesInterface> weak_factory_{this};
+  base::WeakPtrFactory<DbusProperties> weak_factory_{this};
 
-  DISALLOW_COPY_AND_ASSIGN(DbusPropertiesInterface);
+  DISALLOW_COPY_AND_ASSIGN(DbusProperties);
 };
 
-#endif  // COMPONENTS_DBUS_MENU_PROPERTIES_INTERFACE_H_
+#endif  // COMPONENTS_DBUS_PROPERTIES_DBUS_PROPERTIES_H_

components/dbus/menu/success_barrier_callback.cc → components/dbus/properties/success_barrier_callback.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "components/dbus/menu/success_barrier_callback.h"
+#include "components/dbus/properties/success_barrier_callback.h"
 
 #include "base/bind.h"
 #include "base/callback.h"

components/dbus/menu/success_barrier_callback.h → components/dbus/properties/success_barrier_callback.h

@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef COMPONENTS_DBUS_MENU_SUCCESS_BARRIER_CALLBACK_H_
-#define COMPONENTS_DBUS_MENU_SUCCESS_BARRIER_CALLBACK_H_
+#ifndef COMPONENTS_DBUS_PROPERTIES_SUCCESS_BARRIER_CALLBACK_H_
+#define COMPONENTS_DBUS_PROPERTIES_SUCCESS_BARRIER_CALLBACK_H_
 
 #include <cstddef>
 
@@ -20,4 +20,4 @@ base::RepeatingCallback<void(bool)> SuccessBarrierCallback(
     size_t num_calls,
     base::OnceCallback<void(bool)> done_callback);
 
-#endif  // COMPONENTS_DBUS_MENU_SUCCESS_BARRIER_CALLBACK_H_
+#endif  // COMPONENTS_DBUS_PROPERTIES_SUCCESS_BARRIER_CALLBACK_H_

components/dbus/menu/success_barrier_callback_unittest.cc → components/dbus/properties/success_barrier_callback_unittest.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "components/dbus/menu/success_barrier_callback.h"
+#include "components/dbus/properties/success_barrier_callback.h"
 
 #include "base/bind.h"
 #include "base/callback.h"

components/dbus/menu/types.cc → components/dbus/properties/types.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "components/dbus/menu/types.h"
+#include "components/dbus/properties/types.h"
 
 #include "dbus/message.h"
 #include "dbus/object_path.h"
@@ -57,6 +57,32 @@ std::string DbusUint32::GetSignature() {
   return "u";
 }
 
+DbusInt64::DbusInt64(int64_t value) : value_(value) {}
+DbusInt64::DbusInt64(DbusInt64&& other) = default;
+DbusInt64::~DbusInt64() = default;
+
+void DbusInt64::Write(dbus::MessageWriter* writer) const {
+  writer->AppendInt64(value_);
+}
+
+// static
+std::string DbusInt64::GetSignature() {
+  return "x";
+}
+
+DbusDouble::DbusDouble(double value) : value_(value) {}
+DbusDouble::DbusDouble(DbusDouble&& other) = default;
+DbusDouble::~DbusDouble() = default;
+
+void DbusDouble::Write(dbus::MessageWriter* writer) const {
+  writer->AppendDouble(value_);
+}
+
+// static
+std::string DbusDouble::GetSignature() {
+  return "d";
+}
+
 DbusString::DbusString(const std::string& value) : value_(value) {}
 DbusString::DbusString(DbusString&& other) = default;
 DbusString::~DbusString() = default;
@@ -131,3 +157,32 @@ void DbusByteArray::Write(dbus::MessageWriter* writer) const {
 std::string DbusByteArray::GetSignature() {
   return "ay";  // lmao
 }
+
+DbusDictionary::DbusDictionary() = default;
+DbusDictionary::DbusDictionary(DbusDictionary&& other) = default;
+DbusDictionary::~DbusDictionary() = default;
+
+bool DbusDictionary::Put(const std::string& key, DbusVariant&& value) {
+  auto it = value_.find(key);
+  const bool updated = it == value_.end() || it->second != value;
+  value_[key] = std::move(value);
+  return updated;
+}
+
+void DbusDictionary::Write(dbus::MessageWriter* writer) const {
+  dbus::MessageWriter array_writer(nullptr);
+  writer->OpenArray("{sv}", &array_writer);
+  for (const auto& pair : value_) {
+    dbus::MessageWriter dict_entry_writer(nullptr);
+    array_writer.OpenDictEntry(&dict_entry_writer);
+    dict_entry_writer.AppendString(pair.first);
+    pair.second.Write(&dict_entry_writer);
+    array_writer.CloseContainer(&dict_entry_writer);
+  }
+  writer->CloseContainer(&array_writer);
+}
+
+// static
+std::string DbusDictionary::GetSignature() {
+  return "a{sv}";
+}

components/dbus/menu/types.h → components/dbus/properties/types.h

@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef COMPONENTS_DBUS_MENU_TYPES_H_
-#define COMPONENTS_DBUS_MENU_TYPES_H_
+#ifndef COMPONENTS_DBUS_PROPERTIES_TYPES_H_
+#define COMPONENTS_DBUS_PROPERTIES_TYPES_H_
 
 #include <stdint.h>
 
@@ -150,6 +150,40 @@ class COMPONENT_EXPORT(DBUS) DbusUint32 : public DbusTypeImpl<DbusUint32> {
   uint32_t value_;
 };
 
+class COMPONENT_EXPORT(DBUS) DbusInt64 : public DbusTypeImpl<DbusInt64> {
+ public:
+  explicit DbusInt64(int64_t value);
+  DbusInt64(DbusInt64&& other);
+  ~DbusInt64() override;
+
+  // DbusType:
+  void Write(dbus::MessageWriter* writer) const override;
+
+  static std::string GetSignature();
+
+ private:
+  friend class DbusTypeImpl<DbusInt64>;
+
+  int64_t value_;
+};
+
+class COMPONENT_EXPORT(DBUS) DbusDouble : public DbusTypeImpl<DbusDouble> {
+ public:
+  explicit DbusDouble(double value);
+  DbusDouble(DbusDouble&& other);
+  ~DbusDouble() override;
+
+  // DbusType:
+  void Write(dbus::MessageWriter* writer) const override;
+
+  static std::string GetSignature();
+
+ private:
+  friend class DbusTypeImpl<DbusDouble>;
+
+  double value_;
+};
+
 class COMPONENT_EXPORT(DBUS) DbusString : public DbusTypeImpl<DbusString> {
  public:
   explicit DbusString(const std::string& value);
@@ -192,6 +226,13 @@ class COMPONENT_EXPORT(DBUS) DbusVariant : public DbusTypeImpl<DbusVariant> {
   DbusVariant(DbusVariant&& other);
   ~DbusVariant() override;
 
+  template <typename T>
+  T* GetAs() {
+    return value_ && value_->GetSignatureDynamic() == T::GetSignature()
+               ? reinterpret_cast<T*>(value_.get())
+               : nullptr;
+  }
+
   DbusVariant& operator=(DbusVariant&& other);
 
   explicit operator bool() const;
@@ -337,4 +378,31 @@ auto MakeDbusDictEntry(K&& k, V&& v) {
   return DbusDictEntry<K, V>(std::move(k), std::move(v));
 }
 
-#endif  // COMPONENTS_DBUS_MENU_TYPES_H_
+// A convenience class for DbusArray<DbusDictEntry<DbusString, DbusVariant>>,
+// which is a common idiom for DBus APIs.  Except this class has some subtle
+// differences:
+//   1. Duplicate keys are not allowed.
+//   2. You cannot control the ordering of keys.  They will always be in sorted
+//      order.
+class COMPONENT_EXPORT(DBUS) DbusDictionary
+    : public DbusTypeImpl<DbusDictionary> {
+ public:
+  DbusDictionary();
+  DbusDictionary(DbusDictionary&& other);
+  ~DbusDictionary() override;
+
+  // Returns true iff the value corresponding to |key| was updated.
+  bool Put(const std::string& key, DbusVariant&& value);
+
+  // DbusType:
+  void Write(dbus::MessageWriter* writer) const override;
+
+  static std::string GetSignature();
+
+ private:
+  friend class DbusTypeImpl<DbusDictionary>;
+
+  std::map<std::string, DbusVariant> value_;
+};
+
+#endif  // COMPONENTS_DBUS_PROPERTIES_TYPES_H_

components/dbus/menu/types_unittest.cc → components/dbus/properties/types_unittest.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "components/dbus/menu/types.h"
+#include "components/dbus/properties/types.h"
 
 #include "base/memory/ref_counted_memory.h"
 #include "dbus/object_path.h"

ui/base/mpris/BUILD.gn

@@ -16,6 +16,7 @@ component("mpris") {
   deps = [
     "//base",
     "//build:branding_buildflags",
+    "//components/dbus/properties",
     "//components/dbus/thread_linux",
     "//dbus",
   ]

ui/base/mpris/mpris_service_impl.h

@@ -11,20 +11,16 @@
 #include "base/containers/flat_map.h"
 #include "base/containers/flat_set.h"
 #include "base/memory/scoped_refptr.h"
-#include "base/memory/singleton.h"
 #include "base/observer_list.h"
 #include "base/timer/timer.h"
+#include "components/dbus/properties/types.h"
 #include "dbus/bus.h"
 #include "dbus/exported_object.h"
 #include "ui/base/mpris/mpris_service.h"
 
-namespace base {
-class DictionaryValue;
-class Value;
-}  // namespace base
+class DbusProperties;
 
 namespace dbus {
-class MessageWriter;
 class MethodCall;
 }  // namespace dbus
 
@@ -64,6 +60,7 @@ class COMPONENT_EXPORT(MPRIS) MprisServiceImpl : public MprisService {
   void OnExported(const std::string& interface_name,
                   const std::string& method_name,
                   bool success);
+  void OnInitialized(bool success);
   void OnOwnership(const std::string& service_name, bool success);
 
   // org.mpris.MediaPlayer2.Player interface.
@@ -84,47 +81,12 @@ class COMPONENT_EXPORT(MPRIS) MprisServiceImpl : public MprisService {
   void DoNothing(dbus::MethodCall* method_call,
                  dbus::ExportedObject::ResponseSender response_sender);
 
-  // org.freedesktop.DBus.Properties interface.
-  void GetAllProperties(dbus::MethodCall* method_call,
-                        dbus::ExportedObject::ResponseSender response_sender);
-  void GetProperty(dbus::MethodCall* method_call,
-                   dbus::ExportedObject::ResponseSender response_sender);
-
-  using PropertyMap = base::flat_map<std::string, base::Value>;
-
-  // Sets a value on the given PropertyMap and sends a PropertiesChanged signal
-  // if necessary.
-  void SetPropertyInternal(PropertyMap& property_map,
-                           const std::string& property_name,
-                           const base::Value& new_value);
-
   // Sets a value on the Metadata property map and sends a PropertiesChanged
   // signal if necessary.
   void SetMetadataPropertyInternal(const std::string& property_name,
-                                   const base::Value& new_value);
-
-  // Updates a timer to debounce calls to |EmitPropertiesChangedSignal|.
-  void EmitPropertiesChangedSignalDebounced();
-
-  // Emits a org.freedesktop.DBus.Properties.PropertiesChanged signal for the
-  // given map of changed properties.
-  void EmitPropertiesChangedSignal();
+                                   DbusVariant&& new_value);
 
-  // Writes all properties onto writer.
-  void AddPropertiesToWriter(dbus::MessageWriter* writer,
-                             const PropertyMap& properties);
-
-  // Writes the metadata property onto writer. Metadata is handled differently
-  // than other properties since it has sub-properties that need to be handled
-  // as non-variants.
-  void AddMetadataToWriter(dbus::MessageWriter* writer,
-                           const base::DictionaryValue* metadata);
-
-  // Map of org.mpris.MediaPlayer2 interface properties.
-  PropertyMap media_player2_properties_;
-
-  // Map of org.mpris.MediaPlayer2.Player interface properties.
-  PropertyMap media_player2_player_properties_;
+  std::unique_ptr<DbusProperties> properties_;
 
   scoped_refptr<dbus::Bus> bus_;
   dbus::ExportedObject* exported_object_;
@@ -132,24 +94,11 @@ class COMPONENT_EXPORT(MPRIS) MprisServiceImpl : public MprisService {
   // The generated service name given to |bus_| when requesting ownership.
   const std::string service_name_;
 
-  // The number of methods that have been successfully exported through
-  // |exported_object_|.
-  int num_methods_exported_ = 0;
+  base::RepeatingCallback<void(bool)> barrier_;
 
   // True if we have finished creating the DBus service and received ownership.
   bool service_ready_ = false;
 
-  // True if we failed to start the MPRIS DBus service.
-  bool service_failed_to_start_ = false;
-
-  // Used to only send 1 PropertiesChanged signal when many properties are
-  // changed at once.
-  base::OneShotTimer properties_changed_debounce_timer_;
-
-  // Holds a list of properties that have changed since the last time we emitted
-  // a PropertiesChanged signal.
-  base::flat_set<std::string> changed_properties_;
-
   base::ObserverList<MprisServiceObserver> observers_;
 
   DISALLOW_COPY_AND_ASSIGN(MprisServiceImpl);