Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2500663002:
Relay Text Field Updates from ContentView to External InputMethodManager. (Closed)
DescriptionRelay Text Field Updates from ContentView to External InputMethodManager.
This change surfaces up all ExtractedText updates from the content view
back to the InputMethodManager.
Is is applicable to allow consumers of the Android InputManager
to receive text change events back.
This change does not affect Standard Input Method Managers such as the
Android Default Keyboard registered to the platform but specifically
affects External Input Method apps like virtual floating keyboard
implementations or screen keyboard overlay implementations.
In addition this change is consistent with the guidelines given as part of
https://developer.android.com/reference/android/view/inputmethod/InputConnection.html
States that if the GET_EXTRACTED_TEXT_MONITOR flag is set,
you should be calling updateExtractedText(View, int, ExtractedText)
whenever you call updateSelection(View, int, int, int, int).
BUG=673819
TEST=New ContentShellJunitTests, Existing ContentShell Instrumentation Tests.
Review-Url: https://codereview.chromium.org/2500663002
Cr-Commit-Position: refs/heads/master@{#470488}
Committed: https://chromium.googlesource.com/chromium/src/+/9b5ebac3230f3c90782296ae95396e7f512729bc
Patch Set 1 #Patch Set 2 : 1. Added ImeAdapter Junit Tests 2. Rebased on Tip of Master 3. Fixed logic to continiously request … #Patch Set 3 : Minimize CL size #
Total comments: 10
Patch Set 4 : Addressed cl comments from changwan #Patch Set 5 : minimize patch size #
Total comments: 12
Patch Set 6 : Addressing aelias@'s comments #Patch Set 7 : Removing unused method and annotating package private fields with @VisibleForTesting #
Total comments: 4
Patch Set 8 : Addressing comments and ensuring that we stop sending extracted text when InputConnection is reset … #
Total comments: 4
Patch Set 9 : Rewriting Unit test set-up to use IME thread for execution and cleaning up logic for ThreadedInputC… #
Total comments: 15
Patch Set 10 : Refactoring Unit tests with the recommended Public API pattern for ThreadedInputConnectionTests #
Total comments: 2
Patch Set 11 : Modify ThreadedInputConnection#resetOnUiThread cleanup logic to be invoked via handler. #
Total comments: 2
Patch Set 12 : Using thread safe TextInputState reference within ThreadedInputConnection#updateSelection #
Messages
Total messages: 49 (17 generated)
Description was changed from ========== Relay Text Field Updates from ContentView to External InputMethodManager. This change surfaces up all ExtractedText updates from the content view back to th InputMethodManager. This change is applicable to allow consumers of the Android InputManager to receive text change events back. This change does not affect Standard Input Method Managers such as the Android Default Keyboard registered to the platform but specifically affects External Input Method apps like virtual floating keyboard implementations or screen keyboard overlay implementations. In addition this change is consistent with the guidelines given as part of https://developer.android.com/reference/android/view/inputmethod/InputConnect... States that if the GET_EXTRACTED_TEXT_MONITOR flag is set, you should be calling updateExtractedText(View, int, ExtractedText) whenever you call updateSelection(View, int, int, int, int). BUG=0 TEST=ContentShell Instrumentation Tests ========== to ========== Relay Text Field Updates from ContentView to External InputMethodManager. This change surfaces up all ExtractedText updates from the content view back to the InputMethodManager. Is is applicable to allow consumers of the Android InputManager to receive text change events back. This change does not affect Standard Input Method Managers such as the Android Default Keyboard registered to the platform but specifically affects External Input Method apps like virtual floating keyboard implementations or screen keyboard overlay implementations. In addition this change is consistent with the guidelines given as part of https://developer.android.com/reference/android/view/inputmethod/InputConnect... States that if the GET_EXTRACTED_TEXT_MONITOR flag is set, you should be calling updateExtractedText(View, int, ExtractedText) whenever you call updateSelection(View, int, int, int, int). BUG=0 TEST=ContentShell Instrumentation Tests ==========
bhallam@amazon.com changed reviewers: + aelias@chromium.org
On 2016/11/11 23:36:40, bhallam wrote: > mailto:bhallam@amazon.com changed reviewers: > + mailto:aelias@chromium.org Hi Aelias, Can you please take a look at this patch request. Thanks! Mohit
On 2016/11/11 23:38:42, bhallam wrote: > On 2016/11/11 23:36:40, bhallam wrote: > > mailto:bhallam@amazon.com changed reviewers: > > + mailto:aelias@chromium.org > > Hi Aelias, > Can you please take a look at this patch request. > > Thanks! > Mohit In Addition I intentionally refrained from changing the class hierarchy which would have involved creating a base Class which abstracts out the extracted text metadata retrieval shared between the Two concrete implementations of ChromiumBaseInputConnection as the current hierarchy for the Replica and Threaded Input connections involves of a Mixin of BaseInputConnection and ChromiumBaseInputConnection.
Hi Aelias, Can you PTAL at this patch. Thanks! Mohit
bhallam@amazon.com changed reviewers: + changwan@chromium.org
bhallam@amazon.com changed reviewers: - aelias@chromium.org
Hi changwan, Can you PTAL at this CL. Thanks! Mohit
Hi bhallam, Thanks for making this change. Could you also add some tests to ImeTest.java to make sure we get the correct extracted text? Also, could you create a crbug.com to explain why this is needed? (If there is any business need, please state it as well.)
On 2016/11/24 05:45:09, Changwan Ryu wrote: > Hi bhallam, > > Thanks for making this change. Could you also add some tests to ImeTest.java to > make sure we get the correct extracted text? Also, could you create a http://crbug.com > to explain why this is needed? (If there is any business need, please state it > as well.) Thanks for the response. I am on it asap! Thanks
On 2016/11/30 18:28:23, bhallam wrote: > On 2016/11/24 05:45:09, Changwan Ryu wrote: > > Hi bhallam, > > > > Thanks for making this change. Could you also add some tests to ImeTest.java > to > > make sure we get the correct extracted text? Also, could you create a > http://crbug.com > > to explain why this is needed? (If there is any business need, please state it > > as well.) > > Thanks for the response. > I am on it asap! > > Thanks The current changelist addressed 3 things 1. Added ImeAdapter Junit Tests 2. Rebased on Tip of Master 3. Fixed logic to continuously request extracted text when encountering flag android.view.inputmethod.InputConnection#GET_EXTRACTED_TEXT_MONITOR as part of android.view.inputmethod.InputConnection#getExtractedText(ExtractedTextRequest request, int flags) callback. 4. Created CR Bug 673819
Description was changed from ========== Relay Text Field Updates from ContentView to External InputMethodManager. This change surfaces up all ExtractedText updates from the content view back to the InputMethodManager. Is is applicable to allow consumers of the Android InputManager to receive text change events back. This change does not affect Standard Input Method Managers such as the Android Default Keyboard registered to the platform but specifically affects External Input Method apps like virtual floating keyboard implementations or screen keyboard overlay implementations. In addition this change is consistent with the guidelines given as part of https://developer.android.com/reference/android/view/inputmethod/InputConnect... States that if the GET_EXTRACTED_TEXT_MONITOR flag is set, you should be calling updateExtractedText(View, int, ExtractedText) whenever you call updateSelection(View, int, int, int, int). BUG=0 TEST=ContentShell Instrumentation Tests ========== to ========== Relay Text Field Updates from ContentView to External InputMethodManager. This change surfaces up all ExtractedText updates from the content view back to the InputMethodManager. Is is applicable to allow consumers of the Android InputManager to receive text change events back. This change does not affect Standard Input Method Managers such as the Android Default Keyboard registered to the platform but specifically affects External Input Method apps like virtual floating keyboard implementations or screen keyboard overlay implementations. In addition this change is consistent with the guidelines given as part of https://developer.android.com/reference/android/view/inputmethod/InputConnect... States that if the GET_EXTRACTED_TEXT_MONITOR flag is set, you should be calling updateExtractedText(View, int, ExtractedText) whenever you call updateSelection(View, int, int, int, int). BUG=673819 TEST=New ContentShellJunitTests, Existing ContentShell Instrumentation Tests. ==========
https://codereview.chromium.org/2500663002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/2500663002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:511: could you move this inside ThreadedInputConnection? Then you don't need to expose shouldUpdateExtractedText() and getCurrentExtractedTextRequestToken(). Also, if you move this there you don't need to document this - I think it's mostly self-explanatory because getExtractedText() sets some fields there. (Or you could add comment to the fields instead..) https://codereview.chromium.org/2500663002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:522: request.flags = InputConnection.GET_TEXT_WITH_STYLES; Why do you need to override request flags? https://codereview.chromium.org/2500663002/diff/40001/content/public/android/... File content/public/android/junit/src/org/chromium/content/browser/input/ImeAdapterTest.java (right): https://codereview.chromium.org/2500663002/diff/40001/content/public/android/... content/public/android/junit/src/org/chromium/content/browser/input/ImeAdapterTest.java:62: public void testUpdateSelectionBehaviorWhenUpdatesRequested() { Could you move this test case to ThreadedInputConnectionTest? https://codereview.chromium.org/2500663002/diff/40001/content/public/android/... content/public/android/junit/src/org/chromium/content/browser/input/ImeAdapterTest.java:79: .updateExtractedText(mView, request.token, mExtractedText); Please compare the exact values in the object instead of comparing the object. You're overriding flags in ImeAdapter in this patch but they aren't reflected in the test. If you're doing something wrong in the real logic, you won't be able to find it with this test. https://codereview.chromium.org/2500663002/diff/40001/content/public/android/... content/public/android/junit/src/org/chromium/content/browser/input/ImeAdapterTest.java:86: public void testUpdateSelectionBehaviorWhenUpdatesNotRequested() { Probably you don't need this test because if we still update text when update was not requested then other tests should fail.
On 2016/12/21 23:47:58, Changwan Ryu wrote: > https://codereview.chromium.org/2500663002/diff/40001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java > (right): > > https://codereview.chromium.org/2500663002/diff/40001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:511: > > could you move this inside ThreadedInputConnection? Then you don't need to > expose shouldUpdateExtractedText() and getCurrentExtractedTextRequestToken(). > Also, if you move this there you don't need to document this - I think it's > mostly self-explanatory because getExtractedText() sets some fields there. (Or > you could add comment to the fields instead..) > > Makes sense and Done. https://codereview.chromium.org/2500663002/diff/40001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:522: > request.flags = InputConnection.GET_TEXT_WITH_STYLES; > Why do you need to override request flags? > > I was not sure what attributes to set and the only reference i could find was from android.inputmethodservice.InputMethodService#startExtractingText from the Android SDK https://android.googlesource.com/platform/frameworks/base/+/master/core/java/... https://codereview.chromium.org/2500663002/diff/40001/content/public/android/... > File > content/public/android/junit/src/org/chromium/content/browser/input/ImeAdapterTest.java > (right): > > https://codereview.chromium.org/2500663002/diff/40001/content/public/android/... > content/public/android/junit/src/org/chromium/content/browser/input/ImeAdapterTest.java:62: > public void testUpdateSelectionBehaviorWhenUpdatesRequested() { > Could you move this test case to ThreadedInputConnectionTest? > > Done. https://codereview.chromium.org/2500663002/diff/40001/content/public/android/... > content/public/android/junit/src/org/chromium/content/browser/input/ImeAdapterTest.java:79: > .updateExtractedText(mView, request.token, mExtractedText); > Please compare the exact values in the object instead of comparing the object. > > You're overriding flags in ImeAdapter in this patch but they aren't reflected in > the test. If you're doing something wrong in the real logic, you won't be able > to find it with this test. > > Agreed and Fixed. https://codereview.chromium.org/2500663002/diff/40001/content/public/android/... > content/public/android/junit/src/org/chromium/content/browser/input/ImeAdapterTest.java:86: > public void testUpdateSelectionBehaviorWhenUpdatesNotRequested() { > Probably you don't need this test because if we still update text when update > was not requested then other tests should fail. Done.
Hi Changwan, Can you please take a look at the latest CL. Thanks! Mohit Bhalla https://codereview.chromium.org/2500663002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/2500663002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:511: On 2016/12/21 23:47:58, Changwan Ryu wrote: > could you move this inside ThreadedInputConnection? Then you don't need to > expose shouldUpdateExtractedText() and getCurrentExtractedTextRequestToken(). > Also, if you move this there you don't need to document this - I think it's > mostly self-explanatory because getExtractedText() sets some fields there. (Or > you could add comment to the fields instead..) Makes sense and Done. https://codereview.chromium.org/2500663002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:522: request.flags = InputConnection.GET_TEXT_WITH_STYLES; On 2016/12/21 23:47:58, Changwan Ryu wrote: > Why do you need to override request flags? I was not sure what attributes to set and the only reference i could find was from android.inputmethodservice.InputMethodService#startExtractingText from the Android SDK https://android.googlesource.com/platform/frameworks/base/+/master/core/java/... https://codereview.chromium.org/2500663002/diff/40001/content/public/android/... File content/public/android/junit/src/org/chromium/content/browser/input/ImeAdapterTest.java (right): https://codereview.chromium.org/2500663002/diff/40001/content/public/android/... content/public/android/junit/src/org/chromium/content/browser/input/ImeAdapterTest.java:62: public void testUpdateSelectionBehaviorWhenUpdatesRequested() { On 2016/12/21 23:47:58, Changwan Ryu wrote: > Could you move this test case to ThreadedInputConnectionTest? Done. https://codereview.chromium.org/2500663002/diff/40001/content/public/android/... content/public/android/junit/src/org/chromium/content/browser/input/ImeAdapterTest.java:79: .updateExtractedText(mView, request.token, mExtractedText); On 2016/12/21 23:47:58, Changwan Ryu wrote: > Please compare the exact values in the object instead of comparing the object. > > You're overriding flags in ImeAdapter in this patch but they aren't reflected in > the test. If you're doing something wrong in the real logic, you won't be able > to find it with this test. Agreed and Fixed. https://codereview.chromium.org/2500663002/diff/40001/content/public/android/... content/public/android/junit/src/org/chromium/content/browser/input/ImeAdapterTest.java:86: public void testUpdateSelectionBehaviorWhenUpdatesNotRequested() { On 2016/12/21 23:47:58, Changwan Ryu wrote: > Probably you don't need this test because if we still update text when update > was not requested then other tests should fail. Done.
bhallam@amazon.com changed reviewers: + aelias@chromium.org
Hi aelias, Can you PTAL at this CL. Thanks bhallam
https://codereview.chromium.org/2500663002/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java (right): https://codereview.chromium.org/2500663002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:101: final ExtractedTextRequest mExtractedTextRequest = new ExtractedTextRequest(); This isn't needed if you stop calling getExtractedText() from updateSelection() below, please delete it. https://codereview.chromium.org/2500663002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:220: void updateSelection(Range selection, Range composition) { Overloading is disallowed by style guide and I don't see the point of these two new methods anyway, please merge them into original updateSelection. Also, please don't call updateSelection directly from the test, please write the test against the public API surface instead. https://codereview.chromium.org/2500663002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:246: final ExtractedText extractedText = getExtractedText(mExtractedTextRequest, This causes an unnecessary synchronous round-trip to the renderer process. Please split off the code to generate extracted text from mTextInputState into a helper method and call that here instead. https://codereview.chromium.org/2500663002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:422: if ((flags & GET_EXTRACTED_TEXT_MONITOR) > 0) { Calling this without the flag should also clear the token. https://codereview.chromium.org/2500663002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:440: void setUpdateExtractedTextForTesting(boolean flag) { It seems this shouldn't be needed and you can test via the public API. https://codereview.chromium.org/2500663002/diff/80001/content/public/android/... File content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java (right): https://codereview.chromium.org/2500663002/diff/80001/content/public/android/... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:237: mRunningOnUiThread = true; These calls aren't coming in on the UI thread in reality, so please remove this.
Addressing CR comments https://codereview.chromium.org/2500663002/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java (right): https://codereview.chromium.org/2500663002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:101: final ExtractedTextRequest mExtractedTextRequest = new ExtractedTextRequest(); On 2017/04/27 00:46:52, aelias wrote: > This isn't needed if you stop calling getExtractedText() from updateSelection() > below, please delete it. Done. https://codereview.chromium.org/2500663002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:220: void updateSelection(Range selection, Range composition) { On 2017/04/27 00:46:52, aelias wrote: > Overloading is disallowed by style guide and I don't see the point of these two > new methods anyway, please merge them into original updateSelection. Also, > please don't call updateSelection directly from the test, please write the test > against the public API surface instead. Done. Howevever can I ask which style guide are you referring to as I am not sure why would overloading would be disallowed? https://codereview.chromium.org/2500663002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:246: final ExtractedText extractedText = getExtractedText(mExtractedTextRequest, On 2017/04/27 00:46:52, aelias wrote: > This causes an unnecessary synchronous round-trip to the renderer process. > Please split off the code to generate extracted text from mTextInputState into a > helper method and call that here instead. Done! https://codereview.chromium.org/2500663002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:422: if ((flags & GET_EXTRACTED_TEXT_MONITOR) > 0) { On 2017/04/27 00:46:52, aelias wrote: > Calling this without the flag should also clear the token. Done. https://codereview.chromium.org/2500663002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:440: void setUpdateExtractedTextForTesting(boolean flag) { On 2017/04/27 00:46:52, aelias wrote: > It seems this shouldn't be needed and you can test via the public API. Done.
https://codereview.chromium.org/2500663002/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java (right): https://codereview.chromium.org/2500663002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:220: void updateSelection(Range selection, Range composition) { On 2017/04/27 at 23:31:43, bhallam wrote: > On 2017/04/27 00:46:52, aelias wrote: > > Overloading is disallowed by style guide and I don't see the point of these two > > new methods anyway, please merge them into original updateSelection. Also, > > please don't call updateSelection directly from the test, please write the test > > against the public API surface instead. > > Done. > Howevever can I ask which style guide are you referring to as I am not sure why would overloading would be disallowed? "Cons" in https://google.github.io/styleguide/cppguide.html#Function_Overloading, although I think it was loosened from a ban since the last time I looked it up. https://codereview.chromium.org/2500663002/diff/120001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java (right): https://codereview.chromium.org/2500663002/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:92: @VisibleForTesting I'd like to avoid @VisibleForTesting and write the test purely against the InputConnection API (and possibly mocks/fakes of attached objects). changwan@, do you have any advice on the best way to write the test for this? https://codereview.chromium.org/2500663002/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:212: final ExtractedTextRequest extractedTextRequest = new ExtractedTextRequest(); This request isn't used for anything, please delete it. https://codereview.chromium.org/2500663002/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:401: private ExtractedText getExtractedText(TextInputState textInputState) { This overloading is particularly confusing because the methods do completely different things. Please rename this to convertToExtractedText and make it static.
One thing that isn't quite clear - should we keep sending extracted text even after onCreateInputConnection is called? The spec doesn't say anything, but I'm afraid that the current implementation will send extracted text even after we switch keyboards, which we probably don't want to. Assuming there isn't a good way to detect this case, I think we should stop sending extracted text once onCreateInputConnection is called. Please add a test around this case as well. https://codereview.chromium.org/2500663002/diff/120001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java (right): https://codereview.chromium.org/2500663002/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:92: @VisibleForTesting On 2017/04/27 23:53:20, aelias wrote: > I'd like to avoid @VisibleForTesting and write the test purely against the > InputConnection API (and possibly mocks/fakes of attached objects). changwan@, > do you have any advice on the best way to write the test for this? I think checking these variables aren't needed as we already check the extracted text and also we use 'never' to ensure it's never called. You can remove these variables and its reference.
On 2017/04/28 00:16:57, Changwan Ryu wrote: > One thing that isn't quite clear - should we keep sending extracted text even > after onCreateInputConnection is called? > > The spec doesn't say anything, but I'm afraid that the current implementation > will send extracted text even after we switch keyboards, which we probably don't > want to. Assuming there isn't a good way to detect this case, I think we should > stop sending extracted text once onCreateInputConnection is called. > > Please add a test around this case as well. > > https://codereview.chromium.org/2500663002/diff/120001/content/public/android... > File > content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java > (right): > > https://codereview.chromium.org/2500663002/diff/120001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:92: > @VisibleForTesting > On 2017/04/27 23:53:20, aelias wrote: > > I'd like to avoid @VisibleForTesting and write the test purely against the > > InputConnection API (and possibly mocks/fakes of attached objects). > changwan@, > > do you have any advice on the best way to write the test for this? > > I think checking these variables aren't needed as we already check the extracted > text and also we use 'never' to ensure it's never called. > > You can remove these variables and its reference. Done
https://codereview.chromium.org/2500663002/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java (right): https://codereview.chromium.org/2500663002/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:91: private int mCurrentExtractedTextRequestToken = 0; nit: remove "= 0", since the value of the token is irrelevant when mShouldUpdateExtractedText is false and there's no reason to call attention to it. https://codereview.chromium.org/2500663002/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:372: mCurrentExtractedTextRequestToken = 0; mShouldUpdateExtractedText = false; https://codereview.chromium.org/2500663002/diff/140001/content/public/android... File content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java (right): https://codereview.chromium.org/2500663002/diff/140001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:244: mRunningOnUiThread = true; Please remove all of these.
https://codereview.chromium.org/2500663002/diff/140001/content/public/android... File content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java (right): https://codereview.chromium.org/2500663002/diff/140001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:244: mRunningOnUiThread = true; On 2017/04/28 22:07:40, aelias wrote: > Please remove all of these. Currently by design of the test infrastructure for the ThreadedInputConnection, ThreadedInputConnectionTest#mRunningOnUiThread is required to be set true before invoking ThreadedInputConnection#getExtractedText in tests to mock out behavior of the ThreadedInputConnection#runningOnUiThread(), otherwise any attempt to fetch the TextInputState via ThreadedInputConnection#requestAndWaitForTextInputState() from within ThreadedInputConnection#getExtractedText() will block unless request is being carried out from the UiThread. A similar approach was employed in the test ThreadedInputConnectionTest#testUiThreadAccess. Do you have any suggestions how I can mock the blocking request without changing any existing implementation of the ThreadedInputConnection?
On 2017/04/28 at 23:00:47, bhallam wrote: > https://codereview.chromium.org/2500663002/diff/140001/content/public/android... > File content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java (right): > > https://codereview.chromium.org/2500663002/diff/140001/content/public/android... > content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:244: mRunningOnUiThread = true; > On 2017/04/28 22:07:40, aelias wrote: > > Please remove all of these. > > Currently by design of the test infrastructure for the ThreadedInputConnection, > ThreadedInputConnectionTest#mRunningOnUiThread is required to be set true > before invoking ThreadedInputConnection#getExtractedText in tests > to mock out behavior of the ThreadedInputConnection#runningOnUiThread(), > otherwise any attempt to fetch the TextInputState via ThreadedInputConnection#requestAndWaitForTextInputState() from within ThreadedInputConnection#getExtractedText() will block unless request is being carried out from the UiThread. > > A similar approach was employed in the test ThreadedInputConnectionTest#testUiThreadAccess. > > Do you have any suggestions how I can mock the blocking request without changing any existing implementation of the ThreadedInputConnection? changwan@, any advice? Ideally all of the tests should be running on the IME thread when calling into InputConnection public API, except for the specific one testUiThreadAccess. It looks like the other tests in this class are accidentally relying on the absence of assertion in getTextBeforeCursor().
On 2017/04/28 23:08:01, aelias wrote: > On 2017/04/28 at 23:00:47, bhallam wrote: > > > https://codereview.chromium.org/2500663002/diff/140001/content/public/android... > > File > content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java > (right): > > > > > https://codereview.chromium.org/2500663002/diff/140001/content/public/android... > > > content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:244: > mRunningOnUiThread = true; > > On 2017/04/28 22:07:40, aelias wrote: > > > Please remove all of these. > > > > Currently by design of the test infrastructure for the > ThreadedInputConnection, > > ThreadedInputConnectionTest#mRunningOnUiThread is required to be set true > > before invoking ThreadedInputConnection#getExtractedText in tests > > to mock out behavior of the ThreadedInputConnection#runningOnUiThread(), > > otherwise any attempt to fetch the TextInputState via > ThreadedInputConnection#requestAndWaitForTextInputState() from within > ThreadedInputConnection#getExtractedText() will block unless request is being > carried out from the UiThread. > > > > A similar approach was employed in the test > ThreadedInputConnectionTest#testUiThreadAccess. > > > > Do you have any suggestions how I can mock the blocking request without > changing any existing implementation of the ThreadedInputConnection? > > changwan@, any advice? Ideally all of the tests should be running on the IME > thread when calling into InputConnection public API, except for the specific one > testUiThreadAccess. It looks like the other tests in this class are > accidentally relying on the absence of assertion in getTextBeforeCursor(). In this test, it is implicitly assumed that test thread acts as IME thread. The pattern around get* methods in this test file is, instead of block-waiting for the state to come from the renderer process, add the state to the queue as preparation before calling get* methods. (The order is actually changed here, but this is the simplest way...) We can use the same pattern to avoid the blocking method for getExtractedText. testUiThreadAccess is a special case where it tests against UI thread, and mRunningOnUiThread should not be used in new tests.
On 2017/04/28 23:29:47, Changwan Ryu wrote: > On 2017/04/28 23:08:01, aelias wrote: > > On 2017/04/28 at 23:00:47, bhallam wrote: > > > > > > https://codereview.chromium.org/2500663002/diff/140001/content/public/android... > > > File > > > content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java > > (right): > > > > > > > > > https://codereview.chromium.org/2500663002/diff/140001/content/public/android... > > > > > > content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:244: > > mRunningOnUiThread = true; > > > On 2017/04/28 22:07:40, aelias wrote: > > > > Please remove all of these. > > > > > > Currently by design of the test infrastructure for the > > ThreadedInputConnection, > > > ThreadedInputConnectionTest#mRunningOnUiThread is required to be set true > > > before invoking ThreadedInputConnection#getExtractedText in tests > > > to mock out behavior of the ThreadedInputConnection#runningOnUiThread(), > > > otherwise any attempt to fetch the TextInputState via > > ThreadedInputConnection#requestAndWaitForTextInputState() from within > > ThreadedInputConnection#getExtractedText() will block unless request is being > > carried out from the UiThread. > > > > > > A similar approach was employed in the test > > ThreadedInputConnectionTest#testUiThreadAccess. > > > > > > Do you have any suggestions how I can mock the blocking request without > > changing any existing implementation of the ThreadedInputConnection? > > > > changwan@, any advice? Ideally all of the tests should be running on the IME > > thread when calling into InputConnection public API, except for the specific > one > > testUiThreadAccess. It looks like the other tests in this class are > > accidentally relying on the absence of assertion in getTextBeforeCursor(). > > In this test, it is implicitly assumed that test thread acts as IME thread. > The pattern around get* methods in this test file is, instead of block-waiting > for the state to come from the renderer process, > add the state to the queue as preparation before calling get* methods. (The > order is actually changed here, but this is the simplest way...) > > We can use the same pattern to avoid the blocking method for getExtractedText. > testUiThreadAccess is a special case where it tests against UI thread, > and mRunningOnUiThread should not be used in new tests. Done!
https://codereview.chromium.org/2500663002/diff/160001/content/public/android... File content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java (right): https://codereview.chromium.org/2500663002/diff/160001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:252: mConnection.getQueueForTest().put(mUnblocker); You need to call in the following order: when(mImeAdapter.requestTextInputStateUpdate()).thenReturn(true); mConnection.updateStateOnUiThread(...); mConnection.getExtractedText(...); Then you don't need to put unblocker. Also, you will need to update once more, with a different state, to verify that monitoring actually works. https://codereview.chromium.org/2500663002/diff/160001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:267: requestTokenCaptor.capture(), extractedTextArgumentCaptor.capture()); requestTokenCaptor isn't used anywhere, so you can replace this argument by anyInt? https://codereview.chromium.org/2500663002/diff/160001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:293: mConnection.getQueueForTest().put(mUnblocker); ditto about the call order. https://codereview.chromium.org/2500663002/diff/160001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:320: mConnection.getQueueForTest().put(mUnblocker); ditto.
https://codereview.chromium.org/2500663002/diff/160001/content/public/android... File content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java (right): https://codereview.chromium.org/2500663002/diff/160001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:252: mConnection.getQueueForTest().put(mUnblocker); On 2017/05/01 20:58:49, Changwan Ryu wrote: > You need to call in the following order: > > when(mImeAdapter.requestTextInputStateUpdate()).thenReturn(true); > mConnection.updateStateOnUiThread(...); > mConnection.getExtractedText(...); > > Then you don't need to put unblocker. > > Also, you will need to update once more, with a different state, to verify that > monitoring actually works. I think I will still need the unblocker for the tests. I have tried out the approach that you suggested; however it still seems to block on ThreadedInputConnection#blockAndGetStateUpdate(). Additionally as I need to pass in replyToRequest = true in the public public API call to ThreadedInputConnection#updateStateOnUiThread to verify the feature functionality that I have to test; the ThreadedInputConnection#processPendingInputStates() routine invoked from the updateStateOnUiThread API will empty the blocking queue before the call to mConnection.getExtractedText(...); will in turn blocks as the TextInputState Queue is of Size 0.
https://codereview.chromium.org/2500663002/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java (right): https://codereview.chromium.org/2500663002/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:107: mShouldUpdateExtractedText = false; The other two values are being considered moving to somewhere else, so it wasn't taken care of properly... But I don't think we should add new variables that are accessed from two threads. Could you reset them on IME thread along with other variables? https://codereview.chromium.org/2500663002/diff/160001/content/public/android... File content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java (right): https://codereview.chromium.org/2500663002/diff/160001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:252: mConnection.getQueueForTest().put(mUnblocker); On 2017/05/01 22:54:03, bhallam wrote: > On 2017/05/01 20:58:49, Changwan Ryu wrote: > > You need to call in the following order: > > > > when(mImeAdapter.requestTextInputStateUpdate()).thenReturn(true); > > mConnection.updateStateOnUiThread(...); > > mConnection.getExtractedText(...); > > > > Then you don't need to put unblocker. > > > > Also, you will need to update once more, with a different state, to verify > that > > monitoring actually works. > > I think I will still need the unblocker for the tests. > I have tried out the approach that you suggested; > however it still seems to block on > ThreadedInputConnection#blockAndGetStateUpdate(). > > Additionally as I need to pass in replyToRequest = true in the public public > API call to > ThreadedInputConnection#updateStateOnUiThread to verify the feature > functionality that I have to test; > the ThreadedInputConnection#processPendingInputStates() routine invoked from > the updateStateOnUiThread API will empty the blocking queue before the call to > mConnection.getExtractedText(...); will in turn blocks as the TextInputState > Queue is of Size 0. > It should work once you call updateStateOnUiThread(replyToRequest=true). I just locally checked that it works. If you need debugging, you can do the following: import org.robolectric.shadows.ShadowLog; ... setUp() { ShadowLog.stream = System.out; ... } System.out.println("XXX"); https://codereview.chromium.org/2500663002/diff/160001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:255: mConnection.getExtractedText(request, InputConnection.GET_EXTRACTED_TEXT_MONITOR); You need to check the return value of this function, and then call another updateState with a slightly different value to verify the next state update. https://codereview.chromium.org/2500663002/diff/160001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:258: composition.start(), composition.end(), true, false); this should be true for the first call. https://codereview.chromium.org/2500663002/diff/160001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:296: mConnection.getExtractedText(request, 0); You need to check the return value. https://codereview.chromium.org/2500663002/diff/160001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:299: composition.start(), composition.end(), true, false); replyToRequest should be true and this method should be called before getExtractedText(). https://codereview.chromium.org/2500663002/diff/160001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:303: .updateExtractedText(anyInt(), any(ExtractedText.class)); I don't think you should check this. https://codereview.chromium.org/2500663002/diff/160001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:306: .updateSelection( I don't think you should check this. https://codereview.chromium.org/2500663002/diff/160001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:323: mConnection.getExtractedText(request, InputConnection.GET_EXTRACTED_TEXT_MONITOR); you can check the return value here. https://codereview.chromium.org/2500663002/diff/160001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:326: composition.start(), composition.end(), true, false); you need to call this twice, once before getExtractedText with replyToRequest=true, and then another after getExtractedText. The second value needs to be slightly different to be verified as a new state update.
Hi, Can you PTAL at the latest changes to the unit tests? Thanks bhallam
test looks good, but one more thing to be done. I'll leave the final approval to aelias@. https://codereview.chromium.org/2500663002/diff/180001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java (right): https://codereview.chromium.org/2500663002/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:106: mCurrentExtractedTextRequestToken = 0; mHandler.post(new Runnable() { @public void run() { mNumNestedBatchEdits = 0; mPendingAccent = 0; mCurrentExtractedTextRequestToken = 0; mShouldUpdateExtractedText = false; } });
Hi aelias, I have just addressed last the comment by changwan@. Can you PTAL at this patch. Thanks bhallam https://codereview.chromium.org/2500663002/diff/180001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java (right): https://codereview.chromium.org/2500663002/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:106: mCurrentExtractedTextRequestToken = 0; On 2017/05/09 02:52:18, Changwan Ryu wrote: > mHandler.post(new Runnable() { > @public > void run() { > mNumNestedBatchEdits = 0; > mPendingAccent = 0; > mCurrentExtractedTextRequestToken = 0; > mShouldUpdateExtractedText = false; > } > }); Done.
https://codereview.chromium.org/2500663002/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java (right): https://codereview.chromium.org/2500663002/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:203: final ExtractedText extractedText = convertToExtractedText(mCachedTextInputState); This shouldn't be using mCachedTextInputState, that's not thread-safe. Just use the textInputState that's passed in here as a parameter.
Hi aelias, I have updated the latest patch-set with the correct/threadsafe copy of TextInputState within org.chromium.content.browser.input.ThreadedInputConnection#updateSelection. Thanks! bhallam https://codereview.chromium.org/2500663002/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java (right): https://codereview.chromium.org/2500663002/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:203: final ExtractedText extractedText = convertToExtractedText(mCachedTextInputState); On 2017/05/09 21:51:06, aelias wrote: > This shouldn't be using mCachedTextInputState, that's not thread-safe. Just use > the textInputState that's passed in here as a parameter. Done.
lgtm, thanks!
The CQ bit was checked by aelias@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by aelias@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bhallam@amazon.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1494394374689490,
"parent_rev": "224f8c294d66cdb6af36a93ec677859c50dd46e2", "commit_rev":
"9b5ebac3230f3c90782296ae95396e7f512729bc"}
Message was sent while issue was closed.
Description was changed from ========== Relay Text Field Updates from ContentView to External InputMethodManager. This change surfaces up all ExtractedText updates from the content view back to the InputMethodManager. Is is applicable to allow consumers of the Android InputManager to receive text change events back. This change does not affect Standard Input Method Managers such as the Android Default Keyboard registered to the platform but specifically affects External Input Method apps like virtual floating keyboard implementations or screen keyboard overlay implementations. In addition this change is consistent with the guidelines given as part of https://developer.android.com/reference/android/view/inputmethod/InputConnect... States that if the GET_EXTRACTED_TEXT_MONITOR flag is set, you should be calling updateExtractedText(View, int, ExtractedText) whenever you call updateSelection(View, int, int, int, int). BUG=673819 TEST=New ContentShellJunitTests, Existing ContentShell Instrumentation Tests. ========== to ========== Relay Text Field Updates from ContentView to External InputMethodManager. This change surfaces up all ExtractedText updates from the content view back to the InputMethodManager. Is is applicable to allow consumers of the Android InputManager to receive text change events back. This change does not affect Standard Input Method Managers such as the Android Default Keyboard registered to the platform but specifically affects External Input Method apps like virtual floating keyboard implementations or screen keyboard overlay implementations. In addition this change is consistent with the guidelines given as part of https://developer.android.com/reference/android/view/inputmethod/InputConnect... States that if the GET_EXTRACTED_TEXT_MONITOR flag is set, you should be calling updateExtractedText(View, int, ExtractedText) whenever you call updateSelection(View, int, int, int, int). BUG=673819 TEST=New ContentShellJunitTests, Existing ContentShell Instrumentation Tests. Review-Url: https://codereview.chromium.org/2500663002 Cr-Commit-Position: refs/heads/master@{#470488} Committed: https://chromium.googlesource.com/chromium/src/+/9b5ebac3230f3c90782296ae9539... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/9b5ebac3230f3c90782296ae9539... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
