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

Issue 699333003: Support InputMethodManager#updateCursorAnchorInfo for Android 5.0 (Closed)

Created:
6 years, 1 month ago by yukawa
Modified:
3 years, 9 months ago
CC:
chromium-reviews, darin-cc_chromium.org, jam, nona+watch_chromium.org, penghuang+watch_chromium.org, James Su, yukishiino+watch_chromium.org, yusukes+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Support InputMethodManager#updateCursorAnchorInfo for Android 5.0 This is the 4th part of the series of CLs to enable InputMethodManager#updateCursorAnchorInfo support for Android 5.0. Here are previous CLs: - part1 crrev.com/671503005 - part2 crrev.com/679223002 - part3 crrev.com/755853004 - part4 crrev.com/784503002 - part5 crrev.com/757233003 Basically this CL consists of following 6 parts. 1. Implements native wrapper for CursorAnchorInfo.Builder APIs so that we can implement that functionality in native side as much as possible. 2. Exposes View#GetLocationOnScreen to native side. 3. Adds CursorAnchorInfoController as a native class that manages when and how CursorAnchorInfo object should be built and sent to the IME. 4. Adds tests for CursorAnchorInfoController. 5. Update IME API wrappers so that we can call new IME APIs for L from the native side. 6. Introduces --enable-cursor-anchor-info so that we can start testing this functionality behind the flag. As discussed in the previous CLs, that callback is called in Android 5.0 and later. Hence no performance impact is intended in KitKat and prior. Design Doc: https://docs.google.com/a/chromium.org/document/d/1KdnoynGURrJBP1vDHcv2jwUdnsXHzRSxb5v4rK7UgtU/view BUG=424866 TEST=Manually done on Nexus 7 Build/LRX21P

Patch Set 1 #

Total comments: 3

Patch Set 2 : Removed "just-in-case" checks for the simplicity #

Total comments: 10

Patch Set 3 : Rebase #

Patch Set 4 : Address comments from jdduke@ and aelias@ #

Total comments: 9

Patch Set 5 : Rebase / Handle CursorAnchorInfo in native side rather than Java side #

Patch Set 6 : Rebase on top of crrev.com/755853004 #

Patch Set 7 : Rebase on top of crrev.com/757233003 #

Patch Set 8 : Rebase and handle FocusedNodeChanged for performance optimization #

Total comments: 10

Patch Set 9 : Address comments from jdduke@ / add --enable-cursor-anchor-info flag #

Patch Set 10 : Rebase #

Total comments: 2

Patch Set 11 : Move the core logic into Java side #

Total comments: 12

Patch Set 12 : Rebase + address commments from jdduke #

Total comments: 9

Patch Set 13 : Address trivial commments from jdduke before splitting this CL. #

Patch Set 14 : Rebase before splitting this CL. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1621 lines, -16 lines) Patch
M base/android/jni_array.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +4 lines, -0 lines 0 comments Download
M base/android/jni_array.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +14 lines, -0 lines 0 comments Download
M base/android/jni_array_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +26 lines, -0 lines 0 comments Download
M chrome/app/generated_resources.grd View 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +9 lines, -0 lines 0 comments Download
M content/browser/android/content_view_core_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +6 lines, -1 line 0 comments Download
M content/browser/android/content_view_core_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +28 lines, -2 lines 0 comments Download
M content/browser/renderer_host/ime_adapter_android.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +4 lines, -0 lines 0 comments Download
M content/browser/renderer_host/ime_adapter_android.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +26 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_process_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_android.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +3 lines, -2 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +12 lines, -1 line 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java View 1 2 3 4 5 6 7 8 9 10 2 chunks +23 lines, -0 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +11 lines, -0 lines 0 comments Download
A content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +467 lines, -0 lines 0 comments Download
A content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoWrapper.java View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +74 lines, -0 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 7 chunks +69 lines, -0 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/input/InputMethodManagerWrapper.java View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +6 lines, -5 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/common/ContentSwitches.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +4 lines, -0 lines 0 comments Download
A content/public/android/javatests/src/org/chromium/content/browser/input/CursorAnchorInfoControllerTest.java View 1 2 3 4 5 6 7 8 9 10 1 chunk +576 lines, -0 lines 0 comments Download
M content/public/common/content_switches.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M content/public/common/content_switches.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +3 lines, -0 lines 0 comments Download
A content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCursorAnchorInfoWrapper.java View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +218 lines, -0 lines 0 comments Download
M content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestInputMethodManagerWrapper.java View 1 2 3 4 5 6 7 8 9 10 4 chunks +21 lines, -1 line 0 comments Download
M content/renderer/render_widget.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +8 lines, -4 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 56 (19 generated)
yukawa
Hi Aurimas. This is the 3rd part of my CL to enable CursorAnchorInfo APIs added ...
6 years, 1 month ago (2014-11-05 16:45:32 UTC) #2
aurimas (slooooooooow)
+bcwhite
6 years, 1 month ago (2014-11-06 18:38:51 UTC) #4
bcwhite
lgtm https://codereview.chromium.org/699333003/diff/1/content/browser/renderer_host/render_widget_host_view_android.cc File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/699333003/diff/1/content/browser/renderer_host/render_widget_host_view_android.cc#newcode1251 content/browser/renderer_host/render_widget_host_view_android.cc:1251: selection_start.edge_top.x() == selection_end.edge_bottom.x(); Is there any chance these ...
6 years, 1 month ago (2014-11-06 18:57:20 UTC) #5
yukawa
https://codereview.chromium.org/699333003/diff/1/content/browser/renderer_host/render_widget_host_view_android.cc File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/699333003/diff/1/content/browser/renderer_host/render_widget_host_view_android.cc#newcode1251 content/browser/renderer_host/render_widget_host_view_android.cc:1251: selection_start.edge_top.x() == selection_end.edge_bottom.x(); On 2014/11/06 18:57:19, bcwhite wrote: > ...
6 years, 1 month ago (2014-11-07 00:52:47 UTC) #7
bcwhite
lgtm https://codereview.chromium.org/699333003/diff/1/content/browser/renderer_host/render_widget_host_view_android.cc File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/699333003/diff/1/content/browser/renderer_host/render_widget_host_view_android.cc#newcode1251 content/browser/renderer_host/render_widget_host_view_android.cc:1251: selection_start.edge_top.x() == selection_end.edge_bottom.x(); Yes. If it's not broken, ...
6 years, 1 month ago (2014-11-07 01:15:07 UTC) #8
yukawa
Hi Ben, Alexandre, Jared. Can you take a look at following parts as an OWNER? ...
6 years, 1 month ago (2014-11-07 01:42:53 UTC) #10
jdduke (slow)
https://codereview.chromium.org/699333003/diff/40001/content/browser/renderer_host/render_widget_host_view_android.cc File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/699333003/diff/40001/content/browser/renderer_host/render_widget_host_view_android.cc#newcode1275 content/browser/renderer_host/render_widget_host_view_android.cc:1275: has_insertion_marker, Do we have to go through ContentViewCore? Can ...
6 years, 1 month ago (2014-11-07 01:47:00 UTC) #11
jdduke (slow)
https://codereview.chromium.org/699333003/diff/40001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/699333003/diff/40001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode2184 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2184: RenderCoordinates.NormalizedPoint normalizedPoint = This work could potentially be triggered ...
6 years, 1 month ago (2014-11-07 01:49:04 UTC) #12
aelias_OOO_until_Jul13
https://codereview.chromium.org/699333003/diff/40001/content/browser/renderer_host/render_widget_host_view_android.cc File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/699333003/diff/40001/content/browser/renderer_host/render_widget_host_view_android.cc#newcode1275 content/browser/renderer_host/render_widget_host_view_android.cc:1275: has_insertion_marker, I would like to keep this method as ...
6 years, 1 month ago (2014-11-07 02:04:29 UTC) #13
yukawa
Thank you for insightful comments and sorry for my slow response. I was travelling from ...
6 years, 1 month ago (2014-11-12 10:21:50 UTC) #14
jdduke (slow)
https://codereview.chromium.org/699333003/diff/80001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/699333003/diff/80001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode2181 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2181: frameMetadataSelectionStartType, frameMetadataSelectionStartEdgeTopX, Should we be plumbing this data if ...
6 years, 1 month ago (2014-11-12 19:05:07 UTC) #15
aelias_OOO_until_Jul13
https://codereview.chromium.org/699333003/diff/80001/cc/BUILD.gn File cc/BUILD.gn (right): https://codereview.chromium.org/699333003/diff/80001/cc/BUILD.gn#newcode846 cc/BUILD.gn:846: if (is_android) { Sorry, I realize I'm contradicting my ...
6 years, 1 month ago (2014-11-12 19:30:56 UTC) #16
yukawa
Sorry for the delayed response. I have looked at the code again and now hopefully ...
6 years ago (2014-12-05 07:42:19 UTC) #24
yukawa
Hi Alexandre and Jared. I'm back from long vacation and want to resume this task. ...
5 years, 11 months ago (2015-01-05 22:55:52 UTC) #25
jdduke (slow)
On 2015/01/05 22:55:52, yukawa wrote: > Hi Alexandre and Jared. I'm back from long vacation ...
5 years, 11 months ago (2015-01-06 17:03:26 UTC) #26
yukawa
On 2015/01/06 17:03:26, jdduke wrote: > On 2015/01/05 22:55:52, yukawa wrote: > > Hi Alexandre ...
5 years, 11 months ago (2015-01-07 02:12:41 UTC) #27
jdduke (slow)
https://codereview.chromium.org/699333003/diff/320001/ui/base/ime/android/cursor_anchor_info_controller.cc File ui/base/ime/android/cursor_anchor_info_controller.cc (right): https://codereview.chromium.org/699333003/diff/320001/ui/base/ime/android/cursor_anchor_info_controller.cc#newcode160 ui/base/ime/android/cursor_anchor_info_controller.cc:160: gfx::ScaleVector2d(frame_metadata.root_scroll_offset, -scale) Hmm, it looks like you're trying to ...
5 years, 11 months ago (2015-01-12 16:33:44 UTC) #29
yukawa
https://codereview.chromium.org/699333003/diff/320001/ui/base/ime/android/cursor_anchor_info_controller.cc File ui/base/ime/android/cursor_anchor_info_controller.cc (right): https://codereview.chromium.org/699333003/diff/320001/ui/base/ime/android/cursor_anchor_info_controller.cc#newcode160 ui/base/ime/android/cursor_anchor_info_controller.cc:160: gfx::ScaleVector2d(frame_metadata.root_scroll_offset, -scale) On 2015/01/12 16:33:44, jdduke wrote: > Hmm, ...
5 years, 11 months ago (2015-01-12 17:08:30 UTC) #30
yukawa
On 2015/01/12 17:08:30, yukawa wrote: > https://codereview.chromium.org/699333003/diff/320001/ui/base/ime/android/cursor_anchor_info_controller.cc > File ui/base/ime/android/cursor_anchor_info_controller.cc (right): > > https://codereview.chromium.org/699333003/diff/320001/ui/base/ime/android/cursor_anchor_info_controller.cc#newcode160 > ...
5 years, 11 months ago (2015-01-13 09:16:20 UTC) #31
jdduke (slow)
https://codereview.chromium.org/699333003/diff/320001/ui/android/java/src/org/chromium/ui/base/CursorAnchorInfoBuilder.java File ui/android/java/src/org/chromium/ui/base/CursorAnchorInfoBuilder.java (right): https://codereview.chromium.org/699333003/diff/320001/ui/android/java/src/org/chromium/ui/base/CursorAnchorInfoBuilder.java#newcode45 ui/android/java/src/org/chromium/ui/base/CursorAnchorInfoBuilder.java:45: if (builder == null) { Hmm, when will the ...
5 years, 11 months ago (2015-01-14 17:17:10 UTC) #32
yukawa
https://codereview.chromium.org/699333003/diff/320001/ui/android/java/src/org/chromium/ui/base/CursorAnchorInfoBuilder.java File ui/android/java/src/org/chromium/ui/base/CursorAnchorInfoBuilder.java (right): https://codereview.chromium.org/699333003/diff/320001/ui/android/java/src/org/chromium/ui/base/CursorAnchorInfoBuilder.java#newcode45 ui/android/java/src/org/chromium/ui/base/CursorAnchorInfoBuilder.java:45: if (builder == null) { On 2015/01/14 17:17:09, jdduke ...
5 years, 11 months ago (2015-01-14 22:21:59 UTC) #38
jdduke (slow)
I hate to give you the runaround here, but honestly I think I prefer patch ...
5 years, 11 months ago (2015-01-20 21:17:34 UTC) #39
yukawa
On 2015/01/20 21:17:34, jdduke wrote: > I hate to give you the runaround here, but ...
5 years, 11 months ago (2015-01-21 04:17:20 UTC) #40
jdduke (slow)
How hard would it be to get patchset #4 into a workable state? Ideally in ...
5 years, 11 months ago (2015-01-21 19:16:42 UTC) #41
yukawa
On 2015/01/21 19:16:42, jdduke wrote: > How hard would it be to get patchset #4 ...
5 years, 11 months ago (2015-01-22 04:14:56 UTC) #42
jdduke (slow)
On 2015/01/22 04:14:56, yukawa wrote: > On 2015/01/21 19:16:42, jdduke wrote: > > How hard ...
5 years, 11 months ago (2015-01-26 16:09:34 UTC) #43
yukawa
On 2015/01/26 16:09:34, jdduke wrote: > On 2015/01/22 04:14:56, yukawa wrote: > > On 2015/01/21 ...
5 years, 11 months ago (2015-01-27 13:37:31 UTC) #44
yukawa
On 2015/01/27 13:37:31, yukawa wrote: > On 2015/01/26 16:09:34, jdduke wrote: > > On 2015/01/22 ...
5 years, 10 months ago (2015-02-02 18:36:04 UTC) #45
jdduke (slow)
Thanks! In general, this approach looks as reasonable as any, though I still don't love ...
5 years, 10 months ago (2015-02-09 17:01:17 UTC) #46
yukawa
On 2015/02/09 17:01:17, jdduke wrote: > Thanks! > > In general, this approach looks as ...
5 years, 10 months ago (2015-02-10 17:24:54 UTC) #47
yukawa
Hi Jared, can we resume the discussion? Feel free to schedule a meeting or drop ...
5 years, 8 months ago (2015-04-14 22:57:01 UTC) #48
jdduke (slow)
On 2015/04/14 22:57:01, yukawa wrote: > Hi Jared, can we resume the discussion? Feel free ...
5 years, 8 months ago (2015-04-14 23:56:27 UTC) #49
jdduke (slow)
The last big question I have is whether we can know if the CursorAnchorInfo updates ...
5 years, 8 months ago (2015-04-15 19:38:24 UTC) #50
jdduke (slow)
On 2015/04/15 19:38:24, jdduke wrote: > The last big question I have is whether we ...
5 years, 8 months ago (2015-04-15 21:31:17 UTC) #51
yukawa
On 2015/04/15 21:31:17, jdduke wrote: > On 2015/04/15 19:38:24, jdduke wrote: > > The last ...
5 years, 8 months ago (2015-04-15 22:22:33 UTC) #52
jdduke (slow)
On 2015/04/15 22:22:33, yukawa wrote: > On 2015/04/15 21:31:17, jdduke wrote: > > On 2015/04/15 ...
5 years, 8 months ago (2015-04-15 22:29:42 UTC) #53
yukawa
5 years, 8 months ago (2015-04-15 22:44:44 UTC) #54
On 2015/04/15 22:29:42, jdduke wrote:
> On 2015/04/15 22:22:33, yukawa wrote:
> > On 2015/04/15 21:31:17, jdduke wrote:
> > > On 2015/04/15 19:38:24, jdduke wrote:
> > > > The last big question I have is whether we can know if the
> CursorAnchorInfo
> > > > updates will be worthwhile/used for a given keyboard/IME. That is, can
we
> > > > dynamically subscribe to CursorAnchorInfo updates? That could either be
> > > toggling
> > > > our |RenderWidget::UpdateCompositionInfo| subscription in the renderer,
or
> > > just
> > > > dropping the composition info updates before we pass them through the
JNI
> > > > boundary.
> > > 
> > > I should clarify that by saying, if we land this behind a flag, dynamic
> > > subscription isn't really necessary. That can come later if/when we flip
the
> > bit
> > > to enable by default.
> > 
> > Yes, it's technically possible to implement the dynamic subscription
mechanism
> > later.
> > It's only for the performance optimization.  The JavaDoc doesn't disallow
the
> > application to always send CursorAnchorInfo.
> > 
> > As for the first question, the |RenderWidget::UpdateCompositionInfo|
> > subscription
> > needs to be always enabled for now (in L/L-MR1 devices).
> > 
> > What we need to take care of is InputConnection#CURSOR_UPDATE_IMMEDIATE. 
When
> > the
> > application receives this request, the app should send back CursorAnchorInfo
> to
> > the
> > IME no matter if the CursorAnchorInfo monitor mode is disabled or enabled.
> > However, since |RenderWidget::UpdateCompositionInfo| subscription is a
> > push-style
> > notification from the renderer process to the browser process, we need to
> track
> > the
> > latest value in the browser process.  It's technically possible to add a new
> IPC
> > message that is issued from the browser process to the renderer process for
> > further optimization though.
> > 
> > What do you think?  I'm happy to split out the dynamic subscription
mechanism
> to
> > subsequent CLs.
> 
> We can add that later. Landing this behind a flag without that optimization is
> fine.

All right.  I'll do it, address your comments, and upload a screenrecord.

Powered by Google App Engine
This is Rietveld 408576698