Cancel coming updates when certain JumpList APIs time out to fail

chrome/browser/win/jumplist.cc

Index: chrome/browser/win/jumplist.cc
diff --git a/chrome/browser/win/jumplist.cc b/chrome/browser/win/jumplist.cc
index 7ebb5eab0553e7be09175a4f82c941bac03e2de6..32e386273a978739794aa95126d15ee9b9df834a 100644
--- a/chrome/browser/win/jumplist.cc
+++ b/chrome/browser/win/jumplist.cc
@@ -76,10 +76,10 @@ constexpr base::TimeDelta kDelayForJumplistUpdate =
 constexpr base::TimeDelta kTimeOutForJumplistBeginUpdate =
     base::TimeDelta::FromMilliseconds(500);
 
-// The maximum allowed time for updating both "most visited" and "recently
-// closed" categories via JumpListUpdater::AddCustomCategory.
+// The maximum allowed time for updating the category via
+// JumpListUpdater::AddCustomCategory.
 constexpr base::TimeDelta kTimeOutForAddCustomCategory =

[grt (UTC plus 2), 2017/06/30 21:34:12]

this is now specifically "timeout for add most visited pages custom category"
since recently closed is no longer timed. please make this more explicit in the
documentation here.

why does it make sense to so accurately time the most visited category but not
the recently closed category? it makes sense that the first category added may
take longer than the second since the code and data may be "hot" for the second
category. if that's the reason for only timing one of the two, then i think it's
worth documenting that. otherwise, what's the advantage of timing only one
rather than both?

[chengx, 2017/06/30 23:18:05]

I've updated the documentation. 
Here is the reason. 
1. If processing the first category times out or fails, there is no need to
process the second category. In this case, we are not able to time both
categories. Then we need to select one category. 
2. The most visited category is selected because it always has 5 items, while
the number of items in recently closed category varies from 1 to 3. This means
the runtime of AddCustomCategory API should be fixed for most visited category,
but not for recently closed category. So a fixed timeout threshold is actually
only valid for most visited category. That's why it makes sense to only time the
most visited category, but not the other one. 
I've added the explanation above to the file.

-    base::TimeDelta::FromMilliseconds(500);
+    base::TimeDelta::FromMilliseconds(300);
 
 // The maximum allowed time for JumpListUpdater::CommitUpdate.
 constexpr base::TimeDelta kTimeOutForJumplistCommitUpdate =
@@ -120,9 +120,10 @@ bool CreateIconFile(const gfx::ImageSkia& image_skia,
     std::vector<float> supported_scales = image_skia.GetSupportedScales();
     for (auto& scale : supported_scales) {
       gfx::ImageSkiaRep image_skia_rep = image_skia.GetRepresentation(scale);
-      if (!image_skia_rep.is_null())
+      if (!image_skia_rep.is_null()) {
         image_family.Add(
             gfx::Image::CreateFrom1xBitmap(image_skia_rep.sk_bitmap()));
+      }
     }
   }
 
@@ -704,17 +705,19 @@ void JumpList::CreateNewJumpListAndNotifyOS(
 
   base::ElapsedTimer begin_update_timer;
 
-  if (!jumplist_updater.BeginUpdate())
-    return;
+  bool begin_success = jumplist_updater.BeginUpdate();
 
-  // Discard this JumpList update if JumpListUpdater::BeginUpdate takes longer
-  // than the maximum allowed time, as it's very likely the following update
-  // steps will also take a long time.
+  // If JumpListUpdater::BeginUpdate takes longer than the maximum allowed time,
+  // abort the current update as it's very likely the following steps will also
+  // take a long time, and skip the next |kUpdatesToSkipUnderHeavyLoad| updates.
   if (begin_update_timer.Elapsed() >= kTimeOutForJumplistBeginUpdate) {
     update_transaction->update_timeout = true;
     return;
   }
 
+  if (!begin_success)
+    return;
+
   // Record the desired number of icons created in this JumpList update.
   int icons_created = 0;
 
@@ -747,12 +750,20 @@ void JumpList::CreateNewJumpListAndNotifyOS(
   // 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, kMostVisitedItems)) {
+  bool add_category_success = jumplist_updater.AddCustomCategory(
+      l10n_util::GetStringUTF16(IDS_NEW_TAB_MOST_VISITED), most_visited_pages,
+      kMostVisitedItems);
+
+  // If AddCustomCategory takes longer than the maximum allowed time, abort the
+  // current update and skip the next |kUpdatesToSkipUnderHeavyLoad| updates.
+  if (add_custom_category_timer.Elapsed() >= kTimeOutForAddCustomCategory) {
+    update_transaction->update_timeout = true;
     return;
   }
 
+  if (!add_category_success)
+    return;
+
   // Update the "Recently Closed" category of the JumpList.
   if (!jumplist_updater.AddCustomCategory(
           l10n_util::GetStringUTF16(IDS_RECENTLY_CLOSED), recently_closed_pages,
@@ -760,13 +771,6 @@ void JumpList::CreateNewJumpListAndNotifyOS(
     return;
   }
 
-  // If AddCustomCategory takes longer than the maximum allowed time, abort the
-  // current update and skip the next |kUpdatesToSkipUnderHeavyLoad| updates.
-  if (add_custom_category_timer.Elapsed() >= kTimeOutForAddCustomCategory) {
-    update_transaction->update_timeout = true;
-    return;
-  }
-
   // Update the "Tasks" category of the JumpList.
   if (!UpdateTaskCategory(&jumplist_updater, incognito_availability))
     return;