Mac:Dont always enable findbar buttons when restoring focus with search string

Mac:Dont always enable findbar buttons when restoring focus with search string

Avoids enabling findbar buttons when there is no search match.

TEST=On mac: Search for something not in page, verify next/prev buttons
disabled due to no results, focus away from findbar, do ctrl-f and verify
buttons remain disabled.

BUG=634289
Review-Url: https://codereview.chromium.org/2928333002
Cr-Commit-Position: refs/heads/master@{#492002}
Committed: https://chromium.googlesource.com/chromium/src/+/91d01230acfaf11ce9ef59d1af6e9ce232cc9e01

[carloschilazo, 2017-06-11 20:43:28]

The CQ bit was checked by carloschilazo@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-11 20:43:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-11 20:43:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-11 20:43:40]

Dry run: No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[carloschilazo, 2017-06-11 20:49:39]

Description was changed from

==========
Mac:Dont always enable findbar buttons when restoring focus with search string

Avoids enabling findbar buttons when there is no search match.

TEST=On mac: Search for something not in page, verify next/prev buttons
disabled due to no results, focus away from findbar, do ctrl-f and verify
buttons remain disabled.

BUG=634289
==========

to

==========
Mac:Dont always enable findbar buttons when restoring focus with search string

Avoids enabling findbar buttons when there is no search match.

TEST=On mac: Search for something not in page, verify next/prev buttons
disabled due to no results, focus away from findbar, do ctrl-f and verify
buttons remain disabled.

BUG=634289
==========

[carloschilazo, 2017-06-11 20:49:39]

carloschilazo@gmail.com changed reviewers:
+ rohitrao@chromium.org

[carloschilazo, 2017-06-11 20:52:33]

From what I could find interactive UI tests seem the best way to automatically
test this, but they don't run in Mac from what I can see.
Btw, CLA agreement and all that fun has been done previously.

[carloschilazo, 2017-06-11 21:00:27]

Forgot to mention, from manual tests: state is preserved properly for the
opposite test: when there are matches buttons remain enabled.
Any suggestion as to get this tested, lemme know.

[rohitrao (ping after 24h), 2017-06-13 00:23:37]

Let me send this to the trybots and see what they do.

Do you think this is something we could write a browsertest for?  One for each
of the manual tests you listed?

https://codereview.chromium.org/2928333002/diff/1/chrome/browser/ui/cocoa/fin...
File chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm (left):

https://codereview.chromium.org/2928333002/diff/1/chrome/browser/ui/cocoa/fin...
chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm:366: // Enable the
buttons if the find text is non-empty.
I'm worried that this code existed for a reason that we're not considering.  For
example, something involving restoring state after a tab switch.

It does seem odd that we're setting button state here, since this method
purports to be about setting focus and selection.  But I don't want to delete
the code outright without verifying that it's actually not needed by any other
scenario.

[rohitrao (ping after 24h), 2017-06-13 00:23:44]

The CQ bit was checked by rohitrao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 00:24:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-13 00:43:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-13 00:43:43]

Dry run: This issue passed the CQ dry run.

[carloschilazo, 2017-06-14 23:47:30]

On 2017/06/13 00:23:37, rohitrao (ping after 24h) wrote:
> Let me send this to the trybots and see what they do.
> 
> Do you think this is something we could write a browsertest for?  One for each
> of the manual tests you listed?
> 
>
https://codereview.chromium.org/2928333002/diff/1/chrome/browser/ui/cocoa/fin...
> File chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm (left):
> 
>
https://codereview.chromium.org/2928333002/diff/1/chrome/browser/ui/cocoa/fin...
> chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm:366: // Enable
the
> buttons if the find text is non-empty.
> I'm worried that this code existed for a reason that we're not considering. 
For
> example, something involving restoring state after a tab switch.
> 
> It does seem odd that we're setting button state here, since this method
> purports to be about setting focus and selection.  But I don't want to delete
> the code outright without verifying that it's actually not needed by any other
> scenario.

Thanks for submitting to trybot, all green yay!
I agree there was probably a reason why this was added circa 2010 but seems to
not be needed anymore.
Will take a look at adding browsertests, thanks for the pointer.

[carloschilazo, 2017-06-20 00:47:31]

Browser tests added Rohit, let's see what the trybots say and take it from
there.

[rohitrao (ping after 24h), 2017-06-20 00:52:43]

The CQ bit was checked by rohitrao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 00:53:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-20 02:51:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-20 02:51:50]

Dry run: This issue passed the CQ dry run.

[rohitrao (ping after 24h), 2017-06-26 13:20:15]

https://codereview.chromium.org/2928333002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm (left):

https://codereview.chromium.org/2928333002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm:366: // Enable the
buttons if the find text is non-empty.
Here's a case that appears to have regressed with this CL.  Can you also
reproduce this locally?

1) Open two tabs.
2) Search for something (with matches) in the first tab.
3) Close the find bar.  Switch to the second tab.  Switch back.
4) Press Cmd-F to open the find bar again.

At this point, I see the find bar, populated with a search string, but the
buttons are disabled.  I think I'd expect the buttons to be enabled at this
point?  Otherwise there's no way to trigger a search except by using the
keyboard shortcuts or deleting and retyping the text.

https://codereview.chromium.org/2928333002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm:367: BOOL
buttonsEnabled = ([[findText_ stringValue] length] > 0) ? YES : NO;
So it seems that this function is called from at least one place where it's
important to set the button state correctly, and doing nothing doesn't work.

Maybe we need to continue setting the state, but use different logic for
|buttonsEnabled|?  We could look at whether or not there is an active find
operation, and if so, use the current results to set the enabled states.

[carloschilazo, 2017-06-28 22:50:22]

https://codereview.chromium.org/2928333002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm (left):

https://codereview.chromium.org/2928333002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm:367: BOOL
buttonsEnabled = ([[findText_ stringValue] length] > 0) ? YES : NO;
On 2017/06/26 13:20:15, rohitrao (ping after 24h) wrote:
> So it seems that this function is called from at least one place where it's
> important to set the button state correctly, and doing nothing doesn't work.
> 
> Maybe we need to continue setting the state, but use different logic for
> |buttonsEnabled|?  We could look at whether or not there is an active find
> operation, and if so, use the current results to set the enabled states.

Thanks for looking into it Rohit! I agree, we need to then add logic for the
enablement instead of directly removing it. I'll make some time this weekend to
look into it.

[carloschilazo, 2017-07-04 23:42:11]

Hey Rohit, quick update, let me know what you think:
The test case you mentioned works by accident as we optimistically always enable
the next/prev buttons.
Seems most of the behavior around this was designed to be reactive to find
results which gets more complicated with the loss of state when closing &
switching to other tabs (beyond the global pasteboard).
I'll be looking at finding a way to 'patch' some of these scenarios when we know
there are _no_ results and continue to be optimistic when we don't know.
Most likely will have to rely on the state of the FindTabHelper.
Please let me know if this sounds reasonable or you'd prefer a different
approach.
Thanks!

[carloschilazo, 2017-07-09 19:31:45]

Went ahead and implemented my proposed approach, I added a browser test for the
manual scenario that was previously regressed.

[rohitrao (ping after 24h), 2017-07-19 08:34:18]

On 2017/07/09 19:31:45, carloschilazo wrote:
> Went ahead and implemented my proposed approach, I added a browser test for
the
> manual scenario that was previously regressed.

Sorry for the delay.  I'm out until next week, but I can take a look then. 
Thanks again for your persistence on this =)

[rohitrao (ping after 24h), 2017-07-26 14:55:31]

lgtm, thanks for adding the tests!

One other case that's a little wonky is doing a search, getting "0/0" results,
then closing the findbar and reopening.  You see "0/0" with the buttons enabled,
which looks strange, but I'm not sure this is actually a bug.  The app has this
behavior even without your CL, so it's definitely not a regression.

https://codereview.chromium.org/2928333002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm (right):

https://codereview.chromium.org/2928333002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm:682: return
findTabHelper->find_result().number_of_matches();
For my own reference, number_of_matches() appears to return -1 after the findbar
is closed.  So after closing the findbar and switching tabs, this returns -1 and
the logic in setFocusAndSelection enables the buttons.

[rohitrao (ping after 24h), 2017-07-26 14:55:41]

The CQ bit was checked by rohitrao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-26 14:55:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-26 15:17:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-26 15:17:26]

Dry run: This issue passed the CQ dry run.

[carloschilazo, 2017-07-29 18:49:42]

On 2017/07/26 14:55:31, rohitrao (ping after 24h) wrote:
> lgtm, thanks for adding the tests!
> 
> One other case that's a little wonky is doing a search, getting "0/0" results,
> then closing the findbar and reopening.  You see "0/0" with the buttons
enabled,
> which looks strange, but I'm not sure this is actually a bug.  The app has
this
> behavior even without your CL, so it's definitely not a regression.
> 
>
https://codereview.chromium.org/2928333002/diff/40001/chrome/browser/ui/cocoa...
> File chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm (right):
> 
>
https://codereview.chromium.org/2928333002/diff/40001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm:682: return
> findTabHelper->find_result().number_of_matches();
> For my own reference, number_of_matches() appears to return -1 after the
findbar
> is closed.  So after closing the findbar and switching tabs, this returns -1
and
> the logic in setFocusAndSelection enables the buttons.

Thanks, anything else I need to do to push this forward & merged?

[rohitrao (ping after 24h), 2017-08-04 12:28:19]

The CQ bit was checked by rohitrao@chromium.org

[commit-bot: I haz the power, 2017-08-04 12:28:25]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-04 12:49:17]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1501849699833350,
"parent_rev": "50f5a150afb4fa7b1d5923d89b213d3d53dccd6c", "commit_rev":
"91d01230acfaf11ce9ef59d1af6e9ce232cc9e01"}

[commit-bot: I haz the power, 2017-08-04 12:49:32]

Description was changed from

==========
Mac:Dont always enable findbar buttons when restoring focus with search string

Avoids enabling findbar buttons when there is no search match.

TEST=On mac: Search for something not in page, verify next/prev buttons
disabled due to no results, focus away from findbar, do ctrl-f and verify
buttons remain disabled.

BUG=634289
==========

to

==========
Mac:Dont always enable findbar buttons when restoring focus with search string

Avoids enabling findbar buttons when there is no search match.

TEST=On mac: Search for something not in page, verify next/prev buttons
disabled due to no results, focus away from findbar, do ctrl-f and verify
buttons remain disabled.

BUG=634289

Review-Url: https://codereview.chromium.org/2928333002
Cr-Commit-Position: refs/heads/master@{#492002}
Committed:
https://chromium.googlesource.com/chromium/src/+/91d01230acfaf11ce9ef59d1af6e...
==========

[commit-bot: I haz the power, 2017-08-04 12:49:34]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/91d01230acfaf11ce9ef59d1af6e...