Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(335)

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

Created:
7 months ago by yhanada
Modified:
6 months, 3 weeks 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!
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 ...
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 ...
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 ...
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 ...
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 ...
6 months, 4 weeks 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 ...
6 months, 4 weeks 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 ...
6 months, 4 weeks ago (2017-05-15 08:06:38 UTC) #19
hidehiko
lgtm. Please wait for kinaba@'s review.
6 months, 4 weeks 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 ...
6 months, 4 weeks ago (2017-05-15 15:44:27 UTC) #22
kinaba
lgtm assuming Luis's points will be addressed
6 months, 4 weeks 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 ...
6 months, 4 weeks 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 ...
6 months, 4 weeks 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 ...
6 months, 4 weeks 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 > ...
6 months, 4 weeks 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 ...
6 months, 4 weeks 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 ...
6 months, 4 weeks 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 ...
6 months, 4 weeks 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 ...
6 months, 4 weeks ago (2017-05-17 02:16:18 UTC) #41
Luis Héctor Chávez
lgtm
6 months, 3 weeks 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: > > ...
6 months, 3 weeks 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 ...
6 months, 3 weeks 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
6 months, 3 weeks 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)
6 months, 3 weeks 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
6 months, 3 weeks 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)
6 months, 3 weeks 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
6 months, 3 weeks 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)
6 months, 3 weeks 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
6 months, 3 weeks ago (2017-05-18 10:32:55 UTC) #59
commit-bot: I haz the power
6 months, 3 weeks 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 0eb02b776