221845 - (Patch) (OS X) put type ahead find string on find pasteboard

Status: RESOLVED FIXED

(SeaMonkey :: Find In Page, defect)

Description

[David Haas (gone, not reading mail)]

On OS X, Cocoa apps (and Carbon, too) share a global Find pasteboard.  This
patch puts the current type-ahead find string onto that pasteboard.  

If for no other reason, add it to help fix Camino's bug 160771.

(Patch forthcoming)

Comment 1

[David Haas (gone, not reading mail)]

Attachment: patch to nsTypeAheadFind.cpp

Comment 2

[Boris Zbarsky [:bzbarsky]]

Does normal find do it?  If so, we should probably share that code....

Comment 3

[David Haas (gone, not reading mail)]

My ~1-2 month old builds of Mozilla & Mozilla Firebird neither read from nor
write to the global find pasteboard.  They should.  But that's a different bug.  

Comment 4

[Boris Zbarsky [:bzbarsky]]

David, all I'm saying is that architecturally having the pasteboard (and similar
functionality on other OSes if it exists) be handled in one place would be far
preferable to sticking ifdefs everywhere.

Comment 5

[David Haas (gone, not reading mail)]

While I'd agree it would be nice if everything was in one place, I'm not sure
where that place should be.  Some of this comes from unfamiliarity with the
codebase.  Some of it comes from the fact that there seems already to be 2
locations storing Find information - nsWebBrowserFind and nsFindService.  If
you'll notice, the function I patched happily stores the new type-ahead find
string in both of these locations.

Comment 6

[Boris Zbarsky [:bzbarsky]]

akkana, thoughts?

Comment 7

[Akkana Peck]

Re the ifdef issue: I'm not clear what the find pasteboard does.  Is it
something that could be merged into part of the nsIClipboard functionality? 
Then code could call into that, and do nothing if not OSX.  If it's completely
different, maybe it isn't worth making a separate class just for that.  Simon,
any thoughts?

But Boris' comment 2 is more important: I don't understand why this is just
typeahead find and not find in general.  Why isn't it in nsFind.cpp?  Is this
something the mac only does for typeahead fine, and not in find dialogs?

Comment 8

[David Haas (gone, not reading mail)]

comment 7: Explanation of the find pasteboard on OS X:
I'm using Mail.app on OS X.  I pull up the find dialog and search for "kittens".  
I then switch applictions - let's say to Project Builder.  I pull up a find dialog.  
The dialog comes up with the string "kittens" already entered in the text field.

So, also on comment 7: No, this shouldn't happen for just type-ahead finds, but for
ALL finds on the the Mac.  And if somebody points me to a central file from which all
find strings are born, I'll happily move the patch there.  But where is that file?
Type-ahead find is currently saving its find string into both nsWebBrowserFind and
nsFindService - so to me, it looks like there's already duplication in the codebase.

As far as nsFind goes - in nsFind::Find, whatever's in aPatText should at some point be
saved into the find pasteboard on OS X.  But where did aPatText come from?  Whatever's
calling nsFind::Find I think should be responsible for stuffing the text string into the find
pasteboard.

Comment 9

[Boris Zbarsky [:bzbarsky]]

The documentation for nsIFindService
(http://lxr.mozilla.org/seamonkey/source/xpfe/components/find/public/nsIFindService.idl#46)
says that that's the place to store the last thing searched for.

nsIWebBrowserFind lets you set the thing you are currently searching for.  This
is per-window, so you could theoretically search for things without setting the
findservice string to them....

It seems to me that the findservice should handle putting the string on the
pasteboard when it gets it.

Comment 10

[Boris Zbarsky [:bzbarsky]]

And on a further note, the findservice should GET the string from the pasteboard
when it's requested.  This should lead to us using the pasteboard the way it's
supposed to be used, as far as I can see.

Comment 11

[David Haas (gone, not reading mail)]

Attachment: patch to nsWebBrowserFind.cpp

I made a patch against nsWebBrowserFind.  There are 3 reasons I did this rather
than nsFindService.  There's also 2 issues with this patch.

Three reasons:
1) Camino doens't use nsFindService.  It does use nsWebBrowserFind.
2) If you patch nsFindService, you also have to link it against the Carbon
framework, a new dependency.  One doesn't have to do that for the
nsWebBrowserFind.
3) Patching nsFindService doesn't seem to work after the 1st time a Find window
is open for a particular window.  In other words, start a new Mozilla window,
open Find - you get the global find pasteboard.  Change the global pasteboard
in some other app - that doesn't propigate to Mozilla.	If you open another new
Mozilla window, though, it gets the global find pasteboard.  But not on it's
2nd attempt.  Etc.

Issues:
1) In nsWebBrowserFind::GetSearchString, I malloc a new buffer to hold a
unicode string, assign it to mSearchString (another malloc?), free the buffer I
malloc'd, and then mSearchString malloc's a new unicode string out into
aSearchString.	The problem is, if I go directly from buffer 1 to buffer 4, I
get funky characters appearing in the find box after the search string 99.9% of
the time.  This method gives me funky characters 5% of the time.  Clearly the
string is not getting terminated correctly.  Any advice would be appreciated. 

2) I'm using malloc and free, rather than any sort of XPCOM allocator.	Fixing
issue 1 might fix this for free.

Comment 12

[Boris Zbarsky [:bzbarsky]]

#3 is interesting.  It does look like each browser window stores the last string
searched for in that window and that the last string searched for over all
windows is stored in the find service...  akkana, how is this supposed to fit
together exactly?

As for the issues....  I assume that GetScrapFlavorData does not null-terminate
what it returns?  I bet the following would work much better:

NS_ASSERTION(byteCount%2 == 0, "byteCount not a multiple of 2");
mSearchString.SetCapacity(byteCount/2);
err =
GetScrapFlavorData(scrap,kScrapFlavorTypeUnicode,&byteCount,mSearchString.get());

Alternately, if you want to be safe in case allocations fail, you could do:

NS_ASSERTION(byteCount%2 == 0, "byteCount not a multiple of 2");
nsAutoArrayPtr<PRUnichar> buffer = new PRUnichar[byteCount/2 + 1];
if (buffer) {
  err = GetScrapFlavorData(scrap,kScrapFlavorTypeUnicode,&byteCount,buffer);
  if (err == noErr) {
    buffer[byteCount/2] = PRUnichar('\0');
    mSearchString.Assign(buffer);
  }
}

If this fails to compile, you may need to pass buffer.get() to
GetScrapFlavorData instead of passing buffer or something like that.

Comment 13

[Akkana Peck]

> how is this supposed to fit together exactly?

The find service was supposed to be a singleton which stores a global find
string.  Individual windows shouldn't be storing anything, I shouldn't think,
but nsWebBrowserFind does store the last search string since it's the object
responsible for iterating over frames, and nsIWebBrowserFind::findNext() doesn't
take a search string as a parameter.  That isn't something we can change since
that API is frozen.  Perhaps that's why you're not seeing the string change
after the dialog first comes up -- the nsIWebBrowserFind has been created and
probably won't be recreated after that (but might be for a new window).

Perhaps any code that calls nsIWebBrowserFind's findNext() should always set
searchString first from the find service (unless there's a good reason not to,
and I don't currently know of any cases in browser or composer that would have
such a reason).

Comment 14

[David Haas (gone, not reading mail)]

Attachment: patch to nsWebBrowserFind.cpp

Updated patch to nsWebBrowserFind.cpp.	I used your "be safe in case allocation
fails" method.	It appears to work.

The only remaining question I had was did I need to explicitly free/release the

nsAutoArrayPtr buffer I created, or does it free itself?

Comment 15

[David Haas (gone, not reading mail)]

Attachment: real patch to nsWebBrowserFind.cpp

sorry wrong patch before.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Tha "auto" part of the name refers to its automatically deallocating itself
properly when it goes out of scope.

Comment 17

[David Haas (gone, not reading mail)]

Comment on attachment 133249 [details] [diff] [review]
real patch to nsWebBrowserFind.cpp

this guy's ready for review.  Preferably by somebody with a Mac.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 133249 [details] [diff] [review]
real patch to nsWebBrowserFind.cpp

You want to request review from someone in particular...

Comment 19

[Simon Fraser [no longer active]]

Comment on attachment 133249 [details] [diff] [review]
real patch to nsWebBrowserFind.cpp

sr=sfraser, as long as you put spaces after commas, and scope API calls with ::

Comment 20

[David Haas (gone, not reading mail)]

Attachment: updates as requested

as per comment 19, added spaces after commas and ::'s before API calls.

Comment 21

[Akkana Peck]

I think you made a mistake with that last patch -- "+" doesn't show up on all
the new lines.  Better double check that.

I don't know how unlikely it is that byteCount%2 != 0, but if it happens, might
you want to bomb out of this clause?  You're confident the mac will never use
UTF-8 or ascii, I gather? 

Is NS_ENSURE_TRUE the best test for an nsAutoArrayPtr object?  It should work,
but might NS_ENSURE_ARG_POINTER be better?

Since this was okay with Simon I'll assume the byte count thing is okay, and
give r=akkana (but I'm not marking it on either attachment, because one is
marked obsolete and the other isn't a real patch) but a mac person (Simon?)
might want to look and make sure.  You can go either way on the array ptr test
since we don't have a real macro for testing autoptr objects.

Comment 22

[David Haas (gone, not reading mail)]

Attachment: update

Hmm - don't know what diff did to the last patch.  Oh well - this one's fixed.

We're specifically calling the Unicode text scrap (there's a separate scrap for
ASCII strings) which should ensure we always get a multiple-of-two byte string.
 OS X is internally canonical unicode and only converts to (for example) UTF8
on request.  It shouldn't be a problem.

I didn't change the NS_ENSURE test since you were ambivalent about it.

So do I set the r & sr flags on the new patch myself? 'Cause I think it's now
officialy good to go.

Comment 23

[David Haas (gone, not reading mail)]

Comment on attachment 133612 [details] [diff] [review]
update

carrying over r+ from comment 21 and sr+ from comment 19.

Comment 24

[Boris Zbarsky [:bzbarsky]]

David, please ping me once the tree reopens for 1.6b and I'll check this in...

Comment 25

[timeless]

checked in

Comment 26

[David Haas (gone, not reading mail)]

looks like it works in 11-03-2003 Firebird build.