Jumplist Bug

chrome/browser/jumplist_win.h

 // Copyright (c) 2011 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.
 
 #ifndef CHROME_BROWSER_JUMPLIST_WIN_H_
 #define CHROME_BROWSER_JUMPLIST_WIN_H_
 #pragma once
 
 #include <list>
 #include <string>
 #include <utility>
 #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/history/top_sites.h"

[MAD, 2011/08/15 16:16:16]

Do we need this include here?

[MAD, 2011/08/15 21:52:42]

Here...

 #include "chrome/browser/sessions/tab_restore_service.h"
 #include "chrome/browser/sessions/tab_restore_service_observer.h"
+#include "chrome/browser/ui/webui/ntp/base_most_visited_handler.h"

[MAD, 2011/08/15 16:16:16]

Please remove deprecated most visited base class inclusion and mentions in
comments...

[MAD, 2011/08/15 21:52:42]

Here...

 #include "content/browser/cancelable_request.h"
 
 class FilePath;
 class Profile;
 class PageUsageData;
 
 // Represents a class used for creating an IShellLink object by the utility
 // functions in this file.
 // This class consists of three strings and a integer.
 // * arguments (std::wstring)
@@ skipping 61 matching lines @@
 // JumpList:
 // * Retrieving "Most Visited" pages from HistoryService;
 // * Retrieving strings from the application resource;
 // * Creatng COM objects used by JumpList from PageUsageData objects;
 // * Adding COM objects to JumpList, etc.
 //
 // This class also implements TabRestoreServiceObserver. So, once we call
 // AddObserver() and register this class as an observer, it automatically
 // updates a JumpList when a tab is added or removed.
 //
+// This class also implements BaseMostVisitedHandler. So by calling
+// AddObserver() we also register for TOP_SITES_CHANGED notifications, and the
+// JumpList is updated when a top site is added or blacklisted.

[MAD, 2011/08/15 16:16:16]

I think this whole paragraph can go away...

[MAD, 2011/08/15 21:52:42]

Here...

+//
 // 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
 // task when it actually updates a JumpList. (This task is implemented in an

[MAD, 2011/08/15 16:16:16]

Please update comment since we now use a runnable method instead of a separate
task class.

[MAD, 2011/08/15 21:52:42]

Here...

 // anomynous namespace in "jumplist_win.cc".)
-class JumpList : public TabRestoreServiceObserver {
+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.
   bool AddObserver(Profile* profile);
   void RemoveObserver();
 
   // Observer callback for TabRestoreService::Observer to notify when a tab is
   // added or removed.
   // This function sends a query that retrieves "Most Visited" pages to
   // HistoryService. When the query finishes successfully, HistoryService call
@@ skipping 10 matching lines @@
   // * Chromium is lauched without a "--disable-custom-jumplist" option.
   // TODO(hbono): to be enabled by default when we finalize the categories and
   // items of our JumpList.
   static bool Enabled();
 
  protected:
   // Creates a ShellLinkItem object from a tab (or a window) and add it to the
   // 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,
-              size_t max_items);
-  bool AddWindow(const TabRestoreService::Window* window,
-                 ShellLinkItemList* list,
-                 size_t max_items);
+  bool AddTab(const TabRestoreService::Tab* tab, size_t max_items);
+  bool AddWindow(const TabRestoreService::Window* window, size_t max_items);
 
   // Starts loading a favicon for each URL in |icon_urls_|.
   // This function just sends a query to HistoryService.
   bool StartLoadingFavicon();
 
   // A callback function for HistoryService that notify 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 OnSegmentUsageAvailable(CancelableRequestProvider::Handle handle,
                                std::vector<PageUsageData*>* data);
 
   // 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.
   // When finishing loading all favicons, this function posts a task that
   // decompresses collected favicons and updates a JumpList.
   void OnFaviconDataAvailable(HistoryService::Handle handle,
                               history::FaviconData favicon);
 
+  // Callback for TopSites. BaseMostVisitedHandler that notifies when the "Most

[MAD, 2011/08/15 16:16:16]

Please update comment.

[MAD, 2011/08/15 21:52:42]

Here...

+  // 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);
+
+  // Runnable method that updates the jumplist, once have all the data
+  // has been fetched.
+  void JumpList::RunUpdate();
+
+  // Helper method for RunUpdate to decode the data about the asynchrounously
+  // loaded icons
+  void DecodeIconData(const ShellLinkItemList& item_list);
+
+  // Send a request to the HistoryService to get the most visited pages.
+  void StartQueryForTopSites();
+
  private:
   // Our consumers for HistoryService.
   CancelableRequestConsumer most_visited_consumer_;
   CancelableRequestConsumer favicon_consumer_;
+  CancelableRequestConsumer topsites_consumer_;
 
-  // The Profile object used for listening its events.
   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.
   ShellLinkItemList most_visited_pages_;
 
   // Items in the "Recently Closed" category of the application JumpList.
   ShellLinkItemList recently_closed_pages_;
 
   // A list of URLs we need to retrieve their favicons.
   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.

[MAD, 2011/08/15 16:16:16]

Please mention in comments above these data members that they are (and must
stay) protected by a the list_lock_

[MAD, 2011/08/15 21:52:42]

Here...

+  base::Lock list_lock_;
 };
 
 #endif  // CHROME_BROWSER_JUMPLIST_WIN_H_