IME for Mus: Add ui.mojom.CompositionText and its struct traits.

IME for Mus: Add ui.mojom.CompositionText and its struct traits.

- Adds ui.mojom.CompositionUnderline and ui.mojom.CompositionText.
- Adds corresponding struct traits + unittest for struct traits.
- Breaks ui.mojom.TextInputClient::OnCompositionEvent() into multiple
  functions, each corresponding to a function in ui::TextInputClient.

BUG=631524
Committed: https://crrev.com/ead76a6a60d2db90bec1d257f47422c2e059db1c
Cr-Commit-Position: refs/heads/master@{#436143}

[Hadi, 2016-11-29 15:45:08]

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

[commit-bot: I haz the power, 2016-11-29 15:45:21]

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

[Hadi, 2016-11-29 15:53:14]

Description was changed from

==========
WIP: Support all composition events in ime.mojom.

BUG=NONE
==========

to

==========
IME for Mus: Add ui.mojom.CompositionText and its struct traits.

- Adds ui.mojom.CompositionUnderline and ui.mojom.CompositionText.
- Adds corresponding struct traits + unittest for struct traits.
- Breaks ui.mojom.TextInputClient::OnCompositionEvent() into multiple
  functions, each corresponding to a function in ui::TextInputClient.

BUG=631524
==========

[Hadi, 2016-11-29 15:56:06]

moshayedi@chromium.org changed reviewers:
+ sadrul@chromium.org, sky@chromium.org

[Hadi, 2016-11-29 15:56:07]

PTAL.

I could make InputMethod extensions work in a local branch. I'm breaking those
changes into multiple CLs.

[commit-bot: I haz the power, 2016-11-29 16:33:27]

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

[commit-bot: I haz the power, 2016-11-29 16:33:28]

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

[sadrul, 2016-11-29 20:51:35]

https://codereview.chromium.org/2539453002/diff/80001/services/ui/public/inte...
File services/ui/public/interfaces/ime/ime.mojom (right):

https://codereview.chromium.org/2539453002/diff/80001/services/ui/public/inte...
services/ui/public/interfaces/ime/ime.mojom:120: // crbug.com/631527.
Should this TODO be updated?

https://codereview.chromium.org/2539453002/diff/80001/ui/base/ime/BUILD.gn
File ui/base/ime/BUILD.gn (right):

https://codereview.chromium.org/2539453002/diff/80001/ui/base/ime/BUILD.gn#ne...
ui/base/ime/BUILD.gn:124: "//skia",
Why is this needed?

[Hadi, 2016-11-29 21:33:38]

https://codereview.chromium.org/2539453002/diff/80001/services/ui/public/inte...
File services/ui/public/interfaces/ime/ime.mojom (right):

https://codereview.chromium.org/2539453002/diff/80001/services/ui/public/inte...
services/ui/public/interfaces/ime/ime.mojom:120: // crbug.com/631527.
On 2016/11/29 20:51:35, sadrul wrote:
> Should this TODO be updated?

"Input text confirmation" should have been "Input context information". Fixed
it. We don't have those functions here yet.

https://codereview.chromium.org/2539453002/diff/80001/ui/base/ime/BUILD.gn
File ui/base/ime/BUILD.gn (right):

https://codereview.chromium.org/2539453002/diff/80001/ui/base/ime/BUILD.gn#ne...
ui/base/ime/BUILD.gn:124: "//skia",
On 2016/11/29 20:51:35, sadrul wrote:
> Why is this needed?

composition_underline.h uses SkColor in a public field. If we skip this, we get
"fatal error: 'SkTypes.h' file not found" when building ime_struct_traits.
Directly depending on "//third_party/skia" causes errors like "is_fuchsia not
defined".

Searching for "//third_party/skia" in BUILD.gn files doesn't return much
results:
https://cs.chromium.org/search/?q=//third_party/skia+file:BUILD.gn&sq=package...,
so I went with "//skia".

[sadrul, 2016-11-30 17:08:33]

lgtm

[sky, 2016-11-30 21:55:41]

https://codereview.chromium.org/2539453002/diff/140001/services/ui/ime/ime_un...
File services/ui/ime/ime_unittest.cc (right):

https://codereview.chromium.org/2539453002/diff/140001/services/ui/ime/ime_un...
services/ui/ime/ime_unittest.cc:103: EXPECT_TRUE(received_event &&
received_event->IsKeyEvent());
EXPECT->ASSERT, otherwise line 106 will crash.

https://codereview.chromium.org/2539453002/diff/140001/services/ui/public/int...
File services/ui/public/interfaces/ime/ime.mojom (right):

https://codereview.chromium.org/2539453002/diff/140001/services/ui/public/int...
services/ui/public/interfaces/ime/ime.mojom:111: ConfirmCompositionText();
Please add descriptions of all functions.

https://codereview.chromium.org/2539453002/diff/140001/services/ui/public/int...
services/ui/public/interfaces/ime/ime.mojom:114: InsertChar(ui.mojom.Event
event);
Is there a reason to have both insert text and char? Isn't char a subset of
text?

[Hadi, 2016-12-01 16:12:00]

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

[commit-bot: I haz the power, 2016-12-01 16:12:15]

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-12-01 17:32:20]

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

[commit-bot: I haz the power, 2016-12-01 17:32:21]

Dry run: This issue passed the CQ dry run.

[Hadi, 2016-12-01 19:31:41]

moshayedi@chromium.org changed reviewers:
+ tsepez@chromium.org

[Hadi, 2016-12-01 19:31:42]

Addressed feedback.

tsepez@: can you please review mojom files?

Fixing the weird windows compile errors in
https://codereview.chromium.org/2547573002/.

Thanks.

https://codereview.chromium.org/2539453002/diff/140001/services/ui/ime/ime_un...
File services/ui/ime/ime_unittest.cc (right):

https://codereview.chromium.org/2539453002/diff/140001/services/ui/ime/ime_un...
services/ui/ime/ime_unittest.cc:103: EXPECT_TRUE(received_event &&
received_event->IsKeyEvent());
On 2016/11/30 21:55:41, sky wrote:
> EXPECT->ASSERT, otherwise line 106 will crash.

Done.

https://codereview.chromium.org/2539453002/diff/140001/services/ui/public/int...
File services/ui/public/interfaces/ime/ime.mojom (right):

https://codereview.chromium.org/2539453002/diff/140001/services/ui/public/int...
services/ui/public/interfaces/ime/ime.mojom:111: ConfirmCompositionText();
On 2016/11/30 21:55:41, sky wrote:
> Please add descriptions of all functions.

Done.

https://codereview.chromium.org/2539453002/diff/140001/services/ui/public/int...
services/ui/public/interfaces/ime/ime.mojom:114: InsertChar(ui.mojom.Event
event);
On 2016/11/30 21:55:41, sky wrote:
> Is there a reason to have both insert text and char? Isn't char a subset of
> text?

Comments in ui/base/ime/text_input_client.h say that the text in InsertText() is
processed by input method, but the event in InsertChar() is not processed by
input method.

The implementation of these two functions in Textfield is slightly different.
For example, for passwords InsertChar() is used, which may reveal the last
character if needed.

We probably could've used only InsertText() for the mojo interface, and decide
to use ui::TextInputClient::InsertChar() or InsertText() on the client side
based on the text length, but for now I thought we better follow
ui::TextInputClient's interface until we find a good reason not to.

[Hadi, 2016-12-01 19:33:15]

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

[commit-bot: I haz the power, 2016-12-01 19:33:50]

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

[Tom Sepez, 2016-12-01 19:46:40]

I'd expected to see a manifest file showing where this can be called.  The
ability to send input text is fairly sensitive.  How do we know this will only
be called from a suitable client?  Thanks.

[commit-bot: I haz the power, 2016-12-01 20:18:07]

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

[commit-bot: I haz the power, 2016-12-01 20:18:08]

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

[Hadi, 2016-12-01 20:19:48]

On 2016/12/01 19:46:40, Tom Sepez wrote:
> I'd expected to see a manifest file showing where this can be called.  The
> ability to send input text is fairly sensitive.  How do we know this will only
> be called from a suitable client?  Thanks.

Thanks for the feedback. Here is my understanding, please comment if I'm wrong.

We have three actors here:
 - General mus+ash app: Implements TextInputClient, but doesn't have it in
"provides" list in the manifest file.
 - Mus: Provides IMEServer to apps, provides IMERegistrar for clients requiring
"ui:ime_registrar" (See ui/services/manifest.json).
 - IME driver: Provides "ime:ime_driver" so Mus can discover it via the Catalog.
I've put "ui::mojom::IMEDriver" as an interface it provides, but I think I
should remove it. See
chrome/app/mash/chrome_mash_content_browser_manifest_overlay.json.

Mus discovers the IME driver using Catalog service and launches it. The IME
driver registers itself to Mus. The client connects to Mus' IMEServer, and
provides a pointer to its implementation of TextInputClient. Mus passes this
interface pointer to the driver. Driver now can call InsertText(), etc.

I think because TextInputClient is not provided via the manifest, a random app
shouldn't be able to connect to other apps and insert text.

[sky, 2016-12-01 20:20:19]

Makes sense, LGTM

[Tom Sepez, 2016-12-01 21:20:11]

lgtm

[Hadi, 2016-12-02 21:11:55]

The CQ bit was checked by moshayedi@chromium.org

[Hadi, 2016-12-02 21:11:56]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2539453002/#ps180001
(title: "Addressed feedback.")

[commit-bot: I haz the power, 2016-12-02 21:12:26]

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-12-03 00:37:35]

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

[commit-bot: I haz the power, 2016-12-03 00:37:36]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Hadi, 2016-12-03 00:44:02]

The CQ bit was checked by moshayedi@chromium.org

[commit-bot: I haz the power, 2016-12-03 00:44:49]

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-12-03 02:26:50]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1480725842032710,
"parent_rev": "91dc9053e95d9160a03eb6102be92d2cdd749d6c", "commit_rev":
"4701a75201e9e67344ef1a1f3d6d9dd0394eafe3"}

[commit-bot: I haz the power, 2016-12-03 02:27:15]

Description was changed from

==========
IME for Mus: Add ui.mojom.CompositionText and its struct traits.

- Adds ui.mojom.CompositionUnderline and ui.mojom.CompositionText.
- Adds corresponding struct traits + unittest for struct traits.
- Breaks ui.mojom.TextInputClient::OnCompositionEvent() into multiple
  functions, each corresponding to a function in ui::TextInputClient.

BUG=631524
==========

to

==========
IME for Mus: Add ui.mojom.CompositionText and its struct traits.

- Adds ui.mojom.CompositionUnderline and ui.mojom.CompositionText.
- Adds corresponding struct traits + unittest for struct traits.
- Breaks ui.mojom.TextInputClient::OnCompositionEvent() into multiple
  functions, each corresponding to a function in ui::TextInputClient.

BUG=631524
==========

[commit-bot: I haz the power, 2016-12-03 02:27:17]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-12-03 02:29:56]

Description was changed from

==========
IME for Mus: Add ui.mojom.CompositionText and its struct traits.

- Adds ui.mojom.CompositionUnderline and ui.mojom.CompositionText.
- Adds corresponding struct traits + unittest for struct traits.
- Breaks ui.mojom.TextInputClient::OnCompositionEvent() into multiple
  functions, each corresponding to a function in ui::TextInputClient.

BUG=631524
==========

to

==========
IME for Mus: Add ui.mojom.CompositionText and its struct traits.

- Adds ui.mojom.CompositionUnderline and ui.mojom.CompositionText.
- Adds corresponding struct traits + unittest for struct traits.
- Breaks ui.mojom.TextInputClient::OnCompositionEvent() into multiple
  functions, each corresponding to a function in ui::TextInputClient.

BUG=631524

Committed: https://crrev.com/ead76a6a60d2db90bec1d257f47422c2e059db1c
Cr-Commit-Position: refs/heads/master@{#436143}
==========

[commit-bot: I haz the power, 2016-12-03 02:29:57]

Patchset 10 (id:??) landed as
https://crrev.com/ead76a6a60d2db90bec1d257f47422c2e059db1c
Cr-Commit-Position: refs/heads/master@{#436143}