Closed Bug 123087 Opened 23 years ago Closed 22 years ago

redesign Find API

Categories

(Core :: Find Backend, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla0.9.9

People

(Reporter: akkzilla, Assigned: akkzilla)

References

Details

Attachments

(1 file, 5 obsolete files)

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.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.9
Blocks: 62467
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.
Attached file Proposed nsIFind interface (obsolete) —
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.
Keywords: nsbeta1+
Attached patch Implementation (obsolete) — Splinter Review
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. :-)
Attachment #67803 - Attachment is obsolete: true
Attached patch Minor update (obsolete) — Splinter Review
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.
Attachment #68463 - Attachment is obsolete: true
Attached patch fix wraparound (obsolete) — Splinter Review
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.
Attachment #68568 - Attachment is obsolete: true
Attached patch Patch implementing revised API (obsolete) — Splinter Review
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.
Attachment #68594 - Attachment is obsolete: true
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

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. -)
Attachment #68937 - Attachment is obsolete: true
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).
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
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.
Component: XP Miscellany → General
QA Contact: brendan → general
Component: General → Find Backend
QA Contact: general → find-backend
Depends on: 830439
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: