321481 - [Trunk] Gmail - phrase not found - even though it is (find bar updating incorrectly)

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051225 Firefox/1.6a1 ID:2005122501

1.Open Gmail, any mail
2.open Find bar and type a word you see a few times in the mail
3.Press "Highlight all"

result:
- all the matches are correctly highlighted 
- the findbar turns red and the "phrase not found" message is displayed

expected:
- don't tell me the phrase isn't found.

Comment 1

[Peter van der Woude [:Peter6]]

No problem with FF1.5
I'll try to find the regressionwindow later today or tomorrow

Comment 2

[Peter van der Woude [:Peter6]]

regression window:
works 20051216 1526pdt
fails 20051216 1923pdt

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&filetype=match&whotype=match&sortby=Date&hours=2&date=explicit&mindate=20051216+1510&maxdate=20051216+1923&cvsroot=%2Fcvsroot

bug 313653 seems a good candidate

Comment 3

[Jason Barnabe (np)]

Someone send me a Gmail invite and I'll see what I can find out about this.

Comment 4

[Peter van der Woude [:Peter6]]

(In reply to comment #3)
> Someone send me a Gmail invite and I'll see what I can find out about this.
> 
Done

Comment 5

[Jason Barnabe (np)]

Username: firefoxtest
Password: testfirefox1

The problem here is indeed the code from bug 313653. highlightDoc is fired once for every document, not once per find, so if a page has frames, the "update status" code added in bug 313653 will be fired multiple times. If the last frame doesn't contain the search term, the last time it is fired, it shows "not found".

Comment 6

[Jason Barnabe (np)]

Attachment: patch v1

This moves the update status code up higher - from highlightText to toggleHighlight. Do to this, toggleHighlight needs to know if the word was found, so I changed highlightText and highlightDoc to return true if they find something. I've also added javadoc-style documentation to the affected functions.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 206898 [details] [diff] [review]
patch v1

Hmm, does it really need to keep track of the found status when unhighlighting?

>Index: toolkit/components/typeaheadfind/content/findBar.js

>   toggleHighlight: function (aHighlight)
>   {

>+    // We have to update the status because we might still have the status
>+    // of another tab (bug 313653)

Giving bug numbers in source isn't usually useful for straight fixes, cvsblame can show those. Used more for showing dependencies that aren't immediately obvious, like hacks depending on other bugs.

>   highlightDoc: function (highBackColor, highTextColor, word, win)
>   {
>     if (!win)
>       win = window._content;

Change this to just "window.content" while you're here?

>+    var textFound = false;
>     for (var i = 0; win.frames && i < win.frames.length; i++) {
>-      this.highlightDoc(highBackColor, highTextColor, word, win.frames[i]);
>+      textFound = textFound | this.highlightDoc(highBackColor, highTextColor, 
>+                                                word, win.frames[i]);

Why not || ? Same with the equivalent below.

>     var doc = win.document;
>     if (!document)
>-      return;
>+      return textFound;

I know this isn't your code, but isn't this check kind of bogus? Shouldn't it be if (!doc)?

>     if (!("body" in doc))
>-      return;
>+      return textFound;

Could combine this with the previous statement.

Comment 8

[Jason Barnabe (np)]

(In reply to comment #7)
> (From update of attachment 206898 [details] [diff] [review] [edit])
> Hmm, does it really need to keep track of the found status when unhighlighting?
I don't think so, but it's not an expensive thing to do and it keeps the function consistent with itself. However, it may be that we don't have to update the displayed status when unhighlighting. I'll try it out.

> Why not || ? Same with the equivalent below.
Because I want highlightDoc to be called regardless of whether I've found something already.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #8)
> > Why not || ? Same with the equivalent below.
> Because I want highlightDoc to be called regardless of whether I've found
> something already.

Doh! I forgot about short circuiting, you're right. Maybe just put it first, then?

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Though after reading the IRC logs, Neil preferred Mook's suggestion of if(highLightdoc){found=true;}, and I think I agree. Doesn't make all that much of a difference, don't let it hold up anything :)

Comment 11

[Jason Barnabe (np)]

Attachment: patch v2

All requested changes.

Switching tabs turns off highlighting, so there's no situation where turning off highlighting requires the extra status update.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Sorry if I'm not catching on quickly enough here, bug why are you setting textFound = true in the |if (!highBackColor) {| block of highlightDoc? That's only called when un-highlighting, right? Seems like checking the status from hightlightText should cover all cases.

Comment 13

[Jason Barnabe (np)]

(In reply to comment #12)
> Sorry if I'm not catching on quickly enough here, bug why are you setting
> textFound = true in the |if (!highBackColor) {| block of highlightDoc? That's
> only called when un-highlighting, right? Seems like checking the status from
> hightlightText should cover all cases.
> 

a) I have to return something from the function to prevent a strict warning
b) If I have to return something in the unhighlight code, might as well make it consistent with what I'm returning in the highlight code (true if the text was found)
c) It only requires one extra line and two changed lines to do so, and it's not at all resource intensive.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Yeah, I didn't mean to imply it was a bad thing, I was just curious. Thanks for explaining! I'll land this now.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

mozilla/toolkit/components/typeaheadfind/content/findBar.js; new revision: 1.34;

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Fixed on the 1.8 branch by bug 313149.