Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1589953005:
Support InputMethodManager#updateCursorAnchorInfo for Android 5.0 (Closed)
|
Created:
4 years, 11 months ago by kinaba Modified:
4 years, 9 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, nona+watch_chromium.org, shuchen+watch_chromium.org, James Su, vmpstr+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. |
DescriptionSupport InputMethodManager#updateCursorAnchorInfo for Android 5.0
This is the 6th part of the series of CLs to enable
InputMethodManager#updateCursorAnchorInfo support for Android 5.0.
- part1 crrev.com/671503005
- part2 crrev.com/679223002
- part3 crrev.com/755853004
- part4 crrev.com/784503002
- part5 crrev.com/757233003
It enables the feature under a flag, with one possible optimization
chance (dynamic subscription) not yet implemented.
This is based on yukawa@'s original CL (http://crrev.com/699333003),
and adds accommodation with recent changes on coordinate systems
in Chromium trunk. It also adds a few comments on coordinate systems
so that the intention of the code become clearer.
This CL consists of the following 3 parts,
1. Adds CursorAnchorInfoController as the central place to control
when and how CursorAnchorInfo object should be built and sent to the IME.
2. Adds tests for CursorAnchorInfoController.
3. Modifies renderer_widget_host_view_android and ContentViewCore to
wire necessary parameters to CursorAnchorInfoController.
Design Doc: https://docs.google.com/a/chromium.org/document/d/1KdnoynGURrJBP1vDHcv2jwUdnsXHzRSxb5v4rK7UgtU/view
BUG=424866
TEST=Manually done on Nexus 6 Build/MMB29S, Nexus 4 Build/LMY48T
TEST=build/android/test_runner.py instrumentation --release \
--test-apk=ContentShellTest -f '*CursorAnchorInfoControllerTest*'
TEST=build/android/test_runner.py instrumentation --release \
--test-apk=ContentShellTest -f '*ImeLollipopTest*'
Committed: https://crrev.com/d607fd36000c470d91dd96976698e63beeb87980
Cr-Commit-Position: refs/heads/master@{#380504}
Patch Set 1 #Patch Set 2 : Rebase (yukawa@'s base CL + fix only build errors) #Patch Set 3 : Update to reflect coordinate changes. #Patch Set 4 : Fix tests, add comments #Patch Set 5 : (WIP for reflecting (#c6): removed KK compatibility layer.) #Patch Set 6 : (Partially) added ImeTest. Reset on InputConnection creation. #Patch Set 7 : Fix Findbugs warning / Simplify the state transition. #
Total comments: 33
Patch Set 8 : Address/reply to the comments. (flag is still remaining.) #Patch Set 9 : Flag removed version. #
Total comments: 1
Patch Set 10 : Removed the renderer side flagging completely. #Patch Set 11 : Comment remove. #
Total comments: 10
Patch Set 12 : Rebase over ThreadedInputConnnection CL. #
Total comments: 8
Patch Set 13 : Addressed #36. Added ImeLollipopTest for IMMEDIATE case. #
Total comments: 32
Patch Set 14 : (bare rebase) #Patch Set 15 : Addressed comments in #38 #Messages
Total messages: 48 (16 generated)
Patchset #2 (id:20001) has been deleted
Description was changed from ========== [!!!WIP!!!]: Support InputMethodManager#updateCursorAnchorInfo for Android 5.0 Just almost blindly rebased yukawa@'s https://codereview.chromium.org/699333003/ to the ToT Chromium codebase. For now it compiles and works only for fixed and un-zoomed text area. Working on a fix on coordinate scale factor changes happeded somewhere. BUG= ========== to ========== Support InputMethodManager#updateCursorAnchorInfo for Android 5.0 This is based on yukawa@'s original CL (http://crrev.com/699333003), but accommodating with recent changes on coordinates in ToT Chromium. This is the 6th part of the series of CLs to enable InputMethodManager#updateCursorAnchorInfo support for Android 5.0. - 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/1KdnoynGURrJBP1vDHcv2jwUdns... BUG=424866 TEST=Manually done on Nexus 6 Build/MMB29S ==========
Description was changed from ========== Support InputMethodManager#updateCursorAnchorInfo for Android 5.0 This is based on yukawa@'s original CL (http://crrev.com/699333003), but accommodating with recent changes on coordinates in ToT Chromium. This is the 6th part of the series of CLs to enable InputMethodManager#updateCursorAnchorInfo support for Android 5.0. - 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/1KdnoynGURrJBP1vDHcv2jwUdns... BUG=424866 TEST=Manually done on Nexus 6 Build/MMB29S ========== to ========== Support InputMethodManager#updateCursorAnchorInfo for Android 5.0 This is based on yukawa@'s original CL (http://crrev.com/699333003), but accommodating with recent changes on coordinates in ToT Chromium. This is the 6th part of the series of CLs to enable InputMethodManager#updateCursorAnchorInfo support for Android 5.0. - 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/1KdnoynGURrJBP1vDHcv2jwUdns... BUG=424866 TEST=Manually done on Nexus 6 Build/MMB29S TEST=build/android/test_runner.py gtest --release -s base_unittests \ --isolate_file_path=base/base_unittests.isolate --gtest_filter='*JniArray*' TEST=build/android/test_runner.py instrumentation --release \ --test-apk=ContentShellTest -f '*CursorAnchorInfoControllerTest*' ==========
Description was changed from ========== Support InputMethodManager#updateCursorAnchorInfo for Android 5.0 This is based on yukawa@'s original CL (http://crrev.com/699333003), but accommodating with recent changes on coordinates in ToT Chromium. This is the 6th part of the series of CLs to enable InputMethodManager#updateCursorAnchorInfo support for Android 5.0. - 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/1KdnoynGURrJBP1vDHcv2jwUdns... BUG=424866 TEST=Manually done on Nexus 6 Build/MMB29S TEST=build/android/test_runner.py gtest --release -s base_unittests \ --isolate_file_path=base/base_unittests.isolate --gtest_filter='*JniArray*' TEST=build/android/test_runner.py instrumentation --release \ --test-apk=ContentShellTest -f '*CursorAnchorInfoControllerTest*' ========== to ========== Support InputMethodManager#updateCursorAnchorInfo for Android 5.0 This is the 6th part of the series of CLs to enable InputMethodManager#updateCursorAnchorInfo support for Android 5.0. - part1 crrev.com/671503005 - part2 crrev.com/679223002 - part3 crrev.com/755853004 - part4 crrev.com/784503002 - part5 crrev.com/757233003 It enables the feature under a flag, with one possible optimization chance (dynamic subscription) not yet implemented. This is based on yukawa@'s original CL (http://crrev.com/699333003), and adds accommodation with recent changes on coordinate systems in Chromium trunk. It also adds a few comments on coordinate systems so that the intention of the code become clearer. This CL consists of following 2 auxiliary parts, 1. Adds a JNI helper for converting float arrays. (base/*) 2. Introduces --enable-cursor-anchor-info so that we can start testing this functionality behind the flag. (*flags*, *switches*, *resources*) and the following 4 main parts: 1. Adds CursorAnchorInfoController as the central place to control when and how CursorAnchorInfo object should be built and sent to the IME. 2. Adds tests for CursorAnchorInfoController. 3. Updates IME API wrappers so that we can call new IME APIs for L. 4. Modifies renderer_widget_host_view_android and ContentViewCore to wire necessary parameters to CursorAnchorInfoController. 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/1KdnoynGURrJBP1vDHcv2jwUdns... BUG=424866 TEST=Manually done on Nexus 6 Build/MMB29S TEST=build/android/test_runner.py gtest --release -s base_unittests \ --isolate_file_path=base/base_unittests.isolate --gtest_filter='*JniArray*' TEST=build/android/test_runner.py instrumentation --release \ --test-apk=ContentShellTest -f '*CursorAnchorInfoControllerTest*' ==========
changwan@chromium.org changed reviewers: + aelias@chromium.org, changwan@chromium.org
Thanks for making this change. Some questions before looking into details. 1) Do we really need the switch? Adding more switch will adds complexity since we'll have to test all the combinations of different switches. What are the risks that you expect with the change? 2) Do we really need to test against KK? If not, can we remove CursorAnchorInfoWrapper, TestCursorAnchorInfoWrapper, and a major portion of CursorAnchorInfoController? A similar case would be document mode (only applicable to L and above), and we run related tests only on L. 3) Do we need to cache cursor anchor info in CursorAnchorInfoController? How about just sending it to inputmethodmanager whenever appropriate? It seems that IMM checks equality anyways, so we don't even need to check against last sent info (for MONITOR case). Also, for IMMEDIATE case (I read it as ONCE), I wonder if it's ok to wait until we get the update from the renderer. It is async callback anyways. Since we're trying the new design ( https://codereview.chromium.org/1278593004/), I'm afraid that depending on the timing there may be slight difference in composition text of cursoranchorinfocontroller and that from getTextBeforeCursor (and/or updateSelection). Which option does Google Japanese IME use between IMMEDIATE and MONITOR? 4) Could you make lifetime of update more explicitly? For example, when we create new input connection, should we clear the MONITOR state? (We do not get focusedNodeChanged when it happens). 5) Could you add some tests to ImeTest?
kinaba@chromium.org changed reviewers: + yukawa@chromium.org
+yukawa: I delegated one question to you. Thanks for the comments! On 2016/02/02 09:07:48, Changwan Ryu wrote: > 1) Do we really need the switch? The concern is performance, (crbug.com/427090, comments from crrev.com/671503005, crrev.com/699333003). If it (especially the renderer->browser IPC for UpdateCompositionInfo) has bad impact, we need further optimization, like sending the IPC only when IME called requestCursorAnchorInfo. > 2) Do we really need to test against KK? I have no preference personally. I'll try shrinking down the CL to run tests only on L. > 3) Do we need to cache cursor anchor info in CursorAnchorInfoController? > > Also, for IMMEDIATE case (I read it as ONCE), I wonder if it's ok to wait until > we get the update from the renderer. Caching is for handling IMMEDIATE. It is SEND_ONCE_EVEN_IF_THERE_IS_NO_UPDATE, rather than just ONCE. Waiting for the next update does not confirm the spec. Adding a browser->renderer IPC for requesting one-shot update is another option for implementing IMMEDIATE, but I think it'll be more complicated. > Which option does Google Japanese IME use between IMMEDIATE and MONITOR? Both (IMMEDIATE | MONITOR). > 4) Could you make lifetime of update more explicitly? > For example, when we create new input connection, should we clear the MONITOR > state? (We do not get focusedNodeChanged when it happens). Good point. My understanding of the API spec is that it never clears the state except the explicit call to onRequestCursorUpdates, but I'd like to defer the question to yukawa@ for clarification. Either way I'd like to make it more explicit in the code. > 5) Could you add some tests to ImeTest? Will take a look.
On 2016/02/03 05:57:25, kinaba wrote: > > Which option does Google Japanese IME use between IMMEDIATE and MONITOR? > > Both (IMMEDIATE | MONITOR). Correct. Just to clarify, in most of cases, I'd recommend IME developers relying on MONITOR mode but in some situations only IMMEDIATE can help. For example, 1. Launch Chrome. 2. The user taps a text field. The current IME X uses floating-UI that does not cause re-layout in Chrome. 3. The user switches to another IME Y, and it also X uses floating-UI that does not cause re-layout in Chrome. In this situations, the IME Y may want to know what is the latest CursorAnchorInfo, but without one-shot request, the IME Y cannot know the current cursor coordinates until re-layout happens in Chrome side. There could be similar scenarios when turning on/off display, device rotation, and so on. Note also that we can specify CURSOR_UPDATE_IMMEDIATE and CURSOR_UPDATE_MONITOR at the same time, so most likely the most convenient way would be to specify both of them when staring monitoring mode. > > 4) Could you make lifetime of update more explicitly? > > For example, when we create new input connection, should we clear the MONITOR > > state? (We do not get focusedNodeChanged when it happens). > > Good point. My understanding of the API spec is that it never clears the state > except the explicit call to onRequestCursorUpdates, > but I'd like to defer the question to yukawa@ for clarification. > > Either way I'd like to make it more explicit in the code. Sorry for the unclear documents. It's OK to assume that this state is per-InputConnection. Yes, you can clear the MONITOR bit when you restart input.
Thanks for the inputs. Patch Set 6 includes the following changes: - Limited the tests to run only on Android L, and reduced wrappers a lot. - Added reset on monitoring state on InputConnection creation. - Started adding ImeTest (I'll add more in subsequent patchset, but uploading the current one for rolling the review iteration earlier.) - Rebased on top of ToT. - Found a bug when used together with --use-zoom-for-dsf (and reported: Bug 585043.)
The approach looks fine at a high level, below are mostly just comments on implementation details. Thanks for working on this! https://codereview.chromium.org/1589953005/diff/140001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1589953005/diff/140001/chrome/app/generated_r... chrome/app/generated_resources.grd:5322: + <message name="IDS_FLAGS_ENABLE_CURSOR_ANCHOR_INFO_NAME" desc="Name of the flag to enable the CursorAnchorInfo API for Android 5.0 and later."> Is a flag really needed? This seems relatively low risk to me as it's an "additive" feature without too many complex interactions, I'd be OK personally with landing and enabling immediately. https://codereview.chromium.org/1589953005/diff/140001/content/browser/androi... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/1589953005/diff/140001/content/browser/androi... content/browser/android/content_view_core_impl.h:312: // Selection bounds are in DIP coordinates. This comment may cause confusion because it makes selection bounds sound like in a different coordinate space from the rest, when they're not. So I'd prefer it wasn't added. DIP and CSS pixels are basically the same thing. Even zoom-for-dsf mode will not change that as I understand it, and there are no medium-term plans to change Android to zoom-for-dsf new mode anyway. https://codereview.chromium.org/1589953005/diff/140001/content/browser/render... File content/browser/renderer_host/ime_adapter_android.h (right): https://codereview.chromium.org/1589953005/diff/140001/content/browser/render... content/browser/renderer_host/ime_adapter_android.h:85: void SetCharacterBounds(const std::vector<gfx::Rect>& rects); Looks like the bounds are in floats in the Java layer, so you should also be using gfx::RectF in C++, not gfx::Rect. https://codereview.chromium.org/1589953005/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java (right): https://codereview.chromium.org/1589953005/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:1: // Copyright 2014 The Chromium Authors. All rights reserved. Copyright 2016 https://codereview.chromium.org/1589953005/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:47: private CharSequence mText; Let's not store the full text here. We're planning to move away from the assumption that the browser will hold the entire contents of the textbox (it causes performance problems with very large contenteditable elements). Please only store the subsequence up front (and pass only the subsequence in the API of the class as well.), or possibly just have a call out to get the subsequence from IMEAdapter. https://codereview.chromium.org/1589953005/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:57: private float mScale; A lot of this is copies of state held by the RenderCoordinates and IMEAdapter. In general, I prefer to avoid persisting multiple copies of state as we have to remember to keep them up to date, and there's less clarity about what is expected to be identical value to what. Could you call back out to RenderCoordinates and ImeAdapter to get most of the information you need? You could only store unique information here (possibly insertion markers, although even that seems possibly redundant with elsewhere). https://codereview.chromium.org/1589953005/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:70: private volatile CursorAnchorInfo mLastCursorAnchorInfo; Why "volatile"? This is another scary multithreading keyword like "synchronized" that doesn't appear to have any effect here. https://codereview.chromium.org/1589953005/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:82: private static final boolean sIsSupported = isSupportedInit(); Checking a command-line flags at static initialization time seems sketchy, I would need to look up the semantics but I don't have 100% confidence that the flag would always be updated to the final value at this time. It seems like a premature optimization so please just get rid of the static variable and check the version code and command-line flag every time. The command-line flag is due for deletion now or at some point later anyway. https://codereview.chromium.org/1589953005/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:180: public synchronized void setCompositionCharacterBounds(float[] compositionCharacterBounds) { Why "synchronized"? Don't we just have a single UI thread doing everything? Is there some multithread stuff going on somewhere I don't see? Please remove all the "synchronized" from this patch if it's just a misunderstanding of what that keyword does in Java (but if there's an actual reason, then this probably calls for some design discussion). https://codereview.chromium.org/1589953005/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:219: || Math.abs(scale - mScale) > 1e-5 What's the point of these epsilon comparisons? Does it solve any known problem? If it's just a premature optimization, I'd rather we delete these. https://codereview.chromium.org/1589953005/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:258: mSelectionStart = -1; Almost all the nulling here except "mHasCoordinateInfo = false;" doesn't have any functional effect, so I'd prefer they were deleted. (It becomes moot if you don't store copies of the state in this class here as I suggested anyway.) https://codereview.chromium.org/1589953005/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:281: // Does nothing when at least one unknown bit flag is set. Is this some kind of API future-proofing plan? It seems more like anti-future-proofing: a new kind of bit is guaranteed to break existing behavior on the other bits, even if they are orthogonal. So I think this 'return false' should probably be removed. https://codereview.chromium.org/1589953005/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/1589953005/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:178: mCursorAnchorInfoController.resetMonitoringState(); Anything else needs to be reset here? How about mHasPendingImmediateRequest for example? https://codereview.chromium.org/1589953005/diff/140001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1589953005/diff/140001/content/renderer/rende... content/renderer/render_widget.cc:1753: base::android::BuildInfo::GetInstance()->sdk_int() >= 21 && We shouldn't add an SDK version level check in the renderer. I'd prefer we just sent up the message unconditionally, even if K and below won't use it for anything.
Thanks for the comments!! Sorry my belated response (I was on a holidays last Thu-Fri and today I'm a sheriff on ARC team and its tree is on fire ;(.) I hope I'll get back here tomorrow and address your points. But before that, I want to clarify two thing. On DIP and CSS pixels: I borrowed the terms from RenderCoordinates.java https://code.google.com/p/chromium/codesearch#chromium/src/content/public/and... where CSS*mPageScaleFactor==DIP (and DIP*mDeviceScaleFactor==PHYSICAL) Am I missing anything? On the threading concern: I guess this is due to my total unfamiliality of Android Chromium's threading model. My understanding was: - onRequestCursorUpdates comes from some arbitrary binder thread (where InputConnection methods is called on.) - All other calls are from IO thread (where Render->Browser IPC happens.) Hence I thought we need to care multithreading. Is it wrong and we can safely assume they are all on a single thread? (Disclaimer: I noticed the current implementation is not at all good enough even regarding this point and hence I anyway need a rewrite, but I'd like to clarify the thing.)
On 2016/02/15 07:59:16, kinaba wrote: > My understanding was: > - onRequestCursorUpdates comes from some arbitrary binder thread (where > InputConnection methods is called on.) Just FYI, we can assume that InputConnection#onRequestCursorUpdates is dispatched on view.getHandler().getLooper(), where |view| is the View that is used to create the InputConnection with View#onCreateInputConnection(). (There is no CTS to guarantee it though) One tricky point here is that InputConnection is not used for Binder calls. There are @hide interfaces named IInputContext and IInputContextCallback, and an internal proxy object that keeps receiving incoming Binder calls from the IME to dispatch them on the UI-thread (to be precise, it's |view.getHandler().getLooper()| mentioned above) https://android.googlesource.com/platform/frameworks/base/+/android-6.0.1_r16... https://android.googlesource.com/platform/frameworks/base/+/android-6.0.1_r16... https://android.googlesource.com/platform/frameworks/base/+/android-6.0.1_r16... https://android.googlesource.com/platform/frameworks/base/+/android-6.0.1_r16...
On 2016/02/15 08:26:00, yukawa wrote: > internal proxy object that keeps receiving incoming Binder calls from the IME to > dispatch them on the UI-thread Ah, thanks! That's awesome. So my remaining concern is whether the other methods (originating from the renderer) is on which thread.
On 2016/02/15 10:45:52, kinaba wrote: > On 2016/02/15 08:26:00, yukawa wrote: > > internal proxy object that keeps receiving incoming Binder calls from the IME > to > > dispatch them on the UI-thread > > Ah, thanks! That's awesome. So my remaining concern is whether the other methods > (originating from the renderer) is on which thread. Sorry that I'm asking this again (and chiming in late), but I didn't clearly state my question... (I understand that IMMEDIATE should not wait for the next update because it should be sent when there is no change.) How quick should editor respond to ONCE or MONITOR request? IMMEDIATE sounds scary because it sounds as if there is time constraint, but if it simply means ONCE could browser simply ask renderer to turn on/off or send one update? (MONITOR implementation should not worry about time constraint once it's turned on, right?) That way you don't need to cache anything and browser side change could be minimized. I'm also asking this because you're worried about IPC spamming / performance, so I'm curious if that alternative implementation would soothe the problem (at least when that feature isn't being used or turned off). Is the alternative solution really laggy? Once again, I'm sorry for the late reply. Was quite busy with my main work.
> On the threading concern: OK, that's totally due to my misunderstanding. I read the RenderProcessHost code and also did some logging and am convinced that everything is on UI thread. I'll just drop the synchonization code. Thanks for chiming! > could browser simply ask renderer to turn on/off or send one update? Yes, that's another direction to implement the feature in satisfying way. (and this is what I wanted to mean in my first reply :) "Adding a browser->renderer IPC for requesting one-shot update".) My only concern is its complexity, not the roundtrip lag. We need to add a new IPC, a state control logic in the renderer, and maybe a new route to re-send frame metadata (since currently it is fed to ContentViewCore as a side-load of updating the compositor frame itself, which I guess we don't want to re-send.) I expect that replacing the cache variables in CursorAnchorInfoController by a query to RenderCoordinates and ImeAdapter, as suggested by aelias in #11, might address your concern. Let me try (tomorrow; sorry about my delay).
Patchset #8 (id:160001) has been deleted
On 2016/02/16 08:53:22, kinaba wrote: > > could browser simply ask renderer to turn on/off or send one update? > > Yes, that's another direction to implement the feature in satisfying way. > (and this is what I wanted to mean in my first reply :) "Adding a > browser->renderer IPC for requesting one-shot update".) > > My only concern is its complexity, not the roundtrip lag. We need to add a new > IPC, > a state control logic in the renderer, and maybe a new route to re-send frame > metadata > (since currently it is fed to ContentViewCore as a side-load of updating the > compositor > frame itself, which I guess we don't want to re-send.) Yeah probably it's technically possible to do such an optimization. Actually we used to have a similar mechanism for desktop IME support, which caused some tricky bugs and we finally ended up removing it in Issue 296509 though. Just FYI, the current implementation of InputConnection#CURSOR_UPDATE_IMMEDIATE in built-in EditText internally relies on View#requestLayout(). https://android.googlesource.com/platform/frameworks/base/+/d8636ea7ca78df83d... It's a bit suboptimal in terms of efficiency and timeliness, especially for IMEs that unnecessary keeps requesting CURSOR_UPDATE_IMMEDIATE, but I think it's OK to focus on the reliability and simplicity rather than efficiency and timeliness as for CURSOR_UPDATE_IMMEDIATE support, especially in the initial implementation.
https://codereview.chromium.org/1589953005/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java (right): https://codereview.chromium.org/1589953005/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:57: private float mScale; On 2016/02/10 08:23:41, aelias wrote: > A lot of this is copies of state held by the RenderCoordinates and IMEAdapter. > In general, I prefer to avoid persisting multiple copies of state as we have to > remember to keep them up to date, and there's less clarity about what is > expected to be identical value to what. > > Could you call back out to RenderCoordinates and ImeAdapter to get most of the > information you need? You could only store unique information here (possibly > insertion markers, although even that seems possibly redundant with elsewhere). Hmm... Just curious, what happens when marker positions and selection / composition are not in complete sync? kinaba@, could we rely on IME apps to resolve conflict if any? Failing that, we may need to do either 1) keep CursorAnchorInfo related data separately from last state update from ImeAdapter 2) or guarantee / test that the two set of data are same when sending out the update.
Patchset #8 (id:180001) has been deleted
Description was changed from ========== Support InputMethodManager#updateCursorAnchorInfo for Android 5.0 This is the 6th part of the series of CLs to enable InputMethodManager#updateCursorAnchorInfo support for Android 5.0. - part1 crrev.com/671503005 - part2 crrev.com/679223002 - part3 crrev.com/755853004 - part4 crrev.com/784503002 - part5 crrev.com/757233003 It enables the feature under a flag, with one possible optimization chance (dynamic subscription) not yet implemented. This is based on yukawa@'s original CL (http://crrev.com/699333003), and adds accommodation with recent changes on coordinate systems in Chromium trunk. It also adds a few comments on coordinate systems so that the intention of the code become clearer. This CL consists of following 2 auxiliary parts, 1. Adds a JNI helper for converting float arrays. (base/*) 2. Introduces --enable-cursor-anchor-info so that we can start testing this functionality behind the flag. (*flags*, *switches*, *resources*) and the following 4 main parts: 1. Adds CursorAnchorInfoController as the central place to control when and how CursorAnchorInfo object should be built and sent to the IME. 2. Adds tests for CursorAnchorInfoController. 3. Updates IME API wrappers so that we can call new IME APIs for L. 4. Modifies renderer_widget_host_view_android and ContentViewCore to wire necessary parameters to CursorAnchorInfoController. 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/1KdnoynGURrJBP1vDHcv2jwUdns... BUG=424866 TEST=Manually done on Nexus 6 Build/MMB29S TEST=build/android/test_runner.py gtest --release -s base_unittests \ --isolate_file_path=base/base_unittests.isolate --gtest_filter='*JniArray*' TEST=build/android/test_runner.py instrumentation --release \ --test-apk=ContentShellTest -f '*CursorAnchorInfoControllerTest*' ========== to ========== Support InputMethodManager#updateCursorAnchorInfo for Android 5.0 This is the 6th part of the series of CLs to enable InputMethodManager#updateCursorAnchorInfo support for Android 5.0. - part1 crrev.com/671503005 - part2 crrev.com/679223002 - part3 crrev.com/755853004 - part4 crrev.com/784503002 - part5 crrev.com/757233003 It enables the feature under a flag, with one possible optimization chance (dynamic subscription) not yet implemented. This is based on yukawa@'s original CL (http://crrev.com/699333003), and adds accommodation with recent changes on coordinate systems in Chromium trunk. It also adds a few comments on coordinate systems so that the intention of the code become clearer. This CL consists of the following 3 parts, 1. Adds CursorAnchorInfoController as the central place to control when and how CursorAnchorInfo object should be built and sent to the IME. 2. Adds tests for CursorAnchorInfoController. 3. Modifies renderer_widget_host_view_android and ContentViewCore to wire necessary parameters to CursorAnchorInfoController. 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/1KdnoynGURrJBP1vDHcv2jwUdns... BUG=424866 TEST=Manually done on Nexus 6 Build/MMB29S, Nexus 4 Build/LMY48T TEST=build/android/test_runner.py instrumentation --release \ --test-apk=ContentShellTest -f '*CursorAnchorInfoControllerTest*' TEST=build/android/test_runner.py instrumentation --release \ --test-apk=ContentShellTest -f '*ImeTest*' ==========
https://codereview.chromium.org/1589953005/diff/140001/content/browser/render... File content/browser/renderer_host/ime_adapter_android.h (right): https://codereview.chromium.org/1589953005/diff/140001/content/browser/render... content/browser/renderer_host/ime_adapter_android.h:85: void SetCharacterBounds(const std::vector<gfx::Rect>& rects); On 2016/02/10 08:23:41, aelias_OOO_until_Feb_23 wrote: > Looks like the bounds are in floats in the Java layer, so you should also be > using gfx::RectF in C++, not gfx::Rect. Done. https://codereview.chromium.org/1589953005/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java (right): https://codereview.chromium.org/1589953005/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/02/10 08:23:42, aelias_OOO_until_Feb_23 wrote: > Copyright 2016 Done. https://codereview.chromium.org/1589953005/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:47: private CharSequence mText; On 2016/02/10 08:23:42, aelias_OOO_until_Feb_23 wrote: > Let's not store the full text here. We're planning to move away from the > assumption that the browser will hold the entire contents of the textbox (it > causes performance problems with very large contenteditable elements). > > Please only store the subsequence up front (and pass only the subsequence in the > API of the class as well.), or possibly just have a call out to get the > subsequence from IMEAdapter. Done. This is retrieved from IMEAdapter. https://codereview.chromium.org/1589953005/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:57: private float mScale; On 2016/02/10 08:23:41, aelias_OOO_until_Feb_23 wrote: > A lot of this is copies of state held by the RenderCoordinates and IMEAdapter. > In general, I prefer to avoid persisting multiple copies of state as we have to > remember to keep them up to date, and there's less clarity about what is > expected to be identical value to what. > > Could you call back out to RenderCoordinates and ImeAdapter to get most of the > information you need? You could only store unique information here (possibly > insertion markers, although even that seems possibly redundant with elsewhere). Removed the duplicates with IMEAdapter. For RenderCoordinates, I want to take a diff with previous values so that unnecessary events are not sent to IMEs. So I think the values need to be stored here. https://codereview.chromium.org/1589953005/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:57: private float mScale; On 2016/02/17 00:21:05, Changwan Ryu wrote: > Hmm... Just curious, what happens when marker positions and selection / > composition are not in complete sync? They some floating UIs IME render may not be placed in the right (relative) position. But, the lag should be resolved shortly afterwards once all the info arrive. Since the data are coming from different sources (compositors and the main renderer thread) that are running independently by design, I'm not sure if is practical to aim the complete sync. https://codereview.chromium.org/1589953005/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:70: private volatile CursorAnchorInfo mLastCursorAnchorInfo; On 2016/02/10 08:23:42, aelias_OOO_until_Feb_23 wrote: > Why "volatile"? This is another scary multithreading keyword like > "synchronized" that doesn't appear to have any effect here. Removed. https://codereview.chromium.org/1589953005/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:82: private static final boolean sIsSupported = isSupportedInit(); On 2016/02/10 08:23:41, aelias_OOO_until_Feb_23 wrote: > Checking a command-line flags at static initialization time seems sketchy, I > would need to look up the semantics but I don't have 100% confidence that the > flag would always be updated to the final value at this time. It seems like a > premature optimization so please just get rid of the static variable and check > the version code and command-line flag every time. The command-line flag is due > for deletion now or at some point later anyway. Done. https://codereview.chromium.org/1589953005/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:180: public synchronized void setCompositionCharacterBounds(float[] compositionCharacterBounds) { On 2016/02/10 08:23:42, aelias_OOO_until_Feb_23 wrote: > Why "synchronized"? Don't we just have a single UI thread doing everything? Is > there some multithread stuff going on somewhere I don't see? > > Please remove all the "synchronized" from this patch if it's just a > misunderstanding of what that keyword does in Java (but if there's an actual > reason, then this probably calls for some design discussion). Done. (As I wrote in the previous comment, I thought this class may be called from different thread, but I learned that it is not the case at all. Thanks!) https://codereview.chromium.org/1589953005/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:219: || Math.abs(scale - mScale) > 1e-5 On 2016/02/10 08:23:42, aelias_OOO_until_Feb_23 wrote: > What's the point of these epsilon comparisons? Does it solve any known problem? > If it's just a premature optimization, I'd rather we delete these. The sole reason is to shut up FindBugs checker running on bots. FindBugs didn't like Floating point == comparisons. https://codereview.chromium.org/1589953005/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:258: mSelectionStart = -1; On 2016/02/10 08:23:42, aelias_OOO_until_Feb_23 wrote: > Almost all the nulling here except "mHasCoordinateInfo = false;" doesn't have > any functional effect, so I'd prefer they were deleted. (It becomes moot if you > don't store copies of the state in this class here as I suggested anyway.) Done. https://codereview.chromium.org/1589953005/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:281: // Does nothing when at least one unknown bit flag is set. On 2016/02/10 08:23:41, aelias_OOO_until_Feb_23 wrote: > Is this some kind of API future-proofing plan? It seems more like > anti-future-proofing: a new kind of bit is guaranteed to break existing behavior > on the other bits, even if they are orthogonal. So I think this 'return false' > should probably be removed. Done. https://codereview.chromium.org/1589953005/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/1589953005/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:178: mCursorAnchorInfoController.resetMonitoringState(); On 2016/02/10 08:23:42, aelias_OOO_until_Feb_23 wrote: > Anything else needs to be reset here? How about mHasPendingImmediateRequest for > example? Done (mHasPendingImmediateRequest will also be reset.)
kinaba@chromium.org changed reviewers: + jdduke@chromium.org
Inviting +jdduke as a reviewer. Could you kindly take a look? I took over the old CL by yukawa@ (crrev.com/699333003) succeeding his work. My CL mostly follows the implementation of the previous CL, but rebased onto the latest codebase. Concerns were raised on releasing this feature first under the flag, mainly due to maitaining and testing cost for combination of flags. So I'm a bit inclined to go forward directly, without any flag. The question is, what will be the best way to (sort of) prove that there's no performance issue on the current approach? Is there any standard benchmark? I tried my local build on Nexus 4 and personally I don't see any slowness, but I doubt if that is enough. https://codereview.chromium.org/1589953005/diff/140001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1589953005/diff/140001/content/renderer/rende... content/renderer/render_widget.cc:1753: base::android::BuildInfo::GetInstance()->sdk_int() >= 21 && On 2016/02/10 08:23:42, aelias_OOO_until_Feb_23 wrote: > We shouldn't add an SDK version level check in the renderer. I'd prefer we just > sent up the message unconditionally, even if K and below won't use it for > anything. I guess devices stuck at K or below tends to be older and slower one hence I think this is an optimization worth doing. (Originally the information was always sent, but later this #ifdef(OS_ANDROID) was added for avoiding the unnecessary flood of IPCs. I'd rather respect the history.) WDYT?
On 2016/02/19 12:44:08, kinaba wrote: > Inviting +jdduke as a reviewer. Could you kindly take a look? > I took over the old CL by yukawa@ (crrev.com/699333003) succeeding his work. > My CL mostly follows the implementation of the previous CL, but rebased onto the > latest codebase. I'm no longer actively working on Chromium, so I'll defer to changwan and aelias for further review. https://codereview.chromium.org/1589953005/diff/140001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1589953005/diff/140001/content/renderer/rende... content/renderer/render_widget.cc:1753: base::android::BuildInfo::GetInstance()->sdk_int() >= 21 && On 2016/02/19 12:44:08, kinaba wrote: > On 2016/02/10 08:23:42, aelias_OOO_until_Feb_23 wrote: > > We shouldn't add an SDK version level check in the renderer. I'd prefer we > just > > sent up the message unconditionally, even if K and below won't use it for > > anything. > > I guess devices stuck at K or below tends to be older and slower one hence I > think > this is an optimization worth doing. (Originally the information was always > sent, > but later this #ifdef(OS_ANDROID) was added for avoiding the unnecessary flood > of IPCs. > I'd rather respect the history.) WDYT? Seems reasonable. I suppose you could gait the flag itself on an SDK version check in the browser, not sure if that's a cleaner approach though.
https://codereview.chromium.org/1589953005/diff/140001/content/browser/androi... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/1589953005/diff/140001/content/browser/androi... content/browser/android/content_view_core_impl.h:312: // Selection bounds are in DIP coordinates. > On DIP and CSS pixels: > I borrowed the terms from RenderCoordinates.java > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/and... where CSS*mPageScaleFactor==DIP (and DIP*mDeviceScaleFactor==PHYSICAL) > Am I missing anything? Well, not exactly, I guess I forgot that terminology distinction (which probably I wrote myself years ago). Anyway, my main point is that I don't want us to add a special comment referring to selection bounds when it's in the same space as everything else. https://codereview.chromium.org/1589953005/diff/140001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1589953005/diff/140001/content/renderer/rende... content/renderer/render_widget.cc:1753: base::android::BuildInfo::GetInstance()->sdk_int() >= 21 && On 2016/02/19 at 16:28:07, jdduke (slow) wrote: > On 2016/02/19 12:44:08, kinaba wrote: > > On 2016/02/10 08:23:42, aelias_OOO_until_Feb_23 wrote: > > > We shouldn't add an SDK version level check in the renderer. I'd prefer we > > just > > > sent up the message unconditionally, even if K and below won't use it for > > > anything. > > > > I guess devices stuck at K or below tends to be older and slower one hence I > > think > > this is an optimization worth doing. (Originally the information was always > > sent, > > but later this #ifdef(OS_ANDROID) was added for avoiding the unnecessary flood > > of IPCs. > > I'd rather respect the history.) WDYT? Regardless of the history, I stand by what I said in my original comment. Forking this behavior based on Android version is unnecessary complexity. <K devices are also in the same ballpark of performance as L+ ones so there's no reason to give them special optimizations, we don't do that anywhere else. If there's actually a performance problem here, then the solution would look something like merging this into another of our IPCs like UpdateTextInputState, which would benefit all Android versions, not to just exclude some subset of devices while paying the cost others. But personally I'm just not worried about the performance of this. You don't need to go out of your way to test performance of this patch as far as I'm concerned, simply blinding landing and waiting to see if any perf sheriff complains is good enough for me. User typing rate is pretty slow (like 1 character every 100ms at most), and there are already heavy costs like raster associated with it, so the cost of a single IPC per character (in the 100s of microseconds cost) should really be lost in the noise here.
https://codereview.chromium.org/1589953005/diff/220001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1589953005/diff/220001/content/renderer/rende... content/renderer/render_widget.cc:1750: void RenderWidget::UpdateCompositionInfo(bool should_update_range) { The problem is, this method isn't just called when the compositon is updated. It appears to be called at the start of each compositor frame (see https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... and https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...). It's possible there are some clever early-out optimizations here and there, but as it stands this is called way too frequently for low-end devices (perhaps even high-end devices). We might need a similar compositor thread optimization as we have now for the composited selection bounds updated.
On 2016/02/24 at 04:00:45, jdduke wrote: > https://codereview.chromium.org/1589953005/diff/220001/content/renderer/rende... > File content/renderer/render_widget.cc (right): > > https://codereview.chromium.org/1589953005/diff/220001/content/renderer/rende... > content/renderer/render_widget.cc:1750: void RenderWidget::UpdateCompositionInfo(bool should_update_range) { > The problem is, this method isn't just called when the compositon is updated. It appears to be called at the start of each compositor frame (see https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... and https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...). > > It's possible there are some clever early-out optimizations here and there, but as it stands this is called way too frequently for low-end devices (perhaps even high-end devices). We might need a similar compositor thread optimization as we have now for the composited selection bounds updated. OK, thanks for pointing out it's called every frame, I hadn't noticed. That said it does look to me like the ShouldUpdateCompositionInfo() early out is enough to avoid the IPC being sent in the 99.9% case of there being no active composition, and also it will not be sent if there is an active composition and something else random is animating. Compositions are really, really transient, they aren't nearly as persistent as selections. It is unusual for the user to be scrolling during an ongoing composition, which is the only time this incurs a cost. Pushing this info up into the CC layer and CompositorFrameMetadata like selection handles does sound like an appropriate thing for this in principle. But since this feature can never achieve perfect synchronization during scrolling due to the IPC hop from browser process to IME process anyway, I'm not sure it's really worth the effort.
On 2016/02/24 04:18:49, aelias wrote: > On 2016/02/24 at 04:00:45, jdduke wrote: > > > https://codereview.chromium.org/1589953005/diff/220001/content/renderer/rende... > > File content/renderer/render_widget.cc (right): > > > > > https://codereview.chromium.org/1589953005/diff/220001/content/renderer/rende... > > content/renderer/render_widget.cc:1750: void > RenderWidget::UpdateCompositionInfo(bool should_update_range) { > > The problem is, this method isn't just called when the compositon is updated. > It appears to be called at the start of each compositor frame (see > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... > and > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...). > > > > It's possible there are some clever early-out optimizations here and there, > but as it stands this is called way too frequently for low-end devices (perhaps > even high-end devices). We might need a similar compositor thread optimization > as we have now for the composited selection bounds updated. > > OK, thanks for pointing out it's called every frame, I hadn't noticed. That > said it does look to me like the ShouldUpdateCompositionInfo() early out is > enough to avoid the IPC being sent in the 99.9% case of there being no active > composition, and also it will not be sent if there is an active composition and > something else random is animating. Compositions are really, really transient, > they aren't nearly as persistent as selections. It is unusual for the user to > be scrolling during an ongoing composition, which is the only time this incurs a > cost. > > Pushing this info up into the CC layer and CompositorFrameMetadata like > selection handles does sound like an appropriate thing for this in principle. > But since this feature can never achieve perfect synchronization during > scrolling due to the IPC hop from browser process to IME process anyway, I'm not > sure it's really worth the effort. Removed the early-out by by Android version from the renderer. Indeed, the only case it can matter is scrolling while composition is active, and yes I agree it is unusual.
https://codereview.chromium.org/1589953005/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java (right): https://codereview.chromium.org/1589953005/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:219: || Math.abs(scale - mScale) > 1e-5 On 2016/02/19 at 12:28:29, kinaba wrote: > On 2016/02/10 08:23:42, aelias_OOO_until_Feb_23 wrote: > > What's the point of these epsilon comparisons? Does it solve any known problem? > > If it's just a premature optimization, I'd rather we delete these. > > The sole reason is to shut up FindBugs checker running on bots. > FindBugs didn't like Floating point == comparisons. I strongly disagree with findbugs on best practices here, I think exact equality testing of cached floats is totally legit, and the epsilon-using code is ugly and misleading. Please either use @SuppressFBWarnings("FE_FLOATING_POINT_EQUALITY") or rephrase in terms of "!=" (which I think findbugs doesn't warn about -- which is silly, but anyway). https://codereview.chromium.org/1589953005/diff/260001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java (right): https://codereview.chromium.org/1589953005/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:167: mViewDelegate.getLocationOnScreen(view, mViewOrigin); You've tested that this gets the right position for WebView, right? https://codereview.chromium.org/1589953005/diff/260001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/1589953005/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:112: int mLastSyntheticKeyCode; Looks like you accidentally reintroduced this deleted line of code during conflict resolution. https://codereview.chromium.org/1589953005/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:135: new CursorAnchorInfoController.ComposingTextDelegate() { It's a bit odd to be new'ing this object just to immediately abandon it on K and below. Could you pull out the isSupported() branch to be in this method instead, and just never call create() if it's not? https://codereview.chromium.org/1589953005/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:150: return BaseInputConnection.getComposingSpanStart(mEditable); The refactoring https://codereview.chromium.org/1278593004/ just landed and you'll need to rebaseline on it. The main substantive conflict is that you can't do this anymore, as the new approach doesn't use Editable nor BaseInputConnection. The information you need still passes through ImeAdapter.updateState() though. https://codereview.chromium.org/1589953005/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:683: mTextInputType = 0; Another deleted line accidentally reintroduced?
Description was changed from ========== Support InputMethodManager#updateCursorAnchorInfo for Android 5.0 This is the 6th part of the series of CLs to enable InputMethodManager#updateCursorAnchorInfo support for Android 5.0. - part1 crrev.com/671503005 - part2 crrev.com/679223002 - part3 crrev.com/755853004 - part4 crrev.com/784503002 - part5 crrev.com/757233003 It enables the feature under a flag, with one possible optimization chance (dynamic subscription) not yet implemented. This is based on yukawa@'s original CL (http://crrev.com/699333003), and adds accommodation with recent changes on coordinate systems in Chromium trunk. It also adds a few comments on coordinate systems so that the intention of the code become clearer. This CL consists of the following 3 parts, 1. Adds CursorAnchorInfoController as the central place to control when and how CursorAnchorInfo object should be built and sent to the IME. 2. Adds tests for CursorAnchorInfoController. 3. Modifies renderer_widget_host_view_android and ContentViewCore to wire necessary parameters to CursorAnchorInfoController. 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/1KdnoynGURrJBP1vDHcv2jwUdns... BUG=424866 TEST=Manually done on Nexus 6 Build/MMB29S, Nexus 4 Build/LMY48T TEST=build/android/test_runner.py instrumentation --release \ --test-apk=ContentShellTest -f '*CursorAnchorInfoControllerTest*' TEST=build/android/test_runner.py instrumentation --release \ --test-apk=ContentShellTest -f '*ImeTest*' ========== to ========== Support InputMethodManager#updateCursorAnchorInfo for Android 5.0 This is the 6th part of the series of CLs to enable InputMethodManager#updateCursorAnchorInfo support for Android 5.0. - part1 crrev.com/671503005 - part2 crrev.com/679223002 - part3 crrev.com/755853004 - part4 crrev.com/784503002 - part5 crrev.com/757233003 It enables the feature under a flag, with one possible optimization chance (dynamic subscription) not yet implemented. This is based on yukawa@'s original CL (http://crrev.com/699333003), and adds accommodation with recent changes on coordinate systems in Chromium trunk. It also adds a few comments on coordinate systems so that the intention of the code become clearer. This CL consists of the following 3 parts, 1. Adds CursorAnchorInfoController as the central place to control when and how CursorAnchorInfo object should be built and sent to the IME. 2. Adds tests for CursorAnchorInfoController. 3. Modifies renderer_widget_host_view_android and ContentViewCore to wire necessary parameters to CursorAnchorInfoController. Design Doc: https://docs.google.com/a/chromium.org/document/d/1KdnoynGURrJBP1vDHcv2jwUdns... BUG=424866 TEST=Manually done on Nexus 6 Build/MMB29S, Nexus 4 Build/LMY48T TEST=build/android/test_runner.py instrumentation --release \ --test-apk=ContentShellTest -f '*CursorAnchorInfoControllerTest*' TEST=build/android/test_runner.py instrumentation --release \ --test-apk=ContentShellTest -f '*ImeLollipopTest*' ==========
https://codereview.chromium.org/1589953005/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java (right): https://codereview.chromium.org/1589953005/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:219: || Math.abs(scale - mScale) > 1e-5 On 2016/02/26 08:25:32, aelias wrote: > On 2016/02/19 at 12:28:29, kinaba wrote: > > On 2016/02/10 08:23:42, aelias_OOO_until_Feb_23 wrote: > > > What's the point of these epsilon comparisons? Does it solve any known > problem? > > > If it's just a premature optimization, I'd rather we delete these. > > > > The sole reason is to shut up FindBugs checker running on bots. > > FindBugs didn't like Floating point == comparisons. > > I strongly disagree with findbugs on best practices here, I think exact equality > testing of cached floats is totally legit, and the epsilon-using code is ugly > and misleading. Please either use > @SuppressFBWarnings("FE_FLOATING_POINT_EQUALITY") or rephrase in terms of "!=" > (which I think findbugs doesn't warn about -- which is silly, but anyway). Done. https://codereview.chromium.org/1589953005/diff/260001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java (right): https://codereview.chromium.org/1589953005/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:167: mViewDelegate.getLocationOnScreen(view, mViewOrigin); On 2016/02/26 08:25:33, aelias wrote: > You've tested that this gets the right position for WebView, right? At least in WebView shell (android_webview_apk), yes. https://codereview.chromium.org/1589953005/diff/260001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/1589953005/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:112: int mLastSyntheticKeyCode; On 2016/02/26 08:25:33, aelias wrote: > Looks like you accidentally reintroduced this deleted line of code during > conflict resolution. Ooops, removed. https://codereview.chromium.org/1589953005/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:135: new CursorAnchorInfoController.ComposingTextDelegate() { On 2016/02/26 08:25:33, aelias wrote: > It's a bit odd to be new'ing this object just to immediately abandon it on K and > below. Could you pull out the isSupported() branch to be in this method > instead, and just never call create() if it's not? Done. https://codereview.chromium.org/1589953005/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:150: return BaseInputConnection.getComposingSpanStart(mEditable); On 2016/02/26 08:25:33, aelias wrote: > The refactoring https://codereview.chromium.org/1278593004/ just landed and > you'll need to rebaseline on it. The main substantive conflict is that you > can't do this anymore, as the new approach doesn't use Editable nor > BaseInputConnection. The information you need still passes through > ImeAdapter.updateState() though. Done. https://codereview.chromium.org/1589953005/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:683: mTextInputType = 0; On 2016/02/26 08:25:33, aelias wrote: > Another deleted line accidentally reintroduced? Ouch. Sorry for my carelessness.. Done,.
aelias@chromium.org changed reviewers: + sievers@chromium.org, tedchoc@chromium.org - jdduke@chromium.org
lgtm, thanks. Adding tedchoc@ for other browser files OWNERs, sievers@ for small content/renderer change.
On 2016/03/01 18:49:53, aelias wrote: > lgtm, thanks. Adding tedchoc@ for other browser files OWNERs, sievers@ for > small content/renderer change. lgtm
lgtm with nits https://codereview.chromium.org/1589953005/diff/280001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/1589953005/diff/280001/content/public/android... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:80: * Sets several fields for unit test. (used by {@link CursorAnchorInfoSourceTest}). you mean CursorAnchorInfoControllerTest? https://codereview.chromium.org/1589953005/diff/280001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/CursorAnchorInfoControllerTest.java (right): https://codereview.chromium.org/1589953005/diff/280001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/CursorAnchorInfoControllerTest.java:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1589953005/diff/280001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/CursorAnchorInfoControllerTest.java:29: public static final int CURSOR_UPDATE_IMMEDIATE = 1 << 0; I think you can just use InputConnection.CURSOR_UPDATE_* values instead. https://codereview.chromium.org/1589953005/diff/280001/content/public/test/an... File content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestInputMethodManagerWrapper.java (right): https://codereview.chromium.org/1589953005/diff/280001/content/public/test/an... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestInputMethodManagerWrapper.java:38: private int mUpdateCursorAnchorInfoCounter = 0; no need to set 0 here
Thanks guys! Reflected comments by Changwan and filled my TODO in ImeLollipopTest. I'll wait for tedchoc@'s eyes as a content/browser OWNER. https://codereview.chromium.org/1589953005/diff/280001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/1589953005/diff/280001/content/public/android... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:80: * Sets several fields for unit test. (used by {@link CursorAnchorInfoSourceTest}). On 2016/03/01 19:11:42, Changwan Ryu wrote: > you mean CursorAnchorInfoControllerTest? Done. I forgot to change the name remaining from very old datasets. https://codereview.chromium.org/1589953005/diff/280001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/CursorAnchorInfoControllerTest.java (right): https://codereview.chromium.org/1589953005/diff/280001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/CursorAnchorInfoControllerTest.java:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/03/01 19:11:42, Changwan Ryu wrote: > 2016 Done. https://codereview.chromium.org/1589953005/diff/280001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/CursorAnchorInfoControllerTest.java:29: public static final int CURSOR_UPDATE_IMMEDIATE = 1 << 0; On 2016/03/01 19:11:42, Changwan Ryu wrote: > I think you can just use InputConnection.CURSOR_UPDATE_* values instead. Good catch. Done. https://codereview.chromium.org/1589953005/diff/280001/content/public/test/an... File content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestInputMethodManagerWrapper.java (right): https://codereview.chromium.org/1589953005/diff/280001/content/public/test/an... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestInputMethodManagerWrapper.java:38: private int mUpdateCursorAnchorInfoCounter = 0; On 2016/03/01 19:11:42, Changwan Ryu wrote: > no need to set 0 here Done.
nothing too major and mostly style nits https://codereview.chromium.org/1589953005/diff/300001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/1589953005/diff/300001/content/browser/androi... content/browser/android/content_view_core_impl.cc:437: // Android Framework as of API Level 21. Actually supporting non-zero width While I think this comment is informative, it seems a bit too details for the code. I think this is sufficient: "The CursorAnchorInfo API in Android only support zero width selection bounds" The other things can live in the crbug, but here it's not very actionable. https://codereview.chromium.org/1589953005/diff/300001/content/browser/androi... content/browser/android/content_view_core_impl.cc:445: selection_start.type == cc::SELECTION_BOUND_CENTER; I assume SELECTION_BOUND_CENTER is the only bit that is needed to determine a zero width selection bounds? https://codereview.chromium.org/1589953005/diff/300001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java (right): https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:23: /* add a extra * to make this a javadoc comment https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:42: public CharSequence getText(); don't need publics in interfaces https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:259: mMatrix.setScale(mScale, mScale); the matrix is never reset anywhere...I assume there are no additive operations that require you to reset it? https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:269: CursorAnchorInfo.FLAG_HAS_INVISIBLE_REGION); +4 indent https://codereview.chromium.org/1589953005/diff/300001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:218: mCursorAnchorInfoController.setInputMethodManagerWrapper(immw); Should this method also be called setInputMethodManagerWrapperForTest? Or do you call it for normal paths as well? https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:634: /* add another * https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:635: * Forward the cursor update request to CursorAnchorInfoController. This comment describes the code not why you should call it, which is not particularly helpful. https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:644: * Notify the location of composing characters to the IME if it explicitly requested them. Again this comment isn't really a good description of this method. This should just state something like "Notified when a frame has been produced by the renderer and all the associated metadata". onUpdateFrameInfo has not directly associated with composing characters. Or you can rename it like updateComposingCharacterPositions or something. https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:646: * View-local Physical (screen) coordinates align with the start of the sentence above (coordinate information ...). Same for subsequent lines. https://codereview.chromium.org/1589953005/diff/300001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/CursorAnchorInfoControllerTest.java (right): https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/CursorAnchorInfoControllerTest.java:37: public int mLocationX; no leading m prefix for public fields. https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/CursorAnchorInfoControllerTest.java:97: expectedMatrix.getValues(expectedMatrixValues); The documentation states it copies 9, is that incorrect and it actually copies 12? http://developer.android.com/reference/android/graphics/Matrix.html#getValues... https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/CursorAnchorInfoControllerTest.java:101: assertEquals(expectedMatrixValues[i], actualMatrixValues[i]); why not use Matrix.equals instead of comparing floats? Also, it is sad that MoreAsserts doesn't have a .equals for float arrays https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/CursorAnchorInfoControllerTest.java:135: private void assertEquals(RectF expectedRect, RectF actualRect) { same question as Matrix, why not use the .equals https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/CursorAnchorInfoControllerTest.java:165: controller.setCompositionCharacterBounds(new float[]{0.0f, 1.0f, 2.0f, 3.0f}); pretty sure there is supposed to be a space after the [] and before {
Thanks! https://codereview.chromium.org/1589953005/diff/300001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/1589953005/diff/300001/content/browser/androi... content/browser/android/content_view_core_impl.cc:437: // Android Framework as of API Level 21. Actually supporting non-zero width On 2016/03/07 21:31:45, Ted C wrote: > While I think this comment is informative, it seems a bit too details for the > code. > > I think this is sufficient: > "The CursorAnchorInfo API in Android only support zero width selection bounds" > > The other things can live in the crbug, but here it's not very actionable. Done. https://codereview.chromium.org/1589953005/diff/300001/content/browser/androi... content/browser/android/content_view_core_impl.cc:445: selection_start.type == cc::SELECTION_BOUND_CENTER; On 2016/03/07 21:31:45, Ted C wrote: > I assume SELECTION_BOUND_CENTER is the only bit that is needed to determine a > zero width selection bounds? Yes. To be precice it's an enum with 4 possible values {LEFT=0,RIGHT=1,CENTER=2,EMPTY=3} not a bitmasks. LEFT/RIGHT means ranged selection and EMPTY for no selection; CENTER is the one corresponding to zero-width. https://codereview.chromium.org/1589953005/diff/300001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java (right): https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:23: /* On 2016/03/07 21:31:45, Ted C wrote: > add a extra * to make this a javadoc comment Done. https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:42: public CharSequence getText(); On 2016/03/07 21:31:45, Ted C wrote: > don't need publics in interfaces Done. https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:259: mMatrix.setScale(mScale, mScale); On 2016/03/07 21:31:45, Ted C wrote: > the matrix is never reset anywhere...I assume there are no additive operations > that require you to reset it? This is the point of reset actually. setXXX methods (including setScale) resets all the values previously set, and configures the matrix to do just XXX only. https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/CursorAnchorInfoController.java:269: CursorAnchorInfo.FLAG_HAS_INVISIBLE_REGION); On 2016/03/07 21:31:45, Ted C wrote: > +4 indent Done. https://codereview.chromium.org/1589953005/diff/300001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:218: mCursorAnchorInfoController.setInputMethodManagerWrapper(immw); On 2016/03/07 21:31:45, Ted C wrote: > Should this method also be called setInputMethodManagerWrapperForTest? > > Or do you call it for normal paths as well? Good point. It's only for testing. Done. https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:634: /* On 2016/03/07 21:31:45, Ted C wrote: > add another * Done. https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:635: * Forward the cursor update request to CursorAnchorInfoController. On 2016/03/07 21:31:45, Ted C wrote: > This comment describes the code not why you should call it, which is not > particularly helpful. Done. https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:644: * Notify the location of composing characters to the IME if it explicitly requested them. On 2016/03/07 21:31:45, Ted C wrote: > Again this comment isn't really a good description of this method. > > This should just state something like "Notified when a frame has been produced > by the renderer and all the associated metadata". > > onUpdateFrameInfo has not directly associated with composing characters. Or you > can rename it like updateComposingCharacterPositions or something. Done. https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:646: * View-local Physical (screen) coordinates On 2016/03/07 21:31:45, Ted C wrote: > align with the start of the sentence above (coordinate information ...). Same > for subsequent lines. Done. https://codereview.chromium.org/1589953005/diff/300001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/CursorAnchorInfoControllerTest.java (right): https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/CursorAnchorInfoControllerTest.java:37: public int mLocationX; On 2016/03/07 21:31:45, Ted C wrote: > no leading m prefix for public fields. Done. https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/CursorAnchorInfoControllerTest.java:97: expectedMatrix.getValues(expectedMatrixValues); On 2016/03/07 21:31:45, Ted C wrote: > The documentation states it copies 9, is that incorrect and it actually copies > 12? > > http://developer.android.com/reference/android/graphics/Matrix.html#getValues... It's 9. https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/CursorAnchorInfoControllerTest.java:101: assertEquals(expectedMatrixValues[i], actualMatrixValues[i]); On 2016/03/07 21:31:45, Ted C wrote: > why not use Matrix.equals instead of comparing floats? > > Also, it is sad that MoreAsserts doesn't have a .equals for float arrays Done. https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/CursorAnchorInfoControllerTest.java:135: private void assertEquals(RectF expectedRect, RectF actualRect) { On 2016/03/07 21:31:45, Ted C wrote: > same question as Matrix, why not use the .equals Done. https://codereview.chromium.org/1589953005/diff/300001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/CursorAnchorInfoControllerTest.java:165: controller.setCompositionCharacterBounds(new float[]{0.0f, 1.0f, 2.0f, 3.0f}); On 2016/03/07 21:31:45, Ted C wrote: > pretty sure there is supposed to be a space after the [] and before { Done.
lgtm
On 2016/03/10 18:14:42, Ted C wrote: > lgtm Thank you, Ted, and all reviewers for your thorough discussion!
The CQ bit was checked by kinaba@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, changwan@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1589953005/#ps340001 (title: "Addressed comments in #38")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1589953005/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1589953005/340001
Message was sent while issue was closed.
Description was changed from ========== Support InputMethodManager#updateCursorAnchorInfo for Android 5.0 This is the 6th part of the series of CLs to enable InputMethodManager#updateCursorAnchorInfo support for Android 5.0. - part1 crrev.com/671503005 - part2 crrev.com/679223002 - part3 crrev.com/755853004 - part4 crrev.com/784503002 - part5 crrev.com/757233003 It enables the feature under a flag, with one possible optimization chance (dynamic subscription) not yet implemented. This is based on yukawa@'s original CL (http://crrev.com/699333003), and adds accommodation with recent changes on coordinate systems in Chromium trunk. It also adds a few comments on coordinate systems so that the intention of the code become clearer. This CL consists of the following 3 parts, 1. Adds CursorAnchorInfoController as the central place to control when and how CursorAnchorInfo object should be built and sent to the IME. 2. Adds tests for CursorAnchorInfoController. 3. Modifies renderer_widget_host_view_android and ContentViewCore to wire necessary parameters to CursorAnchorInfoController. Design Doc: https://docs.google.com/a/chromium.org/document/d/1KdnoynGURrJBP1vDHcv2jwUdns... BUG=424866 TEST=Manually done on Nexus 6 Build/MMB29S, Nexus 4 Build/LMY48T TEST=build/android/test_runner.py instrumentation --release \ --test-apk=ContentShellTest -f '*CursorAnchorInfoControllerTest*' TEST=build/android/test_runner.py instrumentation --release \ --test-apk=ContentShellTest -f '*ImeLollipopTest*' ========== to ========== Support InputMethodManager#updateCursorAnchorInfo for Android 5.0 This is the 6th part of the series of CLs to enable InputMethodManager#updateCursorAnchorInfo support for Android 5.0. - part1 crrev.com/671503005 - part2 crrev.com/679223002 - part3 crrev.com/755853004 - part4 crrev.com/784503002 - part5 crrev.com/757233003 It enables the feature under a flag, with one possible optimization chance (dynamic subscription) not yet implemented. This is based on yukawa@'s original CL (http://crrev.com/699333003), and adds accommodation with recent changes on coordinate systems in Chromium trunk. It also adds a few comments on coordinate systems so that the intention of the code become clearer. This CL consists of the following 3 parts, 1. Adds CursorAnchorInfoController as the central place to control when and how CursorAnchorInfo object should be built and sent to the IME. 2. Adds tests for CursorAnchorInfoController. 3. Modifies renderer_widget_host_view_android and ContentViewCore to wire necessary parameters to CursorAnchorInfoController. Design Doc: https://docs.google.com/a/chromium.org/document/d/1KdnoynGURrJBP1vDHcv2jwUdns... BUG=424866 TEST=Manually done on Nexus 6 Build/MMB29S, Nexus 4 Build/LMY48T TEST=build/android/test_runner.py instrumentation --release \ --test-apk=ContentShellTest -f '*CursorAnchorInfoControllerTest*' TEST=build/android/test_runner.py instrumentation --release \ --test-apk=ContentShellTest -f '*ImeLollipopTest*' ==========
Message was sent while issue was closed.
Committed patchset #15 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Support InputMethodManager#updateCursorAnchorInfo for Android 5.0 This is the 6th part of the series of CLs to enable InputMethodManager#updateCursorAnchorInfo support for Android 5.0. - part1 crrev.com/671503005 - part2 crrev.com/679223002 - part3 crrev.com/755853004 - part4 crrev.com/784503002 - part5 crrev.com/757233003 It enables the feature under a flag, with one possible optimization chance (dynamic subscription) not yet implemented. This is based on yukawa@'s original CL (http://crrev.com/699333003), and adds accommodation with recent changes on coordinate systems in Chromium trunk. It also adds a few comments on coordinate systems so that the intention of the code become clearer. This CL consists of the following 3 parts, 1. Adds CursorAnchorInfoController as the central place to control when and how CursorAnchorInfo object should be built and sent to the IME. 2. Adds tests for CursorAnchorInfoController. 3. Modifies renderer_widget_host_view_android and ContentViewCore to wire necessary parameters to CursorAnchorInfoController. Design Doc: https://docs.google.com/a/chromium.org/document/d/1KdnoynGURrJBP1vDHcv2jwUdns... BUG=424866 TEST=Manually done on Nexus 6 Build/MMB29S, Nexus 4 Build/LMY48T TEST=build/android/test_runner.py instrumentation --release \ --test-apk=ContentShellTest -f '*CursorAnchorInfoControllerTest*' TEST=build/android/test_runner.py instrumentation --release \ --test-apk=ContentShellTest -f '*ImeLollipopTest*' ========== to ========== Support InputMethodManager#updateCursorAnchorInfo for Android 5.0 This is the 6th part of the series of CLs to enable InputMethodManager#updateCursorAnchorInfo support for Android 5.0. - part1 crrev.com/671503005 - part2 crrev.com/679223002 - part3 crrev.com/755853004 - part4 crrev.com/784503002 - part5 crrev.com/757233003 It enables the feature under a flag, with one possible optimization chance (dynamic subscription) not yet implemented. This is based on yukawa@'s original CL (http://crrev.com/699333003), and adds accommodation with recent changes on coordinate systems in Chromium trunk. It also adds a few comments on coordinate systems so that the intention of the code become clearer. This CL consists of the following 3 parts, 1. Adds CursorAnchorInfoController as the central place to control when and how CursorAnchorInfo object should be built and sent to the IME. 2. Adds tests for CursorAnchorInfoController. 3. Modifies renderer_widget_host_view_android and ContentViewCore to wire necessary parameters to CursorAnchorInfoController. Design Doc: https://docs.google.com/a/chromium.org/document/d/1KdnoynGURrJBP1vDHcv2jwUdns... BUG=424866 TEST=Manually done on Nexus 6 Build/MMB29S, Nexus 4 Build/LMY48T TEST=build/android/test_runner.py instrumentation --release \ --test-apk=ContentShellTest -f '*CursorAnchorInfoControllerTest*' TEST=build/android/test_runner.py instrumentation --release \ --test-apk=ContentShellTest -f '*ImeLollipopTest*' Committed: https://crrev.com/d607fd36000c470d91dd96976698e63beeb87980 Cr-Commit-Position: refs/heads/master@{#380504} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/d607fd36000c470d91dd96976698e63beeb87980 Cr-Commit-Position: refs/heads/master@{#380504} |
