OS: Win 10 Let's use this bug to keep track related changes.
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f985ba98e23d71da6fe19d4226ca730d40b45ad7 commit f985ba98e23d71da6fe19d4226ca730d40b45ad7 Author: Xi Cheng <chengx@chromium.org> Date: Mon Sep 24 17:55:34 2018 Lower the task priority of the notification image deletion task USER_BLOCKING is too high for this task as it doesn't affect UI. This CL lowers the task priority to BEST_EFFORT. Bug: 888276 Change-Id: I5d2a45915dfb82206e2880e58d1c4ec593ef09ca Reviewed-on: https://chromium-review.googlesource.com/1239162 Commit-Queue: Xi Cheng <chengx@chromium.org> Reviewed-by: Finnur Thorarinsson <finnur@chromium.org> Reviewed-by: Robert Liao <robliao@chromium.org> Cr-Commit-Position: refs/heads/master@{#593593} [modify] https://crrev.com/f985ba98e23d71da6fe19d4226ca730d40b45ad7/chrome/browser/notifications/notification_platform_bridge_win.cc [modify] https://crrev.com/f985ba98e23d71da6fe19d4226ca730d40b45ad7/chrome/browser/notifications/notification_platform_bridge_win.h
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8894113d2c598a96ca86550938df089323194e60 commit 8894113d2c598a96ca86550938df089323194e60 Author: Xi Cheng <chengx@chromium.org> Date: Tue Sep 25 18:19:30 2018 Some tweaks for class NotificationImageRetainer 1) Change DetermineImageDirectory() from being a private class member to being in the anonymous space; 2) Make RegisterTemporaryImage() early return when there is no image data. 3) Clean up the headers. Bug: 888276 Change-Id: Ib7006121d279bfc8147ad6ae71382a95b94d05be Reviewed-on: https://chromium-review.googlesource.com/1239728 Commit-Queue: Xi Cheng <chengx@chromium.org> Reviewed-by: Finnur Thorarinsson <finnur@chromium.org> Cr-Commit-Position: refs/heads/master@{#594012} [modify] https://crrev.com/8894113d2c598a96ca86550938df089323194e60/chrome/browser/notifications/win/notification_image_retainer.cc [modify] https://crrev.com/8894113d2c598a96ca86550938df089323194e60/chrome/browser/notifications/win/notification_image_retainer.h
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/43ab0ff8fdcf3a10a89c4d0d0421f461967f2bd5 commit 43ab0ff8fdcf3a10a89c4d0d0421f461967f2bd5 Author: Xi Cheng <chengx@chromium.org> Date: Thu Sep 27 21:29:35 2018 Record notification image retainer initialization and destruction time These two metrics are used to estimate the time cost of notification image file deletion, which provides insight about whether the image folder is corrupted or not. Bug: 888276 Change-Id: I65eee40cae08007ef6dba056b3698436f0d563e7 Reviewed-on: https://chromium-review.googlesource.com/1246783 Reviewed-by: Finnur Thorarinsson <finnur@chromium.org> Reviewed-by: Steven Holte <holte@chromium.org> Commit-Queue: Xi Cheng <chengx@chromium.org> Cr-Commit-Position: refs/heads/master@{#594873} [modify] https://crrev.com/43ab0ff8fdcf3a10a89c4d0d0421f461967f2bd5/chrome/browser/notifications/win/notification_image_retainer.cc [modify] https://crrev.com/43ab0ff8fdcf3a10a89c4d0d0421f461967f2bd5/tools/metrics/histograms/histograms.xml
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b011ab5675993a0d0eb2210afe1c338e77de54a0 commit b011ab5675993a0d0eb2210afe1c338e77de54a0 Author: Xi Cheng <chengx@chromium.org> Date: Fri Oct 19 18:24:58 2018 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} [modify] https://crrev.com/b011ab5675993a0d0eb2210afe1c338e77de54a0/chrome/browser/notifications/notification_platform_bridge_win.cc [modify] https://crrev.com/b011ab5675993a0d0eb2210afe1c338e77de54a0/chrome/browser/notifications/notification_platform_bridge_win_unittest.cc [modify] https://crrev.com/b011ab5675993a0d0eb2210afe1c338e77de54a0/chrome/browser/notifications/win/mock_notification_image_retainer.cc [modify] https://crrev.com/b011ab5675993a0d0eb2210afe1c338e77de54a0/chrome/browser/notifications/win/mock_notification_image_retainer.h [modify] https://crrev.com/b011ab5675993a0d0eb2210afe1c338e77de54a0/chrome/browser/notifications/win/notification_image_retainer.cc [modify] https://crrev.com/b011ab5675993a0d0eb2210afe1c338e77de54a0/chrome/browser/notifications/win/notification_image_retainer.h [modify] https://crrev.com/b011ab5675993a0d0eb2210afe1c338e77de54a0/chrome/browser/notifications/win/notification_image_retainer_unittest.cc [modify] https://crrev.com/b011ab5675993a0d0eb2210afe1c338e77de54a0/chrome/browser/notifications/win/notification_template_builder.cc [modify] https://crrev.com/b011ab5675993a0d0eb2210afe1c338e77de54a0/chrome/browser/notifications/win/notification_template_builder.h [modify] https://crrev.com/b011ab5675993a0d0eb2210afe1c338e77de54a0/chrome/browser/notifications/win/notification_template_builder_unittest.cc [modify] https://crrev.com/b011ab5675993a0d0eb2210afe1c338e77de54a0/tools/metrics/histograms/histograms.xml
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8509079936cc4c950c0be8dc67bd9e3f0760973f commit 8509079936cc4c950c0be8dc67bd9e3f0760973f Author: Xi Cheng <chengx@chromium.org> Date: Sat Oct 20 00:45:14 2018 Revert "Upgrade image file management for Win 10 native notification" This reverts commit b011ab5675993a0d0eb2210afe1c338e77de54a0. 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/1292834 Reviewed-by: Xi Cheng <chengx@chromium.org> Commit-Queue: Xi Cheng <chengx@chromium.org> Cr-Commit-Position: refs/heads/master@{#601376} [modify] https://crrev.com/8509079936cc4c950c0be8dc67bd9e3f0760973f/chrome/browser/notifications/notification_platform_bridge_win.cc [modify] https://crrev.com/8509079936cc4c950c0be8dc67bd9e3f0760973f/chrome/browser/notifications/notification_platform_bridge_win_unittest.cc [modify] https://crrev.com/8509079936cc4c950c0be8dc67bd9e3f0760973f/chrome/browser/notifications/win/mock_notification_image_retainer.cc [modify] https://crrev.com/8509079936cc4c950c0be8dc67bd9e3f0760973f/chrome/browser/notifications/win/mock_notification_image_retainer.h [modify] https://crrev.com/8509079936cc4c950c0be8dc67bd9e3f0760973f/chrome/browser/notifications/win/notification_image_retainer.cc [modify] https://crrev.com/8509079936cc4c950c0be8dc67bd9e3f0760973f/chrome/browser/notifications/win/notification_image_retainer.h [modify] https://crrev.com/8509079936cc4c950c0be8dc67bd9e3f0760973f/chrome/browser/notifications/win/notification_image_retainer_unittest.cc [modify] https://crrev.com/8509079936cc4c950c0be8dc67bd9e3f0760973f/chrome/browser/notifications/win/notification_template_builder.cc [modify] https://crrev.com/8509079936cc4c950c0be8dc67bd9e3f0760973f/chrome/browser/notifications/win/notification_template_builder.h [modify] https://crrev.com/8509079936cc4c950c0be8dc67bd9e3f0760973f/chrome/browser/notifications/win/notification_template_builder_unittest.cc [modify] https://crrev.com/8509079936cc4c950c0be8dc67bd9e3f0760973f/tools/metrics/histograms/histograms.xml
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/78d3f4d388f39a3e9606f56f4f9f515449c08c09 commit 78d3f4d388f39a3e9606f56f4f9f515449c08c09 Author: Xi Cheng <chengx@chromium.org> Date: Tue Oct 23 01:43:36 2018 Reland "Upgrade image file management for Win 10 native notification" This is a reland of b011ab5675993a0d0eb2210afe1c338e77de54a0 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/1292894 Reviewed-by: Xi Cheng <chengx@chromium.org> Commit-Queue: Xi Cheng <chengx@chromium.org> Cr-Commit-Position: refs/heads/master@{#601819} [modify] https://crrev.com/78d3f4d388f39a3e9606f56f4f9f515449c08c09/chrome/browser/notifications/notification_platform_bridge_win.cc [modify] https://crrev.com/78d3f4d388f39a3e9606f56f4f9f515449c08c09/chrome/browser/notifications/notification_platform_bridge_win_unittest.cc [modify] https://crrev.com/78d3f4d388f39a3e9606f56f4f9f515449c08c09/chrome/browser/notifications/win/mock_notification_image_retainer.cc [modify] https://crrev.com/78d3f4d388f39a3e9606f56f4f9f515449c08c09/chrome/browser/notifications/win/mock_notification_image_retainer.h [modify] https://crrev.com/78d3f4d388f39a3e9606f56f4f9f515449c08c09/chrome/browser/notifications/win/notification_image_retainer.cc [modify] https://crrev.com/78d3f4d388f39a3e9606f56f4f9f515449c08c09/chrome/browser/notifications/win/notification_image_retainer.h [modify] https://crrev.com/78d3f4d388f39a3e9606f56f4f9f515449c08c09/chrome/browser/notifications/win/notification_image_retainer_unittest.cc [modify] https://crrev.com/78d3f4d388f39a3e9606f56f4f9f515449c08c09/chrome/browser/notifications/win/notification_template_builder.cc [modify] https://crrev.com/78d3f4d388f39a3e9606f56f4f9f515449c08c09/chrome/browser/notifications/win/notification_template_builder.h [modify] https://crrev.com/78d3f4d388f39a3e9606f56f4f9f515449c08c09/chrome/browser/notifications/win/notification_template_builder_unittest.cc [modify] https://crrev.com/78d3f4d388f39a3e9606f56f4f9f515449c08c09/tools/metrics/histograms/histograms.xml
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/148a8316031a29d0793e033e85553bc4043dcc47 commit 148a8316031a29d0793e033e85553bc4043dcc47 Author: Xi Cheng <chengx@chromium.org> Date: Wed Oct 24 22:35:39 2018 Optimize pointer usage for NotificationImageRetainer object Previously, a pointer to the NotificationImageRetainer object is stored in NotificationTemplateBuilder. This is unnecessary as this object is only used by two private methods called within Build() and never used again. This CL changed to pass the pointer into those functions directly. Separately, a GetCleanupTask() method is added for NotificationImageRetainer so that it doesn't need to expose AsWeakPtr() to the public. Bug: 888276 Change-Id: I4a7fa2a39e30c739f9adb7021b4809a4b56062fa Reviewed-on: https://chromium-review.googlesource.com/c/1292712 Commit-Queue: Xi Cheng <chengx@chromium.org> Reviewed-by: Greg Thompson <grt@chromium.org> Reviewed-by: Finnur Thorarinsson <finnur@chromium.org> Cr-Commit-Position: refs/heads/master@{#602496} [modify] https://crrev.com/148a8316031a29d0793e033e85553bc4043dcc47/chrome/browser/notifications/notification_platform_bridge_win.cc [modify] https://crrev.com/148a8316031a29d0793e033e85553bc4043dcc47/chrome/browser/notifications/notification_platform_bridge_win_unittest.cc [modify] https://crrev.com/148a8316031a29d0793e033e85553bc4043dcc47/chrome/browser/notifications/win/notification_image_retainer.cc [modify] https://crrev.com/148a8316031a29d0793e033e85553bc4043dcc47/chrome/browser/notifications/win/notification_image_retainer.h [modify] https://crrev.com/148a8316031a29d0793e033e85553bc4043dcc47/chrome/browser/notifications/win/notification_template_builder.cc [modify] https://crrev.com/148a8316031a29d0793e033e85553bc4043dcc47/chrome/browser/notifications/win/notification_template_builder.h [modify] https://crrev.com/148a8316031a29d0793e033e85553bc4043dcc47/chrome/browser/notifications/win/notification_template_builder_unittest.cc
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f54583b6a04bfc946ac8b6700f96073176ec4e06 commit f54583b6a04bfc946ac8b6700f96073176ec4e06 Author: Xi Cheng <chengx@chromium.org> Date: Thu Oct 25 17:00:10 2018 Use FilePath rather than string16 for file names Bug: 888276 Change-Id: I73658cfcbe755a3c68fe64610f2cca3e213c26d2 Reviewed-on: https://chromium-review.googlesource.com/c/1298349 Reviewed-by: Greg Thompson <grt@chromium.org> Commit-Queue: Xi Cheng <chengx@chromium.org> Cr-Commit-Position: refs/heads/master@{#602763} [modify] https://crrev.com/f54583b6a04bfc946ac8b6700f96073176ec4e06/chrome/browser/notifications/win/notification_image_retainer.cc [modify] https://crrev.com/f54583b6a04bfc946ac8b6700f96073176ec4e06/chrome/browser/notifications/win/notification_image_retainer.h
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0d633b3b0feef6b14830902a0e30281b38be65ae commit 0d633b3b0feef6b14830902a0e30281b38be65ae Author: Xi Cheng <chengx@chromium.org> Date: Mon Nov 05 01:21:41 2018 Simplify class NotificationTemplateBuilder to a single public function Bug: 888276 Change-Id: Ibd3e557ad55ff48daf1e3347ec27ff6fd235a47f Reviewed-on: https://chromium-review.googlesource.com/c/1311837 Commit-Queue: Xi Cheng <chengx@chromium.org> Reviewed-by: Greg Thompson <grt@chromium.org> Cr-Commit-Position: refs/heads/master@{#605223} [modify] https://crrev.com/0d633b3b0feef6b14830902a0e30281b38be65ae/chrome/browser/notifications/notification_platform_bridge_win.cc [modify] https://crrev.com/0d633b3b0feef6b14830902a0e30281b38be65ae/chrome/browser/notifications/notification_platform_bridge_win.h [modify] https://crrev.com/0d633b3b0feef6b14830902a0e30281b38be65ae/chrome/browser/notifications/notification_platform_bridge_win_unittest.cc [modify] https://crrev.com/0d633b3b0feef6b14830902a0e30281b38be65ae/chrome/browser/notifications/win/notification_template_builder.cc [modify] https://crrev.com/0d633b3b0feef6b14830902a0e30281b38be65ae/chrome/browser/notifications/win/notification_template_builder.h [modify] https://crrev.com/0d633b3b0feef6b14830902a0e30281b38be65ae/chrome/browser/notifications/win/notification_template_builder_unittest.cc
Comment 1 by chengx@chromium.org
, Sep 24Components: UI>Notifications