Closed
Bug 389159
Opened 18 years ago
Closed 12 years ago
Implement match count calculation for nsWebBrowserFind
Categories
(Core Graveyard :: Embedding: APIs, enhancement)
Core Graveyard
Embedding: APIs
Tracking
(Not tracked)
RESOLVED
DUPLICATE
of bug 257061
mozilla1.9beta4
People
(Reporter: ehsan.akhgari, Unassigned)
References
Details
Attachments
(1 file)
20.12 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•17 years ago
|
Target Milestone: mozilla1.9 M8 → mozilla1.9 M9
Reporter | ||
Comment 1•17 years ago
|
||
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)
![]() |
||
Comment 2•17 years ago
|
||
Please don't add unfrozen interfaces to SDK_XPIDLSRCS.
Reporter | ||
Comment 3•17 years ago
|
||
(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 4•17 years ago
|
||
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)
Comment 5•17 years ago
|
||
Who can (should?) do reviews for find code?
Comment 6•17 years ago
|
||
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)
Reporter | ||
Updated•17 years ago
|
Flags: blocking1.9?
Comment 7•17 years ago
|
||
Not a blocker at this point in the cycle.
Severity: normal → enhancement
Flags: blocking1.9? → blocking1.9-
![]() |
||
Comment 9•17 years ago
|
||
I'd suggest using e-mail to ping, for what it's worth.
Reporter | ||
Comment 10•17 years ago
|
||
Done. Thanks for the suggestion, Boris.
Comment 11•17 years ago
|
||
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 :(
Comment 12•17 years ago
|
||
Ugh, that sucks :(
Does anyone know someone who could review this?
Note that the patch already is waiting 4 months for review now.
Reporter | ||
Updated•17 years ago
|
Flags: wanted1.9.1?
Comment 13•16 years ago
|
||
Any chance of getting a review before the next release (1.9.1)?
Flags: blocking1.9.1?
Comment 14•16 years ago
|
||
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.
![]() |
||
Comment 15•16 years ago
|
||
> 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.
Comment 16•16 years ago
|
||
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-
Comment 17•16 years ago
|
||
I guess it's impossible to get this patch in then :(
Reporter | ||
Comment 18•15 years ago
|
||
Updating to reality: I won't have the time to work on this for the foreseeable future!
Assignee: ehsan.akhgari → nobody
Status: ASSIGNED → NEW
Blocks: 582198
Comment 19•12 years ago
|
||
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)
Reporter | ||
Comment 20•12 years ago
|
||
Sorry, I no longer have time to work on this. The bug is still relevant though.
Comment 21•12 years ago
|
||
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
Updated•6 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•