Revert of Handling new frames and frame navigations with find-in-page during a...

Revert of Handling new frames and frame navigations with find-in-page during a find session. (patchset #2 id:120001 of https://codereview.chromium.org/2236403004/ )

Reason for revert:
This seems to be failing pretty consistently on some bots (e.g. https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29)

Original issue's description:
> Handling new frames and frame navigations with find-in-page during a find session.
>
> This patch implements new find-in-page functionality that allows for frames that are either newly added or navigated during a find session to be automatically searched so that matches in their new content are automatically highlighted and included in the results shown in the find bar.
>
> Also, there is a fix included in this patch to prevent the find-in-page bar from closing when a subframe navigates (the find session should only end if the main frame navigates).
>
> Design doc: https://docs.google.com/document/d/1r4F19FIKg4zPJCaSyJ9-T0sgFbxlCGKL3FjqQEAKegg/view
>
> BUG=2220,621660
>
> Committed: https://crrev.com/ec9c7f30eb10ab3fc9ab6707f7b736446ab89aa1
> Cr-Commit-Position: refs/heads/master@{#420715}

TBR=dcheng@chromium.org,pkasting@chromium.org,kenrb@chromium.org,paulmeyer@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=2220,621660

Review-Url: https://codereview.chromium.org/2363993003
Cr-Commit-Position: refs/heads/master@{#420774}

chrome/browser/ui/find_bar/find_bar_controller.cc

@@ -159,7 +159,7 @@ void FindBarController::Observe(int type,
       ui::PageTransition transition_type =
           commit_details->entry->GetTransitionType();
       // Hide the find bar on reload or navigation.
-      if (find_bar_->IsFindBarVisible() && commit_details->is_main_frame &&
+      if (find_bar_->IsFindBarVisible() &&
           (ui::PageTransitionCoreTypeIs(transition_type,
                                         ui::PAGE_TRANSITION_RELOAD) ||
            commit_details->is_navigation_to_different_page()))

content/browser/find_request_manager.cc

@@ -151,18 +151,11 @@ void FindRequestManager::OnFindReply(RenderFrameHost* rfh,
   // Check for an update to the number of matches.
   if (number_of_matches != -1) {
     DCHECK_GE(number_of_matches, 0);
+    // Increment the global number of matches by the number of additional
+    // matches found for this frame.
     auto matches_per_frame_it = matches_per_frame_.find(rfh);
-    if (int matches_delta = number_of_matches - matches_per_frame_it->second) {
-      // Increment the global number of matches by the number of additional
-      // matches found for this frame.
-      number_of_matches_ += matches_delta;
-      matches_per_frame_it->second = number_of_matches;
-
-      // The active match ordinal may need updating since the number of matches
-      // before the active match may have changed.
-      if (rfh != active_frame_)
-        UpdateActiveMatchOrdinal();
-    }
+    number_of_matches_ += number_of_matches - matches_per_frame_it->second;
+    matches_per_frame_it->second = number_of_matches;
   }
 
   // Check for an update to the selection rect.
@@ -353,15 +346,6 @@ void FindRequestManager::OnFindMatchRectsReply(
 }
 #endif
 
-void FindRequestManager::DidFinishLoad(RenderFrameHost* rfh,
-                                       const GURL& validated_url) {
-  if (current_session_id_ == kInvalidId)
-    return;
-
-  RemoveFrame(rfh);
-  AddFrame(rfh, true /* force */);
-}
-
 void FindRequestManager::RenderFrameDeleted(RenderFrameHost* rfh) {
   RemoveFrame(rfh);
 }
@@ -421,7 +405,7 @@ void FindRequestManager::FindInternal(const FindRequest& request) {
   // This is an initial find operation.
   Reset(request);
   for (FrameTreeNode* node : contents_->GetFrameTree()->Nodes())
-    AddFrame(node->current_frame_host(), false /* force */);
+    AddFrame(node->current_frame_host());
 }
 
 void FindRequestManager::AdvanceQueue(int request_id) {
@@ -500,19 +484,18 @@ RenderFrameHost* FindRequestManager::Traverse(RenderFrameHost* from_rfh,
   return nullptr;
 }
 
-void FindRequestManager::AddFrame(RenderFrameHost* rfh, bool force) {
+void FindRequestManager::AddFrame(RenderFrameHost* rfh) {
   if (!rfh || !rfh->IsRenderFrameLive())
     return;
 
-  // A frame that is already being searched should not normally be added again.
-  DCHECK(force || !CheckFrame(rfh));
+  // A frame that is already being searched should not be added again.
+  DCHECK(!CheckFrame(rfh));
 
   matches_per_frame_[rfh] = 0;
 
   FindRequest request = current_request_;
   request.id = current_session_id_;
   request.options.findNext = false;
-  request.options.force = force;
   SendFindIPC(request, rfh);
 }
 
@@ -545,7 +528,7 @@ void FindRequestManager::FinalUpdateReceived(int request_id,
                                              RenderFrameHost* rfh) {
   if (!number_of_matches_ ||
       (active_match_ordinal_ && !pending_active_match_ordinal_) ||
-      pending_find_next_reply_) {
+      request_id != current_request_.id) {
     // All the find results for |request_id| are in and ready to report. Note
     // that |final_update| will be set to false if there are still pending
     // replies expected from the initial find request.
@@ -559,7 +542,7 @@ void FindRequestManager::FinalUpdateReceived(int request_id,
   // request must be sent.
 
   RenderFrameHost* target_rfh;
-  if (request_id == current_request_.id && current_request_.options.findNext) {
+  if (current_request_.options.findNext) {
     // If this was a find next operation, then the active match will be in the
     // next frame with matches after this one.
     target_rfh = Traverse(rfh,

content/browser/find_request_manager.h

@@ -103,7 +103,6 @@ class CONTENT_EXPORT FindRequestManager : public WebContentsObserver {
   };
 
   // WebContentsObserver implementation.
-  void DidFinishLoad(RenderFrameHost* rfh, const GURL& validated_url) override;
   void RenderFrameDeleted(RenderFrameHost* rfh) override;
   void RenderFrameHostChanged(RenderFrameHost* old_host,
                               RenderFrameHost* new_host) override;
@@ -146,11 +145,8 @@ class CONTENT_EXPORT FindRequestManager : public WebContentsObserver {
 
   // Adds a frame to the set of frames that are being searched. The new frame
   // will automatically be searched when added, using the same options (stored
-  // in |current_request_.options|). |force| should be set to true when a
-  // dynamic content change is suspected, which will treat the frame as a newly
-  // added frame even if it has already been searched. This will force a
-  // re-search of the frame.
-  void AddFrame(RenderFrameHost* rfh, bool force);
+  // in |current_request_.options|).
+  void AddFrame(RenderFrameHost* rfh);
 
   // Returns whether |rfh| is in the set of frames being searched in the current
   // find session.

content/browser/find_request_manager_browsertest.cc

@@ -7,7 +7,6 @@
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/common/content_switches.h"
-#include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test.h"
 #include "content/public/test/content_browser_test_utils.h"
 #include "content/public/test/test_navigation_observer.h"
@@ -477,124 +476,8 @@ IN_PROC_BROWSER_TEST_P(FindRequestManagerTest, MAYBE(RemoveFrame)) {
   results = delegate()->GetFindResults();
   EXPECT_EQ(12, results.number_of_matches);
   EXPECT_EQ(8, results.active_match_ordinal);
-}
-
-// Tests adding a frame during a find session.
-IN_PROC_BROWSER_TEST_P(FindRequestManagerTest, MAYBE(AddFrame)) {
-  LoadMultiFramePage(2 /* height */, GetParam() /* cross_process */);
 
-  blink::WebFindOptions options;
-  Find("result", options);
-  options.findNext = true;
-  Find("result", options);
-  Find("result", options);
-  Find("result", options);
-  Find("result", options);
-  delegate()->WaitForFinalReply();
-
-  FindResults results = delegate()->GetFindResults();
-  EXPECT_EQ(last_request_id(), results.request_id);
-  EXPECT_EQ(21, results.number_of_matches);
-  EXPECT_EQ(5, results.active_match_ordinal);
-
-  // Add a frame. It contains 5 new matches.
-  std::string url = embedded_test_server()->GetURL(
-      GetParam() ? "b.com" : "a.com", "/find_in_simple_page.html").spec();
-  std::string script = std::string() +
-      "var frame = document.createElement('iframe');" +
-      "frame.src = '" + url + "';" +
-      "document.body.appendChild(frame);";
-  delegate()->MarkNextReply();
-  ASSERT_TRUE(ExecuteScript(shell(), script));
-  delegate()->WaitForNextReply();
-
-  // The number of matches should update automatically to include the matches
-  // from the newly added frame.
-  results = delegate()->GetFindResults();
-  EXPECT_EQ(26, results.number_of_matches);
-  EXPECT_EQ(5, results.active_match_ordinal);
-}
-
-// Tests adding a frame during a find session where there were previously no
-// matches.
-IN_PROC_BROWSER_TEST_F(FindRequestManagerTest, MAYBE(AddFrameAfterNoMatches)) {
-  TestNavigationObserver navigation_observer(contents());
-  NavigateToURL(shell(), GURL("about:blank"));
-  EXPECT_TRUE(navigation_observer.last_navigation_succeeded());
-
-  blink::WebFindOptions default_options;
-  Find("result", default_options);
-  delegate()->WaitForFinalReply();
-
-  // Initially, there are no matches on the page.
-  FindResults results = delegate()->GetFindResults();
-  EXPECT_EQ(last_request_id(), results.request_id);
-  EXPECT_EQ(0, results.number_of_matches);
-  EXPECT_EQ(0, results.active_match_ordinal);
-
-  // Add a frame. It contains 5 new matches.
-  std::string url =
-      embedded_test_server()->GetURL("/find_in_simple_page.html").spec();
-  std::string script = std::string() +
-      "var frame = document.createElement('iframe');" +
-      "frame.src = '" + url + "';" +
-      "document.body.appendChild(frame);";
-  delegate()->MarkNextReply();
-  ASSERT_TRUE(ExecuteScript(shell(), script));
-  delegate()->WaitForNextReply();
-
-  // The matches from the new frame should be found automatically, and the first
-  // match in the frame should be activated.
-  results = delegate()->GetFindResults();
-  EXPECT_EQ(5, results.number_of_matches);
-  EXPECT_EQ(1, results.active_match_ordinal);
-}
-
-// Tests a frame navigating to a different page during a find session.
-IN_PROC_BROWSER_TEST_P(FindRequestManagerTest, MAYBE(NavigateFrame)) {
-  LoadMultiFramePage(2 /* height */, GetParam() /* cross_process */);
-
-  blink::WebFindOptions options;
-  Find("result", options);
-  options.findNext = true;
-  options.forward = false;
-  Find("result", options);
-  Find("result", options);
-  Find("result", options);
-  delegate()->WaitForFinalReply();
-
-  FindResults results = delegate()->GetFindResults();
-  EXPECT_EQ(last_request_id(), results.request_id);
-  EXPECT_EQ(21, results.number_of_matches);
-  EXPECT_EQ(19, results.active_match_ordinal);
-
-  // Navigate one of the empty frames to a page with 5 matches.
-  FrameTreeNode* root =
-      static_cast<WebContentsImpl*>(shell()->web_contents())->
-      GetFrameTree()->root();
-  GURL url(embedded_test_server()->GetURL(
-      GetParam() ? "b.com" : "a.com", "/find_in_simple_page.html"));
-  delegate()->MarkNextReply();
-  TestNavigationObserver navigation_observer(contents());
-  NavigateFrameToURL(root->child_at(0)->child_at(1)->child_at(0), url);
-  EXPECT_TRUE(navigation_observer.last_navigation_succeeded());
-  delegate()->WaitForNextReply();
-
-  // The navigation results in an extra reply before the one we care about. This
-  // extra reply happens because the RenderFrameHost changes before it navigates
-  // (because the navigation is cross-origin). The first reply will not change
-  // the number of matches because the frame that is navigating was empty
-  // before.
-  if (delegate()->GetFindResults().number_of_matches == 21) {
-    delegate()->MarkNextReply();
-    delegate()->WaitForNextReply();
-  }
-
-  // The number of matches and the active match ordinal should update
-  // automatically to include the new matches.
-  results = delegate()->GetFindResults();
-  EXPECT_EQ(26, results.number_of_matches);
-  EXPECT_EQ(24, results.active_match_ordinal);
+  // TODO(paulemeyer): Once adding frames mid-session is handled, test that too.
 }
 
 // Tests Searching in a hidden frame. Matches in the hidden frame should be
@@ -696,6 +579,7 @@ IN_PROC_BROWSER_TEST_F(FindRequestManagerTest, MAYBE(FindInPage_Issue644448)) {
   results = delegate()->GetFindResults();
   EXPECT_EQ(last_request_id(), results.request_id);
   EXPECT_EQ(5, results.number_of_matches);
+  EXPECT_EQ(1, results.active_match_ordinal);
 }
 
 #if defined(OS_ANDROID)

content/common/frame_messages.h

@@ -112,7 +112,6 @@ IPC_STRUCT_TRAITS_BEGIN(blink::WebFindOptions)
   IPC_STRUCT_TRAITS_MEMBER(forward)
   IPC_STRUCT_TRAITS_MEMBER(matchCase)
   IPC_STRUCT_TRAITS_MEMBER(findNext)
-  IPC_STRUCT_TRAITS_MEMBER(force)
 IPC_STRUCT_TRAITS_END()
 
 IPC_STRUCT_TRAITS_BEGIN(content::ColorSuggestion)

third_party/WebKit/Source/web/TextFinder.cpp

@@ -200,6 +200,7 @@ bool TextFinder::find(int identifier, const WebString& searchText, const WebFind
         reportFindInPageSelection(selectionRect, m_activeMatchIndex + 1, identifier);
     }
 
+    m_lastFindRequestCompletedWithNoMatches = false;
     return true;
 }
 
@@ -207,7 +208,6 @@ void TextFinder::clearActiveFindMatch()
 {
     m_currentActiveMatchFrame = false;
     setMarkerActive(m_activeMatch.get(), false);
-    resetActiveMatch();
 }
 
 void TextFinder::stopFindingAndClearSelection()
@@ -285,7 +285,7 @@ void TextFinder::scopeStringMatches(int identifier, const WebString& searchText,
         return;
     }
 
-    if (!shouldScopeMatches(searchText, options)) {
+    if (!shouldScopeMatches(searchText)) {
         finishCurrentScopingEffort(identifier);
         return;
     }
@@ -673,23 +673,17 @@ void TextFinder::unmarkAllTextMatches()
         frame->document()->markers().removeMarkers(DocumentMarker::TextMatch);
 }
 
-bool TextFinder::shouldScopeMatches(const String& searchText, const WebFindOptions& options)
+bool TextFinder::shouldScopeMatches(const String& searchText)
 {
     // Don't scope if we can't find a frame or a view.
     // The user may have closed the tab/application, so abort.
     LocalFrame* frame = ownerFrame().frame();
-    if (!frame || !frame->view() || !frame->page())
+    if (!frame || !frame->view() || !frame->page() || !ownerFrame().hasVisibleContent())
         return false;
 
     DCHECK(frame->document());
     DCHECK(frame->view());
 
-    if (options.force)
-        return true;
-
-    if (!ownerFrame().hasVisibleContent())
-        return false;
-
     // If the frame completed the scoping operation and found 0 matches the last
     // time it was searched, then we don't have to search it again if the user is
     // just adding to the search string or sending the same search string again.

third_party/WebKit/Source/web/TextFinder.h

@@ -167,7 +167,7 @@ private:
     // It is not necessary if the frame is invisible, for example, or if this
     // is a repeat search that already returned nothing last time the same prefix
     // was searched.
-    bool shouldScopeMatches(const WTF::String& searchText, const WebFindOptions&);
+    bool shouldScopeMatches(const WTF::String& searchText);
 
     // Removes the current frame from the global scoping effort and triggers any
     // updates if appropriate. This method does not mark the scoping operation

third_party/WebKit/Source/web/WebLocalFrameImpl.cpp

@@ -1974,7 +1974,7 @@ void WebLocalFrameImpl::didCallIsSearchProviderInstalled()
 void WebLocalFrameImpl::requestFind(int identifier, const WebString& searchText, const WebFindOptions& options)
 {
     // Send "no results" if this frame has no visible content.
-    if (!hasVisibleContent() && !options.force) {
+    if (!hasVisibleContent()) {
         client()->reportFindInPageMatchCount(identifier, 0 /* count */, true /* finalUpdate */);
         return;
     }

third_party/WebKit/public/web/WebFindOptions.h

@@ -53,17 +53,12 @@ struct WebFindOptions {
     // an uppercase letter followed by a lowercase or non-letter. Accepts several other intra-word matches.
     bool medialCapitalAsWordStart;
 
-    // Force a re-search of the frame: typically used when forcing a re-search
-    // after the frame navigates.
-    bool force;
-
     WebFindOptions()
         : forward(true)
         , matchCase(false)
         , findNext(false)
         , wordStart(false)
-        , medialCapitalAsWordStart(false)
-        , force(false) { }
+        , medialCapitalAsWordStart(false) { }
 };
 
 } // namespace blink