Support the OS X find pasteboard on OS X.

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.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=27015

[Nico, 2009-09-17 21:25:42]

Rohitrao: You wrote the mac findbar, so take a look at that.
Pink: You reviewed the cmd-e part of this, so this might be interesting. But you
can treat this as FYI.
Finnur: Pink says you did the core findbar stuff, so please look at that.

[Nico, 2009-09-17 21:26:53]

"""Also, it looks like hitting the "find prev/next" buttons is not recorded in
UserMetrics. Is that intentional?""" was supposed to be in the mail, not in the
issue description, and is a question for Rohit :-)

[Nico, 2009-09-17 21:34:19]

I just checked, and both windows and gtk call tab_contents()->StartFinding()
directly on button press, so they don't record metrics in that case either.
Hence, the mac behavior is consistent with that.

[pink (ping after 24hrs), 2009-09-17 21:53:59]

http://codereview.chromium.org/206035/diff/8001/7020
File chrome/browser/cocoa/find_pasteboard.h (right):

http://codereview.chromium.org/206035/diff/8001/7020#newcode5
Line 5: #import <Cocoa/Cocoa.h>
this entire header should have a #ifndef guard around it.

http://codereview.chromium.org/206035/diff/8001/7020#newcode20
Line 20: @interface FindPasteboard : NSObject {
Add a TODO to unit test this as soon as we land OCMock so that we can correctly
mock out the find pasteboard w/out disturbing anything in the system. Or, better
yet, create a unit test file and put the TODO there :-)

http://codereview.chromium.org/206035/diff/8001/7020#newcode26
Line 26: + (FindPasteboard*)instance;
sharedInstance, as in other appkit classes.

http://codereview.chromium.org/206035/diff/8001/7021
File chrome/browser/cocoa/find_pasteboard.mm (right):

http://codereview.chromium.org/206035/diff/8001/7021#newcode13
Line 13: - (id)init;
not needed. this doesn't actually make it private and is redundant as init
already exists for all objects.

http://codereview.chromium.org/206035/diff/8001/7021#newcode19
Line 19: static FindPasteboard* instance = nil;
rather than making this in global file scope, make it a static variable of the
-sharedInstance method.

http://codereview.chromium.org/206035/diff/8001/7021#newcode23
Line 23: [[FindPasteboard alloc] init];
this should read like instance = [[FindPasteboard alloc] init];

http://codereview.chromium.org/206035/diff/8001/7021#newcode29
Line 29: if (instance) {
what is this doing?

http://codereview.chromium.org/206035/diff/8001/7021#newcode35
Line 35: instance = self;
do this in the sharedInstance method, not here.

http://codereview.chromium.org/206035/diff/8001/7021#newcode55
Line 55: 
omit blank line

http://codereview.chromium.org/206035/diff/8001/7024
File chrome/browser/tab_contents/tab_contents_view_mac.mm (right):

http://codereview.chromium.org/206035/diff/8001/7024#newcode351
Line 351: - (void)findPrevFromFindPasteboard:(id)sender {
i don't see why this has to be mac-specific and can't be in browser.cc

[Finnur, 2009-09-17 22:08:53]

Copying Ben as a token UX decider (Ben: see what I wrote at the bottom).

Before looking at the changelist I had a few questions/comments.

The changelist description is very terse (one line), hard to understand and not
in sync with the bug description. CL description talks about cmd-f and cmd-g but
bug description talks about cmd-e.

Overall, I'm worried supporting cmd-e might be ok short term but in conflict
with what we want long term. We have been talking about implementing a related
thing for Windows, where selecting something on the page and pressing Ctrl+F
would pre-populate the Find box with what is selected on the page. I guess my
question is, if we do that, how would that affect the Chrome Mac implementation?
Would it not prepopulate the Find box with what is selected? If if does
prepopulate, wouldn't it be better to just have one implementation that does
this for all platforms?

> Also, it looks like hitting the "find prev/next" buttons is not recorded in
UserMetrics. Is that intentional?

No, can't think of a reason why we would want that to not trigger a UMA record.


And now that I've looked into the changelist, this is 99% mac stuff that I have
no insight into. The remaining 1% brought one question to mind.

http://codereview.chromium.org/206035/diff/8001/7022
File chrome/browser/find_bar_controller.cc (right):

http://codereview.chromium.org/206035/diff/8001/7022#newcode95
Line 95: // the find pasteboard mechanism, so don't set the text here.
Wait... We purposely designed the Find box to be tab scoped. This is the first
time I've seen us diverge from that path (and only on one platform). This feels
like something we shouldn't just slip in without some discussion with the UX
team...

[Ben Goodger (Google), 2009-09-17 22:35:37]

I agree with Finnur. We should match the behavior on Windows. No need to diverge
here.

[Avi (use Gerrit), 2009-09-17 22:41:39]

On 2009/09/17 22:35:37, Ben Goodger wrote:
> I agree with Finnur. We should match the behavior on Windows. No need to
diverge
> here.

Ability to do cmd-e and then cmd-g is one of the biggest things that makes an
app Mac-like to me; the shared find pasteboard is huge. Can we find some kind of
compromise that allows chromium mac to be mac-y and still be reasonably
consistent cross-platform?

I'd like a list of what's OK and what's not OK with you.

If I go cmd-e in another app, switch to chromium and do cmd-g, is it OK if that
works?
If I go cmd-e in chromium and then cmd-g is it OK if that works?

I'm willing to tolerate cmd-f pre-populating the search field (though that's
really windows-y), but taking away common mac keycommands feels weird.

[rohitrao (ping after 24h), 2009-09-17 22:41:41]

On 2009/09/17 22:35:37, Ben Goodger wrote:
> I agree with Finnur. We should match the behavior on Windows. No need to
diverge
> here.

Enough people have bugged me about this that it's probably safe to call this
standard Mac behavior.

Cmd-E copies text to the findboard, Cmd-F opens the findbar and prepopulates it
with whatever is in the findboard, Cmd-G does a find next with whatever is in
the findboard.  When you press Cmd-E in one application, all of the other apps
update their findbars with the selected text.  I have yet to find a Mac app that
doesn't behave this way :)

[Nico, 2009-09-18 18:28:18]

After some conversations, I think we all seem to agree that the findbar contents
should be find pasteboard contents. I will send a mail to the UI team and make
sure they agree.

With this revision, our behavior for cmd-g is slightly different from other mac
apps (it opens the find bar), but we think that makes sense in our case – see
below for reasoning.

http://codereview.chromium.org/206035/diff/8001/7020
File chrome/browser/cocoa/find_pasteboard.h (right):

http://codereview.chromium.org/206035/diff/8001/7020#newcode5
Line 5: #import <Cocoa/Cocoa.h>
On 2009/09/17 21:53:59, pink wrote:
> this entire header should have a #ifndef guard around it.

Done.

http://codereview.chromium.org/206035/diff/8001/7020#newcode20
Line 20: @interface FindPasteboard : NSObject {
Done (the former).

http://codereview.chromium.org/206035/diff/8001/7020#newcode26
Line 26: + (FindPasteboard*)instance;
On 2009/09/17 21:53:59, pink wrote:
> sharedInstance, as in other appkit classes.

Done.

http://codereview.chromium.org/206035/diff/8001/7021
File chrome/browser/cocoa/find_pasteboard.mm (right):

http://codereview.chromium.org/206035/diff/8001/7021#newcode13
Line 13: - (id)init;
Done. Not sure why I put it there, it seems pretty stupid to me.

http://codereview.chromium.org/206035/diff/8001/7021#newcode19
Line 19: static FindPasteboard* instance = nil;
On 2009/09/17 21:53:59, pink wrote:
> rather than making this in global file scope, make it a static variable of the
> -sharedInstance method.

Done.

http://codereview.chromium.org/206035/diff/8001/7021#newcode23
Line 23: [[FindPasteboard alloc] init];
On 2009/09/17 21:53:59, pink wrote:
> this should read like instance = [[FindPasteboard alloc] init];

Done.

http://codereview.chromium.org/206035/diff/8001/7021#newcode29
Line 29: if (instance) {
On 2009/09/17 21:53:59, pink wrote:
> what is this doing?

If someone writes [[FindPasteboard alloc] init], this would return the singleton
instance an dealloc the unnecessary object.

I wanted to convert this to a DCHECK(!instance), but instance is now local to
sharedInstance…so I just removed the block and will hope that the callers will
always use sharedInstance.

http://codereview.chromium.org/206035/diff/8001/7021#newcode35
Line 35: instance = self;
On 2009/09/17 21:53:59, pink wrote:
> do this in the sharedInstance method, not here.

Done.

http://codereview.chromium.org/206035/diff/8001/7021#newcode55
Line 55: 
On 2009/09/17 21:53:59, pink wrote:
> omit blank line

Done.

http://codereview.chromium.org/206035/diff/8001/7024
File chrome/browser/tab_contents/tab_contents_view_mac.mm (right):

http://codereview.chromium.org/206035/diff/8001/7024#newcode351
Line 351: - (void)findPrevFromFindPasteboard:(id)sender {
On 2009/09/17 21:53:59, pink wrote:
> i don't see why this has to be mac-specific and can't be in browser.cc

My original comment was: Because it calls objc code. Also, "Find Next"/"Prev" in
main menu.nib now call this method on the responder chain, so i need some
instance in the chain to implement this method.

Then we talked about this in irc and we agreed that FindPasteboard should just
be a c++ class instead and this should happen in browser.cc.

I thought about this some more and come to the conclusion that having this in
browser.cc doesn't buy us all that much, but then I realized that hitting cmd-g
while the findbar is focussed doesn't work with my approach (contents not in the
responder chain in that case, it seems), so that invalidated my conclusions
again and I moved this to browser.cc. This has the consequence of showing the
findbar on cmd-g, which is not typical for mac apps (but not all that bad imo). 

After talking with finnur and pink on OS X, we agreed that this does make sense
for us however, because searching is more like a mode for us (we highlight all
hits e.g.), and opening the findbar makes it possible to leave that mode again.

[Finnur, 2009-09-18 18:31:59]

Let me know when you have buy-in for what you want to do from the UX team and
I'll take another look.


On 2009/09/18 18:28:18, Nico wrote:
> After some conversations, I think we all seem to agree that the findbar
contents
> should be find pasteboard contents. I will send a mail to the UI team and make
> sure they agree.
> 
> With this revision, our behavior for cmd-g is slightly different from other
mac
> apps (it opens the find bar), but we think that makes sense in our case – see
> below for reasoning.
> 
> http://codereview.chromium.org/206035/diff/8001/7020
> File chrome/browser/cocoa/find_pasteboard.h (right):
> 
> http://codereview.chromium.org/206035/diff/8001/7020#newcode5
> Line 5: #import <Cocoa/Cocoa.h>
> On 2009/09/17 21:53:59, pink wrote:
> > this entire header should have a #ifndef guard around it.
> 
> Done.
> 
> http://codereview.chromium.org/206035/diff/8001/7020#newcode20
> Line 20: @interface FindPasteboard : NSObject {
> Done (the former).
> 
> http://codereview.chromium.org/206035/diff/8001/7020#newcode26
> Line 26: + (FindPasteboard*)instance;
> On 2009/09/17 21:53:59, pink wrote:
> > sharedInstance, as in other appkit classes.
> 
> Done.
> 
> http://codereview.chromium.org/206035/diff/8001/7021
> File chrome/browser/cocoa/find_pasteboard.mm (right):
> 
> http://codereview.chromium.org/206035/diff/8001/7021#newcode13
> Line 13: - (id)init;
> Done. Not sure why I put it there, it seems pretty stupid to me.
> 
> http://codereview.chromium.org/206035/diff/8001/7021#newcode19
> Line 19: static FindPasteboard* instance = nil;
> On 2009/09/17 21:53:59, pink wrote:
> > rather than making this in global file scope, make it a static variable of
the
> > -sharedInstance method.
> 
> Done.
> 
> http://codereview.chromium.org/206035/diff/8001/7021#newcode23
> Line 23: [[FindPasteboard alloc] init];
> On 2009/09/17 21:53:59, pink wrote:
> > this should read like instance = [[FindPasteboard alloc] init];
> 
> Done.
> 
> http://codereview.chromium.org/206035/diff/8001/7021#newcode29
> Line 29: if (instance) {
> On 2009/09/17 21:53:59, pink wrote:
> > what is this doing?
> 
> If someone writes [[FindPasteboard alloc] init], this would return the
singleton
> instance an dealloc the unnecessary object.
> 
> I wanted to convert this to a DCHECK(!instance), but instance is now local to
> sharedInstance…so I just removed the block and will hope that the callers will
> always use sharedInstance.
> 
> http://codereview.chromium.org/206035/diff/8001/7021#newcode35
> Line 35: instance = self;
> On 2009/09/17 21:53:59, pink wrote:
> > do this in the sharedInstance method, not here.
> 
> Done.
> 
> http://codereview.chromium.org/206035/diff/8001/7021#newcode55
> Line 55: 
> On 2009/09/17 21:53:59, pink wrote:
> > omit blank line
> 
> Done.
> 
> http://codereview.chromium.org/206035/diff/8001/7024
> File chrome/browser/tab_contents/tab_contents_view_mac.mm (right):
> 
> http://codereview.chromium.org/206035/diff/8001/7024#newcode351
> Line 351: - (void)findPrevFromFindPasteboard:(id)sender {
> On 2009/09/17 21:53:59, pink wrote:
> > i don't see why this has to be mac-specific and can't be in browser.cc
> 
> My original comment was: Because it calls objc code. Also, "Find Next"/"Prev"
in
> main menu.nib now call this method on the responder chain, so i need some
> instance in the chain to implement this method.
> 
> Then we talked about this in irc and we agreed that FindPasteboard should just
> be a c++ class instead and this should happen in browser.cc.
> 
> I thought about this some more and come to the conclusion that having this in
> browser.cc doesn't buy us all that much, but then I realized that hitting
cmd-g
> while the findbar is focussed doesn't work with my approach (contents not in
the
> responder chain in that case, it seems), so that invalidated my conclusions
> again and I moved this to browser.cc. This has the consequence of showing the
> findbar on cmd-g, which is not typical for mac apps (but not all that bad
imo). 
> 
> After talking with finnur and pink on OS X, we agreed that this does make
sense
> for us however, because searching is more like a mode for us (we highlight all
> hits e.g.), and opening the findbar makes it possible to leave that mode
again.

[Nico, 2009-09-23 15:26:56]

Obtained buy-in from UX team. Please take another look.

[Finnur, 2009-09-23 15:52:24]

I have just nits and couple of questions:

Your changelist description doesn't mention whether the Find bar should open
when you press Cmd+G. This was discussed at some point, and I think the
consensus was to always open it on FindNext. Correct?

Also, there are no new tests added for this new Mac specific behavior. This is
exactly the kind of things that developers on other platforms will overlook and
potentially break when making changes to the Find functionality. Is it possible
to add tests for this?

http://codereview.chromium.org/206035/diff/13001/14001
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/206035/diff/13001/14001#newcode2903
Line 2903: }
Don't need the extra newline before }.

http://codereview.chromium.org/206035/diff/13001/14007
File chrome/browser/cocoa/find_pasteboard.mm (right):

http://codereview.chromium.org/206035/diff/13001/14007#newcode43
Line 43: DCHECK(false);  // This should never be deleted.
drive-by (don't feel qualified to review the mac code specifically, I just
noticed this off-hand):  out of curiosity, why should this never be deleted?
Does this warrant further explanation in a comment?

[Nico, 2009-09-23 16:53:23]

Thanks for the review, especially for taking care that the CL description is
up-to-date.

Updated CL description and added a few tests.

http://codereview.chromium.org/206035/diff/13001/14001
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/206035/diff/13001/14001#newcode2903
Line 2903: }
On 2009/09/23 15:52:24, Finnur wrote:
> Don't need the extra newline before }.

Done.

http://codereview.chromium.org/206035/diff/13001/14007
File chrome/browser/cocoa/find_pasteboard.mm (right):

http://codereview.chromium.org/206035/diff/13001/14007#newcode43
Line 43: DCHECK(false);  // This should never be deleted.
On 2009/09/23 15:52:24, Finnur wrote:
> drive-by (don't feel qualified to review the mac code specifically, I just
> noticed this off-hand):  out of curiosity, why should this never be deleted?
> Does this warrant further explanation in a comment?

Done.

[Finnur, 2009-09-23 17:54:11]

LG. Thanks for adding the tests.

I'd still like to know how many people actually use the Find Pasteboard on the
Mac, since for those who don't the functionality of the Find box is now slightly
lessened in my opinion. But, anyway. That's my only caveat, but you can go
ahead.

On 2009/09/23 16:53:23, Nico wrote:
> Thanks for the review, especially for taking care that the CL description is
> up-to-date.
> 
> Updated CL description and added a few tests.
> 
> http://codereview.chromium.org/206035/diff/13001/14001
> File chrome/browser/browser.cc (right):
> 
> http://codereview.chromium.org/206035/diff/13001/14001#newcode2903
> Line 2903: }
> On 2009/09/23 15:52:24, Finnur wrote:
> > Don't need the extra newline before }.
> 
> Done.
> 
> http://codereview.chromium.org/206035/diff/13001/14007
> File chrome/browser/cocoa/find_pasteboard.mm (right):
> 
> http://codereview.chromium.org/206035/diff/13001/14007#newcode43
> Line 43: DCHECK(false);  // This should never be deleted.
> On 2009/09/23 15:52:24, Finnur wrote:
> > drive-by (don't feel qualified to review the mac code specifically, I just
> > noticed this off-hand):  out of curiosity, why should this never be deleted?
> > Does this warrant further explanation in a comment?
> 
> Done.

[pink (ping after 24hrs), 2009-09-23 19:22:30]

http://codereview.chromium.org/206035/diff/17001/18006
File chrome/browser/cocoa/find_pasteboard.h (right):

http://codereview.chromium.org/206035/diff/17001/18006#newcode24
Line 24: // This is not thread-safe.
Mention this has to be on the main thread. That's an additional restriction to
not being thread safe.

http://codereview.chromium.org/206035/diff/17001/18006#newcode27
Line 27: // TODO(thakis): Add unit tests once OCMock is landed.
you can still add at least one unit test that creates and deletes it and
validates that calling findText returns non-nil and that deallocing doesn't leak
in valgrind.

http://codereview.chromium.org/206035/diff/17001/18006#newcode36
Line 36: // Returns the current find text.
Comment that this will return an empty string if there is nothing on the pboard.

http://codereview.chromium.org/206035/diff/17001/18007
File chrome/browser/cocoa/find_pasteboard.mm (right):

http://codereview.chromium.org/206035/diff/17001/18007#newcode60
Line 60: - (void)setFindText:(NSString*)newText {
what happens if newText is nil? should you check for that (and add comments
about it in the header)?

http://codereview.chromium.org/206035/diff/17001/18007#newcode61
Line 61: BOOL needToSendNotification = ![findText_ isEqualToString:newText];
do you want to add a DCHECK to ensure we're now on the main thread? Just to
catch anyone that is using it improperly?

[Nico, 2009-09-23 20:55:05]

http://codereview.chromium.org/206035/diff/17001/18006
File chrome/browser/cocoa/find_pasteboard.h (right):

http://codereview.chromium.org/206035/diff/17001/18006#newcode24
Line 24: // This is not thread-safe.
On 2009/09/23 19:22:30, pink wrote:
> Mention this has to be on the main thread. That's an additional restriction to
> not being thread safe.

Done.

http://codereview.chromium.org/206035/diff/17001/18006#newcode27
Line 27: // TODO(thakis): Add unit tests once OCMock is landed.
Added real tests instead, this has so little dependencies that I can mock them
out manually.

http://codereview.chromium.org/206035/diff/17001/18006#newcode36
Line 36: // Returns the current find text.
On 2009/09/23 19:22:30, pink wrote:
> Comment that this will return an empty string if there is nothing on the
pboard.

Done.

http://codereview.chromium.org/206035/diff/17001/18007
File chrome/browser/cocoa/find_pasteboard.mm (right):

http://codereview.chromium.org/206035/diff/17001/18007#newcode60
Line 60: - (void)setFindText:(NSString*)newText {
On 2009/09/23 19:22:30, pink wrote:
> what happens if newText is nil? should you check for that (and add comments
> about it in the header)?

Done.

http://codereview.chromium.org/206035/diff/17001/18007#newcode61
Line 61: BOOL needToSendNotification = ![findText_ isEqualToString:newText];
On 2009/09/23 19:22:30, pink wrote:
> do you want to add a DCHECK to ensure we're now on the main thread? Just to
> catch anyone that is using it improperly?

Done.

[pink (ping after 24hrs), 2009-09-23 22:03:39]

lgtm