Jumplist Bug

chrome/browser/jumplist_win.cc

 // 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.
 
 #include "chrome/browser/jumplist_win.h"
 
 #include <windows.h>
 #include <shobjidl.h>
 #include <propkey.h>
 #include <propvarutil.h>
 
 #include <string>
 #include <vector>
 
 #include "base/callback.h"
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/path_service.h"
 #include "base/string_util.h"
 #include "base/threading/thread.h"
 #include "base/utf_string_conversions.h"
 #include "base/win/scoped_comptr.h"
 #include "base/win/windows_version.h"
 #include "chrome/browser/favicon/favicon_service.h"
 #include "chrome/browser/history/history.h"
 #include "chrome/browser/history/page_usage_data.h"
+#include "chrome/browser/history/top_sites.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sessions/session_types.h"
 #include "chrome/browser/sessions/tab_restore_service.h"
 #include "chrome/browser/sessions/tab_restore_service_factory.h"
 #include "chrome/browser/shell_integration.h"
 #include "chrome/common/chrome_constants.h"
+#include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/url_constants.h"
 #include "content/browser/browser_thread.h"
+#include "content/common/notification_service.h"
 #include "googleurl/src/gurl.h"
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "ui/gfx/icon_util.h"
 
 
 namespace {
@@ skipping 423 matching lines @@
     return false;
 
   // Commit this transaction and send the updated JumpList to Windows.
   result = destination_list->CommitList();
   if (FAILED(result))
     return false;
 
   return true;
 }
 
-// Represents a task which updates an application JumpList.
-// This task encapsulates all I/O tasks and OS-specific tasks required for
-// updating a JumpList from Chromium, such as:
-// * Deleting the directory containing temporary icon files;
-// * Creating temporary icon files used by the JumpList;
-// * Creating an ICustomDestinationList instance;
-// * Adding items in the ICustomDestinationList instance.
-// To spawn this task,
-// 1. Prepare objects required by this task:
-//    * a std::wstring that contains a temporary icons;
-//    * a ShellLinkItemList that contains the items of the "Most Visited"
-//      category, and;
-//    * a ShellLinkItemList that contains the items of the "Recently Closed"
-//      category.
-// 2. Create a JumpListUpdateTask instance, and;
-// 3. Post this task to the file thread.
-class JumpListUpdateTask : public Task {
- public:
-  JumpListUpdateTask(const wchar_t* app_id,
-                     const FilePath& icon_dir,
-                     const ShellLinkItemList& most_visited_pages,
-                     const ShellLinkItemList& recently_closed_pages)
-    : app_id_(app_id),
-      icon_dir_(icon_dir),
-      most_visited_pages_(most_visited_pages),
-      recently_closed_pages_(recently_closed_pages) {
-  }
-
- private:
-  // Represents an entry point of this task.
-  // When we post this task to a file thread, the thread calls this function.
-  void Run();
-
-  // 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_;
-};
-
-void JumpListUpdateTask::Run() {
-  // Delete the directory which contains old icon files, rename the current
-  // icon directory, and create a new directory which contains new JumpList
-  // icon files.
-  FilePath icon_dir_old(icon_dir_.value() + L"Old");
-  if (file_util::PathExists(icon_dir_old))
-    file_util::Delete(icon_dir_old, true);
-  file_util::Move(icon_dir_, icon_dir_old);
-  file_util::CreateDirectory(icon_dir_);
-
-  // Create temporary icon files for shortcuts in the "Most Visited" category.
-  for (ShellLinkItemList::const_iterator item = most_visited_pages_.begin();
-       item != most_visited_pages_.end(); ++item) {
-    SkBitmap icon_bitmap;
-    if ((*item)->data().get() &&
-        gfx::PNGCodec::Decode((*item)->data()->front(),
-                              (*item)->data()->size(),
-                              &icon_bitmap)) {
-      FilePath icon_path;
-      if (CreateIconFile(icon_bitmap, icon_dir_, &icon_path))
-        (*item)->SetIcon(icon_path.value(), 0, true);
-    }
-  }
-
-  // Create temporary icon files for shortcuts in the "Recently Closed"
-  // category.
-  for (ShellLinkItemList::const_iterator item = recently_closed_pages_.begin();
-       item != recently_closed_pages_.end(); ++item) {
-    SkBitmap icon_bitmap;
-    if ((*item)->data().get() &&
-        gfx::PNGCodec::Decode((*item)->data()->front(),
-                              (*item)->data()->size(),
-                              &icon_bitmap)) {
-      FilePath icon_path;
-      if (CreateIconFile(icon_bitmap, icon_dir_, &icon_path))
-        (*item)->SetIcon(icon_path.value(), 0, true);
-    }
-  }
-
-  // We finished collecting all resources needed for updating an appliation
-  // JumpList. So, create a new JumpList and replace the current JumpList
-  // with it.
-  UpdateJumpList(app_id_.c_str(), most_visited_pages_, recently_closed_pages_);
-
-  // Delete all items in these lists now since we don't need the ShellLinkItem
-  // objects in these lists.
-  most_visited_pages_.clear();
-  recently_closed_pages_.clear();
-}
-
 }  // namespace
 
-JumpList::JumpList() : profile_(NULL) {

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

We should still set the profile to NULL here...

+JumpList::JumpList():handle_(NULL) {
 }
 
 JumpList::~JumpList() {
   RemoveObserver();
 }
 
 // static
 bool JumpList::Enabled() {
   return (base::win::GetVersion() >= base::win::VERSION_WIN7 &&
           !CommandLine::ForCurrentProcess()->HasSwitch(
@@ skipping 10 matching lines @@
     return false;
 
   TabRestoreService* tab_restore_service =
       TabRestoreServiceFactory::GetForProfile(profile);
   if (!tab_restore_service)
     return false;
 
   app_id_ = ShellIntegration::GetChromiumAppId(profile->GetPath());
   icon_dir_ = profile->GetPath().Append(chrome::kJumpListIconDirname);
   profile_ = profile;
+  history::TopSites* top_sites = profile_->GetTopSites();
+  if (top_sites) {
+    // TopSites updates itself after a delay. This is especially noticable when
+    // your profile is empty. Ask TopSites to update itself when we're about to
+    // show the new tab page.
+    top_sites->SyncWithHistory();
+    // Register for notification when TopSites changes so that we can update
+    // ourself.
+    registrar_.Add(this, chrome::NOTIFICATION_TOP_SITES_CHANGED,
+                   Source<history::TopSites>(top_sites));
+  }
   tab_restore_service->AddObserver(this);
   return true;
 }
 
+void JumpList::Observe(int type,
+                       const NotificationSource& source,
+                       const NotificationDetails& details) {
+  DCHECK_EQ(type, chrome::NOTIFICATION_TOP_SITES_CHANGED);
+
+  // Most visited urls changed, query again.
+  StartQueryForTopSites();
+}
+
 void JumpList::RemoveObserver() {
   if (profile_) {
     TabRestoreService* tab_restore_service =
         TabRestoreServiceFactory::GetForProfile(profile_);
     if (tab_restore_service)
       tab_restore_service->RemoveObserver(this);
   }
   profile_ = NULL;
 }
 
+void JumpList::StartQueryForTopSites() {

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

I think this code is small enough and unlikely to be reused, that we can
actually have it directly within the Observe implementation...

+  history::TopSites* top_sites = profile_->GetTopSites();
+  if (top_sites) {
+    top_sites->GetMostVisitedURLs(
+        &topsites_consumer_,
+        NewCallback(this, &JumpList::OnMostVisitedURLsAvailable));
+  }
+}
+
+void JumpList::OnMostVisitedURLsAvailable(
+    const history::MostVisitedURLList& data) {
+
+  // If we have a pending favicon request, cancel it here (it is out of date).
+  if (handle_) {
+    FaviconService* favicon_service =
+        profile_->GetFaviconService(Profile::EXPLICIT_ACCESS);
+    favicon_service->CancelRequest(handle_);
+    handle_ = NULL;
+  }
+
+  {
+    base::AutoLock auto_lock(list_lock_);
+    most_visited_pages_.clear();
+    for (size_t i = 0; i < data.size(); i++) {
+      const history::MostVisitedURL& url = data[i];
+      scoped_refptr<ShellLinkItem> link(new ShellLinkItem);
+      std::string url_string = url.url.spec();
+      link->SetArguments(UTF8ToWide(url_string));
+      link->SetTitle(!url.title.empty()? url.title : link->arguments());
+      most_visited_pages_.push_back(link);
+      icon_urls_.push_back(make_pair(url_string, link));
+    }
+  }
+
+  // Send a query that retrieves the first favicon.
+  StartLoadingFavicon();
+}
+
 void JumpList::TabRestoreServiceChanged(TabRestoreService* service) {
-  // Added or removed a tab.
-  // Exit if we are updating the application JumpList.
-  if (!icon_urls_.empty())
-    return;
+  // if we have a pending handle request, cancel it here (it is out of date).
+  if (handle_) {
+    FaviconService* favicon_service =

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

Maybe extract this common code to a method...

+        profile_->GetFaviconService(Profile::EXPLICIT_ACCESS);
+    favicon_service->CancelRequest(handle_);
+    handle_ = NULL;
+  }
 
-  // Send a query to HistoryService and retrieve the "Most Visited" pages.
-  // This code is copied from MostVisitedHandler::HandleGetMostVisited() to
-  // emulate its behaviors.
-  const int kMostVisitedScope = 90;
-  const int kMostVisitedCount = 9;
-  int result_count = kMostVisitedCount;
-  HistoryService* history_service =
-      profile_->GetHistoryService(Profile::EXPLICIT_ACCESS);
-  history_service->QuerySegmentUsageSince(
-      &most_visited_consumer_,
-      base::Time::Now() - base::TimeDelta::FromDays(kMostVisitedScope),
-      result_count,
-      NewCallback(this, &JumpList::OnSegmentUsageAvailable));
+  {
+    base::AutoLock auto_lock(list_lock_);
+    recently_closed_pages_.clear();

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

I'm a it uncomfortable with the isolated locks here, and also the full lock
within AddTab below (more details in there)...

I'm afraid we might get calls in between in the other thread when the list will
be in the process of being filled...

Maybe we should try to fill a local list instead (sorry, this would mean to
bring back the extra argument for the list that I asked you to remove, I should
have thought about this at first... :-( )

This way, we could reset the list in one shot, within a single lock/unlock
section... That would be safer I think... Unless I'm missing something...

+  }
+
+  // Create a list of ShellLinkItems from the "Recently Closed" pages.
+  // As noted above, we create a ShellLinkItem objects with the following
+  // parameters.
+  // * arguments
+  //   The last URL of the tab object.
+  // * title
+  //   The title of the last URL.
+  // * icon
+  //   An empty string. This value is to be updated in OnFaviconDataAvailable().
+  // This code is copied from
+  // RecentlyClosedTabsHandler::TabRestoreServiceChanged() to emulate it.
+  const int kRecentlyClosedCount = 4;
+  TabRestoreService* tab_restore_service =
+      TabRestoreServiceFactory::GetForProfile(profile_);
+  const TabRestoreService::Entries& entries = tab_restore_service->entries();
+  for (TabRestoreService::Entries::const_iterator it = entries.begin();
+       it != entries.end(); ++it) {
+    const TabRestoreService::Entry* entry = *it;
+    if (entry->type == TabRestoreService::TAB) {
+      AddTab(static_cast<const TabRestoreService::Tab*>(entry),
+             kRecentlyClosedCount);
+    } else if (entry->type == TabRestoreService::WINDOW) {
+      AddWindow(static_cast<const TabRestoreService::Window*>(entry),
+                kRecentlyClosedCount);
+    }
+  }
+
+  // Send a query that retrieves the first favicon.
+  StartLoadingFavicon();
 }
 
 void JumpList::TabRestoreServiceDestroyed(TabRestoreService* service) {
 }
 
-bool JumpList::AddTab(const TabRestoreService::Tab* tab,
-                      ShellLinkItemList* list,
-                      size_t max_items) {
+bool JumpList::AddTab(const TabRestoreService::Tab* tab, size_t max_items) {
   // This code adds the URL and the title strings of the given tab to the
   // specified list.
   // This code is copied from RecentlyClosedTabsHandler::TabToValue().
-  if (tab->navigations.empty() || list->size() >= max_items)
+  base::AutoLock auto_lock(list_lock_);
+  if (tab->navigations.empty() || recently_closed_pages_.size() >= max_items)

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

If we would keep this within a lock (which we might not based on a previous
comment above), we should add a comment that accessing tab->navigations is
currently safe to do within a lock, but if this changes to a method call on the
TabRestoreService object, it might become unsafe to do so...

     return false;
 
   const TabNavigation& current_navigation =
       tab->navigations.at(tab->current_navigation_index);
   if (current_navigation.virtual_url() == GURL(chrome::kChromeUINewTabURL))
     return false;
 
   scoped_refptr<ShellLinkItem> link(new ShellLinkItem);
   std::string url = current_navigation.virtual_url().spec();

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

Not sure if it is safe to call virtual_url on the TabNavigation object within a
lock... Most likely is sure... But... maybe not... Should at least have a
comment saying it if we keep the lock in this method...

   link->SetArguments(UTF8ToWide(url));
   link->SetTitle(current_navigation.title());
-  list->push_back(link);
+  recently_closed_pages_.push_back(link);
   icon_urls_.push_back(make_pair(url, link));
   return true;
 }
 
 bool JumpList::AddWindow(const TabRestoreService::Window* window,
-                         ShellLinkItemList* list,
                          size_t max_items) {
   // This code enumerates al the tabs in the given window object and add their
   // URLs and titles to the list.
   // This code is copied from RecentlyClosedTabsHandler::WindowToValue().
   if (window->tabs.empty()) {
     NOTREACHED();
     return false;
   }
   for (size_t i = 0; i < window->tabs.size(); ++i) {
-    if (!AddTab(&window->tabs[i], list, max_items))
+    if (!AddTab(&window->tabs[i], max_items))
       return false;
   }
 
   return true;
 }
 
 bool JumpList::StartLoadingFavicon() {
-  if (icon_urls_.empty())
-    return false;
+  {
+    base::AutoLock auto_lock(list_lock_);
+    if (icon_urls_.empty())
+      return false;
+  }
 
   // Ask FaviconService if it has a favicon of a URL.
   // When FaviconService has one, it will call OnFaviconDataAvailable().
-  GURL url(icon_urls_.front().first);
+  GURL url;
+  {
+    base::AutoLock auto_lock(list_lock_);
+    url = GURL(icon_urls_.front().first);
+  }
   FaviconService* favicon_service =
       profile_->GetFaviconService(Profile::EXPLICIT_ACCESS);
-  FaviconService::Handle handle = favicon_service->GetFaviconForURL(
+  handle_ = favicon_service->GetFaviconForURL(
       url, history::FAVICON, &favicon_consumer_,
       NewCallback(this, &JumpList::OnFaviconDataAvailable));
   return true;
 }
 
-void JumpList::OnSegmentUsageAvailable(
-    CancelableRequestProvider::Handle handle,
-    std::vector<PageUsageData*>* data) {
-  // Create a list of ShellLinkItem objects from the given list of
-  // PageUsageData objects.
-  // The command that opens a web page with chrome is:
-  //   "chrome.exe <url-to-the-web-page>".
-  // So, we create a ShellLinkItem object with the following parameters.
-  // * arguments
-  //   The URL of a PageUsagedata object (converted to std::wstring).
-  // * title
-  //   The title of a PageUsageData object. If this string is empty, we use
-  //   the URL as our "Most Visited" page does.
-  // * icon
-  //   An empty string. This value is to be updated in OnFaviconDataAvailable().
-  most_visited_pages_.clear();
-  for (std::vector<PageUsageData*>::const_iterator page = data->begin();
-       page != data->end(); ++page) {
-    scoped_refptr<ShellLinkItem> link(new ShellLinkItem);
-    std::string url = (*page)->GetURL().spec();
-    link->SetArguments(UTF8ToWide(url));
-    link->SetTitle(
-        !(*page)->GetTitle().empty() ? (*page)->GetTitle() : link->arguments());
-    most_visited_pages_.push_back(link);
-    icon_urls_.push_back(make_pair(url, link));
-  }
-
-  // Create a list of ShellLinkItems from the "Recently Closed" pages.
-  // As noted above, we create a ShellLinkItem objects with the following
-  // parameters.
-  // * arguments
-  //   The last URL of the tab object.
-  // * title
-  //   The title of the last URL.
-  // * icon
-  //   An empty string. This value is to be updated in OnFaviconDataAvailable().
-  // This code is copied from
-  // RecentlyClosedTabsHandler::TabRestoreServiceChanged() to emulate it.
-  const int kRecentlyClosedCount = 4;
-  recently_closed_pages_.clear();
-  TabRestoreService* tab_restore_service =
-      TabRestoreServiceFactory::GetForProfile(profile_);
-  const TabRestoreService::Entries& entries = tab_restore_service->entries();
-  for (TabRestoreService::Entries::const_iterator it = entries.begin();
-       it != entries.end(); ++it) {
-    const TabRestoreService::Entry* entry = *it;
-    if (entry->type == TabRestoreService::TAB) {
-      AddTab(static_cast<const TabRestoreService::Tab*>(entry),
-             &recently_closed_pages_, kRecentlyClosedCount);
-    } else if (entry->type == TabRestoreService::WINDOW) {
-      AddWindow(static_cast<const TabRestoreService::Window*>(entry),
-                &recently_closed_pages_, kRecentlyClosedCount);
-    }
-  }
-
-  // Send a query that retrieves the first favicon.
-  StartLoadingFavicon();
-}
-
 void JumpList::OnFaviconDataAvailable(
     FaviconService::Handle handle,
     history::FaviconData favicon) {
-  // Attach the received data to the ShellLinkItem object.
-  // This data will be decoded by JumpListUpdateTask.
-  if (favicon.is_valid()) {
-    if (!icon_urls_.empty() && icon_urls_.front().second)
-      icon_urls_.front().second->SetIconData(favicon.image_data);
+  // If there is currently an favicon request in progress, it is now outdated,

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

an favicon -> a favicon

+  // as we have received another, so nullify the handle from the old request.
+  handle_ = NULL;
+  // lock the list to set icon data and pop the url
+  {
+    base::AutoLock auto_lock(list_lock_);
+    // Attach the received data to the ShellLinkItem object.
+    // This data will be decoded by the RunUpdate method.
+    if (favicon.is_valid()) {
+      if (!icon_urls_.empty() && icon_urls_.front().second)
+        icon_urls_.front().second->SetIconData(favicon.image_data);
+    }
+
+    if (!icon_urls_.empty())
+      icon_urls_.pop_front();
   }
-
   // if we need to load more favicons, we send another query and exit.
-  if (!icon_urls_.empty())
-    icon_urls_.pop_front();
   if (StartLoadingFavicon())
     return;
 
   // Finished loading all favicons needed by the application JumpList.
-  // We create a JumpListUpdateTask that creates icon files, and we post it to
+  // We create a RunnableMethod that creates icon files, and we post it to

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

We don't actually "create" a runnable method...

   // the file thread.
+  this->AddRef();

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

Do we need this??? If so, please add a comment...

[Cait (Slow), 2011/08/15 19:44:45]

We *shouldn't* need this (I think RunnableMethod handles adding and removing
refs from the object on which the method is being called), but when I remove it,
I get errors when the jumplist deconstructor is called....

On 2011/08/15 16:16:16, MAD wrote:

   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
-      new JumpListUpdateTask(app_id_.c_str(), icon_dir_, most_visited_pages_,
-                             recently_closed_pages_));
+                           NewRunnableMethod(this, &JumpList::RunUpdate));

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

Alignment should be kept as it was, i.e., at the same level as previous line...

+}
 
-  // Delete all items in these lists since we don't need these lists any longer.
-  most_visited_pages_.clear();
-  recently_closed_pages_.clear();
+void JumpList::RunUpdate() {
+  ShellLinkItemList local_most_visited_pages;
+  ShellLinkItemList local_recently_closed_pages;
+
+  {
+    base::AutoLock auto_lock(list_lock_);
+    // Make sure we are not out of date: if icon_urls_ is not empty, then
+    // another notification has been received since we processed this one
+    if (!icon_urls_.empty())
+      return;
+
+    // Make local copies of lists so we can release the lock.
+    local_most_visited_pages = most_visited_pages_;
+    local_recently_closed_pages = recently_closed_pages_;
+  }
+
+  // Delete the directory which contains old icon files, rename the current
+  // icon directory, and create a new directory which contains new JumpList
+  // icon files.
+  FilePath icon_dir_old(icon_dir_.value() + L"Old");
+  if (file_util::PathExists(icon_dir_old))
+    file_util::Delete(icon_dir_old, true);
+  file_util::Move(icon_dir_, icon_dir_old);
+  file_util::CreateDirectory(icon_dir_);
+
+  // Create temporary icon files for shortcuts in the "Most Visited" category.
+  DecodeIconData(local_most_visited_pages);
+
+  // Create temporary icon files for shortcuts in the "Recently Closed"
+  // category.
+  DecodeIconData(local_recently_closed_pages);
+
+  // We finished collecting all resources needed for updating an appliation
+  // JumpList. So, create a new JumpList and replace the current JumpList
+  // with it.
+  UpdateJumpList(app_id_.c_str(), local_most_visited_pages,
+                                  local_recently_closed_pages);
 }
+
+void JumpList::DecodeIconData(const ShellLinkItemList& item_list) {
+  for (ShellLinkItemList::const_iterator item = item_list.begin();
+     item != item_list.end(); ++item) {

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

indent two more spaces please...

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

You missed this one too...

+    SkBitmap icon_bitmap;
+    if ((*item)->data().get() &&
+        gfx::PNGCodec::Decode((*item)->data()->front(),
+                              (*item)->data()->size(),
+                              &icon_bitmap)) {
+      FilePath icon_path;
+      if (CreateIconFile(icon_bitmap, icon_dir_, &icon_path))
+        (*item)->SetIcon(icon_path.value(), 0, true);
+    }
+  }
+}