Closed Bug 389159 Opened 18 years ago Closed 12 years ago

Implement match count calculation for nsWebBrowserFind

Categories

(Core Graveyard :: Embedding: APIs, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 257061
mozilla1.9beta4

People

(Reporter: ehsan.akhgari, Unassigned)

References

Details

Attachments

(1 file)

We need APIs in nsWebBrowserFind to calculate the number of matches for a particular search, so that this match count can be shown in the UI (see bug 257061), among other uses. The proposed plan is laid out in bug 257061 comment 7. I'm in the process of writing the necessary code, and I'll attach the patch to this bug when I finish.
Target Milestone: mozilla1.9 M8 → mozilla1.9 M9
This patch defines two new interfaces: nsIWebBrowserFind2 and nsIWebBrowserFindMatchFilter. nsIWebBrowserFind2 is used to obtain the match count for a search string. It implements facilities to specify the maximum number of matches to look for (for performance reasons when searching large documents, for example) and also a filtering mechanism, through nsIWebBrowserFindMatchFilter. nsIWebBrowserFindMatchFilter is designed to be implemented by the callers, and gives them a chance to examine each found match (in form of an nsIDOMRange) and decide if they want it to be counted or not (by returning true or false, respectively.) This will be implemented by the typeaheadfind component, for example, to filter out invisible matches, as well as matches outside of a link, when the typeaheadfind components is asked to look only through links. The API provided in this patch will be used to implement display of the match count in the find bar in bug 257061.
Attachment #284753 - Flags: superreview?(benjamin)
Attachment #284753 - Flags: review?(benjamin)
Please don't add unfrozen interfaces to SDK_XPIDLSRCS.
(In reply to comment #2) > Please don't add unfrozen interfaces to SDK_XPIDLSRCS. I have not read <http://www.mozilla.org/projects/embedding/HowToFreeze.html> so I didn't know I was doing something wrong. My appologies. I'll correct this in the next revision of the patch. Let's see what the review results in.
Comment on attachment 284753 [details] [diff] [review] Implement match count I do not know this code at all and I am not the correct reviewer.
Attachment #284753 - Flags: superreview?(benjamin)
Attachment #284753 - Flags: review?(benjamin)
Who can (should?) do reviews for find code?
Comment on attachment 284753 [details] [diff] [review] Implement match count Ehsan, if you could write testcases for the code you've written, that would be great. Especially mochitests, that can be checked into the tree, would probably be ideal here: http://developer.mozilla.org/en/docs/Mochitest
Attachment #284753 - Flags: review?(jst)
Flags: blocking1.9?
Not a blocker at this point in the cycle.
Severity: normal → enhancement
Flags: blocking1.9? → blocking1.9-
jst: ping...
Target Milestone: mozilla1.9beta1 → mozilla1.9beta4
I'd suggest using e-mail to ping, for what it's worth.
Done. Thanks for the suggestion, Boris.
This is still in my list of things to review, but given that this has been blocking1.9-'ed I don't think I'll be able to review this before the release :(
Ugh, that sucks :( Does anyone know someone who could review this? Note that the patch already is waiting 4 months for review now.
Flags: wanted1.9.1?
Blocks: 414109
Any chance of getting a review before the next release (1.9.1)?
Flags: blocking1.9.1?
I'm pretty rusty on the nsWebBrowserFind code, but I hate to see people waiting that long for a review, so I thought I'd take a look and see if I could help any (then maybe JST can take a look as super-reviewer). I have some questions: Should nsWebBrowserFind::FindRangeInFrame() be a static, since it no longer needs to use any of the class variables? > *aFoundRange = foundRange.forget().get(); What does this do? I'm not seeing forget() in nsIDOMRange.idl. Could you add a comment explaining (and also make it clearer to me) what nsWebBrowserFind::SearchInFrame does that makes it different from nsWebBrowserFind::FindRangeInFrame? You have FindRangeInFrame listed with the same comment that used to go with SearchInFrame (which may make sense since it's mostly the old SearchInFrame code) but it's confusing to read without a brief description of the new function. Also, minor style issue: could you remove the spaces in the increment lines, like ++ countFound; I haven't seen any other Mozilla code that puts a space after a ++. I'm a little confused by this code: if (mFindFilter) { PRBool filter = PR_FALSE; rv = mFindFilter->FilterMatch(retRange, &filter); if (NS_SUCCEEDED(rv)) { if (filter) ++ countFound; } else { ++ countFound; } } else { ++ countFound; } How is that whole thing different from: ++countFound; ? Is FilterMatch() expected to have side effects? Shouldn't we be counting only cases where the filter did match, if there is a filter? Or am I misunderstanding what the filter is for? I guess a lot of what this change does is make it possible to use the find-in-a-frame code without creating and setting mFind? I'm not really clear why you need an extra routine without mFind for that. The code creating the new interface, etc. all looks fine to me.
> I'm not seeing forget() in nsIDOMRange.idl It's in nsCOMPtr.h, but this would probably be better as: foundRange.forget(*aFoundRange); in any case.
Not blocking on this. Feel free to mark wanted+ if the other half gets wanted+ per beltzner.
Flags: wanted1.9.1?
Flags: wanted1.9.1-
Flags: blocking1.9.1?
Flags: blocking1.9.1-
I guess it's impossible to get this patch in then :(
Updating to reality: I won't have the time to work on this for the foreseeable future!
Assignee: ehsan.akhgari → nobody
Status: ASSIGNED → NEW
Comment on attachment 284753 [details] [diff] [review] Implement match count Clearing out old reviews. If this is still relevant, please re-request review for this patch.
Attachment #284753 - Flags: review?(jst)
Sorry, I no longer have time to work on this. The bug is still relevant though.
Match counting and getting/ keeping a ref to the found match is done in bug 257061, so there's no need to keep this around. I'd consider that a good thing :)
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: