diff options
| -rw-r--r-- | chrome/app/chrome_dll_resource.h | 2 | ||||
| -rw-r--r-- | chrome/app/generated_resources.grd | 3 | ||||
| -rw-r--r-- | chrome/app/nibs/MainMenu.xib | 7 | ||||
| -rw-r--r-- | chrome/browser/cocoa/history_menu_bridge.h | 148 | ||||
| -rw-r--r-- | chrome/browser/cocoa/history_menu_bridge.mm | 220 | ||||
| -rw-r--r-- | chrome/browser/cocoa/history_menu_bridge_unittest.mm | 278 | ||||
| -rw-r--r-- | chrome/browser/cocoa/history_menu_cocoa_controller.h | 1 | ||||
| -rw-r--r-- | chrome/browser/cocoa/history_menu_cocoa_controller.mm | 33 | ||||
| -rw-r--r-- | chrome/browser/cocoa/history_menu_cocoa_controller_unittest.mm | 88 | ||||
| -rw-r--r-- | chrome/browser/sessions/tab_restore_service.h | 6 |
10 files changed, 519 insertions, 267 deletions
diff --git a/chrome/app/chrome_dll_resource.h b/chrome/app/chrome_dll_resource.h index feadb72..c72cf1a 100644 --- a/chrome/app/chrome_dll_resource.h +++ b/chrome/app/chrome_dll_resource.h @@ -273,8 +273,6 @@ #define IDC_SYSTEM_OPTIONS 45000 // ChromeOS only #define IDC_INTERNET_OPTIONS 45100 // ChromeOS only #define IDC_HISTORY_MENU 46000 // OSX only -#define IDC_HISTORY_MENU_VISITED 46100 // OSX only -#define IDC_HISTORY_MENU_CLOSED 46200 // OSX only #define IDC_INPUT_METHODS_MENU 46300 // Linux only // Custom context menu entries diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd index 19ea2420..cca5768 100644 --- a/chrome/app/generated_resources.grd +++ b/chrome/app/generated_resources.grd @@ -7119,6 +7119,9 @@ Keep your key file in a safe place. You will need it to create new versions of y <message name="IDS_HISTORY_CLOSED_MAC" desc="The Mac menu item for the header of recently closed items in the history menu."> Recently Closed </message> + <message name="IDS_HISTORY_CLOSED_RESTORE_WINDOW_MAC" desc="The Mac menu item for restoring all the tabs of a recently closed window."> + Restore All Tabs + </message> <!-- Bookmarks menu --> <message name="IDS_BOOKMARK_MANAGER_MAC" desc="The Mac menu item for opening the bookmark manager, in the bookmark menu."> Bookmark Manager diff --git a/chrome/app/nibs/MainMenu.xib b/chrome/app/nibs/MainMenu.xib index 38a97ec..c4e689c 100644 --- a/chrome/app/nibs/MainMenu.xib +++ b/chrome/app/nibs/MainMenu.xib @@ -878,6 +878,7 @@ <int key="NSMnemonicLoc">2147483647</int> <reference key="NSOnImage" ref="353210768"/> <reference key="NSMixedImage" ref="549394948"/> + <int key="NSTag">400</int> </object> <object class="NSMenuItem" id="792145602"> <reference key="NSMenu" ref="436720301"/> @@ -887,7 +888,7 @@ <int key="NSMnemonicLoc">2147483647</int> <reference key="NSOnImage" ref="353210768"/> <reference key="NSMixedImage" ref="549394948"/> - <int key="NSTag">46100</int> + <int key="NSTag">401</int> </object> <object class="NSMenuItem" id="259488787"> <reference key="NSMenu" ref="436720301"/> @@ -898,6 +899,7 @@ <int key="NSMnemonicLoc">2147483647</int> <reference key="NSOnImage" ref="353210768"/> <reference key="NSMixedImage" ref="549394948"/> + <int key="NSTag">440</int> </object> <object class="NSMenuItem" id="101838950"> <reference key="NSMenu" ref="436720301"/> @@ -907,7 +909,7 @@ <int key="NSMnemonicLoc">2147483647</int> <reference key="NSOnImage" ref="353210768"/> <reference key="NSMixedImage" ref="549394948"/> - <int key="NSTag">46200</int> + <int key="NSTag">441</int> </object> <object class="NSMenuItem" id="517951834"> <reference key="NSMenu" ref="436720301"/> @@ -918,6 +920,7 @@ <int key="NSMnemonicLoc">2147483647</int> <reference key="NSOnImage" ref="353210768"/> <reference key="NSMixedImage" ref="549394948"/> + <int key="NSTag">480</int> </object> <object class="NSMenuItem" id="64100325"> <reference key="NSMenu" ref="436720301"/> diff --git a/chrome/browser/cocoa/history_menu_bridge.h b/chrome/browser/cocoa/history_menu_bridge.h index bfa7959b..ef84031 100644 --- a/chrome/browser/cocoa/history_menu_bridge.h +++ b/chrome/browser/cocoa/history_menu_bridge.h @@ -6,12 +6,14 @@ #define CHROME_BROWSER_COCOA_HISTORY_MENU_BRIDGE_H_ #import <Cocoa/Cocoa.h> +#include <map> + #include "base/scoped_nsobject.h" #include "base/scoped_ptr.h" -#include "base/scoped_vector.h" #include "chrome/browser/cancelable_request.h" #import "chrome/browser/favicon_service.h" #include "chrome/browser/history/history.h" +#include "chrome/browser/sessions/session_id.h" #include "chrome/browser/sessions/tab_restore_service.h" #include "chrome/common/notification_observer.h" @@ -22,24 +24,34 @@ class Profile; class TabNavigationEntry; @class HistoryMenuCocoaController; -// C++ controller for the history menu; one per AppController (means there +namespace { + +class HistoryMenuBridgeTest; + +} + +// C++ bridge for the history menu; one per AppController (means there // is only one). This class observes various data sources, namely the // HistoryService and the TabRestoreService, and then updates the NSMenu when // there is new data. // // The history menu is broken up into sections: most visisted and recently -// closed. The overall menu has a tag of IDC_HISTORY_MENU, with two sections -// IDC_HISTORY_MENU_VISITED and IDC_HISTORY_MENU_CLOSED, which are used to -// delineate the two sections. Items within these sections are assigned tags -// within IDC_HISTORY_MENU_* + 1..99. Most Chromium Cocoa menu items are static -// from a nib (e.g. New Tab), but may be enabled/disabled under certain -// circumstances (e.g. Cut and Paste). In addition, most Cocoa menu items have -// firstResponder: as a target. Unusually, history menu items are created -// dynamically. They also have a target of HistoryMenuCocoaController, not -// firstResponder. See HistoryMenuBridge::AddItemToMenu(). Unlike most of our -// Cocoa-Bridge classes, the HMCC is not at the root of the ownership model -// because its only function is to respond to menu item actions; everything -// else is done in this bridge. +// closed. The overall menu has a tag of IDC_HISTORY_MENU, with the user content +// items having the local tags defined in the enum below. Items within a section +// all share the same tag. The structure of the menu is laid out in MainMenu.xib +// and the generated content is inserted after the Title elements. The recently +// closed section is special in that those menu items can have submenus to list +// all the tabs within that closed window. By convention, these submenu items +// have a tag that's equal to the parent + 1. Tags within the history menu have +// a range of [400,500) and do not go through CommandDispatch for their target- +// action mechanism. +// +// These menu items do not use firstResponder as their target. Rather, they are +// hooked directly up to the HistoryMenuCocoaController that then bridges back +// to this class. These items are created via the AddItemToMenu() helper. Also, +// unlike the typical ownership model, this bridge owns its controller. The +// controller is very thin and only exists to interact with Cocoa, but this +// class does the bulk of the work. class HistoryMenuBridge : public NotificationObserver, public TabRestoreService::Observer { public: @@ -47,7 +59,19 @@ class HistoryMenuBridge : public NotificationObserver, // we pull things from different sources with different data types. struct HistoryItem { public: - HistoryItem() : icon_requested(false) {} + HistoryItem() + : icon_requested(false), + menu_item(nil), + session_id(0) {} + + // Copy constructor allowed. + HistoryItem(const HistoryItem& copy) + : title(copy.title), + url(copy.url), + icon_requested(false), + menu_item(nil), + session_id(copy.session_id) {} + ~HistoryItem() {} // The title for the menu item. @@ -67,8 +91,36 @@ class HistoryMenuBridge : public NotificationObserver, // The pointer to the item after it has been created. Weak. NSMenuItem* menu_item; + // This ID is unique for a browser session and can be passed to the + // TabRestoreService to re-open the closed window or tab that this + // references. A non-0 session ID indicates that this is an entry can be + // restored that way. Otherwise, the URL will be used to open the item and + // this ID will be 0. + SessionID::id_type session_id; + + // If the HistoryItem is a window, this will be the vector of tabs. Note + // that this is a list of weak references. The |menu_item_map_| is the owner + // of all items. If it is not a window, then the entry is a single page and + // the vector will be empty. + std::vector<HistoryItem*> tabs; + private: - DISALLOW_COPY_AND_ASSIGN(HistoryItem); + // Copying is explicitly allowed, but assignment is not. + void operator=(const HistoryItem&); + }; + + // These tags are not global view tags and are local to the history menu. The + // normal procedure for menu items is to go through CommandDispatch, but since + // history menu items are hooked directly up to their target, they do not need + // to have the global IDC view tags. + enum Tags { + kMostVisitedSeparator = 400, // Separator before most visited section. + kMostVisitedTitle = 401, // Title of the most visited section. + kMostVisited = 420, // Used for all entries in the most visited section. + kRecentlyClosedSeparator = 440, // Item before recently closed section. + kRecentlyClosedTitle = 441, // Title of recently closed section. + kRecentlyClosed = 460, // Used for items in the recently closed section. + kShowFullSeparator = 480 // Separator after the recently closed section. }; explicit HistoryMenuBridge(Profile* profile); @@ -83,27 +135,34 @@ class HistoryMenuBridge : public NotificationObserver, virtual void TabRestoreServiceChanged(TabRestoreService* service); virtual void TabRestoreServiceDestroyed(TabRestoreService* service); + // Looks up an NSMenuItem in the |menu_item_map_| and returns the + // corresponding HistoryItem. + HistoryItem* HistoryItemForMenuItem(NSMenuItem* item); + // I wish I has a "friend @class" construct. These are used by the HMCC // to access model information when responding to actions. HistoryService* service(); Profile* profile(); - const ScopedVector<HistoryItem>* const visited_results(); - const ScopedVector<HistoryItem>* const closed_results(); protected: // Return the History menu. - NSMenu* HistoryMenu(); - - // Clear items in the given |menu|. The menu is broken into sections, defined - // by IDC_HISTORY_MENU_* constants. This function will clear |count| menu - // items, starting from |tag|. - void ClearMenuSection(NSMenu* menu, NSInteger tag, unsigned int count); - - // Adds a given title and URL to HistoryMenu() with a certain tag and index. - void AddItemToMenu(HistoryItem* item, - NSMenu* menu, - NSInteger tag, - NSInteger index); + virtual NSMenu* HistoryMenu(); + + // Clear items in the given |menu|. Menu items in the same section are given + // the same tag. This will go through the entire history menu, removing all + // items with a given tag. Note that this will recurse to submenus, removing + // child items from the menu item map. This will only remove items that have + // a target hooked up to the |controller_|. + void ClearMenuSection(NSMenu* menu, NSInteger tag); + + // Adds a given title and URL to the passed-in menu with a certain tag and + // index. This will add |item| and the newly created menu item to the + // |menu_item_map_|, which takes ownership. Items are deleted in + // ClearMenuSection(). This returns the new menu item that was just added. + NSMenuItem* AddItemToMenu(HistoryItem* item, + NSMenu* menu, + NSInteger tag, + NSInteger index); // Called by the ctor if |service_| is ready at the time, or by a // notification receiver. Finishes initialization tasks by subscribing for @@ -118,9 +177,9 @@ class HistoryMenuBridge : public NotificationObserver, void OnVisitedHistoryResults(CancelableRequestProvider::Handle handle, std::vector<PageUsageData*>* results); - // Tries to add the current Tab's TabNavigationEntry's NavigationEntry object - // to |closed_results_|. Return TRUE if the operation completed successfully. - bool AddNavigationForTab(const TabRestoreService::Tab& entry); + // Creates a HistoryItem* for the given tab entry. Caller takes ownership of + // the result and must delete it when finished. + HistoryItem* HistoryItemForTab(const TabRestoreService::Tab& entry); // Helper function that sends an async request to the FaviconService to get // an icon. The callback will update the NSMenuItem directly. @@ -141,28 +200,29 @@ class HistoryMenuBridge : public NotificationObserver, void CancelFaviconRequest(HistoryItem* item); private: - friend class HistoryMenuBridgeTest; + friend class ::HistoryMenuBridgeTest; + friend class HistoryMenuCocoaControllerTest; scoped_nsobject<HistoryMenuCocoaController> controller_; // strong - Profile* profile_; // weak - HistoryService* history_service_; // weak - TabRestoreService* tab_restore_service_; // weak + Profile* profile_; // weak + HistoryService* history_service_; // weak + TabRestoreService* tab_restore_service_; // weak NotificationRegistrar registrar_; CancelableRequestConsumer cancelable_request_consumer_; - // The most recent results we've received. - ScopedVector<HistoryItem> visited_results_; - ScopedVector<HistoryItem> closed_results_; + // Mapping of NSMenuItems to HistoryItems. This owns the HistoryItems until + // they are removed and deleted via ClearMenuSection(). + std::map<NSMenuItem*, HistoryItem*> menu_item_map_; // Maps HistoryItems to favicon request Handles. CancelableRequestConsumerTSimple<HistoryItem*> favicon_consumer_; - // We coalesce requests to re-create the menu. |create_in_progress_| is true - // whenever we are either waiting for the history service to return query - // results, or when we are rebuilding. |need_recreate_| is true whenever a - // rebuild has been scheduled but is waiting for the current one to finish. + // Requests to re-create the menu are coalesced. |create_in_progress_| is true + // when either waiting for the history service to return query results, or + // when the menu is rebuilding. |need_recreate_| is true whenever a rebuild + // has been scheduled but is waiting for the current one to finish. bool create_in_progress_; bool need_recreate_; diff --git a/chrome/browser/cocoa/history_menu_bridge.mm b/chrome/browser/cocoa/history_menu_bridge.mm index 82c8beb..093ab40 100644 --- a/chrome/browser/cocoa/history_menu_bridge.mm +++ b/chrome/browser/cocoa/history_menu_bridge.mm @@ -4,6 +4,7 @@ #include "chrome/browser/cocoa/history_menu_bridge.h" +#include "app/l10n_util.h" #include "app/resource_bundle.h" #include "base/callback.h" #include "base/stl_util-inl.h" @@ -20,6 +21,7 @@ #include "chrome/common/url_constants.h" #include "gfx/codec/png_codec.h" #include "grit/app_resources.h" +#include "grit/generated_resources.h" #include "grit/theme_resources.h" #include "skia/ext/skia_utils_mac.h" #include "third_party/skia/include/core/SkBitmap.h" @@ -27,10 +29,10 @@ namespace { // Menus more than this many chars long will get trimmed. -const NSUInteger kMaximumMenuWidthInChars = 65; +const NSUInteger kMaximumMenuWidthInChars = 50; // When trimming, use this many chars from each side. -const NSUInteger kMenuTrimSizeInChars = 30; +const NSUInteger kMenuTrimSizeInChars = 25; // Number of days to consider when getting the number of most visited items. const int kMostVisitedScope = 90; @@ -64,8 +66,10 @@ HistoryMenuBridge::HistoryMenuBridge(Profile* profile) // TODO(???): NULL here means we're OTR. Show this in the GUI somehow? tab_restore_service_ = profile_->GetTabRestoreService(); - if (tab_restore_service_) + if (tab_restore_service_) { tab_restore_service_->AddObserver(this); + tab_restore_service_->LoadTabsFromLastSession(); + } } ResourceBundle& rb = ResourceBundle::GetSharedInstance(); @@ -99,6 +103,14 @@ HistoryMenuBridge::~HistoryMenuBridge() { if (tab_restore_service_) tab_restore_service_->RemoveObserver(this); + + // Since the map owns the HistoryItems, delete anything that still exists. + std::map<NSMenuItem*, HistoryItem*>::iterator it = menu_item_map_.begin(); + while (it != menu_item_map_.end()) { + HistoryItem* item = it->second; + menu_item_map_.erase(it++); + delete item; + } } void HistoryMenuBridge::Observe(NotificationType type, @@ -129,51 +141,88 @@ void HistoryMenuBridge::Observe(NotificationType type, void HistoryMenuBridge::TabRestoreServiceChanged(TabRestoreService* service) { const TabRestoreService::Entries& entries = service->entries(); - // Clear the history menu before modifying |closed_results_|. + // Clear the history menu before rebuilding. NSMenu* menu = HistoryMenu(); - ClearMenuSection(menu, IDC_HISTORY_MENU_CLOSED, closed_results_.size()); - closed_results_.reset(); + ClearMenuSection(menu, kRecentlyClosed); + + // Index for the next menu item. + NSInteger index = [menu indexOfItemWithTag:kRecentlyClosedTitle] + 1; + NSUInteger added_count = 0; - unsigned int added_count = 0; for (TabRestoreService::Entries::const_iterator it = entries.begin(); it != entries.end() && added_count < kRecentlyClosedCount; ++it) { TabRestoreService::Entry* entry = *it; - // If we have a window, loop over all of its tabs. This could consume all - // of |kRecentlyClosedCount| in a given outer loop iteration. + // If this is a window, create a submenu for all of its tabs. if (entry->type == TabRestoreService::WINDOW) { TabRestoreService::Window* entry_win = (TabRestoreService::Window*)entry; - std::vector<TabRestoreService::Tab> tabs = entry_win->tabs; + std::vector<TabRestoreService::Tab>& tabs = entry_win->tabs; + if (!tabs.size()) + continue; + + // Create the item for the parent/window. + HistoryItem* item = new HistoryItem(); + if (tabs.size() == 1) { + item->title = l10n_util::GetStringUTF16( + IDS_NEW_TAB_RECENTLY_CLOSED_WINDOW_SINGLE); + } else { + item->title =l10n_util::GetStringFUTF16( + IDS_NEW_TAB_RECENTLY_CLOSED_WINDOW_MULTIPLE, + IntToString16(tabs.size())); + } + item->session_id = entry_win->id; + + // Create the submenu. + scoped_nsobject<NSMenu> submenu([[NSMenu alloc] init]); + + // Create standard items within the window submenu. + NSString* restore_title = l10n_util::GetNSString( + IDS_HISTORY_CLOSED_RESTORE_WINDOW_MAC); + scoped_nsobject<NSMenuItem> restore_item( + [[NSMenuItem alloc] initWithTitle:restore_title + action:@selector(openHistoryMenuItem:) + keyEquivalent:@""]); + [restore_item setTarget:controller_.get()]; + // Duplicate the HistoryItem otherwise the different NSMenuItems will + // point to the same HistoryItem, which would then be double-freed when + // removing the items from the map or in the dtor. + HistoryItem* dup_item = new HistoryItem(*item); + menu_item_map_.insert(std::make_pair(restore_item.get(), dup_item)); + [submenu addItem:restore_item.get()]; + [submenu addItem:[NSMenuItem separatorItem]]; + + // Loop over the window's tabs and add them to the submenu. + NSInteger subindex = [[submenu itemArray] count]; std::vector<TabRestoreService::Tab>::const_iterator it; - for (it = tabs.begin(); it != tabs.end() && - added_count < kRecentlyClosedCount; ++it) { + for (it = tabs.begin(); it != tabs.end(); ++it) { TabRestoreService::Tab tab = *it; - if (AddNavigationForTab(tab)) - ++added_count; + HistoryItem* tab_item = HistoryItemForTab(tab); + if (tab_item) { + item->tabs.push_back(tab_item); + AddItemToMenu(tab_item, submenu.get(), kRecentlyClosed + 1, + subindex++); + } + } + + // Sometimes it is possible for there to not be any subitems for a given + // window; if that is the case, do not add the entry to the main menu. + if ([[submenu itemArray] count] > 2) { + // Create the menu item parent. + NSMenuItem* parent_item = + AddItemToMenu(item, menu, kRecentlyClosed, index++); + [parent_item setSubmenu:submenu.get()]; + ++added_count; } } else if (entry->type == TabRestoreService::TAB) { TabRestoreService::Tab* tab = static_cast<TabRestoreService::Tab*>(entry); - if (AddNavigationForTab(*tab)) + HistoryItem* item = HistoryItemForTab(*tab); + if (item) { + AddItemToMenu(item, menu, kRecentlyClosed, index++); ++added_count; + } } } - - // Remove extraneous/old results. - if (closed_results_.size() > kRecentlyClosedCount) - STLDeleteContainerPointers(closed_results_.begin(), - closed_results_.end() - kRecentlyClosedCount); - - NSInteger top_index = [menu indexOfItemWithTag:IDC_HISTORY_MENU_CLOSED] + 1; - - int i = 0; // Count offsets for |tag| and |index| in AddItemToMenu(). - for (ScopedVector<HistoryItem>::const_iterator it = closed_results_.begin(); - it != closed_results_.end(); ++it) { - HistoryItem* item = *it; - NSInteger tag = IDC_HISTORY_MENU_CLOSED + 1 + i; - AddItemToMenu(item, HistoryMenu(), tag, top_index + i); - ++i; - } } void HistoryMenuBridge::TabRestoreServiceDestroyed( @@ -181,6 +230,15 @@ void HistoryMenuBridge::TabRestoreServiceDestroyed( // Intentionally left blank. We hold a weak reference to the service. } +HistoryMenuBridge::HistoryItem* HistoryMenuBridge::HistoryItemForMenuItem( + NSMenuItem* item) { + std::map<NSMenuItem*, HistoryItem*>::iterator it = menu_item_map_.find(item); + if (it != menu_item_map_.end()) { + return it->second; + } + return NULL; +} + HistoryService* HistoryMenuBridge::service() { return history_service_; } @@ -189,61 +247,42 @@ Profile* HistoryMenuBridge::profile() { return profile_; } -const ScopedVector<HistoryMenuBridge::HistoryItem>* const - HistoryMenuBridge::visited_results() { - return &visited_results_; -} - -const ScopedVector<HistoryMenuBridge::HistoryItem>* const - HistoryMenuBridge::closed_results() { - return &closed_results_; -} - NSMenu* HistoryMenuBridge::HistoryMenu() { NSMenu* history_menu = [[[NSApp mainMenu] itemWithTag:IDC_HISTORY_MENU] submenu]; return history_menu; } -void HistoryMenuBridge::ClearMenuSection(NSMenu* menu, - NSInteger tag, - unsigned int count) { - const NSInteger max_tag = tag + count + 1; - - // Get the index of the first item in the section, excluding the header. - NSInteger index = [menu indexOfItemWithTag:tag] + 1; - if (index <= 0 || index >= [menu numberOfItems]) - return; // The section is at the end, empty. - - // Remove at the same index, usually, because the menu will shrink by one - // item each time, shifting all the lower elements up. If we hit a "unhooked" - // menu item, don't remove it, but advance the index to skip the item. - NSInteger item_tag = tag; - while (count > 0 && item_tag < max_tag && index < [menu numberOfItems]) { - NSMenuItem* menu_item = [menu itemAtIndex:index]; - item_tag = [menu_item tag]; - if ([menu_item action] == @selector(openHistoryMenuItem:)) { - // If there is a pending favicon request for this menu item, find and - // cancel it. - HistoryItem* item = - const_cast<HistoryItem*>([controller_ itemForTag:item_tag]); - if (item) +void HistoryMenuBridge::ClearMenuSection(NSMenu* menu, NSInteger tag) { + for (NSMenuItem* menu_item in [menu itemArray]) { + if ([menu_item tag] == tag && [menu_item target] == controller_.get()) { + // This is an item that should be removed, so find the corresponding model + // item. + HistoryItem* item = HistoryItemForMenuItem(menu_item); + + // Cancel favicon requests that could hold onto stale pointers. Also + // remove the item from the mapping. + if (item) { CancelFaviconRequest(item); + menu_item_map_.erase(menu_item); + delete item; + } - // Now remove it from the menu. - [menu removeItemAtIndex:index]; - --count; - } - else { - ++index; + // If this menu item has a submenu, recurse. + if ([menu_item hasSubmenu]) { + ClearMenuSection([menu_item submenu], tag + 1); + } + + // Now actually remove the item from the menu. + [menu removeItem:menu_item]; } } } -void HistoryMenuBridge::AddItemToMenu(HistoryItem* item, - NSMenu* menu, - NSInteger tag, - NSInteger index) { +NSMenuItem* HistoryMenuBridge::AddItemToMenu(HistoryItem* item, + NSMenu* menu, + NSInteger tag, + NSInteger index) { NSString* title = base::SysUTF16ToNSString(item->title); std::string url_string = item->url.possibly_invalid_spec(); @@ -251,7 +290,7 @@ void HistoryMenuBridge::AddItemToMenu(HistoryItem* item, if ([title isEqualToString:@""]) title = base::SysUTF8ToNSString(url_string); NSString* full_title = title; - if (false && [title length] > kMaximumMenuWidthInChars) { + if ([title length] > kMaximumMenuWidthInChars) { // TODO(rsesek): use app/text_elider.h once it uses string16 and can // take out the middle of strings. title = [NSString stringWithFormat:@"%@…%@", @@ -268,7 +307,7 @@ void HistoryMenuBridge::AddItemToMenu(HistoryItem* item, [menu_item setTag:tag]; if (item->icon.get()) [menu_item setImage:item->icon.get()]; - else + else if (!item->tabs.size()) [menu_item setImage:default_favicon_.get()]; // Add a tooltip. @@ -277,7 +316,10 @@ void HistoryMenuBridge::AddItemToMenu(HistoryItem* item, [menu_item setToolTip:tooltip]; [menu insertItem:menu_item atIndex:index]; - item->menu_item = menu_item; + item->menu_item = menu_item.get(); + menu_item_map_.insert(std::make_pair(menu_item.get(), item)); + + return menu_item; } void HistoryMenuBridge::Init() { @@ -305,10 +347,8 @@ void HistoryMenuBridge::OnVisitedHistoryResults( CancelableRequestProvider::Handle handle, std::vector<PageUsageData*>* results) { NSMenu* menu = HistoryMenu(); - NSInteger top_item = [menu indexOfItemWithTag:IDC_HISTORY_MENU_VISITED] + 1; - - ClearMenuSection(menu, IDC_HISTORY_MENU_VISITED, visited_results_.size()); - visited_results_.reset(); + ClearMenuSection(menu, kMostVisited); + NSInteger top_item = [menu indexOfItemWithTag:kMostVisitedTitle] + 1; size_t count = results->size(); for (size_t i = 0; i < count; ++i) { @@ -323,12 +363,8 @@ void HistoryMenuBridge::OnVisitedHistoryResults( } else { GetFaviconForHistoryItem(item); } - visited_results_.push_back(item); // ScopedVector takes ownership. - - // Use the large gaps in tags assignment to create the tag for history menu - // items. - NSInteger tag = IDC_HISTORY_MENU_VISITED + 1 + i; - AddItemToMenu(item, HistoryMenu(), tag, top_item + i); + // This will add |item| to the |menu_item_map_|, which takes ownership. + AddItemToMenu(item, HistoryMenu(), kMostVisited, top_item + i); } // We are already invalid by the time we finished, darn. @@ -338,25 +374,25 @@ void HistoryMenuBridge::OnVisitedHistoryResults( create_in_progress_ = false; } -bool HistoryMenuBridge::AddNavigationForTab( +HistoryMenuBridge::HistoryItem* HistoryMenuBridge::HistoryItemForTab( const TabRestoreService::Tab& entry) { if (entry.navigations.empty()) - return false; + return NULL; const TabNavigation& current_navigation = entry.navigations.at(entry.current_navigation_index); if (current_navigation.virtual_url() == GURL(chrome::kChromeUINewTabURL)) - return false; + return NULL; HistoryItem* item = new HistoryItem(); item->title = current_navigation.title(); item->url = current_navigation.virtual_url(); - closed_results_.push_back(item); // ScopedVector takes ownership. + item->session_id = entry.id; // Tab navigations don't come with icons, so we always have to request them. GetFaviconForHistoryItem(item); - return true; + return item; } void HistoryMenuBridge::GetFaviconForHistoryItem(HistoryItem* item) { diff --git a/chrome/browser/cocoa/history_menu_bridge_unittest.mm b/chrome/browser/cocoa/history_menu_bridge_unittest.mm index e8d5dd7..0d23906 100644 --- a/chrome/browser/cocoa/history_menu_bridge_unittest.mm +++ b/chrome/browser/cocoa/history_menu_bridge_unittest.mm @@ -11,36 +11,58 @@ #include "chrome/app/chrome_dll_resource.h" #include "chrome/browser/browser.h" #include "chrome/browser/cancelable_request.h" +#include "chrome/browser/cocoa/browser_test_helper.h" #include "chrome/browser/cocoa/cocoa_test_helper.h" #include "chrome/browser/cocoa/history_menu_bridge.h" -#include "chrome/browser/cocoa/browser_test_helper.h" +#include "chrome/browser/sessions/tab_restore_service.h" #include "gfx/codec/png_codec.h" +#include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" +namespace { + +class MockTRS : public TabRestoreService { + public: + MockTRS(Profile* profile) : TabRestoreService(profile, NULL) {} + MOCK_CONST_METHOD0(entries, const TabRestoreService::Entries&()); +}; + +class MockBridge : public HistoryMenuBridge { + public: + MockBridge(Profile* profile) + : HistoryMenuBridge(profile), + menu_([[NSMenu alloc] initWithTitle:@"History"]) {} + + virtual NSMenu* HistoryMenu() { + return menu_.get(); + } + + private: + scoped_nsobject<NSMenu> menu_; +}; + class HistoryMenuBridgeTest : public CocoaTest { public: virtual void SetUp() { CocoaTest::SetUp(); browser_test_helper_.profile()->CreateFaviconService(); + bridge_.reset(new MockBridge(browser_test_helper_.profile())); } // We are a friend of HistoryMenuBridge (and have access to // protected methods), but none of the classes generated by TEST_F() // are. Wraps common commands. - void ClearMenuSection(HistoryMenuBridge* bridge, - NSMenu* menu, - NSInteger tag, - unsigned int count) { - bridge->ClearMenuSection(menu, tag, count); + void ClearMenuSection(NSMenu* menu, + NSInteger tag) { + bridge_->ClearMenuSection(menu, tag); } - void AddItemToBridgeMenu(HistoryMenuBridge* bridge, - HistoryMenuBridge::HistoryItem* item, + void AddItemToBridgeMenu(HistoryMenuBridge::HistoryItem* item, NSMenu* menu, NSInteger tag, NSInteger index) { - bridge->AddItemToMenu(item, menu, tag, index); + bridge_->AddItemToMenu(item, menu, tag, index); } NSMenuItem* AddItemToMenu(NSMenu* menu, @@ -50,8 +72,10 @@ class HistoryMenuBridgeTest : public CocoaTest { NSMenuItem* item = [[[NSMenuItem alloc] initWithTitle:title action:NULL keyEquivalent:@""] autorelease]; [item setTag:tag]; - if (selector) + if (selector) { [item setAction:selector]; + [item setTarget:bridge_->controller_.get()]; + } [menu addItem:item]; return item; } @@ -64,93 +88,88 @@ class HistoryMenuBridgeTest : public CocoaTest { return item; } - void GetFaviconForHistoryItem(HistoryMenuBridge* bridge, - HistoryMenuBridge::HistoryItem* item) { - bridge->GetFaviconForHistoryItem(item); + MockTRS::Tab CreateSessionTab(const GURL& url, const string16& title) { + MockTRS::Tab tab; + tab.current_navigation_index = 0; + tab.navigations.push_back( + TabNavigation(0, url, GURL(), title, std::string(), + PageTransition::LINK)); + return tab; } - void GotFaviconData(HistoryMenuBridge* bridge, - FaviconService::Handle handle, + void GetFaviconForHistoryItem(HistoryMenuBridge::HistoryItem* item) { + bridge_->GetFaviconForHistoryItem(item); + } + + void GotFaviconData(FaviconService::Handle handle, bool know_favicon, scoped_refptr<RefCountedBytes> data, bool expired, GURL url) { - bridge->GotFaviconData(handle, know_favicon, data, expired, url); + bridge_->GotFaviconData(handle, know_favicon, data, expired, url); } CancelableRequestConsumerTSimple<HistoryMenuBridge::HistoryItem*>& - favicon_consumer(HistoryMenuBridge* bridge) { - return bridge->favicon_consumer_; + favicon_consumer() { + return bridge_->favicon_consumer_; } BrowserTestHelper browser_test_helper_; + scoped_ptr<MockBridge> bridge_; }; // Edge case test for clearing until the end of a menu. TEST_F(HistoryMenuBridgeTest, ClearHistoryMenuUntilEnd) { - scoped_ptr<HistoryMenuBridge> bridge( - new HistoryMenuBridge(browser_test_helper_.profile())); - EXPECT_TRUE(bridge.get()); - NSMenu* menu = [[[NSMenu alloc] initWithTitle:@"history foo"] autorelease]; - NSInteger tag = IDC_HISTORY_MENU_VISITED; - AddItemToMenu(menu, @"HEADER", NULL, tag); + AddItemToMenu(menu, @"HEADER", NULL, HistoryMenuBridge::kMostVisitedTitle); - AddItemToMenu(menu, @"alpha", @selector(openHistoryMenuItem:), ++tag); - AddItemToMenu(menu, @"bravo", @selector(openHistoryMenuItem:), ++tag); - AddItemToMenu(menu, @"charlie", @selector(openHistoryMenuItem:), ++tag); - AddItemToMenu(menu, @"delta", @selector(openHistoryMenuItem:), ++tag); + NSInteger tag = HistoryMenuBridge::kMostVisited; + AddItemToMenu(menu, @"alpha", @selector(openHistoryMenuItem:), tag); + AddItemToMenu(menu, @"bravo", @selector(openHistoryMenuItem:), tag); + AddItemToMenu(menu, @"charlie", @selector(openHistoryMenuItem:), tag); + AddItemToMenu(menu, @"delta", @selector(openHistoryMenuItem:), tag); - ClearMenuSection(bridge.get(), menu, IDC_HISTORY_MENU_VISITED, 4); + ClearMenuSection(menu, HistoryMenuBridge::kMostVisited); EXPECT_EQ(1, [menu numberOfItems]); - EXPECT_EQ(@"HEADER", [[menu itemWithTag:IDC_HISTORY_MENU_VISITED] title]); + EXPECT_TRUE([@"HEADER" isEqualToString: + [[menu itemWithTag:HistoryMenuBridge::kMostVisitedTitle] title]]); } // Skip menu items that are not hooked up to |-openHistoryMenuItem:|. TEST_F(HistoryMenuBridgeTest, ClearHistoryMenuSkipping) { - scoped_ptr<HistoryMenuBridge> bridge( - new HistoryMenuBridge(browser_test_helper_.profile())); - EXPECT_TRUE(bridge.get()); - NSMenu* menu = [[[NSMenu alloc] initWithTitle:@"history foo"] autorelease]; - NSInteger section_tag = IDC_HISTORY_MENU_VISITED; - AddItemToMenu(menu, @"HEADER", NULL, section_tag); - NSInteger tag = section_tag; + AddItemToMenu(menu, @"HEADER", NULL, HistoryMenuBridge::kMostVisitedTitle); - AddItemToMenu(menu, @"alpha", @selector(openHistoryMenuItem:), ++tag); - AddItemToMenu(menu, @"bravo", @selector(openHistoryMenuItem:), ++tag); - AddItemToMenu(menu, @"unhooked", NULL, ++tag); - AddItemToMenu(menu, @"charlie", @selector(openHistoryMenuItem:), ++tag); + NSInteger tag = HistoryMenuBridge::kMostVisited; + AddItemToMenu(menu, @"alpha", @selector(openHistoryMenuItem:), tag); + AddItemToMenu(menu, @"bravo", @selector(openHistoryMenuItem:), tag); + AddItemToMenu(menu, @"TITLE", NULL, HistoryMenuBridge::kRecentlyClosedTitle); + AddItemToMenu(menu, @"charlie", @selector(openHistoryMenuItem:), tag); - ClearMenuSection(bridge.get(), menu, section_tag, 4); + ClearMenuSection(menu, tag); EXPECT_EQ(2, [menu numberOfItems]); - EXPECT_EQ(@"HEADER", [[menu itemWithTag:section_tag] title]); - EXPECT_EQ(@"unhooked", [[menu itemAtIndex:1] title]); + EXPECT_TRUE([@"HEADER" isEqualToString: + [[menu itemWithTag:HistoryMenuBridge::kMostVisitedTitle] title]]); + EXPECT_TRUE([@"TITLE" isEqualToString: + [[menu itemAtIndex:1] title]]); } // Edge case test for clearing an empty menu. TEST_F(HistoryMenuBridgeTest, ClearHistoryMenuEmpty) { - scoped_ptr<HistoryMenuBridge> bridge( - new HistoryMenuBridge(browser_test_helper_.profile())); - EXPECT_TRUE(bridge.get()); - NSMenu* menu = [[[NSMenu alloc] initWithTitle:@"history foo"] autorelease]; - AddItemToMenu(menu, @"HEADER", NULL, IDC_HISTORY_MENU_VISITED); + AddItemToMenu(menu, @"HEADER", NULL, HistoryMenuBridge::kMostVisited); - ClearMenuSection(bridge.get(), menu, IDC_HISTORY_MENU_VISITED, 1); + ClearMenuSection(menu, HistoryMenuBridge::kMostVisited); EXPECT_EQ(1, [menu numberOfItems]); - EXPECT_EQ(@"HEADER", [[menu itemWithTag:IDC_HISTORY_MENU_VISITED] title]); + EXPECT_TRUE([@"HEADER" isEqualToString: + [[menu itemWithTag:HistoryMenuBridge::kMostVisited] title]]); } // Test that AddItemToMenu() properly adds HistoryItem objects as menus. TEST_F(HistoryMenuBridgeTest, AddItemToMenu) { - scoped_ptr<HistoryMenuBridge> bridge( - new HistoryMenuBridge(browser_test_helper_.profile())); - EXPECT_TRUE(bridge.get()); - NSMenu* menu = [[[NSMenu alloc] initWithTitle:@"history foo"] autorelease]; const string16 short_url = ASCIIToUTF16("http://foo/"); @@ -159,11 +178,13 @@ TEST_F(HistoryMenuBridgeTest, AddItemToMenu) { "or.be.reasonably-displayed-in-a-menu" "without.looking-ridiculous.com/"); // 140 chars total - scoped_ptr<HistoryMenuBridge::HistoryItem> item1(CreateItem(short_url)); - AddItemToBridgeMenu(bridge.get(), item1.get(), menu, 100, 0); + // HistoryItems are owned by the HistoryMenuBridge when AddItemToBridgeMenu() + // is called, which places them into the |menu_item_map_|, which owns them. + HistoryMenuBridge::HistoryItem* item1 = CreateItem(short_url); + AddItemToBridgeMenu(item1, menu, 100, 0); - scoped_ptr<HistoryMenuBridge::HistoryItem> item2(CreateItem(long_url)); - AddItemToBridgeMenu(bridge.get(), item2.get(), menu, 101, 1); + HistoryMenuBridge::HistoryItem* item2 = CreateItem(long_url); + AddItemToBridgeMenu(item2, menu, 101, 1); EXPECT_EQ(2, [menu numberOfItems]); @@ -188,22 +209,142 @@ TEST_F(HistoryMenuBridgeTest, AddItemToMenu) { EXPECT_GE([[[menu itemAtIndex:1] toolTip] length], (2*long_url.length()-5)); } +// Test that the menu is created for a set of simple tabs. +TEST_F(HistoryMenuBridgeTest, RecentlyClosedTabs) { + scoped_refptr<MockTRS> trs(new MockTRS(browser_test_helper_.profile())); + MockTRS::Entries entries; + + MockTRS::Tab tab1 = CreateSessionTab(GURL("http://google.com"), + ASCIIToUTF16("Google")); + tab1.id = 24; + entries.push_back(&tab1); + + MockTRS::Tab tab2 = CreateSessionTab(GURL("http://apple.com"), + ASCIIToUTF16("Apple")); + tab2.id = 42; + entries.push_back(&tab2); + + using ::testing::ReturnRef; + EXPECT_CALL(*trs.get(), entries()).WillOnce(ReturnRef(entries)); + + bridge_->TabRestoreServiceChanged(trs.get()); + + NSMenu* menu = bridge_->HistoryMenu(); + ASSERT_EQ(2U, [[menu itemArray] count]); + + NSMenuItem* item1 = [menu itemAtIndex:0]; + MockBridge::HistoryItem* hist1 = bridge_->HistoryItemForMenuItem(item1); + EXPECT_TRUE(hist1); + EXPECT_EQ(24, hist1->session_id); + EXPECT_TRUE([@"Google" isEqualToString:[item1 title]]); + + NSMenuItem* item2 = [menu itemAtIndex:1]; + MockBridge::HistoryItem* hist2 = bridge_->HistoryItemForMenuItem(item2); + EXPECT_TRUE(hist2); + EXPECT_EQ(42, hist2->session_id); + EXPECT_TRUE([@"Apple" isEqualToString:[item2 title]]); +} + +// Test that the menu is created for a mix of windows and tabs. +TEST_F(HistoryMenuBridgeTest, RecentlyClosedTabsAndWindows) { + scoped_refptr<MockTRS> trs(new MockTRS(browser_test_helper_.profile())); + MockTRS::Entries entries; + + MockTRS::Tab tab1 = CreateSessionTab(GURL("http://google.com"), + ASCIIToUTF16("Google")); + tab1.id = 24; + entries.push_back(&tab1); + + MockTRS::Window win1; + win1.id = 30; + win1.tabs.push_back( + CreateSessionTab(GURL("http://foo.com"), ASCIIToUTF16("foo"))); + win1.tabs[0].id = 31; + win1.tabs.push_back( + CreateSessionTab(GURL("http://bar.com"), ASCIIToUTF16("bar"))); + win1.tabs[1].id = 32; + entries.push_back(&win1); + + MockTRS::Tab tab2 = CreateSessionTab(GURL("http://apple.com"), + ASCIIToUTF16("Apple")); + tab2.id = 42; + entries.push_back(&tab2); + + MockTRS::Window win2; + win2.id = 50; + win2.tabs.push_back( + CreateSessionTab(GURL("http://magic.com"), ASCIIToUTF16("magic"))); + win2.tabs[0].id = 51; + win2.tabs.push_back( + CreateSessionTab(GURL("http://goats.com"), ASCIIToUTF16("goats"))); + win2.tabs[1].id = 52; + win2.tabs.push_back( + CreateSessionTab(GURL("http://teleporter.com"), + ASCIIToUTF16("teleporter"))); + win2.tabs[1].id = 53; + entries.push_back(&win2); + + using ::testing::ReturnRef; + EXPECT_CALL(*trs.get(), entries()).WillOnce(ReturnRef(entries)); + + bridge_->TabRestoreServiceChanged(trs.get()); + + NSMenu* menu = bridge_->HistoryMenu(); + ASSERT_EQ(4U, [[menu itemArray] count]); + + NSMenuItem* item1 = [menu itemAtIndex:0]; + MockBridge::HistoryItem* hist1 = bridge_->HistoryItemForMenuItem(item1); + EXPECT_TRUE(hist1); + EXPECT_EQ(24, hist1->session_id); + EXPECT_TRUE([@"Google" isEqualToString:[item1 title]]); + + NSMenuItem* item2 = [menu itemAtIndex:1]; + MockBridge::HistoryItem* hist2 = bridge_->HistoryItemForMenuItem(item2); + EXPECT_TRUE(hist2); + EXPECT_EQ(30, hist2->session_id); + EXPECT_EQ(2U, hist2->tabs.size()); + // Do not test menu item title because it is localized. + NSMenu* submenu1 = [item2 submenu]; + EXPECT_EQ(4U, [[submenu1 itemArray] count]); + // Do not test Restore All Tabs because it is localiced. + EXPECT_TRUE([[submenu1 itemAtIndex:1] isSeparatorItem]); + EXPECT_TRUE([@"foo" isEqualToString:[[submenu1 itemAtIndex:2] title]]); + EXPECT_TRUE([@"bar" isEqualToString:[[submenu1 itemAtIndex:3] title]]); + + NSMenuItem* item3 = [menu itemAtIndex:2]; + MockBridge::HistoryItem* hist3 = bridge_->HistoryItemForMenuItem(item3); + EXPECT_TRUE(hist3); + EXPECT_EQ(42, hist3->session_id); + EXPECT_TRUE([@"Apple" isEqualToString:[item3 title]]); + + NSMenuItem* item4 = [menu itemAtIndex:3]; + MockBridge::HistoryItem* hist4 = bridge_->HistoryItemForMenuItem(item4); + EXPECT_TRUE(hist4); + EXPECT_EQ(50, hist4->session_id); + EXPECT_EQ(3U, hist4->tabs.size()); + // Do not test menu item title because it is localized. + NSMenu* submenu2 = [item4 submenu]; + EXPECT_EQ(5U, [[submenu2 itemArray] count]); + // Do not test Restore All Tabs because it is localiced. + EXPECT_TRUE([[submenu2 itemAtIndex:1] isSeparatorItem]); + EXPECT_TRUE([@"magic" isEqualToString:[[submenu2 itemAtIndex:2] title]]); + EXPECT_TRUE([@"goats" isEqualToString:[[submenu2 itemAtIndex:3] title]]); + EXPECT_TRUE([@"teleporter" isEqualToString:[[submenu2 itemAtIndex:4] title]]); +} + // Tests that we properly request an icon from the FaviconService. TEST_F(HistoryMenuBridgeTest, GetFaviconForHistoryItem) { - TestingProfile* profile = browser_test_helper_.profile(); - HistoryMenuBridge bridge(profile); - // Create a fake item. HistoryMenuBridge::HistoryItem item; item.title = ASCIIToUTF16("Title"); item.url = GURL("http://google.com"); // Request the icon. - GetFaviconForHistoryItem(&bridge, &item); + GetFaviconForHistoryItem(&item); // Make sure that there is ClientData for the request. std::vector<HistoryMenuBridge::HistoryItem*> data; - favicon_consumer(&bridge).GetAllClientData(&data); + favicon_consumer().GetAllClientData(&data); ASSERT_EQ(data.size(), 1U); EXPECT_EQ(&item, data[0]); @@ -213,9 +354,6 @@ TEST_F(HistoryMenuBridgeTest, GetFaviconForHistoryItem) { } TEST_F(HistoryMenuBridgeTest, GotFaviconData) { - TestingProfile* profile = browser_test_helper_.profile(); - HistoryMenuBridge bridge(profile); - // Create a dummy bitmap. SkBitmap bitmap; bitmap.setConfig(SkBitmap::kARGB_8888_Config, 25, 25); @@ -233,13 +371,15 @@ TEST_F(HistoryMenuBridgeTest, GotFaviconData) { HistoryMenuBridge::HistoryItem item; scoped_nsobject<NSMenuItem> menu_item([[NSMenuItem alloc] init]); item.menu_item = menu_item.get(); - GetFaviconForHistoryItem(&bridge, &item); + GetFaviconForHistoryItem(&item); // Pretend to be called back. - GotFaviconData(&bridge, item.icon_handle, true, bytes, false, GURL()); + GotFaviconData(item.icon_handle, true, bytes, false, GURL()); // Make sure the callback works. EXPECT_EQ(false, item.icon_requested); EXPECT_TRUE(item.icon.get()); EXPECT_TRUE([item.menu_item image]); } + +} // namespace diff --git a/chrome/browser/cocoa/history_menu_cocoa_controller.h b/chrome/browser/cocoa/history_menu_cocoa_controller.h index beb5576..efe7645 100644 --- a/chrome/browser/cocoa/history_menu_cocoa_controller.h +++ b/chrome/browser/cocoa/history_menu_cocoa_controller.h @@ -25,7 +25,6 @@ @end // HistoryMenuCocoaController @interface HistoryMenuCocoaController (ExposedForUnitTests) -- (const HistoryMenuBridge::HistoryItem*)itemForTag:(NSInteger)tag; - (void)openURLForItem:(const HistoryMenuBridge::HistoryItem*)node; @end // HistoryMenuCocoaController (ExposedForUnitTests) diff --git a/chrome/browser/cocoa/history_menu_cocoa_controller.mm b/chrome/browser/cocoa/history_menu_cocoa_controller.mm index a0672b3..8e3f672 100644 --- a/chrome/browser/cocoa/history_menu_cocoa_controller.mm +++ b/chrome/browser/cocoa/history_menu_cocoa_controller.mm @@ -11,6 +11,7 @@ #include "chrome/browser/cocoa/event_utils.h" #include "chrome/browser/history/history.h" #include "chrome/browser/history/history_types.h" +#include "chrome/browser/profile.h" #include "chrome/browser/tab_contents/tab_contents.h" #include "webkit/glue/window_open_disposition.h" @@ -29,34 +30,22 @@ Browser* browser = Browser::GetOrCreateTabbedBrowser(bridge_->profile()); WindowOpenDisposition disposition = event_utils::WindowOpenDispositionFromNSEvent([NSApp currentEvent]); - browser->OpenURL(node->url, GURL(), disposition, - PageTransition::AUTO_BOOKMARK); -} -- (const HistoryMenuBridge::HistoryItem*)itemForTag:(NSInteger)tag { - const ScopedVector<HistoryMenuBridge::HistoryItem>* results = NULL; - NSInteger tag_base = 0; - if (tag > IDC_HISTORY_MENU_VISITED && tag < IDC_HISTORY_MENU_CLOSED) { - results = bridge_->visited_results(); - tag_base = IDC_HISTORY_MENU_VISITED; - } else if (tag > IDC_HISTORY_MENU_CLOSED) { - results = bridge_->closed_results(); - tag_base = IDC_HISTORY_MENU_CLOSED; + // If this item can be restored using TabRestoreService, do so. Otherwise, + // just load the URL. + TabRestoreService* service = bridge_->profile()->GetTabRestoreService(); + if (node->session_id && service) { + service->RestoreEntryById(browser, node->session_id, false); } else { - NOTREACHED(); + DCHECK(node->url.is_valid()); + browser->OpenURL(node->url, GURL(), disposition, + PageTransition::AUTO_BOOKMARK); } - - DCHECK(tag > tag_base); - size_t index = tag - tag_base - 1; - if (index >= results->size()) - return NULL; - return (*results)[index]; } - (IBAction)openHistoryMenuItem:(id)sender { - NSInteger tag = [sender tag]; - const HistoryMenuBridge::HistoryItem* item = [self itemForTag:tag]; - DCHECK(item->url.is_valid()); + const HistoryMenuBridge::HistoryItem* item = + bridge_->HistoryItemForMenuItem(sender); [self openURLForItem:item]; } diff --git a/chrome/browser/cocoa/history_menu_cocoa_controller_unittest.mm b/chrome/browser/cocoa/history_menu_cocoa_controller_unittest.mm index 9f687f4..a57e12b 100644 --- a/chrome/browser/cocoa/history_menu_cocoa_controller_unittest.mm +++ b/chrome/browser/cocoa/history_menu_cocoa_controller_unittest.mm @@ -2,11 +2,14 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include "base/scoped_vector.h" +#include "base/scoped_nsobject.h" +#include "base/scoped_ptr.h" #include "base/string_util.h" #include "base/sys_string_conversions.h" #include "chrome/app/chrome_dll_resource.h" #include "chrome/browser/browser.h" +#include "chrome/browser/cocoa/browser_test_helper.h" +#include "chrome/browser/cocoa/cocoa_test_helper.h" #include "chrome/browser/cocoa/history_menu_bridge.h" #include "chrome/browser/cocoa/history_menu_cocoa_controller.h" #include "testing/gtest/include/gtest/gtest.h" @@ -14,13 +17,12 @@ @interface FakeHistoryMenuController : HistoryMenuCocoaController { @public BOOL opened_[2]; - ScopedVector<HistoryMenuBridge::HistoryItem> items_; } @end @implementation FakeHistoryMenuController -- (id)init { +- (id)initTest { if ((self = [super init])) { opened_[0] = NO; opened_[1] = NO; @@ -28,40 +30,62 @@ return self; } -- (HistoryMenuBridge::HistoryItem*)itemForTag:(NSInteger)tag { - HistoryMenuBridge::HistoryItem* item = new HistoryMenuBridge::HistoryItem(); - if (tag == 0) { - item->title = ASCIIToUTF16("uno"); +- (void)openURLForItem:(HistoryMenuBridge::HistoryItem*)item { + opened_[item->session_id] = YES; +} + +@end // FakeHistoryMenuController + +class HistoryMenuCocoaControllerTest : public CocoaTest { + public: + + virtual void SetUp() { + CocoaTest::SetUp(); + bridge_.reset(new HistoryMenuBridge(browser_test_helper_.profile())); + bridge_->controller_.reset( + [[FakeHistoryMenuController alloc] initWithBridge:bridge_.get()]); + [controller() initTest]; + } + + void CreateItems(NSMenu* menu) { + HistoryMenuBridge::HistoryItem* item = new HistoryMenuBridge::HistoryItem(); item->url = GURL("http://google.com"); - } else if (tag == 1) { - item->title = ASCIIToUTF16("duo"); + item->session_id = 0; + bridge_->AddItemToMenu(item, menu, HistoryMenuBridge::kMostVisited, 0); + + item = new HistoryMenuBridge::HistoryItem(); item->url = GURL("http://apple.com"); - } else { - NOTREACHED(); + item->session_id = 1; + bridge_->AddItemToMenu(item, menu, HistoryMenuBridge::kMostVisited, 1); } - // We push the item into a scoped vector that will delete it on destruction. - items_.push_back(item); - return item; -} -- (void)openURLForItem:(HistoryMenuBridge::HistoryItem*)item { - std::string url = item->url.possibly_invalid_spec(); - if (url.find("http://google.com") != std::string::npos) - opened_[0] = YES; - if (url.find("http://apple.com") != std::string::npos) - opened_[1] = YES; -} + std::map<NSMenuItem*, HistoryMenuBridge::HistoryItem*>& menu_item_map() { + return bridge_->menu_item_map_; + } -@end // FakeHistoryMenuController + FakeHistoryMenuController* controller() { + return static_cast<FakeHistoryMenuController*>(bridge_->controller_.get()); + } + + private: + BrowserTestHelper browser_test_helper_; + scoped_ptr<HistoryMenuBridge> bridge_; +}; + +TEST_F(HistoryMenuCocoaControllerTest, OpenURLForItem) { + + scoped_nsobject<NSMenu> menu([[NSMenu alloc] initWithTitle:@"History"]); + CreateItems(menu.get()); + + std::map<NSMenuItem*, HistoryMenuBridge::HistoryItem*>& items = + menu_item_map(); + std::map<NSMenuItem*, HistoryMenuBridge::HistoryItem*>::iterator it = + items.begin(); -TEST(HistoryMenuCocoaControllerTest, OpenURLForItem) { - FakeHistoryMenuController* c = [[FakeHistoryMenuController alloc] init]; - NSMenuItem* item = [[[NSMenuItem alloc] init] autorelease]; - for (int i = 0; i < 2; ++i) { - [item setTag:i]; - ASSERT_EQ(c->opened_[i], NO); - [c openHistoryMenuItem:item]; - ASSERT_EQ(c->opened_[i], YES); + for ( ; it != items.end(); ++it) { + HistoryMenuBridge::HistoryItem* item = it->second; + EXPECT_FALSE(controller()->opened_[item->session_id]); + [controller() openHistoryMenuItem:it->first]; + EXPECT_TRUE(controller()->opened_[item->session_id]); } - [c release]; } diff --git a/chrome/browser/sessions/tab_restore_service.h b/chrome/browser/sessions/tab_restore_service.h index 229557f..9e81722 100644 --- a/chrome/browser/sessions/tab_restore_service.h +++ b/chrome/browser/sessions/tab_restore_service.h @@ -147,7 +147,7 @@ class TabRestoreService : public BaseSessionService { // Returns the entries, ordered with most recently closed entries at the // front. - const Entries& entries() const { return entries_; } + virtual const Entries& entries() const { return entries_; } // Restores the most recently closed entry. Does nothing if there are no // entries to restore. If the most recently restored entry is a tab, it is @@ -172,6 +172,8 @@ class TabRestoreService : public BaseSessionService { protected: virtual void Save(); + virtual ~TabRestoreService(); + private: // Used to indicate what has loaded. enum LoadState { @@ -191,8 +193,6 @@ class TabRestoreService : public BaseSessionService { LOADED_LAST_SESSION = 1 << 4 }; - virtual ~TabRestoreService(); - // Populates the tab's navigations from the NavigationController, and its // browser_id and tabstrip_index from the browser. void PopulateTab(Tab* tab, |