Fix status icons in GNOME and Pantheon environments

This CL changes StatusIconLinuxDbus to send the icon as a file instead
of as a DBus property.  All of the StatusNotifierItem client libraries
send icons as files, so some StatusNotifierHosts don't behave well when
given an icon property since that codepath wasn't ever tested.

gnome-shell-extension-appindicator doesn't scale property icons, but
does scale file icons.  wingpanel-indicator-ayatana (used on Pantheon)
only supports file icons.  There's no way using StatusNotifierWatcher to
get information about specific StatusNotifierHosts.  So the only way we
can detect these two hosts is based on the desktop environment.

To prevent race conditions involved with cleanup up the icon file,
StatusIconLinuxDbus must be made ref-counted, which adds some
unfortunate complexity.

BUG=10034265
R=thestig

Change-Id: I79db4b83f6ab49319069b11ab761bd23985eeba3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1815055
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#699008}

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

@@ -8,8 +8,11 @@
 #include <string>
 
 #include "base/callback_forward.h"
+#include "base/files/file_path.h"
 #include "base/macros.h"
+#include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
+#include "base/sequenced_task_runner.h"
 #include "chrome/browser/ui/views/status_icons/concat_menu_model.h"
 #include "dbus/bus.h"
 #include "dbus/exported_object.h"
@@ -29,10 +32,10 @@ class DbusProperties;
 // A status icon following the StatusNotifierItem specification.
 // https://www.freedesktop.org/wiki/Specifications/StatusNotifierItem/StatusNotifierItem/
 class StatusIconLinuxDbus : public views::StatusIconLinux,
-                            public ui::SimpleMenuModel::Delegate {
+                            public ui::SimpleMenuModel::Delegate,
+                            public base::RefCounted<StatusIconLinuxDbus> {
  public:
   StatusIconLinuxDbus();
-  ~StatusIconLinuxDbus() override;
 
   // StatusIcon:
   void SetIcon(const gfx::ImageSkia& image) override;
@@ -44,6 +47,10 @@ class StatusIconLinuxDbus : public views::StatusIconLinux,
   void ExecuteCommand(int command_id, int event_flags) override;
 
  private:
+  friend class base::RefCounted<StatusIconLinuxDbus>;
+
+  ~StatusIconLinuxDbus() override;
+
   // Step 0: send the request to verify that the StatusNotifierWatcher service
   // is owned.
   void CheckStatusNotifierWatcherHasOwner();
@@ -85,6 +92,12 @@ class StatusIconLinuxDbus : public views::StatusIconLinux,
 
   void UpdateMenuImpl(ui::MenuModel* model, bool send_signal);
 
+  void SetIconImpl(const gfx::ImageSkia& image, bool send_signals);
+
+  void OnIconFileWritten(const base::FilePath& icon_file);
+
+  void CleanupIconFile();
+
   scoped_refptr<dbus::Bus> bus_;
 
   int service_id_ = 0;
@@ -111,6 +124,11 @@ class StatusIconLinuxDbus : public views::StatusIconLinux,
   // our own menu.
   std::unique_ptr<views::MenuRunner> menu_runner_;
 
+  const bool should_write_icon_to_file_;
+  const scoped_refptr<base::SequencedTaskRunner> icon_task_runner_;
+  size_t icon_file_id_ = 0;
+  base::FilePath icon_file_;
+
   base::WeakPtrFactory<StatusIconLinuxDbus> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(StatusIconLinuxDbus);

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

@@ -8,6 +8,7 @@
 
 #include "base/feature_list.h"
 #include "base/memory/ptr_util.h"
+#include "base/memory/scoped_refptr.h"
 #include "chrome/browser/ui/ui_features.h"
 #include "ui/message_center/public/cpp/notifier_id.h"
 #include "ui/views/linux_ui/linux_ui.h"
@@ -42,11 +43,11 @@ gfx::ImageSkia GetBestImageRep(const gfx::ImageSkia& image) {
 }  // namespace
 
 StatusIconLinuxWrapper::StatusIconLinuxWrapper(
-    std::unique_ptr<views::StatusIconLinux> status_icon,
+    views::StatusIconLinux* status_icon,
     StatusIconType status_icon_type,
     const gfx::ImageSkia& image,
     const base::string16& tool_tip)
-    : status_icon_(std::move(status_icon)),
+    : status_icon_(status_icon),
       status_icon_type_(status_icon_type),
       image_(GetBestImageRep(image)),
       tool_tip_(tool_tip),
@@ -54,6 +55,28 @@ StatusIconLinuxWrapper::StatusIconLinuxWrapper(
   status_icon_->SetDelegate(this);
 }
 
+#if defined(USE_DBUS)
+StatusIconLinuxWrapper::StatusIconLinuxWrapper(
+    scoped_refptr<StatusIconLinuxDbus> status_icon,
+    const gfx::ImageSkia& image,
+    const base::string16& tool_tip)
+    : StatusIconLinuxWrapper(status_icon.get(), kTypeDbus, image, tool_tip) {
+  status_icon_dbus_ = status_icon;
+}
+#endif
+
+StatusIconLinuxWrapper::StatusIconLinuxWrapper(
+    std::unique_ptr<views::StatusIconLinux> status_icon,
+    StatusIconType status_icon_type,
+    const gfx::ImageSkia& image,
+    const base::string16& tool_tip)
+    : StatusIconLinuxWrapper(status_icon.get(),
+                             status_icon_type,
+                             image,
+                             tool_tip) {
+  status_icon_linux_ = std::move(status_icon);
+}
+
 StatusIconLinuxWrapper::~StatusIconLinuxWrapper() {
   if (menu_model_)
     menu_model_->RemoveObserver(this);
@@ -103,7 +126,9 @@ void StatusIconLinuxWrapper::OnImplInitializationFailed() {
   switch (status_icon_type_) {
     case kTypeDbus:
 #if defined(USE_X11)
-      status_icon_ = std::make_unique<StatusIconLinuxX11>();
+      status_icon_dbus_.reset();
+      status_icon_linux_ = std::make_unique<StatusIconLinuxX11>();
+      status_icon_ = status_icon_linux_.get();
       status_icon_type_ = kTypeX11;
       status_icon_->SetDelegate(this);
       return;
@@ -113,7 +138,8 @@ void StatusIconLinuxWrapper::OnImplInitializationFailed() {
       FALLTHROUGH;
 #endif
     case kTypeX11:
-      status_icon_.reset();
+      status_icon_linux_.reset();
+      status_icon_ = nullptr;
       status_icon_type_ = kTypeOther;
       if (menu_model_)
         menu_model_->RemoveObserver(this);
@@ -136,7 +162,7 @@ StatusIconLinuxWrapper::CreateWrappedStatusIcon(
   if (base::FeatureList::IsEnabled(features::kEnableDbusAndX11StatusIcons)) {
 #if defined(USE_DBUS)
     return base::WrapUnique(new StatusIconLinuxWrapper(
-        std::make_unique<StatusIconLinuxDbus>(), kTypeDbus, image, tool_tip));
+        base::MakeRefCounted<StatusIconLinuxDbus>(), image, tool_tip));
 #elif defined(USE_X11)
     return base::WrapUnique(new StatusIconLinuxWrapper(
         std::make_unique<StatusIconLinuxX11>(), kTypeX11, image, tool_tip));

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

@@ -8,10 +8,13 @@
 #include <memory>
 
 #include "base/macros.h"
+#include "base/memory/scoped_refptr.h"
 #include "chrome/browser/status_icons/desktop_notification_balloon.h"
 #include "chrome/browser/status_icons/status_icon.h"
 #include "ui/views/linux_ui/status_icon_linux.h"
 
+class StatusIconLinuxDbus;
+
 // Wrapper class for StatusIconLinux that implements the standard StatusIcon
 // interface. Also handles callbacks from StatusIconLinux.
 class StatusIconLinuxWrapper : public StatusIcon,
@@ -59,6 +62,15 @@ class StatusIconLinuxWrapper : public StatusIcon,
 
   // A status icon wrapper should only be created by calling
   // CreateWrappedStatusIcon().
+  StatusIconLinuxWrapper(views::StatusIconLinux* status_icon,
+                         StatusIconType status_icon_type,
+                         const gfx::ImageSkia& image,
+                         const base::string16& tool_tip);
+#if defined(USE_DBUS)
+  StatusIconLinuxWrapper(scoped_refptr<StatusIconLinuxDbus> status_icon,
+                         const gfx::ImageSkia& image,
+                         const base::string16& tool_tip);
+#endif
   StatusIconLinuxWrapper(std::unique_ptr<views::StatusIconLinux> status_icon,
                          StatusIconType status_icon_type,
                          const gfx::ImageSkia& image,
@@ -67,7 +79,14 @@ class StatusIconLinuxWrapper : public StatusIcon,
   // Notification balloon.
   DesktopNotificationBalloon notification_;
 
-  std::unique_ptr<views::StatusIconLinux> status_icon_;
+  // The status icon may be ref-counted (via |status_icon_dbus_|) or owned by
+  // |this| (via |status_icon_linux_|).  Either way, |status_icon_| points to
+  // the underlying object.
+#if defined(USE_DBUS)
+  scoped_refptr<StatusIconLinuxDbus> status_icon_dbus_;
+#endif
+  std::unique_ptr<views::StatusIconLinux> status_icon_linux_;
+  views::StatusIconLinux* status_icon_;
   StatusIconType status_icon_type_;
 
   gfx::ImageSkia image_;