|
|
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. |
DescriptionImplement 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 #
Messages
Total messages: 62 (32 generated)
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yhanada@chromium.org changed reviewers: + hidehiko@chromium.org, kinaba@chromium.org
PTAL. Thanks!
https://codereview.chromium.org/2876693004/diff/1/components/arc/common/ime.m... File components/arc/common/ime.mojom (right): https://codereview.chromium.org/2876693004/diff/1/components/arc/common/ime.m... components/arc/common/ime.mojom:38: uint32 end; Is |end| inclusive or exclusive? As this is interface between Chrome and ARC, could you explicitly document it? https://codereview.chromium.org/2876693004/diff/1/components/arc/common/ime.m... components/arc/common/ime.mojom:67: [MinVersion=5] OnCursorRectChangedWithSurroundingText@4( Could you document the args? Specifically what is the different between |text_range| and |selection_range| passed here? https://codereview.chromium.org/2876693004/diff/1/components/arc/ime/arc_ime_... File components/arc/ime/arc_ime_service.cc (right): https://codereview.chromium.org/2876693004/diff/1/components/arc/ime/arc_ime_... components/arc/ime/arc_ime_service.cc:240: text_range_ = text_range; These values should be invalidated when the text is being updated? E.g. updating composing text / focus switching etc.? https://codereview.chromium.org/2876693004/diff/1/components/arc/ime/arc_ime_... components/arc/ime/arc_ime_service.cc:368: if (!text_range_.IsValid() || range != text_range_) Could you document the limitation of the range? (IIUC, technically it is possible to support sub-range of |text_range_|, but it is not necessary for some reason?)
https://codereview.chromium.org/2876693004/diff/1/components/arc/common/ime.m... File components/arc/common/ime.mojom (right): https://codereview.chromium.org/2876693004/diff/1/components/arc/common/ime.m... components/arc/common/ime.mojom:57: OnCursorRectChanged@1(CursorRect rect); Will this still be used or is left for backward compatibility only? If the latter is the case, how about renaming it to OnCursorRectChangedDeprecated@1 ? (with a comment) https://codereview.chromium.org/2876693004/diff/1/components/arc/common/ime.m... components/arc/common/ime.mojom:67: [MinVersion=5] OnCursorRectChangedWithSurroundingText@4( Could you also add a comment why all these info are piggy-backed into a single method? (I.e., e.g., because Chrome OS IME tries to retrieve these information synchronously, we need to update them all at once to keep consistency, etc)
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please take another look. Thank you! https://codereview.chromium.org/2876693004/diff/1/components/arc/common/ime.m... File components/arc/common/ime.mojom (right): https://codereview.chromium.org/2876693004/diff/1/components/arc/common/ime.m... components/arc/common/ime.mojom:38: uint32 end; On 2017/05/11 14:32:31, hidehiko wrote: > Is |end| inclusive or exclusive? As this is interface between Chrome and ARC, > could you explicitly document it? Done. https://codereview.chromium.org/2876693004/diff/1/components/arc/common/ime.m... components/arc/common/ime.mojom:57: OnCursorRectChanged@1(CursorRect rect); On 2017/05/12 01:28:35, kinaba wrote: > Will this still be used or is left for backward compatibility only? > If the latter is the case, how about renaming it to > OnCursorRectChangedDeprecated@1 ? (with a comment) This will be used when failing to get the text and the selection range in Android side. https://codereview.chromium.org/2876693004/diff/1/components/arc/common/ime.m... components/arc/common/ime.mojom:67: [MinVersion=5] OnCursorRectChangedWithSurroundingText@4( On 2017/05/12 01:28:35, kinaba wrote: > Could you also add a comment why all these info are piggy-backed > into a single method? (I.e., e.g., because Chrome OS IME tries > to retrieve these information synchronously, we need to update them > all at once to keep consistency, etc) Done. https://codereview.chromium.org/2876693004/diff/1/components/arc/ime/arc_ime_... File components/arc/ime/arc_ime_service.cc (right): https://codereview.chromium.org/2876693004/diff/1/components/arc/ime/arc_ime_... components/arc/ime/arc_ime_service.cc:240: text_range_ = text_range; On 2017/05/11 14:32:32, hidehiko wrote: > These values should be invalidated when the text is being updated? E.g. updating > composing text / focus switching etc.? These value should be used from InputMethod::OnCaretBoundsChanged(). I changed to invalidate them after OnCaretBoundsChanged() call. https://codereview.chromium.org/2876693004/diff/1/components/arc/ime/arc_ime_... components/arc/ime/arc_ime_service.cc:368: if (!text_range_.IsValid() || range != text_range_) On 2017/05/11 14:32:31, hidehiko wrote: > Could you document the limitation of the range? > (IIUC, technically it is possible to support sub-range of |text_range_|, but it > is not necessary for some reason?) As I commented above, I suppose that this method is called only from InputMethod::OnCaretBoundsChanged(). In that method, the range obtained from GetTextRange() is used as the argument of this method. It's why I added |range != text_range_|. I added the comment.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2876693004/diff/20001/components/arc/ime/arc_... File components/arc/ime/arc_ime_service.cc (right): https://codereview.chromium.org/2876693004/diff/20001/components/arc/ime/arc_... components/arc/ime/arc_ime_service.cc:244: InvalidateSurroundingTextAndSelectionRange(); I guess you meant to Invalidate() in other place(s)..? https://codereview.chromium.org/2876693004/diff/20001/components/arc/ime/arc_... components/arc/ime/arc_ime_service.cc:369: // It's supposed that this methdo is called only from methdo => method https://codereview.chromium.org/2876693004/diff/20001/components/arc/ime/arc_... components/arc/ime/arc_ime_service.cc:432: text_in_range_ = base::ASCIIToUTF16(""); base::string16()
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
https://codereview.chromium.org/2876693004/diff/20001/components/arc/ime/arc_... File components/arc/ime/arc_ime_service.cc (right): https://codereview.chromium.org/2876693004/diff/20001/components/arc/ime/arc_... components/arc/ime/arc_ime_service.cc:244: InvalidateSurroundingTextAndSelectionRange(); On 2017/05/15 01:35:09, kinaba wrote: > I guess you meant to Invalidate() in other place(s)..? Done. https://codereview.chromium.org/2876693004/diff/20001/components/arc/ime/arc_... components/arc/ime/arc_ime_service.cc:369: // It's supposed that this methdo is called only from On 2017/05/15 01:35:09, kinaba wrote: > methdo => method Done. https://codereview.chromium.org/2876693004/diff/20001/components/arc/ime/arc_... components/arc/ime/arc_ime_service.cc:432: text_in_range_ = base::ASCIIToUTF16(""); On 2017/05/15 01:35:09, kinaba wrote: > base::string16() Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
thanks. almost there for me https://codereview.chromium.org/2876693004/diff/40001/components/arc/ime/arc_... File components/arc/ime/arc_ime_service.cc (right): https://codereview.chromium.org/2876693004/diff/40001/components/arc/ime/arc_... components/arc/ime/arc_ime_service.cc:243: return; Can't there be a case that the rects are the same but surrounding texts differ? https://codereview.chromium.org/2876693004/diff/40001/components/arc/ime/arc_... components/arc/ime/arc_ime_service.cc:401: ime_bridge_->SendExtendSelectionAndDelete(before, after); InvalidateSurroundingTextAndSelectionRange here too
https://codereview.chromium.org/2876693004/diff/40001/components/arc/ime/arc_... File components/arc/ime/arc_ime_service.cc (right): https://codereview.chromium.org/2876693004/diff/40001/components/arc/ime/arc_... components/arc/ime/arc_ime_service.cc:243: return; On 2017/05/15 07:55:18, kinaba wrote: > Can't there be a case that the rects are the same but > surrounding texts differ? It is possible. I changed to save the text and the ranges even if the rect is not changed. https://codereview.chromium.org/2876693004/diff/40001/components/arc/ime/arc_... components/arc/ime/arc_ime_service.cc:401: ime_bridge_->SendExtendSelectionAndDelete(before, after); On 2017/05/15 07:55:18, kinaba wrote: > InvalidateSurroundingTextAndSelectionRange here too Done.
lgtm. Please wait for kinaba@'s review.
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
drive-by https://codereview.chromium.org/2876693004/diff/60001/components/arc/common/i... File components/arc/common/ime.mojom (right): https://codereview.chromium.org/2876693004/diff/60001/components/arc/common/i... components/arc/common/ime.mojom:35: // Represents the range in the text. It is an open interval [start, end). nit: can you document what these offsets represent? Seems like they should be the UTF-16 index, based on the other comments in the file. https://codereview.chromium.org/2876693004/diff/60001/components/arc/common/i... components/arc/common/ime.mojom:79: ); nit: Clang-format does not work here, but can you format it in a style consistent with the rest of Chromium? [MinVersion=5] OnCursorRectChangedWithSurroundingText@4( CursorRect rect, // The cursor position. TextRange text_range, // The range of |text_in_range| in the current text // in the editor. string text_in_range, // The text around the cursor. TextRange selection_range // The range of the selected text // in the current text in the editor. ); https://codereview.chromium.org/2876693004/diff/60001/components/arc/ime/arc_... File components/arc/ime/arc_ime_bridge_impl.h (right): https://codereview.chromium.org/2876693004/diff/60001/components/arc/ime/arc_... components/arc/ime/arc_ime_bridge_impl.h:50: mojom::CursorRectPtr rect, Can you add typemappings from mojom::CursorRectPtr -> gfx::Rect and mojom::TextRangePtr -> gfx::Range so you don't need the (manual) conversions?
lgtm assuming Luis's points will be addressed
Description was changed from ========== 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=705048, b/32862214 ========== to ========== 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 ==========
yhanada@chromium.org changed reviewers: + dcheng@chromium.org, rsesek@chromium.org
I changed to use typemappings. Please take another look. +rsesek@: I added a dependency to ui/gfx/range/range.h. PTAL. +dcheng@: PTAL at mojom changes and typemappings change. Thanks. https://codereview.chromium.org/2876693004/diff/60001/components/arc/common/i... File components/arc/common/ime.mojom (right): https://codereview.chromium.org/2876693004/diff/60001/components/arc/common/i... components/arc/common/ime.mojom:35: // Represents the range in the text. It is an open interval [start, end). On 2017/05/15 15:44:26, Luis Héctor Chávez wrote: > nit: can you document what these offsets represent? Seems like they should be > the UTF-16 index, based on the other comments in the file. Done. https://codereview.chromium.org/2876693004/diff/60001/components/arc/common/i... components/arc/common/ime.mojom:79: ); On 2017/05/15 15:44:27, Luis Héctor Chávez wrote: > nit: Clang-format does not work here, but can you format it in a style > consistent with the rest of Chromium? > > [MinVersion=5] OnCursorRectChangedWithSurroundingText@4( > CursorRect rect, // The cursor position. > TextRange text_range, // The range of |text_in_range| in the current text > // in the editor. > string text_in_range, // The text around the cursor. > TextRange selection_range // The range of the selected text > // in the current text in the editor. > ); Done. https://codereview.chromium.org/2876693004/diff/60001/components/arc/ime/arc_... File components/arc/ime/arc_ime_bridge_impl.h (right): https://codereview.chromium.org/2876693004/diff/60001/components/arc/ime/arc_... components/arc/ime/arc_ime_bridge_impl.h:50: mojom::CursorRectPtr rect, On 2017/05/15 15:44:27, Luis Héctor Chávez wrote: > Can you add typemappings from mojom::CursorRectPtr -> gfx::Rect and > mojom::TextRangePtr -> gfx::Range so you don't need the (manual) conversions? Done.
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rsesek is a security OWNER as well, so I'll defer to him for the entire patchset.
https://codereview.chromium.org/2876693004/diff/80001/components/arc/common/i... File components/arc/common/ime.mojom (right): https://codereview.chromium.org/2876693004/diff/80001/components/arc/common/i... components/arc/common/ime.mojom:75: CursorRect rect, // The cursor position. Optional: How about using Rect in ui/gfx/geometry/mojo/geometry.mojom, then? https://codereview.chromium.org/2876693004/diff/80001/components/arc/common/i... components/arc/common/ime.mojom:76: TextRange text_range, // The range of |text_in_range| in the current Ditto, for ui/gfx/range/mojo/range.mojom.
On 2017/05/16 10:08:59, hidehiko wrote: > https://codereview.chromium.org/2876693004/diff/80001/components/arc/common/i... > File components/arc/common/ime.mojom (right): > > https://codereview.chromium.org/2876693004/diff/80001/components/arc/common/i... > components/arc/common/ime.mojom:75: CursorRect rect, // The cursor > position. > Optional: How about using Rect in ui/gfx/geometry/mojo/geometry.mojom, then? > > https://codereview.chromium.org/2876693004/diff/80001/components/arc/common/i... > components/arc/common/ime.mojom:76: TextRange text_range, // The range of > |text_in_range| in the current > Ditto, for ui/gfx/range/mojo/range.mojom. I will make follow-up CL to address these types and use typemappings for other types (e.g. TextInputType, CompositionSegement).
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/i... > > File components/arc/common/ime.mojom (right): > > > > > https://codereview.chromium.org/2876693004/diff/80001/components/arc/common/i... > > components/arc/common/ime.mojom:75: CursorRect rect, // The cursor > > position. > > Optional: How about using Rect in ui/gfx/geometry/mojo/geometry.mojom, then? > > > > > https://codereview.chromium.org/2876693004/diff/80001/components/arc/common/i... > > components/arc/common/ime.mojom:76: TextRange text_range, // The range of > > |text_in_range| in the current > > Ditto, for ui/gfx/range/mojo/range.mojom. > > I will make follow-up CL to address these types and use typemappings for other > types (e.g. TextInputType, CompositionSegement). oh, because ui/gfx/**/*.mojom is not in Android repository, we cannot use them for arc mojom files...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
On 2017/05/16 11:01:55, yhanada wrote: > 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/i... > > > File components/arc/common/ime.mojom (right): > > > > > > > > > https://codereview.chromium.org/2876693004/diff/80001/components/arc/common/i... > > > components/arc/common/ime.mojom:75: CursorRect rect, // The cursor > > > position. > > > Optional: How about using Rect in ui/gfx/geometry/mojo/geometry.mojom, then? > > > > > > > > > https://codereview.chromium.org/2876693004/diff/80001/components/arc/common/i... > > > components/arc/common/ime.mojom:76: TextRange text_range, // The range > of > > > |text_in_range| in the current > > > Ditto, for ui/gfx/range/mojo/range.mojom. > > > > I will make follow-up CL to address these types and use typemappings for other > > types (e.g. TextInputType, CompositionSegement). > > oh, because ui/gfx/**/*.mojom is not in Android repository, we cannot use them > for arc mojom files... Can they be made available? It seems undesirable to have two different deserializers for the same type.
On 2017/05/16 14:10:32, Robert Sesek wrote: > On 2017/05/16 11:01:55, yhanada wrote: > > 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/i... > > > > File components/arc/common/ime.mojom (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2876693004/diff/80001/components/arc/common/i... > > > > components/arc/common/ime.mojom:75: CursorRect rect, // The > cursor > > > > position. > > > > Optional: How about using Rect in ui/gfx/geometry/mojo/geometry.mojom, > then? > > > > > > > > > > > > > > https://codereview.chromium.org/2876693004/diff/80001/components/arc/common/i... > > > > components/arc/common/ime.mojom:76: TextRange text_range, // The > range > > of > > > > |text_in_range| in the current > > > > Ditto, for ui/gfx/range/mojo/range.mojom. > > > > > > I will make follow-up CL to address these types and use typemappings for > other > > > types (e.g. TextInputType, CompositionSegement). > > > > oh, because ui/gfx/**/*.mojom is not in Android repository, we cannot use them > > for arc mojom files... > > Can they be made available? It seems undesirable to have two different > deserializers for the same type. Currently we are coping all mojom files in components/arc to Android repository manually. This makes it difficult to re-use mojom files not in components/arc. I filed a tracking bug for it (http://crbug.com/723224) and will address this in follow-up CLs.
On 2017/05/17 02:12:01, yhanada wrote: > On 2017/05/16 14:10:32, Robert Sesek wrote: > > On 2017/05/16 11:01:55, yhanada wrote: > > > 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/i... > > > > > File components/arc/common/ime.mojom (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2876693004/diff/80001/components/arc/common/i... > > > > > components/arc/common/ime.mojom:75: CursorRect rect, // The > > cursor > > > > > position. > > > > > Optional: How about using Rect in ui/gfx/geometry/mojo/geometry.mojom, > > then? > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2876693004/diff/80001/components/arc/common/i... > > > > > components/arc/common/ime.mojom:76: TextRange text_range, // The > > range > > > of > > > > > |text_in_range| in the current > > > > > Ditto, for ui/gfx/range/mojo/range.mojom. > > > > > > > > I will make follow-up CL to address these types and use typemappings for > > other > > > > types (e.g. TextInputType, CompositionSegement). > > > > > > oh, because ui/gfx/**/*.mojom is not in Android repository, we cannot use > them > > > for arc mojom files... > > > > Can they be made available? It seems undesirable to have two different > > deserializers for the same type. > > Currently we are coping all mojom files in components/arc to Android repository > manually. This makes it difficult to re-use mojom files not in components/arc. > I filed a tracking bug for it (http://crbug.com/723224) and will address this in > follow-up CLs. Support for this will land in the next few days :)
lgtm
LGTM On 2017/05/17 02:12:01, yhanada wrote: > On 2017/05/16 14:10:32, Robert Sesek wrote: > > On 2017/05/16 11:01:55, yhanada wrote: > > > 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/i... > > > > > File components/arc/common/ime.mojom (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2876693004/diff/80001/components/arc/common/i... > > > > > components/arc/common/ime.mojom:75: CursorRect rect, // The > > cursor > > > > > position. > > > > > Optional: How about using Rect in ui/gfx/geometry/mojo/geometry.mojom, > > then? > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2876693004/diff/80001/components/arc/common/i... > > > > > components/arc/common/ime.mojom:76: TextRange text_range, // The > > range > > > of > > > > > |text_in_range| in the current > > > > > Ditto, for ui/gfx/range/mojo/range.mojom. > > > > > > > > I will make follow-up CL to address these types and use typemappings for > > other > > > > types (e.g. TextInputType, CompositionSegement). > > > > > > oh, because ui/gfx/**/*.mojom is not in Android repository, we cannot use > them > > > for arc mojom files... > > > > Can they be made available? It seems undesirable to have two different > > deserializers for the same type. > > Currently we are coping all mojom files in components/arc to Android repository > manually. This makes it difficult to re-use mojom files not in components/arc. > I filed a tracking bug for it (http://crbug.com/723224) and will address this in > follow-up CLs. Can you leave a TODO in the .mojom for this? https://codereview.chromium.org/2876693004/diff/80001/components/arc/ime/arc_... File components/arc/ime/arc_ime_struct_traits.h (right): https://codereview.chromium.org/2876693004/diff/80001/components/arc/ime/arc_... components/arc/ime/arc_ime_struct_traits.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 and in the .cc
Thank you for reviewing! https://codereview.chromium.org/2876693004/diff/80001/components/arc/ime/arc_... File components/arc/ime/arc_ime_struct_traits.h (right): https://codereview.chromium.org/2876693004/diff/80001/components/arc/ime/arc_... components/arc/ime/arc_ime_struct_traits.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/05/17 22:04:32, Robert Sesek wrote: > 2017 and in the .cc Done.
The CQ bit was checked by yhanada@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org, kinaba@chromium.org, rsesek@chromium.org, lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2876693004/#ps100001 (title: "add TODOs in ime.mojom")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by yhanada@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by yhanada@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by yhanada@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495103515945360, "parent_rev": "882e84aeece1a7166cd898354e21008aefabf00e", "commit_rev": "c0f1238e60e98da53c9f03c853a36df951065c0e"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/c0f1238e60e98da53c9f03c853a3... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/c0f1238e60e98da53c9f03c853a3... |