916534 - Find with highlights not working in frames after bug 666816

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Luís Miguel [:quicksaver] (not reading bugmail)]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0 (Beta/Release)
Build ID: 20130814063812

Steps to reproduce:

I have a question, the new Finder._highlight() method calls itself for frames, and just like before it seems to expect a return value (I don't know how to properly enclose code bits in comments, sorry):

    for (let i = 0; win.frames && i < win.frames.length; i++) {
      if (this._highlight(aHighlight, aWord, win.frames[i]))
        result = Ci.nsITypeAheadFind.FIND_FOUND;
    }

However there is only one case defined when there actually is a return value, which is when there is a selection controller and aHighlight is not true, in this case it returns true. When there isn't a controller it just returns (I'm assuming it just returns (I'm guessing this is the equivalent of "return undefined;" so that should work, but in all other cases it doesn't even return, just notifies.

My question is, is that as it is supposed to be? Shouldn't it only notify the listeners on the base level _highlight() call only and not on every frame subcall? And shouldn't it always return a found value?

If it returns undefined by default, and the listeners take care of the rest of the work, then it should still work. I don't think it is working though, as if you turn on the highlights here (frameset example) http://www.quackit.com/html/templates/frames/frames_example_4.html it will report "Phrase not found" always, even though the search exists and the highlights are placed.

Comment 1

[Luís Miguel [:quicksaver] (not reading bugmail)]

The second paragraph was badly written, here it is reviewed:

However there is only one case defined when there actually is a return value, which is when there is a selection controller and aHighlight is not true; in this case it returns true. When there isn't a controller it just returns. I'm guessing this is the equivalent of "return undefined;" so that should work in theory, but in all other cases it doesn't even return, just notifies the listeners.

Comment 2

[Tom S [:evilpie]]

Thanks for reporting this issue, I will upload the patch tomorrow.

Comment 3

[Tom S [:evilpie]]

Attachment: highlight-fix

Thanks again for spotting this! The new code now almost matches the old code, plus some additional comment. Basically we only return true when we are removing the highlight, because for the UI this has the meaning of success. And it's kind of the same as trying to search for an empty string. Luis noticed that there was some unused function in findbar.xml, so I removed that as well.

Comment 4

[Mike de Boer [:mikedeboer]]

Tom, I will review your patch asap (not feeling too well today), but I can already say you'll get additional brownie points if you add a unit test to catch future regressions. Do you have time to do that?

Comment 5

[Tom S [:evilpie]]

Attachment: really simple test

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 805279 [details] [diff] [review]
highlight-fix

Review of attachment 805279 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/Finder.jsm
@@ +248,1 @@
>        return true;

I don't think explicitly returning here is not really necessary. I think you can just say `found = true` and let the value returning happen in the statement below.

I also think you can restrict this comment to `Removing the highlighting always succeeds, so return true.`

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 805377 [details] [diff] [review]
really simple test

Review of attachment 805377 [details] [diff] [review]:
-----------------------------------------------------------------

I agree that this is a very simple test, but not exactly what I had in mind (my fault, I prolly didn't explain it well enough!):
The problem is that no unit test caught this failure. This means that there is no unit test that covers find with Highlight All in a page with frames. I meant to suggest adding a unit test that will catch this regression when it happens again.

Something else: you might want to check your default editor settings, as it seems to be set on 4-space indentation by default.

Comment 8

[Tom S [:evilpie]]

You might want to look at this again, there is an iframe with text content. I am pretty sure this test would have failed before, because we would get NOTFOUND.

Comment 9

[Luís Miguel [:quicksaver] (not reading bugmail)]

Sorry to interrupt the discussion, just something I noticed (nit); I think it's unnecessary to explicitly pass "null" and "false" as arguments to _highlight() and _notify() respectively, as they will default to that state. Please give me a heads up if I'm wrong (and yourself as there are several calls to _notify() without those arguments explicitly set throughout the code).

Comment 10

[Tom S [:evilpie]]

Well it's not wrong, but I prefer passing them explicitly.

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 805377 [details] [diff] [review]
really simple test

By Jeeves, you're right! If you re-upload this patch with the correct indentation settings... r=me.

Comment 12

[Tom S [:evilpie]]

Attachment: highlight-test

Comment 13

[Mike de Boer [:mikedeboer]]

Comment on attachment 805973 [details] [diff] [review]
highlight-test

Review of attachment 805973 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/tests/browser/browser_Finder.js
@@ +8,5 @@
> +
> +function test () {
> +  waitForExplicitFinish();
> +
> +  tab = gBrowser.addTab("data:text/html,<iframe srcdoc='content' />")

nit: space not needed and missing semicolon: `'content'/>");`

Comment 14

[Tom S [:evilpie]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/9ebe2f2a6cc3
http://hg.mozilla.org/integration/mozilla-inbound/rev/e9e4af1645b5

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e9e4af1645b5
https://hg.mozilla.org/mozilla-central/rev/9ebe2f2a6cc3

Comment 16

[Tom S [:evilpie]]

Comment on attachment 805279 [details] [diff] [review]
highlight-fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 666816
User impact if declined: Findbar matches might not be indicated
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): not very risky, this code is now actually more similar to what we had before
String or IDL/UUID changes made by this patch: none

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0129db608054
https://hg.mozilla.org/releases/mozilla-aurora/rev/59170fb44684