donnd@, can you provide us with an estimate when this work will be done? We have deployed out-of-process iframes (OOPIF) to the world, which means this code might not be functioning correctly when OOPIFs are present.

I have no good estimate for this change since it's not a priority (other than for supporting OOPIFs).

+cc Smart Selection folks that might have an interested in this code-cleanup.

I wonder if it would be better to make SelectWordAroundCaret and the SelectWordAroundCaretAck use Mojo.

donnd@ do we have any test so we can make sure the refactor won't break anything?

The SelectWordAroundCaretAck is vital to the functioning of the tap gesture for Contextual Search so it's well covered by automated tests, I believe.

Good to know we have tests.

It looks reasonable to put SelectWordAroundCaret() in https://cs.chromium.org/chromium/src/content/common/input/input_handler.mojom to use Mojo. However, for SelectWordAroundCaretAck(), I didn't see any existing place to put it in, maybe still need to use legacy IPC since the main purpose of this refactor is to handle OOPIF.

donnd@, changwan@, WDYT? I can start soon if this plan looks ok.

RE: #c7: for SelectWordAroundCaretAck(), I didn't see any existing place to put it in, maybe still need to use legacy IPC

Drive-by comment below - I hope it makes sense (and helps).

Looking at RenderViewImpl::OnSelectWordAroundCaret, I see that ViewHostMsg_SelectWordAroundCaretAck is always sent right after processing ViewMsg_SelectWordAroundCaret.  Therefore, I think there is no need for a *seperate* mojo method corresponding to SelectWordAroundCaretAck.  Instead, I think SelectWordAroundCaretAck can be modeled as an asynchronous mojo method result (i.e. the |=> ()| part below):

interface ... {
   ...

   SelectWordAroundCaret() => ();
}

This is based on my reading of mojo docs at https://chromium.googlesource.com/chromium/src/+/1085cbf44c1fbdfda0b946923aeaac92ef029029/mojo/public/tools/bindings#interfaces
and at https://chromium.googlesource.com/chromium/src/+/1085cbf44c1fbdfda0b946923aeaac92ef029029/mojo/public/cpp/bindings#Receiving-Responses

Using the mojo response for the Ack message makes sense to me, though I don't have much experience with Mojo.  

The browser just needs to know that the Renderer has completed processing the SelectWordAroundCaret and how the selection was changed by the SelectWordAroundCaret operation.  So I think the mojo interface looks more like:

interface ... {
   ...

   SelectWordAroundCaret() => (bool didSelect, int32 startAdjust, int32 endAdjust);
}

You're right - I missed that you need to send back didSelect+startAdjust+endAdjust.

Thank you lukasza@ for pointing out this type of usage of Mojo! It's indeed the way IMO.

oops.

dtapuska@, I am trying to add

interface ... {
   ...

   SelectWordAroundCaret() => (bool didSelect, int32 startAdjust, int32 endAdjust);
}

to FrameInputHandlerImpl, however, since FrameInputHandlerImpl has its own way to manage threads, I am keep hitting a DCHECK by calling the above callback:

[FATAL:interface_endpoint_client.cc(76)] Check failed: task_runner_->RunsTasksInCurrentSequence().

This is because the generated callback template expecting main thread but the responder in FrameInputHandlerImpl is compositor thread IIUC.

On the other hand, I tried to set the responder to main thread, by calling BindNow(std::move(request)); in the constructor of FrameInputHandlerImpl directly, it works fine, but probably not the correct way to do so.

Could you please give advice on this? Thanks! My draft cl http://crrev/c/821561.

twellington@, donnd@, amaralp@, I finally resolved the DCHECK issue I mentioned in #c13 (I am taking this to be part of my make it better week tasks). Now I notice that sometimes there is an assertion violation in Contextual Search code path [1]. I think there is a race condition between SelectionPopupControllerImpl.onSelectionChanged() and SelectionPopupControllerImpl.onSelectWordAroundCaretAck().

ContextualSearchManager.resolveSearchTerm() is assuming we already get the selection text before onSelectWordAroundCaretAck(), but from my experiment (http://crrev/c/821561), it isn't always true. With my refactor, it seems onSelecWordAroundCaretAck() arrives faster than before, which makes the situation worse :P.

In order to resolve this, we probably want some kind of synchronization mechanism, I am considering to sync onSelectWordAroundCaretAck() and onSelectionChanged() in SelectionPopupController, but want to know if CS could resolve this on it's end, since CS is the only consumer of onSelectWordAroundCaretAck() right now. Not familiar with CS code, so not sure how feasible though.

[1] https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java?rcl=76346c9b0afb2febbf2bf3c8d2785ce342e8ad0e&l=1688

09-24 16:36:48.941 31708 31708 W System.err: java.lang.AssertionError
09-24 16:36:48.941 31708 31708 W System.err: 	at org.chromium.chrome.browser.contextualsearch.ContextualSearchManager$6.resolveSearchTerm(ContextualSearchManager.java:1688)
09-24 16:36:48.942 31708 31708 W System.err: 	at org.chromium.chrome.browser.contextualsearch.ContextualSearchInternalStateController.startWorkingOn(ContextualSearchInternalStateController.java:284)
09-24 16:36:48.942 31708 31708 W System.err: 	at org.chromium.chrome.browser.contextualsearch.ContextualSearchInternalStateController.transitionTo(ContextualSearchInternalStateController.java:242)
09-24 16:36:48.942 31708 31708 W System.err: 	at org.chromium.chrome.browser.contextualsearch.ContextualSearchInternalStateController.transitionTo(ContextualSearchInternalStateController.java:223)
09-24 16:36:48.942 31708 31708 W System.err: 	at org.chromium.chrome.browser.contextualsearch.ContextualSearchInternalStateController.finishWorkingOn(ContextualSearchInternalStateController.java:359)
09-24 16:36:48.942 31708 31708 W System.err: 	at org.chromium.chrome.browser.contextualsearch.ContextualSearchInternalStateController.notifyFinishedWorkOn(ContextualSearchInternalStateController.java:207)
09-24 16:36:48.942 31708 31708 W System.err: 	at org.chromium.chrome.browser.contextualsearch.ContextualSearchManager$6.showContextualSearchTapUi(ContextualSearchManager.java:1704)
09-24 16:36:48.942 31708 31708 W System.err: 	at org.chromium.chrome.browser.contextualsearch.ContextualSearchInternalStateController.startWorkingOn(ContextualSearchInternalStateController.java:281)
09-24 16:36:48.942 31708 31708 W System.err: 	at org.chromium.chrome.browser.contextualsearch.ContextualSearchInternalStateController.transitionTo(ContextualSearchInternalStateController.java:242)
09-24 16:36:48.942 31708 31708 W System.err: 	at org.chromium.chrome.browser.contextualsearch.ContextualSearchInternalStateController.transitionTo(ContextualSearchInternalStateController.java:223)
09-24 16:36:48.942 31708 31708 W System.err: 	at org.chromium.chrome.browser.contextualsearch.ContextualSearchInternalStateController.finishWorkingOn(ContextualSearchInternalStateController.java:355)
09-24 16:36:48.942 31708 31708 W System.err: 	at org.chromium.chrome.browser.contextualsearch.ContextualSearchInternalStateController.notifyFinishedWorkOn(ContextualSearchInternalStateController.java:207)
09-24 16:36:48.942 31708 31708 W System.err: 	at org.chromium.chrome.browser.contextualsearch.ContextualSearchManager$ContextualSearchSelectionClient.selectWordAroundCaretAck(ContextualSearchManager.java:1330)
09-24 16:36:48.942 31708 31708 W System.err: 	at org.chromium.chrome.browser.contextualsearch.SelectionClientManager$SelectionClientBridge.selectWordAroundCaretAck(SelectionClientManager.java:167)
09-24 16:36:48.942 31708 31708 W System.err: 	at org.chromium.content.browser.selection.SelectionPopupControllerImpl.onSelectWordAroundCaretAck(SelectionPopupControllerImpl.java:1414)
09-24 16:36:48.942 31708 31708 W System.err: 	at android.os.MessageQueue.nativePollOnce(Native Method)
09-24 16:36:48.942 31708 31708 W System.err: 	at android.os.MessageQueue.next(MessageQueue.java:327)
09-24 16:36:48.942 31708 31708 W System.err: 	at android.os.Looper.loop(Looper.java:169)
09-24 16:36:48.942 31708 31708 W System.err: 	at android.app.ActivityThread.main(ActivityThread.java:6873)
09-24 16:36:48.942 31708 31708 W System.err: 	at java.lang.reflect.Method.invoke(Native Method)
09-24 16:36:48.943 31708 31708 W System.err: 	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:486)
09-24 16:36:48.943 31708 31708 W System.err: 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:837)

> I think there is a race condition between SelectionPopupControllerImpl.onSelectionChanged() and SelectionPopupControllerImpl.onSelectWordAroundCaretAck().
Pedro, do you remember discussing this a year or two ago?  IIRC we knew there would be a race if there were multiple IPCs, and there was an idea that the Smart Selection path would use another notification mechanism.  Does that ring a bell?

I have not looked at the Contextual Search DCHECK, but am happy to if that seems like the direction we should take.

I don't remember the conversation but there probably is a race condition. I agree with ctzsm@ that either we fix this on Contextual Search or do some sort of synchronization.

Synced with Donn and Pedro offline.

Donn will try to address the race by removing the dependency on selection string first (to utilize surrounding text). The other way we are thinking of is to pass the selection string along with SelectWordAroundCaretAck(). Thanks!

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e697c83235c50b57633fcb2b0af98664be5269ec

commit e697c83235c50b57633fcb2b0af98664be5269ec
Author: Shimi Zhang <ctzsm@chromium.org>
Date: Thu Oct 04 23:19:17 2018

Refactor SelectWordAroundCaret{Ack}()

This CL:
1) Moves SelectWordAroundCaret{Ack}() from RenderViewHost to
RenderFrameHost.
2) Moves SelectWordAroundCaret() from RenderView to FrameInputHandler,
the new place is more suitable for this function.
3) Migrates IPC to mojo, and does other IPC cleanup.

Bug:  716618 , 545684
Change-Id: Ied823345dc7ff68ea6dd505eeb14ab5c6aca17d1
Reviewed-on: https://chromium-review.googlesource.com/c/821561
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Shimi Zhang <ctzsm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596914}
[modify] https://crrev.com/e697c83235c50b57633fcb2b0af98664be5269ec/content/browser/renderer_host/render_view_host_impl.cc
[modify] https://crrev.com/e697c83235c50b57633fcb2b0af98664be5269ec/content/browser/renderer_host/render_view_host_impl.h
[modify] https://crrev.com/e697c83235c50b57633fcb2b0af98664be5269ec/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/e697c83235c50b57633fcb2b0af98664be5269ec/content/browser/renderer_host/render_widget_host_view_android.h
[modify] https://crrev.com/e697c83235c50b57633fcb2b0af98664be5269ec/content/browser/web_contents/web_contents_android.cc
[modify] https://crrev.com/e697c83235c50b57633fcb2b0af98664be5269ec/content/browser/web_contents/web_contents_android.h
[modify] https://crrev.com/e697c83235c50b57633fcb2b0af98664be5269ec/content/common/input/input_handler.mojom
[modify] https://crrev.com/e697c83235c50b57633fcb2b0af98664be5269ec/content/common/view_messages.h
[modify] https://crrev.com/e697c83235c50b57633fcb2b0af98664be5269ec/content/public/browser/render_view_host.h
[modify] https://crrev.com/e697c83235c50b57633fcb2b0af98664be5269ec/content/renderer/input/frame_input_handler_impl.cc
[modify] https://crrev.com/e697c83235c50b57633fcb2b0af98664be5269ec/content/renderer/input/frame_input_handler_impl.h
[modify] https://crrev.com/e697c83235c50b57633fcb2b0af98664be5269ec/content/renderer/render_view_impl.cc
[modify] https://crrev.com/e697c83235c50b57633fcb2b0af98664be5269ec/content/renderer/render_view_impl.h