Multi-Process Find-in-Page.

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index 75bb31eb4732ef4c141537b0b8d236f3bd05a214..2ffa6471706c6983c19385d046e2067ffd949aad 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -1460,13 +1460,16 @@ bool RenderFrameImpl::OnMessageReceived(const IPC::Message& msg) {
                         OnGetSerializedHtmlWithLocalLinks)
     IPC_MESSAGE_HANDLER(FrameMsg_SerializeAsMHTML, OnSerializeAsMHTML)
     IPC_MESSAGE_HANDLER(FrameMsg_Find, OnFind)
+    IPC_MESSAGE_HANDLER(FrameMsg_ClearActiveFindMatch, OnClearActiveFindMatch)
     IPC_MESSAGE_HANDLER(FrameMsg_StopFinding, OnStopFinding)
     IPC_MESSAGE_HANDLER(FrameMsg_EnableViewSourceMode, OnEnableViewSourceMode)
     IPC_MESSAGE_HANDLER(FrameMsg_SuppressFurtherDialogs,
                         OnSuppressFurtherDialogs)
 #if defined(OS_ANDROID)
-    IPC_MESSAGE_HANDLER(InputMsg_ActivateNearestFindResult,
+    IPC_MESSAGE_HANDLER(FrameMsg_ActivateNearestFindResult,
                         OnActivateNearestFindResult)
+    IPC_MESSAGE_HANDLER(FrameMsg_GetNearestFindResult,
+                        OnGetNearestFindResult)
     IPC_MESSAGE_HANDLER(FrameMsg_FindMatchRects, OnFindMatchRects)
     IPC_MESSAGE_HANDLER(FrameMsg_SelectPopupMenuItems, OnSelectPopupMenuItems)
 #elif defined(OS_MACOSX)
@@ -4072,20 +4075,19 @@ void RenderFrameImpl::willInsertBody(blink::WebLocalFrame* frame) {
 void RenderFrameImpl::reportFindInPageMatchCount(int request_id,
                                                  int count,
                                                  bool final_update) {
-  int active_match_ordinal = -1;  // -1 = don't update active match ordinal
-  if (!count)
-    active_match_ordinal = 0;
+  // -1 here means don't update the active match ordinal.
+  int active_match_ordinal = count ? -1 : 0;
 
-  Send(new FrameHostMsg_Find_Reply(routing_id_, request_id, count, gfx::Rect(),
-                                   active_match_ordinal, final_update));
+  SendFindReply(request_id, count, active_match_ordinal, gfx::Rect(),
+                final_update);
 }
 
 void RenderFrameImpl::reportFindInPageSelection(
     int request_id,
     int active_match_ordinal,
     const blink::WebRect& selection_rect) {
-  Send(new FrameHostMsg_Find_Reply(routing_id_, request_id, -1, selection_rect,
-                                   active_match_ordinal, false));
+  SendFindReply(request_id, -1 /* match_count */, active_match_ordinal,
+                selection_rect, false /* final_status_update */);
 }
 
 void RenderFrameImpl::requestStorageQuota(
@@ -5001,13 +5003,6 @@ void RenderFrameImpl::OnSerializeAsMHTML(
 void RenderFrameImpl::OnFind(int request_id,
                              const base::string16& search_text,
                              const WebFindOptions& options) {
-  // This should only be received on the main frame, since find-in-page is
-  // currently orchestrated by the main frame.
-  if (!is_main_frame_) {
-    NOTREACHED();
-    return;
-  }
-
   DCHECK(!search_text.empty());
 
   blink::WebPlugin* plugin = GetWebPluginForFind();
@@ -5019,126 +5014,69 @@ void RenderFrameImpl::OnFind(int request_id,
     } else {
       if (!plugin->startFind(search_text, options.matchCase, request_id)) {
         // Send "no results".
-        SendFindReply(request_id, 0, 0, gfx::Rect(), true);
+        SendFindReply(request_id, 0 /* match_count */, 0 /* ordinal */,
+                      gfx::Rect(), true /* final_status_update */ );
       }
     }
     return;
   }
 
-  WebLocalFrame* main_frame = GetWebFrame();
-  WebLocalFrame* focused_frame =
-      render_view_->webview()->focusedFrame()->toWebLocalFrame();
-  // Start searching in the focused frame.
-  WebLocalFrame* search_frame = focused_frame;
-
-  // Check for multiple searchable frames.
-  bool multi_frame = (main_frame->traverseNextLocal(true) != main_frame);
-
-  // If we have multiple frames, we don't want to wrap the search within the
-  // frame, so we check here if we only have |main_frame| in the chain.
-  bool wrap_within_frame = !multi_frame;
+  // Send "no results" if this frame has no visible content.
+  if (!frame_->hasVisibleContent()) {
+    SendFindReply(request_id, 0 /* match_count */, 0 /* ordinal */,
+                  gfx::Rect(), true /* final_status_update */ );
+    return;
+  }
 
   WebRect selection_rect;
-  bool result = false;
   bool active_now = false;
 
   // If something is selected when we start searching it means we cannot just
   // increment the current match ordinal; we need to re-generate it.
-  WebRange current_selection = focused_frame->selectionRange();
-
-  do {
-    result =
-        search_frame->find(request_id, search_text, options, wrap_within_frame,
-                           &selection_rect, &active_now);
-
-    if (!result) {
-      // Don't leave text selected as you move to the next frame.
-      search_frame->executeCommand(WebString::fromUTF8("Unselect"),
-                                   GetFocusedElement());
-
-      // Find the next frame, but skip the invisible ones.
-      do {
-        // What is the next frame to search (we might be going backwards)? Note
-        // that we specify wrap=true so that search_frame never becomes NULL.
-        search_frame = options.forward
-                ? search_frame->traverseNextLocal(true)
-                : search_frame->traversePreviousLocal(true);
-      } while (!search_frame->hasVisibleContent() &&
-               search_frame != focused_frame);
-
-      // Make sure selection doesn't affect the search operation in new frame.
-      search_frame->executeCommand(WebString::fromUTF8("Unselect"),
-                                   GetFocusedElement());
-
-      // If we have multiple frames and we have wrapped back around to the
-      // focused frame, we need to search it once more allowing wrap within
-      // the frame, otherwise it will report 'no match' if the focused frame has
-      // reported matches, but no frames after the focused_frame contain a
-      // match for the search word(s).
-      if (multi_frame && search_frame == focused_frame) {
-        result = search_frame->find(request_id, search_text, options,
-                                    true,  // Force wrapping.
-                                    &selection_rect, &active_now);
-      }
+  WebRange current_selection = frame_->selectionRange();
+
+  // Search for an active match only if this frame is focused or if this is a
+  // find next request.
+  if (frame_->isFocused() || options.findNext) {

[dcheng, 2016/05/14 00:03:44]

I think we can just use WebView::focusedFrame() == frame_.

[paulmeyer, 2016/05/16 15:25:11]

Actually, that wouldn't work. I tried that at some point, and it led to
confusing bugs. It turns out that WebView::focusedFrame() is not named very
appropriately, since it sometimes returns the focused frame, and sometimes
returns other frames (this isn't a bug though, that's what it's supposed to do,
it's just a really misleading name).

So, that's why I created the isFocused() method to properly determine whether
the frame has focus. This main difference is that isFocused() hooks into
FocusController::focusedFrame() instead of
FocusController::focusedOrMainFrame().

[dcheng, 2016/05/16 17:11:02]

It's confusing to have isFocused() and focusedFrame() not agree though =)

For example, in this particular block, it seems like Blink should be able to do
the check itself, so this can just be simplified to:

if (options.findNext) {
  bool result = frame_->find(...);
  if (result) {
    SendFindReply(...);
  }
}

[paulmeyer, 2016/05/16 17:21:10]

Blink could do the check, but what you put here is not equivalent. If the frame
is focused, it should be searched regardless of options.findNext. I also feel
that it is clearer what's going on here if we check for focus here. It's easy to
see here exactly under which conditions the search is performed.

[dcheng, 2016/05/16 17:31:37]

OK, in that case, we don't need the conditionals at all in //content. Just have
Blink do all this work: the remainder of this function is just using the Blink
public API anyway.
Moving this all into Blink will let us reduce the size of the Blink API and let
us avoid having multiple ways to get a focused frame. So the rest of this
function will just look like:

if (frame_->find(...))
  SendFindReply(...);
return;

[paulmeyer, 2016/05/16 19:27:49]

I could move all of this to Blink, but it still wouldn't remove the need for
IsFocused(). It's called here and in OnStopFinding. I could move both into
WebLocalFrame, but it still makes sense to have the method instead of calling
into FocusController the long way twice (though I could make it private).

Also, that method isn't a second way to get the focused frame. Right now it's
the only way to tell if the frame is focused, since WebView::focusedFrame
doesn't do that, and I think that's something useful to be able to know. I agree
that it's confusing that they don't agree, but that's only because
WebView::focusedFrame is confusing. I think the correct way to fix this would be
to rename WebView::focusedFrame to focusedOrMainFrame(), like the underlying
method in FocusController that it uses (though that name is also misleading,
since the method can also return a frame that is neither of those :P). That
would probably be a separate CL though.

[dcheng, 2016/05/16 20:04:21]

I don't think it should be needed in OnStopFinding. It doesn't make sense for
the browser to trigger a STOP_FIND_ACTION_ACTIVATE_SELECTION action for a
non-focused frame.
At least it won't be on the Blink public API.
It provides two ways to do the same thing. One way is to just use
WebView::focusedFrame(). The other is to loop through all frames looking for the
one with isFocused() == true =)

[paulmeyer, 2016/05/17 20:40:46]

Following our offline discussion, I've made WebLocalFrame::isFocused() private
so that it is out of the public API.

I agree that it doesn't make sense for STOP_FIND_ACTION_ACTIVATE_SELECTION to be
used on a non-focused frame, but I think it's still possible for that action to
be requested, so I'm going to keep that check. I moved that block into Blink so
that RenderFrameImpl doesn't need to use isFocused().

I also moved the part of RenderFrameImpl::OnFind() that used isFocused() into
Blink for the same reason, but I didn't move everything after it because that
would alter the order that find replies are sent, which I don't want to do.

Does that look good to you?

[dcheng, 2016/05/17 21:07:55]

Why would it happen? Doesn't that indicate a bug in the browser process code?
I don't think I understand: how does it change the order replies are sent? We're
in an async message handler here, so it shouldn't matter if we send the reply
out in the middle of the ipc handler or at the end: either way, the message is
queued to be sent and not necessarily sent immediately.

[paulmeyer, 2016/05/17 22:14:35]

No, not a bug. Not sure if there are other examples, but one is that the
<webview> tag find API can stop finding whenever it wants, and specify an
activate action with it, whether the active match is in a focused frame or not.
If it's in a non-focused frame, this condition will stop it. In any case, I
think it makes sense to enforce here that only a selection in the focused frame
will be activated, rather than rely on behavior elsewhere.
It changes the order because SendFindReply isn't the only place that a reply can
be sent from. For example, increaseMatchCount below will result in a reply as
well. If we wrap that all into find(), then that reply will be sent before
SendFindReply, whereas now it is sent after.

[dcheng, 2016/05/17 23:58:45]

Hmm. OK.
But find() can just report back directly by calling
reportFindInPageMatchCount(), right? So I don't think that should be a problem.

[paulmeyer, 2016/05/18 14:10:46]

find() could call that, but that would still result in that reply coming before
rather than after the reply from SendFindReply(). It wouldn't be impossible to
make that work, but I don't think that is within the scope of this CL. This CL
didn't introduce these calls here, so if they need to be cleaned up, it can be
done in a separate CL.

[dcheng, 2016/05/18 17:43:07]

Are you willing to commit to fixing this up in a followup CL then?

I find this logic rather difficult to follow, that's why I'm asking for these
changes =)

[paulmeyer, 2016/05/18 19:26:14]

Yeah, I could work on cleaning this up a bit in a followup CL, but it'll be
prioritized among other work. You can assign a bug to me about it.

I can see how this logic is a bit confusing, but all of the lines you're asking
about (and their logic) were already here before this CL. I just don't want to
confound an already large CL with additional cleanup on code that doesn't need
to be changed by this CL. I also don't want to risk potentially changing
something in the logic that might introduce subtle bugs at this late stage of
such a large implementation.

+    bool result  = frame_->find(request_id, search_text, options,
+                                false /* wrapWithinFrame */, &selection_rect,
+                                &active_now);
+    if (result) {
+      // Indicate that at least one match has been found. 1 here means possibly
+      // more matches could be coming. -1 here means that the exact active match
+      // ordinal is not yet known.
+      SendFindReply(request_id, 1 /* match_count */, -1 /* ordinal */,
+                    gfx::Rect(), false /* final_status_update */ );
     }
-
-    render_view_->webview()->setFocusedFrame(search_frame);
-  } while (!result && search_frame != focused_frame);
-
-  if (options.findNext && current_selection.isNull() && active_now) {
-    // Force the main_frame to report the actual count.
-    main_frame->increaseMatchCount(0, request_id);
-  } else {
-    // If nothing is found, set result to "0 of 0", otherwise, set it to
-    // "-1 of 1" to indicate that we found at least one item, but we don't know
-    // yet what is active.
-    int ordinal = result ? -1 : 0;  // -1 here means we might know more later.
-    int match_count = result ? 1 : 0;  // 1 here means possibly more coming.
-
-    // If we find no matches then this will be our last status update.
-    // Otherwise the scoping effort will send more results.
-    bool final_status_update = !result;
-
-    SendFindReply(request_id, match_count, ordinal, selection_rect,
-                  final_status_update);
-
-    // Scoping effort begins, starting with the main frame.
-    search_frame = main_frame;
-
-    main_frame->resetMatchCount();
-
-    do {
-      // Cancel all old scoping requests before starting a new one.
-      search_frame->cancelPendingScopingEffort();
-
-      // We don't start another scoping effort unless at least one match has
-      // been found.
-      if (result) {
-        // Start new scoping request. If the scoping function determines that it
-        // needs to scope, it will defer until later.
-        search_frame->scopeStringMatches(request_id, search_text, options,
-                                         true);  // reset the tickmarks
-      }
-
-      // Iterate to the next frame. The frame will not necessarily scope, for
-      // example if it is not visible.
-      search_frame = search_frame->traverseNextLocal(true);
-    } while (search_frame != main_frame);
   }
-}
 
-void RenderFrameImpl::OnStopFinding(StopFindAction action) {
-  // This should only be received on the main frame, since find-in-page is
-  // currently orchestrated by the main frame.
-  if (!is_main_frame_) {
-    NOTREACHED();
+  if (options.findNext && current_selection.isNull() && active_now) {
+    // Force report of the actual count.
+    frame_->increaseMatchCount(0, request_id);
     return;
   }
 
-  WebView* view = render_view_->webview();
-  if (!view)
-    return;
+  // Scoping effort begins.
+  frame_->resetMatchCount();
 
+  // Cancel all old scoping requests before starting a new one.
+  frame_->cancelPendingScopingEffort();
+
+  // Start new scoping request. If the scoping function determines that it
+  // needs to scope, it will defer until later.
+  frame_->scopeStringMatches(request_id,
+                             search_text,
+                             options,
+                             true);  // reset the tickmarks
+}
+
+void RenderFrameImpl::OnClearActiveFindMatch() {
+  frame_->executeCommand(WebString::fromUTF8("Unselect"),
+                         GetFocusedElement());
+  frame_->clearActiveFindMatch();
+}
+
+void RenderFrameImpl::OnStopFinding(StopFindAction action) {
   blink::WebPlugin* plugin = GetWebPluginForFind();
   if (plugin) {
     plugin->stopFind();
@@ -5147,25 +5085,18 @@ void RenderFrameImpl::OnStopFinding(StopFindAction action) {
 
   bool clear_selection = action == STOP_FIND_ACTION_CLEAR_SELECTION;
   if (clear_selection) {
-    view->focusedFrame()->executeCommand(WebString::fromUTF8("Unselect"),
-                                         GetFocusedElement());
+    frame_->executeCommand(WebString::fromUTF8("Unselect"),
+                           GetFocusedElement());
   }
 
-  WebLocalFrame* frame = GetWebFrame();
-  while (frame) {
-    frame->stopFinding(clear_selection);
-    frame = frame->traverseNextLocal(false);
-  }
+  frame_->stopFinding(clear_selection);
 
-  if (action == STOP_FIND_ACTION_ACTIVATE_SELECTION) {
-    WebFrame* focused_frame = view->focusedFrame();
-    if (focused_frame) {
-      WebDocument doc = focused_frame->document();
-      if (!doc.isNull()) {
-        WebElement element = doc.focusedElement();
-        if (!element.isNull())
-          element.simulateClick();
-      }
+  if (action == STOP_FIND_ACTION_ACTIVATE_SELECTION && frame_->isFocused()) {
+    WebDocument doc = frame_->document();
+    if (!doc.isNull()) {
+      WebElement element = doc.focusedElement();
+      if (!element.isNull())
+        element.simulateClick();
     }
   }
 }
@@ -5199,6 +5130,14 @@ void RenderFrameImpl::OnActivateNearestFindResult(int request_id,
                 true /* final_update */);
 }
 
+void RenderFrameImpl::OnGetNearestFindResult(int nfr_request_id,
+                                             float x,
+                                             float y) {
+  float distance = frame_->nearestFindMatch(WebFloatPoint(x, y));
+  Send(new FrameHostMsg_GetNearestFindResult_Reply(
+      routing_id_, nfr_request_id, distance));
+}
+
 void RenderFrameImpl::OnFindMatchRects(int current_version) {
   std::vector<gfx::RectF> match_rects;
 
@@ -6068,9 +6007,6 @@ blink::ServiceRegistry* RenderFrameImpl::serviceRegistry() {
 }
 
 blink::WebPlugin* RenderFrameImpl::GetWebPluginForFind() {
-  if (!is_main_frame_)
-    return nullptr;
-
   if (frame_->document().isPluginDocument())
     return frame_->document().to<WebPluginDocument>().plugin();
 
@@ -6087,8 +6023,17 @@ void RenderFrameImpl::SendFindReply(int request_id,
                                     int ordinal,
                                     const WebRect& selection_rect,
                                     bool final_status_update) {
-  Send(new FrameHostMsg_Find_Reply(routing_id_, request_id, match_count,
-                                   selection_rect, ordinal,
+  if (final_status_update && !ordinal) {
+    frame_->executeCommand(WebString::fromUTF8("Unselect"),
+                           GetFocusedElement());
+  }
+  DCHECK(ordinal >= -1);
+
+  Send(new FrameHostMsg_Find_Reply(routing_id_,
+                                   request_id,
+                                   match_count,
+                                   selection_rect,
+                                   ordinal,
                                    final_status_update));
 }