Fix extraneous beeps on empty page search.

chrome/browser/ui/find_bar/find_bar_controller.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/find_bar/find_bar_controller.h"
 
 #include <algorithm>
 
 #include "base/i18n/rtl.h"
 #include "base/logging.h"
@@ skipping 40 matching lines @@
     find_tab_helper->set_find_ui_active(true);
     find_bar_->Show(true);
   }
   find_bar_->SetFocusAndSelection();
 }
 
 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_) {
     FindTabHelper* find_tab_helper =
         FindTabHelper::FromWebContents(web_contents_);
 
     // When we hide the window, we need to notify the renderer that we are done
     // for now, so that we can abort the scoping effort and clear all the
     // tickmarks and highlighting.
     find_tab_helper->StopFinding(selection_action);
@@ skipping 61 matching lines @@
 void FindBarController::Observe(int type,
                                 const content::NotificationSource& source,
                                 const content::NotificationDetails& details) {
   FindTabHelper* find_tab_helper =
       FindTabHelper::FromWebContents(web_contents_);
   if (type == chrome::NOTIFICATION_FIND_RESULT_AVAILABLE) {
     // Don't update for notifications from WebContentses other than the one we
     // 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 :)

         }
       }
     }
   } else if (type == content::NOTIFICATION_NAV_ENTRY_COMMITTED) {
     NavigationController* source_controller =
         content::Source<NavigationController>(source).ptr();
     if (source_controller == &web_contents_->GetController()) {
       content::LoadCommittedDetails* commit_details =
           content::Details<content::LoadCommittedDetails>(details).ptr();
       ui::PageTransition transition_type =
@@ skipping 93 matching lines @@
   }
 
   // Update the find bar with existing results and search text, regardless of
   // whether or not the find bar is visible, so that if it's subsequently
   // shown it is showing the right state for this tab. We update the find text
   // _first_ since the FindBarView checks its emptiness to see if it should
   // clear the result count display when there's nothing in the box.
   find_bar_->SetFindTextAndSelectedRange(find_string,
                                          find_tab_helper->selected_range());
 }