Reland "Upgrade image file management for Win 10 native notification"

This is a reland of b011ab56

In the original change, NotificationImageRetainerTest.CleanupFilesFromPrevSessions
and NotificationImageRetainerTest.RegisterTemporaryImage failed on Win and Win64
trunk official.desktop.continuous builders.

The root cause is that for some reason, chrome::DIR_USER_DATA used in the test
alternated between the chrome-bot directory (i.e., C:/Users/chrome-bot/AppData...)
and the full directory (i.e., C:/Users/CHROME~1/AppData). From the test results,
they clearly referred to the same file, so the code is working fine in production.
However, the mismatch of the two strings failed the string comparison in the tests.

This CL attempted to fix it.

Original change's description:
> Upgrade image file management for Win 10 native notification
>
> This CL makes the following changes:
> 1) Change to run image folder cleanup after Chrome startup using a background
>    task, rather than running it at Chrome shutdown using a high priority task.
> 2) Retain the image folder to avoid deleting and creating it repeatedly.
> 3) Remove <subdir> from the full path for the temp file, which is unnecessary.
> 4) Delete the temp files in batch.
>
> These changes have the following benefits:
> 1) Fix the issue of Chrome shutdown being blocked by image file IO for some users
>    as in the current implementation.
> 2) Save some disk IO related to the sub-directories in the image folder and the
>    image folder itself.
> 3) Avoid creating a deletion task for each file, otherwise the overhead can be
>    large when there is a steady stream of notifications coming rapidly.
>
> Bug: 888276
> Change-Id: I214680aa00bd7f19e84e207e82ada553583a094b
> Reviewed-on: https://chromium-review.googlesource.com/c/1260498
> Commit-Queue: Xi Cheng <chengx@chromium.org>
> Reviewed-by: Ilya Sherman <isherman@chromium.org>
> Reviewed-by: Greg Thompson <grt@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#601227}

TBR=isherman@chromium.org, grt@chromium.org, finnur@chromium.org

Bug: 888276, 897352
Change-Id: I9b1bde98be8545f8634473e7540b0a2a3bee7172
Reviewed-on: https://chromium-review.googlesource.com/c/1292894Reviewed-by: Xi Cheng <chengx@chromium.org>
Commit-Queue: Xi Cheng <chengx@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601819}

chrome/browser/notifications/notification_platform_bridge_win.cc

@@ -78,6 +78,7 @@ typedef winfoundtn::ITypedEventHandler<
     winui::Notifications::ToastNotification*,
     winui::Notifications::ToastDismissedEventArgs*>
     ToastDismissedHandler;
+
 typedef winfoundtn::ITypedEventHandler<
     winui::Notifications::ToastNotification*,
     winui::Notifications::ToastFailedEventArgs*>
@@ -108,9 +109,9 @@ void ForwardNotificationOperationOnUiThread(
 
   g_browser_process->profile_manager()->LoadProfile(
       profile_id, incognito,
-      base::Bind(&NotificationDisplayServiceImpl::ProfileLoadedCallback,
-                 operation, notification_type, origin, notification_id,
-                 action_index, reply, by_user));
+      base::BindOnce(&NotificationDisplayServiceImpl::ProfileLoadedCallback,
+                     operation, notification_type, origin, notification_id,
+                     action_index, reply, by_user));
 }
 
 }  // namespace
@@ -137,18 +138,14 @@ class NotificationPlatformBridgeWinImpl
             base::win::ResolveCoreWinRTDelayload() &&
             ScopedHString::ResolveCoreWinRTStringDelayload()),
         notification_task_runner_(std::move(notification_task_runner)),
-        // file_deletion_task_runner_ runs tasks to delete notification image
-        // files on the disk. This task will be retried on each Chrome startup,
-        // so it's okay to specify TaskShutdownBehavior to be
-        // CONTINUE_ON_SHUTDOWN (i.e., ignore this task for all purposes at
-        // shutdown).
-        file_deletion_task_runner_(base::CreateSequencedTaskRunnerWithTraits(
-            {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
-             base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN})),
-        image_retainer_(std::make_unique<NotificationImageRetainer>(
-            file_deletion_task_runner_)) {
+        image_retainer_(std::make_unique<NotificationImageRetainer>()) {
+    // Delete any remaining temp files in the image folder from the previous
+    // sessions.
     DCHECK(notification_task_runner_);
-    DCHECK(file_deletion_task_runner_);
+    content::BrowserThread::PostAfterStartupTask(
+        FROM_HERE, notification_task_runner_,
+        base::BindOnce(&NotificationImageRetainer::CleanupFilesFromPrevSessions,
+                       image_retainer_->AsWeakPtr()));
   }
 
   // Obtain an IToastNotification interface from a given XML (provided by the
@@ -356,8 +353,8 @@ class NotificationPlatformBridgeWinImpl
                                    profile_id, incognito,
                                    notification->origin_url());
     std::unique_ptr<NotificationTemplateBuilder> notification_template =
-        NotificationTemplateBuilder::Build(
-            image_retainer_->AsWeakPtr(), launch_id, profile_id, *notification);
+        NotificationTemplateBuilder::Build(image_retainer_->AsWeakPtr(),
+                                           launch_id, *notification);
     mswr::ComPtr<winui::Notifications::IToastNotification> toast =
         GetToastNotification(*notification, *notification_template, profile_id,
                              incognito);
@@ -667,7 +664,9 @@ class NotificationPlatformBridgeWinImpl
   friend class MockIToastNotifier;
   friend class NotificationPlatformBridgeWin;
 
-  ~NotificationPlatformBridgeWinImpl() = default;
+  ~NotificationPlatformBridgeWinImpl() {
+    notification_task_runner_->DeleteSoon(FROM_HERE, image_retainer_.release());
+  }
 
   base::string16 GetAppId() const {
     return ShellUtil::GetBrowserModelId(InstallUtil::IsPerUserInstall());
@@ -758,9 +757,6 @@ class NotificationPlatformBridgeWinImpl
   // The task runner running notification related tasks.
   scoped_refptr<base::SequencedTaskRunner> notification_task_runner_;
 
-  // The task runner running tasks to delete files.
-  scoped_refptr<base::SequencedTaskRunner> file_deletion_task_runner_;
-
   // An object that keeps temp files alive long enough for Windows to pick up.
   std::unique_ptr<NotificationImageRetainer> image_retainer_;
 

chrome/browser/notifications/notification_platform_bridge_win_unittest.cc

@@ -28,6 +28,7 @@
 #include "chrome/browser/notifications/win/mock_notification_image_retainer.h"
 #include "chrome/browser/notifications/win/notification_launch_id.h"
 #include "chrome/browser/notifications/win/notification_template_builder.h"
+#include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/message_center/public/cpp/notification.h"
 #include "ui/message_center/public/cpp/notifier_id.h"
@@ -50,15 +51,21 @@ constexpr char kProfileId[] = "Default";
 class NotificationPlatformBridgeWinTest : public testing::Test {
  public:
   NotificationPlatformBridgeWinTest()
-      : notification_platform_bridge_win_(
-            std::make_unique<NotificationPlatformBridgeWin>()) {}
+      : scoped_task_environment_(
+            base::test::ScopedTaskEnvironment::MainThreadType::MOCK_TIME),
+        thread_bundle_(content::TestBrowserThreadBundle::PLAIN_MAINLOOP) {}
+
   ~NotificationPlatformBridgeWinTest() override = default;
 
+ protected:
   mswr::ComPtr<winui::Notifications::IToastNotification2> GetToast(
+      NotificationPlatformBridgeWin* bridge,
       const NotificationLaunchId& launch_id,
       bool renotify,
       const std::string& profile_id,
       bool incognito) {
+    DCHECK(bridge);
+
     GURL origin(kOrigin);
     auto notification = std::make_unique<message_center::Notification>(
         message_center::NOTIFICATION_TYPE_SIMPLE, kNotificationId, L"title",
@@ -68,12 +75,12 @@ class NotificationPlatformBridgeWinTest : public testing::Test {
     notification->set_renotify(renotify);
     auto image_retainer = std::make_unique<MockNotificationImageRetainer>();
     std::unique_ptr<NotificationTemplateBuilder> builder =
-        NotificationTemplateBuilder::Build(
-            image_retainer->AsWeakPtr(), launch_id, profile_id, *notification);
+        NotificationTemplateBuilder::Build(image_retainer->AsWeakPtr(),
+                                           launch_id, *notification);
 
     mswr::ComPtr<winui::Notifications::IToastNotification> toast =
-        notification_platform_bridge_win_->GetToastNotificationForTesting(
-            *notification, *builder, profile_id, incognito);
+        bridge->GetToastNotificationForTesting(*notification, *builder,
+                                               profile_id, incognito);
     if (!toast) {
       LOG(ERROR) << "GetToastNotificationForTesting failed";
       return nullptr;
@@ -89,11 +96,8 @@ class NotificationPlatformBridgeWinTest : public testing::Test {
     return toast2;
   }
 
- protected:
   base::test::ScopedTaskEnvironment scoped_task_environment_;
-
-  std::unique_ptr<NotificationPlatformBridgeWin>
-      notification_platform_bridge_win_;
+  content::TestBrowserThreadBundle thread_bundle_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(NotificationPlatformBridgeWinTest);
@@ -107,11 +111,13 @@ TEST_F(NotificationPlatformBridgeWinTest, GroupAndTag) {
 
   base::win::ScopedCOMInitializer com_initializer;
 
+  NotificationPlatformBridgeWin bridge;
+
   NotificationLaunchId launch_id(kLaunchId);
   ASSERT_TRUE(launch_id.is_valid());
 
   mswr::ComPtr<winui::Notifications::IToastNotification2> toast2 =
-      GetToast(launch_id, /*renotify=*/false, kProfileId,
+      GetToast(&bridge, launch_id, /*renotify=*/false, kProfileId,
                /*incognito=*/false);
   ASSERT_TRUE(toast2);
 
@@ -139,6 +145,8 @@ TEST_F(NotificationPlatformBridgeWinTest, GroupAndTagUniqueness) {
 
   base::win::ScopedCOMInitializer com_initializer;
 
+  NotificationPlatformBridgeWin bridge;
+
   NotificationLaunchId launch_id(kLaunchId);
   ASSERT_TRUE(launch_id.is_valid());
 
@@ -149,9 +157,9 @@ TEST_F(NotificationPlatformBridgeWinTest, GroupAndTagUniqueness) {
 
   // Different profiles, same incognito status -> Unique tags.
   {
-    toastA = GetToast(launch_id, /*renotify=*/false, "Profile1",
+    toastA = GetToast(&bridge, launch_id, /*renotify=*/false, "Profile1",
                       /*incognito=*/true);
-    toastB = GetToast(launch_id, /*renotify=*/false, "Profile2",
+    toastB = GetToast(&bridge, launch_id, /*renotify=*/false, "Profile2",
                       /*incognito=*/true);
 
     ASSERT_TRUE(toastA);
@@ -168,9 +176,9 @@ TEST_F(NotificationPlatformBridgeWinTest, GroupAndTagUniqueness) {
 
   // Same profile, different incognito status -> Unique tags.
   {
-    toastA = GetToast(launch_id, /*renotify=*/false, "Profile1",
+    toastA = GetToast(&bridge, launch_id, /*renotify=*/false, "Profile1",
                       /*incognito=*/true);
-    toastB = GetToast(launch_id, /*renotify=*/false, "Profile1",
+    toastB = GetToast(&bridge, launch_id, /*renotify=*/false, "Profile1",
                       /*incognito=*/false);
 
     ASSERT_TRUE(toastA);
@@ -187,9 +195,9 @@ TEST_F(NotificationPlatformBridgeWinTest, GroupAndTagUniqueness) {
 
   // Same profile, same incognito status -> Identical tags.
   {
-    toastA = GetToast(launch_id, /*renotify=*/false, "Profile1",
+    toastA = GetToast(&bridge, launch_id, /*renotify=*/false, "Profile1",
                       /*incognito=*/true);
-    toastB = GetToast(launch_id, /*renotify=*/false, "Profile1",
+    toastB = GetToast(&bridge, launch_id, /*renotify=*/false, "Profile1",
                       /*incognito=*/true);
 
     ASSERT_TRUE(toastA);
@@ -214,10 +222,11 @@ TEST_F(NotificationPlatformBridgeWinTest, Suppress) {
 
   base::win::ScopedCOMInitializer com_initializer;
 
+  NotificationPlatformBridgeWin bridge;
+
   std::vector<mswr::ComPtr<winui::Notifications::IToastNotification>>
       notifications;
-  notification_platform_bridge_win_->SetDisplayedNotificationsForTesting(
-      &notifications);
+  bridge.SetDisplayedNotificationsForTesting(&notifications);
 
   mswr::ComPtr<winui::Notifications::IToastNotification2> toast2;
   boolean suppress;
@@ -227,7 +236,7 @@ TEST_F(NotificationPlatformBridgeWinTest, Suppress) {
 
   // Make sure this works a toast is not suppressed when no notifications are
   // registered.
-  toast2 = GetToast(launch_id, /*renotify=*/false, kProfileId,
+  toast2 = GetToast(&bridge, launch_id, /*renotify=*/false, kProfileId,
                     /*incognito=*/false);
   ASSERT_TRUE(toast2);
   ASSERT_HRESULT_SUCCEEDED(toast2->get_SuppressPopup(&suppress));
@@ -243,7 +252,7 @@ TEST_F(NotificationPlatformBridgeWinTest, Suppress) {
       L"<toast launch=\"0|0|Default|0|https://foo.com/|id\"></toast>", tag));
 
   // Request this notification with renotify true (should not be suppressed).
-  toast2 = GetToast(launch_id, /*renotify=*/true, kProfileId,
+  toast2 = GetToast(&bridge, launch_id, /*renotify=*/true, kProfileId,
                     /*incognito=*/false);
   ASSERT_TRUE(toast2);
   ASSERT_HRESULT_SUCCEEDED(toast2->get_SuppressPopup(&suppress));
@@ -251,13 +260,12 @@ TEST_F(NotificationPlatformBridgeWinTest, Suppress) {
   toast2.Reset();
 
   // Request this notification with renotify false (should be suppressed).
-  toast2 = GetToast(launch_id, /*renotify=*/false, kProfileId,
+  toast2 = GetToast(&bridge, launch_id, /*renotify=*/false, kProfileId,
                     /*incognito=*/false);
   ASSERT_TRUE(toast2);
   ASSERT_HRESULT_SUCCEEDED(toast2->get_SuppressPopup(&suppress));
   ASSERT_TRUE(suppress);
   toast2.Reset();
 
-  notification_platform_bridge_win_->SetDisplayedNotificationsForTesting(
-      nullptr);
+  bridge.SetDisplayedNotificationsForTesting(nullptr);
 }

chrome/browser/notifications/win/mock_notification_image_retainer.cc

@@ -5,13 +5,14 @@
 #include "chrome/browser/notifications/win/mock_notification_image_retainer.h"
 
 #include "base/files/file_path.h"
+#include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
 #include "ui/gfx/image/image.h"
 
+void MockNotificationImageRetainer::CleanupFilesFromPrevSessions() {}
+
 base::FilePath MockNotificationImageRetainer::RegisterTemporaryImage(
-    const gfx::Image& image,
-    const std::string& profile_id,
-    const GURL& origin) {
+    const gfx::Image& image) {
   base::string16 file = base::string16(L"c:\\temp\\img") +
                         base::IntToString16(counter_++) +
                         base::string16(L".tmp");

chrome/browser/notifications/win/mock_notification_image_retainer.h

@@ -5,13 +5,9 @@
 #ifndef CHROME_BROWSER_NOTIFICATIONS_WIN_MOCK_NOTIFICATION_IMAGE_RETAINER_H_
 #define CHROME_BROWSER_NOTIFICATIONS_WIN_MOCK_NOTIFICATION_IMAGE_RETAINER_H_
 
-#include <string>
-
 #include "base/macros.h"
 #include "chrome/browser/notifications/win/notification_image_retainer.h"
 
-class GURL;
-
 namespace gfx {
 class Image;
 }
@@ -20,13 +16,12 @@ class Image;
 // predictable paths to callers wanting to register temporary files.
 class MockNotificationImageRetainer : public NotificationImageRetainer {
  public:
-  MockNotificationImageRetainer() : NotificationImageRetainer(nullptr) {}
+  MockNotificationImageRetainer() : NotificationImageRetainer() {}
   ~MockNotificationImageRetainer() override = default;
 
   // NotificationImageRetainer implementation:
-  base::FilePath RegisterTemporaryImage(const gfx::Image& image,
-                                        const std::string& profile_id,
-                                        const GURL& origin) override;
+  void CleanupFilesFromPrevSessions() override;
+  base::FilePath RegisterTemporaryImage(const gfx::Image& image) override;
 
  private:
   int counter_ = 0;

chrome/browser/notifications/win/notification_image_retainer.h

@@ -5,14 +5,19 @@
 #ifndef CHROME_BROWSER_NOTIFICATIONS_WIN_NOTIFICATION_IMAGE_RETAINER_H_
 #define CHROME_BROWSER_NOTIFICATIONS_WIN_NOTIFICATION_IMAGE_RETAINER_H_
 
-#include <string>
+#include <memory>
+#include <utility>
+#include <vector>
 
 #include "base/files/file_path.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
+#include "base/sequence_checker.h"
 #include "base/sequenced_task_runner.h"
-
-class GURL;
+#include "base/strings/string16.h"
+#include "base/time/tick_clock.h"
+#include "base/time/time.h"
+#include "base/timer/timer.h"
 
 namespace gfx {
 class Image;
@@ -26,44 +31,63 @@ class Image;
 //
 // Also, on Windows, temp file deletion is not guaranteed and, since the images
 // can potentially be large, this presents a problem because Chrome might then
-// be leaving chunks of dead bits lying around on user’s computers during
+// be leaving chunks of dead bits lying around on users' computers during
 // unclean shutdowns.
-class NotificationImageRetainer
-    : public base::SupportsWeakPtr<NotificationImageRetainer> {
+class NotificationImageRetainer {
  public:
-  explicit NotificationImageRetainer(
-      scoped_refptr<base::SequencedTaskRunner> task_runner);
+  NotificationImageRetainer(
+      scoped_refptr<base::SequencedTaskRunner> deletion_task_runner,
+      const base::TickClock* tick_clock);
+
+  NotificationImageRetainer();
   virtual ~NotificationImageRetainer();
 
-  // Stores an |image| from a particular profile (|profile_id|) and |origin| on
-  // disk in a temporary (short-lived) file. Returns the path to the file
-  // created, which will be valid for a few seconds only. It will be deleted
-  // either after a short timeout or after a restart of Chrome (the next time
-  // this function is called). The function returns an empty FilePath if file
-  // creation fails.
-  virtual base::FilePath RegisterTemporaryImage(const gfx::Image& image,
-                                                const std::string& profile_id,
-                                                const GURL& origin);
-
-  // Sets whether to override temp file destruction time. If set to |true|, the
-  // temp files will be scheduled for deletion right after their creation. If
-  // |false|, the standard deletion delay will apply.
-  static void OverrideTempFileLifespanForTesting(
-      bool override_file_destruction);
+  // Deletes all the remaining files in image_dir_ due to previous unclean
+  // shutdowns.
+  virtual void CleanupFilesFromPrevSessions();
+
+  // Stores an |image| on disk in a temporary (short-lived) file. Returns the
+  // path to the file created, which will be valid for a few seconds only. It
+  // will be deleted either after a short timeout or after a restart of Chrome
+  // (the next time this function is called). The function returns an empty
+  // FilePath if file creation fails.
+  virtual base::FilePath RegisterTemporaryImage(const gfx::Image& image);
+
+  base::WeakPtr<NotificationImageRetainer> AsWeakPtr();
+
+  const base::FilePath& image_dir() { return image_dir_; }
 
  private:
+  using NameAndTime = std::pair<base::string16, base::TimeTicks>;
+  using NamesAndTimes = std::vector<NameAndTime>;
+
+  // Deletes expired (older than a pre-defined threshold) files.
+  void DeleteExpiredFiles();
+
+  // A collection of names to registered image files in image_dir_, each of
+  // which must stay valid for a short time while the Notification Center
+  // processes them. Each file has a corresponding registration timestamp. Files
+  // in this collection that have outlived the required minimum lifespan are
+  // scheduled for deletion periodically by |deletion_timer_|. The items in this
+  // collection are sorted by increasing registration time.
+  NamesAndTimes registered_images_;
+
+  // The task runner used to handle file deletion.
+  scoped_refptr<base::SequencedTaskRunner> deletion_task_runner_;
+
   // The path to where to store the temporary files.
-  base::FilePath image_directory_;
+  const base::FilePath image_dir_;
+
+  // Not owned.
+  const base::TickClock* const tick_clock_;
 
-  // Whether this class has initialized.
-  bool initialized_ = false;
+  // A timer used to handle deleting files in batch.
+  base::RepeatingTimer deletion_timer_;
 
-  // Whether to override the time to wait before deleting the temp files. For
-  // testing use only.
-  static bool override_file_destruction_;
+  SEQUENCE_CHECKER(sequence_checker_);
 
-  // The task runner to use.
-  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+  // For callbacks may run after destruction.
+  base::WeakPtrFactory<NotificationImageRetainer> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(NotificationImageRetainer);
 };

chrome/browser/notifications/win/notification_image_retainer_unittest.cc

@@ -8,101 +8,161 @@
 #include "base/files/file_util.h"
 #include "base/macros.h"
 #include "base/run_loop.h"
-#include "base/strings/string_number_conversions.h"
-#include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_path_override.h"
 #include "base/test/scoped_task_environment.h"
+#include "chrome/common/chrome_paths.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/image/image.h"
-#include "url/gurl.h"
 
 namespace {
 
-const char kProfileId1[] = "Default";
-const char kProfileId2[] = "User";
+// This value has to stay in sync with that in notification_image_retainer.cc.
+constexpr base::TimeDelta kDeletionDelay = base::TimeDelta::FromSeconds(12);
 
 }  // namespace
 
 class NotificationImageRetainerTest : public ::testing::Test {
  public:
-  NotificationImageRetainerTest() = default;
-  ~NotificationImageRetainerTest() override = default;
-
-  void SetUp() override {
-    NotificationImageRetainer::OverrideTempFileLifespanForTesting(true);
-  }
+  NotificationImageRetainerTest()
+      : scoped_task_environment_(
+            base::test::ScopedTaskEnvironment::MainThreadType::MOCK_TIME),
+        user_data_dir_override_(chrome::DIR_USER_DATA) {}
 
-  void TearDown() override {
-    NotificationImageRetainer::OverrideTempFileLifespanForTesting(false);
-  }
+  ~NotificationImageRetainerTest() override = default;
 
  protected:
   base::test::ScopedTaskEnvironment scoped_task_environment_;
 
-  base::string16 CalculateHash(const std::string& profile_id,
-                               const GURL& origin) {
-    return base::UintToString16(base::Hash(profile_id + origin.spec()));
-  }
-
  private:
+  base::ScopedPathOverride user_data_dir_override_;
+
   DISALLOW_COPY_AND_ASSIGN(NotificationImageRetainerTest);
 };
 
-TEST_F(NotificationImageRetainerTest, FileCreation) {
+TEST_F(NotificationImageRetainerTest, RegisterTemporaryImage) {
   auto image_retainer = std::make_unique<NotificationImageRetainer>(
-      scoped_task_environment_.GetMainThreadTaskRunner());
+      scoped_task_environment_.GetMainThreadTaskRunner(),
+      scoped_task_environment_.GetMockTickClock());
 
   SkBitmap icon;
   icon.allocN32Pixels(64, 64);
   icon.eraseARGB(255, 100, 150, 200);
   gfx::Image image = gfx::Image::CreateFrom1xBitmap(icon);
 
-  GURL origin1("https://www.google.com");
-  GURL origin2("https://www.chromium.org");
-
-  // Expecting separate directories per profile and origin.
-  base::string16 dir_profile1_origin1 = CalculateHash(kProfileId1, origin1);
-  base::string16 dir_profile1_origin2 = CalculateHash(kProfileId1, origin2);
-  base::string16 dir_profile2_origin1 = CalculateHash(kProfileId2, origin1);
-  base::string16 dir_profile2_origin2 = CalculateHash(kProfileId2, origin2);
-
-  base::FilePath path1 =
-      image_retainer->RegisterTemporaryImage(image, kProfileId1, origin1);
-  ASSERT_TRUE(base::PathExists(path1));
-  ASSERT_TRUE(path1.value().find(dir_profile1_origin1) != std::string::npos)
-      << path1.value().c_str();
-
-  std::vector<base::FilePath::StringType> components;
-  path1.GetComponents(&components);
-  base::FilePath image_dir;
-  for (size_t i = 0; i < components.size() - 2; ++i)
-    image_dir = image_dir.Append(components[i]);
-
-  base::FilePath path2 =
-      image_retainer->RegisterTemporaryImage(image, kProfileId1, origin2);
-  ASSERT_TRUE(base::PathExists(path2));
-  ASSERT_TRUE(path2.value().find(dir_profile1_origin2) != std::string::npos)
-      << path2.value().c_str();
-
-  base::FilePath path3 =
-      image_retainer->RegisterTemporaryImage(image, kProfileId2, origin1);
-  ASSERT_TRUE(base::PathExists(path3));
-  ASSERT_TRUE(path3.value().find(dir_profile2_origin1) != std::string::npos)
-      << path3.value().c_str();
-
-  base::FilePath path4 =
-      image_retainer->RegisterTemporaryImage(image, kProfileId2, origin2);
-  ASSERT_TRUE(base::PathExists(path4));
-  ASSERT_TRUE(path4.value().find(dir_profile2_origin2) != std::string::npos)
-      << path4.value().c_str();
-
-  base::RunLoop().RunUntilIdle();
-
-  ASSERT_FALSE(base::PathExists(path1));
-  ASSERT_FALSE(base::PathExists(path2));
-  ASSERT_FALSE(base::PathExists(path3));
-  ASSERT_FALSE(base::PathExists(path4));
-
-  image_retainer.reset(nullptr);
-  ASSERT_FALSE(base::PathExists(image_dir));
+  base::FilePath temp_file = image_retainer->RegisterTemporaryImage(image);
+  ASSERT_FALSE(temp_file.empty());
+  ASSERT_TRUE(base::PathExists(temp_file));
+
+  // Fast-forward the task runner so that the file deletion task posted in
+  // RegisterTemporaryImage() finishes running.
+  scoped_task_environment_.FastForwardBy(kDeletionDelay);
+
+  // The temp file should be deleted now.
+  ASSERT_FALSE(base::PathExists(temp_file));
+
+  // The destruction of the image retainer object won't delete the image
+  // directory.
+  image_retainer.reset();
+  ASSERT_TRUE(base::PathExists(temp_file.DirName()));
+}
+
+TEST_F(NotificationImageRetainerTest, DeleteFilesInBatch) {
+  auto image_retainer = std::make_unique<NotificationImageRetainer>(
+      scoped_task_environment_.GetMainThreadTaskRunner(),
+      scoped_task_environment_.GetMockTickClock());
+
+  SkBitmap icon;
+  icon.allocN32Pixels(64, 64);
+  icon.eraseARGB(255, 100, 150, 200);
+  gfx::Image image = gfx::Image::CreateFrom1xBitmap(icon);
+
+  // Create 1st image file on disk.
+  base::FilePath temp_file1 = image_retainer->RegisterTemporaryImage(image);
+  ASSERT_FALSE(temp_file1.empty());
+  ASSERT_TRUE(base::PathExists(temp_file1));
+
+  // Simulate ticking of the clock so that the next image file has a different
+  // registration time.
+  scoped_task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(1));
+
+  // Create 2nd image file on disk.
+  base::FilePath temp_file2 = image_retainer->RegisterTemporaryImage(image);
+  ASSERT_FALSE(temp_file2.empty());
+  ASSERT_TRUE(base::PathExists(temp_file2));
+
+  // Simulate ticking of the clock so that the next image file has a different
+  // registration time.
+  scoped_task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(1));
+
+  // Create 3rd image file on disk.
+  base::FilePath temp_file3 = image_retainer->RegisterTemporaryImage(image);
+  ASSERT_FALSE(temp_file3.empty());
+  ASSERT_TRUE(base::PathExists(temp_file3));
+
+  // Fast-forward the task runner by kDeletionDelay. The first temp file should
+  // be deleted now, while the other two should still be around.
+  scoped_task_environment_.FastForwardBy(kDeletionDelay);
+  ASSERT_FALSE(base::PathExists(temp_file1));
+  ASSERT_TRUE(base::PathExists(temp_file2));
+  ASSERT_TRUE(base::PathExists(temp_file3));
+
+  // Fast-forward the task runner again. The second and the third temp files
+  // are deleted simultaneously.
+  scoped_task_environment_.FastForwardBy(kDeletionDelay);
+  ASSERT_FALSE(base::PathExists(temp_file2));
+  ASSERT_FALSE(base::PathExists(temp_file3));
+}
+
+TEST_F(NotificationImageRetainerTest, CleanupFilesFromPrevSessions) {
+  auto image_retainer = std::make_unique<NotificationImageRetainer>(
+      scoped_task_environment_.GetMainThreadTaskRunner(),
+      scoped_task_environment_.GetMockTickClock());
+
+  const base::FilePath& image_dir = image_retainer->image_dir();
+  ASSERT_TRUE(base::CreateDirectory(image_dir));
+
+  // Create two temp files as if they were created in previous sessions.
+  base::FilePath temp_file1;
+  ASSERT_TRUE(base::CreateTemporaryFileInDir(image_dir, &temp_file1));
+
+  base::FilePath temp_file2;
+  ASSERT_TRUE(base::CreateTemporaryFileInDir(image_dir, &temp_file2));
+
+  ASSERT_TRUE(base::PathExists(temp_file1));
+  ASSERT_TRUE(base::PathExists(temp_file2));
+
+  // Create a new temp file in the current session. This file will be scheduled
+  // to delete after kDeletionDelay seconds.
+  SkBitmap icon;
+  icon.allocN32Pixels(64, 64);
+  icon.eraseARGB(255, 100, 150, 200);
+  gfx::Image image = gfx::Image::CreateFrom1xBitmap(icon);
+
+  base::FilePath temp_file3 = image_retainer->RegisterTemporaryImage(image);
+  ASSERT_FALSE(temp_file3.empty());
+  ASSERT_TRUE(base::PathExists(temp_file3));
+
+  // Schedule a file cleanup task.
+  image_retainer->CleanupFilesFromPrevSessions();
+
+  // Now the file cleanup task finishes running.
+  scoped_task_environment_.RunUntilIdle();
+
+  // The two temp files from previous sessions should be deleted now.
+  ASSERT_FALSE(base::PathExists(temp_file1));
+  ASSERT_FALSE(base::PathExists(temp_file2));
+
+  // The temp file created in this session should still be around.
+  ASSERT_TRUE(base::PathExists(temp_file3));
+
+  // Fast-forward the task runner so that the file deletion task posted in
+  // RegisterTemporaryImage() finishes running.
+  scoped_task_environment_.FastForwardBy(kDeletionDelay);
+
+  // The temp file created in this session should be deleted now.
+  ASSERT_FALSE(base::PathExists(temp_file3));
+
+  // The image directory should still be around.
+  ASSERT_TRUE(base::PathExists(image_dir));
 }

chrome/browser/notifications/win/notification_template_builder.cc

@@ -84,10 +84,9 @@ const char* NotificationTemplateBuilder::context_menu_label_override_ = nullptr;
 std::unique_ptr<NotificationTemplateBuilder> NotificationTemplateBuilder::Build(
     base::WeakPtr<NotificationImageRetainer> notification_image_retainer,
     const NotificationLaunchId& launch_id,
-    const std::string& profile_id,
     const message_center::Notification& notification) {
   std::unique_ptr<NotificationTemplateBuilder> builder = base::WrapUnique(
-      new NotificationTemplateBuilder(notification_image_retainer, profile_id));
+      new NotificationTemplateBuilder(notification_image_retainer));
 
   builder->StartToastElement(launch_id, notification);
   builder->StartVisualElement();
@@ -144,11 +143,9 @@ std::unique_ptr<NotificationTemplateBuilder> NotificationTemplateBuilder::Build(
 }
 
 NotificationTemplateBuilder::NotificationTemplateBuilder(
-    base::WeakPtr<NotificationImageRetainer> notification_image_retainer,
-    const std::string& profile_id)
+    base::WeakPtr<NotificationImageRetainer> notification_image_retainer)
     : xml_writer_(std::make_unique<XmlWriter>()),
-      image_retainer_(notification_image_retainer),
-      profile_id_(profile_id) {
+      image_retainer_(notification_image_retainer) {
   xml_writer_->StartWriting();
 }
 
@@ -251,26 +248,23 @@ void NotificationTemplateBuilder::WriteItems(
 
 void NotificationTemplateBuilder::WriteIconElement(
     const message_center::Notification& notification) {
-  WriteImageElement(notification.icon(), notification.origin_url(),
-                    kPlacementAppLogoOverride, kHintCropNone);
+  WriteImageElement(notification.icon(), kPlacementAppLogoOverride,
+                    kHintCropNone);
 }
 
 void NotificationTemplateBuilder::WriteLargeImageElement(
     const message_center::Notification& notification) {
-  WriteImageElement(notification.image(), notification.origin_url(), kHero,
-                    std::string());
+  WriteImageElement(notification.image(), kHero, std::string());
 }
 
 void NotificationTemplateBuilder::WriteImageElement(
     const gfx::Image& image,
-    const GURL& origin,
     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, profile_id_, origin);
+  base::FilePath path = image_retainer_->RegisterTemporaryImage(image);
   if (!path.empty()) {
     xml_writer_->StartElement(kImageElement);
     xml_writer_->AddAttribute(kPlacement, placement);
@@ -319,7 +313,7 @@ void NotificationTemplateBuilder::AddActions(
   }
 
   for (size_t i = 0; i < buttons.size(); ++i)
-    WriteActionElement(buttons[i], i, notification.origin_url(), launch_id);
+    WriteActionElement(buttons[i], i, launch_id);
 }
 
 void NotificationTemplateBuilder::AddContextMenu(
@@ -351,7 +345,6 @@ void NotificationTemplateBuilder::WriteAudioSilentElement() {
 void NotificationTemplateBuilder::WriteActionElement(
     const message_center::ButtonInfo& button,
     int index,
-    const GURL& origin,
     NotificationLaunchId copied_launch_id) {
   xml_writer_->StartElement(kActionElement);
   xml_writer_->AddAttribute(kActivationType, kForeground);
@@ -363,8 +356,7 @@ void NotificationTemplateBuilder::WriteActionElement(
     // Although image_retainer_ is a WeakPtr, it should never be nullptr here.
     DCHECK(image_retainer_);
 
-    base::FilePath path = image_retainer_->RegisterTemporaryImage(
-        button.icon, profile_id_, origin);
+    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

@@ -49,7 +49,6 @@ class NotificationTemplateBuilder {
   static std::unique_ptr<NotificationTemplateBuilder> Build(
       base::WeakPtr<NotificationImageRetainer> notification_image_retainer,
       const NotificationLaunchId& launch_id,
-      const std::string& profile_id,
       const message_center::Notification& notification);
 
   // Set label for the context menu item in testing. The caller owns |label| and
@@ -66,8 +65,7 @@ class NotificationTemplateBuilder {
   enum class TextType { NORMAL, ATTRIBUTION };
 
   NotificationTemplateBuilder(
-      base::WeakPtr<NotificationImageRetainer> notification_image_retainer,
-      const std::string& profile_id);
+      base::WeakPtr<NotificationImageRetainer> notification_image_retainer);
 
   // Formats the |origin| for display in the notification template.
   std::string FormatOrigin(const GURL& origin) const;
@@ -103,7 +101,6 @@ class NotificationTemplateBuilder {
 
   // A helper for constructing image xml.
   void WriteImageElement(const gfx::Image& image,
-                         const GURL& origin,
                          const std::string& placement,
                          const std::string& hint_crop);
 
@@ -123,7 +120,6 @@ class NotificationTemplateBuilder {
                   const NotificationLaunchId& launch_id);
   void WriteActionElement(const message_center::ButtonInfo& button,
                           int index,
-                          const GURL& origin,
                           NotificationLaunchId copied_launch_id);
 
   // Adds context menu actions to the notification sent by |origin|.
@@ -147,9 +143,6 @@ class NotificationTemplateBuilder {
   // The image retainer.
   base::WeakPtr<NotificationImageRetainer> image_retainer_;
 
-  // The id of the profile the notification is intended for.
-  std::string profile_id_;
-
   DISALLOW_COPY_AND_ASSIGN(NotificationTemplateBuilder);
 };
 

chrome/browser/notifications/win/notification_template_builder_unittest.cc

@@ -13,6 +13,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_task_environment.h"
 #include "chrome/browser/notifications/win/mock_notification_image_retainer.h"
 #include "chrome/browser/notifications/win/notification_launch_id.h"
 #include "chrome/grit/chromium_strings.h"
@@ -32,7 +33,6 @@ const char kNotificationId[] = "notification_id";
 const char kNotificationTitle[] = "My Title";
 const char kNotificationMessage[] = "My Message";
 const char kNotificationOrigin[] = "https://example.com";
-const char kProfileId[] = "Default";
 
 bool FixedTime(base::Time* time) {
   base::Time::Exploded exploded = {0};
@@ -85,17 +85,18 @@ class NotificationTemplateBuilderTest : public ::testing::Test {
                  const base::string16& xml_template) {
     auto image_retainer = std::make_unique<MockNotificationImageRetainer>();
     NotificationLaunchId launch_id(kEncodedId);
-    template_ = NotificationTemplateBuilder::Build(
-        image_retainer->AsWeakPtr(), launch_id, kProfileId, notification);
+    template_ = NotificationTemplateBuilder::Build(image_retainer->AsWeakPtr(),
+                                                   launch_id, notification);
 
     ASSERT_TRUE(template_);
 
     EXPECT_EQ(template_->GetNotificationTemplate(), xml_template);
   }
 
- protected:
   std::unique_ptr<NotificationTemplateBuilder> template_;
 
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(NotificationTemplateBuilderTest);
 };

tools/metrics/histograms/histograms.xml

@@ -66485,6 +66485,9 @@ uploading your change for review.
 </histogram>
 
 <histogram name="Notifications.Windows.ImageRetainerDestructionTime" units="ms">
+  <obsolete>
+    Obsolete 10/2018 as we no long record this metric.
+  </obsolete>
   <owner>chengx@chromium.org</owner>
   <owner>finnur@chromium.org</owner>
   <owner>peter@chromium.org</owner>
@@ -66496,6 +66499,9 @@ uploading your change for review.
 
 <histogram name="Notifications.Windows.ImageRetainerInitializationTime"
     units="ms">
+  <obsolete>
+    Obsolete 10/2018 as we no long record this metric.
+  </obsolete>
   <owner>chengx@chromium.org</owner>
   <owner>finnur@chromium.org</owner>
   <owner>peter@chromium.org</owner>