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

content/browser/find_request_manager.cc

Index: content/browser/find_request_manager.cc
diff --git a/content/browser/find_request_manager.cc b/content/browser/find_request_manager.cc
index db706a283a7ccf9267e7669048611d6fac0939c9..5c2eeb2df374da1d18e384648cea298a81708156 100644
--- a/content/browser/find_request_manager.cc
+++ b/content/browser/find_request_manager.cc
@@ -151,11 +151,18 @@ 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);
-    number_of_matches_ += number_of_matches - matches_per_frame_it->second;
-    matches_per_frame_it->second = number_of_matches;
+    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;

[dcheng, 2016/09/21 07:15:44]

This used to get set to 0, now I'm not sure where that happens?

[paulmeyer, 2016/09/21 15:40:52]

I'm not sure exactly what you mean. It is possible that |number_of_matches| is
0, which would set |matches_per_frame_it->second| to 0. Or, if you mean the
initial state of each entry in |matches_per_frame_|, that is initialized to 0 in
FindRequestManager::AddFrame().

[dcheng, 2016/09/22 00:36:47]

  if (int matches_delta = ...)

If this evaluates to zero, this branch is never taken, and
matches_per_frame_it->second is never set.

[paulmeyer, 2016/09/22 15:11:42]

Oh, if that evaluates to 0, then |matches_per_frame_it->second| is already equal
to |number_of_matches|, and doesn't need to be set.

+
+      // The active match ordinal may need updating since the number of matches
+      // before the active match may have changed.
+      if (rfh != active_frame_)
+        UpdateActiveMatchOrdinal();
+    }
   }
 
   // Check for an update to the selection rect.
@@ -346,6 +353,15 @@ 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);
 }
@@ -405,7 +421,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());
+    AddFrame(node->current_frame_host(), false /* force */);
 }
 
 void FindRequestManager::AdvanceQueue(int request_id) {
@@ -484,18 +500,19 @@ RenderFrameHost* FindRequestManager::Traverse(RenderFrameHost* from_rfh,
   return nullptr;
 }
 
-void FindRequestManager::AddFrame(RenderFrameHost* rfh) {
+void FindRequestManager::AddFrame(RenderFrameHost* rfh, bool force) {
   if (!rfh || !rfh->IsRenderFrameLive())
     return;
 
-  // A frame that is already being searched should not be added again.
-  DCHECK(!CheckFrame(rfh));
+  // A frame that is already being searched should not normally be added again.
+  DCHECK(force || !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);
 }
 
@@ -528,7 +545,7 @@ void FindRequestManager::FinalUpdateReceived(int request_id,
                                              RenderFrameHost* rfh) {
   if (!number_of_matches_ ||
       (active_match_ordinal_ && !pending_active_match_ordinal_) ||
-      request_id != current_request_.id) {
+      pending_find_next_reply_) {

[dcheng, 2016/09/21 07:15:44]

Can you help me understand the purpose of the next two changes? I don't know the
find logic quite as well.

[paulmeyer, 2016/09/21 15:40:52]

First change:

So, really, this part of the check was always supposed to detect whether there
would be another reply coming, and |request_id != current_request_.id| was a
good approximation of that before |pending_find_next_reply_| existed (it is a
more recent addition to the class that was needed for other reasons as well).
Also, there were previously no cases where this would do the wrong thing. Now,
this change is needed to allow the second part of this function to be reached in
the case where a frame is added/navigated with matches that will come in a find
reply that is not associated specifically with the current request. If after the
frame change, there are matches in the page, but there is not yet an active
match and no other replies expected, the second part of this function will
ensure that an active match will still be found (there must always be an active
match if there are any matches on the page).


Second change:

It was previously guaranteed that |request_id == current_request_.id| was true
when reaching this part of the function, and that was relied upon. Now that it
is not guaranteed, it is checked explicitly.

[dcheng, 2016/09/22 00:36:47]

I think I only vaguely understand this, so I'll just assume it's right =)

     // 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.
@@ -542,7 +559,7 @@ void FindRequestManager::FinalUpdateReceived(int request_id,
   // request must be sent.
 
   RenderFrameHost* target_rfh;
-  if (current_request_.options.findNext) {
+  if (request_id == current_request_.id && 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,