Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2912473002:
Fix an IME bug where RWHImpl::ImeFinishCompositingText is called twice in a row (Closed)
DescriptionFix an IME bug where RWHImpl::ImeFinishCompositingText is called twice in a row
When a mouse click is received during an on going IME composition, the
RenderWidgetHostViewEventHandler finishes the composition and also
cancels the composition at the InputMethod side. This is done through
calling ImeFinishComposingText on the focused widget and then notifying
the input method.
This however undermines the fact that
RenderWidgetHostViewAura/TextInputClient keeps track of different IME
states internally, such as |has_composition_text_|. Therefore, the
correct practice should be to route all IME events through
TextInputClient instead (as opposed to sending the IPC directly).
In the case of this bug, calling
RenderWidgetHostImpl::ImeFinishComposingText will not reset the
|has_composition_text_| flag inside RWHVAura and hecne, later calling
ImeCancelComposition (to cancel the composition on the InputMethod side)
will lead to yet another IPC for ImeFinishComposingText.
BUG=723024
Review-Url: https://codereview.chromium.org/2912473002
Cr-Commit-Position: refs/heads/master@{#475908}
Committed: https://chromium.googlesource.com/chromium/src/+/428a2f9eb3e0e72bac3ef401fe7e1a13e2009e75
Patch Set 1 #Patch Set 2 : Modified the Test #
Total comments: 3
Patch Set 3 : Change EXPECT_EQ => ASSERT_EQ #
Messages
Total messages: 23 (17 generated)
The CQ bit was checked by ekaramad@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 checked by ekaramad@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 checked by ekaramad@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...
ekaramad@chromium.org changed reviewers: + sadrul@chromium.org, shuchen@chromium.org
PTAL. sadrul@: reviewer of the original patch adding the test shuchen@: originally add the logic and the test (https://codereview.chromium.org/614553002).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm But please wait for shuchen@'s approval too. https://codereview.chromium.org/2912473002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/2912473002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1584: EXPECT_EQ(InputMsg_ImeFinishComposingText::ID, ASSERT that sink_->message_count() >= 2 before accessing GetMessageAt(0)/1 to avoid crashes on failure.
lgtm https://codereview.chromium.org/2912473002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/2912473002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1584: EXPECT_EQ(InputMsg_ImeFinishComposingText::ID, On 2017/05/30 21:03:03, sadrul wrote: > ASSERT that sink_->message_count() >= 2 before accessing GetMessageAt(0)/1 to > avoid crashes on failure. +1. Can we assert message_count == 2? Or can we check no dup ImeFinishComposingText?
The CQ bit was checked by ekaramad@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...
Thanks for the reviews! I will CQ soon. https://codereview.chromium.org/2912473002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/2912473002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1584: EXPECT_EQ(InputMsg_ImeFinishComposingText::ID, On 2017/05/31 01:16:36, Shu Chen wrote: > On 2017/05/30 21:03:03, sadrul wrote: > > ASSERT that sink_->message_count() >= 2 before accessing GetMessageAt(0)/1 to > > avoid crashes on failure. > > +1. Can we assert message_count == 2? Or can we check no dup > ImeFinishComposingText? Thanks! Changed the EXPECT on line 1581 into ASSERT.
The CQ bit was unchecked by ekaramad@chromium.org
The CQ bit was checked by ekaramad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shuchen@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2912473002/#ps40001 (title: "Change EXPECT_EQ => ASSERT_EQ")
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": 40001, "attempt_start_ts": 1496239929609110,
"parent_rev": "317fdbe13422752c4a2ba6eb44027ae3fb2bd1e5", "commit_rev":
"428a2f9eb3e0e72bac3ef401fe7e1a13e2009e75"}
Message was sent while issue was closed.
Description was changed from ========== Fix an IME bug where RWHImpl::ImeFinishCompositingText is called twice in a row When a mouse click is received during an on going IME composition, the RenderWidgetHostViewEventHandler finishes the composition and also cancels the composition at the InputMethod side. This is done through calling ImeFinishComposingText on the focused widget and then notifying the input method. This however undermines the fact that RenderWidgetHostViewAura/TextInputClient keeps track of different IME states internally, such as |has_composition_text_|. Therefore, the correct practice should be to route all IME events through TextInputClient instead (as opposed to sending the IPC directly). In the case of this bug, calling RenderWidgetHostImpl::ImeFinishComposingText will not reset the |has_composition_text_| flag inside RWHVAura and hecne, later calling ImeCancelComposition (to cancel the composition on the InputMethod side) will lead to yet another IPC for ImeFinishComposingText. BUG=7230724 ========== to ========== Fix an IME bug where RWHImpl::ImeFinishCompositingText is called twice in a row When a mouse click is received during an on going IME composition, the RenderWidgetHostViewEventHandler finishes the composition and also cancels the composition at the InputMethod side. This is done through calling ImeFinishComposingText on the focused widget and then notifying the input method. This however undermines the fact that RenderWidgetHostViewAura/TextInputClient keeps track of different IME states internally, such as |has_composition_text_|. Therefore, the correct practice should be to route all IME events through TextInputClient instead (as opposed to sending the IPC directly). In the case of this bug, calling RenderWidgetHostImpl::ImeFinishComposingText will not reset the |has_composition_text_| flag inside RWHVAura and hecne, later calling ImeCancelComposition (to cancel the composition on the InputMethod side) will lead to yet another IPC for ImeFinishComposingText. BUG=7230724 Review-Url: https://codereview.chromium.org/2912473002 Cr-Commit-Position: refs/heads/master@{#475908} Committed: https://chromium.googlesource.com/chromium/src/+/428a2f9eb3e0e72bac3ef401fe7e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/428a2f9eb3e0e72bac3ef401fe7e...
Message was sent while issue was closed.
Description was changed from ========== Fix an IME bug where RWHImpl::ImeFinishCompositingText is called twice in a row When a mouse click is received during an on going IME composition, the RenderWidgetHostViewEventHandler finishes the composition and also cancels the composition at the InputMethod side. This is done through calling ImeFinishComposingText on the focused widget and then notifying the input method. This however undermines the fact that RenderWidgetHostViewAura/TextInputClient keeps track of different IME states internally, such as |has_composition_text_|. Therefore, the correct practice should be to route all IME events through TextInputClient instead (as opposed to sending the IPC directly). In the case of this bug, calling RenderWidgetHostImpl::ImeFinishComposingText will not reset the |has_composition_text_| flag inside RWHVAura and hecne, later calling ImeCancelComposition (to cancel the composition on the InputMethod side) will lead to yet another IPC for ImeFinishComposingText. BUG=7230724 Review-Url: https://codereview.chromium.org/2912473002 Cr-Commit-Position: refs/heads/master@{#475908} Committed: https://chromium.googlesource.com/chromium/src/+/428a2f9eb3e0e72bac3ef401fe7e... ========== to ========== Fix an IME bug where RWHImpl::ImeFinishCompositingText is called twice in a row When a mouse click is received during an on going IME composition, the RenderWidgetHostViewEventHandler finishes the composition and also cancels the composition at the InputMethod side. This is done through calling ImeFinishComposingText on the focused widget and then notifying the input method. This however undermines the fact that RenderWidgetHostViewAura/TextInputClient keeps track of different IME states internally, such as |has_composition_text_|. Therefore, the correct practice should be to route all IME events through TextInputClient instead (as opposed to sending the IPC directly). In the case of this bug, calling RenderWidgetHostImpl::ImeFinishComposingText will not reset the |has_composition_text_| flag inside RWHVAura and hecne, later calling ImeCancelComposition (to cancel the composition on the InputMethod side) will lead to yet another IPC for ImeFinishComposingText. BUG=723024 Review-Url: https://codereview.chromium.org/2912473002 Cr-Commit-Position: refs/heads/master@{#475908} Committed: https://chromium.googlesource.com/chromium/src/+/428a2f9eb3e0e72bac3ef401fe7e... ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
