Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(133)

Issue 2876693004: Implement GetTextFromRange(), GetTextRange() and GetSelectionRange() for ArcImeService. (Closed)

Created:
3 years, 7 months ago by yhanada
Modified:
3 years, 7 months ago
CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, droger+watchlist_chromium.org, viettrungluu+watch_chromium.org, blundell+watchlist_chromium.org, hidehiko+watch_chromium.org, sdefresne+watchlist_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, darin (slow to review), qsr+mojo_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Implement GetTextFromRange(), GetTextRange() and GetSelectionRange() for ArcImeService. This CL adds new mojo call OnCaretBoundsChangedWithSurroundingText to send text around cursor and selection range together with new cursor rect. Because GetTextFromRange(), GetTextRange() and GetSelectionRange() are called only after calling ArcImeService::OnCursorRectChanged(), the new mojo call is enought to sync the state of text and selection range. BUG=722671 Review-Url: https://codereview.chromium.org/2876693004 Cr-Commit-Position: refs/heads/master@{#472775} Committed: https://chromium.googlesource.com/chromium/src/+/c0f1238e60e98da53c9f03c853a36df951065c0e

Patch Set 1 #

Total comments: 11

Patch Set 2 : add comments #

Total comments: 6

Patch Set 3 : address the comments #

Total comments: 4

Patch Set 4 : save surrounding text even if cursor rect is not changed #

Total comments: 6

Patch Set 5 : use typemapping #

Total comments: 4

Patch Set 6 : add TODOs in ime.mojom #

Unified diffs Side-by-side diffs Delta from patch set Stats (+233 lines, -23 lines) Patch
M components/arc/DEPS View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M components/arc/common/ime.mojom View 1 2 3 4 5 4 chunks +27 lines, -1 line 0 comments Download
A components/arc/common/ime.typemap View 1 2 3 4 1 chunk +20 lines, -0 lines 0 comments Download
M components/arc/common/typemaps.gni View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
A components/arc/ime/OWNERS View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M components/arc/ime/arc_ime_bridge.h View 2 chunks +6 lines, -0 lines 0 comments Download
M components/arc/ime/arc_ime_bridge_impl.h View 1 2 3 4 1 chunk +6 lines, -1 line 0 comments Download
M components/arc/ime/arc_ime_bridge_impl.cc View 1 2 3 4 2 chunks +11 lines, -4 lines 0 comments Download
M components/arc/ime/arc_ime_service.h View 1 4 chunks +14 lines, -4 lines 0 comments Download
M components/arc/ime/arc_ime_service.cc View 1 2 3 12 chunks +60 lines, -13 lines 0 comments Download
M components/arc/ime/arc_ime_service_unittest.cc View 1 chunk +25 lines, -0 lines 0 comments Download
A components/arc/ime/arc_ime_struct_traits.h View 1 2 3 4 5 1 chunk +33 lines, -0 lines 0 comments Download
A components/arc/ime/arc_ime_struct_traits.cc View 1 2 3 4 5 1 chunk +27 lines, -0 lines 0 comments Download

Messages

Total messages: 62 (32 generated)
yhanada
PTAL. Thanks!
3 years, 7 months ago (2017-05-11 12:29:04 UTC) #6
hidehiko
https://codereview.chromium.org/2876693004/diff/1/components/arc/common/ime.mojom File components/arc/common/ime.mojom (right): https://codereview.chromium.org/2876693004/diff/1/components/arc/common/ime.mojom#newcode38 components/arc/common/ime.mojom:38: uint32 end; Is |end| inclusive or exclusive? As this ...
3 years, 7 months ago (2017-05-11 14:32:32 UTC) #7
kinaba
https://codereview.chromium.org/2876693004/diff/1/components/arc/common/ime.mojom File components/arc/common/ime.mojom (right): https://codereview.chromium.org/2876693004/diff/1/components/arc/common/ime.mojom#newcode57 components/arc/common/ime.mojom:57: OnCursorRectChanged@1(CursorRect rect); Will this still be used or is ...
3 years, 7 months ago (2017-05-12 01:28:35 UTC) #8
yhanada
Please take another look. Thank you! https://codereview.chromium.org/2876693004/diff/1/components/arc/common/ime.mojom File components/arc/common/ime.mojom (right): https://codereview.chromium.org/2876693004/diff/1/components/arc/common/ime.mojom#newcode38 components/arc/common/ime.mojom:38: uint32 end; On ...
3 years, 7 months ago (2017-05-12 07:52:07 UTC) #11
kinaba
https://codereview.chromium.org/2876693004/diff/20001/components/arc/ime/arc_ime_service.cc File components/arc/ime/arc_ime_service.cc (right): https://codereview.chromium.org/2876693004/diff/20001/components/arc/ime/arc_ime_service.cc#newcode244 components/arc/ime/arc_ime_service.cc:244: InvalidateSurroundingTextAndSelectionRange(); I guess you meant to Invalidate() in other ...
3 years, 7 months ago (2017-05-15 01:35:09 UTC) #14
yhanada
https://codereview.chromium.org/2876693004/diff/20001/components/arc/ime/arc_ime_service.cc File components/arc/ime/arc_ime_service.cc (right): https://codereview.chromium.org/2876693004/diff/20001/components/arc/ime/arc_ime_service.cc#newcode244 components/arc/ime/arc_ime_service.cc:244: InvalidateSurroundingTextAndSelectionRange(); On 2017/05/15 01:35:09, kinaba wrote: > I guess ...
3 years, 7 months ago (2017-05-15 07:42:58 UTC) #16
kinaba
thanks. almost there for me https://codereview.chromium.org/2876693004/diff/40001/components/arc/ime/arc_ime_service.cc File components/arc/ime/arc_ime_service.cc (right): https://codereview.chromium.org/2876693004/diff/40001/components/arc/ime/arc_ime_service.cc#newcode243 components/arc/ime/arc_ime_service.cc:243: return; Can't there be ...
3 years, 7 months ago (2017-05-15 07:55:18 UTC) #18
yhanada
https://codereview.chromium.org/2876693004/diff/40001/components/arc/ime/arc_ime_service.cc File components/arc/ime/arc_ime_service.cc (right): https://codereview.chromium.org/2876693004/diff/40001/components/arc/ime/arc_ime_service.cc#newcode243 components/arc/ime/arc_ime_service.cc:243: return; On 2017/05/15 07:55:18, kinaba wrote: > Can't there ...
3 years, 7 months ago (2017-05-15 08:06:38 UTC) #19
hidehiko
lgtm. Please wait for kinaba@'s review.
3 years, 7 months ago (2017-05-15 13:40:26 UTC) #20
Luis Héctor Chávez
drive-by https://codereview.chromium.org/2876693004/diff/60001/components/arc/common/ime.mojom File components/arc/common/ime.mojom (right): https://codereview.chromium.org/2876693004/diff/60001/components/arc/common/ime.mojom#newcode35 components/arc/common/ime.mojom:35: // Represents the range in the text. It ...
3 years, 7 months ago (2017-05-15 15:44:27 UTC) #22
kinaba
lgtm assuming Luis's points will be addressed
3 years, 7 months ago (2017-05-16 01:12:52 UTC) #23
yhanada
I changed to use typemappings. Please take another look. +rsesek@: I added a dependency to ...
3 years, 7 months ago (2017-05-16 10:02:13 UTC) #26
dcheng
rsesek is a security OWNER as well, so I'll defer to him for the entire ...
3 years, 7 months ago (2017-05-16 10:07:41 UTC) #29
hidehiko
https://codereview.chromium.org/2876693004/diff/80001/components/arc/common/ime.mojom File components/arc/common/ime.mojom (right): https://codereview.chromium.org/2876693004/diff/80001/components/arc/common/ime.mojom#newcode75 components/arc/common/ime.mojom:75: CursorRect rect, // The cursor position. Optional: How about ...
3 years, 7 months ago (2017-05-16 10:08:59 UTC) #30
yhanada
On 2017/05/16 10:08:59, hidehiko wrote: > https://codereview.chromium.org/2876693004/diff/80001/components/arc/common/ime.mojom > File components/arc/common/ime.mojom (right): > > https://codereview.chromium.org/2876693004/diff/80001/components/arc/common/ime.mojom#newcode75 > ...
3 years, 7 months ago (2017-05-16 10:59:55 UTC) #31
yhanada
On 2017/05/16 10:59:55, yhanada wrote: > On 2017/05/16 10:08:59, hidehiko wrote: > > > https://codereview.chromium.org/2876693004/diff/80001/components/arc/common/ime.mojom ...
3 years, 7 months ago (2017-05-16 11:01:55 UTC) #32
Robert Sesek
On 2017/05/16 11:01:55, yhanada wrote: > On 2017/05/16 10:59:55, yhanada wrote: > > On 2017/05/16 ...
3 years, 7 months ago (2017-05-16 14:10:32 UTC) #39
yhanada
On 2017/05/16 14:10:32, Robert Sesek wrote: > On 2017/05/16 11:01:55, yhanada wrote: > > On ...
3 years, 7 months ago (2017-05-17 02:12:01 UTC) #40
lhc(google)
On 2017/05/17 02:12:01, yhanada wrote: > On 2017/05/16 14:10:32, Robert Sesek wrote: > > On ...
3 years, 7 months ago (2017-05-17 02:16:18 UTC) #41
Luis Héctor Chávez
lgtm
3 years, 7 months ago (2017-05-17 16:01:33 UTC) #42
Robert Sesek
LGTM On 2017/05/17 02:12:01, yhanada wrote: > On 2017/05/16 14:10:32, Robert Sesek wrote: > > ...
3 years, 7 months ago (2017-05-17 22:04:32 UTC) #43
yhanada
Thank you for reviewing! https://codereview.chromium.org/2876693004/diff/80001/components/arc/ime/arc_ime_struct_traits.h File components/arc/ime/arc_ime_struct_traits.h (right): https://codereview.chromium.org/2876693004/diff/80001/components/arc/ime/arc_ime_struct_traits.h#newcode1 components/arc/ime/arc_ime_struct_traits.h:1: // Copyright 2016 The Chromium ...
3 years, 7 months ago (2017-05-18 04:03:11 UTC) #44
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876693004/100001
3 years, 7 months ago (2017-05-18 04:05:24 UTC) #47
commit-bot: I haz the power
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_rel_ng/builds/430169)
3 years, 7 months ago (2017-05-18 06:04:20 UTC) #49
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876693004/100001
3 years, 7 months ago (2017-05-18 06:21:49 UTC) #51
commit-bot: I haz the power
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_rel_ng/builds/430260)
3 years, 7 months ago (2017-05-18 08:30:00 UTC) #53
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876693004/100001
3 years, 7 months ago (2017-05-18 08:31:04 UTC) #55
commit-bot: I haz the power
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_rel_ng/builds/430341)
3 years, 7 months ago (2017-05-18 10:30:18 UTC) #57
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876693004/100001
3 years, 7 months ago (2017-05-18 10:32:55 UTC) #59
commit-bot: I haz the power
3 years, 7 months ago (2017-05-18 12:08:00 UTC) #62
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/c0f1238e60e98da53c9f03c853a3...

Powered by Google App Engine
This is Rietveld 408576698