Delete JumpListIconsOld and update revised links in every update

chrome/browser/win/jumplist.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/win/jumplist.h"
 
-#include <Shlwapi.h>
-
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/files/file_util.h"
 #include "base/macros.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/path_service.h"
 #include "base/sequenced_task_runner.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 234 matching lines @@
   }
 
   // Delete the contents in JumpListIcons directory and log the delete status
   // to UMA.
   FolderDeleteResult delete_status =
       DeleteDirectoryContent(icon_dir, kFileDeleteLimit);
 
   UMA_HISTOGRAM_ENUMERATION("WinJumplist.DeleteStatusJumpListIcons",
                             delete_status, END);
 
-  // If JumpListIcons directory is not empty, skip jumplist update and return
-  // early. If the directory doesn't exist which shouldn't though, try to create
-  // a new JumpListIcons directory. If the creation fails, return early.
+  // If JumpListIcons directory is not empty, skip updating the jumplist.
+  // If the directory doesn't exist which shouldn't though, try to create
+  // a new JumpListIcons directory. If the creation fails, skip updating the
+  // jumplist.
+
+  DirectoryStatus dir_status = NON_EXIST;
   if (base::DirectoryExists(icon_dir)) {

[grt (UTC plus 2), 2017/03/27 08:03:39]

nit: omit braces for single-line conditional and body

[chengx, 2017/03/27 18:32:41]

Done.

-    DirectoryEmptyStatus empty_status =
-        ::PathIsDirectoryEmpty(icon_dir.value().c_str()) ? EMPTY : NON_EMPTY;
-    UMA_HISTOGRAM_ENUMERATION("WinJumplist.EmptyStatusJumpListIcons",
-                              empty_status, EMPTY_STATUS_END);
-    if (empty_status == NON_EMPTY)
-      return;
-  } else if (!base::CreateDirectory(icon_dir)) {
-    return;
+    dir_status = base::IsDirectoryEmpty(icon_dir) ? EMPTY : NON_EMPTY;
   }
 
-  // Create temporary icon files for shortcuts in the "Most Visited" category.
-  CreateIconFiles(icon_dir, local_most_visited_pages);
+  if (dir_status == NON_EXIST && base::CreateDirectory(icon_dir)) {

[grt (UTC plus 2), 2017/03/27 08:03:39]

nit: omit braces

[chengx, 2017/03/27 18:32:41]

Done.

+    dir_status = EMPTY;
+  }
 
-  // Create temporary icon files for shortcuts in the "Recently Closed"
-  // category.
-  CreateIconFiles(icon_dir, local_recently_closed_pages);
+  if (dir_status == EMPTY) {
+    // Create temporary icon files for shortcuts in the "Most Visited" category.
+    CreateIconFiles(icon_dir, local_most_visited_pages);
 
-  // We finished collecting all resources needed for updating an application
-  // JumpList. So, create a new JumpList and replace the current JumpList
-  // with it.
-  UpdateJumpList(app_id.c_str(), local_most_visited_pages,
-                 local_recently_closed_pages, incognito_availability);
+    // Create temporary icon files for shortcuts in the "Recently Closed"
+    // category.
+    CreateIconFiles(icon_dir, local_recently_closed_pages);
+
+    // We finished collecting all resources needed for updating an application
+    // JumpList. So, create a new JumpList and replace the current JumpList
+    // with it.
+    UpdateJumpList(app_id.c_str(), local_most_visited_pages,
+                   local_recently_closed_pages, incognito_availability);
+  }

[grt (UTC plus 2), 2017/03/27 08:03:39]

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?

[chengx, 2017/03/27 18:32:41]

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.

[grt (UTC plus 2), 2017/03/27 19:18:47]

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?

[chengx, 2017/03/27 20:26:08]

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. 

 
   // Post a background task to delete JumpListIconsOld folder if it exists and
   // log the delete results to UMA.
   base::FilePath icon_dir_old = icon_dir.DirName().Append(
       icon_dir.BaseName().value() + FILE_PATH_LITERAL("Old"));
 
   if (base::DirectoryExists(icon_dir_old)) {
     sequenced_task_runner->PostTask(
         FROM_HERE, base::Bind(&DeleteDirectoryAndLogResults, icon_dir_old,
                               kFileDeleteLimit));
@@ skipping 324 matching lines @@
 void JumpList::TopSitesLoaded(history::TopSites* top_sites) {
 }
 
 void JumpList::TopSitesChanged(history::TopSites* top_sites,
                                ChangeReason change_reason) {
   top_sites->GetMostVisitedURLs(
       base::Bind(&JumpList::OnMostVisitedURLsAvailable,
                  weak_ptr_factory_.GetWeakPtr()),
       false);
 }