Fix extraneous beeps on empty page search.

chrome/browser/ui/find_bar/find_bar_controller.cc

Index: chrome/browser/ui/find_bar/find_bar_controller.cc
diff --git a/chrome/browser/ui/find_bar/find_bar_controller.cc b/chrome/browser/ui/find_bar/find_bar_controller.cc
index db9cfcefd123182f20140dccf9165747410969e3..b5a089d5b759ffcd8775e66bbb8c5994c438aea7 100644
--- a/chrome/browser/ui/find_bar/find_bar_controller.cc
+++ b/chrome/browser/ui/find_bar/find_bar_controller.cc
@@ -58,6 +58,10 @@ void FindBarController::EndFindSession(SelectionAction selection_action,
                                        ResultAction result_action) {
   find_bar_->Hide(true);
 
+  // If the user searches again for this string, it should notify if the result
+  // comes back empty again.
+  last_reported_empty_search_.clear();
+
   // |web_contents_| can be NULL for a number of reasons, for example when the
   // tab is closing. We must guard against that case. See issue 8030.
   if (web_contents_) {
@@ -139,14 +143,27 @@ void FindBarController::Observe(int type,
     // are actively tracking.
     if (content::Source<WebContents>(source).ptr() == web_contents_) {
       UpdateFindBarForCurrentResult();
+
+      // A final update can occur multiple times if the document changes.
       if (find_tab_helper->find_result().final_update() &&
           find_tab_helper->find_result().number_of_matches() == 0) {
         const base::string16& last_search =
             find_tab_helper->previous_find_text();
         const base::string16& current_search = find_tab_helper->find_text();
-        if (!base::StartsWith(last_search, current_search,
-                              base::CompareCase::SENSITIVE)) {
-          find_bar_->AudibleAlert();
+
+        // Make sure only the first "final update" of any search results in a
+        // notification.
+        if (current_search != last_reported_empty_search_) {
+          // True if the current search is a substring of the previous one, such
+          // as if the user has backspaced.
+          const bool is_contracted_search = base::StartsWith(
+              last_search, current_search, base::CompareCase::SENSITIVE);
+
+          // Keep track of the last notified search string, even if the
+          // notification itself is elided.
+          last_reported_empty_search_ = current_search;
+          if (!is_contracted_search)
+            find_bar_->AudibleAlert();

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

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.

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

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 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 :)
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 :)

         }
       }
     }