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

This reverts commit b011ab56.

Reason for revert: caused test failures -- see crbug/897352

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,finnur@chromium.org,grt@chromium.org,chengx@chromium.org

Change-Id: Ie79e62373c0076346499e450e48c2d16b55c6dc4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 888276, 897352
Reviewed-on: https://chromium-review.googlesource.com/c/1292834Reviewed-by: Xi Cheng <chengx@chromium.org>
Commit-Queue: Xi Cheng <chengx@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601376}

chrome/browser/notifications/notification_platform_bridge_win.cc

@@ -78,7 +78,6 @@ typedef winfoundtn::ITypedEventHandler<
     winui::Notifications::ToastNotification*,
     winui::Notifications::ToastDismissedEventArgs*>
     ToastDismissedHandler;
-
 typedef winfoundtn::ITypedEventHandler<
     winui::Notifications::ToastNotification*,
     winui::Notifications::ToastFailedEventArgs*>
@@ -109,9 +108,9 @@ void ForwardNotificationOperationOnUiThread(
 
   g_browser_process->profile_manager()->LoadProfile(
       profile_id, incognito,
-      base::BindOnce(&NotificationDisplayServiceImpl::ProfileLoadedCallback,
-                     operation, notification_type, origin, notification_id,
-                     action_index, reply, by_user));
+      base::Bind(&NotificationDisplayServiceImpl::ProfileLoadedCallback,
+                 operation, notification_type, origin, notification_id,
+                 action_index, reply, by_user));
 }
 
 }  // namespace
@@ -138,14 +137,18 @@ class NotificationPlatformBridgeWinImpl
             base::win::ResolveCoreWinRTDelayload() &&
             ScopedHString::ResolveCoreWinRTStringDelayload()),
         notification_task_runner_(std::move(notification_task_runner)),
-        image_retainer_(std::make_unique<NotificationImageRetainer>()) {
-    // Delete any remaining temp files in the image folder from the previous
-    // sessions.
+        // 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_)) {
     DCHECK(notification_task_runner_);
-    content::BrowserThread::PostAfterStartupTask(
-        FROM_HERE, notification_task_runner_,
-        base::BindOnce(&NotificationImageRetainer::CleanupFilesFromPrevSessions,
-                       image_retainer_->AsWeakPtr()));
+    DCHECK(file_deletion_task_runner_);
   }
 
   // Obtain an IToastNotification interface from a given XML (provided by the
@@ -353,8 +356,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_->AsWeakPtr(), launch_id, profile_id, *notification);
     mswr::ComPtr<winui::Notifications::IToastNotification> toast =
         GetToastNotification(*notification, *notification_template, profile_id,
                              incognito);
@@ -664,9 +667,7 @@ class NotificationPlatformBridgeWinImpl
   friend class MockIToastNotifier;
   friend class NotificationPlatformBridgeWin;
 
-  ~NotificationPlatformBridgeWinImpl() {
-    notification_task_runner_->DeleteSoon(FROM_HERE, image_retainer_.release());
-  }
+  ~NotificationPlatformBridgeWinImpl() = default;
 
   base::string16 GetAppId() const {
     return ShellUtil::GetBrowserModelId(InstallUtil::IsPerUserInstall());
@@ -757,6 +758,9 @@ 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,7 +28,6 @@
 #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"
@@ -51,21 +50,15 @@ constexpr char kProfileId[] = "Default";
 class NotificationPlatformBridgeWinTest : public testing::Test {
  public:
   NotificationPlatformBridgeWinTest()
-      : scoped_task_environment_(
-            base::test::ScopedTaskEnvironment::MainThreadType::MOCK_TIME),
-        thread_bundle_(content::TestBrowserThreadBundle::PLAIN_MAINLOOP) {}
-
+      : notification_platform_bridge_win_(
+            std::make_unique<NotificationPlatformBridgeWin>()) {}
   ~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",
@@ -75,12 +68,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, *notification);
+        NotificationTemplateBuilder::Build(
+            image_retainer->AsWeakPtr(), launch_id, profile_id, *notification);
 
     mswr::ComPtr<winui::Notifications::IToastNotification> toast =
-        bridge->GetToastNotificationForTesting(*notification, *builder,
-                                               profile_id, incognito);
+        notification_platform_bridge_win_->GetToastNotificationForTesting(
+            *notification, *builder, profile_id, incognito);
     if (!toast) {
       LOG(ERROR) << "GetToastNotificationForTesting failed";
       return nullptr;
@@ -96,8 +89,11 @@ class NotificationPlatformBridgeWinTest : public testing::Test {
     return toast2;
   }
 
+ protected:
   base::test::ScopedTaskEnvironment scoped_task_environment_;
-  content::TestBrowserThreadBundle thread_bundle_;
+
+  std::unique_ptr<NotificationPlatformBridgeWin>
+      notification_platform_bridge_win_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(NotificationPlatformBridgeWinTest);
@@ -111,13 +107,11 @@ 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(&bridge, launch_id, /*renotify=*/false, kProfileId,
+      GetToast(launch_id, /*renotify=*/false, kProfileId,
                /*incognito=*/false);
   ASSERT_TRUE(toast2);
 
@@ -145,8 +139,6 @@ TEST_F(NotificationPlatformBridgeWinTest, GroupAndTagUniqueness) {
 
   base::win::ScopedCOMInitializer com_initializer;
 
-  NotificationPlatformBridgeWin bridge;
-
   NotificationLaunchId launch_id(kLaunchId);
   ASSERT_TRUE(launch_id.is_valid());
 
@@ -157,9 +149,9 @@ TEST_F(NotificationPlatformBridgeWinTest, GroupAndTagUniqueness) {
 
   // Different profiles, same incognito status -> Unique tags.
   {
-    toastA = GetToast(&bridge, launch_id, /*renotify=*/false, "Profile1",
+    toastA = GetToast(launch_id, /*renotify=*/false, "Profile1",
                       /*incognito=*/true);
-    toastB = GetToast(&bridge, launch_id, /*renotify=*/false, "Profile2",
+    toastB = GetToast(launch_id, /*renotify=*/false, "Profile2",
                       /*incognito=*/true);
 
     ASSERT_TRUE(toastA);
@@ -176,9 +168,9 @@ TEST_F(NotificationPlatformBridgeWinTest, GroupAndTagUniqueness) {
 
   // Same profile, different incognito status -> Unique tags.
   {
-    toastA = GetToast(&bridge, launch_id, /*renotify=*/false, "Profile1",
+    toastA = GetToast(launch_id, /*renotify=*/false, "Profile1",
                       /*incognito=*/true);
-    toastB = GetToast(&bridge, launch_id, /*renotify=*/false, "Profile1",
+    toastB = GetToast(launch_id, /*renotify=*/false, "Profile1",
                       /*incognito=*/false);
 
     ASSERT_TRUE(toastA);
@@ -195,9 +187,9 @@ TEST_F(NotificationPlatformBridgeWinTest, GroupAndTagUniqueness) {
 
   // Same profile, same incognito status -> Identical tags.
   {
-    toastA = GetToast(&bridge, launch_id, /*renotify=*/false, "Profile1",
+    toastA = GetToast(launch_id, /*renotify=*/false, "Profile1",
                       /*incognito=*/true);
-    toastB = GetToast(&bridge, launch_id, /*renotify=*/false, "Profile1",
+    toastB = GetToast(launch_id, /*renotify=*/false, "Profile1",
                       /*incognito=*/true);
 
     ASSERT_TRUE(toastA);
@@ -222,11 +214,10 @@ TEST_F(NotificationPlatformBridgeWinTest, Suppress) {
 
   base::win::ScopedCOMInitializer com_initializer;
 
-  NotificationPlatformBridgeWin bridge;
-
   std::vector<mswr::ComPtr<winui::Notifications::IToastNotification>>
       notifications;
-  bridge.SetDisplayedNotificationsForTesting(&notifications);
+  notification_platform_bridge_win_->SetDisplayedNotificationsForTesting(
+      &notifications);
 
   mswr::ComPtr<winui::Notifications::IToastNotification2> toast2;
   boolean suppress;
@@ -236,7 +227,7 @@ TEST_F(NotificationPlatformBridgeWinTest, Suppress) {
 
   // Make sure this works a toast is not suppressed when no notifications are
   // registered.
-  toast2 = GetToast(&bridge, launch_id, /*renotify=*/false, kProfileId,
+  toast2 = GetToast(launch_id, /*renotify=*/false, kProfileId,
                     /*incognito=*/false);
   ASSERT_TRUE(toast2);
   ASSERT_HRESULT_SUCCEEDED(toast2->get_SuppressPopup(&suppress));
@@ -252,7 +243,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(&bridge, launch_id, /*renotify=*/true, kProfileId,
+  toast2 = GetToast(launch_id, /*renotify=*/true, kProfileId,
                     /*incognito=*/false);
   ASSERT_TRUE(toast2);
   ASSERT_HRESULT_SUCCEEDED(toast2->get_SuppressPopup(&suppress));
@@ -260,12 +251,13 @@ TEST_F(NotificationPlatformBridgeWinTest, Suppress) {
   toast2.Reset();
 
   // Request this notification with renotify false (should be suppressed).
-  toast2 = GetToast(&bridge, launch_id, /*renotify=*/false, kProfileId,
+  toast2 = GetToast(launch_id, /*renotify=*/false, kProfileId,
                     /*incognito=*/false);
   ASSERT_TRUE(toast2);
   ASSERT_HRESULT_SUCCEEDED(toast2->get_SuppressPopup(&suppress));
   ASSERT_TRUE(suppress);
   toast2.Reset();
 
-  bridge.SetDisplayedNotificationsForTesting(nullptr);
+  notification_platform_bridge_win_->SetDisplayedNotificationsForTesting(
+      nullptr);
 }

chrome/browser/notifications/win/mock_notification_image_retainer.cc

@@ -5,14 +5,13 @@
 #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 gfx::Image& image,
+    const std::string& profile_id,
+    const GURL& origin) {
   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,9 +5,13 @@
 #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;
 }
@@ -16,12 +20,13 @@ class Image;
 // predictable paths to callers wanting to register temporary files.
 class MockNotificationImageRetainer : public NotificationImageRetainer {
  public:
-  MockNotificationImageRetainer() : NotificationImageRetainer() {}
+  MockNotificationImageRetainer() : NotificationImageRetainer(nullptr) {}
   ~MockNotificationImageRetainer() override = default;
 
   // NotificationImageRetainer implementation:
-  void CleanupFilesFromPrevSessions() override;
-  base::FilePath RegisterTemporaryImage(const gfx::Image& image) override;
+  base::FilePath RegisterTemporaryImage(const gfx::Image& image,
+                                        const std::string& profile_id,
+                                        const GURL& origin) override;
 
  private:
   int counter_ = 0;

chrome/browser/notifications/win/notification_image_retainer.h

@@ -5,18 +5,14 @@
 #ifndef CHROME_BROWSER_NOTIFICATIONS_WIN_NOTIFICATION_IMAGE_RETAINER_H_
 #define CHROME_BROWSER_NOTIFICATIONS_WIN_NOTIFICATION_IMAGE_RETAINER_H_
 
-#include <memory>
-#include <utility>
-#include <vector>
+#include <string>
 
 #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"
-#include "base/time/tick_clock.h"
-#include "base/time/time.h"
-#include "base/timer/timer.h"
+
+class GURL;
 
 namespace gfx {
 class Image;
@@ -30,63 +26,44 @@ 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 users' computers during
+// be leaving chunks of dead bits lying around on user’s computers during
 // unclean shutdowns.
-class NotificationImageRetainer {
+class NotificationImageRetainer
+    : public base::SupportsWeakPtr<NotificationImageRetainer> {
  public:
-  NotificationImageRetainer(
-      scoped_refptr<base::SequencedTaskRunner> deletion_task_runner,
-      const base::TickClock* tick_clock);
-
-  NotificationImageRetainer();
+  explicit NotificationImageRetainer(
+      scoped_refptr<base::SequencedTaskRunner> task_runner);
   virtual ~NotificationImageRetainer();
 
-  // 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_; }
+  // 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);
 
  private:
-  using PathAndTime = std::pair<base::FilePath, base::TimeTicks>;
-  using PathsAndTimes = std::vector<PathAndTime>;
-
-  // Deletes expired (older than a pre-defined threshold) files.
-  void DeleteExpiredFiles();
-
-  // A collection of paths to registered image files, each of which must stay
-  // valid for a short time while the Notification Center processes them. Each
-  // path 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.
-  PathsAndTimes 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.
-  const base::FilePath image_dir_;
-
-  // Not owned.
-  const base::TickClock* const tick_clock_;
+  base::FilePath image_directory_;
 
-  // A timer used to handle deleting files in batch.
-  base::RepeatingTimer deletion_timer_;
+  // Whether this class has initialized.
+  bool initialized_ = false;
 
-  SEQUENCE_CHECKER(sequence_checker_);
+  // Whether to override the time to wait before deleting the temp files. For
+  // testing use only.
+  static bool override_file_destruction_;
 
-  // For callbacks may run after destruction.
-  base::WeakPtrFactory<NotificationImageRetainer> weak_ptr_factory_;
+  // The task runner to use.
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
 
   DISALLOW_COPY_AND_ASSIGN(NotificationImageRetainer);
 };

chrome/browser/notifications/win/notification_image_retainer_unittest.cc

@@ -8,158 +8,101 @@
 #include "base/files/file_util.h"
 #include "base/macros.h"
 #include "base/run_loop.h"
-#include "base/test/scoped_path_override.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/utf_string_conversions.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 {
 
-// This value has to stay in sync with that in notification_image_retainer.cc.
-constexpr base::TimeDelta kDeletionDelay = base::TimeDelta::FromSeconds(12);
+const char kProfileId1[] = "Default";
+const char kProfileId2[] = "User";
 
 }  // namespace
 
 class NotificationImageRetainerTest : public ::testing::Test {
  public:
-  NotificationImageRetainerTest()
-      : scoped_task_environment_(
-            base::test::ScopedTaskEnvironment::MainThreadType::MOCK_TIME),
-        user_data_dir_override_(chrome::DIR_USER_DATA) {}
-
+  NotificationImageRetainerTest() = default;
   ~NotificationImageRetainerTest() override = default;
 
+  void SetUp() override {
+    NotificationImageRetainer::OverrideTempFileLifespanForTesting(true);
+  }
+
+  void TearDown() override {
+    NotificationImageRetainer::OverrideTempFileLifespanForTesting(false);
+  }
+
  protected:
   base::test::ScopedTaskEnvironment scoped_task_environment_;
 
- private:
-  base::ScopedPathOverride user_data_dir_override_;
+  base::string16 CalculateHash(const std::string& profile_id,
+                               const GURL& origin) {
+    return base::UintToString16(base::Hash(profile_id + origin.spec()));
+  }
 
+ private:
   DISALLOW_COPY_AND_ASSIGN(NotificationImageRetainerTest);
 };
 
-TEST_F(NotificationImageRetainerTest, RegisterTemporaryImage) {
-  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);
-
-  base::FilePath temp_file = image_retainer->RegisterTemporaryImage(image);
-  ASSERT_TRUE(base::PathExists(temp_file));
-
-  EXPECT_EQ(temp_file.DirName(), image_retainer->image_dir());
-
-  // 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_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_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_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) {
+TEST_F(NotificationImageRetainerTest, FileCreation) {
   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));
+      scoped_task_environment_.GetMainThreadTaskRunner());
 
-  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_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));
+  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));
 }

chrome/browser/notifications/win/notification_template_builder.cc

@@ -84,9 +84,10 @@ 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));
+      new NotificationTemplateBuilder(notification_image_retainer, profile_id));
 
   builder->StartToastElement(launch_id, notification);
   builder->StartVisualElement();
@@ -143,9 +144,11 @@ std::unique_ptr<NotificationTemplateBuilder> NotificationTemplateBuilder::Build(
 }
 
 NotificationTemplateBuilder::NotificationTemplateBuilder(
-    base::WeakPtr<NotificationImageRetainer> notification_image_retainer)
+    base::WeakPtr<NotificationImageRetainer> notification_image_retainer,
+    const std::string& profile_id)
     : xml_writer_(std::make_unique<XmlWriter>()),
-      image_retainer_(notification_image_retainer) {
+      image_retainer_(notification_image_retainer),
+      profile_id_(profile_id) {
   xml_writer_->StartWriting();
 }
 
@@ -248,23 +251,26 @@ void NotificationTemplateBuilder::WriteItems(
 
 void NotificationTemplateBuilder::WriteIconElement(
     const message_center::Notification& notification) {
-  WriteImageElement(notification.icon(), kPlacementAppLogoOverride,
-                    kHintCropNone);
+  WriteImageElement(notification.icon(), notification.origin_url(),
+                    kPlacementAppLogoOverride, kHintCropNone);
 }
 
 void NotificationTemplateBuilder::WriteLargeImageElement(
     const message_center::Notification& notification) {
-  WriteImageElement(notification.image(), kHero, std::string());
+  WriteImageElement(notification.image(), notification.origin_url(), 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);
+  base::FilePath path =
+      image_retainer_->RegisterTemporaryImage(image, profile_id_, origin);
   if (!path.empty()) {
     xml_writer_->StartElement(kImageElement);
     xml_writer_->AddAttribute(kPlacement, placement);
@@ -313,7 +319,7 @@ void NotificationTemplateBuilder::AddActions(
   }
 
   for (size_t i = 0; i < buttons.size(); ++i)
-    WriteActionElement(buttons[i], i, launch_id);
+    WriteActionElement(buttons[i], i, notification.origin_url(), launch_id);
 }
 
 void NotificationTemplateBuilder::AddContextMenu(
@@ -345,6 +351,7 @@ 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);
@@ -356,7 +363,8 @@ 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);
+    base::FilePath path = image_retainer_->RegisterTemporaryImage(
+        button.icon, profile_id_, origin);
     if (!path.empty())
       xml_writer_->AddAttribute(kImageUri, path.AsUTF8Unsafe());
   }

chrome/browser/notifications/win/notification_template_builder.h

@@ -49,6 +49,7 @@ 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
@@ -65,7 +66,8 @@ class NotificationTemplateBuilder {
   enum class TextType { NORMAL, ATTRIBUTION };
 
   NotificationTemplateBuilder(
-      base::WeakPtr<NotificationImageRetainer> notification_image_retainer);
+      base::WeakPtr<NotificationImageRetainer> notification_image_retainer,
+      const std::string& profile_id);
 
   // Formats the |origin| for display in the notification template.
   std::string FormatOrigin(const GURL& origin) const;
@@ -101,6 +103,7 @@ 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);
 
@@ -120,6 +123,7 @@ 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|.
@@ -143,6 +147,9 @@ 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,7 +13,6 @@
 #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"
@@ -33,6 +32,7 @@ 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,18 +85,17 @@ 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, notification);
+    template_ = NotificationTemplateBuilder::Build(
+        image_retainer->AsWeakPtr(), launch_id, kProfileId, 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

@@ -66387,9 +66387,6 @@ 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>
@@ -66401,9 +66398,6 @@ 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>