From 6a4f5af29737b966d1cb9e0ab149d7cd0055e5fd Mon Sep 17 00:00:00 2001 From: "thakis@chromium.org" Date: Wed, 23 Sep 2009 22:43:00 +0000 Subject: Let cmd-f/cmd-g use the findboard. In a nutshell, this means that the find bars honor the global find pasteboard, which is like a clipboard, but for searches. See the TEST section below for consequences, and also see the bug for more information. BUG=14562 TEST= * Select some text, hit cmd-e, cmd-g. This should search for the marked text and open the find bar if it's not open. * Open TextEdit, hit cmd-f. Enter some text, hit enter. Switch back to Chrome with an open find bar. The find bar should now contain the text you entered in TextEdit * Enter different text into chrome's find bar, switch back to TextEdit. Its find window should now contain the new text. * Search for something in one tab, switch to another tab. It should contain the same text in the findbar as the first one. * Open the findbar, select some text, hit cmd-e. The find bar should be updated with the selected text and this text should be highlighted in the web page. Find bars in other tabs should be updated with that text as well. Review URL: http://codereview.chromium.org/206035 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@27015 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/browser/browser.cc | 11 +- chrome/browser/cocoa/find_bar_bridge.mm | 4 +- chrome/browser/cocoa/find_bar_cocoa_controller.h | 2 +- chrome/browser/cocoa/find_bar_cocoa_controller.mm | 32 ++++-- .../cocoa/find_bar_cocoa_controller_unittest.mm | 46 +++++++-- chrome/browser/cocoa/find_pasteboard.h | 57 +++++++++++ chrome/browser/cocoa/find_pasteboard.mm | 82 +++++++++++++++ chrome/browser/cocoa/find_pasteboard_unittest.mm | 111 +++++++++++++++++++++ chrome/browser/find_bar_controller.cc | 6 ++ .../renderer_host/resource_message_filter_mac.mm | 21 +++- 10 files changed, 351 insertions(+), 21 deletions(-) create mode 100644 chrome/browser/cocoa/find_pasteboard.h create mode 100644 chrome/browser/cocoa/find_pasteboard.mm create mode 100644 chrome/browser/cocoa/find_pasteboard_unittest.mm (limited to 'chrome/browser') diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc index 960badf..90a4e59 100644 --- a/chrome/browser/browser.cc +++ b/chrome/browser/browser.cc @@ -91,6 +91,10 @@ #include "chrome/browser/dock_info.h" #endif +#if defined(OS_MACOSX) +#include "chrome/browser/cocoa/find_pasteboard.h" +#endif + using base::TimeDelta; // How long we wait before updating the browser chrome while loading a page. @@ -2888,7 +2892,12 @@ GURL Browser::GetHomePage() const { void Browser::FindInPage(bool find_next, bool forward_direction) { ShowFindBar(); if (find_next) { - GetSelectedTabContents()->StartFinding(string16(), + string16 find_text; +#if defined(OS_MACOSX) + // We always want to search for the contents of the find pasteboard on OS X. + find_text = GetFindPboardText(); +#endif + GetSelectedTabContents()->StartFinding(find_text, forward_direction, false); // Not case sensitive. } diff --git a/chrome/browser/cocoa/find_bar_bridge.mm b/chrome/browser/cocoa/find_bar_bridge.mm index 7c1ecca..cd7733c 100644 --- a/chrome/browser/cocoa/find_bar_bridge.mm +++ b/chrome/browser/cocoa/find_bar_bridge.mm @@ -3,6 +3,8 @@ // found in the LICENSE file. #import "chrome/browser/cocoa/find_bar_bridge.h" + +#include "base/sys_string_conversions.h" #import "chrome/browser/cocoa/find_bar_cocoa_controller.h" FindBarBridge::FindBarBridge() { @@ -30,7 +32,7 @@ void FindBarBridge::ClearResults(const FindNotificationDetails& results) { } void FindBarBridge::SetFindText(const string16& find_text) { - [cocoa_controller_ setFindText:find_text]; + [cocoa_controller_ setFindText:base::SysUTF16ToNSString(find_text)]; } void FindBarBridge::UpdateUIForFindResult(const FindNotificationDetails& result, diff --git a/chrome/browser/cocoa/find_bar_cocoa_controller.h b/chrome/browser/cocoa/find_bar_cocoa_controller.h index 71ff248..523db10 100644 --- a/chrome/browser/cocoa/find_bar_cocoa_controller.h +++ b/chrome/browser/cocoa/find_bar_cocoa_controller.h @@ -62,7 +62,7 @@ class FindNotificationDetails; - (void)stopAnimation; - (void)setFocusAndSelection; - (void)restoreSavedFocus; -- (void)setFindText:(const string16&)findText; +- (void)setFindText:(NSString*)findText; - (void)clearResults:(const FindNotificationDetails&)results; - (void)updateUIForFindResult:(const FindNotificationDetails&)results diff --git a/chrome/browser/cocoa/find_bar_cocoa_controller.mm b/chrome/browser/cocoa/find_bar_cocoa_controller.mm index 35274e9..265483e 100644 --- a/chrome/browser/cocoa/find_bar_cocoa_controller.mm +++ b/chrome/browser/cocoa/find_bar_cocoa_controller.mm @@ -12,6 +12,7 @@ #include "chrome/browser/cocoa/browser_window_cocoa.h" #import "chrome/browser/cocoa/find_bar_cocoa_controller.h" #import "chrome/browser/cocoa/find_bar_bridge.h" +#import "chrome/browser/cocoa/find_pasteboard.h" #import "chrome/browser/cocoa/focus_tracker.h" #import "chrome/browser/cocoa/tab_strip_controller.h" #include "chrome/browser/tab_contents/tab_contents.h" @@ -36,6 +37,11 @@ static float kFindBarCloseDuration = 0.15; - (id)init { if ((self = [super initWithNibName:@"FindBar" bundle:mac_util::MainAppBundle()])) { + [[NSNotificationCenter defaultCenter] + addObserver:self + selector:@selector(findPboardUpdated:) + name:kFindPasteboardChangedNotification + object:[FindPasteboard sharedInstance]]; } return self; } @@ -44,6 +50,7 @@ static float kFindBarCloseDuration = 0.15; // All animations should be explicitly stopped by the TabContents before a tab // is closed. DCHECK(!currentAnimation_.get()); + [[NSNotificationCenter defaultCenter] removeObserver:self]; [super dealloc]; } @@ -75,6 +82,10 @@ static float kFindBarCloseDuration = 0.15; true, false); } +- (void)findPboardUpdated:(NSNotification*)notification { + [self setFindText:[[FindPasteboard sharedInstance] findText]]; +} + // Positions the find bar container view in the correct location based on the // current state of the window. The find bar container is always positioned one // pixel above the infobar container. Note that we are using the infobar @@ -110,14 +121,16 @@ static float kFindBarCloseDuration = 0.15; if (!tab_contents) return; - string16 findText = base::SysNSStringToUTF16([findText_ stringValue]); - if (findText.length() > 0) { - tab_contents->StartFinding(findText, true, false); + NSString* findText = [findText_ stringValue]; + [[FindPasteboard sharedInstance] setFindText:findText]; + + if ([findText length] > 0) { + tab_contents->StartFinding(base::SysNSStringToUTF16(findText), true, false); } else { // The textbox is empty so we reset. tab_contents->StopFinding(true); // true = clear selection on page. [self updateUIForFindResult:tab_contents->find_result() - withText:string16()]; + withText:string16()]; } } @@ -210,8 +223,12 @@ static float kFindBarCloseDuration = 0.15; focusTracker_.reset(nil); } -- (void)setFindText:(const string16&)findText { - [findText_ setStringValue:base::SysUTF16ToNSString(findText)]; +- (void)setFindText:(NSString*)findText { + [findText_ setStringValue:findText]; + + // Make sure the text in the find bar always ends up in the find pasteboard + // (and, via notifications, in the other find bars too). + [[FindPasteboard sharedInstance] setFindText:findText]; } - (void)clearResults:(const FindNotificationDetails&)results { @@ -266,7 +283,8 @@ static float kFindBarCloseDuration = 0.15; [resultsLabel_ setFrame:labelFrame]; // TODO(rohitrao): If the search string is too long, then it will overlap with - // the results label. Fix. + // the results label. Fix. Perhaps use the code that fades out the tab titles + // if they are too long. } - (BOOL)isFindBarVisible { diff --git a/chrome/browser/cocoa/find_bar_cocoa_controller_unittest.mm b/chrome/browser/cocoa/find_bar_cocoa_controller_unittest.mm index 575c3f7..5b48350 100644 --- a/chrome/browser/cocoa/find_bar_cocoa_controller_unittest.mm +++ b/chrome/browser/cocoa/find_bar_cocoa_controller_unittest.mm @@ -12,13 +12,15 @@ #include "chrome/browser/find_notification_details.h" #import "chrome/browser/cocoa/cocoa_test_helper.h" #import "chrome/browser/cocoa/find_bar_cocoa_controller.h" +#import "chrome/browser/cocoa/find_pasteboard.h" #include "testing/gtest/include/gtest/gtest.h" #include "testing/platform_test.h" // Expose private variables to make testing easier. @interface FindBarCocoaController(Testing) - (NSView*)findBarView; -- (NSTextField*)findText; +- (NSString*)findText; +- (NSTextField*)findTextField; - (NSTextField*)resultsLabel; @end @@ -27,7 +29,11 @@ return findBarView_; } -- (NSTextField*)findText { +- (NSString*)findText { + return [findText_ stringValue]; +} + +- (NSTextField*)findTextField { return findText_; } @@ -70,18 +76,18 @@ TEST_F(FindBarCocoaControllerTest, ShowAndHide) { } TEST_F(FindBarCocoaControllerTest, SetFindText) { - NSTextField* findText = [controller_ findText]; + NSTextField* findTextField = [controller_ findTextField]; // Start by making the find bar visible. [controller_ showFindBar:NO]; EXPECT_TRUE([controller_ isFindBarVisible]); // Set the find text. - const std::string kFindText = "Google"; - [controller_ setFindText:ASCIIToUTF16(kFindText)]; + const NSString* kFindText = @"Google"; + [controller_ setFindText:kFindText]; EXPECT_EQ( NSOrderedSame, - [[findText stringValue] compare:base::SysUTF8ToNSString(kFindText)]); + [[findTextField stringValue] compare:kFindText]); // Call clearResults, which doesn't actually clear the find text but // simply sets it back to what it was before. This is silly, but @@ -92,7 +98,7 @@ TEST_F(FindBarCocoaControllerTest, SetFindText) { [controller_ clearResults:details]; EXPECT_EQ( NSOrderedSame, - [[findText stringValue] compare:base::SysUTF8ToNSString(kFindText)]); + [[findTextField stringValue] compare:kFindText]); } TEST_F(FindBarCocoaControllerTest, ResultLabelUpdatesCorrectly) { @@ -100,4 +106,30 @@ TEST_F(FindBarCocoaControllerTest, ResultLabelUpdatesCorrectly) { // FindNotificationDetails objects. } +TEST_F(FindBarCocoaControllerTest, FindTextIsGlobal) { + scoped_nsobject otherController( + [[FindBarCocoaController alloc] init]); + [helper_.contentView() addSubview:[otherController view]]; + + // Setting the text in one controller should update the other controller's + // text as well. + const NSString* kFindText = @"Respect to the man in the ice cream van"; + [controller_ setFindText:kFindText]; + EXPECT_EQ( + NSOrderedSame, + [[controller_ findText] compare:kFindText]); + EXPECT_EQ( + NSOrderedSame, + [[otherController.get() findText] compare:kFindText]); +} + +TEST_F(FindBarCocoaControllerTest, SettingFindTextUpdatesFindPboard) { + const NSString* kFindText = + @"It's not a bird, it's not a plane, it must be Dave who's on the train"; + [controller_ setFindText:kFindText]; + EXPECT_EQ( + NSOrderedSame, + [[[FindPasteboard sharedInstance] findText] compare:kFindText]); +} + } // namespace diff --git a/chrome/browser/cocoa/find_pasteboard.h b/chrome/browser/cocoa/find_pasteboard.h new file mode 100644 index 0000000..bf5d213 --- /dev/null +++ b/chrome/browser/cocoa/find_pasteboard.h @@ -0,0 +1,57 @@ +// Copyright (c) 2009 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_COCOA_FIND_PASTEBOARD_H_ +#define CHROME_BROWSER_COCOA_FIND_PASTEBOARD_H_ + +#include "base/string16.h" + +#ifdef __OBJC__ + +#import + +#include "base/scoped_nsobject.h" + +extern NSString* kFindPasteboardChangedNotification; + +// Manages the find pasteboard. Use this to copy text to the find pasteboard, +// to get the text currently on the find pasteboard, and to receive +// notifications when the text on the find pasteboard has changed. You should +// always use this class instead of accessing +// [NSPasteboard pasteboardWithName:NSFindPboard] directly. +// +// This is not thread-safe and must be used on the main thread. +// +// This is supposed to be a singleton. +@interface FindPasteboard : NSObject { + @private + scoped_nsobject findText_; +} + +// Returns the singleton instance of this class. ++ (FindPasteboard*)sharedInstance; + +// Returns the current find text. This is never nil; if there is no text on the +// find pasteboard, this returns an empty string. +- (NSString*)findText; + +// Sets the current find text to |newText| and sends a +// |kFindPasteboardChangedNotification| to the default notification center if +// it the new text different from the current text. |newText| must not be nil. +- (void)setFindText:(NSString*)newText; +@end + +@interface FindPasteboard (TestingAPI) +- (void)loadTextFromPasteboard:(NSNotification*)notification; + +// This methods is meant to be overridden in tests. +- (NSPasteboard*)findPboard; +@end + +#endif // __OBJC__ + +// Also provide a c++ interface +string16 GetFindPboardText(); + +#endif // CHROME_BROWSER_COCOA_FIND_PASTEBOARD_H_ diff --git a/chrome/browser/cocoa/find_pasteboard.mm b/chrome/browser/cocoa/find_pasteboard.mm new file mode 100644 index 0000000..0d081d0 --- /dev/null +++ b/chrome/browser/cocoa/find_pasteboard.mm @@ -0,0 +1,82 @@ +// Copyright (c) 2009 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. + +#import "chrome/browser/cocoa/find_pasteboard.h" + +#include "base/logging.h" +#include "base/sys_string_conversions.h" + +NSString* kFindPasteboardChangedNotification = + @"kFindPasteboardChangedNotification_Chrome"; + +@implementation FindPasteboard + ++ (FindPasteboard*)sharedInstance { + static FindPasteboard* instance = nil; + if (!instance) { + instance = [[FindPasteboard alloc] init]; + } + return instance; +} + +- (id)init { + if ((self = [super init])) { + findText_.reset([[NSString alloc] init]); + + // Check if the text in the findboard has changed on app activate. + [[NSNotificationCenter defaultCenter] + addObserver:self + selector:@selector(loadTextFromPasteboard:) + name:NSApplicationDidBecomeActiveNotification + object:nil]; + [self loadTextFromPasteboard:nil]; + } + return self; +} + +- (void)dealloc { + // Since this is a singleton, this should only be executed in test code. + [[NSNotificationCenter defaultCenter] removeObserver:self]; + [super dealloc]; +} + +- (NSPasteboard*)findPboard { + return [NSPasteboard pasteboardWithName:NSFindPboard]; +} + +- (void)loadTextFromPasteboard:(NSNotification*)notification { + NSPasteboard* findPboard = [self findPboard]; + if ([[findPboard types] containsObject:NSStringPboardType]) + [self setFindText:[findPboard stringForType:NSStringPboardType]]; +} + +- (NSString*)findText { + return findText_; +} + +- (void)setFindText:(NSString*)newText { + DCHECK(newText); + if (!newText) + return; + + DCHECK([NSThread isMainThread]); + + BOOL needToSendNotification = ![findText_.get() isEqualToString:newText]; + if (needToSendNotification) { + findText_.reset([newText copy]); + NSPasteboard* findPboard = [self findPboard]; + [findPboard declareTypes:[NSArray arrayWithObject:NSStringPboardType] + owner:nil]; + [findPboard setString:findText_.get() forType:NSStringPboardType]; + [[NSNotificationCenter defaultCenter] + postNotificationName:kFindPasteboardChangedNotification + object:self]; + } +} + +@end + +string16 GetFindPboardText() { + return base::SysNSStringToUTF16([[FindPasteboard sharedInstance] findText]); +} diff --git a/chrome/browser/cocoa/find_pasteboard_unittest.mm b/chrome/browser/cocoa/find_pasteboard_unittest.mm new file mode 100644 index 0000000..643d95c --- /dev/null +++ b/chrome/browser/cocoa/find_pasteboard_unittest.mm @@ -0,0 +1,111 @@ +// Copyright (c) 2009 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. + +#import + +#include "base/scoped_nsobject.h" +#import "chrome/browser/cocoa/find_pasteboard.h" +#import "chrome/browser/cocoa/cocoa_test_helper.h" +#include "testing/gtest/include/gtest/gtest.h" +#include "testing/platform_test.h" + +// A subclass of FindPasteboard that doesn't write to the real find pasteboard. +@interface FindPasteboardTesting : FindPasteboard { + @public + int notificationCount_; + @private + scoped_nsobject pboard_; +} +- (NSPasteboard*)findPboard; + +- (void)callback:(id)sender; + +// These are for checking that pasteboard content is copied to/from the +// FindPasteboard correctly. +- (NSString*)findPboardText; +- (void)setFindPboardText:(NSString*)text; +@end + +@implementation FindPasteboardTesting + +- (id)init { + if ((self = [super init])) { + pboard_.reset([[NSPasteboard pasteboardWithUniqueName] retain]); + } + return self; +} + +- (NSPasteboard*)findPboard { + return pboard_.get(); +} + +- (void)callback:(id)sender { + ++notificationCount_; +} + +- (void)setFindPboardText:(NSString*)text { + [pboard_.get() declareTypes:[NSArray arrayWithObject:NSStringPboardType] + owner:nil]; + [pboard_.get() setString:text forType:NSStringPboardType]; +} + +- (NSString*)findPboardText { + return [pboard_.get() stringForType:NSStringPboardType]; +} +@end + +namespace { + +class FindPasteboardTest : public PlatformTest { + public: + FindPasteboardTest() { + pboard_.reset([[FindPasteboardTesting alloc] init]); + } + protected: + scoped_nsobject pboard_; + CocoaTestHelper helper_; +}; + +TEST_F(FindPasteboardTest, SettingTextUpdatesPboard) { + [pboard_.get() setFindText:@"text"]; + EXPECT_EQ( + NSOrderedSame, + [[pboard_.get() findPboardText] compare:@"text"]); +} + +TEST_F(FindPasteboardTest, ReadingFromPboardUpdatesFindText) { + [pboard_.get() setFindPboardText:@"text"]; + [pboard_.get() loadTextFromPasteboard:nil]; + EXPECT_EQ( + NSOrderedSame, + [[pboard_.get() findText] compare:@"text"]); +} + +TEST_F(FindPasteboardTest, SendsNotificationWhenTextChanges) { + [[NSNotificationCenter defaultCenter] + addObserver:pboard_.get() + selector:@selector(callback:) + name:kFindPasteboardChangedNotification + object:pboard_.get()]; + EXPECT_EQ(0, pboard_.get()->notificationCount_); + [pboard_.get() setFindText:@"text"]; + EXPECT_EQ(1, pboard_.get()->notificationCount_); + [pboard_.get() setFindText:@"text"]; + EXPECT_EQ(1, pboard_.get()->notificationCount_); + [pboard_.get() setFindText:@"other text"]; + EXPECT_EQ(2, pboard_.get()->notificationCount_); + + [pboard_.get() setFindPboardText:@"other text"]; + [pboard_.get() loadTextFromPasteboard:nil]; + EXPECT_EQ(2, pboard_.get()->notificationCount_); + + [pboard_.get() setFindPboardText:@"otherer text"]; + [pboard_.get() loadTextFromPasteboard:nil]; + EXPECT_EQ(3, pboard_.get()->notificationCount_); + + [[NSNotificationCenter defaultCenter] removeObserver:pboard_.get()]; +} + + +} // namespace diff --git a/chrome/browser/find_bar_controller.cc b/chrome/browser/find_bar_controller.cc index 17fb22c..bef7273 100644 --- a/chrome/browser/find_bar_controller.cc +++ b/chrome/browser/find_bar_controller.cc @@ -75,6 +75,7 @@ void FindBarController::ChangeTabContents(TabContents* contents) { registrar_.Add(this, NotificationType::NAV_ENTRY_COMMITTED, Source(&tab_contents_->controller())); +#if !defined(OS_MACOSX) // Find out what we should show in the find text box. Usually, this will be // the last search in this tab, but if no search has been issued in this tab // we use the last search string (from any tab). @@ -88,6 +89,11 @@ void FindBarController::ChangeTabContents(TabContents* contents) { // _first_ since the FindBarView checks its emptiness to see if it should // clear the result count display when there's nothing in the box. find_bar_->SetFindText(find_string); +#else + // Having a per-tab find_string is not compatible with OS X's find pasteboard, + // so we always have the same find text in all find bars. This is done through + // the find pasteboard mechanism, so don't set the text here. +#endif if (tab_contents_->find_ui_active()) { // A tab with a visible find bar just got selected and we need to show the diff --git a/chrome/browser/renderer_host/resource_message_filter_mac.mm b/chrome/browser/renderer_host/resource_message_filter_mac.mm index b63c5bf..f7687c9 100644 --- a/chrome/browser/renderer_host/resource_message_filter_mac.mm +++ b/chrome/browser/renderer_host/resource_message_filter_mac.mm @@ -6,23 +6,36 @@ #import +#include "base/message_loop.h" #include "base/sys_string_conversions.h" +#import "chrome/browser/cocoa/find_pasteboard.h" // The number of utf16 code units that will be written to the find pasteboard, // longer texts are silently ignored. This is to prevent that a compromised // renderer can write unlimited amounts of data into the find pasteboard. static const size_t kMaxFindPboardStringLength = 4096; +class WriteFindPboardTask : public Task { + public: + explicit WriteFindPboardTask(NSString* text) + : text_([text retain]) {} + + void Run() { + [[FindPasteboard sharedInstance] setFindText:text_]; + } + + private: + scoped_nsobject text_; +}; + // Called on the IO thread. void ResourceMessageFilter::OnClipboardFindPboardWriteString( const string16& text) { if (text.length() <= kMaxFindPboardStringLength) { NSString* nsText = base::SysUTF16ToNSString(text); if (nsText) { - NSPasteboard* findPboard = [NSPasteboard pasteboardWithName:NSFindPboard]; - [findPboard declareTypes:[NSArray arrayWithObject:NSStringPboardType] - owner:nil]; - [findPboard setString:nsText forType:NSStringPboardType]; + // FindPasteboard must be used on the UI thread. + ui_loop()->PostTask(FROM_HERE, new WriteFindPboardTask(nsText)); } } } -- cgit v1.1