Fix stability and data racing issues, coalesce more updates for JumpList

chrome/browser/win/jumplist.h

Index: chrome/browser/win/jumplist.h
diff --git a/chrome/browser/win/jumplist.h b/chrome/browser/win/jumplist.h
index 42473cf41ac391eef923b3256415a778ea212294..21c800d252e8837e2c3778a44e7994382da54d5a 100644
--- a/chrome/browser/win/jumplist.h
+++ b/chrome/browser/win/jumplist.h
@@ -67,38 +67,31 @@ class JumpList : public sessions::TabRestoreServiceObserver,
                  public history::TopSitesObserver,
                  public RefcountedKeyedService {

[grt (UTC plus 2), 2017/06/09 10:42:58]

can this be a regular KeyedService? i don't see any evidence of refcounting
being used.

[chengx, 2017/06/10 05:56:18]

I think refcounting is used when one user (i.e., one profile) has multiple
Chrome windows (each window may contain lots of tabs) open. Please see
crrev/2323603002 where refcounting was introduced.

[grt (UTC plus 2), 2017/06/12 07:15:32]

Making it a keyed service accomplishes that (one instance per Profile rather
than one per browser window). I don't see any reason for it to be refcounted.

[chengx, 2017/06/12 22:05:28]

I've made it not refcounted.

  public:
-  struct JumpListData {
-    JumpListData();
-    ~JumpListData();
-
-    // Lock for most_visited_pages_, recently_closed_pages_, icon_urls_
-    // as they may be used by up to 2 threads.
-    base::Lock list_lock_;
-
-    // A list of URLs we need to retrieve their favicons,
-    // protected by the list_lock_.
-    typedef std::pair<std::string, scoped_refptr<ShellLinkItem> > URLPair;
-    std::list<URLPair> icon_urls_;
-
-    // Items in the "Most Visited" category of the application JumpList,
-    // protected by the list_lock_.
-    ShellLinkItemList most_visited_pages_;
-
-    // Items in the "Recently Closed" category of the application JumpList,
-    // protected by the list_lock_.
-    ShellLinkItemList recently_closed_pages_;
-
-    // A boolean flag indicating if "Most Visited" category of the JumpList
-    // has new updates therefore its icons need to be updated.
-    // By default, this flag is set to false. If there's any change in
-    // TabRestoreService, this flag will be set to true.
-    bool most_visited_pages_have_updates_ = false;
-
-    // A boolean flag indicating if "Recently Closed" category of the JumpList
-    // has new updates therefore its icons need to be updated.
-    // By default, this flag is set to false. If there's any change in TopSites
-    // service, this flag will be set to true.
-    bool recently_closed_pages_have_updates_ = false;
+  using URLIconCache = base::flat_map<std::string, base::FilePath>;
+
+  // Holds results of the RunUpdateJumpList run.
+  struct JumpListUpdateResults {

[grt (UTC plus 2), 2017/06/09 10:42:58]

nit: just UpdateResults since this is within JumpList. also, can this type be
private?

[chengx, 2017/06/10 05:56:18]

Done. Renamed to UpdateResults and made private.

+    JumpListUpdateResults();
+    ~JumpListUpdateResults();
+
+    // Icon file paths of the most visited links, indexed by tab url.
+    // Holding a copy of most_visited_icons_ initially, it's updated by the
+    // JumpList update run. If the update run succeeds, it overwrites
+    // most_visited_icons_.
+    URLIconCache most_visited_icons_in_update_;
+
+    // icon file paths of the recently closed links, indexed by tab url.
+    // Holding a copy of recently_closed_icons_ initially, it's updated by the
+    // JumpList update run. If the update run succeeds, it overwrites
+    // recently_closed_icons_.
+    URLIconCache recently_closed_icons_in_update;
+
+    // A flag indicating if a JumpList update run is successful.
+    bool update_success_ = true;

[grt (UTC plus 2), 2017/06/09 10:42:58]

initialize to false and set it to true in the one success case rather than
initializing to true and setting to false in the N failure cases.

[chengx, 2017/06/10 05:56:18]

Done.

+
+    // A flag indicating if there is a timeout in notifying the JumpList update
+    // to shell. Note that this variable is independent of update_success_.
+    bool update_timeout_ = false;
   };
 
   // Observer callback for TabRestoreService::Observer to notify when a tab is

[grt (UTC plus 2), 2017/06/09 10:42:57]

nit: remove the comments on these two overrides. the only comment needed is:
  // sessions::TabRestoreServiceObserver:
just above the two of them.

please use the same notation for the other overrides as well. thanks.

[grt (UTC plus 2), 2017/06/09 10:42:58]

nit: move these overrides down into the private: section just before the
TopSitesObserver overrides. the inconsistency here has always bugged me. :-)

[chengx, 2017/06/10 05:56:18]

Done with moving the TopSitesObserver overrides to the private section down
below.

[chengx, 2017/06/10 05:56:19]

Done. Comments updated.

@@ -110,10 +103,10 @@ class JumpList : public sessions::TabRestoreServiceObserver,
   void TabRestoreServiceDestroyed(
       sessions::TabRestoreService* service) override;
 
-  // Cancel a pending jumplist update.
+  // Cancel a pending JumpList update.
   void CancelPendingUpdate();

[grt (UTC plus 2), 2017/06/09 10:42:58]

this should be private:

[chengx, 2017/06/10 05:56:18]

Done.

 
-  // Terminate the jumplist: cancel any pending updates and stop observing
+  // Terminate the JumpList: cancel any pending updates and stop observing
   // the Profile and its services. This must be called before the |profile_|
   // is destroyed.
   void Terminate();

[grt (UTC plus 2), 2017/06/09 10:42:57]

private as well

[chengx, 2017/06/10 05:56:18]

Done.

@@ -129,19 +122,14 @@ class JumpList : public sessions::TabRestoreServiceObserver,
   explicit JumpList(Profile* profile);  // Use JumpListFactory instead
   ~JumpList() override;
 
-  enum class JumpListCategory { kMostVisited, kRecentlyClosed };
+  // Adds a new ShellLinkItem for |tab| to the JumpList data provided that doing
+  // so will not exceed |max_items|.
+  bool AddTab(const sessions::TabRestoreService::Tab& tab, size_t max_items);
 
-  // Adds a new ShellLinkItem for |tab| to |data| provided that doing so will
-  // not exceed |max_items|.
-  bool AddTab(const sessions::TabRestoreService::Tab& tab,
-              size_t max_items,
-              JumpListData* data);
-
-  // Adds a new ShellLinkItem for each tab in |window| to |data| provided that
-  // doing so will not exceed |max_items|.
+  // Adds a new ShellLinkItem for each tab in |window| to the JumpList data
+  // provided that doing so will not exceed |max_items|.
   void AddWindow(const sessions::TabRestoreService::Window& window,
-                 size_t max_items,
-                 JumpListData* data);
+                 size_t max_items);
 
   // Starts loading a favicon for each URL in |icon_urls_|.
   // This function sends a query to HistoryService.
@@ -149,10 +137,9 @@ class JumpList : public sessions::TabRestoreServiceObserver,
   // decompresses collected favicons and updates a JumpList.
   void StartLoadingFavicon();
 
-  // A callback function for HistoryService that notify when a requested favicon
-  // is available.
-  // To avoid file operations, this function just attaches the given data to
-  // a ShellLinkItem object.
+  // Callback for HistoryService that notifies when a requested favicon is
+  // available. To avoid file operations, this function just attaches the given
+  // data to a ShellLinkItem object.
   void OnFaviconDataAvailable(
       const favicon_base::FaviconImageResult& image_result);
 
@@ -174,56 +161,58 @@ class JumpList : public sessions::TabRestoreServiceObserver,
   void TopSitesChanged(history::TopSites* top_sites,
                        ChangeReason change_reason) override;
 
-  // Called on a timer to update the most visited URLs after requests storms
-  // have subsided.
+  // Called on a timer after requests storms have subsided. Calls APIs
+  // DeferredTop{SitesChanged, ServiceChanged} on demand to do the actual work.
+  void DeferredChanged();

[grt (UTC plus 2), 2017/06/09 10:42:58]

nit: i think this should either be named after when it is called (e.g.,
OnDelayTimer) or for what it does (e.g., HandlePendingNotifications).

[chengx, 2017/06/10 05:56:18]

I'll go with OnDelayTimer.

+
+  // Update the most visited URLs, called in DeferredChanged().

[grt (UTC plus 2), 2017/06/09 10:42:58]

i don't think it's necessary to document who calls the function. it doesn't
really matter. if the function has only one job, then maybe it's suitable to
call it from multiple places some day.

[chengx, 2017/06/10 05:56:18]

I've updated the comments.

   void DeferredTopSitesChanged();

[grt (UTC plus 2), 2017/06/09 10:42:58]

since the "deferred" part is now taken care of by the method above, this could
now aptly be named ProcessTopSitesNotification, or something like that.

[chengx, 2017/06/10 05:56:18]

Renamed to ProcessTopSitesNotification

 
-  // Called on a timer to update the "Recently Closed" category of JumpList
-  // after requests storms have subsided.
+  // Update the recently closed URLs, called in DeferredChanged().
   void DeferredTabRestoreServiceChanged();

[grt (UTC plus 2), 2017/06/09 10:42:58]

ProcessTabRestoreServiceNotification?

[chengx, 2017/06/10 05:56:18]

SGTM. I've renamed the variable.

 
-  // Deletes icon files of |category| in |icon_dir| which are not in the cache
-  // anymore.
-  void DeleteIconFiles(const base::FilePath& icon_dir,
-                       JumpListCategory category);
+  // Deletes icon files in |icon_dir| which are not in |icon_cache| anymore.
+  static void DeleteIconFiles(const base::FilePath& icon_dir,
+                              URLIconCache* icon_cache);
 
-  // Creates at most |max_items| icon files of |category| in |icon_dir| for the
-  // asynchrounously loaded icons stored in |item_list|.
+  // In |icon_dir|, creates at most |max_items| icon files which are not in
+  // |icon_cache| for the asynchrounously loaded icons stored in |item_list|.
+  // |icon_cache| is also updated for newly created icons.
   // Returns the number of new icon files created.
-  int CreateIconFiles(const base::FilePath& icon_dir,
-                      const ShellLinkItemList& item_list,
-                      size_t max_items,
-                      JumpListCategory category);
-
-  // Updates icon files in |icon_dir|, which includes deleting old icons and
-  // creating at most |slot_limit| new icons for |page_list|.
+  static int CreateIconFiles(const base::FilePath& icon_dir,
+                             const ShellLinkItemList& item_list,
+                             size_t max_items,
+                             URLIconCache* icon_cache);
+
+  // Updates icon files for |page_list| in |icon_dir|, which consists of
+  // 1) creating at most |slot_limit| new icons which are not in |icon_cache|;
+  // 2) deleting old icons which are not in |icon_cache|.
   // Returns the number of new icon files created.
-  int UpdateIconFiles(const base::FilePath& icon_dir,
-                      const ShellLinkItemList& page_list,
-                      size_t slot_limit,
-                      JumpListCategory category);
-
-  // Updates the jumplist, once all the data has been fetched. This method calls
-  // UpdateJumpList() to do most of the work.
-  void RunUpdateJumpList(
-      IncognitoModePrefs::Availability incognito_availability,
+  static int UpdateIconFiles(const base::FilePath& icon_dir,
+                             const ShellLinkItemList& page_list,
+                             size_t slot_limit,
+                             URLIconCache* icon_cache);
+
+  // Updates the application JumpList, which consists of 1) create new icon
+  // files; 2) delete obsolete icon files; 3) notify the OS.
+  // Note that any timeout error along the way results in the old JumpList being
+  // left as-is, while any non-timeout error results in the old JumpList being
+  // left as-is, but without icon files.
+  static void RunUpdateJumpList(
       const base::string16& app_id,
       const base::FilePath& profile_dir,
-      base::RefCountedData<JumpListData>* ref_counted_data);
+      const ShellLinkItemList& most_visited_pages,
+      const ShellLinkItemList& recently_closed_pages,
+      bool most_visited_pages_have_updates,
+      bool recently_closed_pages_have_updates,
+      IncognitoModePrefs::Availability incognito_availability,
+      JumpListUpdateResults* update_results);
 
-  // Updates the application JumpList, which consists of 1) delete old icon
-  // files; 2) create new icon files; 3) notify the OS. This method is called
-  // from RunUpdateJumpList().
-  // Note that any timeout error along the way results in the old jumplist being
-  // left as-is, while any non-timeout error results in the old jumplist being
-  // left as-is, but without icon files.
-  bool UpdateJumpList(const base::string16& app_id,
-                      const base::FilePath& profile_dir,
-                      const ShellLinkItemList& most_visited_pages,
-                      const ShellLinkItemList& recently_closed_pages,
-                      bool most_visited_pages_have_updates,
-                      bool recently_closed_pages_have_updates,
-                      IncognitoModePrefs::Availability incognito_availability);
+  // Callback for RunUpdateJumpList that notifies when it finishes running.
+  // Updates certain JumpList member variables and/or triggers a new JumpList
+  // update based on |update_results|.
+  void OnRunUpdateCompletion(
+      std::unique_ptr<JumpListUpdateResults> update_results);
 
   // Tracks FaviconService tasks.
   base::CancelableTaskTracker cancelable_task_tracker_;
@@ -234,32 +223,50 @@ class JumpList : public sessions::TabRestoreServiceObserver,
   // Lives on the UI thread.

[grt (UTC plus 2), 2017/06/09 10:42:58]

all members live on the UI thread now, so this comment is no longer needed.

[chengx, 2017/06/10 05:56:18]

Done.

   std::unique_ptr<PrefChangeRegistrar> pref_change_registrar_;
 
-  // App id to associate with the jump list.
+  // App id to associate with the JumpList.
   base::string16 app_id_;
 
-  // Timer for requesting delayed updates of the "Most Visited" category of
-  // jumplist.
-  base::OneShotTimer timer_most_visited_;
+  // Timer for requesting delayed JumpList updates.
+  base::OneShotTimer timer_;
 
-  // Timer for requesting delayed updates of the "Recently Closed" category of
-  // jumplist.
-  base::OneShotTimer timer_recently_closed_;
+  // A list of URLs we need to retrieve their favicons,
+  typedef std::pair<std::string, scoped_refptr<ShellLinkItem>> URLPair;

[grt (UTC plus 2), 2017/06/09 10:42:57]

wdyt of:
  using UrlAndLinkItem = ...

[chengx, 2017/06/10 05:56:18]

Done.

+  std::list<URLPair> icon_urls_;
 
-  // Number of updates to skip to alleviate the machine when a previous update
-  // was too slow. Updates will be resumed when this reaches 0 again.
-  int updates_to_skip_ = 0;
+  // Items in the "Most Visited" category of the JumpList.
+  ShellLinkItemList most_visited_pages_;
+
+  // Items in the "Recently Closed" category of the JumpList.
+  ShellLinkItemList recently_closed_pages_;
+
+  // The icon file paths of the most visited links, indexed by tab url.
+  URLIconCache most_visited_icons_;
+
+  // The icon file paths of the recently closed links, indexed by tab url.
+  URLIconCache recently_closed_icons_;
+
+  // A flag indicating if "Most Visited" category has pending notifications.
+  bool most_visited_has_pending_notification_ = false;
+
+  // A flag indicating if "Recently Closed" category has pending notifications.
+  bool recently_closed_has_pending_notification_ = false;
+
+  // A flag indicating if "Most Visited" category has new updates.
+  bool most_visited_pages_have_updates_ = false;
+
+  // A flag indicating if "Recently Closed" category has new updates.
+  bool recently_closed_pages_have_updates_ = false;
+
+  // A boolean flag indicating if there's a JumpList update task already posted
+  // or currently running.
+  bool update_in_progress_ = false;
 
   // A boolean flag indicating if a session has at least one tab closed.
   bool has_tab_closed_ = false;
 
-  // Holds data that can be accessed from multiple threads.
-  scoped_refptr<base::RefCountedData<JumpListData>> jumplist_data_;
-
-  // The icon file paths of the most visited links and the recently closed links
-  // in the current jumplist, indexed by tab url, respectively.
-  // They may only be accessed on update_jumplist_task_runner_.
-  base::flat_map<std::string, base::FilePath> most_visited_icons_;
-  base::flat_map<std::string, base::FilePath> recently_closed_icons_;
+  // Number of updates to skip to alleviate the machine when a previous update
+  // was too slow. Updates will be resumed when this reaches 0 again.
+  int updates_to_skip_ = 0;
 
   // Id of last favicon task. It's used to cancel current task if a new one
   // comes in before it finishes.
@@ -274,7 +281,7 @@ class JumpList : public sessions::TabRestoreServiceObserver,
 
   SEQUENCE_CHECKER(sequence_checker_);
 
-  // For callbacks may be run after destruction.
+  // For callbacks may run after destruction.
   base::WeakPtrFactory<JumpList> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(JumpList);