Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2778513002:
Delete JumpListIconsOld and update revised links in every update (Closed)
DescriptionDelete JumpListIconsOld and update revised links in every update
This CL includes the following changes:
1) In crrev.com/2752063002 landed recently, when JumpListIcons is not
empty or doesn't exist, we are not trying to delete JumpListIconsOld.
This is not optimal, as deleting JumpListIconsOld is now independent of
the folder status of JumpListIcons.
2) When the JumpListIcons folder is not empty after the deletion of its
content, currently we skip creating the new icons as well as updating
the revised links. However, updating the links shouldn't be skipped as
it doesn't involve file IO. This CL fixes it.
3) This CL logs more detailed directory information for jumplist folders
to UMA. In addition to logging if the folder is empty or not, it now
also logs if the folder exists or not.
4) Use base::IsDirectoryEmpty to replace PathIsDirectoryEmpty Windows
API. Thus we can remove the inclusion of Shlwapi.h in a few files.
5) Reduce the limit of files to delete from 100 to 60. One reason is
that setting the limit to more than twice of the normal amount is more
than enough. Another reason is that we are now trying to delete
JumpListIconsOld folder in every jumplist update.
BUG=40407, 179576
Review-Url: https://codereview.chromium.org/2778513002
Cr-Commit-Position: refs/heads/master@{#460023}
Committed: https://chromium.googlesource.com/chromium/src/+/4e4edb9d083a96781c37b447cf47e4efd7ca8eb7
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix nits in histograms.xml #
Total comments: 14
Patch Set 3 : Update jumplist links anyway #
Total comments: 6
Patch Set 4 : Address comments. #
Messages
Total messages: 59 (50 generated)
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Try to delete JumpListIconsOld directory in every update BUG=40407, 179576 ========== to ========== Delete JumpListIconsOld directory in every update and log more detailed directory status to UMA BUG=40407, 179576 ==========
Description was changed from ========== Delete JumpListIconsOld directory in every update and log more detailed directory status to UMA BUG=40407, 179576 ========== to ========== Delete JumpListIconsOld in every update and log detailed directory status to UMA BUG=40407, 179576 ==========
Description was changed from ========== Delete JumpListIconsOld in every update and log detailed directory status to UMA BUG=40407, 179576 ========== to ========== Delete JumpListIconsOld in every update and log detailed directory status to UMA In crrev.com/2752063002/ landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld in addition to skipping the jumplist icon update. This is not optimal, as deleting JumpListIconsOld should be independent of the status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. Also, base::IsDirectoryEmpty() is used to replace PathIsDirectoryEmpty Windows API. Thus we can get rid of the inclusion of <Shlwapi.h>. BUG=40407, 179576 ==========
Description was changed from ========== Delete JumpListIconsOld in every update and log detailed directory status to UMA In crrev.com/2752063002/ landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld in addition to skipping the jumplist icon update. This is not optimal, as deleting JumpListIconsOld should be independent of the status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. Also, base::IsDirectoryEmpty() is used to replace PathIsDirectoryEmpty Windows API. Thus we can get rid of the inclusion of <Shlwapi.h>. BUG=40407, 179576 ========== to ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002/ landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld in addition to skipping the jumplist icon update. This is not optimal, as deleting JumpListIconsOld should be independent of the status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. Also, base::IsDirectoryEmpty() is used to replace PathIsDirectoryEmpty Windows API. Thus we can get rid of the inclusion of <Shlwapi.h>. BUG=40407, 179576 ==========
Description was changed from ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002/ landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld in addition to skipping the jumplist icon update. This is not optimal, as deleting JumpListIconsOld should be independent of the status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. Also, base::IsDirectoryEmpty() is used to replace PathIsDirectoryEmpty Windows API. Thus we can get rid of the inclusion of <Shlwapi.h>. BUG=40407, 179576 ========== to ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002/ landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld in addition to skipping the jumplist icon update. This is not optimal, as deleting JumpListIconsOld should be independent of the status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. Moreover, base::IsDirectoryEmpty() is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of <Shlwapi.h>. BUG=40407, 179576 ==========
Description was changed from ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002/ landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld in addition to skipping the jumplist icon update. This is not optimal, as deleting JumpListIconsOld should be independent of the status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. Moreover, base::IsDirectoryEmpty() is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of <Shlwapi.h>. BUG=40407, 179576 ========== to ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002/ landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld in addition to skipping the jumplist icon update. This is not optimal, as deleting JumpListIconsOld should be independent of the status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. Moreover, base::IsDirectoryEmpty() is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of <Shlwapi.h>. BUG=40407, 179576 ==========
Description was changed from ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002/ landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld in addition to skipping the jumplist icon update. This is not optimal, as deleting JumpListIconsOld should be independent of the status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. Moreover, base::IsDirectoryEmpty() is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of <Shlwapi.h>. BUG=40407, 179576 ========== to ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002/ landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld in addition to skipping the jumplist icon update. This is not optimal, as deleting JumpListIconsOld should be independent of the status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. Moreover, base::IsDirectoryEmpty is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of <Shlwapi.h>. BUG=40407, 179576 ==========
chengx@chromium.org changed reviewers: + grt@chromium.org, isherman@chromium.org
This is a small follow up improvement to crrev.com/2752063002 just landed. PTAL~
Metrics LGTM % nits: https://codereview.chromium.org/2778513002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2778513002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:80423: + This metric records the exist/empty status of folder JumpListIcons. nit: "This metric records whether the folder JumpListIcons exists; and if it does, whether it is empty or non-empty." https://codereview.chromium.org/2778513002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:98602: + <int value="2" label="Non-Exist"/> nit: "Doesn't exist"
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002/ landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld in addition to skipping the jumplist icon update. This is not optimal, as deleting JumpListIconsOld should be independent of the status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. Moreover, base::IsDirectoryEmpty is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of <Shlwapi.h>. BUG=40407, 179576 ========== to ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld in addition to skipping the jumplist icon update. This is not optimal, as deleting JumpListIconsOld should be independent of the status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. Moreover, base::IsDirectoryEmpty is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of <Shlwapi.h>. BUG=40407, 179576 ==========
Description was changed from ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld in addition to skipping the jumplist icon update. This is not optimal, as deleting JumpListIconsOld should be independent of the status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. Moreover, base::IsDirectoryEmpty is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of <Shlwapi.h>. BUG=40407, 179576 ========== to ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld in addition to skipping the jumplist icon update. This is not optimal, as deleting JumpListIconsOld should be independent of the folder status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. Moreover, base::IsDirectoryEmpty is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of <Shlwapi.h>. BUG=40407, 179576 ==========
Description was changed from ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld in addition to skipping the jumplist icon update. This is not optimal, as deleting JumpListIconsOld should be independent of the folder status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. Moreover, base::IsDirectoryEmpty is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of <Shlwapi.h>. BUG=40407, 179576 ========== to ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld in addition to skipping the jumplist icon update. This is not optimal, as deleting JumpListIconsOld should be independent of the folder status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. Moreover, base::IsDirectoryEmpty is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of <Shlwapi.h>. BUG=40407, 179576 ==========
Description was changed from ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld in addition to skipping the jumplist icon update. This is not optimal, as deleting JumpListIconsOld should be independent of the folder status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. Moreover, base::IsDirectoryEmpty is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of <Shlwapi.h>. BUG=40407, 179576 ========== to ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld in addition to skipping the jumplist icon update. This is not optimal, as deleting JumpListIconsOld is now independent of the folder status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. Moreover, base::IsDirectoryEmpty is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of <Shlwapi.h>. BUG=40407, 179576 ==========
Description was changed from ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld in addition to skipping the jumplist icon update. This is not optimal, as deleting JumpListIconsOld is now independent of the folder status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. Moreover, base::IsDirectoryEmpty is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of <Shlwapi.h>. BUG=40407, 179576 ========== to ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld in addition to skipping the jumplist icon update. This is not optimal, as deleting JumpListIconsOld is now independent of the folder status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. Moreover, base::IsDirectoryEmpty is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of <Shlwapi.h>. BUG=40407, 179576 ==========
Description was changed from ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld in addition to skipping the jumplist icon update. This is not optimal, as deleting JumpListIconsOld is now independent of the folder status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. Moreover, base::IsDirectoryEmpty is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of <Shlwapi.h>. BUG=40407, 179576 ========== to ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld in addition to skipping the jumplist icon update. This is not optimal, as deleting JumpListIconsOld is now independent of the folder status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. Moreover, base::IsDirectoryEmpty is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of Shlwapi.h in some files. BUG=40407, 179576 ==========
Description was changed from ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld in addition to skipping the jumplist icon update. This is not optimal, as deleting JumpListIconsOld is now independent of the folder status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. Moreover, base::IsDirectoryEmpty is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of Shlwapi.h in some files. BUG=40407, 179576 ========== to ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld in addition to skipping the jumplist icon update. This is not optimal, as deleting JumpListIconsOld is now independent of the folder status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. Moreover, base::IsDirectoryEmpty is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of Shlwapi.h in a few files. BUG=40407, 179576 ==========
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld in addition to skipping the jumplist icon update. This is not optimal, as deleting JumpListIconsOld is now independent of the folder status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. Moreover, base::IsDirectoryEmpty is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of Shlwapi.h in a few files. BUG=40407, 179576 ========== to ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld. This is not optimal, as deleting JumpListIconsOld is now independent of the folder status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. Moreover, base::IsDirectoryEmpty is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of Shlwapi.h in a few files. BUG=40407, 179576 ==========
Description was changed from ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld. This is not optimal, as deleting JumpListIconsOld is now independent of the folder status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. Moreover, base::IsDirectoryEmpty is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of Shlwapi.h in a few files. BUG=40407, 179576 ========== to ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld. This is not optimal, as deleting JumpListIconsOld is now independent of the folder status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. Moreover, base::IsDirectoryEmpty is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of Shlwapi.h in a few files. BUG=40407, 179576 ==========
Description was changed from ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld. This is not optimal, as deleting JumpListIconsOld is now independent of the folder status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. Moreover, base::IsDirectoryEmpty is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of Shlwapi.h in a few files. BUG=40407, 179576 ========== to ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld. This is not optimal, as deleting JumpListIconsOld is now independent of the folder status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. Moreover, base::IsDirectoryEmpty is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of Shlwapi.h in a few files. BUG=40407, 179576 ==========
https://codereview.chromium.org/2778513002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2778513002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:267: if (base::DirectoryExists(icon_dir)) { nit: omit braces for single-line conditional and body https://codereview.chromium.org/2778513002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:271: if (dir_status == NON_EXIST && base::CreateDirectory(icon_dir)) { nit: omit braces https://codereview.chromium.org/2778513002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:288: } in the "else" case, the jumplist retains the old most visited and recently closed links, but the icons may be missing, correct? is it better to update it with the revised links but no icons? https://codereview.chromium.org/2778513002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist_file_util.cc (right): https://codereview.chromium.org/2778513002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist_file_util.cc:116: if (base::DirectoryExists(path)) { nit: omit braces https://codereview.chromium.org/2778513002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist_file_util.h (right): https://codereview.chromium.org/2778513002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist_file_util.h:8: #include "base/files/file_util.h" i believe file_path.h is still correct based on iwyu -- file_util.h should be included by the .cc file
Description was changed from ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld. This is not optimal, as deleting JumpListIconsOld is now independent of the folder status of JumpListIcons. Besides, this CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. Moreover, base::IsDirectoryEmpty is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of Shlwapi.h in a few files. BUG=40407, 179576 ========== to ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA This CL includes the following changes: 1) In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld. This is not optimal, as deleting JumpListIconsOld is now independent of the folder status of JumpListIcons. 2) When the JumpListIcons folder is not empty after the deletion of its content, currently we skip creating the new icons as well as updating the revised links. However, updating the links shouldn't be skipped as it doesn't involve file IO. This CL fixes it. 3) This CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. 4) base::IsDirectoryEmpty is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of Shlwapi.h in a few files. 5) Reduce the limit of files to delete from 100 to 60. One reason is that setting the limit to more than twice of the normal amount is more than enough. Another reason is that we are now trying to delete JumpListIconsOld folder in every jumplist update. BUG=40407, 179576 ==========
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA This CL includes the following changes: 1) In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld. This is not optimal, as deleting JumpListIconsOld is now independent of the folder status of JumpListIcons. 2) When the JumpListIcons folder is not empty after the deletion of its content, currently we skip creating the new icons as well as updating the revised links. However, updating the links shouldn't be skipped as it doesn't involve file IO. This CL fixes it. 3) This CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. 4) base::IsDirectoryEmpty is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of Shlwapi.h in a few files. 5) Reduce the limit of files to delete from 100 to 60. One reason is that setting the limit to more than twice of the normal amount is more than enough. Another reason is that we are now trying to delete JumpListIconsOld folder in every jumplist update. BUG=40407, 179576 ========== to ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA This CL includes the following changes: 1) In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld. This is not optimal, as deleting JumpListIconsOld is now independent of the folder status of JumpListIcons. 2) When the JumpListIcons folder is not empty after the deletion of its content, currently we skip creating the new icons as well as updating the revised links. However, updating the links shouldn't be skipped as it doesn't involve file IO. This CL fixes it. 3) This CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. 4) base::IsDirectoryEmpty is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of Shlwapi.h in a few files. 5) Reduce the limit of files to delete from 100 to 60. One reason is that setting the limit to more than twice of the normal amount is more than enough. Another reason is that we are now trying to delete JumpListIconsOld folder in every jumplist update. BUG=40407, 179576 ==========
Description was changed from ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA This CL includes the following changes: 1) In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld. This is not optimal, as deleting JumpListIconsOld is now independent of the folder status of JumpListIcons. 2) When the JumpListIcons folder is not empty after the deletion of its content, currently we skip creating the new icons as well as updating the revised links. However, updating the links shouldn't be skipped as it doesn't involve file IO. This CL fixes it. 3) This CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. 4) base::IsDirectoryEmpty is used to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of Shlwapi.h in a few files. 5) Reduce the limit of files to delete from 100 to 60. One reason is that setting the limit to more than twice of the normal amount is more than enough. Another reason is that we are now trying to delete JumpListIconsOld folder in every jumplist update. BUG=40407, 179576 ========== to ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA This CL includes the following changes: 1) In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld. This is not optimal, as deleting JumpListIconsOld is now independent of the folder status of JumpListIcons. 2) When the JumpListIcons folder is not empty after the deletion of its content, currently we skip creating the new icons as well as updating the revised links. However, updating the links shouldn't be skipped as it doesn't involve file IO. This CL fixes it. 3) This CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. 4) Use base::IsDirectoryEmpty to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of Shlwapi.h in a few files. 5) Reduce the limit of files to delete from 100 to 60. One reason is that setting the limit to more than twice of the normal amount is more than enough. Another reason is that we are now trying to delete JumpListIconsOld folder in every jumplist update. BUG=40407, 179576 ==========
Description was changed from ========== Delete JumpListIconsOld in every update and log more detailed directory status to UMA This CL includes the following changes: 1) In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld. This is not optimal, as deleting JumpListIconsOld is now independent of the folder status of JumpListIcons. 2) When the JumpListIcons folder is not empty after the deletion of its content, currently we skip creating the new icons as well as updating the revised links. However, updating the links shouldn't be skipped as it doesn't involve file IO. This CL fixes it. 3) This CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. 4) Use base::IsDirectoryEmpty to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of Shlwapi.h in a few files. 5) Reduce the limit of files to delete from 100 to 60. One reason is that setting the limit to more than twice of the normal amount is more than enough. Another reason is that we are now trying to delete JumpListIconsOld folder in every jumplist update. BUG=40407, 179576 ========== to ========== Delete folder JumpListIconsOld and update jumplist revised links in every update This CL includes the following changes: 1) In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld. This is not optimal, as deleting JumpListIconsOld is now independent of the folder status of JumpListIcons. 2) When the JumpListIcons folder is not empty after the deletion of its content, currently we skip creating the new icons as well as updating the revised links. However, updating the links shouldn't be skipped as it doesn't involve file IO. This CL fixes it. 3) This CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. 4) Use base::IsDirectoryEmpty to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of Shlwapi.h in a few files. 5) Reduce the limit of files to delete from 100 to 60. One reason is that setting the limit to more than twice of the normal amount is more than enough. Another reason is that we are now trying to delete JumpListIconsOld folder in every jumplist update. BUG=40407, 179576 ==========
Description was changed from ========== Delete folder JumpListIconsOld and update jumplist revised links in every update This CL includes the following changes: 1) In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld. This is not optimal, as deleting JumpListIconsOld is now independent of the folder status of JumpListIcons. 2) When the JumpListIcons folder is not empty after the deletion of its content, currently we skip creating the new icons as well as updating the revised links. However, updating the links shouldn't be skipped as it doesn't involve file IO. This CL fixes it. 3) This CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. 4) Use base::IsDirectoryEmpty to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of Shlwapi.h in a few files. 5) Reduce the limit of files to delete from 100 to 60. One reason is that setting the limit to more than twice of the normal amount is more than enough. Another reason is that we are now trying to delete JumpListIconsOld folder in every jumplist update. BUG=40407, 179576 ========== to ========== Delete JumpListIconsOld and update revised links in every update This CL includes the following changes: 1) In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld. This is not optimal, as deleting JumpListIconsOld is now independent of the folder status of JumpListIcons. 2) When the JumpListIcons folder is not empty after the deletion of its content, currently we skip creating the new icons as well as updating the revised links. However, updating the links shouldn't be skipped as it doesn't involve file IO. This CL fixes it. 3) This CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. 4) Use base::IsDirectoryEmpty to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of Shlwapi.h in a few files. 5) Reduce the limit of files to delete from 100 to 60. One reason is that setting the limit to more than twice of the normal amount is more than enough. Another reason is that we are now trying to delete JumpListIconsOld folder in every jumplist update. BUG=40407, 179576 ==========
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:40001) has been deleted
All the feedback comments are addressed in the new patch set. PTAL, thanks! https://codereview.chromium.org/2778513002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2778513002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:267: if (base::DirectoryExists(icon_dir)) { On 2017/03/27 08:03:39, grt (UTC plus 2) wrote: > nit: omit braces for single-line conditional and body Done. https://codereview.chromium.org/2778513002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:271: if (dir_status == NON_EXIST && base::CreateDirectory(icon_dir)) { On 2017/03/27 08:03:39, grt (UTC plus 2) wrote: > nit: omit braces Done. https://codereview.chromium.org/2778513002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:288: } On 2017/03/27 08:03:39, grt (UTC plus 2) wrote: > in the "else" case, the jumplist retains the old most visited and recently > closed links, but the icons may be missing, correct? is it better to update it > with the revised links but no icons? Correct, in this case, the jumplist retains the old most visited and recently closed links, but the icons may be missing. Agreed that we should update the revised links even we are not creating new icons for them. https://codereview.chromium.org/2778513002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist_file_util.cc (right): https://codereview.chromium.org/2778513002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist_file_util.cc:116: if (base::DirectoryExists(path)) { On 2017/03/27 08:03:39, grt (UTC plus 2) wrote: > nit: omit braces Done. https://codereview.chromium.org/2778513002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist_file_util.h (right): https://codereview.chromium.org/2778513002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist_file_util.h:8: #include "base/files/file_util.h" On 2017/03/27 08:03:39, grt (UTC plus 2) wrote: > i believe file_path.h is still correct based on iwyu -- file_util.h should be > included by the .cc file Done.
https://codereview.chromium.org/2778513002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2778513002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:288: } On 2017/03/27 18:32:41, chengx wrote: > On 2017/03/27 08:03:39, grt (UTC plus 2) wrote: > > in the "else" case, the jumplist retains the old most visited and recently > > closed links, but the icons may be missing, correct? is it better to update it > > with the revised links but no icons? > > Correct, in this case, the jumplist retains the old most visited and recently > closed links, but the icons may be missing. > > Agreed that we should update the revised links even we are not creating new > icons for them. how does the shell handle this -- is Chrome's icon used, or is there a blank space where the icon should be, or something else? https://codereview.chromium.org/2778513002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist_file_util.h (right): https://codereview.chromium.org/2778513002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist_file_util.h:8: #include "base/files/file_util.h" On 2017/03/27 18:32:41, chengx wrote: > On 2017/03/27 08:03:39, grt (UTC plus 2) wrote: > > i believe file_path.h is still correct based on iwyu -- file_util.h should be > > included by the .cc file > > Done. ping https://codereview.chromium.org/2778513002/diff/60001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2778513002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:273: UMA WinJumplist.DirectoryStatusJumpListIcons here? https://codereview.chromium.org/2778513002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:275: // Create temporary icon files for shortcuts in the "Most Visited" category. the files aren't really temporary, are they? their names may be random, but they need to persist as long as the shell references them. https://codereview.chromium.org/2778513002/diff/60001/chrome/browser/win/jump... File chrome/browser/win/jumplist_file_util.h (right): https://codereview.chromium.org/2778513002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist_file_util.h:11: const int kFileDeleteLimit = 60; why lower this? do metrics indicate that 100 is too much?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL~ https://codereview.chromium.org/2778513002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2778513002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:288: } On 2017/03/27 19:18:47, grt (UTC plus 2) wrote: > On 2017/03/27 18:32:41, chengx wrote: > > On 2017/03/27 08:03:39, grt (UTC plus 2) wrote: > > > in the "else" case, the jumplist retains the old most visited and recently > > > closed links, but the icons may be missing, correct? is it better to update > it > > > with the revised links but no icons? > > > > Correct, in this case, the jumplist retains the old most visited and recently > > closed links, but the icons may be missing. > > > > Agreed that we should update the revised links even we are not creating new > > icons for them. > > how does the shell handle this -- is Chrome's icon used, or is there a blank > space where the icon should be, or something else? In this case, Chrome's icon will be used. The case where a blank space is used is when the icons in JumpListIcons are deleted after the update. This time, when right clicking the Chrome icon in the task bar, a blanks space will be used as the icons are missing now. https://codereview.chromium.org/2778513002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist_file_util.h (right): https://codereview.chromium.org/2778513002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist_file_util.h:8: #include "base/files/file_util.h" On 2017/03/27 19:18:47, grt (UTC plus 2) wrote: > On 2017/03/27 18:32:41, chengx wrote: > > On 2017/03/27 08:03:39, grt (UTC plus 2) wrote: > > > i believe file_path.h is still correct based on iwyu -- file_util.h should > be > > > included by the .cc file > > > > Done. > > ping Ah, sorry... Now it is fixed. https://codereview.chromium.org/2778513002/diff/60001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2778513002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:273: On 2017/03/27 19:18:47, grt (UTC plus 2) wrote: > UMA WinJumplist.DirectoryStatusJumpListIcons here? Ah, my bad. Now it is back. https://codereview.chromium.org/2778513002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:275: // Create temporary icon files for shortcuts in the "Most Visited" category. On 2017/03/27 19:18:47, grt (UTC plus 2) wrote: > the files aren't really temporary, are they? their names may be random, but they > need to persist as long as the shell references them. Those are old comments existed for a very long time. But you are right, they need to persist as long as they are referenced by the shell. I have remove “temporary”. https://codereview.chromium.org/2778513002/diff/60001/chrome/browser/win/jump... File chrome/browser/win/jumplist_file_util.h (right): https://codereview.chromium.org/2778513002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist_file_util.h:11: const int kFileDeleteLimit = 60; On 2017/03/27 19:18:48, grt (UTC plus 2) wrote: > why lower this? do metrics indicate that 100 is too much? For users having no trouble in using Chrome, 60 and 100 make no difference. For users who have already thousands of icons in the folder, the difference between 60 and 100 is that as of 60, it takes less time to delete icons per update, but more times to delete them all. However, given that it is suspicious base::Delete has no guarantee that the icons are gone even the function return value is SUCCESS, reducing the limit from 100 to 60 may give users a litter better experience.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by chengx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2778513002/#ps80001 (title: "Address comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490680301604650,
"parent_rev": "6cbf12a1814e63a869a21890199f3a7babf51867", "commit_rev":
"4e4edb9d083a96781c37b447cf47e4efd7ca8eb7"}
Message was sent while issue was closed.
Description was changed from ========== Delete JumpListIconsOld and update revised links in every update This CL includes the following changes: 1) In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld. This is not optimal, as deleting JumpListIconsOld is now independent of the folder status of JumpListIcons. 2) When the JumpListIcons folder is not empty after the deletion of its content, currently we skip creating the new icons as well as updating the revised links. However, updating the links shouldn't be skipped as it doesn't involve file IO. This CL fixes it. 3) This CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. 4) Use base::IsDirectoryEmpty to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of Shlwapi.h in a few files. 5) Reduce the limit of files to delete from 100 to 60. One reason is that setting the limit to more than twice of the normal amount is more than enough. Another reason is that we are now trying to delete JumpListIconsOld folder in every jumplist update. BUG=40407, 179576 ========== to ========== Delete JumpListIconsOld and update revised links in every update This CL includes the following changes: 1) In crrev.com/2752063002 landed recently, when JumpListIcons is not empty or doesn't exist, we are not trying to delete JumpListIconsOld. This is not optimal, as deleting JumpListIconsOld is now independent of the folder status of JumpListIcons. 2) When the JumpListIcons folder is not empty after the deletion of its content, currently we skip creating the new icons as well as updating the revised links. However, updating the links shouldn't be skipped as it doesn't involve file IO. This CL fixes it. 3) This CL logs more detailed directory information for jumplist folders to UMA. In addition to logging if the folder is empty or not, it now also logs if the folder exists or not. 4) Use base::IsDirectoryEmpty to replace PathIsDirectoryEmpty Windows API. Thus we can remove the inclusion of Shlwapi.h in a few files. 5) Reduce the limit of files to delete from 100 to 60. One reason is that setting the limit to more than twice of the normal amount is more than enough. Another reason is that we are now trying to delete JumpListIconsOld folder in every jumplist update. BUG=40407, 179576 Review-Url: https://codereview.chromium.org/2778513002 Cr-Commit-Position: refs/heads/master@{#460023} Committed: https://chromium.googlesource.com/chromium/src/+/4e4edb9d083a96781c37b447cf47... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/4e4edb9d083a96781c37b447cf47... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
