Bug 97157 is getting cumbersome and holds too many comments and patches, so I'm separating this issue out so I can keep track of what needs to be done. Kin and I agree that there should be two levels of Find API: - one like the current one, which requires a pres shell (to get the current selection) and is simple to call from a find dialog and which handles details like whether we search from the current selection, - a lower-level one which doesn't require a pres shell and doesn't work with the current selection, but instead requires a range to be passed in and returns a range representing what was found (if anything). The higher-level interface will call the lower-level one. My intent is that nsIWebBrowserFind will be the current interface (and callers of that class will not need to change any code), while nsFind and nsIFind will be the lower-level interface.

Please expose the lower-level (range) find to web scripts. I'd like to be able to use it for the next version of the highlight bookmarklet (http://www.squarefree.com/bookmarklets/pagedata.html#highlight).

Please note, the nifty feature that is bug 62467 could be enabled by the fixing of this bug.

Both levels will definitely be exposed. Kin wants the lower level, too, for the spell checker and other text services.

nsIWebBrowserFind is frozen, according to the comment in the file, so I'm not touching that (and don't need to anyway). nsIFind is the low-level interface which is changing. Here's the nsIFind.idl I currently have in my tree. It has flags for findBackwards, caseSensitive and entireWord (which has been requested though it's not implemented yet), a method for SetRange, and Find which takes a string and returns a range. How does this look to everyone? A patch implementing this will be coming soon (so please let me know if this interface needs to change).

interface nsITextServicesDocument; interface nsIPresShell; You can remove these. I'd like to see the comments describe the behaviour of Find() when searching backwards, looking for a string which overlaps with the specified collapsed range, e.g. searching for 'time' in now is the time for when setRange specified a collapsed range pointing between the 'i' and 'm' in "time". Basically describe the behaviour of the edge cases.

Extra interface declarations removed. With regard to edge cases: for nsIFind I'm not sure there are any edge cases. It should look inside the range that's passed in, and return failure if it doesn't find anything inside that range. Any wrapping or edge cases are handled at a higher level. Does that sound sensible? That's pretty much what the comment already says, except for the @param description, which was wrong: that now says @param aRange Range specifying search start/end points. Perhaps it would also be clearer to change the comment to say "If range is collapsed, we will search in the indicated direction starting from that point"? The tricky edge cases are the job of nsIWebBrowserFind, and I don't see a consensus yet in bug 112846 as to what the behavior should be. Simon gave a clear description of what mac apps do, which is at adds with the original bug report. What I have in my tree right now pretty much matches that (i.e. no special-case behavior for select-all). When we do have a consensus, I'll add documentation there (the interface is frozen but I'm sure that doesn't preclude adding comments to clarify behavior). Please address any comments regarding nsIWebBrowserFind edge case behavior to bug 112846.

Here's the implementation. I've been running it in my tree for a few days and it seems pretty usable. Please proceed to pick holes in it. :-)

That last patch didn't include the changes making the new low-level find a callable service. Here's a new patch, which includes the files to do that. Changes to the find implementation include: use the new nsFind service, remove a couple of debug prints, ifdef out an assert and change a few comments. No changes to the algorithm.

Bah. Wrapping wasn't working in the last patch. I've added it, and that pointed out some problems with how I was handling the range offset for the first node, so I also fixed that.

Kin wanted the API revised so that the low-level find would be stateless, and to separate the range-in-which-we're-looking from the starting point of the search. I'm not sure that there actually is a case where we'd want to specify a start point that wasn't equal to the beginning or end (depending on direction) of the search range, given that low-level find doesn't do wraparound, but I have rewritten the code to implement Kin's request. If it turns out that we don't actually need the start point range, that code can be removed fairly easily since I wrote the low-level code to use the beginning or end of the range if a null start point was passed in.

The reason I thought we needed a separation between search-start-point and the search-range was because of the issue sfraser brings up above. Most find implementations I've seen will hilite "time" in sfraser's scenario above when searching backwards ... but in your previous api, if you only searched a range from the current caret position to the beginning of the doc, you wouldn't be able to see the full word "time" because the last part of the word falls outside the search range. I also requested that storing references to a doc be removed so that the same instance of the low-level find object could be used on *any* dom tree or doc. We also want to avoid hanging on to a document if the browser has loaded a different one. Do word breakers keep any references to the document or any resources that will prevent the destruction of a document?

No, nsIWordBreaker (which I should be using rather than nsILineBreaker) doesn't even have a way to set the document ... it operates purely on unichar strings and shouldn't cause any problems. The parser service doesn't use a document either. So this code should be clean of anything that contains references to the document.

Comment on attachment 68937 [details] [diff] [review] Patch implementing revised API http://bugzilla.mozilla.org/show_bug.cgi?id=123087

Comment on attachment 68937 [details] [diff] [review] Patch implementing revised API Ok, let's try that one more time ... ------------------- -- nsWebBrowserFind ------------------- -- nsWebBrowserFind::SetRangeToSelection() shouldn't be adjusting offset by +1 or -1 since it could cause things to be skipped. Could also cause offsets to be set that are less than zero and greater than node->ChildCount. -- I think that when searching backwards with the wrap option on, we need to make sure the search-range is the entire doc, and the start-point is the start of the selection. If you do have to wrap cause nothing was found, the search-range can be set from the start of the selection to the end of the doc. Searching forwards should be the inverse of this. That is in the beginning, search-range is from the end of the selection to the end of the doc, and if you have to wrap the search range should extend from the start of the doc to the end of the doc since the pattern you are looking for could straddle the start point. After saying the above, I'm wondering how we're going to be able to tell when to stop searching, since when we are wrapping the search range is the entire doc, and the start points are at the end points of that search range. Perhaps in that case the start range is not collapsed and indicates both the start and end points of the search? -- SetRangeToSelection() sounds a little misleading perhaps GetSearchRangeAndStartPoint() or CalcSearchRanges() would be clearer? -- Do we really have to throw away mFind everytime in nsWebBrowserFind::SearchInFrame()? Since we are now stateless and document ref free, my guess is that we don't. -- NS_FIND_CONTRACTID should probably be defined in nsIFind.idl? -- We should probably be initializing aDidFind to PR_FALSE in nsWebBrowserFind::SearchInFrame() -------------- -- nsIFind.idl -------------- -- Missed an "unsigned" in the CHAR_TO_UNICHAR macro. -- Use nsIWordBreaker instead of nsILineBreaker. -- Can we make word breaker an attribute like the others? If you want examples on how to use it, look back in the CVS revisions of nsWebBrowserFind, ccarlen had code that used it. -- Rename aRange to aSearchRange? ------------- -- nsFind.cpp ------------- -- In nsFind::InitIterator() you added code which attempts to position the iterator. You have to be careful of the case where |which| is either zero or container->NumChildren. In both those cases, you actually want a child in one of the parent container's sibilings hierarchy. -- Also if your start point was at the beginning of the Search range and you were searching backwards, or your start point was at the end of the search range and you were searching forwards, this code: + // no start point specified, or failure getting content + if (mFindBackward) + mIterator->Last(); + else + mIterator->First(); + would actually be doing a wrap behind the driving app's back. Do we really want to do that? -- Replace LineBreaker with WordBreaker. -- Fix "precent" typo in comment.

Generally, contract ID should not go in IDL. It identifies a set of implementations all promising to implement faithfully one or more interfaces. /be

I questioned Kin's second point (setting the range to the whole document) and we discussed it, and he explained (recording here in case I forget) that he is worried about the case where a match spans the end point. We were unable to come up with a solution that didn't require passing an aEndPt range as well as aStartPt. Yuck -- but I suppose we have to. I'm in the process of implementing that and generating a new patch to address all his points.

Here's a new patch which addresses Kin's concerns, and implements the new API we discussed, with both start point and end point. I didn't move the contract ids for nsIFind and nsIWebBrowserFind (or, rather, I did move them then moved them back) because of Brendan's comment, though I agree with Kin that it would be nicer for users if we could put the contract id strings in the idl files where they're more easily findable. Oh, and wonder of wonders, it actually does manage to find a string that spans the selection if wrap is on. -)

Kathy gave a verbal r, pending a few changes (code cleanup and formatting issues) which I have made. Kin gave a verbal sr to check in provided that it is still not enabled by default. At the last minute Kin noticed a problem with finding backward, which I have fixed (clause beginning at line 421 of nsFind.cpp -- I was mistakenly under the impression that I could do the same thing there for forward and reverse find, because I was confusing the start/end points of the search range with the start/end points (ranges) of the find. I have checked it in. Shouldn't affect anyone using the default old find code, but anyone wanting to test it (please! It needs exposure! Report any bugs you see!) can turn it on with user_pref("browser.new_find", true) Closing this bug; I'll use bug 80805 to track turning it on by default, since that's the one that's user visible (find is very slow).

How can I use this new API from JavaScript, for example to make http://www.squarefree.com/bookmarklets/pagedata.html#highlight faster?

For an example of using the old API, nsIWebBrowserFind, from JS, look at EdReplace.js after the patch in bug 80805, or at the current implementation of findUtils.js. Ignore all the FindService stuff -- that's just to remember the values between invocations of the dialog. The part you want starts with var findInst = browser.webBrowserFind; and then does things with findInst. There are not yet any examples of using the new lower-level API, nsIFind, from JS, but you can look at the implementation of nsIWebBrowserFind in nsWebBrowserFind.cpp and see how it uses nsIFind underneath. That should all translate to JS fairly directly. Basically, just create an object with contractid "@mozilla.org/embedcomp/rangefind;1", create several ranges, set the ranges to the right values, and pass them in to Find(). You can pass in null for the start and end points, though that isn't as well tested as passing in collapsed ranges explicitly for the start and end points. (Eventually we'll probably have some JS examples of this, but it hasn't been needed yet since nsIWebBrowserFind was designed to mimic what a typical find dialog exposes.) Either API would probably work for the bookmarklet, and they're equally fast since they both use the nsFind.cpp code underneath.

Bookmarklets run in the security domains of web pages, so I can't use contractid stuff.