Currently, there is a lot of duplication in IME code between WebFrameWidgetImpl and WebViewImpl. Since the logic, as well as the code, are almost identical, we should remove the duplicate code by either moving parts of it to a base class (WebFrameWidgetBase) or introduce a third helper class to hold the code/logic.
cc-ing dglazkov@ for the recent issue on code duplication (See https://codereview.chromium.org/2301173005/). Also, raising the priority of this bug. I will get started on the refactor asap.
I was thinking about adding something to the public interface. I think for the most part there will be on implementation of the class which can be owned by both WebFrameWidgetImpl and WebViewImpl.
has any work gone on here? I am looking to add another member related to this duplicated code and I'd prefer it was cleaned up before adding it :-)
Can the WebWidget return some type of WebInputMethodController as dglazkov@ recommends? And we can move the methods to that. And then the two classes can instantiate that object and return it.
There is an ongoing CL here:
https://codereview.chromium.org/2333813002/
Which adds a getActiveWebInputMethodController() to WebFrameWidget. This will be the proposed WebInputMethodController per WebLocalFrame.
The CL is under review and I need either dglazkov@ or dchecng@ or some other eligible blink reviewer review it :).
The original fix landed but unfortunately I forgot to add the bug number to it. So I am including the CL description here:
Introduce WebInputMethodController to blink
Input method editing is currently handled by WebWidget
which has almost fully duplicate code in both WebFrameWidgetImpl
and WebViewImpl.
The code duplication on one hand, and the reliance of all
IME results on the frame level objects such as
InputMethodController, FrameSelection, etc. on the other
hand gave rise to proposing a refactoring to include most
of such IME code in a separate class owned by a
WebLocalFrameImpl.
The WebInputMethodController is intended to become a
wrapper class around the blink::InputMethodController (and
more) to expose the required APIs for WebKit embedders
using IME and text input. It will implement the logic in
text and IME related methods in WebWidget and possibly removes
those from WebWidget altogether. This patch, specifically,
implements the following WebWidget methods:
setComposition()
commitText()
finishComposingText()
Design document
(Google.com access): https://docs.google.com/a/google.com/document/d/1okeoZVyUZqc8z7oSIPiu-k6zKI0eLYQ9S9c4lgqpzn8/edit?usp=sharing
(Chromium.org access): https://docs.google.com/document/d/13Nidlg5yIsxQRqXgBPqmb-KcT8q85Z1ds9Z1-Vo1XQQ/edit?usp=sharing
Committed: https://crrev.com/2daaf676340283726a05cb1b387a9ff328e020e8
Cr-Commit-Position: refs/heads/master@{#431340}
Following refactors above and works such as https://chromium-review.googlesource.com/779863 for selection bounds there is no longer any OOPIF-IME code duplication in between WebViewImpl and WebFrameWidget. Closing this bug.
Comment 1 by ekaramad@chromium.org
, Sep 2 2016Labels: -Pri-3 Hotlist-Refactoring Pri-2