Changes to how FindRequestManager reports find results.

content/browser/find_request_manager.cc

 // Copyright 2016 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 "content/browser/find_request_manager.h"
 
 #include "content/browser/frame_host/render_frame_host_impl.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/common/frame_messages.h"
 #include "content/common/input_messages.h"
@@ skipping 73 matching lines @@
 FindRequestManager::FindMatchRectsState::~FindMatchRectsState() {}
 #endif
 
 // static
 const int FindRequestManager::kInvalidId = -1;
 
 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),
       relative_active_match_ordinal_(0),
       active_match_ordinal_(0) {}
 
 FindRequestManager::~FindRequestManager() {}
 
 void FindRequestManager::Find(int request_id,
                               const base::string16& search_text,
@@ skipping 73 matching lines @@
       active_frame_ = rfh;
       relative_active_match_ordinal_ = active_match_ordinal;
       UpdateActiveMatchOrdinal();
     }
     if (pending_active_match_ordinal_ && request_id == current_request_.id)
       pending_active_match_ordinal_ = false;
     AdvanceQueue(request_id);
   }
 
   if (!final_update) {
     NotifyFindReply(request_id, false /* final_update */);

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

I noticed we can send NotifyFindReply with an request_id that's different than
the current request id. This is different from the old behavior, and from the
perspective of the WebContents it seems strange to be receiving a reply from an
find operation that's not the current one anymore.

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

That sounds reasonable. I don't think there are any problems with sending the
IDs out of order when the replies actually come out of order, but I agree that
its a bit odd from the perspective of the receiver of the replies. I'll add a
field to keep track of the last reported ID, and make sure that future replies
have >= that ID.

     return;
   }
 
   // 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) {
   if (current_session_id_ == kInvalidId || !CheckFrame(rfh))
     return;
 
   // If matches are counted for the frame that is being removed, decrement the
   // match total before erasing that entry.
   auto it = matches_per_frame_.find(rfh);
   if (it != matches_per_frame_.end()) {
     number_of_matches_ -= it->second;
     matches_per_frame_.erase(it);
   }
 
   // Update the active match ordinal, since it may have changed.
   if (active_frame_ == rfh) {
     active_frame_ = nullptr;
     relative_active_match_ordinal_ = 0;
+    selection_rect_ = gfx::Rect();
   }
   UpdateActiveMatchOrdinal();
 
 #if defined(OS_ANDROID)
   // The removed frame may contain the nearest find result known so far. Note
   // that once all queried frames have responded, if this result was the overall
   // nearest, then no activation will occur.
   if (rfh == activate_.nearest_frame)
     activate_.nearest_frame = nullptr;
 
   // Match rects in the removed frame are no longer relevant.
   if (match_rects_.frame_rects.count(rfh)) {
     match_rects_.frame_rects.erase(rfh);
     ++match_rects_.known_version;
   }
 
   // A reply should not be expected from the removed frame.
   RemoveNearestFindResultPendingReply(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)
 void FindRequestManager::ActivateNearestFindResult(float x, float y) {
   if (current_session_id_ == kInvalidId)
     return;
 
   activate_ = ActivateNearestFindResultState(x, y);
 
   // Request from each frame the distance to the nearest find result (in that
   // frame) from the point (x, y), defined in find-in-page coordinates.
   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);
     rfh->Send(new FrameMsg_GetNearestFindResult(
         rfh->GetRoutingID(), activate_.current_request_id,
         activate_.x, activate_.y));
   }
 }
 
 void FindRequestManager::OnGetNearestFindResultReply(RenderFrameHost* rfh,
@@ skipping 14 matching lines @@
 }
 
 void FindRequestManager::RequestFindMatchRects(int current_version) {
   match_rects_.pending_replies.clear();
   match_rects_.request_version = current_version;
 
   // Request the latest find match rects from each frame.
   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);
     auto it = match_rects_.frame_rects.find(rfh);
     int version = (it != match_rects_.frame_rects.end())
         ? it->second.version : kInvalidId;
     rfh->Send(new FrameMsg_FindMatchRects(rfh->GetRoutingID(), version));
   }
 }
 
@@ skipping 23 matching lines @@
   RemoveFrame(old_host);
 }
 
 void FindRequestManager::FrameDeleted(RenderFrameHost* rfh) {
   RemoveFrame(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;
   active_frame_ = nullptr;
   relative_active_match_ordinal_ = 0;
   active_match_ordinal_ = 0;
   selection_rect_ = gfx::Rect();
 #if defined(OS_ANDROID)
   activate_ = ActivateNearestFindResultState();
   match_rects_.pending_replies.clear();
@@ skipping 36 matching lines @@
     return;
   }
 
   find_request_queue_.pop();
   if (!find_request_queue_.empty())
     FindInternal(find_request_queue_.front());
 }
 
 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));
 }
 
 void FindRequestManager::NotifyFindReply(int request_id,
                                          bool final_update) const {
   if (request_id == kInvalidId) {
     NOTREACHED();
     return;
   }
@@ skipping 16 matching lines @@
                                               bool matches_only,
                                               bool wrap) const {
   FrameTreeNode* node =
       static_cast<RenderFrameHostImpl*>(from_rfh)->frame_tree_node();
 
   while ((node = TraverseNode(node, forward, wrap)) != nullptr) {
     if (!CheckFrame(node->current_frame_host()))
       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|.
       return node->current_frame_host();
     }
     if (wrap && node->current_frame_host() == from_rfh)
       return nullptr;
   }
 
   return nullptr;
 }
 
 void FindRequestManager::AddFrame(RenderFrameHost* rfh) {
   if (!rfh || !rfh->IsRenderFrameLive())
     return;
 
   // 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;
   SendFindIPC(request, 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() {
   active_match_ordinal_ = 0;
 
   if (!active_frame_ || !relative_active_match_ordinal_) {
     DCHECK(!active_frame_ && !relative_active_match_ordinal_);
     return;
   }
 
   // Traverse the frame tree backwards (in search order) and count all of the
   // matches in frames before the frame with the active match, in order to
   // determine the overall active match ordinal.
   RenderFrameHost* frame = active_frame_;
   while ((frame = Traverse(frame,
                            false /* forward */,
                            true /* matches_only */,
                            false /* wrap */)) != nullptr) {
     active_match_ordinal_ += matches_per_frame_[frame];
   }
   active_match_ordinal_ += relative_active_match_ordinal_;
 }
 
 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;
   }
 
   // There are matches, but no active match was returned, so another find next
   // request must be sent.
 
   RenderFrameHost* target_rfh;
   if (current_request_.options.findNext) {
     // If this was a find next operation, then the active match will be in the
@@ skipping 72 matching lines @@
             aggregate_rects.end(), frame_rects.begin(), frame_rects.end());
       }
     }
     contents_->NotifyFindMatchRectsReply(
         match_rects_.known_version, aggregate_rects, match_rects_.active_rect);
   }
 }
 #endif
 
 }  // namespace content