Jumplist Bug

chrome/browser/jumplist_win.h

Index: chrome/browser/jumplist_win.h
===================================================================
--- chrome/browser/jumplist_win.h	(revision 96768)
+++ chrome/browser/jumplist_win.h	(working copy)
@@ -12,7 +12,9 @@
 #include <vector>
 
 #include "base/memory/ref_counted.h"
+#include "base/synchronization/lock.h"
 #include "chrome/browser/history/history.h"
+#include "chrome/browser/history/history_types.h"
 #include "chrome/browser/sessions/tab_restore_service.h"
 #include "chrome/browser/sessions/tab_restore_service_observer.h"
 #include "content/browser/cancelable_request.h"
@@ -98,13 +100,18 @@
 //
 // Updating a JumpList requires some file operations and it is not good to
 // update it in a UI thread. To solve this problem, this class posts a

[MAD, 2011/08/19 14:46:03]

posts TO a runnable...

-// task when it actually updates a JumpList. (This task is implemented in an
-// anomynous namespace in "jumplist_win.cc".)
-class JumpList : public TabRestoreServiceObserver {
+// runnable method when it actually updates a JumpList.
+class JumpList : public TabRestoreServiceObserver,
+                 public NotificationObserver,
+                 public base::RefCountedThreadSafe<JumpList> {
  public:
   JumpList();
-  ~JumpList();
 
+  // NotificationObserver implementation.
+  virtual void Observe(int type,
+                       const NotificationSource& source,
+                       const NotificationDetails& details);
+
   // Registers (or unregisters) this object as an observer.
   // When the TabRestoreService object notifies the tab status is changed, this
   // class automatically updates an application JumpList.
@@ -122,6 +129,14 @@
   // is destroyed.
   virtual void TabRestoreServiceDestroyed(TabRestoreService* service);
 
+  // Cancel a pending jumplist update.
+  void CancelPendingUpdate();
+
+  // Terminate the jumplist: cancel any pending updates and remove observer
+  // from TabRestoreService. This must be called before the profile provided
+  // in the AddObserver method is destroyed.
+  void Terminate();
+
   // Returns true if the custom JumpList is enabled.
   // We use the custom JumpList when we satisfy the following conditions:
   // * Chromium is running on Windows 7 and;
@@ -135,12 +150,10 @@
   // given list.
   // These functions are copied from the RecentlyClosedTabsHandler class for
   // compatibility with the new-tab page.
-  bool AddTab(const TabRestoreService::Tab* tab,
-              ShellLinkItemList* list,
+  bool AddTab(const TabRestoreService::Tab* tab, ShellLinkItemList* list,

[MAD, 2011/08/19 14:46:03]

Maybe we could bring those back to 3 lines to minimize the diff...

               size_t max_items);
   bool AddWindow(const TabRestoreService::Window* window,
-                 ShellLinkItemList* list,
-                 size_t max_items);
+                 ShellLinkItemList* list, size_t max_items);
 
   // Starts loading a favicon for each URL in |icon_urls_|.
   // This function just sends a query to HistoryService.
@@ -162,29 +175,60 @@
   void OnFaviconDataAvailable(HistoryService::Handle handle,
                               history::FaviconData favicon);
 
+  // Callback for TopSites that notifies when the "Most
+  // Visited" list is available. This function updates the ShellLinkItemList
+  // objects and send another query that retrieves a favicon for each URL in
+  // the list.
+  void OnMostVisitedURLsAvailable(
+                          const history::MostVisitedURLList& data);

[MAD, 2011/08/19 14:46:03]

Bad alignment, should only be 4 more indent space from the beginning of 'void'

+
+  // Runnable method that updates the jumplist, once have all the data

[MAD, 2011/08/19 14:46:03]

once all the data... (remove the word have)

+  // has been fetched.
+  void RunUpdate();
+
+  // Helper method for RunUpdate to decode the data about the asynchrounously
+  // loaded icons.
+  void DecodeIconData(const ShellLinkItemList& item_list);
+
  private:
+  friend class base::RefCountedThreadSafe<JumpList>;
+  ~JumpList();
+
   // Our consumers for HistoryService.
   CancelableRequestConsumer most_visited_consumer_;
   CancelableRequestConsumer favicon_consumer_;
+  CancelableRequestConsumer topsites_consumer_;
 
-  // The Profile object used for listening its events.

[MAD, 2011/08/19 14:46:03]

Bring back this comment.

   Profile* profile_;
 
+  NotificationRegistrar registrar_;
+
   // App id to associate with the jump list.
   std::wstring app_id_;
 
   // The directory which contains JumpList icons.
   FilePath icon_dir_;
 
-  // Items in the "Most Visited" category of the application JumpList.
+  // 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.
+  // Items in the "Recently Closed" category of the application JumpList,
+  // protected by the list_lock_.
   ShellLinkItemList recently_closed_pages_;
 
-  // A list of URLs we need to retrieve their favicons.
+  // 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_;
+
+  // Handle of last favicon request used to cancel if a new request
+  // comes in before the current one returns.
+  FaviconService::Handle handle_;
+
+  // Lock for most_visited_pages_, recently_closed_pages_, icon_urls_
+  // as they may be used by up to 3 threads.
+  base::Lock list_lock_;
 };
 
 #endif  // CHROME_BROWSER_JUMPLIST_WIN_H_