OOPIF IME: Renderer Side Changes

third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp

Index: third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp
diff --git a/third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp b/third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp
index 5b8b0654e78eb10ef558300e1352531057ca82be..c02e234784f63bdc9f04856bd8f25cb3169eb119 100644
--- a/third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp
+++ b/third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp
@@ -51,6 +51,7 @@
 #include "platform/KeyboardCodes.h"
 #include "platform/graphics/CompositorMutatorClient.h"
 #include "public/platform/WebFrameScheduler.h"
+#include "public/web/WebRange.h"
 #include "public/web/WebWidgetClient.h"
 #include "web/CompositorMutatorImpl.h"
 #include "web/CompositorProxyClientImpl.h"
@@ -470,32 +471,28 @@ bool WebFrameWidgetImpl::setComposition(
     int selectionStart,
     int selectionEnd)
 {
-    // FIXME: To be implemented.
-    return false;
+    return view()->setComposition(text, underlines, selectionStart, selectionEnd);

[lfg, 2016/07/05 18:30:46]

Can you implement those functions here, instead of delegating to the
WebViewImpl?

[EhsanK, 2016/07/07 14:40:47]

I initially wanted to keep this CL short so I only implemented those whose
WebFrameWidget implementations were crucial. But I guess it might be better to
have them all implemented here. It is mostly copying code with minor
modifications.

[kenrb, 2016/07/07 15:30:38]

Is it really better this way? One advantage of delegating to WebViewImpl is that
there is only one copy of the method implementations. Copying and pasting
becomes a problem if people land changes to one copy and not the other.
Eventually all the code will live exclusively here, but that change isn't
imminent.

[EhsanK, 2016/07/07 20:25:07]

Some of the changes made can be argued to be not related to this CL,  in the
sense that I did not need them to make IME work.

Except for the verification of focused frame being part of the WebWidget, the
rest of the code is identical between WebViewImpl and WebFrameWidgetImpl. 

It is crucial to vrify that frame is part of WebWidget for the following
methods:

textInputType()
compositionRange()
getCompositionCharacterBounds().

We do not want to update the TextInputState for one WebWidget by others given
that the browser is tracking the state based on RenderWidgetHostViews.

Also, for other methods such as set/confirmComposition, which are in response to

IPC from browser (so we are at the correct WebWidget) you could argue that the
focus might have changed by the time we receive the IPC so we might as well
implement those in WebFrameWidgetImpl as well.

 }
 
 bool WebFrameWidgetImpl::confirmComposition()
 {
     // FIXME: To be implemented.
-    return false;
+    return view()->confirmComposition();
 }
 
 bool WebFrameWidgetImpl::confirmComposition(ConfirmCompositionBehavior selectionBehavior)
 {
-    // FIXME: To be implemented.
-    return false;
+    return view()->confirmComposition(selectionBehavior);
 }
 
 bool WebFrameWidgetImpl::confirmComposition(const WebString& text)
 {
-    // FIXME: To be implemented.
-    return false;
+    return view()->confirmComposition(text);
 }
 
 bool WebFrameWidgetImpl::compositionRange(size_t* location, size_t* length)
 {
-    // FIXME: To be implemented.
-    return false;
+    return view()->compositionRange(location, length);
 }
 
 WebTextInputInfo WebFrameWidgetImpl::textInputInfo()
@@ -664,6 +661,42 @@ void WebFrameWidgetImpl::didLosePointerLock()
     page()->pointerLockController().didLosePointerLock();
 }
 
+bool WebFrameWidgetImpl::getCompositionCharacterBounds(WebVector<WebRect>& bounds)
+{
+    size_t offset = 0;
+    size_t characterCount = 0;
+    if (!compositionRange(&offset, &characterCount))
+        return false;
+
+    if (characterCount == 0)
+        return false;
+
+    WebLocalFrame* frame = focusedWebLocalFrameInWidget();
+    if (!frame)

[lfg, 2016/07/05 18:30:46]

Why |frame| can be null? Do we call this function when this widget doesn't have
the focus?

[EhsanK, 2016/07/07 14:40:47]

Yes as the route of this call could be an update on the compositing path. Sample
stack:
WebWidget::getCompositionCharacterBounds()
RenderWidget::GetCompositionCharacterBounds()
RenderWidget::UpdateCompositionInfo()
RenderWidget::UpdateSelectionBounds()
RenderWidget::WillBeginCompositorFrame().

Also, here we look for the focused frame in widget, so there could be a focused
local frame in the process, but not belonging to the local root we are
interested in here.

[lfg, 2016/07/07 15:13:31]

So, in this example, can't you just early return in
RenderWidget::UpdateSelectionBounds if there's no focus? Why do we need to
descend all the way in the stack? You would then be able to get rid of all these
checks if the update is happening in the correct widget, and just assert that
they are.

[EhsanK, 2016/07/07 20:25:07]

We still need to send the IPC so it comes down to checking for focused frame in
this method or before that in RenderWidget.

As for "why we still need to send the IPC" I think one reason is Android:
https://cs.chromium.org/chromium/src/content/renderer/render_widget.cc?rcl=0&...
which comes from the ImeAdapterAndroid. I cannot confirm not sending the IPC in
response to the request might cause issues in android. I suggest we postpone
removing this code to when we work on IME for android. I will try to dig more
meanwhile.

[lfg, 2016/07/07 20:58:26]

Acknowledged.

+        return false;
+
+    WebVector<WebRect> result(characterCount);
+    WebRect webrect;
+    for (size_t i = 0; i < characterCount; ++i) {
+        if (!frame->firstRectForCharacterRange(offset + i, 1, webrect)) {
+            DLOG(ERROR) << "Could not retrieve character rectangle at " << i;
+            return false;
+        }
+        result[i] = webrect;
+    }
+    bounds.swap(result);
+    return true;
+}
+
+void WebFrameWidgetImpl::applyReplacementRange(int start, int length)
+{
+    if (WebLocalFrame* frame = focusedWebLocalFrameInWidget()) {
+        WebRange webrange = WebRange::fromDocumentRange(frame, start, length);
+        if (!webrange.isNull())
+            frame->selectRange(webrange);
+    }
+}
+
 void WebFrameWidgetImpl::handleMouseLeave(LocalFrame& mainFrame, const WebMouseEvent& event)
 {
     // FIXME: WebWidget doesn't have the method below.
@@ -1135,4 +1168,10 @@ HitTestResult WebFrameWidgetImpl::hitTestResultForRootFramePos(const IntPoint& p
     return result;
 }
 
+WebLocalFrame* WebFrameWidgetImpl::focusedWebLocalFrameInWidget() const
+{
+    LocalFrame* frame = page()->focusController().focusedFrame();
+    return (frame && frame->localFrameRoot() == m_localRoot->frame()) ? WebLocalFrameImpl::fromFrame(frame) : nullptr;
+}
+
 } // namespace blink