Make sure active match is updated correctly in various cases.

In cases where there are only one match, active match ordinal might not
update properly due to prevention of update of UI. In other cases when
Find-in-page is forcefully redone due to DidFinishLoad call, previously
found active match is not reactivated correctly. This CL fixes both
of them.

Bug: 890622
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: Icdb287af7a4cd27bdfb70dba1f53e12bd46d4723
Reviewed-on: https://chromium-review.googlesource.com/c/1256393Reviewed-by: Hayato Ito <hayato@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596543}

chrome/browser/ui/find_bar/find_bar_controller.cc

@@ -243,7 +243,6 @@ void FindBarController::UpdateFindBarForCurrentResult() {
   FindTabHelper* find_tab_helper =
       FindTabHelper::FromWebContents(web_contents_);
   const FindNotificationDetails& find_result = find_tab_helper->find_result();
-
   // Avoid bug 894389: When a new search starts (and finds something) it reports
   // an interim match count result of 1 before the scoping effort starts. This
   // is to provide feedback as early as possible that we will find something.
@@ -252,11 +251,13 @@ void FindBarController::UpdateFindBarForCurrentResult() {
   // the scoping effort starts updating the match count. We avoid this flash by
   // ignoring interim results of 1 if we already have a positive number.
   if (find_result.number_of_matches() > -1) {
-    if (last_reported_matchcount_ > 0 &&
-        find_result.number_of_matches() == 1 &&
-        !find_result.final_update())
+    if (last_reported_matchcount_ > 0 && find_result.number_of_matches() == 1 &&
+        !find_result.final_update() &&
+        last_reported_ordinal_ == find_result.active_match_ordinal()) {
       return;  // Don't let interim result override match count.
+    }
     last_reported_matchcount_ = find_result.number_of_matches();
+    last_reported_ordinal_ = find_result.active_match_ordinal();
   }
 
   find_bar_->UpdateUIForFindResult(find_result, find_tab_helper->find_text());

chrome/browser/ui/find_bar/find_bar_controller.h

@@ -112,9 +112,10 @@ class FindBarController : public content::NotificationObserver {
 
   std::unique_ptr<FindBarPlatformHelper> find_bar_platform_helper_;
 
-  // The last match count we reported to the user. This is used by
-  // UpdateFindBarForCurrentResult to avoid flickering.
+  // The last match count and ordinal we reported to the user. This is used
+  // by UpdateFindBarForCurrentResult to avoid flickering.
   int last_reported_matchcount_ = 0;
+  int last_reported_ordinal_ = 0;
 
   // Used to keep track of whether the user has been notified that the find came
   // up empty. A single find session can result in multiple final updates, if

third_party/blink/renderer/core/editing/finder/text_finder.cc

@@ -153,11 +153,17 @@ bool TextFinder::Find(int identifier,
                       const mojom::blink::FindOptions& options,
                       bool wrap_within_frame,
                       bool* active_now) {
-  if (!options.find_next)
+  if (!options.find_next) {
+    // This find-in-page is redone due to the frame finishing loading.
+    // If we can, just reuse the old active match;
+    if (options.force && active_match_) {
+      should_locate_active_rect_ = true;
+      return true;
+    }
     UnmarkAllTextMatches();
-  else
+  } else {
     SetMarkerActive(active_match_.Get(), false);
-
+  }
   if (active_match_ &&
       &active_match_->OwnerDocument() != OwnerFrame().GetFrame()->GetDocument())
     active_match_ = nullptr;
@@ -230,7 +236,7 @@ bool TextFinder::Find(int identifier,
     // Find-next due to DOM alteration (that couldn't be set as active), so
     // we set the flag to ask the scoping effort to find the active rect for
     // us and report it back to the UI.
-    locating_active_rect_ = true;
+    should_locate_active_rect_ = true;
   } else {
     if (!was_active_frame) {
       if (options.forward)
@@ -491,7 +497,7 @@ void TextFinder::ScopeStringMatches(IdleDeadline* deadline,
     // as the active rect.
     IntRect result_bounds = result_range->BoundingBox();
     IntRect active_selection_rect;
-    if (locating_active_rect_) {
+    if (should_locate_active_rect_) {
       active_selection_rect =
           active_match_.Get() ? active_match_->BoundingBox() : result_bounds;
     }
@@ -501,14 +507,14 @@ void TextFinder::ScopeStringMatches(IdleDeadline* deadline,
     // find this rect during scoping it means we have found the active
     // tickmark.
     bool found_active_match = false;
-    if (locating_active_rect_ && (active_selection_rect == result_bounds)) {
+    if (should_locate_active_rect_ && active_selection_rect == result_bounds) {
       // We have found the active tickmark frame.
       current_active_match_frame_ = true;
       found_active_match = true;
       // We also know which tickmark is active now.
       active_match_index_ = total_match_count_ + match_count - 1;
       // To stop looking for the active tickmark, we set this flag.
-      locating_active_rect_ = false;
+      should_locate_active_rect_ = false;
 
       // Notify browser of new location for the selected rectangle.
       ReportFindInPageSelection(
@@ -821,7 +827,7 @@ TextFinder::TextFinder(WebLocalFrameImpl& owner_frame)
       find_request_identifier_(-1),
       next_invalidate_after_(0),
       find_match_markers_version_(0),
-      locating_active_rect_(false),
+      should_locate_active_rect_(false),
       scoping_in_progress_(false),
       last_find_request_completed_with_no_matches_(false),
       find_match_rects_are_valid_(false) {}

third_party/blink/renderer/core/editing/finder/text_finder.h

@@ -279,7 +279,7 @@ class CORE_EXPORT TextFinder final
   // This flag is used by the scoping effort to determine if we need to figure
   // out which rectangle is the active match. Once we find the active
   // rectangle we clear this flag.
-  bool locating_active_rect_;
+  bool should_locate_active_rect_;
 
   // Keeps track of whether there is an scoping effort ongoing in the frame.
   bool scoping_in_progress_;

third_party/blink/renderer/core/exported/web_frame_test.cc

@@ -5615,6 +5615,61 @@ TEST_F(WebFrameTest, FindInPageStopFindActionKeepSelectionInAnotherDocument) {
   // Pass if not crash. See http://crbug.com/719880 for details.
 }
 
+TEST_F(WebFrameTest, FindInPageForcedRedoOfFindInPage) {
+  const WebString search_pattern = WebString::FromUTF8("bar");
+  const char* html = "foo bar foo foo bar";
+  FrameTestHelpers::TestWebFrameClient frame_client;
+  FrameTestHelpers::WebViewHelper web_view_helper;
+  web_view_helper.Initialize(&frame_client);
+
+  WebLocalFrameImpl* frame = web_view_helper.LocalMainFrame();
+  FrameTestHelpers::LoadHTMLString(frame, html,
+                                   URLTestHelpers::ToKURL(base_url_));
+  web_view_helper.Resize(WebSize(640, 480));
+  web_view_helper.GetWebView()->SetFocus(true);
+  RunPendingTasks();
+
+  TestFindInPageClient find_in_page_client;
+  find_in_page_client.SetFrame(frame);
+  const int kFindIdentifier = 12345;
+
+  auto options = mojom::blink::FindOptions::New();
+  options->run_synchronously_for_testing = true;
+  options->find_next = false;
+  options->forward = true;
+  // First run.
+  frame->GetFindInPage()->Find(kFindIdentifier, search_pattern,
+                               options->Clone());
+  RunPendingTasks();
+  EXPECT_EQ(2, find_in_page_client.Count());
+  EXPECT_EQ(1, find_in_page_client.ActiveIndex());
+
+  options->force = true;
+  frame->GetFindInPage()->Find(kFindIdentifier, search_pattern,
+                               options->Clone());
+  RunPendingTasks();
+  EXPECT_EQ(2, find_in_page_client.Count());
+  EXPECT_EQ(1, find_in_page_client.ActiveIndex());
+
+  options->find_next = true;
+  options->force = false;
+
+  frame->GetFindInPage()->Find(kFindIdentifier, search_pattern,
+                               options->Clone());
+  RunPendingTasks();
+  EXPECT_EQ(2, find_in_page_client.Count());
+  EXPECT_EQ(2, find_in_page_client.ActiveIndex());
+
+  options->find_next = false;
+  options->force = true;
+
+  frame->GetFindInPage()->Find(kFindIdentifier, search_pattern,
+                               options->Clone());
+  RunPendingTasks();
+  EXPECT_EQ(2, find_in_page_client.Count());
+  EXPECT_EQ(2, find_in_page_client.ActiveIndex());
+}
+
 static WebPoint TopLeft(const WebRect& rect) {
   return WebPoint(rect.x, rect.y);
 }