Re-enable UMA metric WinJumplist.CreateIconFilesCount

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 <utility>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 97 matching lines @@
     for (auto& scale : supported_scales) {
       gfx::ImageSkiaRep image_skia_rep = image_skia.GetRepresentation(scale);
       if (!image_skia_rep.is_null())
         image_family.Add(
             gfx::Image::CreateFrom1xBitmap(image_skia_rep.sk_bitmap()));
     }
   }
 
   if (!IconUtil::CreateIconFileFromImageFamily(image_family, path,
                                                IconUtil::NORMAL_WRITE))
     return false;

[grt (UTC plus 2), 2017/04/28 07:17:00]

this will leak the temp file. should you not delete it in this failure case?

[chengx, 2017/04/28 18:12:40]

Yes, it should be deleted here right away rather than in the next JumpList
update. I'll use another small CL for this. 

Thanks for catching this bug!

 
   // Add this icon file to the list and return its absolute path.
   // The IShellLink::SetIcon() function needs the absolute path to an icon.
   *icon_path = path;
   return true;
 }
 
 // Helper method for RunUpdate to create icon files for the asynchrounously
 // loaded icons.
 void CreateIconFiles(const base::FilePath& icon_dir,
@@ skipping 104 matching lines @@
     --user_max_items_adjusted;
 
   size_t most_visited_items =
       MulDiv(user_max_items_adjusted, kMostVisited, kTotal);
   size_t recently_closed_items = user_max_items_adjusted - most_visited_items;
   if (recently_closed_pages.size() < recently_closed_items) {
     most_visited_items += recently_closed_items - recently_closed_pages.size();
     recently_closed_items = recently_closed_pages.size();
   }
 
+  // Record the amout of icons created per JumpList update.

[grt (UTC plus 2), 2017/04/28 07:17:00]

nit: amount -> number

[chengx, 2017/04/28 18:12:40]

Done.

+  int icons_created = 0;
+
   if (most_visited_pages_have_updates) {
     // Delete the content in JumpListIconsMostVisited folder and log the results
     // to UMA.
     base::FilePath icon_dir_most_visited = icon_dir.DirName().Append(
         icon_dir.BaseName().value() + FILE_PATH_LITERAL("MostVisited"));
 
     DeleteDirectoryContentAndLogResults(icon_dir_most_visited,
                                         kFileDeleteLimit);
 
     // If the directory doesn't exist (we have tried to create it in
     // DeleteDirectoryContentAndLogResults) or is not empty, skip updating the
     // jumplist icons. The jumplist links should be updated anyway, as it
     // doesn't involve disk IO. In this case, Chrome's icon will be used for the
     // new links.
     if (base::DirectoryExists(icon_dir_most_visited) &&
         base::IsDirectoryEmpty(icon_dir_most_visited)) {
       // Create icon files for shortcuts in the "Most Visited" category.
       CreateIconFiles(icon_dir_most_visited, most_visited_pages,
                       most_visited_items);
+
+      icons_created += std::min(most_visited_pages.size(), most_visited_items);

[grt (UTC plus 2), 2017/04/28 07:17:00]

is the intention to report the number of files successfully created? if so,
there are error paths in CreateIconFiles that may result in fewer files being
created than requested. how about making CreateIconFiles return the number of
files it actually created so that the real value can be used directly?

[chengx, 2017/04/28 18:12:40]

Actually, I only care about the number of icons that Chrome requests to create.
The number of icons created may be fewer than that of being requested, which can
be due to various reasons. Some of them are even unrelated to Chrome. What I
focus on is to reduce the request amount.

I have renamed "icons_created" to "icons_to_create", and updated the CL
description to better convey my intention. Sorry about the confusion.

     }
   }
 
   if (recently_closed_pages_have_updates) {
     // Delete the content in JumpListIconsRecentClosed folder and log the
     // results to UMA.
     base::FilePath icon_dir_recent_closed = icon_dir.DirName().Append(
         icon_dir.BaseName().value() + FILE_PATH_LITERAL("RecentClosed"));
 
     DeleteDirectoryContentAndLogResults(icon_dir_recent_closed,
                                         kFileDeleteLimit);
 
     if (base::DirectoryExists(icon_dir_recent_closed) &&
         base::IsDirectoryEmpty(icon_dir_recent_closed)) {
       // Create icon files for shortcuts in the "Recently Closed" category.
       CreateIconFiles(icon_dir_recent_closed, recently_closed_pages,
                       recently_closed_items);
+
+      icons_created +=
+          std::min(recently_closed_pages.size(), recently_closed_items);
     }
   }
 
   // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
+  UMA_HISTOGRAM_COUNTS_100("WinJumplist.CreateIconFilesCount", icons_created);
+
+  // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
   SCOPED_UMA_HISTOGRAM_TIMER("WinJumplist.UpdateJumpListDuration");
 
   // Update the "Most Visited" category of the JumpList if it exists.
   // This update request is applied into the JumpList when we commit this
   // transaction.
   if (!jumplist_updater.AddCustomCategory(
           l10n_util::GetStringUTF16(IDS_NEW_TAB_MOST_VISITED),
           most_visited_pages, most_visited_items)) {
     return false;
   }
@@ skipping 408 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);
 }