126651 - New find misses after partial matches

Status: RESOLVED FIXED

(SeaMonkey :: Composer, defect)

Description

[[e-mail]]

Steps to reproduce problem:
1. Open a new blank page to edit
2. Type "Eerier Aardvarks"
3. Search for "er"
4. Search for "ar"

Expected resutls: both strings found in two locations

Actual results with editor.new_find enabled: both strings only found once.

Mind you if the debug output is anything to go by I'm not surprised.

Comment 1

[Akkana Peck]

Yuck!  Happens in browser, too.  I'm on it.

Comment 2

[Akkana Peck]

Attachment: Fix resetting after a partial match

Aha ...  I should have known I was confused about how I was handling partial
matches from that commented-out findex=restart line ...  I kept changing my
mind about whether it was needed or not, and the answer is that it's needed
sometimes but not other times.

Seeking review ...  and thanks for the cute example, which I will add to the
find test page I"m working on.

Comment 3

[Akkana Peck]

Attachment: Better patch

In writing a regression test page, I found another problem involving matching
extended spaces.  Here's a patch that fixes them both.

Comment 4

[Akkana Peck]

Cc'ing timeless: I was getting "You have reached the end of the internet. 
Please go forward now" on a lot of my reverse finds.  Commenting out the --iter
fixed the asserts and all my find tests work.  It looks like we were right the
first time about removing the --iter line.  Opinions?  Review?

Comment 5

[Akkana Peck]

Attachment: Test cases

In case anyone's interested (e.g. when reviewing), here's the test case I'm
currently using.  It links to Sujay's editor find/replace test case document,
but includes a lot of tests for specific problems that can crop up in the
algorithm.  I will be checking this in to the mozilla.org doc tree next to
Sujay's document.

By all means suggest other tests I haven't covered if you know of any.

Comment 6

[Akkana Peck]

Attachment: Cleaner patch

Timeless pointed out some confusing indentation, so I've cleaned that up in the
patch.	This version also has some cleanup inside DEBUG_FIND statements (so
nobody except me should be affected) and fixes the copyright to say 2002
instead of 2___.

Timeless has agreed with removing the --iter for the nsDeque iterator, but the
last hunk of the patch needs review: setting findex and restart appropriately,
and setting inWhitespace to false when ending a match.

Comment 7

[Akkana Peck]

Comment on attachment 70773 [details] [diff] [review]
Cleaner patch

I spent some more time on my testcase, and came up with a new tougher case that
breaks.  Reworking.

Comment 8

[Akkana Peck]

Attachment: Patch: de-dequify

Comment 9

[Akkana Peck]

The more I looked at how those deque iterators were working, the more I realized
I wasn't using them right, and the more I realized how little we were using the
deque at all.  Then I realized: the only reason for using the deque had been for
persistence between calls, which no longer applies, and even then, given that we
already can iterate backward and forward with good performance, it's cleaner
just to do that.  In other words, there's no reason to be using a deque at all,
and the deque code just complicates the algorithm and hides subtle bugs.

So I got rid of it.  The last patch uses the same algorithm as "Cleaner patch",
except that all references to the deque have been removed, and all iteration is
done via the dom (which fixes the last few subtle bugs I was seeing on my new
test page).

The new test page is here:
http://www.mozilla.org/quality/browser/front-end/testcases/xpapps-gui/find-on-page/find-in-page.html
It includes all of Kin's test plus every hard test I could think of, and
instructions on how to use the page for regression testing.  (It then links to
Sujay's editor testing page.)

Kathy and/or Charley, could you please review this?  Cc'ing Kin so he knows
what's going on.

Comment 10

[Akkana Peck]

Attachment: Full dedequification

Comment 11

[Akkana Peck]

Comment on attachment 70640 [details]
Test cases

Marking the attached test case obsolete, because the one checked into the
mozilla doc tree is much better.

Comment 12

[timeless]

+/* boolean Find (in wstring aFindText); */
doesn't really match this:
+NS_IMETHODIMP
+nsFind::Find(const PRUnichar *aPatText, nsIDOMRange* aSearchRange,
+             nsIDOMRange* aStartPoint, nsIDOMRange* aEndPoint,
+             nsIDOMRange** aRangeRet)

+  const PRUnichar* aPatStr = patStr.get();
+  PRInt32 aPatLen = patStr.Length() - 1;
misuse of variable naming convention, a is for argument.

+      // Should check return value -- but what would we do if it failed??
+      // We're in big trouble if that happens.
for now, how about an NS_ASSERTION ?

As for de-dequifying, i agree it's the right thing to do.

+      mIterNode->QueryInterface(nsITextContent::GetIID(), (void**)&tc);
is this the prefered way to clal QI (i.e. using ::GetIID)?

Comment 13

[Akkana Peck]

It's true about the variable naming conventions.  To be frank, I was trying to
keep the diffs as small as possible to help reviewers.  But that won't seem very
valid a year from now ... you're right, I should just bite the bullet and rename
them now.  Here's a new patch.

Also clarifying the summary (it's not just editor find and it happens with
default prefs) and adding keywords to nominate it in case that's needed.

Comment 14

[Akkana Peck]

Attachment: Full dedequification 2

Rename the variables as timeless suggested, and remove that misleading and
unneeded usage comment.  It also makes tc an nsCOMPtr (it wasn't before because
we needed to keep track of addrefs explicitly since the deque didn't) so we can
use do_QueryInterface in the normal way.

Comment 15

[Charles Manske]

Comment on attachment 70936 [details] [diff] [review]
Full dedequification 2

r=cmanske

Comment 16

[kinmoz]

Comment on attachment 70936 [details] [diff] [review]
Full dedequification 2

sr=[e-mail] with the changes we discussed on IRC:

1. Lose the queue references in the comments.

2. Fix the double assertion.

Comment 17

[Asa Dotzler [:asa]]

a=asa (on behalf of drivers) for checkin to 0.9.9

Comment 18

[Akkana Peck]

Fixed.