Time out jumplist update for very slow or busy OS

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"
 #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/single_thread_task_runner.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task_scheduler/post_task.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_restrictions.h"
+#include "base/timer/elapsed_timer.h"
 #include "base/trace_event/trace_event.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/favicon/favicon_service_factory.h"
 #include "chrome/browser/history/top_sites_factory.h"
 #include "chrome/browser/metrics/jumplist_metrics_win.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sessions/tab_restore_service_factory.h"
 #include "chrome/browser/shell_integration_win.h"
 #include "chrome/browser/win/jumplist_file_util.h"
 #include "chrome/common/chrome_constants.h"
@@ skipping 24 matching lines @@
 #include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/image/image_skia_rep.h"
 #include "url/gurl.h"
 
 using content::BrowserThread;
 using JumpListData = JumpList::JumpListData;
 
 namespace {
 
 // Delay jumplist updates to allow collapsing of redundant update requests.
-const int kDelayForJumplistUpdateInMS = 3500;
+constexpr base::TimeDelta kDelayForJumplistUpdateInMS =

[grt (UTC plus 2), 2017/04/21 10:11:25]

remove "InMS" since this is a TimeDelta (as i proposed previously). same below.

[chengx, 2017/04/21 18:51:20]

Done.

+    base::TimeDelta::FromMilliseconds(3500);
+
+// Timeout jumplist updates for users with extremely slow OS.
+constexpr base::TimeDelta kTimeOutForJumplistUpdateInMS =
+    base::TimeDelta::FromMilliseconds(500);
 
 // Append the common switches to each shell link.
 void AppendCommonSwitches(ShellLinkItem* shell_link) {
   const char* kSwitchNames[] = { switches::kUserDataDir };
   const base::CommandLine& command_line =
       *base::CommandLine::ForCurrentProcess();
   shell_link->GetCommandLine()->CopySwitchesFrom(command_line,
                                                  kSwitchNames,
                                                  arraysize(kSwitchNames));
 }
@@ skipping 91 matching lines @@
     base::ReplaceSubstringsAfterOffset(
         &incognito_title, 0, L"&", base::StringPiece16());
     incognito->set_title(incognito_title);
     incognito->set_icon(chrome_path.value(), icon_resources::kIncognitoIndex);
     items.push_back(incognito);
   }
 
   return jumplist_updater->AddTasks(items);
 }
 
 // Updates the application JumpList.

[grt (UTC plus 2), 2017/04/21 10:11:25]

please document that any error along the way results in the old jumplist being
left as-is, but without icon files. (assuming that's correct.)

[chengx, 2017/04/21 18:51:20]

Documentation added.

 bool UpdateJumpList(const wchar_t* app_id,

[grt (UTC plus 2), 2017/04/21 10:11:25]

this return value is undocumented and ignored. how about making this function
return void? otherwise, does it make sense to UMA the result?

[chengx, 2017/04/21 18:51:20]

I've updated it to return void.

                     const base::FilePath& icon_dir,
                     const ShellLinkItemList& most_visited_pages,
                     const ShellLinkItemList& recently_closed_pages,
                     IncognitoModePrefs::Availability incognito_availability) {
   // JumpList is implemented only on Windows 7 or later.

[grt (UTC plus 2), 2017/04/21 10:11:25]

This comment is out-of-date -- Chrome doesn't run on anything lower than Win7.
Please update these comments (and JumpListUpdater::IsEnabled) so that they state
the right thing. To be honest, this comment can simply be removed; "if
(!IsEnabled()) return;" is self-documenting. feel free to do this in a separate
cl.

[chengx, 2017/04/21 18:51:20]

Comments removed.

   // So, we should return now when this function is called on earlier versions
   // of Windows.
   if (!JumpListUpdater::IsEnabled())
     return true;
 
+  // Records the time cost of starting a JumpListUpdater.
+  base::ElapsedTimer begin_update_timer;
+
   JumpListUpdater jumplist_updater(app_id);
   if (!jumplist_updater.BeginUpdate())
     return false;
 
+  // Stops jumplist update if JumpListUpdater's start times out, as it's very
+  // likely the following update steps will also take a long time.
+  if (begin_update_timer.Elapsed() >= kTimeOutForJumplistUpdateInMS)
+    return false;
+
   // We allocate 60% of the given JumpList slots to "most-visited" items
   // and 40% to "recently-closed" items, respectively.
   // Nevertheless, if there are not so many items in |recently_closed_pages|,
   // we give the remaining slots to "most-visited" items.
   // The default maximum number of items to display in JumpList is 10.
   // https://msdn.microsoft.com/en-us/library/windows/desktop/dd378398(v=vs.85).aspx
   const int kMostVisited = 60;
   const int kRecentlyClosed = 40;
   const int kTotal = kMostVisited + kRecentlyClosed;
   size_t most_visited_items =
       MulDiv(jumplist_updater.user_max_items(), kMostVisited, kTotal);
   size_t recently_closed_items =
       jumplist_updater.user_max_items() - 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();
   }
 
   // Delete the content in JumpListIcons folder and log the results to UMA.
   DeleteDirectoryContentAndLogResults(icon_dir, kFileDeleteLimit);
 
   // If JumpListIcons directory doesn't exist (we have tried to create it
   // already) 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) && base::IsDirectoryEmpty(icon_dir)) {
-    // TODO(chengx): Remove this UMA metric after fixing http://crbug.com/40407.
-    UMA_HISTOGRAM_COUNTS_100(
-        "WinJumplist.CreateIconFilesCount",
-        most_visited_pages.size() + recently_closed_pages.size());
+  bool is_dir_existed_and_empty =

[grt (UTC plus 2), 2017/04/21 10:11:25]

is_dir_existed_and_empty -> should_create_icons

[chengx, 2017/04/21 18:51:20]

Done.

+      base::DirectoryExists(icon_dir) && base::IsDirectoryEmpty(icon_dir);
 
-    // Create icon files for shortcuts in the "Most Visited" category.
+  // Create icon files for shortcuts in the "Most Visited" category.
+  if (is_dir_existed_and_empty)
     CreateIconFiles(icon_dir, most_visited_pages, most_visited_items);
 
-    // Create icon files for shortcuts in the "Recently Closed" category.
-    CreateIconFiles(icon_dir, recently_closed_pages, recently_closed_items);
-  }
-
-  // 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;
   }
 
+  // Create icon files for shortcuts in the "Recently Closed" category.
+  if (is_dir_existed_and_empty)
+    CreateIconFiles(icon_dir, recently_closed_pages, recently_closed_items);
+
   // Update the "Recently Closed" category of the JumpList.
   if (!jumplist_updater.AddCustomCategory(
           l10n_util::GetStringUTF16(IDS_RECENTLY_CLOSED),
           recently_closed_pages, recently_closed_items)) {
     return false;
   }
 
   // Update the "Tasks" category of the JumpList.
   if (!UpdateTaskCategory(&jumplist_updater, incognito_availability))
     return false;
@@ skipping 332 matching lines @@
   DCHECK(CalledOnValidThread());
 
   TRACE_EVENT0("browser", "JumpList::PostRunUpdate");
   // Initialize the one-shot timer to update the jumplists in a while.
   // If there is already a request queued then cancel it and post the new
   // request. This ensures that JumpListUpdates won't happen until there has
   // been a brief quiet period, thus avoiding update storms.
   if (timer_.IsRunning()) {
     timer_.Reset();
   } else {
-    timer_.Start(FROM_HERE,
-                 base::TimeDelta::FromMilliseconds(kDelayForJumplistUpdateInMS),
-                 this,
+    timer_.Start(FROM_HERE, kDelayForJumplistUpdateInMS, this,
                  &JumpList::DeferredRunUpdate);
   }
 }
 
 void JumpList::DeferredRunUpdate() {
   DCHECK(CalledOnValidThread());
 
   TRACE_EVENT0("browser", "JumpList::DeferredRunUpdate");
   // Check if incognito windows (or normal windows) are disabled by policy.
   IncognitoModePrefs::Availability incognito_availability =
@@ skipping 18 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);
 }