Changes to how FindRequestManager reports find results.

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 ba97753698a77a8e8f00fe80181909fb083d17a5..09415ec800a4785b62fea855aad2bb9c5ddcc7a3 100644
--- a/content/browser/find_request_manager.cc
+++ b/content/browser/find_request_manager.cc
@@ -91,6 +91,7 @@ FindRequestManager::FindRequestManager(WebContentsImpl* web_contents)
     : WebContentsObserver(web_contents),
       contents_(web_contents),
       current_session_id_(kInvalidId),
+      pending_find_next_reply_(nullptr),
       pending_active_match_ordinal_(false),
       number_of_matches_(0),
       active_frame_(nullptr),
@@ -190,16 +191,27 @@ void FindRequestManager::OnFindReply(RenderFrameHost* rfh,
 
   // This is the final update for this frame for the current find operation.
 
-  pending_replies_.erase(rfh);
-  if (request_id == current_session_id_ && !pending_replies_.empty()) {
-    NotifyFindReply(request_id, false /* final_update */);
+  pending_initial_replies_.erase(rfh);
+  if (request_id == current_session_id_ && !pending_initial_replies_.empty())

[lfg, 2016/08/18 18:50:42]

Why did you remove the NotifyFindReply() from this reply? It seems that the
final update from any frame should still be reported, even though it's not the
final reply.

[paulmeyer, 2016/08/29 22:27:41]

I can't remember why I did that. It may have been accidental. I agree that it
should report here.

     return;
-  }
-  DCHECK(request_id == current_session_id_ ||
-         current_request_.options.findNext);
 
   // This is the final update for the current find operation.
-  FinalUpdate(request_id, rfh);
+
+  if (request_id == current_request_.id && request_id != current_session_id_) {
+    DCHECK(current_request_.options.findNext);
+    DCHECK(pending_find_next_reply_ == rfh);

[lfg, 2016/08/18 18:50:42]

nit: DCHECK_EQ.

[paulmeyer, 2016/08/29 22:27:41]

Done.

+    pending_find_next_reply_ = nullptr;
+  }
+
+  // The find results are normally reported under the ID of the find reply they
+  // came from (|request_id|), but if this really is the last update, then they
+  // need to be reported with the latest find request ID
+  // (|current_request_.id|), so that the receiver can know for sure that all
+  // issued find requests have completed.

[lfg, 2016/08/18 18:50:42]

See my comment above, I don't think we should ever return a find ID that's
different than the current one.

[paulmeyer, 2016/08/29 22:27:41]

Done.

+  int report_id = request_id;
+  if (pending_initial_replies_.empty() && !pending_find_next_reply_)
+    report_id = current_request_.id;
+  FinalUpdate(report_id, rfh);
 }
 
 void FindRequestManager::RemoveFrame(RenderFrameHost* rfh) {
@@ -218,6 +230,7 @@ void FindRequestManager::RemoveFrame(RenderFrameHost* rfh) {
   if (active_frame_ == rfh) {
     active_frame_ = nullptr;
     relative_active_match_ordinal_ = 0;
+    selection_rect_ = gfx::Rect();
   }
   UpdateActiveMatchOrdinal();
 
@@ -239,17 +252,28 @@ void FindRequestManager::RemoveFrame(RenderFrameHost* rfh) {
   RemoveFindMatchRectsPendingReply(rfh);
 #endif
 
-  if (pending_replies_.count(rfh)) {
+  // If no pending find replies are expected for the removed frame, then just
+  // report the updated results.
+  if (!pending_initial_replies_.count(rfh) && pending_find_next_reply_ != rfh) {
+    bool final_update =
+        pending_initial_replies_.empty() && !pending_find_next_reply_;
+    NotifyFindReply(current_session_id_, final_update);

[lfg, 2016/08/18 18:50:42]

I don't understand why we avoid calling FinalUpdate() here when final_update is
true.

[paulmeyer, 2016/08/29 22:27:41]

See comment on FinalUpdate(). In this case, a frame is being removed that there
was no expected reply from, so this removal doesn't interact with an outstanding
find request and so can't be the final update of any request. That being said,
if ALL requests are already finished, then we still want to mark this reply as
final since no more are coming.

[lfg, 2016/08/30 14:57:38]

Is it possible that the final update will be sent twice in this case? For
example, all the frames report back, a final update is sent, then a frame is
removed and another final update is sent with the same request id?

[paulmeyer, 2016/08/30 15:40:12]

Yes, that is possible, and intended. Once all frames report back, the last
update from FindRequestManager should be final, since it does not intend to send
another one (it can't predict or expect that a frame will ever be removed). If a
frame is then removed, FindRequestManager needs to send another update, and
again it doesn't expect to send another, so that update is final again.

This is all fine from the perspective of the WebContentsDelegate. In fact, it
used to get even more final updates before this change.

+    return;
+  }
+
+  if (pending_initial_replies_.count(rfh)) {
     // A reply should not be expected from the removed frame.
-    pending_replies_.erase(rfh);
-    if (pending_replies_.empty()) {
-      FinalUpdate(current_request_.id, rfh);
-      return;
+    pending_initial_replies_.erase(rfh);
+    if (pending_initial_replies_.empty()) {
+      FinalUpdate(current_session_id_, rfh);

[lfg, 2016/08/18 18:50:42]

Do we want to send a FinalUpdate if pending_find_next_reply_ != null?

[paulmeyer, 2016/08/29 22:27:41]

See comment on FinalUpdate().

     }
   }
 
-  NotifyFindReply(current_session_id_,
-                  pending_replies_.empty() /* final_update */);
+  if (pending_find_next_reply_ == rfh) {
+    // A reply should not be expected from the removed frame.
+    pending_find_next_reply_ = nullptr;
+    FinalUpdate(current_request_.id, rfh);

[lfg, 2016/08/18 18:50:42]

Same as above, do we want to send a FinalUpdate if
pending_initial_replies_.empty() == false?

[paulmeyer, 2016/08/29 22:27:41]

See comment on FinalUpdate().

+  }
 }
 
 #if defined(OS_ANDROID)
@@ -264,7 +288,7 @@ void FindRequestManager::ActivateNearestFindResult(float x, float y) {
   for (FrameTreeNode* node : contents_->GetFrameTree()->Nodes()) {
     RenderFrameHost* rfh = node->current_frame_host();
 
-    if (!CheckFrame(rfh))
+    if (!CheckFrame(rfh) || !rfh->IsRenderFrameLive())
       continue;
 
     activate_.pending_replies.insert(rfh);
@@ -299,7 +323,7 @@ void FindRequestManager::RequestFindMatchRects(int current_version) {
   for (FrameTreeNode* node : contents_->GetFrameTree()->Nodes()) {
     RenderFrameHost* rfh = node->current_frame_host();
 
-    if (!CheckFrame(rfh))
+    if (!CheckFrame(rfh) || !rfh->IsRenderFrameLive())
       continue;
 
     match_rects_.pending_replies.insert(rfh);
@@ -343,7 +367,8 @@ void FindRequestManager::FrameDeleted(RenderFrameHost* rfh) {
 void FindRequestManager::Reset(const FindRequest& initial_request) {
   current_session_id_ = initial_request.id;
   current_request_ = initial_request;
-  pending_replies_.clear();
+  pending_initial_replies_.clear();
+  pending_find_next_reply_ = nullptr;
   pending_active_match_ordinal_ = true;
   matches_per_frame_.clear();
   number_of_matches_ = 0;
@@ -400,7 +425,14 @@ void FindRequestManager::AdvanceQueue(int request_id) {
 
 void FindRequestManager::SendFindIPC(const FindRequest& request,
                                      RenderFrameHost* rfh) {
-  pending_replies_.insert(rfh);
+  DCHECK(CheckFrame(rfh));
+  DCHECK(rfh->IsRenderFrameLive());
+
+  if (request.options.findNext)
+    pending_find_next_reply_ = rfh;
+  else
+    pending_initial_replies_.insert(rfh);
+
   rfh->Send(new FrameMsg_Find(rfh->GetRoutingID(), request.id,
                               request.search_text, request.options));
 }
@@ -437,7 +469,7 @@ RenderFrameHost* FindRequestManager::Traverse(RenderFrameHost* from_rfh,
       continue;
     RenderFrameHost* current_rfh = node->current_frame_host();
     if (!matches_only || matches_per_frame_.find(current_rfh)->second ||
-        pending_replies_.count(current_rfh)) {
+        pending_initial_replies_.count(current_rfh)) {
       // Note that if there is still a pending reply expected for this frame,
       // then it may have unaccounted matches and will not be skipped via
       // |matches_only|.
@@ -466,7 +498,7 @@ void FindRequestManager::AddFrame(RenderFrameHost* rfh) {
 }
 
 bool FindRequestManager::CheckFrame(RenderFrameHost* rfh) const {
-  return rfh && rfh->IsRenderFrameLive() && matches_per_frame_.count(rfh);
+  return rfh && matches_per_frame_.count(rfh);

[lfg, 2016/08/18 18:50:42]

I don't understand the reason for this change.

[paulmeyer, 2016/08/29 22:27:41]

I discovered that I don't actually want to check IsRenderFrameLive() n all cases
that CheckFrame() is called, so I took it out of here, and just put it next to
CheckFrame() in the cases where it actually matters.

[lfg, 2016/08/30 14:57:38]

Can you add add a comment in the places that this wasn't wanted explaining why
it's not necessary?

[paulmeyer, 2016/08/30 15:40:12]

I don't think that is necessary. If you look at the comment for CheckFrame() in
the header, it actually only claims to check whether |rfh| is one of the frames
searched in the current find session, which is sufficiently done by just
checking if it is in |matches_per_frame_|. It doesn't make any guarantee that
the frame is still live. When that extra property is needed, it is checked
explicitly.

[lfg, 2016/08/30 15:43:23]

Acknowledged.

 }
 
 void FindRequestManager::UpdateActiveMatchOrdinal() {
@@ -492,9 +524,13 @@ void FindRequestManager::UpdateActiveMatchOrdinal() {
 
 void FindRequestManager::FinalUpdate(int request_id, RenderFrameHost* rfh) {
   if (!number_of_matches_ ||
-      !pending_active_match_ordinal_ ||
+      (active_match_ordinal_ && !pending_active_match_ordinal_) ||
       request_id != current_request_.id) {
-    NotifyFindReply(request_id, true /* final_update */);
+    // 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.
+    NotifyFindReply(request_id,
+                    pending_initial_replies_.empty() /* final_update */);

[lfg, 2016/08/18 18:50:42]

The fact that FinalUpdate() doesn't always send a final update is confusing.
(which means some of my comments above are wrong).

[paulmeyer, 2016/08/29 22:27:41]

FinalUpdate() is called once all the replies have been received for a single
find request, which used to almost always result in the find reply from
FindRequestManager being marked final. Now I have purposely changed that because
of the case when one or more find next requests finish before the initial find
request completes (in all frames). In that case, the reply outputted from
FindRequestManager should not be marked final.

[lfg, 2016/08/30 14:57:38]

Can you update the comment on the header to reflect this change?

[paulmeyer, 2016/08/30 15:40:12]

Done. I also renamed FinalUpdate() to FinalUpdateReceived(), which I think more
clearly indicates that it is called when a final update is received for a
particular find request, not necessarily when a final update is going to be sent
by FindRequestManager.

     AdvanceQueue(request_id);
     return;
   }