Implement match count calculation for nsWebBrowserFind

RESOLVED DUPLICATE of bug 257061

Status

()

--
enhancement
RESOLVED DUPLICATE of bug 257061
12 years ago
5 years ago

People

(Reporter: Ehsan, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.9beta4
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 -
wanted1.9.1 -
blocking1.9 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

12 years ago
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

11 years ago
Target Milestone: mozilla1.9 M8 → mozilla1.9 M9
(Reporter)

Comment 1

11 years ago
Created attachment 284753 [details] [diff] [review]
Implement match count

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.

(Reporter)

Comment 3

11 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

11 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)
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)
(Reporter)

Updated

11 years ago
Flags: blocking1.9?
Not a blocker at this point in the cycle.
Severity: normal → enhancement
Flags: blocking1.9? → blocking1.9-
(Reporter)

Comment 8

11 years ago
jst: ping...
Target Milestone: mozilla1.9beta1 → mozilla1.9beta4
I'd suggest using e-mail to ping, for what it's worth.
(Reporter)

Comment 10

11 years ago
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.
(Reporter)

Updated

11 years ago
Flags: wanted1.9.1?
(Reporter)

Updated

10 years ago
Blocks: 414109
Any chance of getting a review before the next release (1.9.1)?
Flags: blocking1.9.1?

Comment 14

10 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.
> 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 :(
(Reporter)

Comment 18

9 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
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

6 years ago
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
Last Resolved: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 257061
You need to log in before you can comment on or make changes to this bug.