Fix extraneous beeps on empty page search.

Fix extraneous beeps on empty page search.

As of https://crrev.com/3ac612d592cd42355a432c8d307051d299d79768 a
find-in-page can issue multiple "final updates" if the document changes.
Each time a final update with zero results occurs, there is an audbible
notification which is a nuisance beyond the first one. Particularly
bothersome is that most or all pages cause a ding as you close them if
they have an active find in page with zero results.

BUG=682299
Review-Url: https://codereview.chromium.org/2839713002
Cr-Commit-Position: refs/heads/master@{#467417}
Committed: https://chromium.googlesource.com/chromium/src/+/59192dd2dd5d23372ca3ace498327f1c690e61ac

[Sigurður Ásgeirsson, 2017-04-24 18:21:01]

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

[commit-bot: I haz the power, 2017-04-24 18:21:54]

Dry run: CQ is trying da patch.

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

[Sigurður Ásgeirsson, 2017-04-24 18:22:14]

siggi@chromium.org changed reviewers:
+ paulmeyer@chromium.org, pkasting@chromium.org

[Sigurður Ásgeirsson, 2017-04-24 18:22:15]

Paul, please take a look - I think this is along the lines you were thinking?

Peter for OWNERs, kindly.

[elawrence, 2017-04-24 18:50:27]

On 2017/04/24 18:22:15, Sigurður Ásgeirsson wrote:
> Paul, please take a look - I think this is along the lines you were thinking?
> 
> Peter for OWNERs, kindly.

Will this work properly if the user searches for the same string in a different
tab? 

My approach to this (https://codereview.chromium.org/2800163002) resolved that
problem by tracking the query in find_tab_helper.

[commit-bot: I haz the power, 2017-04-24 19:13:25]

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

[commit-bot: I haz the power, 2017-04-24 19:13:26]

Dry run: This issue passed the CQ dry run.

[Sigurður Ásgeirsson, 2017-04-24 20:18:57]

Good point, this also fails to notify on empty results if the search is
cancelled and redone in the same tab. Fixing.

On Mon, Apr 24, 2017 at 2:50 PM <elawrence@chromium.org> wrote:

> On 2017/04/24 18:22:15, Sigurður Ásgeirsson wrote:
> > Paul, please take a look - I think this is along the lines you were
> thinking?
> >
> > Peter for OWNERs, kindly.
>
> Will this work properly if the user searches for the same string in a
> different
> tab?
>
> My approach to this (https://codereview.chromium.org/2800163002) resolved
> that
> problem by tracking the query in find_tab_helper.
>
>
>
> https://codereview.chromium.org/2839713002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Sigurður Ásgeirsson, 2017-04-24 20:31:47]

Now clears the notification state when the session ends. This avoids conflating
the notification state across tabs.

[paulmeyer, 2017-04-24 21:35:47]

On 2017/04/24 20:31:47, Sigurður Ásgeirsson wrote:
> Now clears the notification state when the session ends. This avoids
conflating
> the notification state across tabs.

With that fix, LGTM.

[Peter Kasting, 2017-04-24 22:25:46]

Please add a test for this.

LGTM once that's in place.

https://codereview.chromium.org/2839713002/diff/20001/chrome/browser/ui/find_...
File chrome/browser/ui/find_bar/find_bar_controller.cc (right):

https://codereview.chromium.org/2839713002/diff/20001/chrome/browser/ui/find_...
chrome/browser/ui/find_bar/find_bar_controller.cc:166:
find_bar_->AudibleAlert();
Nit: It's simpler, and I would find it clearer, to rewrite the entire above as
follows:

        // Alert the user once per unique search, if they aren't backspacing.
        if ((current_search != alerted_search_) &&
            !base::StartsWith(last_search, current_search,
                              base::CompareCase::SENSITIVE))
          find_bar_->AudibleAlert();
        alerted_search_ = current_search;

You'll notice I renamed |last_reported_empty_search_| to |alerted_search_|.  I
had two goals: (1) Avoid "last X search" in code already locally using "last
search", because it sounds like the "last search" is updated more frequently
than the "last reported empty search", but in fact the opposite is true.  (2)
Use the same terminology as the code: the issue is whether we've "alert"ed the
user, so name the variable to indicate we're tracking that.

[elawrence, 2017-04-25 16:02:45]

On 2017/04/24 20:31:47, Sigurður Ásgeirsson wrote:
> Now clears the notification state when the session ends. This avoids
conflating
> the notification state across tabs.

Can you confirm that your change works in the following case on Windows (where
each tab has its own search value):

1. Open https://whytls.com/test/frames.htm and tick the random cycle box
2. Enter a search string that fails. Hear the ding. Leave the search bar open.
3. Open another tab to whereever. Ctrl+F, perform a search that fails. Hear the
ding.
4. Go back to the first tab and close it. (No dings)

[Sigurður Ásgeirsson, 2017-04-26 16:52:41]

> Can you confirm that your change works in the following case on Windows (where
> each tab has its own search value):
> 
> 1. Open https://whytls.com/test/frames.htm and tick the random cycle box
> 2. Enter a search string that fails. Hear the ding. Leave the search bar open.
> 3. Open another tab to whereever. Ctrl+F, perform a search that fails. Hear
the
> ding.
> 4. Go back to the first tab and close it. (No dings)

Yups this works just fine.

[Sigurður Ásgeirsson, 2017-04-26 16:53:36]

I'd love some pointers on how to write a test for this. It doesn't look like any
of existing tests have a good hookup to add testing for this behavior?

https://codereview.chromium.org/2839713002/diff/20001/chrome/browser/ui/find_...
File chrome/browser/ui/find_bar/find_bar_controller.cc (right):

https://codereview.chromium.org/2839713002/diff/20001/chrome/browser/ui/find_...
chrome/browser/ui/find_bar/find_bar_controller.cc:166:
find_bar_->AudibleAlert();
On 2017/04/24 22:25:46, Peter Kasting wrote:
> Nit: It's simpler, and I would find it clearer, to rewrite the entire above as
> follows:
> 
>         // Alert the user once per unique search, if they aren't backspacing.
>         if ((current_search != alerted_search_) &&
>             !base::StartsWith(last_search, current_search,
>                               base::CompareCase::SENSITIVE))
>           find_bar_->AudibleAlert();
>         alerted_search_ = current_search;
> 
> You'll notice I renamed |last_reported_empty_search_| to |alerted_search_|.  I
> had two goals: (1) Avoid "last X search" in code already locally using "last
> search", because it sounds like the "last search" is updated more frequently
> than the "last reported empty search", but in fact the opposite is true.  (2)
> Use the same terminology as the code: the issue is whether we've "alert"ed the
> user, so name the variable to indicate we're tracking that.

I like "alerted_search_", much better - thanks!

Now, while conceptually identical, I don't like your suggested code for two
reasons:
1. It took me forever to parse the intent with the StartsWith condition. Maybe
I'm just stupid, but by using a temporary variable, at least the *intent* is
somewhat self-documenting.

2. Doesn't matter here - any more than it matters everywhere else - but your
suggested code performs a redundant string assignment on identical searches.
Optimistically this causes two heap operations and a bunch of copying, which is
another cut to the thousands that Chrome is slowly dying by. Heap churn in
Chrome is a an atrocious problem (try running with
--enable-profiling=heap-tracking and go to chrome://profiler, see what I mean),
and I'm loath to compound it for the sake of readability, even in this small
way.

[Peter Kasting, 2017-04-26 17:22:12]

I enjoy principled discussions :)

(To be super clear, I am fine with this change landing in any state whatsoever,
the comments below do not block)

https://codereview.chromium.org/2839713002/diff/20001/chrome/browser/ui/find_...
File chrome/browser/ui/find_bar/find_bar_controller.cc (right):

https://codereview.chromium.org/2839713002/diff/20001/chrome/browser/ui/find_...
chrome/browser/ui/find_bar/find_bar_controller.cc:166:
find_bar_->AudibleAlert();
On 2017/04/26 16:53:36, Sigurður Ásgeirsson wrote:
> On 2017/04/24 22:25:46, Peter Kasting wrote:
> > Nit: It's simpler, and I would find it clearer, to rewrite the entire above
as
> > follows:
> > 
> >         // Alert the user once per unique search, if they aren't
backspacing.
> >         if ((current_search != alerted_search_) &&
> >             !base::StartsWith(last_search, current_search,
> >                               base::CompareCase::SENSITIVE))
> >           find_bar_->AudibleAlert();
> >         alerted_search_ = current_search;
> > 
> > You'll notice I renamed |last_reported_empty_search_| to |alerted_search_|. 
I
> > had two goals: (1) Avoid "last X search" in code already locally using "last
> > search", because it sounds like the "last search" is updated more frequently
> > than the "last reported empty search", but in fact the opposite is true. 
(2)
> > Use the same terminology as the code: the issue is whether we've "alert"ed
the
> > user, so name the variable to indicate we're tracking that.
> 
> I like "alerted_search_", much better - thanks!
> 
> Now, while conceptually identical, I don't like your suggested code for two
> reasons:
> 1. It took me forever to parse the intent with the StartsWith condition. Maybe
> I'm just stupid, but by using a temporary variable, at least the *intent* is
> somewhat self-documenting.

I actually found "is contracted search" a really confusing name, so I struggled
to read your code.  Hence the comment in mine about "if they aren't backspacing"
-- the hope was that since the comment says "A, if not B" and then the code says
"if (A && !B)" the correspondence would be clear.  Maybe not, in which case I
could see using a temp to try to call this out better -- but I would try to pick
a better name :)

> 2. Doesn't matter here - any more than it matters everywhere else - but your
> suggested code performs a redundant string assignment on identical searches.
> Optimistically this causes two heap operations and a bunch of copying, which
is
> another cut to the thousands that Chrome is slowly dying by. Heap churn in
> Chrome is a an atrocious problem (try running with
> --enable-profiling=heap-tracking and go to chrome://profiler, see what I
mean),
> and I'm loath to compound it for the sake of readability, even in this small
> way.

I think this is an issue worth discussing, since it applies to code elsewhere.

What I read your argument as is "we should always minimize heap churn for any
readability cost".  I don't agree with this (and I hope you wouldn't either, in
the limit), so it becomes more about balancing the tradeoff of heap churn and
readability.  In this case, the readability win is minor, but the heap churn is
also minor, because this code isn't running 10,000 times per second, it's
running once per final update and doing two heap operations.  I struggle to
imagine that having any measurable effect on things.  So I would be reluctant to
take even the smallest readability hit for this -- if there actually is a
readability hit :)

[Sigurður Ásgeirsson, 2017-04-26 18:06:35]

Thanks Peter, I'd like to meet you in the middle on this one. Per our offline
discussion, I'm going to commit this as-is, and try and get Finnur to help me
figure out how to test this. I'll leave the bug open until that's done.

[Sigurður Ásgeirsson, 2017-04-26 18:06:41]

The CQ bit was checked by siggi@chromium.org

[Sigurður Ásgeirsson, 2017-04-26 18:06:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
paulmeyer@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2839713002/#ps60001
(title: "Meet Peter in the middle.")

[commit-bot: I haz the power, 2017-04-26 18:07:14]

CQ is trying da patch.

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

[Sigurður Ásgeirsson, 2017-04-26 18:09:19]

The CQ bit was unchecked by siggi@chromium.org

[Sigurður Ásgeirsson, 2017-04-26 18:17:58]

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

[commit-bot: I haz the power, 2017-04-26 18:18:53]

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-04-26 19:29:06]

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

[commit-bot: I haz the power, 2017-04-26 19:29:08]

Dry run: This issue passed the CQ dry run.

[Sigurður Ásgeirsson, 2017-04-26 19:29:27]

The CQ bit was checked by siggi@chromium.org

[Sigurður Ásgeirsson, 2017-04-26 19:29:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
paulmeyer@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2839713002/#ps80001
(title: "Un-reverse the conditional I broke last patch.")

[commit-bot: I haz the power, 2017-04-26 19:30:43]

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-04-26 19:41:49]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493234967700570,
"parent_rev": "4540f48b0b6a2511b938b09986be4baee13a1520", "commit_rev":
"59192dd2dd5d23372ca3ace498327f1c690e61ac"}

[commit-bot: I haz the power, 2017-04-26 19:41:59]

Description was changed from

==========
Fix extraneous beeps on empty page search.

As of https://crrev.com/3ac612d592cd42355a432c8d307051d299d79768 a
find-in-page can issue multiple "final updates" if the document changes.
Each time a final update with zero results occurs, there is an audbible
notification which is a nuisance beyond the first one. Particularly
bothersome is that most or all pages cause a ding as you close them if
they have an active find in page with zero results.

BUG=682299
==========

to

==========
Fix extraneous beeps on empty page search.

As of https://crrev.com/3ac612d592cd42355a432c8d307051d299d79768 a
find-in-page can issue multiple "final updates" if the document changes.
Each time a final update with zero results occurs, there is an audbible
notification which is a nuisance beyond the first one. Particularly
bothersome is that most or all pages cause a ding as you close them if
they have an active find in page with zero results.

BUG=682299

Review-Url: https://codereview.chromium.org/2839713002
Cr-Commit-Position: refs/heads/master@{#467417}
Committed:
https://chromium.googlesource.com/chromium/src/+/59192dd2dd5d23372ca3ace49832...
==========

[commit-bot: I haz the power, 2017-04-26 19:42:02]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/59192dd2dd5d23372ca3ace49832...