Update the find result index "(m of n)" whenever there is an active selection...

Update the find result index "(m of n)" whenever there is an active selection on a find next.

BUG=20883
TEST=included

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=31234

[darin (slow to review), 2009-10-23 20:47:23]

You should get Finnur to review this.  Also, can you please write a test?  I
believe we have a good framework for testing this kind of issue.

[John Gregg, 2009-11-04 23:47:35]

I updated this patch to the refactoring of WebFrame into webkit/api.

I also searched for some good models to base a test on, but didn't find anything
that really exercised the find bar end-to-end.  Finnur if you point me to an
example I'll definitely add a test for this case.

[Finnur, 2009-11-05 00:18:27]

I'll take a look. In the meantime, take a look at a test called
FindInPageControllerTest.FindInPageEndState, which is a browser test that
searches on the page, ends the find session and then uses javascript to check
the page to see what is focused.

I presume you want to do something similar. Search, use javascript to change the
selection to something of your choosing, then search again and make sure the
ordinal and match count is correct.


On 2009/11/04 23:47:35, John Gregg wrote:
> I updated this patch to the refactoring of WebFrame into webkit/api.
> 
> I also searched for some good models to base a test on, but didn't find
anything
> that really exercised the find bar end-to-end.  Finnur if you point me to an
> example I'll definitely add a test for this case.

[Finnur, 2009-11-05 00:38:23]

I like where this is heading.

I have only one minor nit. Please update the patch to include a test for this,
and I'll review again.

http://codereview.chromium.org/335012/diff/6001/7001
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/335012/diff/6001/7001#newcode2785
Line 2785: // result index even on a find-next.
nit: I believe we talk about match count and ordinal everywhere, not "result
index". It would be good to keep it consistent.

[Finnur, 2009-11-05 00:40:31]

Oh, also... Can you (if you use gcl) run...

  gcl try your_patch_name --bot linux

... to get it to retry the Linux try bot. You might have to update the files if
it still complains about the update step, although I think the problem was not
an out of date file.


On 2009/11/05 00:38:23, Finnur wrote:
> I like where this is heading.
> 
> I have only one minor nit. Please update the patch to include a test for this,
> and I'll review again.
> 
> http://codereview.chromium.org/335012/diff/6001/7001
> File chrome/renderer/render_view.cc (right):
> 
> http://codereview.chromium.org/335012/diff/6001/7001#newcode2785
> Line 2785: // result index even on a find-next.
> nit: I believe we talk about match count and ordinal everywhere, not "result
> index". It would be good to keep it consistent.

[John Gregg, 2009-11-05 18:36:41]

Thanks for the pointer to the test!  I added one to cover this issue and
uploaded the new code.  Also kicked off new try jobs.

http://codereview.chromium.org/335012/diff/6001/7001
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/335012/diff/6001/7001#newcode2785
Line 2785: // result index even on a find-next.
On 2009/11/05 00:38:23, Finnur wrote:
> nit: I believe we talk about match count and ordinal everywhere, not "result
> index". It would be good to keep it consistent.

Done.

[Finnur, 2009-11-05 19:05:36]

Thanks for adding the test. This is great.

I have mostly just nits, and then one question.

http://codereview.chromium.org/335012/diff/7015/7016
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/335012/diff/7015/7016#newcode2785
Line 2785: // match ordinal even on a find-next.
nit: After reading this again, I think we should say something like:

If something is selected when we start searching it means we cannot just
increment the current match ordinal, we need to re-generate it.

http://codereview.chromium.org/335012/diff/7015/7018
File chrome/test/data/find_in_page/select_changes_ordinal.html (right):

http://codereview.chromium.org/335012/diff/7015/7018#newcode4
Line 4: var focused_elem = "{nothing focused}";
nit: For JavaScript we have usually used camel casing (?) and we should probably
spell out the full variable name, such as: 

focusedElement

http://codereview.chromium.org/335012/diff/7015/7018#newcode6
Line 6: function setFocusedElem(id) {
nit: Same here, setFocusedElement.

http://codereview.chromium.org/335012/diff/7015/7018#newcode21
Line 21: document.getElementById("log").innerHTML = "Focused element: " +
focused_elem;
nit: 80 cols.

http://codereview.chromium.org/335012/diff/7015/7018#newcode26
Line 26: var sel = window.getSelection();
nit: selection

http://codereview.chromium.org/335012/diff/7015/7018#newcode33
Line 33: 
nit: Don't need the extra line break.

http://codereview.chromium.org/335012/diff/7015/7018#newcode39
Line 39: here is a <a id="link1" href="http://www.google.com"
onfocus="setFocusedElem(this.id)" onblur="clearFocus()">link</a>
nit: 80 cols

http://codereview.chromium.org/335012/diff/7015/7018#newcode40
Line 40: to <a id="link2" href="http://www.google.com"
onfocus="setFocusedElem(this.id)" onblur="clearFocus()">google</a>, where
nit: 80 cols

http://codereview.chromium.org/335012/diff/7015/7018#newcode40
Line 40: to <a id="link2" href="http://www.google.com"
onfocus="setFocusedElem(this.id)" onblur="clearFocus()">google</a>, where
Question: Do we need all this focus stuff? Is it not enough to just set the
selection?

[John Gregg, 2009-11-05 19:18:30]

On 2009/11/05 19:05:36, Finnur wrote:
> Thanks for adding the test. This is great.
> 
> I have mostly just nits, and then one question.
> 
> http://codereview.chromium.org/335012/diff/7015/7016
> File chrome/renderer/render_view.cc (right):
> 
> http://codereview.chromium.org/335012/diff/7015/7016#newcode2785
> Line 2785: // match ordinal even on a find-next.
> nit: After reading this again, I think we should say something like:
> 
> If something is selected when we start searching it means we cannot just
> increment the current match ordinal, we need to re-generate it.
> 
> http://codereview.chromium.org/335012/diff/7015/7018
> File chrome/test/data/find_in_page/select_changes_ordinal.html (right):
> 
> http://codereview.chromium.org/335012/diff/7015/7018#newcode4
> Line 4: var focused_elem = "{nothing focused}";
> nit: For JavaScript we have usually used camel casing (?) and we should
probably
> spell out the full variable name, such as: 
> 
> focusedElement
> 
> http://codereview.chromium.org/335012/diff/7015/7018#newcode6
> Line 6: function setFocusedElem(id) {
> nit: Same here, setFocusedElement.
> 
> http://codereview.chromium.org/335012/diff/7015/7018#newcode21
> Line 21: document.getElementById("log").innerHTML = "Focused element: " +
> focused_elem;
> nit: 80 cols.
> 
> http://codereview.chromium.org/335012/diff/7015/7018#newcode26
> Line 26: var sel = window.getSelection();
> nit: selection
> 
> http://codereview.chromium.org/335012/diff/7015/7018#newcode33
> Line 33: 
> nit: Don't need the extra line break.
> 
> http://codereview.chromium.org/335012/diff/7015/7018#newcode39
> Line 39: here is a <a id="link1" href="http://www.google.com"
> onfocus="setFocusedElem(this.id)" onblur="clearFocus()">link</a>
> nit: 80 cols
> 
> http://codereview.chromium.org/335012/diff/7015/7018#newcode40
> Line 40: to <a id="link2" href="http://www.google.com"
> onfocus="setFocusedElem(this.id)" onblur="clearFocus()">google</a>, where
> nit: 80 cols
> 
> http://codereview.chromium.org/335012/diff/7015/7018#newcode40
> Line 40: to <a id="link2" href="http://www.google.com"
> onfocus="setFocusedElem(this.id)" onblur="clearFocus()">google</a>, where
> Question: Do we need all this focus stuff? Is it not enough to just set the
> selection?

I copied that file from the endState test file and just added a few more
instances of the string 'google' for my test.  So I'm not sure which parts are
required exactly.

[Finnur, 2009-11-05 19:23:33]

For the endstate test, it is critical that the focus be on the link. For this
test, I'm not so sure.

I'd be tempted to remove it to simplify the test and see if the test still
works. Maybe take a local backup of the html file so you can easily go back if
something breaks.

On 2009/11/05 19:18:30, John Gregg wrote:
> On 2009/11/05 19:05:36, Finnur wrote:
> > Thanks for adding the test. This is great.
> > 
> > I have mostly just nits, and then one question.
> > 
> > http://codereview.chromium.org/335012/diff/7015/7016
> > File chrome/renderer/render_view.cc (right):
> > 
> > http://codereview.chromium.org/335012/diff/7015/7016#newcode2785
> > Line 2785: // match ordinal even on a find-next.
> > nit: After reading this again, I think we should say something like:
> > 
> > If something is selected when we start searching it means we cannot just
> > increment the current match ordinal, we need to re-generate it.
> > 
> > http://codereview.chromium.org/335012/diff/7015/7018
> > File chrome/test/data/find_in_page/select_changes_ordinal.html (right):
> > 
> > http://codereview.chromium.org/335012/diff/7015/7018#newcode4
> > Line 4: var focused_elem = "{nothing focused}";
> > nit: For JavaScript we have usually used camel casing (?) and we should
> probably
> > spell out the full variable name, such as: 
> > 
> > focusedElement
> > 
> > http://codereview.chromium.org/335012/diff/7015/7018#newcode6
> > Line 6: function setFocusedElem(id) {
> > nit: Same here, setFocusedElement.
> > 
> > http://codereview.chromium.org/335012/diff/7015/7018#newcode21
> > Line 21: document.getElementById("log").innerHTML = "Focused element: " +
> > focused_elem;
> > nit: 80 cols.
> > 
> > http://codereview.chromium.org/335012/diff/7015/7018#newcode26
> > Line 26: var sel = window.getSelection();
> > nit: selection
> > 
> > http://codereview.chromium.org/335012/diff/7015/7018#newcode33
> > Line 33: 
> > nit: Don't need the extra line break.
> > 
> > http://codereview.chromium.org/335012/diff/7015/7018#newcode39
> > Line 39: here is a <a id="link1" href="http://www.google.com"
> > onfocus="setFocusedElem(this.id)" onblur="clearFocus()">link</a>
> > nit: 80 cols
> > 
> > http://codereview.chromium.org/335012/diff/7015/7018#newcode40
> > Line 40: to <a id="link2" href="http://www.google.com"
> > onfocus="setFocusedElem(this.id)" onblur="clearFocus()">google</a>, where
> > nit: 80 cols
> > 
> > http://codereview.chromium.org/335012/diff/7015/7018#newcode40
> > Line 40: to <a id="link2" href="http://www.google.com"
> > onfocus="setFocusedElem(this.id)" onblur="clearFocus()">google</a>, where
> > Question: Do we need all this focus stuff? Is it not enough to just set the
> > selection?
> 
> I copied that file from the endState test file and just added a few more
> instances of the string 'google' for my test.  So I'm not sure which parts are
> required exactly.

[John Gregg, 2009-11-05 20:48:28]

I see; I removed all the focus stuff and it looks fine.  Uploaded new CL with
simpler test and other nits addressed.

On 2009/11/05 19:23:33, Finnur wrote:
> For the endstate test, it is critical that the focus be on the link. For this
> test, I'm not so sure.
> 
> I'd be tempted to remove it to simplify the test and see if the test still
> works. Maybe take a local backup of the html file so you can easily go back if
> something breaks.
> 
> On 2009/11/05 19:18:30, John Gregg wrote:
> > On 2009/11/05 19:05:36, Finnur wrote:
> > > Thanks for adding the test. This is great.
> > > 
> > > I have mostly just nits, and then one question.
> > > 
> > > http://codereview.chromium.org/335012/diff/7015/7016
> > > File chrome/renderer/render_view.cc (right):
> > > 
> > > http://codereview.chromium.org/335012/diff/7015/7016#newcode2785
> > > Line 2785: // match ordinal even on a find-next.
> > > nit: After reading this again, I think we should say something like:
> > > 
> > > If something is selected when we start searching it means we cannot just
> > > increment the current match ordinal, we need to re-generate it.
> > > 
> > > http://codereview.chromium.org/335012/diff/7015/7018
> > > File chrome/test/data/find_in_page/select_changes_ordinal.html (right):
> > > 
> > > http://codereview.chromium.org/335012/diff/7015/7018#newcode4
> > > Line 4: var focused_elem = "{nothing focused}";
> > > nit: For JavaScript we have usually used camel casing (?) and we should
> > probably
> > > spell out the full variable name, such as: 
> > > 
> > > focusedElement
> > > 
> > > http://codereview.chromium.org/335012/diff/7015/7018#newcode6
> > > Line 6: function setFocusedElem(id) {
> > > nit: Same here, setFocusedElement.
> > > 
> > > http://codereview.chromium.org/335012/diff/7015/7018#newcode21
> > > Line 21: document.getElementById("log").innerHTML = "Focused element: " +
> > > focused_elem;
> > > nit: 80 cols.
> > > 
> > > http://codereview.chromium.org/335012/diff/7015/7018#newcode26
> > > Line 26: var sel = window.getSelection();
> > > nit: selection
> > > 
> > > http://codereview.chromium.org/335012/diff/7015/7018#newcode33
> > > Line 33: 
> > > nit: Don't need the extra line break.
> > > 
> > > http://codereview.chromium.org/335012/diff/7015/7018#newcode39
> > > Line 39: here is a <a id="link1" href="http://www.google.com"
> > > onfocus="setFocusedElem(this.id)" onblur="clearFocus()">link</a>
> > > nit: 80 cols
> > > 
> > > http://codereview.chromium.org/335012/diff/7015/7018#newcode40
> > > Line 40: to <a id="link2" href="http://www.google.com"
> > > onfocus="setFocusedElem(this.id)" onblur="clearFocus()">google</a>, where
> > > nit: 80 cols
> > > 
> > > http://codereview.chromium.org/335012/diff/7015/7018#newcode40
> > > Line 40: to <a id="link2" href="http://www.google.com"
> > > onfocus="setFocusedElem(this.id)" onblur="clearFocus()">google</a>, where
> > > Question: Do we need all this focus stuff? Is it not enough to just set
the
> > > selection?
> > 
> > I copied that file from the endState test file and just added a few more
> > instances of the string 'google' for my test.  So I'm not sure which parts
are
> > required exactly.

[Finnur, 2009-11-05 21:11:31]

LGTM, with one caveat. 

I keep forgetting to mention this... Can you test this on a page with multiple
frames (ie. src/chrome/test/data/find_in_page/frames.html). I don't think you'll
have problems, but it would be good to make sure.

If that works fine, you are good to go.