Disable Find-In-Page buttons when no text is entered

Disable Find-In-Page buttons when no text is entered

This CL fixes a regression caused by https://crrev.com/1660273003 which
enables the buttons of the Find-In-Page bar even if no text is entered
or no search was issued yet.

R=msw@chromium.org
BUG=594097
TEST=FindInPageTest.ButtonsDisabledWithoutText

Committed: https://crrev.com/afc0733e0630499bea163c4f3cf826c945e7baaf
Cr-Commit-Position: refs/heads/master@{#381074}

[msw, 2016-03-14 17:30:10]

https://codereview.chromium.org/1785033004/diff/1/chrome/browser/ui/views/fin...
File chrome/browser/ui/views/find_bar_host.cc (right):

https://codereview.chromium.org/1785033004/diff/1/chrome/browser/ui/views/fin...
chrome/browser/ui/views/find_bar_host.cc:147:
find_bar_view()->ClearMatchCount();
AFAICT, the regression was just from FindBarView::UpdateMatchCountAppearance's
&& -> || change; shouldn't reverting that be sufficient? Why do we want to
change this? (explain in CL description or comment here)

[Mario Pistrich, 2016-03-14 17:52:46]

https://codereview.chromium.org/1785033004/diff/1/chrome/browser/ui/views/fin...
File chrome/browser/ui/views/find_bar_host.cc (right):

https://codereview.chromium.org/1785033004/diff/1/chrome/browser/ui/views/fin...
chrome/browser/ui/views/find_bar_host.cc:147:
find_bar_view()->ClearMatchCount();
On 2016/03/14 17:30:10, msw wrote:
> AFAICT, the regression was just from FindBarView::UpdateMatchCountAppearance's
> && -> || change; shouldn't reverting that be sufficient? Why do we want to
> change this? (explain in CL description or comment here)

Reverting only the change of && -> || would reintroduce the initial bug
(https://crrev.com/1660273003).

FindBarView::UpdateMatchCountAppearance was the wrong place to fix the issue.
The initial bug is caused by always calling FindBarView::ClearMatchCount,
regardless of |find_text|. This temporarily disables the buttons and causes the
focus to jump to the close button (if they were focused before).
If FindBarView::ClearMatchCount is only called when the text is empty, the focus
stays on the buttons as they are not getting temporarily disabled.

[msw, 2016-03-14 18:10:32]

lgtm

https://codereview.chromium.org/1785033004/diff/1/chrome/browser/ui/views/fin...
File chrome/browser/ui/views/find_bar_host.cc (right):

https://codereview.chromium.org/1785033004/diff/1/chrome/browser/ui/views/fin...
chrome/browser/ui/views/find_bar_host.cc:147:
find_bar_view()->ClearMatchCount();
On 2016/03/14 17:52:46, Mario Pistrich wrote:
> On 2016/03/14 17:30:10, msw wrote:
> > AFAICT, the regression was just from
FindBarView::UpdateMatchCountAppearance's
> > && -> || change; shouldn't reverting that be sufficient? Why do we want to
> > change this? (explain in CL description or comment here)
> 
> Reverting only the change of && -> || would reintroduce the initial bug
> (https://crrev.com/1660273003).
> 
> FindBarView::UpdateMatchCountAppearance was the wrong place to fix the issue.
> The initial bug is caused by always calling FindBarView::ClearMatchCount,
> regardless of |find_text|. This temporarily disables the buttons and causes
the
> focus to jump to the close button (if they were focused before).
> If FindBarView::ClearMatchCount is only called when the text is empty, the
focus
> stays on the buttons as they are not getting temporarily disabled.

Okay, that's reasonable; thanks for the fix and explanation.

[Mario Pistrich, 2016-03-14 19:02:48]

The CQ bit was checked by m.pistrich@gmail.com

[commit-bot: I haz the power, 2016-03-14 19:03:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1785033004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1785033004/1

[commit-bot: I haz the power, 2016-03-14 20:23:01]

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

[commit-bot: I haz the power, 2016-03-14 20:23:02]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mario Pistrich, 2016-03-14 20:56:34]

The CQ bit was checked by m.pistrich@gmail.com

[commit-bot: I haz the power, 2016-03-14 20:56:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1785033004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1785033004/1

[commit-bot: I haz the power, 2016-03-14 21:29:55]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-03-14 21:31:08]

Description was changed from

==========
Disable Find-In-Page buttons when no text is entered

This CL fixes a regression caused by https://crrev.com/1660273003 which
enables the buttons of the Find-In-Page bar even if no text is entered
or no search was issued yet.

R=msw@chromium.org
BUG=594097
TEST=FindInPageTest.ButtonsDisabledWithoutText
==========

to

==========
Disable Find-In-Page buttons when no text is entered

This CL fixes a regression caused by https://crrev.com/1660273003 which
enables the buttons of the Find-In-Page bar even if no text is entered
or no search was issued yet.

R=msw@chromium.org
BUG=594097
TEST=FindInPageTest.ButtonsDisabledWithoutText

Committed: https://crrev.com/afc0733e0630499bea163c4f3cf826c945e7baaf
Cr-Commit-Position: refs/heads/master@{#381074}
==========

[commit-bot: I haz the power, 2016-03-14 21:31:09]

Patchset 1 (id:??) landed as
https://crrev.com/afc0733e0630499bea163c4f3cf826c945e7baaf
Cr-Commit-Position: refs/heads/master@{#381074}