Optimize pointer usage for NotificationImageRetainer object

Previously, a pointer to the NotificationImageRetainer object is stored in
NotificationTemplateBuilder. This is unnecessary as this object is only used
by two private methods called within Build() and never used again. This CL
changed to pass the pointer into those functions directly.

Separately, a GetCleanupTask() method is added for NotificationImageRetainer
so that it doesn't need to expose AsWeakPtr() to the public.

Bug: 888276
Change-Id: I4a7fa2a39e30c739f9adb7021b4809a4b56062fa
Reviewed-on: https://chromium-review.googlesource.com/c/1292712
Commit-Queue: Xi Cheng <chengx@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Reviewed-by: Finnur Thorarinsson <finnur@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602496}

chrome/browser/notifications/notification_platform_bridge_win.cc

@@ -16,7 +16,6 @@
 #include "base/feature_list.h"
 #include "base/hash.h"
 #include "base/logging.h"
-#include "base/memory/weak_ptr.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/sequenced_task_runner.h"
 #include "base/strings/string_number_conversions.h"
@@ -144,8 +143,7 @@ class NotificationPlatformBridgeWinImpl
     DCHECK(notification_task_runner_);
     content::BrowserThread::PostAfterStartupTask(
         FROM_HERE, notification_task_runner_,
-        base::BindOnce(&NotificationImageRetainer::CleanupFilesFromPrevSessions,
-                       image_retainer_->AsWeakPtr()));
+        image_retainer_->GetCleanupTask());
   }
 
   // Obtain an IToastNotification interface from a given XML (provided by the
@@ -353,8 +351,8 @@ class NotificationPlatformBridgeWinImpl
                                    profile_id, incognito,
                                    notification->origin_url());
     std::unique_ptr<NotificationTemplateBuilder> notification_template =
-        NotificationTemplateBuilder::Build(image_retainer_->AsWeakPtr(),
-                                           launch_id, *notification);
+        NotificationTemplateBuilder::Build(image_retainer_.get(), launch_id,
+                                           *notification);
     mswr::ComPtr<winui::Notifications::IToastNotification> toast =
         GetToastNotification(*notification, *notification_template, profile_id,
                              incognito);

chrome/browser/notifications/notification_platform_bridge_win_unittest.cc

@@ -14,7 +14,6 @@
 #include <wrl/implements.h>
 
 #include "base/hash.h"
-#include "base/memory/weak_ptr.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
@@ -73,10 +72,10 @@ class NotificationPlatformBridgeWinTest : public testing::Test {
         message_center::NotifierId(origin),
         message_center::RichNotificationData(), nullptr /* delegate */);
     notification->set_renotify(renotify);
-    auto image_retainer = std::make_unique<MockNotificationImageRetainer>();
+    MockNotificationImageRetainer image_retainer;
     std::unique_ptr<NotificationTemplateBuilder> builder =
-        NotificationTemplateBuilder::Build(image_retainer->AsWeakPtr(),
-                                           launch_id, *notification);
+        NotificationTemplateBuilder::Build(&image_retainer, launch_id,
+                                           *notification);
 
     mswr::ComPtr<winui::Notifications::IToastNotification> toast =
         bridge->GetToastNotificationForTesting(*notification, *builder,

chrome/browser/notifications/win/notification_image_retainer.cc

@@ -159,9 +159,10 @@ base::FilePath NotificationImageRetainer::RegisterTemporaryImage(
   return data_write_success ? temp_file : base::FilePath();
 }
 
-base::WeakPtr<NotificationImageRetainer>
-NotificationImageRetainer::AsWeakPtr() {
-  return weak_ptr_factory_.GetWeakPtr();
+base::OnceClosure NotificationImageRetainer::GetCleanupTask() {
+  return base::BindOnce(
+      &NotificationImageRetainer::CleanupFilesFromPrevSessions,
+      weak_ptr_factory_.GetWeakPtr());
 }
 
 void NotificationImageRetainer::DeleteExpiredFiles() {

chrome/browser/notifications/win/notification_image_retainer.h

@@ -9,6 +9,7 @@
 #include <utility>
 #include <vector>
 
+#include "base/callback.h"
 #include "base/files/file_path.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
@@ -53,7 +54,9 @@ class NotificationImageRetainer {
   // FilePath if file creation fails.
   virtual base::FilePath RegisterTemporaryImage(const gfx::Image& image);
 
-  base::WeakPtr<NotificationImageRetainer> AsWeakPtr();
+  // Returns a closure that, when run, performs cleanup operations. This closure
+  // must be run on the notification sequence.
+  base::OnceClosure GetCleanupTask();
 
   const base::FilePath& image_dir() { return image_dir_; }
 

chrome/browser/notifications/win/notification_template_builder.cc

@@ -82,11 +82,13 @@ const char* NotificationTemplateBuilder::context_menu_label_override_ = nullptr;
 
 // static
 std::unique_ptr<NotificationTemplateBuilder> NotificationTemplateBuilder::Build(
-    base::WeakPtr<NotificationImageRetainer> notification_image_retainer,
+    NotificationImageRetainer* image_retainer,
     const NotificationLaunchId& launch_id,
     const message_center::Notification& notification) {
-  std::unique_ptr<NotificationTemplateBuilder> builder = base::WrapUnique(
-      new NotificationTemplateBuilder(notification_image_retainer));
+  DCHECK(image_retainer);
+
+  // Use base::WrapUnique + new because the constructor is private.
+  auto builder = base::WrapUnique(new NotificationTemplateBuilder());
 
   builder->StartToastElement(launch_id, notification);
   builder->StartVisualElement();
@@ -116,10 +118,10 @@ std::unique_ptr<NotificationTemplateBuilder> NotificationTemplateBuilder::Build(
     builder->WriteTextElement(attribution, TextType::ATTRIBUTION);
 
   if (!notification.icon().IsEmpty())
-    builder->WriteIconElement(notification);
+    builder->WriteIconElement(image_retainer, notification);
 
   if (!notification.image().IsEmpty())
-    builder->WriteLargeImageElement(notification);
+    builder->WriteLargeImageElement(image_retainer, notification);
 
   if (notification.type() == message_center::NOTIFICATION_TYPE_PROGRESS)
     builder->WriteProgressElement(notification);
@@ -129,7 +131,7 @@ std::unique_ptr<NotificationTemplateBuilder> NotificationTemplateBuilder::Build(
 
   builder->StartActionsElement();
   if (!notification.buttons().empty())
-    builder->AddActions(notification, launch_id);
+    builder->AddActions(image_retainer, notification, launch_id);
   builder->EnsureReminderHasButton(notification, launch_id);
   builder->AddContextMenu(launch_id);
   builder->EndActionsElement();
@@ -142,10 +144,8 @@ std::unique_ptr<NotificationTemplateBuilder> NotificationTemplateBuilder::Build(
   return builder;
 }
 
-NotificationTemplateBuilder::NotificationTemplateBuilder(
-    base::WeakPtr<NotificationImageRetainer> notification_image_retainer)
-    : xml_writer_(std::make_unique<XmlWriter>()),
-      image_retainer_(notification_image_retainer) {
+NotificationTemplateBuilder::NotificationTemplateBuilder()
+    : xml_writer_(std::make_unique<XmlWriter>()) {
   xml_writer_->StartWriting();
 }
 
@@ -247,24 +247,24 @@ void NotificationTemplateBuilder::WriteItems(
 }
 
 void NotificationTemplateBuilder::WriteIconElement(
+    NotificationImageRetainer* image_retainer,
     const message_center::Notification& notification) {
-  WriteImageElement(notification.icon(), kPlacementAppLogoOverride,
-                    kHintCropNone);
+  WriteImageElement(image_retainer, notification.icon(),
+                    kPlacementAppLogoOverride, kHintCropNone);
 }
 
 void NotificationTemplateBuilder::WriteLargeImageElement(
+    NotificationImageRetainer* image_retainer,
     const message_center::Notification& notification) {
-  WriteImageElement(notification.image(), kHero, std::string());
+  WriteImageElement(image_retainer, notification.image(), kHero, std::string());
 }
 
 void NotificationTemplateBuilder::WriteImageElement(
+    NotificationImageRetainer* image_retainer,
     const gfx::Image& image,
     const std::string& placement,
     const std::string& hint_crop) {
-  // Although image_retainer_ is a WeakPtr, it should never be nullptr here.
-  DCHECK(image_retainer_);
-
-  base::FilePath path = image_retainer_->RegisterTemporaryImage(image);
+  base::FilePath path = image_retainer->RegisterTemporaryImage(image);
   if (!path.empty()) {
     xml_writer_->StartElement(kImageElement);
     xml_writer_->AddAttribute(kPlacement, placement);
@@ -289,6 +289,7 @@ void NotificationTemplateBuilder::WriteProgressElement(
 }
 
 void NotificationTemplateBuilder::AddActions(
+    NotificationImageRetainer* image_retainer,
     const message_center::Notification& notification,
     const NotificationLaunchId& launch_id) {
   const std::vector<message_center::ButtonInfo>& buttons =
@@ -313,7 +314,7 @@ void NotificationTemplateBuilder::AddActions(
   }
 
   for (size_t i = 0; i < buttons.size(); ++i)
-    WriteActionElement(buttons[i], i, launch_id);
+    WriteActionElement(image_retainer, buttons[i], i, launch_id);
 }
 
 void NotificationTemplateBuilder::AddContextMenu(
@@ -343,6 +344,7 @@ void NotificationTemplateBuilder::WriteAudioSilentElement() {
 }
 
 void NotificationTemplateBuilder::WriteActionElement(
+    NotificationImageRetainer* image_retainer,
     const message_center::ButtonInfo& button,
     int index,
     NotificationLaunchId copied_launch_id) {
@@ -353,10 +355,7 @@ void NotificationTemplateBuilder::WriteActionElement(
   xml_writer_->AddAttribute(kArguments, copied_launch_id.Serialize());
 
   if (!button.icon.IsEmpty()) {
-    // Although image_retainer_ is a WeakPtr, it should never be nullptr here.
-    DCHECK(image_retainer_);
-
-    base::FilePath path = image_retainer_->RegisterTemporaryImage(button.icon);
+    base::FilePath path = image_retainer->RegisterTemporaryImage(button.icon);
     if (!path.empty())
       xml_writer_->AddAttribute(kImageUri, path.AsUTF8Unsafe());
   }

chrome/browser/notifications/win/notification_template_builder.h

@@ -10,7 +10,6 @@
 #include <vector>
 
 #include "base/macros.h"
-#include "base/memory/weak_ptr.h"
 #include "base/strings/string16.h"
 #include "chrome/browser/notifications/notification_common.h"
 
@@ -47,7 +46,7 @@ class NotificationTemplateBuilder {
  public:
   // Builds the notification template for the given |notification|.
   static std::unique_ptr<NotificationTemplateBuilder> Build(
-      base::WeakPtr<NotificationImageRetainer> notification_image_retainer,
+      NotificationImageRetainer* image_retainer,
       const NotificationLaunchId& launch_id,
       const message_center::Notification& notification);
 
@@ -64,8 +63,7 @@ class NotificationTemplateBuilder {
   // The different types of text nodes to output.
   enum class TextType { NORMAL, ATTRIBUTION };
 
-  NotificationTemplateBuilder(
-      base::WeakPtr<NotificationImageRetainer> notification_image_retainer);
+  NotificationTemplateBuilder();
 
   // Formats the |origin| for display in the notification template.
   std::string FormatOrigin(const GURL& origin) const;
@@ -93,14 +91,17 @@ class NotificationTemplateBuilder {
   void WriteItems(const std::vector<message_center::NotificationItem>& items);
 
   // Writes the <image> element for the notification icon.
-  void WriteIconElement(const message_center::Notification& notification);
+  void WriteIconElement(NotificationImageRetainer* image_retainer,
+                        const message_center::Notification& notification);
 
   // Writes the <image> element for showing a large image within the
   // notification body.
-  void WriteLargeImageElement(const message_center::Notification& notification);
+  void WriteLargeImageElement(NotificationImageRetainer* image_retainer,
+                              const message_center::Notification& notification);
 
   // A helper for constructing image xml.
-  void WriteImageElement(const gfx::Image& image,
+  void WriteImageElement(NotificationImageRetainer* image_retainer,
+                         const gfx::Image& image,
                          const std::string& placement,
                          const std::string& hint_crop);
 
@@ -116,9 +117,11 @@ class NotificationTemplateBuilder {
 
   // Fills in the details for the actions (the buttons the notification
   // contains).
-  void AddActions(const message_center::Notification& notification,
+  void AddActions(NotificationImageRetainer* image_retainer,
+                  const message_center::Notification& notification,
                   const NotificationLaunchId& launch_id);
-  void WriteActionElement(const message_center::ButtonInfo& button,
+  void WriteActionElement(NotificationImageRetainer* image_retainer,
+                          const message_center::ButtonInfo& button,
                           int index,
                           NotificationLaunchId copied_launch_id);
 
@@ -140,9 +143,6 @@ class NotificationTemplateBuilder {
   // The XML writer to which the template will be written.
   std::unique_ptr<XmlWriter> xml_writer_;
 
-  // The image retainer.
-  base::WeakPtr<NotificationImageRetainer> image_retainer_;
-
   DISALLOW_COPY_AND_ASSIGN(NotificationTemplateBuilder);
 };
 

chrome/browser/notifications/win/notification_template_builder_unittest.cc

@@ -8,7 +8,6 @@
 #include <string>
 
 #include "base/macros.h"
-#include "base/memory/weak_ptr.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
@@ -83,10 +82,10 @@ class NotificationTemplateBuilderTest : public ::testing::Test {
   // must be wrapped in ASSERT_NO_FATAL_FAILURE().
   void VerifyXml(const message_center::Notification& notification,
                  const base::string16& xml_template) {
-    auto image_retainer = std::make_unique<MockNotificationImageRetainer>();
+    MockNotificationImageRetainer image_retainer;
     NotificationLaunchId launch_id(kEncodedId);
-    template_ = NotificationTemplateBuilder::Build(image_retainer->AsWeakPtr(),
-                                                   launch_id, notification);
+    template_ = NotificationTemplateBuilder::Build(&image_retainer, launch_id,
+                                                   notification);
 
     ASSERT_TRUE(template_);