Introduce WebInputMethodController to blink

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

BUG=629721
Committed: https://crrev.com/2daaf676340283726a05cb1b387a9ff328e020e8
Cr-Commit-Position: refs/heads/master@{#431340}

[EhsanK, 2016-09-12 21:27:41]

Patchset #1 (id:1) has been deleted

[EhsanK, 2016-09-12 21:28:01]

Patchset #1 (id:20001) has been deleted

[EhsanK, 2016-09-12 21:31:36]

Description was changed from

==========
Introduce WebInputMethodController to blink

Input method editing is currently handled by WebWidget which has almost
duplicate code in both WebFrameWidgetImpl and WebViewImpl.

The code duplication on one hand, and the reliance of all IME results on the
InputMethodController instance of the focused LocalFrame led to the
conclusion that IME logic in WebWidget should be transferred to a new
class owned by a WebLocalFrameImpl, namely, WebInputMethodController.

The WebInputMethodController is intended to become a wrapper class
around the blink::InputMethodController which is exposing the necessary
API to WebKit embedders. The API for WebInputMethodController is expectd
to expand so that it can include most of the IME and text input related
methods inside it and hopefully remove the code duplication related to
IME from blink.

BUG=629721
==========

to

==========
Introduce WebInputMethodController to blink

Input method editing is currently handled by WebWidget which has almost
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.

Design document (Google.com access):
https://docs.google.com/a/google.com/document/d/1okeoZVyUZqc8z7oSIPiu-k6zKI0e...

BUG=629721
==========

[EhsanK, 2016-09-12 21:32:27]

Description was changed from

==========
Introduce WebInputMethodController to blink

Input method editing is currently handled by WebWidget which has almost
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.

Design document (Google.com access):
https://docs.google.com/a/google.com/document/d/1okeoZVyUZqc8z7oSIPiu-k6zKI0e...

BUG=629721
==========

to

==========
Introduce WebInputMethodController to blink

Input method editing is currently handled by WebWidget 
which has almost 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.

Design document (Google.com access):
https://docs.google.com/a/google.com/document/d/1okeoZVyUZqc8z7oSIPiu-k6zKI0e...

BUG=629721
==========

[EhsanK, 2016-09-12 21:32:40]

Description was changed from

==========
Introduce WebInputMethodController to blink

Input method editing is currently handled by WebWidget 
which has almost 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.

Design document (Google.com access):
https://docs.google.com/a/google.com/document/d/1okeoZVyUZqc8z7oSIPiu-k6zKI0e...

BUG=629721
==========

to

==========
Introduce WebInputMethodController to blink

Input method editing is currently handled by WebWidget 
which has almost 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.

Design document (Google.com access):
https://docs.google.com/a/google.com/document/d/1okeoZVyUZqc8z7oSIPiu-k6zKI0e...

BUG=629721
==========

[EhsanK, 2016-09-12 21:33:05]

Description was changed from

==========
Introduce WebInputMethodController to blink

Input method editing is currently handled by WebWidget 
which has almost 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.

Design document (Google.com access):
https://docs.google.com/a/google.com/document/d/1okeoZVyUZqc8z7oSIPiu-k6zKI0e...

BUG=629721
==========

to

==========
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.

Design document (Google.com access):
https://docs.google.com/a/google.com/document/d/1okeoZVyUZqc8z7oSIPiu-k6zKI0e...

BUG=629721
==========

[EhsanK, 2016-09-12 21:34:49]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 21:35:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-12 21:38:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-12 21:38:44]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[EhsanK, 2016-09-13 15:14:08]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 15:14:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 15:28:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 15:28:21]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[EhsanK, 2016-09-13 20:55:53]

Patchset #2 (id:60001) has been deleted

[EhsanK, 2016-09-13 20:56:00]

Patchset #1 (id:40001) has been deleted

[EhsanK, 2016-09-13 20:56:17]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 20:57:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[EhsanK, 2016-09-13 21:01:56]

Description was changed from

==========
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.

Design document (Google.com access):
https://docs.google.com/a/google.com/document/d/1okeoZVyUZqc8z7oSIPiu-k6zKI0e...

BUG=629721
==========

to

==========
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()
confirmComposition() (all 3 variations and a private implementation of it in
WebViewImpl and WebFrameWidgetImpl).

Design document (Google.com access):
https://docs.google.com/a/google.com/document/d/1okeoZVyUZqc8z7oSIPiu-k6zKI0e...

BUG=629721
==========

[EhsanK, 2016-09-13 22:21:49]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 22:22:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[EhsanK, 2016-09-13 22:22:20]

Patchset #2 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-09-13 22:52:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 22:52:07]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[EhsanK, 2016-09-14 16:00:02]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[EhsanK, 2016-09-14 16:00:11]

Patchset #2 (id:120001) has been deleted

[EhsanK, 2016-09-14 16:00:19]

Patchset #1 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-09-14 16:00:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 16:03:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 16:03:12]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[EhsanK, 2016-09-14 17:58:23]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 17:59:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[EhsanK, 2016-09-14 17:59:51]

Patchset #1 (id:140001) has been deleted

[EhsanK, 2016-09-14 18:00:21]

Description was changed from

==========
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()
confirmComposition() (all 3 variations and a private implementation of it in
WebViewImpl and WebFrameWidgetImpl).

Design document (Google.com access):
https://docs.google.com/a/google.com/document/d/1okeoZVyUZqc8z7oSIPiu-k6zKI0e...

BUG=629721
==========

to

==========
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-k6zKI0e...

BUG=629721
==========

[EhsanK, 2016-09-14 18:55:26]

ekaramad@chromium.org changed reviewers:
+ dcheng@chromium.org, dglazkov@chromium.org

[EhsanK, 2016-09-14 18:55:27]

dcheng@, dglazkov@: Could you please take a look at this change?

I tried to keep this CL very small so that we can only focus on the whole
goodness of this approach.

I would expect to move most of IME and text related WebWidget implementations to
this one, e.g., textInputInfo(), textInputType(), etc. For that reason, I am not
quite certain if WebInputMethodController will be the right class name as
clearly, the class would wrap much more than blink::InputMethodController inside
(it will deal with FrameSelection as well).

https://codereview.chromium.org/2333813002/diff/140002/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameWidget.h (right):

https://codereview.chromium.org/2333813002/diff/140002/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameWidget.h:84: virtual
WebInputMethodController* getActiveWebInputMethodController() const = 0;
With this change, I would expect all the calls to WebWidget::foo()
which are related to IME be replaced with
WebWidget::getActiveWebInputMethodController()::foo().
But I have not made this change yet since this will make the CL much bigger. I
can do that here or later when we have implemented all the IME related code
inside the WebInputMethodControllerImpl class.

[EhsanK, 2016-09-14 19:02:17]

ekaramad@chromium.org changed reviewers:
+ lfg@chromium.org

[EhsanK, 2016-09-14 19:02:17]

Adding lfg@ since this eventually contributes to WebView and WebWidget split.

lfg@: Could you please take a look?

[lfg, 2016-09-14 19:20:26]

Some initial comments.

Also, can you move your design doc to a chromium.org account and make it public?

https://codereview.chromium.org/2333813002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.h (right):

https://codereview.chromium.org/2333813002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.h:332:
WebInputMethodControllerImpl* inputMethodController();
const

https://codereview.chromium.org/2333813002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/2333813002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.h:525: WebInputMethodControllerImpl*
getActiveWebInputMethodController() const;
Why didn't you implement this in WebFrameWidgetBase? This doesn't need to be
duplicated between WebViewImpl and WebFrameWidgetImpl.

It also means that setComposition, commitText and finishCompositingText also
don't have to be duplicated.

https://codereview.chromium.org/2333813002/diff/170001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebInputMethodController.h (right):

https://codereview.chromium.org/2333813002/diff/170001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebInputMethodController.h:55: int selectionEnd) {
return false; }
Can you make those functions pure virtual?

[EhsanK, 2016-09-14 19:41:06]

Description was changed from

==========
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-k6zKI0e...

BUG=629721
==========

to

==========
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-k6zKI0e...

(Chromium.org access):
https://docs.google.com/a/chromium.org/document/d/13Nidlg5yIsxQRqXgBPqmb-KcT8...
BUG=629721
==========

[EhsanK, 2016-09-14 19:41:26]

Description was changed from

==========
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-k6zKI0e...

(Chromium.org access):
https://docs.google.com/a/chromium.org/document/d/13Nidlg5yIsxQRqXgBPqmb-KcT8...
BUG=629721
==========

to

==========
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-k6zKI0e...

(Chromium.org access):
https://docs.google.com/a/chromium.org/document/d/13Nidlg5yIsxQRqXgBPqmb-KcT8...
BUG=629721
==========

[EhsanK, 2016-09-16 15:06:29]

Thanks Lucas! I will submit another patch which takes care of removing those IME
methods from WebWidget. PTAL.

https://codereview.chromium.org/2333813002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.h (right):

https://codereview.chromium.org/2333813002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.h:332:
WebInputMethodControllerImpl* inputMethodController();
On 2016/09/14 19:20:26, lfg wrote:
> const

Done.

https://codereview.chromium.org/2333813002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/2333813002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.h:525: WebInputMethodControllerImpl*
getActiveWebInputMethodController() const;
On 2016/09/14 19:20:26, lfg wrote:
> Why didn't you implement this in WebFrameWidgetBase? This doesn't need to be
> duplicated between WebViewImpl and WebFrameWidgetImpl.
> 
This returns WebInputMethodController which is public and should be accessible
publicly so that RenderWidget or other users of blink can call IME methods on
the focused frame.
> It also means that setComposition, commitText and finishCompositingText also
> don't have to be duplicated.
Those actually should be removed from Web*Widget. and WebView. They now live in
this new class. I did not remove them to avoid making the CL at this stage big.
Following our offline conversation I will add that change in the upcoming patch.

https://codereview.chromium.org/2333813002/diff/170001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebInputMethodController.h (right):

https://codereview.chromium.org/2333813002/diff/170001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebInputMethodController.h:55: int selectionEnd) {
return false; }
On 2016/09/14 19:20:26, lfg wrote:
> Can you make those functions pure virtual?

Acknowledged.

[lfg, 2016-09-16 15:11:30]

https://codereview.chromium.org/2333813002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/2333813002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.h:525: WebInputMethodControllerImpl*
getActiveWebInputMethodController() const;
On 2016/09/16 15:06:28, EhsanK wrote:
> On 2016/09/14 19:20:26, lfg wrote:
> > Why didn't you implement this in WebFrameWidgetBase? This doesn't need to be
> > duplicated between WebViewImpl and WebFrameWidgetImpl.
> > 
> This returns WebInputMethodController which is public and should be accessible
> publicly so that RenderWidget or other users of blink can call IME methods on
> the focused frame.

WebFrameWidgetBase inherits from WebWidget, just override the methods there.

> > It also means that setComposition, commitText and finishCompositingText also
> > don't have to be duplicated.
> Those actually should be removed from Web*Widget. and WebView. They now live
in
> this new class. I did not remove them to avoid making the CL at this stage
big.
> Following our offline conversation I will add that change in the upcoming
patch.

That's right, let's do this now. Override the virtuals in WebFrameWidgetBase and
implement them there. Then remove the implementations from WebViewImpl and
WebFrameWidgetImpl.

[EhsanK, 2016-09-16 18:25:46]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 18:26:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[EhsanK, 2016-09-16 19:01:18]

Thanks Lucas! PTAL.

[lfg, 2016-09-16 20:17:10]

Thanks! Looking much better with all the deleted code duplication.

Another round of comments.

https://codereview.chromium.org/2333813002/diff/230001/components/test_runner...
File components/test_runner/web_widget_test_client.cc (right):

https://codereview.chromium.org/2333813002/diff/230001/components/test_runner...
components/test_runner/web_widget_test_client.cc:110: blink::WebFrameWidget*
frameWidget = nullptr;
frame_widget

https://codereview.chromium.org/2333813002/diff/230001/components/test_runner...
components/test_runner/web_widget_test_client.cc:116: blink::WebLocalFrame*
mainFrame =
main_frame

https://codereview.chromium.org/2333813002/diff/230001/components/test_runner...
components/test_runner/web_widget_test_client.cc:124:
blink::WebWidget::KeepSelection);
Why this method is implemented using KeepSelection, but the implementation in
RenderWidget uses DoNotKeepSelection?

https://codereview.chromium.org/2333813002/diff/230001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2333813002/diff/230001/content/renderer/rende...
content/renderer/render_widget.cc:1405: if (!GetInputMethodController() ||
Why do we need these guards? When do we call this function and
GetInputMethodController() is null?

https://codereview.chromium.org/2333813002/diff/230001/content/renderer/rende...
content/renderer/render_widget.cc:2168: return
GetWebWidget()->isWebFrameWidget()
Should these be a DCHECK instead of returning null?

https://codereview.chromium.org/2333813002/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/2333813002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.h:518: WebInputMethodControllerImpl*
getActiveWebInputMethodController() const;
Can you make this private? We should only be getting this through the
WebFrameWidget.

https://codereview.chromium.org/2333813002/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/2333813002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:1678: bool
contentDetectionRequested() const { return m_contentDetectionRequested; }
Was this an intended change?

https://codereview.chromium.org/2333813002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:1725: return
m_webViewHelper.webView()->getActiveWebInputMethodController();
Can you get this through the WebViewFrameWidget?

[esprehn, 2016-09-16 21:21:28]

FYI, please use the short copyright on all new files that old one is huge. :)

[Changwan Ryu, 2016-09-19 02:08:35]

changwan@chromium.org changed reviewers:
+ changwan@chromium.org

[Changwan Ryu, 2016-09-19 02:08:36]

https://codereview.chromium.org/2333813002/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (left):

https://codereview.chromium.org/2333813002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1252: void
WebLocalFrameImpl::extendSelectionAndDelete(int before, int after)
Hmm... Sorry for my ignorance, but how are we plumbing extendSelectionAndDelete
/ setEditableSelectionOffsets / setCompositionFromExistingText for OOPIF now? I
was thinking of merging WebLocalFrameImpl's and WebViewImpl's IMC access at some
point (https://codereview.chromium.org/2323983003/) because they are similar
operations that just ended up in split places.

If so, now I think either all of IMC operations can actually belong to
WebLocalFrameImpl without WebFrameWidget in the first place or we're doing
something incorrectly for extendSelectionAndDelete and other operations (at
least for Android).

Could you check those possibilities? In any case, if we go ahead with this
refactoring I think you need to consider moving these to
WebInputMethodController as well.

[EhsanK, 2016-09-20 15:38:28]

ekaramad@chromium.org changed reviewers:
+ aelias@chromium.org

[EhsanK, 2016-09-20 15:38:30]

Thanks for the reviews! PTAL

I am also adding aelias@ as a reviewer to kindly comment on the issue raised by
lfg@ on WebWidgetTextclient::resetInputMethod() regarding KeepSelection vs
DoNotKeepSelection.

aelias@: Could you please take a look? Any speicifc reason why we should
KeepSelection here?

https://codereview.chromium.org/2333813002/diff/230001/components/test_runner...
File components/test_runner/web_widget_test_client.cc (right):

https://codereview.chromium.org/2333813002/diff/230001/components/test_runner...
components/test_runner/web_widget_test_client.cc:110: blink::WebFrameWidget*
frameWidget = nullptr;
On 2016/09/16 20:17:09, lfg wrote:
> frame_widget

Acknowledged.

https://codereview.chromium.org/2333813002/diff/230001/components/test_runner...
components/test_runner/web_widget_test_client.cc:116: blink::WebLocalFrame*
mainFrame =
On 2016/09/16 20:17:09, lfg wrote:
> main_frame

Acknowledged.

https://codereview.chromium.org/2333813002/diff/230001/components/test_runner...
components/test_runner/web_widget_test_client.cc:124:
blink::WebWidget::KeepSelection);
On 2016/09/16 20:17:09, lfg wrote:
> Why this method is implemented using KeepSelection, but the implementation in
> RenderWidget uses DoNotKeepSelection?

The one with DoNotKeepSelection sounds more correct to me (for RenderWidget).
But I am not sure why it is KeepSelection here. Adding aelias@ as a reviewer
since he made this change.

https://codereview.chromium.org/2333813002/diff/230001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2333813002/diff/230001/content/renderer/rende...
content/renderer/render_widget.cc:1405: if (!GetInputMethodController() ||
On 2016/09/16 20:17:09, lfg wrote:
> Why do we need these guards? When do we call this function and
> GetInputMethodController() is null?

It will return nullptr if the focused frame is NOT inside the widget. It can
happen if the focus suddenly goes to another WebWidget's frame. I could repro
this locally.

https://codereview.chromium.org/2333813002/diff/230001/content/renderer/rende...
content/renderer/render_widget.cc:2168: return
GetWebWidget()->isWebFrameWidget()
On 2016/09/16 20:17:09, lfg wrote:
> Should these be a DCHECK instead of returning null?

I think for now that we only use WebInputMethodController for the three IME
methods, we probably never get here when RenderViewImpl::frame_widget_ ==
nullptr. But I am not quite sure if that will stay the same if we start moving
other text input methods to WebInputMethodController, i.e., textInputInfo(),
etc.

For now, I can change this into a DCHECK and see if it fires at all.

https://codereview.chromium.org/2333813002/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (left):

https://codereview.chromium.org/2333813002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1252: void
WebLocalFrameImpl::extendSelectionAndDelete(int before, int after)
On 2016/09/19 02:08:36, Changwan Ryu wrote:
> Hmm... Sorry for my ignorance, but how are we plumbing
extendSelectionAndDelete
> / setEditableSelectionOffsets / setCompositionFromExistingText for OOPIF now? 

Very good point! I had not considered those methods at all. :P
extendSelectionAndDelete goes from RenderWidgetHostView to RenderFrame which is
a bit weird:
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...
RWHVAura is the TextInputClient. But it should not be exposed to RFH AFAIK.

Anyway, RenderFrameImpl calls it on |frame_| if RenderWidget agrees with
handling IME.
https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?rc...

I think what would be nicer is to have is get RWHVAura talk to RenderWidget and
RenderWidget to WebInputMethodController and so on; the same way the other IME
results are being routed: setComposition, finsihComposingText, commitText. WDYT?

SetEditableSelectionOffsets and SetCompositionFromExistingText are similar
except that they Android only now and initiated from ImeAdapterAndroid.



>I  was thinking of merging WebLocalFrameImpl's and WebViewImpl's IMC access at
some
> point (https://codereview.chromium.org/2323983003/) because they are similar
> operations that just ended up in split places.

I think if we end up using thie WebInputMethodController (which is owned by
frame), the IME code from WebViewImpl (WebFrameWidgetImpl) and WebLocalFrameImpl
to some extent should be removed. WDYT?

> If so, now I think either all of IMC operations can actually belong to
> WebLocalFrameImpl without WebFrameWidget in the first place or we're doing
> something incorrectly for extendSelectionAndDelete and other operations (at
> least for Android).
> 
The way it is implemented now, IME is mostly routed through RenderWidgetHost
from tab's RWHV (along with keyboard input). I think for now, the least costly
method could be to route the other (3) IME calls through RenderWidget as well.
From RenderWidget, we could use the WebInputMethodController for focused frame
again. WDYT?

> Could you check those possibilities? In any case, if we go ahead with this
> refactoring I think you need to consider moving these to
> WebInputMethodController as well.
Agreed. I intend to include more WebViewImpl/WebFrameWidgetImpl code in this
class. I was thinking textInputInfo(), selectionBounds(), etc should also be in
the same class since they serve Text* anyway. Not sure if this is the right
class for that too, but I think it is a good place for it to be.


For now, I updated my doc to include the methods you mentioned above in the
list:
https://docs.google.com/document/d/13Nidlg5yIsxQRqXgBPqmb-KcT8q85Z1ds9Z1-Vo1X...

https://codereview.chromium.org/2333813002/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/2333813002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.h:518: WebInputMethodControllerImpl*
getActiveWebInputMethodController() const;
On 2016/09/16 20:17:09, lfg wrote:
> Can you make this private? We should only be getting this through the
> WebFrameWidget.

Acknowledged.

https://codereview.chromium.org/2333813002/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/2333813002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:1678: bool
contentDetectionRequested() const { return m_contentDetectionRequested; }
On 2016/09/16 20:17:09, lfg wrote:
> Was this an intended change?

No. Thanks for noticing it. I don't quite understand how the empty line got
removed.

https://codereview.chromium.org/2333813002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:1725: return
m_webViewHelper.webView()->getActiveWebInputMethodController();
On 2016/09/16 20:17:09, lfg wrote:
> Can you get this through the WebViewFrameWidget?

Acknowledged.

[aelias_OOO_until_Jul13, 2016-09-21 08:28:03]

https://codereview.chromium.org/2333813002/diff/230001/components/test_runner...
File components/test_runner/web_widget_test_client.cc (right):

https://codereview.chromium.org/2333813002/diff/230001/components/test_runner...
components/test_runner/web_widget_test_client.cc:124:
blink::WebWidget::KeepSelection);
On 2016/09/20 at 15:38:29, EhsanK wrote:
> On 2016/09/16 20:17:09, lfg wrote:
> > Why this method is implemented using KeepSelection, but the implementation
in
> > RenderWidget uses DoNotKeepSelection?
> 
> The one with DoNotKeepSelection sounds more correct to me (for RenderWidget).
But I am not sure why it is KeepSelection here. Adding aelias@ as a reviewer
since he made this change.

This was a mistake, you can change it to DoNotKeepSelectiom.  KeepSelection is
just for use by Android and Android-related tests.

[Changwan Ryu, 2016-09-21 08:42:04]

https://codereview.chromium.org/2333813002/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (left):

https://codereview.chromium.org/2333813002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1252: void
WebLocalFrameImpl::extendSelectionAndDelete(int before, int after)
Hmm... Thanks for adding them. (I noticed that only public version has it for
now. :) ) But does it mean OOPIF input is broken on Android? Or is Android's
OOPIF not implemented yet? If there are any test cases to verify OOPIF case for
Android, could you point me to those?

[EhsanK, 2016-09-21 16:49:10]

Thanks! PTAL.

https://codereview.chromium.org/2333813002/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (left):

https://codereview.chromium.org/2333813002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1252: void
WebLocalFrameImpl::extendSelectionAndDelete(int before, int after)
On 2016/09/21 08:42:03, Changwan Ryu wrote:
> Hmm... Thanks for adding them. (I noticed that only public version has it for
> now. :) ) But does it mean OOPIF input is broken on Android? Or is Android's
> OOPIF not implemented yet? If there are any test cases to verify OOPIF case
for
> Android, could you point me to those?

Sorry for the public vs private versions of the doc. I should have only had the
public one. I left a comment in the private one that I will be updating the
public, but I guess I will go back and update the private one too.

OOPIF-Android is not complete yet, and neither is IME for it. I am working on
changing text input state tracking for android currently, and plan to implement
the same OOPIF-IME logic for Android soon. I think the methods mentioned above
could be refactored at that time.

[lfg, 2016-09-21 16:56:14]

https://codereview.chromium.org/2333813002/diff/230001/components/test_runner...
File components/test_runner/web_widget_test_client.cc (right):

https://codereview.chromium.org/2333813002/diff/230001/components/test_runner...
components/test_runner/web_widget_test_client.cc:124:
blink::WebWidget::KeepSelection);
On 2016/09/21 08:28:02, aelias wrote:
> On 2016/09/20 at 15:38:29, EhsanK wrote:
> > On 2016/09/16 20:17:09, lfg wrote:
> > > Why this method is implemented using KeepSelection, but the implementation
> in
> > > RenderWidget uses DoNotKeepSelection?
> > 
> > The one with DoNotKeepSelection sounds more correct to me (for
RenderWidget).
> But I am not sure why it is KeepSelection here. Adding aelias@ as a reviewer
> since he made this change.
> 
> This was a mistake, you can change it to DoNotKeepSelectiom.  KeepSelection is
> just for use by Android and Android-related tests.

I think we should be able to just remove this override, and remove the overrides
in WebWidgetTestProxy and WebViewTestProxy, which should make the RenderWidget
handle the calls.

https://codereview.chromium.org/2333813002/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/2333813002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:1723:
WebInputMethodController* WebViewTest::inputMethodController() const
Can you make this function take a frame as an argument? Otherwise it seems like
there's always only a single inputMethodController, which is not true in
general.

[Changwan Ryu, 2016-09-21 23:33:16]

https://codereview.chromium.org/2333813002/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (left):

https://codereview.chromium.org/2333813002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1252: void
WebLocalFrameImpl::extendSelectionAndDelete(int before, int after)
On 2016/09/21 16:49:10, EhsanK wrote:
> On 2016/09/21 08:42:03, Changwan Ryu wrote:
> > Hmm... Thanks for adding them. (I noticed that only public version has it
for
> > now. :) ) But does it mean OOPIF input is broken on Android? Or is Android's
> > OOPIF not implemented yet? If there are any test cases to verify OOPIF case
> for
> > Android, could you point me to those?
> 
> Sorry for the public vs private versions of the doc. I should have only had
the
> public one. I left a comment in the private one that I will be updating the
> public, but I guess I will go back and update the private one too.
> 
> OOPIF-Android is not complete yet, and neither is IME for it. I am working on
> changing text input state tracking for android currently, and plan to
implement
> the same OOPIF-IME logic for Android soon. I think the methods mentioned above
> could be refactored at that time.

I see. Thanks for the clarification!

[EhsanK, 2016-09-26 20:53:31]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 20:54:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[EhsanK, 2016-09-26 20:54:29]

Please, take another look.

https://codereview.chromium.org/2333813002/diff/230001/components/test_runner...
File components/test_runner/web_widget_test_client.cc (right):

https://codereview.chromium.org/2333813002/diff/230001/components/test_runner...
components/test_runner/web_widget_test_client.cc:124:
blink::WebWidget::KeepSelection);
On 2016/09/21 16:56:14, lfg wrote:
> On 2016/09/21 08:28:02, aelias wrote:
> > On 2016/09/20 at 15:38:29, EhsanK wrote:
> > > On 2016/09/16 20:17:09, lfg wrote:
> > > > Why this method is implemented using KeepSelection, but the
implementation
> > in
> > > > RenderWidget uses DoNotKeepSelection?
> > > 
> > > The one with DoNotKeepSelection sounds more correct to me (for
> RenderWidget).
> > But I am not sure why it is KeepSelection here. Adding aelias@ as a reviewer
> > since he made this change.
> > 
> > This was a mistake, you can change it to DoNotKeepSelectiom.  KeepSelection
is
> > just for use by Android and Android-related tests.
> 
> I think we should be able to just remove this override, and remove the
overrides
> in WebWidgetTestProxy and WebViewTestProxy, which should make the RenderWidget
> handle the calls.

The difference would be that here we do not send an IPC back to the browser. In
RenderWidget we do. Removing these methods also causes the following test to
fail all the time:
fast/events/composition-event-source-device-event-sender.html
I ran the test locally on Linux. I think it might need more work.

https://codereview.chromium.org/2333813002/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/2333813002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:1723:
WebInputMethodController* WebViewTest::inputMethodController() const
On 2016/09/21 16:56:14, lfg wrote:
> Can you make this function take a frame as an argument? Otherwise it seems
like
> there's always only a single inputMethodController, which is not true in
> general.

This is supposed to return the controller for the focused frame inside the
widget. In that case I would have to get the focused frame and check the local
root each time, which is the currently implemented code for WebViewFrameWidget.
How about renaming this to something more clear and leaving comments?

[commit-bot: I haz the power, 2016-09-26 22:25:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 22:25:15]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[lfg, 2016-09-29 14:16:41]

https://codereview.chromium.org/2333813002/diff/230001/components/test_runner...
File components/test_runner/web_widget_test_client.cc (right):

https://codereview.chromium.org/2333813002/diff/230001/components/test_runner...
components/test_runner/web_widget_test_client.cc:124:
blink::WebWidget::KeepSelection);
On 2016/09/26 20:54:29, EhsanK wrote:
> On 2016/09/21 16:56:14, lfg wrote:
> > On 2016/09/21 08:28:02, aelias wrote:
> > > On 2016/09/20 at 15:38:29, EhsanK wrote:
> > > > On 2016/09/16 20:17:09, lfg wrote:
> > > > > Why this method is implemented using KeepSelection, but the
> implementation
> > > in
> > > > > RenderWidget uses DoNotKeepSelection?
> > > > 
> > > > The one with DoNotKeepSelection sounds more correct to me (for
> > RenderWidget).
> > > But I am not sure why it is KeepSelection here. Adding aelias@ as a
reviewer
> > > since he made this change.
> > > 
> > > This was a mistake, you can change it to DoNotKeepSelectiom. 
KeepSelection
> is
> > > just for use by Android and Android-related tests.
> > 
> > I think we should be able to just remove this override, and remove the
> overrides
> > in WebWidgetTestProxy and WebViewTestProxy, which should make the
RenderWidget
> > handle the calls.
> 
> The difference would be that here we do not send an IPC back to the browser.
In
> RenderWidget we do. Removing these methods also causes the following test to
> fail all the time:
> fast/events/composition-event-source-device-event-sender.html
> I ran the test locally on Linux. I think it might need more work.

I think its worth investigating these failures and understanding them. All other
IPC methods go through RenderWidget and send IPCs to the browser process, I
don't see why this should be different.

https://codereview.chromium.org/2333813002/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/2333813002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:1723:
WebInputMethodController* WebViewTest::inputMethodController() const
On 2016/09/26 20:54:29, EhsanK wrote:
> On 2016/09/21 16:56:14, lfg wrote:
> > Can you make this function take a frame as an argument? Otherwise it seems
> like
> > there's always only a single inputMethodController, which is not true in
> > general.
> 
> This is supposed to return the controller for the focused frame inside the
> widget. In that case I would have to get the focused frame and check the local
> root each time, which is the currently implemented code for
WebViewFrameWidget.
> How about renaming this to something more clear and leaving comments and then
call it passing the main frame?

I don't think I understand this.

Why can't you just return
WebLocalFrameImpl::fromFrame(frame)->frameWidget()->getActiveWebInputMethodController()?

[dglazkov, 2016-10-06 22:18:59]

lgtm % nits

https://codereview.chromium.org/2333813002/diff/390001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2333813002/diff/390001/content/renderer/rende...
content/renderer/render_widget.cc:1420: if (!GetInputMethodController() ||
this can be inverted and done the same way as I suggested below.

https://codereview.chromium.org/2333813002/diff/390001/content/renderer/rende...
content/renderer/render_widget.cc:1452:
GetInputMethodController()->commitText(text, relative_cursor_pos);
if (auto* controller = GetInputMethodController())
   controller->commitText(text, ...

https://codereview.chromium.org/2333813002/diff/390001/content/renderer/rende...
content/renderer/render_widget.cc:1470: if (GetInputMethodController()) {
same here.

https://codereview.chromium.org/2333813002/diff/390001/content/renderer/rende...
content/renderer/render_widget.cc:2017: if (GetInputMethodController() &&
same here.

[EhsanK, 2016-10-31 23:42:12]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 23:42:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-31 23:57:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-31 23:57:55]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[EhsanK, 2016-11-01 14:45:52]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 14:46:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[EhsanK, 2016-11-01 15:46:03]

ekaramad@chromium.org changed reviewers:
+ dtapuska@chromium.org, lanwei@chromium.org

[EhsanK, 2016-11-01 15:46:05]

Thanks lfg@ and dglazkov@ for the reviews!

Adding dtapuska@ since he is also working on cleaning up IME code inside
WebViewImpl/WebFrameWidgetImpl.

Adding lanwei@ for the modified test.


PTAL.

https://codereview.chromium.org/2333813002/diff/230001/components/test_runner...
File components/test_runner/web_widget_test_client.cc (right):

https://codereview.chromium.org/2333813002/diff/230001/components/test_runner...
components/test_runner/web_widget_test_client.cc:124:
blink::WebWidget::KeepSelection);
On 2016/09/29 14:16:41, lfg wrote:
> On 2016/09/26 20:54:29, EhsanK wrote:
> > On 2016/09/21 16:56:14, lfg wrote:
> > > On 2016/09/21 08:28:02, aelias wrote:
> > > > On 2016/09/20 at 15:38:29, EhsanK wrote:
> > > > > On 2016/09/16 20:17:09, lfg wrote:
> > > > > > Why this method is implemented using KeepSelection, but the
> > implementation
> > > > in
> > > > > > RenderWidget uses DoNotKeepSelection?
> > > > > 
> > > > > The one with DoNotKeepSelection sounds more correct to me (for
> > > RenderWidget).
> > > > But I am not sure why it is KeepSelection here. Adding aelias@ as a
> reviewer
> > > > since he made this change.
> > > > 
> > > > This was a mistake, you can change it to DoNotKeepSelectiom. 
> KeepSelection
> > is
> > > > just for use by Android and Android-related tests.
> > > 
> > > I think we should be able to just remove this override, and remove the
> > overrides
> > > in WebWidgetTestProxy and WebViewTestProxy, which should make the
> RenderWidget
> > > handle the calls.
> > 
> > The difference would be that here we do not send an IPC back to the browser.
> In
> > RenderWidget we do. Removing these methods also causes the following test to
> > fail all the time:
> > fast/events/composition-event-source-device-event-sender.html
> > I ran the test locally on Linux. I think it might need more work.
> 
> I think its worth investigating these failures and understanding them. All
other
> IPC methods go through RenderWidget and send IPCs to the browser process, I
> don't see why this should be different.

I dug deeper and here is what I found:
In the test mentioned, we focus an <input> and setMarkText using
TextInputController API in components/test_runner/ and then blur the input. When
blurring, specifically, we resetInputMethod which will cause some events firing
for composition.

The problem was, javascript generated input.focus() is not user gesture so it
the Element::focus() path will not call IME methods to set text input state.

The solution was to either, manually call ResetInputMethod through JS binding,
or, mark the next event as gesture event. I did the latter but the former is
also possible. I though this way at least we are describing a valid scenario
which would work on Chrome. By forcefully calling a reset method we might be
mocking too much.

Adding lanwei@ for her opinion on the test changes.

https://codereview.chromium.org/2333813002/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/2333813002/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:1723:
WebInputMethodController* WebViewTest::inputMethodController() const
On 2016/09/29 14:16:41, lfg wrote:
> On 2016/09/26 20:54:29, EhsanK wrote:
> > On 2016/09/21 16:56:14, lfg wrote:
> > > Can you make this function take a frame as an argument? Otherwise it seems
> > like
> > > there's always only a single inputMethodController, which is not true in
> > > general.
> > 
> > This is supposed to return the controller for the focused frame inside the
> > widget. In that case I would have to get the focused frame and check the
local
> > root each time, which is the currently implemented code for
> WebViewFrameWidget.
> > How about renaming this to something more clear and leaving comments and
then
> call it passing the main frame?
> 
> I don't think I understand this.
> 
> Why can't you just return
>
WebLocalFrameImpl::fromFrame(frame)->frameWidget()->getActiveWebInputMethodController()?

Removed the method, and now, similar to your suggestion, I am using the
frameWidget() of the corresponding frame to get WebInputMethodController().

https://codereview.chromium.org/2333813002/diff/390001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2333813002/diff/390001/content/renderer/rende...
content/renderer/render_widget.cc:1420: if (!GetInputMethodController() ||
On 2016/10/06 22:18:58, dglazkov wrote:
> this can be inverted and done the same way as I suggested below.

Acknowledged.

https://codereview.chromium.org/2333813002/diff/390001/content/renderer/rende...
content/renderer/render_widget.cc:1452:
GetInputMethodController()->commitText(text, relative_cursor_pos);
On 2016/10/06 22:18:58, dglazkov wrote:
> if (auto* controller = GetInputMethodController())
>    controller->commitText(text, ...

Acknowledged.

https://codereview.chromium.org/2333813002/diff/390001/content/renderer/rende...
content/renderer/render_widget.cc:1470: if (GetInputMethodController()) {
On 2016/10/06 22:18:58, dglazkov wrote:
> same here.

Done.

https://codereview.chromium.org/2333813002/diff/390001/content/renderer/rende...
content/renderer/render_widget.cc:2017: if (GetInputMethodController() &&
On 2016/10/06 22:18:58, dglazkov wrote:
> same here.

Acknowledged.

[lfg, 2016-11-01 15:51:06]

Thanks for digging into it! This looks like a much cleaner solution.

LGTM!

[lanwei, 2016-11-01 15:58:54]

third_party/WebKit/LayoutTests/fast/events/composition-event-source-device-event-sender.html
LGTM!

[commit-bot: I haz the power, 2016-11-01 17:06:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-01 17:06:54]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[EhsanK, 2016-11-02 16:43:33]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 16:44:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 18:44:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-02 18:44:37]

Dry run: This issue passed the CQ dry run.

[EhsanK, 2016-11-02 19:34:00]

Thanks for reviews!

I had to modify the test yet again since on Mac even a user gesture which ends
up in focusing <input> does not lead to an immediate update of text input state.
The new version specifically asks for the update and I have left some comments
in the test explaining the reason.

Please take another look!

[lfg, 2016-11-02 19:36:26]

still lgtm

[EhsanK, 2016-11-02 19:40:33]

ekaramad@chromium.org changed reviewers:
+ creis@chromium.org, tommyw@chromium.org

[EhsanK, 2016-11-02 19:40:34]

Adding tommyw@ for changes in components/test_runner/

Adding creis@ for changes in content/

Please, take a look.

[Charlie Reis, 2016-11-02 20:57:38]

content/ generally looks good, but there's one security bug we should fix (bad
cast).

https://codereview.chromium.org/2333813002/diff/490001/content/public/test/la...
File content/public/test/layouttest_support.h (right):

https://codereview.chromium.org/2333813002/diff/490001/content/public/test/la...
content/public/test/layouttest_support.h:182: // Causes the RenderWidget
corresponding to |render_frame| update its
nit: to update

https://codereview.chromium.org/2333813002/diff/490001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2333813002/diff/490001/content/renderer/rende...
content/renderer/render_widget.cc:1401: auto* controller =
GetInputMethodController();
nit: Let's avoid auto here.  It's helpful to know what type this below, and
isn't saving much.  (I guess I'm ok with it in the cases below, where the
declaration is within a conditional.)

https://codereview.chromium.org/2333813002/diff/490001/content/renderer/rende...
content/renderer/render_widget.cc:2112:
DCHECK(GetWebWidget()->isWebFrameWidget());
This can't be a DCHECK, since we'll get a potentially exploitable bad cast in
release builds if it's wrong.  Make it a CHECK (preferable if we don't think it
can fail) or return early if it fails.

[EhsanK, 2016-11-02 22:16:34]

Thanks Charlie! PTAL.

Also leaving one question from lfg@ regarding the DCHECK/CHECK comment on
RenderWidget::GetInputMethodController().

https://codereview.chromium.org/2333813002/diff/490001/content/public/test/la...
File content/public/test/layouttest_support.h (right):

https://codereview.chromium.org/2333813002/diff/490001/content/public/test/la...
content/public/test/layouttest_support.h:182: // Causes the RenderWidget
corresponding to |render_frame| update its
On 2016/11/02 20:57:38, Charlie Reis wrote:
> nit: to update

Done.

https://codereview.chromium.org/2333813002/diff/490001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2333813002/diff/490001/content/renderer/rende...
content/renderer/render_widget.cc:1401: auto* controller =
GetInputMethodController();
On 2016/11/02 20:57:38, Charlie Reis wrote:
> nit: Let's avoid auto here.  It's helpful to know what type this below, and
> isn't saving much.  (I guess I'm ok with it in the cases below, where the
> declaration is within a conditional.)

Acknowledged.

https://codereview.chromium.org/2333813002/diff/490001/content/renderer/rende...
content/renderer/render_widget.cc:2112:
DCHECK(GetWebWidget()->isWebFrameWidget());
On 2016/11/02 20:57:38, Charlie Reis wrote:
> This can't be a DCHECK, since we'll get a potentially exploitable bad cast in
> release builds if it's wrong.  Make it a CHECK (preferable if we don't think
it
> can fail) or return early if it fails.

I see. Thanks! I don't think we can have a WebWidget which is WebFrameWidget.
But I think lfg@ was mentioning a case this could have happened?

For now I made it a CHECK. lfg@ WDYT?

[EhsanK, 2016-11-02 22:16:38]

Thanks Charlie! PTAL.

Also leaving one question from lfg@ regarding the DCHECK/CHECK comment on
RenderWidget::GetInputMethodController().

[lfg, 2016-11-02 22:20:21]

On 2016/11/02 22:16:34, EhsanK wrote:
> On 2016/11/02 20:57:38, Charlie Reis wrote:
> > This can't be a DCHECK, since we'll get a potentially exploitable bad cast
in
> > release builds if it's wrong.  Make it a CHECK (preferable if we don't think
> it
> > can fail) or return early if it fails.
> 
> I see. Thanks! I don't think we can have a WebWidget which is WebFrameWidget.
> But I think lfg@ was mentioning a case this could have happened?
> 
> For now I made it a CHECK. lfg@ WDYT?

This would only fail if this function was called on a swapped out RenderView.
CHECK is fine, it shouldn't happen.

[Charlie Reis, 2016-11-02 22:25:22]

Thanks!  content/ LGTM.

[dtapuska, 2016-11-03 15:28:32]

On 2016/11/02 22:25:22, Charlie Reis wrote:
> Thanks!  content/ LGTM.

lgtm

[EhsanK, 2016-11-08 23:06:28]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 23:07:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 00:54:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-09 00:54:34]

Dry run: This issue passed the CQ dry run.

[EhsanK, 2016-11-09 15:46:58]

ekaramad@chromium.org changed reviewers:
+ rbyers@chromium.org

[EhsanK, 2016-11-09 15:46:59]

Adding rbyres@ for general reviews and components/test_runner/* permissions
since tommyw@ is OOO.

rbyres@, could you please review this CL?

[Rick Byers, 2016-11-10 17:16:00]

On 2016/11/09 15:46:59, EhsanK wrote:
> Adding rbyres@ for general reviews and components/test_runner/* permissions
> since tommyw@ is OOO.
> 
> rbyres@, could you please review this CL?

I'm OOO at CDS so haven't reviewed the full CL.  But test_runner LGTM

[EhsanK, 2016-11-10 17:21:01]

Description was changed from

==========
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-k6zKI0e...

(Chromium.org access):
https://docs.google.com/a/chromium.org/document/d/13Nidlg5yIsxQRqXgBPqmb-KcT8...
BUG=629721
==========

to

==========
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-k6zKI0e...

(Chromium.org access):
https://docs.google.com/document/d/13Nidlg5yIsxQRqXgBPqmb-KcT8q85Z1ds9Z1-Vo1X...
==========

[EhsanK, 2016-11-10 17:21:59]

Thank you all for the reviews.

cc-ed kojii@ and haraken@ since this is both blink arch. and IME related.

FYI, I will rebase and try CQ-ing.

[EhsanK, 2016-11-10 17:26:29]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 17:26:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[EhsanK, 2016-11-10 19:05:18]

The CQ bit was unchecked by ekaramad@chromium.org

[EhsanK, 2016-11-10 19:05:19]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2016-11-10 19:05:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
dglazkov@chromium.org, lanwei@chromium.org, lfg@chromium.org,
creis@chromium.org, dtapuska@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/2333813002/#ps570001
(title: "Rebased")

[commit-bot: I haz the power, 2016-11-10 19:05:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 20:29:43]

Description was changed from

==========
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-k6zKI0e...

(Chromium.org access):
https://docs.google.com/document/d/13Nidlg5yIsxQRqXgBPqmb-KcT8q85Z1ds9Z1-Vo1X...
==========

to

==========
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-k6zKI0e...

(Chromium.org access):
https://docs.google.com/document/d/13Nidlg5yIsxQRqXgBPqmb-KcT8q85Z1ds9Z1-Vo1X...
==========

[commit-bot: I haz the power, 2016-11-10 20:29:45]

Committed patchset #22 (id:570001)

[commit-bot: I haz the power, 2016-11-10 20:44:42]

Description was changed from

==========
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-k6zKI0e...

(Chromium.org access):
https://docs.google.com/document/d/13Nidlg5yIsxQRqXgBPqmb-KcT8q85Z1ds9Z1-Vo1X...
==========

to

==========
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-k6zKI0e...

(Chromium.org access):
https://docs.google.com/document/d/13Nidlg5yIsxQRqXgBPqmb-KcT8q85Z1ds9Z1-Vo1X...

Committed: https://crrev.com/2daaf676340283726a05cb1b387a9ff328e020e8
Cr-Commit-Position: refs/heads/master@{#431340}
==========

[commit-bot: I haz the power, 2016-11-10 20:44:45]

Patchset 22 (id:??) landed as
https://crrev.com/2daaf676340283726a05cb1b387a9ff328e020e8
Cr-Commit-Position: refs/heads/master@{#431340}

[haraken, 2016-11-11 02:03:05]

On 2016/11/10 17:21:59, EhsanK wrote:
> Thank you all for the reviews.
> 
> cc-ed kojii@ and haraken@ since this is both blink arch. and IME related.
> 
> FYI, I will rebase and try CQ-ing.

I haven't looked at the code in details, but removing the duplicated code by
introducing WebInputMethodController definitely sounds like a good way to go.

[dcheng, 2016-11-11 07:52:15]

On 2016/11/02 22:20:21, lfg wrote:
> On 2016/11/02 22:16:34, EhsanK wrote:
> > On 2016/11/02 20:57:38, Charlie Reis wrote:
> > > This can't be a DCHECK, since we'll get a potentially exploitable bad cast
> in
> > > release builds if it's wrong.  Make it a CHECK (preferable if we don't
think
> > it
> > > can fail) or return early if it fails.
> > 
> > I see. Thanks! I don't think we can have a WebWidget which is
WebFrameWidget.
> > But I think lfg@ was mentioning a case this could have happened?
> > 
> > For now I made it a CHECK. lfg@ WDYT?
> 
> This would only fail if this function was called on a swapped out RenderView.
> CHECK is fine, it shouldn't happen.

Btw I missed this earlier, but this shouldn't be a CHECK: see the guidelines on
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Here, we're documenting a precondition of calling this function: if we violate
this precondition, either the renderer already has arbitrary code execution or
we have a bug in our calling code. Per the guidelines, that means this short of
thing should be a DCHECK (as a CHECK has somewhat higher cost).

[EhsanK, 2016-11-11 19:48:26]

Description was changed from

==========
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-k6zKI0e...

(Chromium.org access):
https://docs.google.com/document/d/13Nidlg5yIsxQRqXgBPqmb-KcT8q85Z1ds9Z1-Vo1X...

Committed: https://crrev.com/2daaf676340283726a05cb1b387a9ff328e020e8
Cr-Commit-Position: refs/heads/master@{#431340}
==========

to

==========
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-k6zKI0e...

(Chromium.org access):
https://docs.google.com/document/d/13Nidlg5yIsxQRqXgBPqmb-KcT8q85Z1ds9Z1-Vo1X...

BUG=629721

Committed: https://crrev.com/2daaf676340283726a05cb1b387a9ff328e020e8
Cr-Commit-Position: refs/heads/master@{#431340}
==========