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}

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,7 +109,7 @@ void ForwardNotificationOperationOnUiThread(
 
   g_browser_process->profile_manager()->LoadProfile(
       profile_id, incognito,
-      base::Bind(&NotificationDisplayServiceImpl::ProfileLoadedCallback,
+      base::BindOnce(&NotificationDisplayServiceImpl::ProfileLoadedCallback,
                      operation, notification_type, origin, notification_id,
                      action_index, reply, by_user));
 }
@@ -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.cc

@@ -4,17 +4,20 @@
 
 #include "chrome/browser/notifications/win/notification_image_retainer.h"
 
+#include <algorithm>
+#include <set>
+
+#include "base/bind.h"
+#include "base/files/file_enumerator.h"
 #include "base/files/file_util.h"
-#include "base/hash.h"
 #include "base/memory/ref_counted_memory.h"
-#include "base/metrics/histogram_macros.h"
+#include "base/numerics/safe_conversions.h"
 #include "base/path_service.h"
-#include "base/strings/string_number_conversions.h"
+#include "base/stl_util.h"
 #include "base/task/post_task.h"
-#include "base/time/time.h"
+#include "base/time/default_tick_clock.h"
 #include "chrome/common/chrome_paths.h"
 #include "ui/gfx/image/image.h"
-#include "url/gurl.h"
 
 namespace {
 
@@ -30,8 +33,7 @@ constexpr base::TimeDelta kDeletionDelay = base::TimeDelta::FromSeconds(12);
 // Returns the temporary directory within the user data directory. The regular
 // temporary directory is not used to minimize the risk of files getting deleted
 // by accident. It is also not profile-bound because the notification bridge
-// handles images for multiple profiles and the separation is handled by
-// RegisterTemporaryImage.
+// is profile-agnostic.
 base::FilePath DetermineImageDirectory() {
   base::FilePath data_dir;
   bool success = base::PathService::Get(chrome::DIR_USER_DATA, &data_dir);
@@ -39,85 +41,157 @@ base::FilePath DetermineImageDirectory() {
   return data_dir.Append(kImageRoot);
 }
 
-// Writes |data| to a new temporary file at |dir_path| and returns the path to
-// the new file, or an empty path if the function fails.
-base::FilePath WriteDataToTmpFile(
-    const base::FilePath& dir_path,
-    const scoped_refptr<base::RefCountedMemory>& data) {
-  if (!base::CreateDirectoryAndGetError(dir_path, nullptr))
-    return base::FilePath();
-
-  base::FilePath temp_file;
-  if (!base::CreateTemporaryFileInDir(dir_path, &temp_file))
-    return base::FilePath();
+// Returns the full paths to all immediate file and directory children of |dir|,
+// excluding those present in |registered_paths|.
+std::vector<base::FilePath> GetFilesFromPrevSessions(
+    const base::FilePath& dir,
+    const std::set<base::FilePath>& registered_paths) {
+  // |dir| may have sub-dirs, created by the old implementation.
+  base::FileEnumerator file_enumerator(
+      dir, /*recursive=*/false,
+      base::FileEnumerator::FILES | base::FileEnumerator::DIRECTORIES,
+      FILE_PATH_LITERAL("*"));
+  std::vector<base::FilePath> files;
+
+  for (base::FilePath current = file_enumerator.Next(); !current.empty();
+       current = file_enumerator.Next()) {
+    // Exclude any new file created in this session.
+    if (!base::ContainsKey(registered_paths, current))
+      files.push_back(std::move(current));
+  }
 
-  int data_len = data->size();
-  if (base::WriteFile(temp_file, data->front_as<char>(), data_len) != data_len)
-    return base::FilePath();
+  return files;
+}
 
-  return temp_file;
+// Deletes files in |paths|.
+void DeleteFiles(std::vector<base::FilePath> paths) {
+  // |file_path| can be a directory, created by the old implementation, so
+  // delete it recursively.
+  for (const auto& file_path : paths)
+    base::DeleteFile(file_path, /*recursive=*/true);
 }
 
 }  // namespace
 
-bool NotificationImageRetainer::override_file_destruction_ = false;
-
 NotificationImageRetainer::NotificationImageRetainer(
-    scoped_refptr<base::SequencedTaskRunner> task_runner)
-    : task_runner_(task_runner) {}
+    scoped_refptr<base::SequencedTaskRunner> deletion_task_runner,
+    const base::TickClock* tick_clock)
+    : deletion_task_runner_(std::move(deletion_task_runner)),
+      image_dir_(DetermineImageDirectory()),
+      tick_clock_(tick_clock),
+      deletion_timer_(tick_clock),
+      weak_ptr_factory_(this) {
+  DCHECK(deletion_task_runner_);
+  DCHECK(tick_clock);
+
+  DETACH_FROM_SEQUENCE(sequence_checker_);
+}
+
+NotificationImageRetainer::NotificationImageRetainer()
+    : NotificationImageRetainer(
+          base::CreateSequencedTaskRunnerWithTraits(
+              {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
+               base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN}),
+          base::DefaultTickClock::GetInstance()) {}
 
 NotificationImageRetainer::~NotificationImageRetainer() {
-  if (!image_directory_.empty()) {
-    SCOPED_UMA_HISTOGRAM_TIMER(
-        "Notifications.Windows.ImageRetainerDestructionTime");
-    base::DeleteFile(image_directory_, true);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+}
+
+void NotificationImageRetainer::CleanupFilesFromPrevSessions() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  // Store all file paths from registered_images in an ordered set for quick
+  // search.
+  std::set<base::FilePath> registered_paths;
+  for (const auto& pair : registered_images_)
+    registered_paths.insert(pair.first);
+
+  std::vector<base::FilePath> files =
+      GetFilesFromPrevSessions(image_dir_, registered_paths);
+
+  // This method is run in an "after startup" task, so it is fine to directly
+  // post the DeleteFiles task to the runner.
+  if (!files.empty()) {
+    deletion_task_runner_->PostTask(
+        FROM_HERE, base::BindOnce(&DeleteFiles, std::move(files)));
   }
 }
 
 base::FilePath NotificationImageRetainer::RegisterTemporaryImage(
-    const gfx::Image& image,
-    const std::string& profile_id,
-    const GURL& origin) {
-  scoped_refptr<base::RefCountedMemory> image_data = image.As1xPNGBytes();
-  if (image_data->size() == 0)
+    const gfx::Image& image) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  scoped_refptr<base::RefCountedMemory> data = image.As1xPNGBytes();
+  if (data->size() == 0)
     return base::FilePath();
 
-  if (!initialized_) {
-    SCOPED_UMA_HISTOGRAM_TIMER(
-        "Notifications.Windows.ImageRetainerInitializationTime");
-    image_directory_ = DetermineImageDirectory();
-    // Delete the old image directory.
-    DeleteFile(image_directory_, /*recursive=*/true);
-    // Recreate the image directory.
-    if (!base::CreateDirectoryAndGetError(image_directory_, nullptr))
+  // Create the image directory. Since Chrome doesn't delete this directory
+  // after showing notifications, this directory creation should happen exactly
+  // once until Chrome is re-installed.
+  if (!base::CreateDirectory(image_dir_))
     return base::FilePath();
-    initialized_ = true;
+
+  base::FilePath temp_file;
+  if (!base::CreateTemporaryFileInDir(image_dir_, &temp_file))
+    return base::FilePath();
+
+  const base::TimeTicks now = tick_clock_->NowTicks();
+  DCHECK(registered_images_.empty() || now >= registered_images_.back().second);
+  registered_images_.emplace_back(temp_file, now);
+
+  // At this point, a temp file is already created. We need to clean it up even
+  // if it fails to write the image data to this file.
+  int data_len = base::checked_cast<int>(data->size());
+  bool data_write_success = (base::WriteFile(temp_file, data->front_as<char>(),
+                                             data_len) == data_len);
+
+  // Starts the timer if it hasn't to delete the expired files in batch. This
+  // avoids creating a deletion task for each file, otherwise the overhead can
+  // be large when there is a steady stream of notifications coming rapidly.
+  if (!deletion_timer_.IsRunning()) {
+    deletion_timer_.Start(
+        FROM_HERE, kDeletionDelay,
+        base::BindRepeating(&NotificationImageRetainer::DeleteExpiredFiles,
+                            weak_ptr_factory_.GetWeakPtr()));
   }
 
-  // To minimize the risk of collisions, separate each request by subdirectory
-  // generated from hashes of the profile and the origin. Each file within the
-  // subdirectory will also be given a unique filename.
-  base::string16 directory =
-      base::UintToString16(base::Hash(profile_id + origin.spec()));
-  base::FilePath temp_file =
-      WriteDataToTmpFile(image_directory_.Append(directory), image_data);
-
-  // Add a future task to try to delete the file. It is OK to fail, the file
-  // will get deleted later.
-  base::TimeDelta delay = override_file_destruction_
-                              ? base::TimeDelta::FromSeconds(0)
-                              : kDeletionDelay;
-  task_runner_->PostDelayedTask(
-      FROM_HERE,
-      base::Bind(base::IgnoreResult(&base::DeleteFile), temp_file,
-                 /*recursive=*/true),
-      delay);
-
-  return temp_file;
+  return data_write_success ? temp_file : base::FilePath();
+}
+
+base::WeakPtr<NotificationImageRetainer>
+NotificationImageRetainer::AsWeakPtr() {
+  return weak_ptr_factory_.GetWeakPtr();
 }
 
-// static
-void NotificationImageRetainer::OverrideTempFileLifespanForTesting(
-    bool override_file_destruction) {
-  override_file_destruction_ = override_file_destruction;
+void NotificationImageRetainer::DeleteExpiredFiles() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(!registered_images_.empty());
+
+  // Find the first file that should not be deleted.
+  const base::TimeTicks then = tick_clock_->NowTicks() - kDeletionDelay;
+  const auto end =
+      std::upper_bound(registered_images_.begin(), registered_images_.end(),
+                       std::make_pair(base::FilePath(), then),
+                       [](const PathAndTime& a, const PathAndTime& b) {
+                         return a.second < b.second;
+                       });
+  if (end == registered_images_.begin())
+    return;  // Nothing to delete yet.
+
+  // Ship the files to be deleted off to the deletion task runner.
+  std::vector<base::FilePath> files_to_delete;
+  files_to_delete.reserve(end - registered_images_.begin());
+  for (auto iter = registered_images_.begin(); iter < end; ++iter)
+    files_to_delete.push_back(std::move(iter->first));
+
+  deletion_task_runner_->PostTask(
+      FROM_HERE, base::BindOnce(&DeleteFiles, std::move(files_to_delete)));
+
+  // Erase the items to be deleted from registered_images_.
+  registered_images_.erase(registered_images_.begin(), end);
+
+  // Stop the recurring timer if all files have been deleted.
+  if (registered_images_.empty())
+    deletion_timer_.Stop();
 }

chrome/browser/notifications/win/notification_image_retainer.h

@@ -5,14 +5,18 @@
 #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/time/tick_clock.h"
+#include "base/time/time.h"
+#include "base/timer/timer.h"
 
 namespace gfx {
 class Image;
@@ -26,44 +30,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 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.
-  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,158 @@
 #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_.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) {
   auto image_retainer = std::make_unique<NotificationImageRetainer>(
-      scoped_task_environment_.GetMainThreadTaskRunner());
+      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);
 
-  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_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));
 }

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

@@ -66384,6 +66384,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>
@@ -66395,6 +66398,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>