Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2743933003:
Fix TextInputManager for Android (Closed)
|
Created:
3 years, 9 months ago by Changwan Ryu Modified:
3 years, 9 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, nona+watch_chromium.org, shuchen+watch_chromium.org, James Su, yusukes+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix TextInputManager for Android
In crrev.com/2354793003, we changed state updates to go through
TextInputManager, but TextInputManager does not propagate state update
when the current and last state was NONE. This can cause IME thread to
hang because it waits for the request to be replied.
BUG=699699
Review-Url: https://codereview.chromium.org/2743933003
Cr-Commit-Position: refs/heads/master@{#456116}
Committed: https://chromium.googlesource.com/chromium/src/+/38c1f9493be8263b28d0f635d240b547efd75e09
Patch Set 1 #
Total comments: 3
Messages
Total messages: 19 (10 generated)
changwan@chromium.org changed reviewers: + aelias@chromium.org, ekaramad@chromium.org
The CQ bit was checked by changwan@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.
Thanks! I posted some thoughts but generally looks good. This is something I have to revisit soon and fix for OOPIFs as well. https://codereview.chromium.org/2743933003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/2743933003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/text_input_manager.cc:108: return; This should be good since we will not have more than one RWHV on a page to overwrite each other's state to begin with. And as I understand we were throwing away some state updates from renderer when the type was NONE, which seems incorrect. (If everything is working fine for normal Android then ignore the rest of this comment pls). But wouldn't we also have to change the code inside RenderWidgetHostViewAndroid::OnUpdateTextInputStateCalled() a bit? Right now the state it is reporting to content_view_core_ is the one from active widget, which is the one with TextInputState.type != NONE. Don't we need to sometimes update the ime_adapter_ with states where the type is NONE? If that is the case then I suggest changing the signature of GetTextInputState to pass an optional parameter for view and get the state from the view, e.g.: void TextInputState::GetTextInputState(RenderWidgetHostViewBase* view = nullptr) { if (view == nullptr) view = active_view_; return view ? text_input_state_map_[view] : nullptr; } And inside RenderWidgetHostViewAndroid::OnUpdateTextInputStateCalled do: const TextInputState* state = text_input_manager_->IsRegisteredView(view) ? text_input_manager_->GetTextInputState(view) : nullptr. The last line is to make sure we are not in destruction (view is still registered) because when a RWHV is destroyed the TextInputManager does not hold its state anymore (not doing this causes crash https:://crbug.com/669375). I also need to rework this anyway and base it on focused RenderWidgetHost since this currently might break things for OOPIF Android (https://crbug.com/631798).
https://codereview.chromium.org/2743933003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/2743933003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/text_input_manager.cc:108: return; On 2017/03/10 15:43:16, EhsanK wrote: > This should be good since we will not have more than one RWHV on a page to > overwrite each other's state to begin with. And as I understand we were throwing > away some state updates from renderer when the type was NONE, which seems > incorrect. > > (If everything is working fine for normal Android then ignore the rest of this > comment pls). > > But wouldn't we also have to change the code inside > RenderWidgetHostViewAndroid::OnUpdateTextInputStateCalled() a bit? > Right now the state it is reporting to content_view_core_ is the one from active > widget, which is the one with TextInputState.type != NONE. Don't we need to > sometimes update the ime_adapter_ with states where the type is NONE? > > If that is the case then I suggest changing the signature of GetTextInputState > to pass an optional parameter for view and get the state from the view, e.g.: > > void TextInputState::GetTextInputState(RenderWidgetHostViewBase* view = nullptr) > { > if (view == nullptr) view = active_view_; > return view ? text_input_state_map_[view] : nullptr; > } > > And inside RenderWidgetHostViewAndroid::OnUpdateTextInputStateCalled do: > > const TextInputState* state = text_input_manager_->IsRegisteredView(view) ? > text_input_manager_->GetTextInputState(view) : nullptr. > > The last line is to make sure we are not in destruction (view is still > registered) because when a RWHV is destroyed the TextInputManager does not hold > its state anymore (not doing this causes crash https:://crbug.com/669375). > > I also need to rework this anyway and base it on focused RenderWidgetHost since > this currently might break things for OOPIF Android (https://crbug.com/631798). As long as it returns something, it should work correctly.
Thanks. LGTM. https://codereview.chromium.org/2743933003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/2743933003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/text_input_manager.cc:108: return; On 2017/03/10 16:01:38, Changwan Ryu wrote: > On 2017/03/10 15:43:16, EhsanK wrote: > > This should be good since we will not have more than one RWHV on a page to > > overwrite each other's state to begin with. And as I understand we were > throwing > > away some state updates from renderer when the type was NONE, which seems > > incorrect. > > > > (If everything is working fine for normal Android then ignore the rest of this > > comment pls). > > > > But wouldn't we also have to change the code inside > > RenderWidgetHostViewAndroid::OnUpdateTextInputStateCalled() a bit? > > Right now the state it is reporting to content_view_core_ is the one from > active > > widget, which is the one with TextInputState.type != NONE. Don't we need to > > sometimes update the ime_adapter_ with states where the type is NONE? > > > > If that is the case then I suggest changing the signature of GetTextInputState > > to pass an optional parameter for view and get the state from the view, e.g.: > > > > void TextInputState::GetTextInputState(RenderWidgetHostViewBase* view = > nullptr) > > { > > if (view == nullptr) view = active_view_; > > return view ? text_input_state_map_[view] : nullptr; > > } > > > > And inside RenderWidgetHostViewAndroid::OnUpdateTextInputStateCalled do: > > > > const TextInputState* state = text_input_manager_->IsRegisteredView(view) ? > > text_input_manager_->GetTextInputState(view) : nullptr. > > > > The last line is to make sure we are not in destruction (view is still > > registered) because when a RWHV is destroyed the TextInputManager does not > hold > > its state anymore (not doing this causes crash https:://crbug.com/669375). > > > > I also need to rework this anyway and base it on focused RenderWidgetHost > since > > this currently might break things for OOPIF Android > (https://crbug.com/631798). > > As long as it returns something, it should work correctly. Right. So here: https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid... if active_view_= nullptr (which is the case in the beginning or might be set at line 142 here), we will be updating content view core with TextInputState() default and seems to be OK. Then I think this is fine. Also I will soon upload a patch to possibly change this whole method to use the focused widget. I don't think relying on just TextInputState.type is good enough (not for android). I will add you as reviewer to that CL.
The CQ bit was checked by changwan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by aelias@chromium.org
lgtm
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": 1, "attempt_start_ts": 1489171385576220, "parent_rev":
"d81b759bf2a2fc62238efe3f01eb5faa74f2d374", "commit_rev":
"38c1f9493be8263b28d0f635d240b547efd75e09"}
Message was sent while issue was closed.
Description was changed from ========== Fix TextInputManager for Android In crrev.com/2354793003, we changed state updates to go through TextInputManager, but TextInputManager does not propagate state update when the current and last state was NONE. This can cause IME thread to hang because it waits for the request to be replied. BUG=699699 ========== to ========== Fix TextInputManager for Android In crrev.com/2354793003, we changed state updates to go through TextInputManager, but TextInputManager does not propagate state update when the current and last state was NONE. This can cause IME thread to hang because it waits for the request to be replied. BUG=699699 Review-Url: https://codereview.chromium.org/2743933003 Cr-Commit-Position: refs/heads/master@{#456116} Committed: https://chromium.googlesource.com/chromium/src/+/38c1f9493be8263b28d0f635d240... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/38c1f9493be8263b28d0f635d240... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
